From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Mon, 11 Apr 2016 17:03:25 +0200 Subject: [U-Boot] [PATCH 6/6] i2c: designware_i2c: Add support for PCI(e) based I2C cores (x86) In-Reply-To: <57027FD8.9010107@denx.de> References: <1458287661-21745-1-git-send-email-sr@denx.de> <1458287661-21745-6-git-send-email-sr@denx.de> <56EFB8D2.5030306@denx.de> <56EFE346.4030008@denx.de> <56EFFF68.6070205@denx.de> <57027FD8.9010107@denx.de> Message-ID: <570BBCBD.3060709@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon, On 04.04.2016 16:53, Stefan Roese wrote: > Hi Simon, > > as you seem to be back from vacation (?), we (Bin and myself) would > like to hear your expert comment on a x86 issue I've discovered > while porting the Designware I2C driver to x86. Please see below: > > On 28.03.2016 08:01, Bin Meng wrote: >> Hi Stefan, >> >> On Mon, Mar 21, 2016 at 10:04 PM, Stefan Roese wrote: >>> Hi Bin, >>> >>> On 21.03.2016 13:43, Bin Meng wrote: >>>> On Mon, Mar 21, 2016 at 8:04 PM, Stefan Roese wrote: >>>>> Hi Bin, >>>>> >>>>> On 21.03.2016 10:03, Stefan Roese wrote: >>>>> >>>>> >>>>> >>>>>>>> static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr, >>>>>>>> @@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) >>>>>>>> { >>>>>>>> struct dw_i2c *priv = dev_get_priv(bus); >>>>>>>> >>>>>>>> +#ifdef CONFIG_X86 >>>>>>>> + /* Save base address from PCI BAR */ >>>>>>>> + priv->regs = (struct i2c_regs *) >>>>>>>> + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); >>>>>>>> + /* Use BayTrail specific timing values */ >>>>>>>> + priv->scl_sda_cfg = &byt_config; >>>>>>>> +#else >>>>>>> >>>>>>> How about: >>>>>>> >>>>>>> if (device_is_on_pci_bus(dev)) { >>>>>>> do the PCI I2C stuff here; >>>>>>> } >>>>>> >>>>>> I've tried this but it generated compilation errors on socfpga, as the >>>>>> dm_pci_xxx functions are not available there. So it definitely needs >>>>>> some #ifdef here. I could go with your suggestion and use >>>>>> #if CONFIG_DM_PCI as well. >>>>>> >>>>>>> See driver/net/designware.c for example. >>>>>>> >>>>>>>> /* Save base address from device-tree */ >>>>>>>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >>>>>>>> +#endif >>>>> >>>>> Enabling this code for x86 via if (device_is_on_pci_bus(dev)) results >>>>> in this ugly compilation warning: >>>>> >>>>> drivers/i2c/designware_i2c.c: In function ?designware_i2c_probe?: >>>>> drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] >>>>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >>>>> ^ >>>>> >>>>> This is because x86 defines fdt_addr_t / phys_addr_t as 64bit. So >>>>> I'm wondering, how dev_get_addr() should get used on x86. Has it >>>>> been used anywhere here at all? Should we perhaps go back to >>>>> a 32bit phy_addr representation again? So that dev_get_addr() >>>>> matches the (void *) size again? >>>>> >>>> >>>> dev_get_addr() is being used on x86 drivers. See >>>> ns16550_serial_ofdata_to_platdata() for example. There is no build >>>> warning for the ns16550 driver. >>> >>> Looking closer, the warning does not occur here, since the registers >>> are stored in a u32 variable "base". And assigning a 64bit value to a >>> 32bit variable as in "plat->base = addr" in ns16550.c does not cause any >>> warnings. >>> >>> Here in the I2C driver though, the base address is stored as a pointer >>> (pointer size is 32 bit for x86). And this triggers this warning, even >>> though its effectively the same assignment. I could cast to u32 but this >>> would cause problems on 64 bit architectures using this driver (in the >>> future). So I came up with this approach: >> >> Thanks for digging out these. >> >>> >>> /* >>> * On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. So assigning the >>> * register base directly in dev_get_addr() results in this compilation warning: >>> * warning: cast to pointer from integer of different size >>> * >>> * Using this macro POINTER_SIZE_CAST, allows us to cast the result of >>> * dev_get_addr() into a 32bit value before casting it to the pointer >>> * (struct i2c_regs *). >>> */ >>> #ifdef CONFIG_X86 >>> #define POINTER_SIZE_CAST u32 >>> #endif >>> >>> ... >>> >>> static int designware_i2c_probe(struct udevice *bus) >>> { >>> struct dw_i2c *priv = dev_get_priv(bus); >>> >>> if (device_is_on_pci_bus(bus)) { >>> #ifdef CONFIG_DM_PCI >>> /* Save base address from PCI BAR */ >>> priv->regs = (struct i2c_regs *) >>> dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); >>> #ifdef CONFIG_X86 >>> /* Use BayTrail specific timing values */ >>> priv->scl_sda_cfg = &byt_config; >>> #endif >>> #endif >>> } else { >>> /* Save base address from device-tree */ >>> priv->regs = (struct i2c_regs *)(POINTER_SIZE_CAST)dev_get_addr(bus); >>> } >>> >>> But I'm not 100% happy with this approach. >>> >> >> Yes, it's annoying. >> >>> So what are the alternatives: >>> >>> a) Don't compile the dev_get_addr() part for x86 similar to what I've >>> done in v1 >>> >>> b) This approach with POINTER_SIZE_CAST >>> >>> Any preferences of other ideas? >>> >>> Side note: My general feeling is, that dev_get_addr() should be able to >>> get cast into a pointer on all platforms. This is how it is used in many >>> drivers, btw. Since this is not possible on x86, we might have a problem >>> here. Simon might have some ideas on this as well... >>> >> >> I would like to hear Simon's input. Simon? > > Yes, Simon, what do you think? > > Please also see my v2 of this patch which uses (__UINTPTR_TYPE__) > for the cast: > > https://patchwork.ozlabs.org/patch/601113/ Simon, could you please take a quick look at this patch? With the general problem of dev_get_addr() on x86 (as described above). Do you have some other suggestions to solve this? Or is the solution in v2 which uses (__UINTPTR_TYPE__) acceptable? https://patchwork.ozlabs.org/patch/601113/ Thanks, Stefan