Hi Stefan, On 20 April 2016 at 08:58, Stefan Roese <[email protected]> wrote: > > Hi Simon. > > On 20.04.2016 16:40, Simon Glass wrote: > > > On 11 April 2016 at 09:03, Stefan Roese <[email protected]> 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 <[email protected]> wrote: > >>>>> > >>>>> Hi Bin, > >>>>> > >>>>> On 21.03.2016 13:43, Bin Meng wrote: > >>>>>> > >>>>>> On Mon, Mar 21, 2016 at 8:04 PM, Stefan Roese <[email protected]> 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? > > Yes, it does. In a somewhat less ugly way. This is my current result: > > } else { > ulong base; > > /* Save base address from device-tree */ > > /* > * 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 an intermediate "ulong" variable before assigning > * this pointer to the "regs" variable solves this issue. > */ > base = dev_get_addr(bus); > priv->regs = (struct i2c_regs *)base; > } > > If you think this is acceptable, I'll send a new patch version to > the list.
Seems fine to me. Perhaps we should have dev_get_addr_ptr() to do this for us? Regards, Simon _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
