[U-Boot] [PATCH 6/6] i2c: designware_i2c: Add support for PCI(e) based I2C cores (x86)
Stefan Roese
sr at denx.de
Mon Apr 11 17:03:25 CEST 2016
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/
Thanks,
Stefan
More information about the U-Boot
mailing list