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::232; helo=mail-it0-x232.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x232.google.com (mail-it0-x232.google.com [IPv6:2607:f8b0:4001:c0b::232]) (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 0E4692277AF1A for ; Mon, 23 Apr 2018 06:39:55 -0700 (PDT) Received: by mail-it0-x232.google.com with SMTP id p3-v6so10470178itc.0 for ; Mon, 23 Apr 2018 06:39:55 -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; bh=pwCRKLwUFpt7kbcTcJ1qSRqJGXKg5PF9ZubCOIjvK6U=; b=em7S+ztymT7DbOsOfvdV6KDiQhWWDsFA/cXjbiFsRn/P4SxC65kaGYEff523RhxFRS htCBagkjBZlNngHYx2aQaV0Z6qkNEhxtUUVz/Pg3NaavPPrBxIEN/Sr5yJX53Io4pCcJ vDiWxTFDcgM3r7kOEYzG/ROXz4oPs9FoDKVLI= 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; bh=pwCRKLwUFpt7kbcTcJ1qSRqJGXKg5PF9ZubCOIjvK6U=; b=B90ILGzfRJpxTMmibFBXu/si6NXhgLBXdc/5h+w0vvnwJ542FC27BGqAnaLmiqSWzV CzPL/Ve2/Fb4r6bFKh539j9RmzWMzepcbdnLEgmuiEcZfHel4tn3I0Bu4fN3GbXEmI8E vSkbiGfyLTWHho+QwH08UStjsrsBBe+kkHyx2M9XmnDbmkmuGLpWMVI/oO+m67tQUgwc XIlmCEF6sQDSrhmUU0IAaettKXfiO0toPhDafMl42Ak3mV1NOTtsQTy5WMKXzbalAPDl 6u7x/N6Rl20ZeYdQF/URyFdmEYjAHzmZRAYcQQkVjUH1p3Vy+4HWrD7FMAIBW0G5K4vT RxQg== X-Gm-Message-State: ALQs6tCfBB4RHdY/TJDI/BfYvHU6DPvOH7qf54E4PGdgvdyRUzFgfh0i wIEyXxQusifgoEmHP5I7tY7yKmeyPmE0GLEf0qy+B9He X-Google-Smtp-Source: AIpwx4+8qfY282kQAGZ3pzM/1VSlnNXY1HIUbdxv8186CLtGg0fmwEkO6dq1Ii1qV1QRfMxgXYzbTzBSb/WcS20fvLE= X-Received: by 2002:a24:b14d:: with SMTP id c13-v6mr14130786itj.106.1524490794735; Mon, 23 Apr 2018 06:39:54 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.187.67 with HTTP; Mon, 23 Apr 2018 06:39:54 -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 15:39:54 +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 13:39:56 -0000 Content-Type: text/plain; charset="UTF-8" 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 for 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 = (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/ and >> > > 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 this >> driver, regardless of whether you need it for this particular platform >> port or not. >> > Current board needs one controller. It is not about what the board wants. It is about reusing code. 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. This also allows you to unbind a driver from a single controller, for instance, and keeping the other connections alive. > 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. >> > There also base address is hard coded SYNQUACER_I2C1_BASE in >> > PlatformDxe driver. >> >> Yes, the PlatformDxe driver statically instantiates dynamic drivers >> for non-discoverable buses. But there is no longer any hard-coded >> addresses in the device driver itself. >> >> / >> Leif