From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Roese <sr@denx.de>
Date: Wed, 20 Apr 2016 17:17:37 +0200
Subject: [U-Boot] [PATCH 6/6] i2c: designware_i2c: Add support for
 PCI(e) based I2C cores (x86)
In-Reply-To: <CAPnjgZ10VHRJPayPJ-urBScnfcJJm57hNGnnRjwB86WaZ=QxXw@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>
 <56EFFF68.6070205@denx.de>
 <CAEUhbmV9jRPDrKpW8pbOfe6rPXuUF6d4A0nMrGbhxkSTaaNgeg@mail.gmail.com>
 <57027FD8.9010107@denx.de> <570BBCBD.3060709@denx.de>
 <CAPnjgZ16jd0QMbVpyU7g3rXbFneY+Xe=tDFuqwLuZ4v8aWTE3A@mail.gmail.com>
 <57179933.9020200@denx.de>
 <CAPnjgZ10VHRJPayPJ-urBScnfcJJm57hNGnnRjwB86WaZ=QxXw@mail.gmail.com>
Message-ID: <57179D91.3090400@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 Simon,

On 20.04.2016 17:09, Simon Glass wrote:
> Hi Stefan,
>
> On 20 April 2016 at 08:58, Stefan Roese <sr@denx.de> wrote:
>>
>> 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.
>
> Seems fine to me. Perhaps we should have dev_get_addr_ptr() to do
> this for us?

Might make sense. I can generate a small patch for this.

Perhaps we should better use "uintptr_t" as type for the intermediate
variable instead. But then we can effectively drop the intermediate
variable and do the casting directly.

What do you think?

Thanks,
Stefan