From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::236; helo=mail-it0-x236.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x236.google.com (mail-it0-x236.google.com [IPv6:2607:f8b0:4001:c0b::236]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C390E2277AF30 for ; Mon, 23 Apr 2018 08:53:20 -0700 (PDT) Received: by mail-it0-x236.google.com with SMTP id e20-v6so11295214itc.1 for ; Mon, 23 Apr 2018 08:53:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=yYtRfEYLiKQmAu5m0poZC+373LCv/DSMeErrAtGeyCI=; b=AF0cAf/zvGnb7mKBCFOFcj39nq6lS/FRaq4oDBafbQuq6MBTZ52SR//qFA3doD8DSZ djoA8DD2nCbsGth19Kl3RFJXSFmPrlf5gy7pS60WjJeELxHL/GlRMy6txR6LqfRoyQUp TAKyaU0sTLaexx6KsUVnzX2PeJD/ssyx9anCY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=yYtRfEYLiKQmAu5m0poZC+373LCv/DSMeErrAtGeyCI=; b=jN3PWGDqpNFIwiAbmyr1+1uMyLwvrksjw7NKCNG4JLUGLlUs/fdqQL+xlSTpM5wIJs 3Au3bEgBXjj+g0Udh22dOlUxMGj994zP9qzcDTm6bXlCbDRNeCZISU7AL+rxy+n7l+O0 IPRGnurVpPYWOUqu0InSa/7nBCgvDiqxhMpkaOovUNDHNfYcYb5sGBDANTOt3Xuevnwg 0v0dCN2gGdXqwlpmShRqUbLbfOht6/L8MA1/eEkBhyqN1V2s1PyeSXY7NvZ6yzgaB12T P880U3l3+ybi5ACgtkglKNWNwnqCSQWLJ9iPOhtLHkk4/ESYV4NYYrb4/5kHC748s6rI 4PbQ== X-Gm-Message-State: ALQs6tAUj9od9wdYnxX7K1JoNGkdrENA8sutmS1opLBy5sngpYKHfGiT NVMlU94rqlBgvHhTmVHt7vxHtWoe4CgwRZWCA57Y7dhc X-Google-Smtp-Source: AB8JxZqi4VAcBi9P0fVF2vuqRmWtPQbhm58939BIWZtbrHn9zzouPL6nyifwmdvIfeWZ8ofvoJXDTXRSdJtuCrtahuk= X-Received: by 2002:a24:67d7:: with SMTP id u206-v6mr14759577itc.138.1524498799461; Mon, 23 Apr 2018 08:53:19 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.187.67 with HTTP; Mon, 23 Apr 2018 08:53:19 -0700 (PDT) In-Reply-To: References: <1518771035-6733-1-git-send-email-meenakshi.aggarwal@nxp.com> <1518771035-6733-6-git-send-email-meenakshi.aggarwal@nxp.com> <20180417163657.rtmpklak5nqlwn3c@bivouac.eciton.net> <20180423083815.wmdb2lu3gqyrmruw@bivouac.eciton.net> From: Ard Biesheuvel Date: Mon, 23 Apr 2018 17:53:19 +0200 Message-ID: To: Meenakshi Aggarwal Cc: Leif Lindholm , "edk2-devel@lists.01.org" , Udit Kumar , Varun Sethi Subject: Re: [PATCH edk2-platforms 05/39] Silicon/NXP: Add support for I2c driver X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Apr 2018 15:53:21 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On 23 April 2018 at 17:50, Meenakshi Aggarwal wrote: > Hi Ard > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> Sent: Monday, April 23, 2018 7:10 PM >> To: Meenakshi Aggarwal >> Cc: Leif Lindholm ; edk2-devel@lists.01.org; U= dit >> Kumar ; Varun Sethi >> Subject: Re: [PATCH edk2-platforms 05/39] Silicon/NXP: Add support for I= 2c >> driver >> >> On 23 April 2018 at 12:34, Meenakshi Aggarwal >> wrote: >> > Hi Leif >> > >> >> -----Original Message----- >> >> From: Leif Lindholm [mailto:leif.lindholm@linaro.org] >> >> Sent: Monday, April 23, 2018 2:08 PM >> >> To: Meenakshi Aggarwal >> >> Cc: ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Udit Kumar >> >> ; Varun Sethi >> >> Subject: Re: [PATCH edk2-platforms 05/39] Silicon/NXP: Add support fo= r >> I2c >> >> driver >> >> >> >> On Mon, Apr 23, 2018 at 08:21:22AM +0000, Meenakshi Aggarwal wrote: >> >> > > > +/** >> >> > > > + Function to read data using i2c bus >> >> > > > + >> >> > > > + @param I2cBus I2c Controller number >> >> > > > + @param Chip Address of slave device from where = data to >> be >> >> read >> >> > > > + @param Offset Offset of slave memory >> >> > > > + @param Alen Address length of slave >> >> > > > + @param Buffer A pointer to the destination buffer= for the >> data >> >> > > > + @param Len Length of data to be read >> >> > > > + >> >> > > > + @retval EFI_NOT_READY Arbitration lost >> >> > > > + @retval EFI_TIMEOUT Failed to initialize data transfer = in >> >> predefined >> >> > > time >> >> > > > + @retval EFI_NOT_FOUND ACK was not recieved >> >> > > > + @retval EFI_SUCCESS Read was successful >> >> > > > + >> >> > > > +**/ >> >> > > > +STATIC >> >> > > > +EFI_STATUS >> >> > > > +I2cDataRead ( >> >> > > > + IN UINT32 I2cBus, >> >> > > > + IN UINT8 Chip, >> >> > > > + IN UINT32 Offset, >> >> > > > + IN UINT32 Alen, >> >> > > > + IN UINT8 *Buffer, >> >> > > > + IN UINT32 Len >> >> > > > + ) >> >> > > > +{ >> >> > > > + EFI_STATUS Status; >> >> > > > + UINT32 Temp; >> >> > > > + INT32 I; >> >> > > > + I2C_REGS *I2cRegs; >> >> > > > + >> >> > > > + I2cRegs =3D (I2C_REGS *)(FixedPcdGet64 (PcdI2c0BaseAddr + >> >> > > > + (I2cBus * FixedPcdGet32 (PcdI2cSize))= )); >> >> > > >> >> > > Please get rid of this hardcoded base address and use >> NonDiscoverable >> >> > > Have a look at Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/ a= nd >> >> > > Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/ >> >> > > for example. >> >> > > >> >> > I have checked SynQuacer code and i dont see its adding much >> advantage. >> >> >> >> What it gives is the ability to cover more than one controller by thi= s >> >> driver, regardless of whether you need it for this particular platfor= m >> >> port or not. >> >> >> > Current board needs one controller. >> >> It is not about what the board wants. It is about reusing code. >> > Ideally, we should reuse as much as possible, > >> If you implemented a driver using the UEFI driver model, you get all >> the binding machinery for free, and you only need to declare the >> existence of a controller and the core will bind and discover drivers. > > But in case of this particular driver, even if we are using UEFI driver m= odel. > Then from code perspective more or less, we need same code to declare i2c= master > Protocol. > >> This also allows you to unbind a driver from a single controller, for >> instance, and keeping the other connections alive. >> > > > I don't see any case in which we need to unbind driver for i2c. > > Coming back to implementation part, there are some > advantages using UEFI driver model. At present we > are not using those features for i2c driver but if you think we should ha= ve these in code. > We will add this support. > >> > In case of multiple controller, we can use a loop to install multiple = protocols >> and >> > If needed then our preference would be to use I2c enumeration protocol= . >> > This will allow to use correct controller for connected devices. >> > >> >> I2C enumeration protocol has nothing to do with this. >> >> > With sample implementation of SynQuacer code, >> > Please advise, how a right controller is being connected to driver >> > e.g. we are registering two controllers mI2c0Desc and mI2c1Desc and >> > both are exporting same protocols. >> > Its user, RTC lib just checking gEfiI2cMasterProtocolGuid. There is >> possibility >> > to connect with any of the controller. I dont see in code where it is = assuring >> connection with right controller. >> > And how we can assure we are connecting to the correct controller. >> > >> >> The PCF8563 RTC driver has a special GUIDed protocol that identifies >> the I2C controller that has the RTC connected to it. This goes outside >> of the UDM because RTC is an architectural protocol. Note that the >> SynQuacer I2C driver is a DXE_RUNTIME_DRIVER module that supports boot >> time and runtime I2C support for controllers, the latter especially >> for things like RTC and varstore support over I2C. >> > > I don=E2=80=99t say much on this because this is implementation , which c= an vary mind to mind. > But in order to maintain many controllers > and devices on the top of it, my preference would be to > go with bus configuration and bus enumeration protocol. > > You can say, why to complicate the code, when this can be done > in easy way. > You are correct: the I2C enumeration and platform protocols should be used in all cases *except* when we are dealing with a I2C device that is used to produce an architectural protocol that is exposed to the OS as a runtime service. This is why the PCF8563 invokes the I2C master protocol directly rather than the I2C I/O protocol.