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

[Marek Vasut]

On 06/07/2017 03:28 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 7 June 2017 at 06:55, Marek Vasut <marex at denx.de> wrote:
>> On 06/07/2017 02:53 PM, Simon Glass wrote:
>>> 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?
>>
>> ulong is not used only for addresses, which your metric doesn't account for.
> 
> OK, but if you look around you can see my point. See for example
> cmd/mem.c which uses ulong everywhere. Image handling is the same -
> see e.g. image.h struct image_info and struct bootm_headers. Are you
> saying those are wrong, or that this case is different, or something
> else?

I guess we could convert them to uintptr_t , but I've mostly used
uintptr_t when converting void __iomem * to numbers written to HW
registers .

I also think being explicit about "hey, this is a pointer converted to a
number" does not hurt, so I like the uintptr_t better than ulong for
such converted pointers.

re cmd/mem.c , it's legacy code , the new code and/or code which used to
trigger compiler warnings on ie. arm64 was fixed up mostly to use the
uintptr_t recently.

-- 
Best regards,
Marek Vasut