Re: [U-Boot] [PATCH 2/3] fix: s5p_gpio: call: dev_get_addr() instead of fdtdec_get_addr()

2015-09-28 Thread Simon Glass 
On 25 September 2015 at 09:48, Stephen Warren  wrote:
> On 09/25/2015 02:36 AM, Przemyslaw Marczak wrote:
>>
>> Hello Stephen,
>>
>> On 09/24/2015 07:29 PM, Stephen Warren wrote:
>>>
>>> On 09/24/2015 09:29 AM, Przemyslaw Marczak wrote:

 After rework in lib/fdtdec.c, the function fdtdec_get_addr()
 doesn't work for nodes with #size-cells property, set to 0.

 To get GPIO's 'reg' property, the code should use one of:
 fdtdec_get_addr_size_auto_no/parent() function.

 Fortunately dm core provides a function to get the property.

 This commit reworks function gpio_exynos_bind(), to properly
 use dev_get_addr() for GPIO device.

 This prevents setting a wrong base register for Exynos GPIOs.
>>>
>>>
>>> Migrating everything to dev_get_addr() is the correct long-term fix, so
>>> this patch,
>>>
>>> Acked-by: Stephen Warren 
>>>
>>> ... although I'd have liked to see a smaller diff that didn't both
>>> re-order all the code /and/ call a different function, but I suppose
>>> that's not possible given the need to pass the device object to
>>> dev_get_addr(). You could have used fdtdec_get_addr_size_auto_parent()
>>> directly.
>>
>>
>> Yes, it's not a single line diff, but the driver supports driver-model,
>> so it's natural that it should use driver model API if can, instead of
>> fdtdec API.
>>
>> This approach makes things easier to test and catch mistakes in the
>> future.
>>
>>>
>>>
>>> I think it'd be good to fix fdtdec_get_addr_size() to have the same
>>> semantics that it previously did. There might be other code in U-Boot
>>> that's affected by the same issue, and fixing fdtdec_get_addr_size()
>>> would make sure that all got fixed too. Are you willing to send that
>>> patch too?
>>>
>>> Essentially, fdtdec_get_addr_size() used to assume:
>>>
>>> #address-cells == sizeof(fdt_addr_t)
>>> if sizep == NULL:
>>>  #size-cells == 0
>>> else:
>>>  #size-cells == sizeof(fdt_addr_t)
>>>
>>> However, it now assumes:
>>>
>>> #address-cells == sizeof(fdt_addr_t)
>>> #size-cells == sizeof(fdt_addr_t)
>>>
>>> Let's just add that condition back by doing something like the following
>>> in fdtdec_get_addr_size():
>>>
>>> u32 ns;
>>>
>>> if (sizep)
>>>  ns = sizeof(fdt_size_t) / sizeof(fdt32_t);
>>> else
>>>  ns = 0;
>>>
>>> ... and replacing the ns parameter that's passed to
>>> fdtdec_get_addr_size_fixed() with that variable, rather than hard-coding
>>> it.
>>
>>
>> Sorry, currently I have some other things to do, and I wouldn't prefer
>> fixing this without proper testing. Such core things should be tested in
>> sandbox by couple of unit tests.
>
>
> OK, I'll take a stab at it.
>
>> This seem to be okay, but is still wrong.
>>
>> We should always call fdtdec_get_addr_size_fixed() with arguments, which
>> fits to the dtb, instead of hardcoded values.
>>
>> So, only the implementation of function
>>
>> fdtdec_get_addr_size_auto_parent()
>>
>> seem to be correct.
>>
>> It check the real #address-cells and #size-cells.
>
>
> Right. All "client" code should be migrated to call function which look at
> #address-cells and #size-cells. That's what
> fdtdec_get_addr_size_auto_parent(), fdtdec_get_addr_size_auto_noparent(),
> and dev_get_addr() do.
>
> However, there is code in U-Boot which (incorrectly) used fdtdec_get_addr()
> to parse properties other than reg. Those properties aren't affected by
> #address-cells and #size-cells. Hence, the hard-coding of na and ns inside
> fdtdec_get_addr_size() is required to support those use-case. Hopefully once
> everything that parses reg is migrated to the functions that look at
> #address-cells and #size-cells, fdtdec_get_addr_size() can be renamed to
> make it obvious it shouldn't be used for parsing reg.
>
>> If this is slow, then maybe we need some cache with nodes, its
>> parents/childs and its size/addr cells to be checked only once?
>
>
> Hopefully all (or almost all) use-cases can use dev_get_addr(). There's no
> slowness there, since there's no searching of the DT to find the parent;
> it's already known directly.

Tested on snow:
Tested-by: Simon Glass 

Acked-by: Simon Glass 
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/3] fix: s5p_gpio: call: dev_get_addr() instead of fdtdec_get_addr()

2015-09-25 Thread Stephen Warren 

On 09/25/2015 02:36 AM, Przemyslaw Marczak wrote:

Hello Stephen,

On 09/24/2015 07:29 PM, Stephen Warren wrote:

On 09/24/2015 09:29 AM, Przemyslaw Marczak wrote:

After rework in lib/fdtdec.c, the function fdtdec_get_addr()
doesn't work for nodes with #size-cells property, set to 0.

To get GPIO's 'reg' property, the code should use one of:
fdtdec_get_addr_size_auto_no/parent() function.

Fortunately dm core provides a function to get the property.

This commit reworks function gpio_exynos_bind(), to properly
use dev_get_addr() for GPIO device.

This prevents setting a wrong base register for Exynos GPIOs.


Migrating everything to dev_get_addr() is the correct long-term fix, so
this patch,

Acked-by: Stephen Warren 

... although I'd have liked to see a smaller diff that didn't both
re-order all the code /and/ call a different function, but I suppose
that's not possible given the need to pass the device object to
dev_get_addr(). You could have used fdtdec_get_addr_size_auto_parent()
directly.


Yes, it's not a single line diff, but the driver supports driver-model,
so it's natural that it should use driver model API if can, instead of
fdtdec API.

This approach makes things easier to test and catch mistakes in the future.




I think it'd be good to fix fdtdec_get_addr_size() to have the same
semantics that it previously did. There might be other code in U-Boot
that's affected by the same issue, and fixing fdtdec_get_addr_size()
would make sure that all got fixed too. Are you willing to send that
patch too?

Essentially, fdtdec_get_addr_size() used to assume:

#address-cells == sizeof(fdt_addr_t)
if sizep == NULL:
 #size-cells == 0
else:
 #size-cells == sizeof(fdt_addr_t)

However, it now assumes:

#address-cells == sizeof(fdt_addr_t)
#size-cells == sizeof(fdt_addr_t)

Let's just add that condition back by doing something like the following
in fdtdec_get_addr_size():

u32 ns;

if (sizep)
 ns = sizeof(fdt_size_t) / sizeof(fdt32_t);
else
 ns = 0;

... and replacing the ns parameter that's passed to
fdtdec_get_addr_size_fixed() with that variable, rather than hard-coding
it.


Sorry, currently I have some other things to do, and I wouldn't prefer
fixing this without proper testing. Such core things should be tested in
sandbox by couple of unit tests.


OK, I'll take a stab at it.


This seem to be okay, but is still wrong.

We should always call fdtdec_get_addr_size_fixed() with arguments, which
fits to the dtb, instead of hardcoded values.

So, only the implementation of function

fdtdec_get_addr_size_auto_parent()

seem to be correct.

It check the real #address-cells and #size-cells.


Right. All "client" code should be migrated to call function which look 
at #address-cells and #size-cells. That's what 
fdtdec_get_addr_size_auto_parent(), 
fdtdec_get_addr_size_auto_noparent(), and dev_get_addr() do.


However, there is code in U-Boot which (incorrectly) used 
fdtdec_get_addr() to parse properties other than reg. Those properties 
aren't affected by #address-cells and #size-cells. Hence, the 
hard-coding of na and ns inside fdtdec_get_addr_size() is required to 
support those use-case. Hopefully once everything that parses reg is 
migrated to the functions that look at #address-cells and #size-cells, 
fdtdec_get_addr_size() can be renamed to make it obvious it shouldn't be 
used for parsing reg.



If this is slow, then maybe we need some cache with nodes, its
parents/childs and its size/addr cells to be checked only once?


Hopefully all (or almost all) use-cases can use dev_get_addr(). There's 
no slowness there, since there's no searching of the DT to find the 
parent; it's already known directly.

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/3] fix: s5p_gpio: call: dev_get_addr() instead of fdtdec_get_addr()

2015-09-25 Thread Przemyslaw Marczak 

Hello Stephen,

On 09/24/2015 07:29 PM, Stephen Warren wrote:

On 09/24/2015 09:29 AM, Przemyslaw Marczak wrote:

After rework in lib/fdtdec.c, the function fdtdec_get_addr()
doesn't work for nodes with #size-cells property, set to 0.

To get GPIO's 'reg' property, the code should use one of:
fdtdec_get_addr_size_auto_no/parent() function.

Fortunately dm core provides a function to get the property.

This commit reworks function gpio_exynos_bind(), to properly
use dev_get_addr() for GPIO device.

This prevents setting a wrong base register for Exynos GPIOs.


Migrating everything to dev_get_addr() is the correct long-term fix, so
this patch,

Acked-by: Stephen Warren 

... although I'd have liked to see a smaller diff that didn't both
re-order all the code /and/ call a different function, but I suppose
that's not possible given the need to pass the device object to
dev_get_addr(). You could have used fdtdec_get_addr_size_auto_parent()
directly.


Yes, it's not a single line diff, but the driver supports driver-model, 
so it's natural that it should use driver model API if can, instead of 
fdtdec API.


This approach makes things easier to test and catch mistakes in the future.




I think it'd be good to fix fdtdec_get_addr_size() to have the same
semantics that it previously did. There might be other code in U-Boot
that's affected by the same issue, and fixing fdtdec_get_addr_size()
would make sure that all got fixed too. Are you willing to send that
patch too?

Essentially, fdtdec_get_addr_size() used to assume:

#address-cells == sizeof(fdt_addr_t)
if sizep == NULL:
 #size-cells == 0
else:
 #size-cells == sizeof(fdt_addr_t)

However, it now assumes:

#address-cells == sizeof(fdt_addr_t)
#size-cells == sizeof(fdt_addr_t)

Let's just add that condition back by doing something like the following
in fdtdec_get_addr_size():

u32 ns;

if (sizep)
 ns = sizeof(fdt_size_t) / sizeof(fdt32_t);
else
 ns = 0;

... and replacing the ns parameter that's passed to
fdtdec_get_addr_size_fixed() with that variable, rather than hard-coding
it.



Sorry, currently I have some other things to do, and I wouldn't prefer 
fixing this without proper testing. Such core things should be tested in 
sandbox by couple of unit tests.


This seem to be okay, but is still wrong.

We should always call fdtdec_get_addr_size_fixed() with arguments, which 
fits to the dtb, instead of hardcoded values.


So, only the implementation of function

fdtdec_get_addr_size_auto_parent()

seem to be correct.

It check the real #address-cells and #size-cells.

If this is slow, then maybe we need some cache with nodes, its 
parents/childs and its size/addr cells to be checked only once?


Best regards,
--
Przemyslaw Marczak
Samsung R Institute Poland
Samsung Electronics
p.marc...@samsung.com
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 2/3] fix: s5p_gpio: call: dev_get_addr() instead of fdtdec_get_addr()

2015-09-24 Thread Przemyslaw Marczak 
After rework in lib/fdtdec.c, the function fdtdec_get_addr()
doesn't work for nodes with #size-cells property, set to 0.

To get GPIO's 'reg' property, the code should use one of:
fdtdec_get_addr_size_auto_no/parent() function.

Fortunately dm core provides a function to get the property.

This commit reworks function gpio_exynos_bind(), to properly
use dev_get_addr() for GPIO device.

This prevents setting a wrong base register for Exynos GPIOs.

Signed-off-by: Przemyslaw Marczak 
---
 drivers/gpio/s5p_gpio.c | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/s5p_gpio.c b/drivers/gpio/s5p_gpio.c
index 17fcfbf..0f22b23 100644
--- a/drivers/gpio/s5p_gpio.c
+++ b/drivers/gpio/s5p_gpio.c
@@ -341,18 +341,22 @@ static int gpio_exynos_bind(struct udevice *parent)
plat = calloc(1, sizeof(*plat));
if (!plat)
return -ENOMEM;
-   reg = fdtdec_get_addr(blob, node, "reg");
-   if (reg != FDT_ADDR_T_NONE)
-   bank = (struct s5p_gpio_bank *)((ulong)base + reg);
-   plat->bank = bank;
-   plat->bank_name = fdt_get_name(blob, node, NULL);
-   debug("dev at %p: %s\n", bank, plat->bank_name);
 
+   plat->bank_name = fdt_get_name(blob, node, NULL);
ret = device_bind(parent, parent->driver,
-   plat->bank_name, plat, -1, );
+ plat->bank_name, plat, -1, );
if (ret)
return ret;
+
dev->of_offset = node;
+
+   reg = dev_get_addr(dev);
+   if (reg != FDT_ADDR_T_NONE)
+   bank = (struct s5p_gpio_bank *)((ulong)base + reg);
+
+   plat->bank = bank;
+
+   debug("dev at %p: %s\n", bank, plat->bank_name);
}
 
return 0;
-- 
1.9.1

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/3] fix: s5p_gpio: call: dev_get_addr() instead of fdtdec_get_addr()

2015-09-24 Thread Stephen Warren 

On 09/24/2015 09:29 AM, Przemyslaw Marczak wrote:

After rework in lib/fdtdec.c, the function fdtdec_get_addr()
doesn't work for nodes with #size-cells property, set to 0.

To get GPIO's 'reg' property, the code should use one of:
fdtdec_get_addr_size_auto_no/parent() function.

Fortunately dm core provides a function to get the property.

This commit reworks function gpio_exynos_bind(), to properly
use dev_get_addr() for GPIO device.

This prevents setting a wrong base register for Exynos GPIOs.


Migrating everything to dev_get_addr() is the correct long-term fix, so 
this patch,


Acked-by: Stephen Warren 

... although I'd have liked to see a smaller diff that didn't both 
re-order all the code /and/ call a different function, but I suppose 
that's not possible given the need to pass the device object to 
dev_get_addr(). You could have used fdtdec_get_addr_size_auto_parent() 
directly.



I think it'd be good to fix fdtdec_get_addr_size() to have the same 
semantics that it previously did. There might be other code in U-Boot 
that's affected by the same issue, and fixing fdtdec_get_addr_size() 
would make sure that all got fixed too. Are you willing to send that 
patch too?


Essentially, fdtdec_get_addr_size() used to assume:

#address-cells == sizeof(fdt_addr_t)
if sizep == NULL:
#size-cells == 0
else:
#size-cells == sizeof(fdt_addr_t)

However, it now assumes:

#address-cells == sizeof(fdt_addr_t)
#size-cells == sizeof(fdt_addr_t)

Let's just add that condition back by doing something like the following 
in fdtdec_get_addr_size():


u32 ns;

if (sizep)
ns = sizeof(fdt_size_t) / sizeof(fdt32_t);
else
ns = 0;

... and replacing the ns parameter that's passed to 
fdtdec_get_addr_size_fixed() with that variable, rather than hard-coding it.

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot