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

[Simon Glass]

Hi Marek,

On 7 June 2017 at 06:41, Marek Vasut <marex at denx.de> wrote:
> On 06/07/2017 02:38 PM, Simon Glass wrote:
>> +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
>
> I don't think this is a valid way to count it at all, since uintptr_t is
> only used for casting pointer to number, while ulong is used for
> arbitrary numbers.
>
>> Clearly we use ulong all over the place for addresses - see image.c,
>> most commands, etc.
>
> But none of those use ulong as device address pointers .

Is there a distinction between a device address pointer and some other
type of address?

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