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

Simon Glass sjg at chromium.org
Thu Jun 8 22:41:33 UTC 2017 

Hi,

On 8 June 2017 at 08:16, Tom Rini <trini at konsulko.com> wrote:
> On Wed, Jun 07, 2017 at 03:40:30PM +0200, Marek Vasut wrote:
>> On 06/07/2017 03:37 PM, Simon Glass wrote:
>> > Hi Marek,
>> >
>> > On 7 June 2017 at 07:33, Marek Vasut <marex at denx.de> wrote:
>> >> 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.
>> >
>> > OK, how about updating https://www.denx.de/wiki/U-Boot/CodingStyle
>> > with what we now want?
>>
>> Works for me ... so what do we want now ? I'm not gonna do decision
>> affecting the entire project on my own, that never works.
>
> No, but you do have a clear and concise way of explaining technical
> requirements, so I would appreciate your wording on what to add here, if
> nothing else.  Thanks!
>
> And yes, I'm chiming in now, to say that I do like using uintptr_t.

Yes that's right. Marek's explanation was convincing to me, even
though I find uintptr_t confusing to read and longer to type. So Marek
I think it is worth putting on the coding style page. That way when it
comes up in code reviews we can point to it.

Regards,
Simon

More information about the U-Boot mailing list