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
