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? Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot