[U-Boot] [PATCH 0/3] Fix fdt 'reg' parsing and unbreak Odroid U3

[U-Boot] [PATCH 0/3] Fix fdt 'reg' parsing and unbreak Odroid U3

[Przemyslaw Marczak]

Booting of Odroid U3 with SD card, ends with error:

MMC:   EXYNOS DWMMC: 0
Card did not respond to voltage select!
*** Warning - MMC init failed, using default environment

Generally this was broken, because of wrong addresses assigned to GPIOs.

The source of the problem was in rework of lib/fdtdec.c, after which
function fdtdec_get_addr() doesn't work as previous and function
dev_get_addr() doesn't works as expected.

The code after rework in lib/fdtdec.c assume, that #size-cells property,
should be always greater or equal to 1. This was wrong, because it can be 0.

In case of debugging the issue I found, that mmc clock was computed wrong,
for Exynos4, because of function get_mmc_clk(), which always return -1 for
this SoC.

Tested on: Odroid U3 and Odroid XU3.

Przemyslaw Marczak (3):
  fix: fdtdec: allow parse 'reg' property with zero value in
    '#size-cells'
  fix: s5p_gpio: call: dev_get_addr() instead of fdtdec_get_addr()
  fix: mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()

 arch/arm/mach-exynos/clock.c | 10 ++++------
 drivers/gpio/s5p_gpio.c      | 18 +++++++++++-------
 lib/fdtdec.c                 |  2 +-
 3 files changed, 16 insertions(+), 14 deletions(-)

-- 
1.9.1

[U-Boot] [PATCH 1/3] fix: fdtdec: allow parse 'reg' property with zero value in '#size-cells'

[Przemyslaw Marczak]

After rework of lib/fdtdec.c by commit:

commit 02464e386bb5f0a022c121f95ae75cf583759d95
Author: Stephen Warren <swarren@nvidia.com>
Date:   Thu Aug 6 15:31:02 2015 -0600

the function fdtdec_get_addr() doesn't work as previous,
because the implementation assumes that properties '#address-cells'
and '#size-cells' are equal to 1, which can be not true sometimes.

The new API introduced fdtdec_get_addr_size_auto_parent() for the 'reg'
property parsing, but the implementation assumes, that #size-cells
can't be less than 1.

This causes that the following children's 'reg' property can't be reached:

parent at 0x0 {
     #address-cells = <1>;
     #size-cells = <0>;
     children at 0x100 {
         reg = < 0x100 >;
     };
};

Change the condition value from '1' to '0', which allows parsing property
with at least zero #size-cells, fixes the issue.

Now, fdtdec_get_addr_size_auto_parent() works properly.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
---
 lib/fdtdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 9f0b65d..9cf57b9 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -149,7 +149,7 @@ fdt_addr_t fdtdec_get_addr_size_auto_parent(const void *blob, int parent,
 	}
 
 	ns = fdt_size_cells(blob, parent);
-	if (ns < 1) {
+	if (ns < 0) {
 		debug("(bad #size-cells)\n");
 		return FDT_ADDR_T_NONE;
 	}
-- 
1.9.1

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

[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 <p.marczak@samsung.com>
---
 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, &dev);
+				  plat->bank_name, plat, -1, &dev);
 		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] [PATCH 3/3] fix: mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()

[Przemyslaw Marczak]

After rework of code by commit:

commit d95279685bb9690a6973226a3bd8a3bae65c2ad7
Author: Akshay Saraswat <akshay.s@samsung.com>
Date:   Wed Feb 4 16:00:03 2015 +0530

function get_mmc_clk() always returns -1 for Exynos 4.

This was caused by omitting, that SDHCI driver for Exynos 4,
calls get_mmc_clk(), with mmc device number as argument,
instead of pinmux peripheral id, like DW MMC driver for Exynos 5.

By this commit, the code directly calls a proper function
to get mmc clock for Exynos 4, without checking the peripheral id.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
---
 arch/arm/mach-exynos/clock.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-exynos/clock.c b/arch/arm/mach-exynos/clock.c
index 1c6baa1..18eadf5 100644
--- a/arch/arm/mach-exynos/clock.c
+++ b/arch/arm/mach-exynos/clock.c
@@ -1661,6 +1661,9 @@ unsigned long get_mmc_clk(int dev_index)
 {
 	enum periph_id id;
 
+	if (cpu_is_exynos4())
+		return exynos4_get_mmc_clk(dev_index);
+
 	switch (dev_index) {
 	case 0:
 		id = PERIPH_ID_SDMMC0;
@@ -1679,12 +1682,7 @@ unsigned long get_mmc_clk(int dev_index)
 		return -1;
 	}
 
-	if (cpu_is_exynos5())
-		return clock_get_periph_rate(id);
-	else if (cpu_is_exynos4())
-		return exynos4_get_mmc_clk(dev_index);
-
-	return 0;
+	return clock_get_periph_rate(id);
 }
 
 void set_mmc_clk(int dev_index, unsigned int div)
-- 
1.9.1

[U-Boot] [PATCH 1/3] fix: fdtdec: allow parse 'reg' property with zero value in '#size-cells'

[Stephen Warren]

On 09/24/2015 09:29 AM, Przemyslaw Marczak wrote:
> After rework of lib/fdtdec.c by commit:
>
> commit 02464e386bb5f0a022c121f95ae75cf583759d95
> Author: Stephen Warren <swarren@nvidia.com>
> Date:   Thu Aug 6 15:31:02 2015 -0600

That'd usually be abbreviated as:

Commit 02464e386bb5 "fdt: add new fdt address parsing functions".

Of course, if you want to shame me that's justified too:-) Tracking down 
regressions sucks:-(

> the function fdtdec_get_addr() doesn't work as previous,
> because the implementation assumes that properties '#address-cells'
> and '#size-cells' are equal to 1, which can be not true sometimes.

"are equal to" should be "is at least"; the purpose of that rework was 
to support values greater than one.

> The new API introduced fdtdec_get_addr_size_auto_parent() for the 'reg'
> property parsing, but the implementation assumes, that #size-cells
> can't be less than 1.
>
> This causes that the following children's 'reg' property can't be reached:
>
> parent at 0x0 {
>       #address-cells = <1>;
>       #size-cells = <0>;
>       children at 0x100 {
>           reg = < 0x100 >;
>       };
> };
>
> Change the condition value from '1' to '0', which allows parsing property
> with at least zero #size-cells, fixes the issue.
>
> Now, fdtdec_get_addr_size_auto_parent() works properly.

Sorry about that. This patch,

Acked-by: Stephen Warren <swarren@nvidia.com>

(but not tested, but since this allows a previously failing case, it's 
hard to see how this patch could cause any problems.)

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

[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 <swarren@nvidia.com>

... 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] [PATCH 0/3] Fix fdt 'reg' parsing and unbreak Odroid U3

[Jaehoon Chung]

Hi, Przemyslaw.

On 09/25/2015 12:29 AM, Przemyslaw Marczak wrote:
> Booting of Odroid U3 with SD card, ends with error:
> 
> MMC:   EXYNOS DWMMC: 0
> Card did not respond to voltage select!
> *** Warning - MMC init failed, using default environment
> 
> Generally this was broken, because of wrong addresses assigned to GPIOs.

Great! I will check this patch-set..But it seems to look good to me. :)

Best Regards,
Jaehoon Chung

> 
> The source of the problem was in rework of lib/fdtdec.c, after which
> function fdtdec_get_addr() doesn't work as previous and function
> dev_get_addr() doesn't works as expected.
> 
> The code after rework in lib/fdtdec.c assume, that #size-cells property,
> should be always greater or equal to 1. This was wrong, because it can be 0.
> 
> In case of debugging the issue I found, that mmc clock was computed wrong,
> for Exynos4, because of function get_mmc_clk(), which always return -1 for
> this SoC.
> 
> Tested on: Odroid U3 and Odroid XU3.
> 
> Przemyslaw Marczak (3):
>   fix: fdtdec: allow parse 'reg' property with zero value in
>     '#size-cells'
>   fix: s5p_gpio: call: dev_get_addr() instead of fdtdec_get_addr()
>   fix: mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()
> 
>  arch/arm/mach-exynos/clock.c | 10 ++++------
>  drivers/gpio/s5p_gpio.c      | 18 +++++++++++-------
>  lib/fdtdec.c                 |  2 +-
>  3 files changed, 16 insertions(+), 14 deletions(-)
> 

[U-Boot] [PATCH 1/3] fix: fdtdec: allow parse 'reg' property with zero value in '#size-cells'

[Przemyslaw Marczak]

Hello Stephen,

On 09/24/2015 07:14 PM, Stephen Warren wrote:
> On 09/24/2015 09:29 AM, Przemyslaw Marczak wrote:
>> After rework of lib/fdtdec.c by commit:
>>
>> commit 02464e386bb5f0a022c121f95ae75cf583759d95
>> Author: Stephen Warren <swarren@nvidia.com>
>> Date:   Thu Aug 6 15:31:02 2015 -0600
>
> That'd usually be abbreviated as:
>
> Commit 02464e386bb5 "fdt: add new fdt address parsing functions".

Ok, I will update the commit message.

>
> Of course, if you want to shame me that's justified too:-) Tracking down
> regressions sucks:-(
>

Oh no no... maybe a little :)

>> the function fdtdec_get_addr() doesn't work as previous,
>> because the implementation assumes that properties '#address-cells'
>> and '#size-cells' are equal to 1, which can be not true sometimes.
>
> "are equal to" should be "is at least"; the purpose of that rework was
> to support values greater than one.
>

But it describe the fdtdec_get_addr(), which calls

fdtdec_get_addr_size_fixed(...)

and for this call we have:

na = sizeof(fdt_addr_t) / sizeof(fdt32_t) == 1

ns = sizeof(fdt_size_t) / sizeof(fdt32_t) == 1

This is consistent with the description for this function in 
include/fdtdec.h.

>> The new API introduced fdtdec_get_addr_size_auto_parent() for the 'reg'
>> property parsing, but the implementation assumes, that #size-cells
>> can't be less than 1.
>>
>> This causes that the following children's 'reg' property can't be
>> reached:
>>
>> parent at 0x0 {
>>       #address-cells = <1>;
>>       #size-cells = <0>;
>>       children at 0x100 {
>>           reg = < 0x100 >;
>>       };
>> };
>>
>> Change the condition value from '1' to '0', which allows parsing property
>> with at least zero #size-cells, fixes the issue.
>>
>> Now, fdtdec_get_addr_size_auto_parent() works properly.
>
> Sorry about that. This patch,

Don't worry, no one is infallible :)

>
> Acked-by: Stephen Warren <swarren@nvidia.com>
>
> (but not tested, but since this allows a previously failing case, it's
> hard to see how this patch could cause any problems.)
>

This just fixes the problem, which I noticed, but it looks, that it 
shouldn't break other things.

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

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

[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 <swarren@nvidia.com>
>
> ... 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&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH 0/3] Fix fdt 'reg' parsing and unbreak Odroid U3

[Przemyslaw Marczak]

Hello,

On 09/24/2015 05:29 PM, Przemyslaw Marczak wrote:
> Booting of Odroid U3 with SD card, ends with error:
>
> MMC:   EXYNOS DWMMC: 0
> Card did not respond to voltage select!
> *** Warning - MMC init failed, using default environment
>
> Generally this was broken, because of wrong addresses assigned to GPIOs.
>
> The source of the problem was in rework of lib/fdtdec.c, after which
> function fdtdec_get_addr() doesn't work as previous and function
> dev_get_addr() doesn't works as expected.
>
> The code after rework in lib/fdtdec.c assume, that #size-cells property,
> should be always greater or equal to 1. This was wrong, because it can be 0.
>
> In case of debugging the issue I found, that mmc clock was computed wrong,
> for Exynos4, because of function get_mmc_clk(), which always return -1 for
> this SoC.
>
> Tested on: Odroid U3 and Odroid XU3.
>
> Przemyslaw Marczak (3):
>    fix: fdtdec: allow parse 'reg' property with zero value in
>      '#size-cells'
>    fix: s5p_gpio: call: dev_get_addr() instead of fdtdec_get_addr()
>    fix: mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()
>
>   arch/arm/mach-exynos/clock.c | 10 ++++------
>   drivers/gpio/s5p_gpio.c      | 18 +++++++++++-------
>   lib/fdtdec.c                 |  2 +-
>   3 files changed, 16 insertions(+), 14 deletions(-)
>

+Tested-on: Odroid X2

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH 0/3] Fix fdt 'reg' parsing and unbreak Odroid U3

[Przemyslaw Marczak]

Hello Jaehoon,

On 09/25/2015 04:40 AM, Jaehoon Chung wrote:
> Hi, Przemyslaw.
>
> On 09/25/2015 12:29 AM, Przemyslaw Marczak wrote:
>> Booting of Odroid U3 with SD card, ends with error:
>>
>> MMC:   EXYNOS DWMMC: 0
>> Card did not respond to voltage select!
>> *** Warning - MMC init failed, using default environment
>>
>> Generally this was broken, because of wrong addresses assigned to GPIOs.
>
> Great! I will check this patch-set..But it seems to look good to me. :)
>
> Best Regards,
> Jaehoon Chung
>

At present, the patchset was tested on U3/X2 and XU3.

>>
>> The source of the problem was in rework of lib/fdtdec.c, after which
>> function fdtdec_get_addr() doesn't work as previous and function
>> dev_get_addr() doesn't works as expected.
>>
>> The code after rework in lib/fdtdec.c assume, that #size-cells property,
>> should be always greater or equal to 1. This was wrong, because it can be 0.
>>
>> In case of debugging the issue I found, that mmc clock was computed wrong,
>> for Exynos4, because of function get_mmc_clk(), which always return -1 for
>> this SoC.
>>
>> Tested on: Odroid U3 and Odroid XU3.
>>
>> Przemyslaw Marczak (3):
>>    fix: fdtdec: allow parse 'reg' property with zero value in
>>      '#size-cells'
>>    fix: s5p_gpio: call: dev_get_addr() instead of fdtdec_get_addr()
>>    fix: mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()
>>
>>   arch/arm/mach-exynos/clock.c | 10 ++++------
>>   drivers/gpio/s5p_gpio.c      | 18 +++++++++++-------
>>   lib/fdtdec.c                 |  2 +-
>>   3 files changed, 16 insertions(+), 14 deletions(-)
>>
>
>

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH 0/3] Fix fdt 'reg' parsing and unbreak Odroid U3

[Przemyslaw Marczak]

Hello again,

On 09/25/2015 10:56 AM, Przemyslaw Marczak wrote:
> Hello,
>
> On 09/24/2015 05:29 PM, Przemyslaw Marczak wrote:
>> Booting of Odroid U3 with SD card, ends with error:
>>
>> MMC:   EXYNOS DWMMC: 0
>> Card did not respond to voltage select!
>> *** Warning - MMC init failed, using default environment
>>
>> Generally this was broken, because of wrong addresses assigned to GPIOs.
>>
>> The source of the problem was in rework of lib/fdtdec.c, after which
>> function fdtdec_get_addr() doesn't work as previous and function
>> dev_get_addr() doesn't works as expected.
>>
>> The code after rework in lib/fdtdec.c assume, that #size-cells property,
>> should be always greater or equal to 1. This was wrong, because it can
>> be 0.
>>
>> In case of debugging the issue I found, that mmc clock was computed
>> wrong,
>> for Exynos4, because of function get_mmc_clk(), which always return -1
>> for
>> this SoC.
>>
>> Tested on: Odroid U3 and Odroid XU3.
>>
>> Przemyslaw Marczak (3):
>>    fix: fdtdec: allow parse 'reg' property with zero value in
>>      '#size-cells'
>>    fix: s5p_gpio: call: dev_get_addr() instead of fdtdec_get_addr()
>>    fix: mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()
>>
>>   arch/arm/mach-exynos/clock.c | 10 ++++------
>>   drivers/gpio/s5p_gpio.c      | 18 +++++++++++-------
>>   lib/fdtdec.c                 |  2 +-
>>   3 files changed, 16 insertions(+), 14 deletions(-)
>>
>
> +Tested-on: Odroid X2
>
> Best regards,

This patchset also fixes broken boot on Trats2. Probably the same for 
other Exynos4 boards.

+Tested-on: Trats2

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH 1/3] fix: fdtdec: allow parse 'reg' property with zero value in '#size-cells'

[Stephen Warren]

On 09/25/2015 02:35 AM, Przemyslaw Marczak wrote:
> Hello Stephen,
>
> On 09/24/2015 07:14 PM, Stephen Warren wrote:
>> On 09/24/2015 09:29 AM, Przemyslaw Marczak wrote:
>>> After rework of lib/fdtdec.c by commit:
>>>
>>> commit 02464e386bb5f0a022c121f95ae75cf583759d95
>>> Author: Stephen Warren <swarren@nvidia.com>
>>> Date:   Thu Aug 6 15:31:02 2015 -0600
>>
>> That'd usually be abbreviated as:
>>
>> Commit 02464e386bb5 "fdt: add new fdt address parsing functions".
>
> Ok, I will update the commit message.
>
>> Of course, if you want to shame me that's justified too:-) Tracking down
>> regressions sucks:-(
>
> Oh no no... maybe a little :)
>
>>> the function fdtdec_get_addr() doesn't work as previous,
>>> because the implementation assumes that properties '#address-cells'
>>> and '#size-cells' are equal to 1, which can be not true sometimes.
>>
>> "are equal to" should be "is at least"; the purpose of that rework was
>> to support values greater than one.
>>
>
> But it describe the fdtdec_get_addr(), which calls
>
> fdtdec_get_addr_size_fixed(...)
>
> and for this call we have:
>
> na = sizeof(fdt_addr_t) / sizeof(fdt32_t) == 1
>
> ns = sizeof(fdt_size_t) / sizeof(fdt32_t) == 1
>
> This is consistent with the description for this function in
> include/fdtdec.h.

Ah yes; I was thinking of the core function 
fdtdec_get_addr_size_fixed(). The description you gave seems correct.

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

[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 <swarren@nvidia.com>
>>
>> ... 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] [PATCH V2 0/3] Fix fdt 'reg' parsing and unbreak few Exynos4 boards

[Przemyslaw Marczak]

Booting of Odroid U3 with SD card, ends with error:

MMC:   EXYNOS DWMMC: 0
Card did not respond to voltage select!
*** Warning - MMC init failed, using default environment

Generally this was broken, because of wrong addresses,
assigned to GPIOs.
The source of the problem was in rework of lib/fdtdec.c, after which
function fdtdec_get_addr() doesn't work as previous and function
dev_get_addr() doesn't work as expected.

The code after rework in lib/fdtdec.c assumed, that #size-cells property,
should be always greater or equal to 1, this was wrong, because it can be 0.

In case of debugging the issue, I found, that mmc clock was computed wrong,
for Exynos4, because of function get_mmc_clk(), which always returns -1 for
this SoC.

The patchset should fix booting on all Exynos4 boards, however it was tested
on: Odroid X2 / U3 / XU3 and Trats2.

Przemyslaw Marczak (3):
  fdtdec: fix parsing 'reg' property with zero value in '#size-cells'
  gpio: s5p: call: dev_get_addr() instead of fdtdec_get_addr()
  mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()

 arch/arm/mach-exynos/clock.c | 10 ++++------
 drivers/gpio/s5p_gpio.c      | 18 +++++++++++-------
 lib/fdtdec.c                 |  2 +-
 3 files changed, 16 insertions(+), 14 deletions(-)

-- 
1.9.1

[U-Boot] [PATCH V2 1/3] fdtdec: fix parsing 'reg' property with zero value in '#size-cells'

[Przemyslaw Marczak]

After rework of lib/fdtdec.c by:

commit: 02464e3 fdt: add new fdt address parsing functions

the function fdtdec_get_addr() doesn't work as previous,
because the implementation assumes that properties '#address-cells'
and '#size-cells' are equal to 1, which can be not true sometimes.

The new API introduced fdtdec_get_addr_size_auto_parent() for the 'reg'
property parsing, but the implementation assumes, that #size-cells
can't be less than 1.

This causes that the following children's 'reg' property can't be reached:

parent at 0x100 {
     #address-cells = <1>;
     #size-cells = <0>;
     children at 0x100 {
         reg = < 0x100 >;
     };
};

Change the condition value from '1' to '0', which allows parsing property
with at least zero #size-cells, fixes the issue.

Now, fdtdec_get_addr_size_auto_parent() works properly.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Acked-by: Stephen Warren <swarren@nvidia.com>
---
Changes V2:
- cleanup commit message
- add acked-by
---
 lib/fdtdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 9f0b65d..9cf57b9 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -149,7 +149,7 @@ fdt_addr_t fdtdec_get_addr_size_auto_parent(const void *blob, int parent,
 	}
 
 	ns = fdt_size_cells(blob, parent);
-	if (ns < 1) {
+	if (ns < 0) {
 		debug("(bad #size-cells)\n");
 		return FDT_ADDR_T_NONE;
 	}
-- 
1.9.1

[U-Boot] [PATCH V2 2/3] gpio: s5p: call: dev_get_addr() instead of fdtdec_get_addr()

[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 <p.marczak@samsung.com>
Acked-by: Stephen Warren <swarren@nvidia.com>
---
Changes V2:
- add acked-by
---
 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, &dev);
+				  plat->bank_name, plat, -1, &dev);
 		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] [PATCH V2 3/3] mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()

[Przemyslaw Marczak]

After rework of code by:

commit: d952796 Exynos5: Use clock_get_periph_rate generic API

function get_mmc_clk() always returns -1 for Exynos 4.

This was caused by omitting, that SDHCI driver for Exynos 4,
calls get_mmc_clk(), with mmc device number as argument,
instead of pinmux peripheral id, like DW MMC driver for Exynos 5.

By this commit, the code directly calls a proper function
to get mmc clock for Exynos 4, without checking the peripheral id.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
---
Changes V2:
- cleanup commit message
---
 arch/arm/mach-exynos/clock.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-exynos/clock.c b/arch/arm/mach-exynos/clock.c
index 1c6baa1..18eadf5 100644
--- a/arch/arm/mach-exynos/clock.c
+++ b/arch/arm/mach-exynos/clock.c
@@ -1661,6 +1661,9 @@ unsigned long get_mmc_clk(int dev_index)
 {
 	enum periph_id id;
 
+	if (cpu_is_exynos4())
+		return exynos4_get_mmc_clk(dev_index);
+
 	switch (dev_index) {
 	case 0:
 		id = PERIPH_ID_SDMMC0;
@@ -1679,12 +1682,7 @@ unsigned long get_mmc_clk(int dev_index)
 		return -1;
 	}
 
-	if (cpu_is_exynos5())
-		return clock_get_periph_rate(id);
-	else if (cpu_is_exynos4())
-		return exynos4_get_mmc_clk(dev_index);
-
-	return 0;
+	return clock_get_periph_rate(id);
 }
 
 void set_mmc_clk(int dev_index, unsigned int div)
-- 
1.9.1

[U-Boot] [PATCH V2 1/3] fdtdec: fix parsing 'reg' property with zero value in '#size-cells'

[Simon Glass]

On 28 September 2015 at 06:17, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> After rework of lib/fdtdec.c by:
>
> commit: 02464e3 fdt: add new fdt address parsing functions
>
> the function fdtdec_get_addr() doesn't work as previous,
> because the implementation assumes that properties '#address-cells'
> and '#size-cells' are equal to 1, which can be not true sometimes.
>
> The new API introduced fdtdec_get_addr_size_auto_parent() for the 'reg'
> property parsing, but the implementation assumes, that #size-cells
> can't be less than 1.
>
> This causes that the following children's 'reg' property can't be reached:
>
> parent at 0x100 {
>      #address-cells = <1>;
>      #size-cells = <0>;
>      children at 0x100 {
>          reg = < 0x100 >;
>      };
> };
>
> Change the condition value from '1' to '0', which allows parsing property
> with at least zero #size-cells, fixes the issue.
>
> Now, fdtdec_get_addr_size_auto_parent() works properly.
>
> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> Acked-by: Stephen Warren <swarren@nvidia.com>
> ---
> Changes V2:
> - cleanup commit message
> - add acked-by
> ---
>  lib/fdtdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I'll pick this series up if no one else is planning to.

Tested on snow
Tested-by: Simon Glass <sjg@chromium.org>

Acked-by: Simon Glass <sjg@chromium.org>

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

[Simon Glass]

On 25 September 2015 at 09:48, Stephen Warren <swarren@wwwdotorg.org> 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 <swarren@nvidia.com>
>>>
>>> ... 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 <sjg@chromium.org>

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH V2 3/3] mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()

[Simon Glass]

Hi,

On 28 September 2015 at 06:17, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> After rework of code by:
>
> commit: d952796 Exynos5: Use clock_get_periph_rate generic API
>
> function get_mmc_clk() always returns -1 for Exynos 4.
>
> This was caused by omitting, that SDHCI driver for Exynos 4,
> calls get_mmc_clk(), with mmc device number as argument,
> instead of pinmux peripheral id, like DW MMC driver for Exynos 5.
>
> By this commit, the code directly calls a proper function
> to get mmc clock for Exynos 4, without checking the peripheral id.
>
> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> ---
> Changes V2:
> - cleanup commit message
> ---
>  arch/arm/mach-exynos/clock.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)

I'll pick this up if needed. I tested that it does not break exynos5,
but have not tested it on exynos4.

Acked-by: Simon Glass <sjg@chromium.org>

Regars,
Simon

[U-Boot] [PATCH V2 1/3] fdtdec: fix parsing 'reg' property with zero value in '#size-cells'

[Minkyu Kang]

On 29/09/15 13:47, Simon Glass wrote:
> On 28 September 2015 at 06:17, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>> After rework of lib/fdtdec.c by:
>>
>> commit: 02464e3 fdt: add new fdt address parsing functions
>>
>> the function fdtdec_get_addr() doesn't work as previous,
>> because the implementation assumes that properties '#address-cells'
>> and '#size-cells' are equal to 1, which can be not true sometimes.
>>
>> The new API introduced fdtdec_get_addr_size_auto_parent() for the 'reg'
>> property parsing, but the implementation assumes, that #size-cells
>> can't be less than 1.
>>
>> This causes that the following children's 'reg' property can't be reached:
>>
>> parent at 0x100 {
>>      #address-cells = <1>;
>>      #size-cells = <0>;
>>      children at 0x100 {
>>          reg = < 0x100 >;
>>      };
>> };
>>
>> Change the condition value from '1' to '0', which allows parsing property
>> with at least zero #size-cells, fixes the issue.
>>
>> Now, fdtdec_get_addr_size_auto_parent() works properly.
>>
>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>> Acked-by: Stephen Warren <swarren@nvidia.com>
>> ---
>> Changes V2:
>> - cleanup commit message
>> - add acked-by
>> ---
>>  lib/fdtdec.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> I'll pick this series up if no one else is planning to.
> 
> Tested on snow
> Tested-by: Simon Glass <sjg@chromium.org>
> 
> Acked-by: Simon Glass <sjg@chromium.org>
> 

Acked-by: Minkyu Kang <mk7.kang@samsung.com>

Thanks,
Minkyu Kang.

[U-Boot] [PATCH V2 3/3] mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()

[Przemyslaw Marczak]

Hello Simon,

On 09/29/2015 06:47 AM, Simon Glass wrote:
> Hi,
>
> On 28 September 2015 at 06:17, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>> After rework of code by:
>>
>> commit: d952796 Exynos5: Use clock_get_periph_rate generic API
>>
>> function get_mmc_clk() always returns -1 for Exynos 4.
>>
>> This was caused by omitting, that SDHCI driver for Exynos 4,
>> calls get_mmc_clk(), with mmc device number as argument,
>> instead of pinmux peripheral id, like DW MMC driver for Exynos 5.
>>
>> By this commit, the code directly calls a proper function
>> to get mmc clock for Exynos 4, without checking the peripheral id.
>>
>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>> ---
>> Changes V2:
>> - cleanup commit message
>> ---
>>   arch/arm/mach-exynos/clock.c | 10 ++++------
>>   1 file changed, 4 insertions(+), 6 deletions(-)
>
> I'll pick this up if needed. I tested that it does not break exynos5,
> but have not tested it on exynos4.
>
> Acked-by: Simon Glass <sjg@chromium.org>
>
> Regars,
> Simon
>

Thank you for testing. I tested this on Trats2 / Odroid U3 and X2  - all 
based on Exynos4412. I must only check Trats (Exynos 4210), since Lukasz 
mentioned about some issue. Anyway I think, that it can be picked up.

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH V2 3/3] mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()

[Jaehoon Chung]

Hi,

On 09/30/2015 04:26 PM, Przemyslaw Marczak wrote:
> Hello Simon,
> 
> On 09/29/2015 06:47 AM, Simon Glass wrote:
>> Hi,
>>
>> On 28 September 2015 at 06:17, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>>> After rework of code by:
>>>
>>> commit: d952796 Exynos5: Use clock_get_periph_rate generic API
>>>
>>> function get_mmc_clk() always returns -1 for Exynos 4.
>>>
>>> This was caused by omitting, that SDHCI driver for Exynos 4,
>>> calls get_mmc_clk(), with mmc device number as argument,
>>> instead of pinmux peripheral id, like DW MMC driver for Exynos 5.
>>>
>>> By this commit, the code directly calls a proper function
>>> to get mmc clock for Exynos 4, without checking the peripheral id.
>>>
>>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>>> ---
>>> Changes V2:
>>> - cleanup commit message
>>> ---
>>>   arch/arm/mach-exynos/clock.c | 10 ++++------
>>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> I'll pick this up if needed. I tested that it does not break exynos5,
>> but have not tested it on exynos4.
>>
>> Acked-by: Simon Glass <sjg@chromium.org>
>>
>> Regars,
>> Simon
>>
> 
> Thank you for testing. I tested this on Trats2 / Odroid U3 and X2  - all based on Exynos4412. I must only check Trats (Exynos 4210), since Lukasz mentioned about some issue. Anyway I think, that it can be picked up.

Looks good to me.

Acked-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> 
> Best regards,

[U-Boot] [PATCH V2 3/3] mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()

[Przemyslaw Marczak]

Hello,

On 09/30/2015 09:26 AM, Przemyslaw Marczak wrote:
> Hello Simon,
>
> On 09/29/2015 06:47 AM, Simon Glass wrote:
>> Hi,
>>
>> On 28 September 2015 at 06:17, Przemyslaw Marczak
>> <p.marczak@samsung.com> wrote:
>>> After rework of code by:
>>>
>>> commit: d952796 Exynos5: Use clock_get_periph_rate generic API
>>>
>>> function get_mmc_clk() always returns -1 for Exynos 4.
>>>
>>> This was caused by omitting, that SDHCI driver for Exynos 4,
>>> calls get_mmc_clk(), with mmc device number as argument,
>>> instead of pinmux peripheral id, like DW MMC driver for Exynos 5.
>>>
>>> By this commit, the code directly calls a proper function
>>> to get mmc clock for Exynos 4, without checking the peripheral id.
>>>
>>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>>> ---
>>> Changes V2:
>>> - cleanup commit message
>>> ---
>>>   arch/arm/mach-exynos/clock.c | 10 ++++------
>>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> I'll pick this up if needed. I tested that it does not break exynos5,
>> but have not tested it on exynos4.
>>
>> Acked-by: Simon Glass <sjg@chromium.org>
>>
>> Regars,
>> Simon
>>
>
> Thank you for testing. I tested this on Trats2 / Odroid U3 and X2  - all
> based on Exynos4412. I must only check Trats (Exynos 4210), since Lukasz
> mentioned about some issue. Anyway I think, that it can be picked up.
>
> Best regards,

+ Tested on Trats (Exynos4210)

Patchset fixes, sdhci issue on this board.

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH V3 0/4] Fix fdt 'reg' parsing and unbreak few Exynos4 boards

[Przemyslaw Marczak]

Booting of Odroid U3/X2 with SD card, ends with error:

MMC:   EXYNOS DWMMC: 0
Card did not respond to voltage select!
*** Warning - MMC init failed, using default environment

Generally this was broken, because of wrong addresses,
assigned to GPIOs.
The source of the problem was in rework of lib/fdtdec.c, after which
function fdtdec_get_addr() doesn't work as previous and function
dev_get_addr() doesn't works as expected.

The code after rework in lib/fdtdec.c assumed, that #size-cells property,
should be always greater or equal to 1, this was wrong, because it can be 0.

In case of debugging the issue, I found, that mmc clock was computed wrong,
for Exynos4, because of function get_mmc_clk(), which always returns -1 for
this SoC.

The patchset should fix booting on all Exynos4 boards, however it was tested
on: Odroid X2 / U3 / XU3 and Trats / Trats2.

Przemyslaw Marczak (4):
  fdtdec: fix parsing 'reg' property with zero value in '#size-cells'
  gpio: s5p: call: dev_get_addr() instead of fdtdec_get_addr()
  mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()
  trats: fdt: disable unused DW MMC

 arch/arm/dts/exynos4210-trats.dts |  4 ++++
 arch/arm/mach-exynos/clock.c      | 10 ++++------
 drivers/gpio/s5p_gpio.c           | 18 +++++++++++-------
 lib/fdtdec.c                      |  2 +-
 4 files changed, 20 insertions(+), 14 deletions(-)

-- 
1.9.1

[U-Boot] [PATCH V3 1/4] fdtdec: fix parsing 'reg' property with zero value in '#size-cells'

[Przemyslaw Marczak]

After rework of lib/fdtdec.c by:

commit: 02464e3 fdt: add new fdt address parsing functions

the function fdtdec_get_addr() doesn't work as previous,
because the implementation assumes that properties '#address-cells'
and '#size-cells' are equal to 1, which can be not true sometimes.

The new API introduced fdtdec_get_addr_size_auto_parent() for the 'reg'
property parsing, but the implementation assumes, that #size-cells
can't be less than 1.

This causes that the following children's 'reg' property can't be reached:

parent at 0x0 {
     #address-cells = <1>;
     #size-cells = <0>;
     children at 0x100 {
         reg = < 0x100 >;
     };
};

Change the condition value from '1' to '0', which allows parsing property
with at least zero #size-cells, fixes the issue.

Now, fdtdec_get_addr_size_auto_parent() works properly.

Tested on: Odroid U3/X2, Trats, Trats2, Odroid XU3, Snow (by Simon).

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Acked-by: Stephen Warren <swarren@nvidia.com>
Acked-by: Simon Glass <sjg@chromium.org>
Tested-by: Simon Glass <sjg@chromium.org>
---
Changes V2:
- cleanup commit message
- add acked-by
Changes V3:
- add acked-by and tested-by Simon
---
 lib/fdtdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 9f0b65d..9cf57b9 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -149,7 +149,7 @@ fdt_addr_t fdtdec_get_addr_size_auto_parent(const void *blob, int parent,
 	}
 
 	ns = fdt_size_cells(blob, parent);
-	if (ns < 1) {
+	if (ns < 0) {
 		debug("(bad #size-cells)\n");
 		return FDT_ADDR_T_NONE;
 	}
-- 
1.9.1

[U-Boot] [PATCH V3 2/4] gpio: s5p: call: dev_get_addr() instead of fdtdec_get_addr()

[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.

Tested on: Odroid U3/X2, Trats, Trats2, Odroid XU3, Snow (by Simon).

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Acked-by: Stephen Warren <swarren@nvidia.com>
Acked-by: Simon Glass <sjg@chromium.org>
Tested-by: Simon Glass <sjg@chromium.org>
---
Changes V2:
- add acked-by
Changes V3:
- add acked-by and tested-by Simon
---
 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, &dev);
+				  plat->bank_name, plat, -1, &dev);
 		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] [PATCH V3 3/4] mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()

[Przemyslaw Marczak]

After rework of code by:

commit: d952796 Exynos5: Use clock_get_periph_rate generic API

function get_mmc_clk() always returns -1 for Exynos 4.

This was caused by omitting, that SDHCI driver for Exynos 4,
calls get_mmc_clk(), with mmc device number as argument,
instead of pinmux peripheral id, like DW MMC driver for Exynos 5.

By this commit, the code directly calls a proper function
to get mmc clock for Exynos 4, without checking the peripheral id.

Tested on: Odroid U3/X2, Trats, Trats2, Odroid XU3, Snow (by Simon).

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
Acked-by: Simon Glass <sjg@chromium.org>
Tested-by: Simon Glass <sjg@chromium.org>
---
Changes V2:
- cleanup commit message
Changes V3:
- add acked-by: Simon and Jaehoon and tested-by: Simon
---
 arch/arm/mach-exynos/clock.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-exynos/clock.c b/arch/arm/mach-exynos/clock.c
index 1c6baa1..18eadf5 100644
--- a/arch/arm/mach-exynos/clock.c
+++ b/arch/arm/mach-exynos/clock.c
@@ -1661,6 +1661,9 @@ unsigned long get_mmc_clk(int dev_index)
 {
 	enum periph_id id;
 
+	if (cpu_is_exynos4())
+		return exynos4_get_mmc_clk(dev_index);
+
 	switch (dev_index) {
 	case 0:
 		id = PERIPH_ID_SDMMC0;
@@ -1679,12 +1682,7 @@ unsigned long get_mmc_clk(int dev_index)
 		return -1;
 	}
 
-	if (cpu_is_exynos5())
-		return clock_get_periph_rate(id);
-	else if (cpu_is_exynos4())
-		return exynos4_get_mmc_clk(dev_index);
-
-	return 0;
+	return clock_get_periph_rate(id);
 }
 
 void set_mmc_clk(int dev_index, unsigned int div)
-- 
1.9.1

[U-Boot] [PATCH V3 4/4] trats: fdt: disable unused DW MMC

[Przemyslaw Marczak]

This device uses SDHCI driver, for eMMC and SD cards.
Trying bind the DW MMC driver with fdt node without all
required properties, causes printing an error.

This commit disables the DW MMC node.

Tested-on: Trats

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Cc: ?ukasz Majewski <l.majewski@samsung.com>
Cc: Minkyu Kang <mk7.kang@samsung.com>
--
Changes V3:
- new commit
---
 arch/arm/dts/exynos4210-trats.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/dts/exynos4210-trats.dts b/arch/arm/dts/exynos4210-trats.dts
index 36d02df..f3fac80 100644
--- a/arch/arm/dts/exynos4210-trats.dts
+++ b/arch/arm/dts/exynos4210-trats.dts
@@ -117,4 +117,8 @@
 	sdhci at 12540000 {
 		status = "disabled";
 	};
+
+	dwmmc at 12550000 {
+		status = "disabled";
+	};
 };
-- 
1.9.1

[U-Boot] [PATCH V3 0/4] Fix fdt 'reg' parsing and unbreak few Exynos4 boards

[Tom Rini]

On Wed, Sep 30, 2015 at 01:14:49PM +0200, Przemyslaw Marczak wrote:

> Booting of Odroid U3/X2 with SD card, ends with error:
> 
> MMC:   EXYNOS DWMMC: 0
> Card did not respond to voltage select!
> *** Warning - MMC init failed, using default environment
> 
> Generally this was broken, because of wrong addresses,
> assigned to GPIOs.
> The source of the problem was in rework of lib/fdtdec.c, after which
> function fdtdec_get_addr() doesn't work as previous and function
> dev_get_addr() doesn't works as expected.
> 
> The code after rework in lib/fdtdec.c assumed, that #size-cells property,
> should be always greater or equal to 1, this was wrong, because it can be 0.
> 
> In case of debugging the issue, I found, that mmc clock was computed wrong,
> for Exynos4, because of function get_mmc_clk(), which always returns -1 for
> this SoC.
> 
> The patchset should fix booting on all Exynos4 boards, however it was tested
> on: Odroid X2 / U3 / XU3 and Trats / Trats2.
> 
> Przemyslaw Marczak (4):
>   fdtdec: fix parsing 'reg' property with zero value in '#size-cells'
>   gpio: s5p: call: dev_get_addr() instead of fdtdec_get_addr()
>   mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()
>   trats: fdt: disable unused DW MMC
> 
>  arch/arm/dts/exynos4210-trats.dts |  4 ++++
>  arch/arm/mach-exynos/clock.c      | 10 ++++------
>  drivers/gpio/s5p_gpio.c           | 18 +++++++++++-------
>  lib/fdtdec.c                      |  2 +-
>  4 files changed, 20 insertions(+), 14 deletions(-)

Should I grab this directly or expect a PR from the DT or Samsung tree?
Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150930/22ab0067/attachment.sig>

[U-Boot] [PATCH V3 0/4] Fix fdt 'reg' parsing and unbreak few Exynos4 boards

[Przemyslaw Marczak]

Hello Tom, Simon,

On 09/30/2015 03:13 PM, Tom Rini wrote:
> On Wed, Sep 30, 2015 at 01:14:49PM +0200, Przemyslaw Marczak wrote:
>
>> Booting of Odroid U3/X2 with SD card, ends with error:
>>
>> MMC:   EXYNOS DWMMC: 0
>> Card did not respond to voltage select!
>> *** Warning - MMC init failed, using default environment
>>
>> Generally this was broken, because of wrong addresses,
>> assigned to GPIOs.
>> The source of the problem was in rework of lib/fdtdec.c, after which
>> function fdtdec_get_addr() doesn't work as previous and function
>> dev_get_addr() doesn't works as expected.
>>
>> The code after rework in lib/fdtdec.c assumed, that #size-cells property,
>> should be always greater or equal to 1, this was wrong, because it can be 0.
>>
>> In case of debugging the issue, I found, that mmc clock was computed wrong,
>> for Exynos4, because of function get_mmc_clk(), which always returns -1 for
>> this SoC.
>>
>> The patchset should fix booting on all Exynos4 boards, however it was tested
>> on: Odroid X2 / U3 / XU3 and Trats / Trats2.
>>
>> Przemyslaw Marczak (4):
>>    fdtdec: fix parsing 'reg' property with zero value in '#size-cells'
>>    gpio: s5p: call: dev_get_addr() instead of fdtdec_get_addr()
>>    mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()
>>    trats: fdt: disable unused DW MMC
>>
>>   arch/arm/dts/exynos4210-trats.dts |  4 ++++
>>   arch/arm/mach-exynos/clock.c      | 10 ++++------
>>   drivers/gpio/s5p_gpio.c           | 18 +++++++++++-------
>>   lib/fdtdec.c                      |  2 +-
>>   4 files changed, 20 insertions(+), 14 deletions(-)
>
> Should I grab this directly or expect a PR from the DT or Samsung tree?
> Thanks!
>

If this is not a problem for you, then it will be nice :)

Simon,
Is that good to you?

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH V3 0/4] Fix fdt 'reg' parsing and unbreak few Exynos4 boards

[Simon Glass]

Hi Przemyslaw,

On 30 September 2015 at 06:25, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> Hello Tom, Simon,
>
>
> On 09/30/2015 03:13 PM, Tom Rini wrote:
>>
>> On Wed, Sep 30, 2015 at 01:14:49PM +0200, Przemyslaw Marczak wrote:
>>
>>> Booting of Odroid U3/X2 with SD card, ends with error:
>>>
>>> MMC:   EXYNOS DWMMC: 0
>>> Card did not respond to voltage select!
>>> *** Warning - MMC init failed, using default environment
>>>
>>> Generally this was broken, because of wrong addresses,
>>> assigned to GPIOs.
>>> The source of the problem was in rework of lib/fdtdec.c, after which
>>> function fdtdec_get_addr() doesn't work as previous and function
>>> dev_get_addr() doesn't works as expected.
>>>
>>> The code after rework in lib/fdtdec.c assumed, that #size-cells property,
>>> should be always greater or equal to 1, this was wrong, because it can be
>>> 0.
>>>
>>> In case of debugging the issue, I found, that mmc clock was computed
>>> wrong,
>>> for Exynos4, because of function get_mmc_clk(), which always returns -1
>>> for
>>> this SoC.
>>>
>>> The patchset should fix booting on all Exynos4 boards, however it was
>>> tested
>>> on: Odroid X2 / U3 / XU3 and Trats / Trats2.
>>>
>>> Przemyslaw Marczak (4):
>>>    fdtdec: fix parsing 'reg' property with zero value in '#size-cells'
>>>    gpio: s5p: call: dev_get_addr() instead of fdtdec_get_addr()
>>>    mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()
>>>    trats: fdt: disable unused DW MMC
>>>
>>>   arch/arm/dts/exynos4210-trats.dts |  4 ++++
>>>   arch/arm/mach-exynos/clock.c      | 10 ++++------
>>>   drivers/gpio/s5p_gpio.c           | 18 +++++++++++-------
>>>   lib/fdtdec.c                      |  2 +-
>>>   4 files changed, 20 insertions(+), 14 deletions(-)
>>
>>
>> Should I grab this directly or expect a PR from the DT or Samsung tree?
>> Thanks!
>>
>
> If this is not a problem for you, then it will be nice :)
>
> Simon,
> Is that good to you?

Yes, thank you both.

Regards,
Simon

[U-Boot] [PATCH V3 4/4] trats: fdt: disable unused DW MMC

[Jaehoon Chung]

Hi, Przemyslaw.

On 09/30/2015 08:14 PM, Przemyslaw Marczak wrote:
> This device uses SDHCI driver, for eMMC and SD cards.
> Trying bind the DW MMC driver with fdt node without all
> required properties, causes printing an error.
> 
> This commit disables the DW MMC node.

Why does it need?
Trats board doesn't support the Designware IP, so i think right that it shouldn't build.

If needs to modify, exynos-common.h should be modified.

> 
> Tested-on: Trats
> 
> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> Cc: ?ukasz Majewski <l.majewski@samsung.com>
> Cc: Minkyu Kang <mk7.kang@samsung.com>
> --
> Changes V3:
> - new commit
> ---
>  arch/arm/dts/exynos4210-trats.dts | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/dts/exynos4210-trats.dts b/arch/arm/dts/exynos4210-trats.dts
> index 36d02df..f3fac80 100644
> --- a/arch/arm/dts/exynos4210-trats.dts
> +++ b/arch/arm/dts/exynos4210-trats.dts
> @@ -117,4 +117,8 @@
>  	sdhci at 12540000 {
>  		status = "disabled";
>  	};
> +
> +	dwmmc at 12550000 {
> +		status = "disabled";
> +	};

It seems to support dwmmc controller. 12550000 addr is for sdhci controller.

Best Regards,
Jaehoon Chung

>  };
> 

[U-Boot] [PATCH V3 4/4] trats: fdt: disable unused DW MMC

[Przemyslaw Marczak]

Hello Jaehoon,

On 10/01/2015 05:37 AM, Jaehoon Chung wrote:
> Hi, Przemyslaw.
>
> On 09/30/2015 08:14 PM, Przemyslaw Marczak wrote:
>> This device uses SDHCI driver, for eMMC and SD cards.
>> Trying bind the DW MMC driver with fdt node without all
>> required properties, causes printing an error.
>>
>> This commit disables the DW MMC node.
>
> Why does it need?
> Trats board doesn't support the Designware IP, so i think right that it shouldn't build.
>
> If needs to modify, exynos-common.h should be modified.
>

I think, that some day, we will have a single config, for at least 
exynos5 and exynos4 (if doesn't exceed the size limit), so using the 
generic configuration is reasonable here.

Trats is based on Exynos4210, which supports this IP, and I checked the 
documentation, the address 0x12550000 is proper - Mobile Storage Host.

For a long time it wasn't enable on this device, and only printed an 
error, that 'bus-width' not found. I tried to enable this, but it 
doesn't work for the same settings as for Trats2. Now I don't have time 
to debug why, so it can be disabled.

>>
>> Tested-on: Trats
>>
>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>> Cc: ?ukasz Majewski <l.majewski@samsung.com>
>> Cc: Minkyu Kang <mk7.kang@samsung.com>
>> --
>> Changes V3:
>> - new commit
>> ---
>>   arch/arm/dts/exynos4210-trats.dts | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm/dts/exynos4210-trats.dts b/arch/arm/dts/exynos4210-trats.dts
>> index 36d02df..f3fac80 100644
>> --- a/arch/arm/dts/exynos4210-trats.dts
>> +++ b/arch/arm/dts/exynos4210-trats.dts
>> @@ -117,4 +117,8 @@
>>   	sdhci at 12540000 {
>>   		status = "disabled";
>>   	};
>> +
>> +	dwmmc at 12550000 {
>> +		status = "disabled";
>> +	};
>
> It seems to support dwmmc controller. 12550000 addr is for sdhci controller.

Please check manual for E4210, I'm sure it's right.

>
> Best Regards,
> Jaehoon Chung
>
>>   };
>>
>
>

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH V3 4/4] trats: fdt: disable unused DW MMC

[Jaehoon Chung]

Hi,

On 10/01/2015 04:11 PM, Przemyslaw Marczak wrote:
> Hello Jaehoon,
> 
> On 10/01/2015 05:37 AM, Jaehoon Chung wrote:
>> Hi, Przemyslaw.
>>
>> On 09/30/2015 08:14 PM, Przemyslaw Marczak wrote:
>>> This device uses SDHCI driver, for eMMC and SD cards.
>>> Trying bind the DW MMC driver with fdt node without all
>>> required properties, causes printing an error.
>>>
>>> This commit disables the DW MMC node.
>>
>> Why does it need?
>> Trats board doesn't support the Designware IP, so i think right that it shouldn't build.
>>
>> If needs to modify, exynos-common.h should be modified.
>>
> 
> I think, that some day, we will have a single config, for at least exynos5 and exynos4 (if doesn't exceed the size limit), so using the generic configuration is reasonable here.

Single config? Well, if do so, it will be great..not yet.

> 
> Trats is based on Exynos4210, which supports this IP, and I checked the documentation, the address 0x12550000 is proper - Mobile Storage Host.

Sorry..I have confused with C110. :)

Best Regards,
Jaehoon Chung

> 
> For a long time it wasn't enable on this device, and only printed an error, that 'bus-width' not found. I tried to enable this, but it doesn't work for the same settings as for Trats2. Now I don't have time to debug why, so it can be disabled.
> 
>>>
>>> Tested-on: Trats
>>>
>>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>>> Cc: ?ukasz Majewski <l.majewski@samsung.com>
>>> Cc: Minkyu Kang <mk7.kang@samsung.com>
>>> -- 
>>> Changes V3:
>>> - new commit
>>> ---
>>>   arch/arm/dts/exynos4210-trats.dts | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/arch/arm/dts/exynos4210-trats.dts b/arch/arm/dts/exynos4210-trats.dts
>>> index 36d02df..f3fac80 100644
>>> --- a/arch/arm/dts/exynos4210-trats.dts
>>> +++ b/arch/arm/dts/exynos4210-trats.dts
>>> @@ -117,4 +117,8 @@
>>>       sdhci at 12540000 {
>>>           status = "disabled";
>>>       };
>>> +
>>> +    dwmmc at 12550000 {
>>> +        status = "disabled";
>>> +    };
>>
>> It seems to support dwmmc controller. 12550000 addr is for sdhci controller.
> 
> Please check manual for E4210, I'm sure it's right.
> 
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>   };
>>>
>>
>>
> 
> Best regards,

[U-Boot] [PATCH V3 0/4] Fix fdt 'reg' parsing and unbreak few Exynos4 boards

[Simon Glass]

Hi,

On 30 September 2015 at 19:30, Simon Glass <sjg@chromium.org> wrote:
> Hi Przemyslaw,
>
> On 30 September 2015 at 06:25, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>> Hello Tom, Simon,
>>
>>
>> On 09/30/2015 03:13 PM, Tom Rini wrote:
>>>
>>> On Wed, Sep 30, 2015 at 01:14:49PM +0200, Przemyslaw Marczak wrote:
>>>
>>>> Booting of Odroid U3/X2 with SD card, ends with error:
>>>>
>>>> MMC:   EXYNOS DWMMC: 0
>>>> Card did not respond to voltage select!
>>>> *** Warning - MMC init failed, using default environment
>>>>
>>>> Generally this was broken, because of wrong addresses,
>>>> assigned to GPIOs.
>>>> The source of the problem was in rework of lib/fdtdec.c, after which
>>>> function fdtdec_get_addr() doesn't work as previous and function
>>>> dev_get_addr() doesn't works as expected.
>>>>
>>>> The code after rework in lib/fdtdec.c assumed, that #size-cells property,
>>>> should be always greater or equal to 1, this was wrong, because it can be
>>>> 0.
>>>>
>>>> In case of debugging the issue, I found, that mmc clock was computed
>>>> wrong,
>>>> for Exynos4, because of function get_mmc_clk(), which always returns -1
>>>> for
>>>> this SoC.
>>>>
>>>> The patchset should fix booting on all Exynos4 boards, however it was
>>>> tested
>>>> on: Odroid X2 / U3 / XU3 and Trats / Trats2.
>>>>
>>>> Przemyslaw Marczak (4):
>>>>    fdtdec: fix parsing 'reg' property with zero value in '#size-cells'
>>>>    gpio: s5p: call: dev_get_addr() instead of fdtdec_get_addr()
>>>>    mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()
>>>>    trats: fdt: disable unused DW MMC
>>>>
>>>>   arch/arm/dts/exynos4210-trats.dts |  4 ++++
>>>>   arch/arm/mach-exynos/clock.c      | 10 ++++------
>>>>   drivers/gpio/s5p_gpio.c           | 18 +++++++++++-------
>>>>   lib/fdtdec.c                      |  2 +-
>>>>   4 files changed, 20 insertions(+), 14 deletions(-)
>>>
>>>
>>> Should I grab this directly or expect a PR from the DT or Samsung tree?
>>> Thanks!
>>>
>>
>> If this is not a problem for you, then it will be nice :)
>>
>> Simon,
>> Is that good to you?
>
> Yes, thank you both.

Hmm I'm going to pick this up for the DT tree as there's another patch
needed from Stephen also. Tom if you already have this locally let me
know.

Regards,
Simon

[U-Boot] [PATCH V3 1/4] fdtdec: fix parsing 'reg' property with zero value in '#size-cells'

[Simon Glass]

On 30 September 2015 at 12:14, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> After rework of lib/fdtdec.c by:
>
> commit: 02464e3 fdt: add new fdt address parsing functions
>
> the function fdtdec_get_addr() doesn't work as previous,
> because the implementation assumes that properties '#address-cells'
> and '#size-cells' are equal to 1, which can be not true sometimes.
>
> The new API introduced fdtdec_get_addr_size_auto_parent() for the 'reg'
> property parsing, but the implementation assumes, that #size-cells
> can't be less than 1.
>
> This causes that the following children's 'reg' property can't be reached:
>
> parent at 0x0 {
>      #address-cells = <1>;
>      #size-cells = <0>;
>      children at 0x100 {
>          reg = < 0x100 >;
>      };
> };
>
> Change the condition value from '1' to '0', which allows parsing property
> with at least zero #size-cells, fixes the issue.
>
> Now, fdtdec_get_addr_size_auto_parent() works properly.
>
> Tested on: Odroid U3/X2, Trats, Trats2, Odroid XU3, Snow (by Simon).
>
> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> Acked-by: Stephen Warren <swarren@nvidia.com>
> Acked-by: Simon Glass <sjg@chromium.org>
> Tested-by: Simon Glass <sjg@chromium.org>
> ---
> Changes V2:
> - cleanup commit message
> - add acked-by
> Changes V3:
> - add acked-by and tested-by Simon
> ---
>  lib/fdtdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to u-boot-fdt, thanks!

[U-Boot] [PATCH V3 2/4] gpio: s5p: call: dev_get_addr() instead of fdtdec_get_addr()

[Simon Glass]

On 30 September 2015 at 12:14, Przemyslaw Marczak <p.marczak@samsung.com> 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.
>
> Tested on: Odroid U3/X2, Trats, Trats2, Odroid XU3, Snow (by Simon).
>
> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> Acked-by: Stephen Warren <swarren@nvidia.com>
> Acked-by: Simon Glass <sjg@chromium.org>
> Tested-by: Simon Glass <sjg@chromium.org>
> ---
> Changes V2:
> - add acked-by
> Changes V3:
> - add acked-by and tested-by Simon
> ---
>  drivers/gpio/s5p_gpio.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)

Applied to u-boot-fdt, thanks!

[U-Boot] [PATCH V3 3/4] mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()

[Simon Glass]

On 30 September 2015 at 12:14, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> After rework of code by:
>
> commit: d952796 Exynos5: Use clock_get_periph_rate generic API
>
> function get_mmc_clk() always returns -1 for Exynos 4.
>
> This was caused by omitting, that SDHCI driver for Exynos 4,
> calls get_mmc_clk(), with mmc device number as argument,
> instead of pinmux peripheral id, like DW MMC driver for Exynos 5.
>
> By this commit, the code directly calls a proper function
> to get mmc clock for Exynos 4, without checking the peripheral id.
>
> Tested on: Odroid U3/X2, Trats, Trats2, Odroid XU3, Snow (by Simon).
>
> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
> Acked-by: Simon Glass <sjg@chromium.org>
> Tested-by: Simon Glass <sjg@chromium.org>
> ---
> Changes V2:
> - cleanup commit message
> Changes V3:
> - add acked-by: Simon and Jaehoon and tested-by: Simon
> ---
>  arch/arm/mach-exynos/clock.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)

Applied to u-boot-fdt, thanks!

[U-Boot] [PATCH V3 4/4] trats: fdt: disable unused DW MMC

[Simon Glass]

On 1 October 2015 at 08:22, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> Hi,
>
> On 10/01/2015 04:11 PM, Przemyslaw Marczak wrote:
>> Hello Jaehoon,
>>
>> On 10/01/2015 05:37 AM, Jaehoon Chung wrote:
>>> Hi, Przemyslaw.
>>>
>>> On 09/30/2015 08:14 PM, Przemyslaw Marczak wrote:
>>>> This device uses SDHCI driver, for eMMC and SD cards.
>>>> Trying bind the DW MMC driver with fdt node without all
>>>> required properties, causes printing an error.
>>>>
>>>> This commit disables the DW MMC node.
>>>
>>> Why does it need?
>>> Trats board doesn't support the Designware IP, so i think right that it shouldn't build.
>>>
>>> If needs to modify, exynos-common.h should be modified.
>>>
>>
>> I think, that some day, we will have a single config, for at least exynos5 and exynos4 (if doesn't exceed the size limit), so using the generic configuration is reasonable here.
>
> Single config? Well, if do so, it will be great..not yet.
>
>>
>> Trats is based on Exynos4210, which supports this IP, and I checked the documentation, the address 0x12550000 is proper - Mobile Storage Host.
>
> Sorry..I have confused with C110. :)
>
> Best Regards,
> Jaehoon Chung

Applied to u-boot-fdt, thanks!

[U-Boot] [PATCH V3 0/4] Fix fdt 'reg' parsing and unbreak few Exynos4 boards

[Przemyslaw Marczak]

Hi Simon,

On 10/03/2015 03:36 PM, Simon Glass wrote:
> Hi,
>
> On 30 September 2015 at 19:30, Simon Glass <sjg@chromium.org> wrote:
>> Hi Przemyslaw,
>>
>> On 30 September 2015 at 06:25, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>>> Hello Tom, Simon,
>>>
>>>
>>> On 09/30/2015 03:13 PM, Tom Rini wrote:
>>>>
>>>> On Wed, Sep 30, 2015 at 01:14:49PM +0200, Przemyslaw Marczak wrote:
>>>>
>>>>> Booting of Odroid U3/X2 with SD card, ends with error:
>>>>>
>>>>> MMC:   EXYNOS DWMMC: 0
>>>>> Card did not respond to voltage select!
>>>>> *** Warning - MMC init failed, using default environment
>>>>>
>>>>> Generally this was broken, because of wrong addresses,
>>>>> assigned to GPIOs.
>>>>> The source of the problem was in rework of lib/fdtdec.c, after which
>>>>> function fdtdec_get_addr() doesn't work as previous and function
>>>>> dev_get_addr() doesn't works as expected.
>>>>>
>>>>> The code after rework in lib/fdtdec.c assumed, that #size-cells property,
>>>>> should be always greater or equal to 1, this was wrong, because it can be
>>>>> 0.
>>>>>
>>>>> In case of debugging the issue, I found, that mmc clock was computed
>>>>> wrong,
>>>>> for Exynos4, because of function get_mmc_clk(), which always returns -1
>>>>> for
>>>>> this SoC.
>>>>>
>>>>> The patchset should fix booting on all Exynos4 boards, however it was
>>>>> tested
>>>>> on: Odroid X2 / U3 / XU3 and Trats / Trats2.
>>>>>
>>>>> Przemyslaw Marczak (4):
>>>>>     fdtdec: fix parsing 'reg' property with zero value in '#size-cells'
>>>>>     gpio: s5p: call: dev_get_addr() instead of fdtdec_get_addr()
>>>>>     mach-exynos: clock: restore calling dead exynos4_get_mmc_clk()
>>>>>     trats: fdt: disable unused DW MMC
>>>>>
>>>>>    arch/arm/dts/exynos4210-trats.dts |  4 ++++
>>>>>    arch/arm/mach-exynos/clock.c      | 10 ++++------
>>>>>    drivers/gpio/s5p_gpio.c           | 18 +++++++++++-------
>>>>>    lib/fdtdec.c                      |  2 +-
>>>>>    4 files changed, 20 insertions(+), 14 deletions(-)
>>>>
>>>>
>>>> Should I grab this directly or expect a PR from the DT or Samsung tree?
>>>> Thanks!
>>>>
>>>
>>> If this is not a problem for you, then it will be nice :)
>>>
>>> Simon,
>>> Is that good to you?
>>
>> Yes, thank you both.
>
> Hmm I'm going to pick this up for the DT tree as there's another patch
> needed from Stephen also. Tom if you already have this locally let me
> know.
>
> Regards,
> Simon
>

Ok, thanks.

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com