Hi Simon,
On 20.04.2016 17:09, Simon Glass wrote:
Hi Stefan,
On 20 April 2016 at 08:58, Stefan Roese sr@denx.de wrote:
Hi Simon.
On 20.04.2016 16:40, Simon Glass wrote:
On 11 April 2016 at 09:03, Stefan Roese sr@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 sr@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 sr@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:
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?
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?
Might make sense. I can generate a small patch for this.
Perhaps we should better use "uintptr_t" as type for the intermediate variable instead. But then we can effectively drop the intermediate variable and do the casting directly.
What do you think?
Thanks, Stefan