| Message ID | 1496756553-19901-5-git-send-email-philipp.tomsich@theobroma-systems.com |
|---|---|
| State | Accepted |
| Commit | 92693b5a4f8f70fcfa3630a00e3e714b5caf547c |
| Delegated to: | Simon Glass |
| Headers | show |
On 06/06/2017 03:42 PM, Philipp Tomsich 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@theobroma-systems.com> Applied, thanks > --- > > 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; > unsigned int usb_phy_ctrl; > unsigned int usb_flags; > unsigned int usb_gusbcfg; >
Hi Philipp, On 6 June 2017 at 07:42, Philipp Tomsich <philipp.tomsich@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@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? > unsigned int usb_phy_ctrl; > unsigned int usb_flags; > unsigned int usb_gusbcfg; > -- > 2.1.4 > Regards, Simon
Simon, > On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote: > > Hi Philipp, > > On 6 June 2017 at 07:42, Philipp Tomsich > <philipp.tomsich@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@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. Cheers, Philipp.
Hi, On 6 June 2017 at 17:59, Dr. Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote: > Simon, > >> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote: >> >> Hi Philipp, >> >> On 6 June 2017 at 07:42, Philipp Tomsich >> <philipp.tomsich@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@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. 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!) Regards, Simon
On 06/07/2017 02:16 AM, Simon Glass wrote: > Hi, > > On 6 June 2017 at 17:59, Dr. Philipp Tomsich > <philipp.tomsich@theobroma-systems.com> wrote: >> Simon, >> >>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote: >>> >>> Hi Philipp, >>> >>> On 6 June 2017 at 07:42, Philipp Tomsich >>> <philipp.tomsich@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@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.
+Tom for comment Hi Marek, On 7 June 2017 at 00:27, Marek Vasut <marex@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@theobroma-systems.com> wrote: >>> Simon, >>> >>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote: >>>> >>>> Hi Philipp, >>>> >>>> On 6 June 2017 at 07:42, Philipp Tomsich >>>> <philipp.tomsich@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@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 Clearly we use ulong all over the place for addresses - see image.c, most commands, etc. If we are going to change then it should be a deliberate decision and a tree-wide update. I'm happy enough with ulong. Tom, what do you what to do here? Regards, Simon
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@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@theobroma-systems.com> wrote: >>>> Simon, >>>> >>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote: >>>>> >>>>> Hi Philipp, >>>>> >>>>> On 6 June 2017 at 07:42, Philipp Tomsich >>>>> <philipp.tomsich@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@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 . > If we are going to change then it should be a > deliberate decision and a tree-wide update. I'm happy enough with > ulong. > > Tom, what do you what to do here? > > Regards, > Simon >
Hi Marek, On 7 June 2017 at 06:41, Marek Vasut <marex@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@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@theobroma-systems.com> wrote: >>>>> Simon, >>>>> >>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote: >>>>>> >>>>>> Hi Philipp, >>>>>> >>>>>> On 6 June 2017 at 07:42, Philipp Tomsich >>>>>> <philipp.tomsich@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@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? > >> If we are going to change then it should be a >> deliberate decision and a tree-wide update. I'm happy enough with >> ulong. >> >> Tom, what do you what to do here? >> Regards, Simon
On 06/07/2017 02:53 PM, Simon Glass wrote: > Hi Marek, > > On 7 June 2017 at 06:41, Marek Vasut <marex@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@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@theobroma-systems.com> wrote: >>>>>> Simon, >>>>>> >>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote: >>>>>>> >>>>>>> Hi Philipp, >>>>>>> >>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich >>>>>>> <philipp.tomsich@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@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.
Hi Marek, On 7 June 2017 at 06:55, Marek Vasut <marex@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@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@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@theobroma-systems.com> wrote: >>>>>>> Simon, >>>>>>> >>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote: >>>>>>>> >>>>>>>> Hi Philipp, >>>>>>>> >>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich >>>>>>>> <philipp.tomsich@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@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? Regards, Simon
On 06/07/2017 03:28 PM, Simon Glass wrote: > Hi Marek, > > On 7 June 2017 at 06:55, Marek Vasut <marex@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@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@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@theobroma-systems.com> wrote: >>>>>>>> Simon, >>>>>>>> >>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote: >>>>>>>>> >>>>>>>>> Hi Philipp, >>>>>>>>> >>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich >>>>>>>>> <philipp.tomsich@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@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.
Hi Marek, On 7 June 2017 at 07:33, Marek Vasut <marex@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@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@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@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@theobroma-systems.com> wrote: >>>>>>>>> Simon, >>>>>>>>> >>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote: >>>>>>>>>> >>>>>>>>>> Hi Philipp, >>>>>>>>>> >>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich >>>>>>>>>> <philipp.tomsich@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@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
On 06/07/2017 03:37 PM, Simon Glass wrote: > Hi Marek, > > On 7 June 2017 at 07:33, Marek Vasut <marex@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@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@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@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@theobroma-systems.com> wrote: >>>>>>>>>> Simon, >>>>>>>>>> >>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote: >>>>>>>>>>> >>>>>>>>>>> Hi Philipp, >>>>>>>>>>> >>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich >>>>>>>>>>> <philipp.tomsich@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@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.
On 06/07/2017 03:37 PM, Simon Glass wrote: > Hi Marek, > > On 7 June 2017 at 07:33, Marek Vasut <marex@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@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@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@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@theobroma-systems.com> wrote: >>>>>>>>>> Simon, >>>>>>>>>> >>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote: >>>>>>>>>>> >>>>>>>>>>> Hi Philipp, >>>>>>>>>>> >>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich >>>>>>>>>>> <philipp.tomsich@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@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(-) >>>>>>>>>>>> Applied to u-boot-rockchip, thanks!
On 06/08/2017 05:34 AM, sjg@google.com wrote: > On 06/07/2017 03:37 PM, Simon Glass wrote: >> Hi Marek, >> >> On 7 June 2017 at 07:33, Marek Vasut <marex@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@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@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@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@theobroma-systems.com> wrote: >>>>>>>>>>> Simon, >>>>>>>>>>> >>>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote: >>>>>>>>>>>> >>>>>>>>>>>> Hi Philipp, >>>>>>>>>>>> >>>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich >>>>>>>>>>>> <philipp.tomsich@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@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(-) >>>>>>>>>>>>> > Applied to u-boot-rockchip, thanks! > This is clearly a USB patch ... why would it go through u-boot-rockchip? But OK, yes, I see we have no structure in place and patches go through whatever random tree these days.
Hi Marek, On 8 June 2017 at 06:33, Marek Vasut <marex@denx.de> wrote: > On 06/08/2017 05:34 AM, sjg@google.com wrote: >> On 06/07/2017 03:37 PM, Simon Glass wrote: >>> Hi Marek, >>> >>> On 7 June 2017 at 07:33, Marek Vasut <marex@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@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@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@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@theobroma-systems.com> wrote: >>>>>>>>>>>> Simon, >>>>>>>>>>>> >>>>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> Hi Philipp, >>>>>>>>>>>>> >>>>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich >>>>>>>>>>>>> <philipp.tomsich@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@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(-) >>>>>>>>>>>>>> >> Applied to u-boot-rockchip, thanks! >> > > This is clearly a USB patch ... why would it go through u-boot-rockchip? > But OK, yes, I see we have no structure in place and patches go through > whatever random tree these days. It is assigned to me in patchwork and is needed to fix a build warning. It is tricky to deal with individual patches within a larger series since there are often dependencies. I had the same issue with video patches. Don't we normally try to keep series together? Regards, Simon
On 06/08/2017 03:45 PM, Simon Glass wrote: > Hi Marek, > > On 8 June 2017 at 06:33, Marek Vasut <marex@denx.de> wrote: >> On 06/08/2017 05:34 AM, sjg@google.com wrote: >>> On 06/07/2017 03:37 PM, Simon Glass wrote: >>>> Hi Marek, >>>> >>>> On 7 June 2017 at 07:33, Marek Vasut <marex@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@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@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@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@theobroma-systems.com> wrote: >>>>>>>>>>>>> Simon, >>>>>>>>>>>>> >>>>>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> Hi Philipp, >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich >>>>>>>>>>>>>> <philipp.tomsich@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@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(-) >>>>>>>>>>>>>>> >>> Applied to u-boot-rockchip, thanks! >>> >> >> This is clearly a USB patch ... why would it go through u-boot-rockchip? >> But OK, yes, I see we have no structure in place and patches go through >> whatever random tree these days. > > It is assigned to me in patchwork I see, USB patch assigned not to USB maintainer ... hmmmm ... > and is needed to fix a build > warning. It is tricky to deal with individual patches within a larger > series since there are often dependencies. I had the same issue with > video patches. > > Don't we normally try to keep series together? Don't we normally at least try to get AB/RB from the maintainer before applying patches this way ? > Regards, > Simon >
Hi Marek, On 8 June 2017 at 07:48, Marek Vasut <marex@denx.de> wrote: > On 06/08/2017 03:45 PM, Simon Glass wrote: >> Hi Marek, >> >> On 8 June 2017 at 06:33, Marek Vasut <marex@denx.de> wrote: >>> On 06/08/2017 05:34 AM, sjg@google.com wrote: >>>> On 06/07/2017 03:37 PM, Simon Glass wrote: >>>>> Hi Marek, >>>>> >>>>> On 7 June 2017 at 07:33, Marek Vasut <marex@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@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@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@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@theobroma-systems.com> wrote: >>>>>>>>>>>>>> Simon, >>>>>>>>>>>>>> >>>>>>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Hi Philipp, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich >>>>>>>>>>>>>>> <philipp.tomsich@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@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(-) >>>>>>>>>>>>>>>> >>>> Applied to u-boot-rockchip, thanks! >>>> >>> >>> This is clearly a USB patch ... why would it go through u-boot-rockchip? >>> But OK, yes, I see we have no structure in place and patches go through >>> whatever random tree these days. >> >> It is assigned to me in patchwork > > I see, USB patch assigned not to USB maintainer ... hmmmm ... That is pretty typical if it is part of a series. It's just too hard to coordinate multiple maintainers picking up bits of a series. patch 1 add board feature patch 2 add usb feature patch 3 enable usb on board Patch 3 cannot be applied until both 1 and 2 are in mainline, meaning that we end up with this sequence: patch 1 applied by board maintainer, send pull request patch 2 applied by USB, send pull request patch 3 applied by board maintainer which is very slow and the feature cannot be tested until the end. I guess you know that already, but acking a patch is helpful as it allows them to stay together. > >> and is needed to fix a build >> warning. It is tricky to deal with individual patches within a larger >> series since there are often dependencies. I had the same issue with >> video patches. >> >> Don't we normally try to keep series together? > > Don't we normally at least try to get AB/RB from the maintainer before > applying patches this way ? Yes that is best. I took your 'applied to' as an ack ;-) Regards, Simon
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@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@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@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@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@theobroma-systems.com> wrote: > >>>>>>>>>> Simon, > >>>>>>>>>> > >>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote: > >>>>>>>>>>> > >>>>>>>>>>> Hi Philipp, > >>>>>>>>>>> > >>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich > >>>>>>>>>>> <philipp.tomsich@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@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.
On Thu, Jun 08, 2017 at 07:59:30AM -0600, Simon Glass wrote: > Hi Marek, > > On 8 June 2017 at 07:48, Marek Vasut <marex@denx.de> wrote: > > On 06/08/2017 03:45 PM, Simon Glass wrote: > >> Hi Marek, > >> > >> On 8 June 2017 at 06:33, Marek Vasut <marex@denx.de> wrote: > >>> On 06/08/2017 05:34 AM, sjg@google.com wrote: > >>>> On 06/07/2017 03:37 PM, Simon Glass wrote: > >>>>> Hi Marek, > >>>>> > >>>>> On 7 June 2017 at 07:33, Marek Vasut <marex@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@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@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@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@theobroma-systems.com> wrote: > >>>>>>>>>>>>>> Simon, > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote: > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Hi Philipp, > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich > >>>>>>>>>>>>>>> <philipp.tomsich@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@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(-) > >>>>>>>>>>>>>>>> > >>>> Applied to u-boot-rockchip, thanks! > >>>> > >>> > >>> This is clearly a USB patch ... why would it go through u-boot-rockchip? > >>> But OK, yes, I see we have no structure in place and patches go through > >>> whatever random tree these days. > >> > >> It is assigned to me in patchwork > > > > I see, USB patch assigned not to USB maintainer ... hmmmm ... > > That is pretty typical if it is part of a series. It's just too hard > to coordinate multiple maintainers picking up bits of a series. > > patch 1 add board feature > patch 2 add usb feature > patch 3 enable usb on board > > Patch 3 cannot be applied until both 1 and 2 are in mainline, meaning > that we end up with this sequence: > > patch 1 applied by board maintainer, send pull request > patch 2 applied by USB, send pull request > patch 3 applied by board maintainer > > which is very slow and the feature cannot be tested until the end. I > guess you know that already, but acking a patch is helpful as it > allows them to stay together. Generally speaking, and with the AB/RB of other relevant maintainers, I endorse the idea that a logical series of reasonable size should not be broken up into N smaller series to go in via N subtrees. That said... > >> and is needed to fix a build > >> warning. It is tricky to deal with individual patches within a larger > >> series since there are often dependencies. I had the same issue with > >> video patches. > >> > >> Don't we normally try to keep series together? > > > > Don't we normally at least try to get AB/RB from the maintainer before > > applying patches this way ? > > Yes that is best. I took your 'applied to' as an ack ;-) If someone wants to grab patches via their own subtree, as Marek does, and in this case already had, it should go via that tree. Do I fail to get this right every time? Yes. Should I be better about this? Yes.
Hi, On 8 June 2017 at 08:16, Tom Rini <trini@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@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@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@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@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@theobroma-systems.com> wrote: >> >>>>>>>>>> Simon, >> >>>>>>>>>> >> >>>>>>>>>>> On 06 Jun 2017, at 23:09, Simon Glass <sjg@chromium.org> wrote: >> >>>>>>>>>>> >> >>>>>>>>>>> Hi Philipp, >> >>>>>>>>>>> >> >>>>>>>>>>> On 6 June 2017 at 07:42, Philipp Tomsich >> >>>>>>>>>>> <philipp.tomsich@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@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. 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
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; unsigned int usb_phy_ctrl; unsigned int usb_flags; unsigned int usb_gusbcfg;
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@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(-)