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

[Bin Meng]

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?

Regards,
Bin