From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Roese <sr@denx.de>
Date: Mon, 21 Mar 2016 15:04:24 +0100
Subject: [U-Boot] [PATCH 6/6] i2c: designware_i2c: Add support for
 PCI(e) based I2C cores (x86)
In-Reply-To: <CAEUhbmXFL81Md_SP80mNMNoWiaGZ-Y3m7k6rsDbqoB-_wRWVew@mail.gmail.com>
References: <1458287661-21745-1-git-send-email-sr@denx.de>
 <1458287661-21745-6-git-send-email-sr@denx.de>
 <CAEUhbmWW6jne1N7OdGLt1xDUkLY0Ky6Rn4VNnyd9TDOO+Z=+JA@mail.gmail.com>
 <56EFB8D2.5030306@denx.de> <56EFE346.4030008@denx.de>
 <CAEUhbmXFL81Md_SP80mNMNoWiaGZ-Y3m7k6rsDbqoB-_wRWVew@mail.gmail.com>
Message-ID: <56EFFF68.6070205@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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:

/*
 * 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.

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...

Thanks,
Stefan