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?
https://patchwork.ozlabs.org/patch/601113/
Thanks, Stefan