[U-Boot] [PATCH 6/6] i2c: designware_i2c: Add support for PCI(e) based I2C cores (x86)

[Stefan Roese]

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/

Thanks,
Stefan