[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 16:40:13 CEST 2016


Hi Stefan,

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?

Regards,
Simon


More information about the U-Boot mailing list