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. -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot