Hi Stefan, On 11 April 2016 at 09:03, Stefan Roese <s...@denx.de> wrote: > 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 <s...@denx.de> wrote: >>>> >>>> Hi Bin, >>>> >>>> On 21.03.2016 13:43, Bin Meng wrote: >>>>> >>>>> On Mon, Mar 21, 2016 at 8:04 PM, Stefan Roese <s...@denx.de> wrote: >>>>>> >>>>>> Hi Bin, >>>>>> >>>>>> On 21.03.2016 10:03, Stefan Roese wrote: >>>>>> >>>>>> <snip> >>>>>> >>>>>>>>> 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/
I feel that you should store the return value from dev_get_addr() in an fdt_addr_t or a ulong. Then it can be cast to a pointer as you wish. Platform data should hold the ulong, and private data (dev_get_priv()) should hold the pointer. I'm not keen on the POINTER_SIZE_CAST idea. Does that fix the problem? Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot