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 <ma...@denx.de> wrote:
> >> On 06/07/2017 03:28 PM, Simon Glass wrote:
> >>> Hi Marek,
> >>>
> >>> On 7 June 2017 at 06:55, Marek Vasut <ma...@denx.de> wrote:
> >>>> On 06/07/2017 02:53 PM, Simon Glass wrote:
> >>>>> Hi Marek,
> >>>>>
> >>>>> On 7 June 2017 at 06:41, Marek Vasut <ma...@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 <ma...@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.toms...@theobroma-systems.com> wrote:
> >>>>>>>>>> Simon,
> >>>>>>>>>>
> >>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <s...@chromium.org> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> Hi Philipp,
> >>>>>>>>>>>
> >>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich
> >>>>>>>>>>> <philipp.toms...@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.toms...@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@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. -- Tom
signature.asc
Description: Digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
