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

[Stefan Roese <sr@denx.de>]

Hi Simon.

On 20.04.2016 16:40, Simon Glass wrote:

> On 11 April 2016 at 09:03, Stefan Roese <sr@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@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@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.

Thanks,
Stefan