[U-Boot] [PATCH v2 4/7] usb: dwc2-otg: make regs_otg (in platdata) a uintptr_t

[Simon Glass]

+Tom for comment

Hi Marek,

On 7 June 2017 at 00:27, Marek Vasut <marex at denx.de> wrote:
> On 06/07/2017 02:16 AM, Simon Glass wrote:
>> Hi,
>>
>> On 6 June 2017 at 17:59, Dr. Philipp Tomsich
>> <philipp.tomsich at theobroma-systems.com> wrote:
>>> Simon,
>>>
>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg at chromium.org> wrote:
>>>>
>>>> Hi Philipp,
>>>>
>>>> On 6 June 2017 at 07:42, Philipp Tomsich
>>>> <philipp.tomsich at theobroma-systems.com> wrote:
>>>>> The regs_otg field in uintptr_t of the platform data structure for
>>>>> dwc2-otg has thus far been an unsigned int, but will eventually be
>>>>> casted into a void*.
>>>>>
>>>>> This raises the following error with GCC 6.3 and buildman:
>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c: In function 'dwc2_udc_probe':
>>>>>  ../drivers/usb/gadget/dwc2_udc_otg.c:821:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>>>>>    reg = (struct dwc2_usbotg_reg *)pdata->regs_otg;
>>>>>          ^
>>>>>
>>>>> This changes regs_otg to a uintptr_t to ensure that it is large enough
>>>>> to hold any valid pointer (and fix the associated warning).
>>>>>
>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
>>>>>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - (new patch) fix a int-to-pointer cast warning for regs_otg in
>>>>>  dwc2-otg to fix a buildman failure for u-boot-rockchip/master at 2b19b2f
>>>>>
>>>>> include/usb/dwc2_udc.h | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/include/usb/dwc2_udc.h b/include/usb/dwc2_udc.h
>>>>> index 7324d8a..1a370e0 100644
>>>>> --- a/include/usb/dwc2_udc.h
>>>>> +++ b/include/usb/dwc2_udc.h
>>>>> @@ -16,7 +16,7 @@ struct dwc2_plat_otg_data {
>>>>>        int             phy_of_node;
>>>>>        int             (*phy_control)(int on);
>>>>>        unsigned int    regs_phy;
>>>>> -       unsigned int    regs_otg;
>>>>> +       uintptr_t       regs_otg;
>>>>
>>>> Can you use ulong instead?
>>>
>>> Sure, but can I first ask “why?”.
>>> I may reopen an old discussion with this… if so, forgive my ignorance:
>>>
>>> uintptr_t makes the most sense for this use case in the C99 (or later) type system,
>>> as we want this field to hold an integer (i.e. the address from the physical memory
>>> map for one of the register blocks) that will be casted into a pointer.
>>> The uintptr_t type will always the matching size in any and all programming models;
>>> in contrast, ulong would be wrong for LLP64 (and LLP64 probably “doesn’t matter”
>>> in the context of U-Boot anyway).
>>>
>>> What I always found odd, was that uintptr_t is optional according to ISO9899.
>>> I would thus not have been surprised if there’s a concern for portability between
>>> compilers behind this, but then again … U-Boot makes extensive use of GCC
>>> extensions (such as inline assembly).
>>>
>>> So I am apparently missing something here.
>>
>> I don't know of any deep reason except that long is defined to be able
>> to hold an address, and ulong makes sense since an address is
>> generally considered unsigned.
>>
>> U-Boot by convention uses ulong for addresses.
>
> I was under the impression that u-boot by convention uses uintptr_t for
> addresses.
>
>> You can see this all
>> around the code base so I am really just arguing in favour of
>> consistency (and I suppose ulong is easier to type!)
>
> Then I guess half of the codebase is inconsistent.

Actually about 10%:

git grep uintptr_t |wc -l
395
git grep ulong |wc -l
4024

Clearly we use ulong all over the place for addresses - see image.c,
most commands, etc. If we are going to change then it should be a
deliberate decision and a tree-wide update. I'm happy enough with
ulong.

Tom, what do you what to do here?

Regards,
Simon