[U-Boot] [PATCH 6/6] i2c: designware_i2c: Add support for PCI(e) based I2C cores (x86)
Simon Glass
sjg at chromium.org
Wed Apr 20 17:09:34 CEST 2016
Hi Stefan,
On 20 April 2016 at 08:58, Stefan Roese <sr at denx.de> wrote:
>
> Hi Simon.
>
> On 20.04.2016 16:40, Simon Glass wrote:
>
> > On 11 April 2016 at 09:03, Stefan Roese <sr at 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 at 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 at 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?
>
> 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
More information about the U-Boot
mailing list