On 06/07/2017 03:28 PM, Simon Glass wrote: > Hi Marek, > > On 7 June 2017 at 06:55, Marek Vasut <[email protected]> wrote: >> On 06/07/2017 02:53 PM, Simon Glass wrote: >>> Hi Marek, >>> >>> On 7 June 2017 at 06:41, Marek Vasut <[email protected]> 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 <[email protected]> wrote: >>>>>> On 06/07/2017 02:16 AM, Simon Glass wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On 6 June 2017 at 17:59, Dr. Philipp Tomsich >>>>>>> <[email protected]> wrote: >>>>>>>> Simon, >>>>>>>> >>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <[email protected]> wrote: >>>>>>>>> >>>>>>>>> Hi Philipp, >>>>>>>>> >>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich >>>>>>>>> <[email protected]> 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 >>>>>>>>>> <[email protected]> >>>>>>>>>> >>>>>>>>>> --- >>>>>>>>>> >>>>>>>>>> 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. -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
