Fix fdt 'reg' parsing and unbreak Odroid U3

After rework of lib/fdtdec.c by commit:

commit 02464e386bb5f0a022c121f95ae75cf583759d95

Author: Stephen Warren <[email protected]>
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.

Quoted 5 lines · Przemyslaw Marczak

After rework of lib/fdtdec.c by commit:

commit 02464e386bb5f0a022c121f95ae75cf583759d95

Author: Stephen Warren <[email protected]>
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:-(

Quoted 3 lines

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.

Quoted 15 lines

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 >;
      };
};

Quoted 4 lines

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 <[email protected]>

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

Hello Stephen,

Quoted 10 lines · Stephen Warren

Quoted 5 lines · Przemyslaw Marczak

After rework of lib/fdtdec.c by commit:

commit 02464e386bb5f0a022c121f95ae75cf583759d95

Author: Stephen Warren <[email protected]>
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.

Quoted 2 lines

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

Oh no no... maybe a little :)

Quoted 7 lines

Quoted 3 lines

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.

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.

Quoted 6 lines · 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.

Quoted 6 lines

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 <[email protected]>

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

Hello Stephen,

Quoted 17 lines · Stephen Warren

Quoted 6 lines · 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.

Quoted 6 lines

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,

Quoted 7 lines

Acked-by Stephen Warren <[email protected]>

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

Quoted 31 lines

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;

Quoted 3 lines

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

Quoted 71 lines · Przemyslaw Marczak

Hello Stephen,

Quoted 20 lines · Stephen Warren

Quoted 7 lines · 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.

Quoted 6 lines

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,

Quoted 7 lines

Acked-by Stephen Warren <[email protected]>

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

Quoted 31 lines

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;

Quoted 8 lines

Quoted 3 lines

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

Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 2 lines

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.

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(-)

Hi, Przemyslaw.

Quoted 6 lines · 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

Quoted 1 line

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

Quoted 20 lines

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()

Quoted 5 lines

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

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(-)

Hello,

Quoted 26 lines · 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

Quoted 9 lines

     '#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,

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 <[email protected]>

Acked-by Stephen Warren <[email protected]>

---
Changes V2:
- cleanup commit message
- add acked-by

---
 lib/fdtdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 29 lines · 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 <[email protected]>

Acked-by Stephen Warren <[email protected]>

Quoted 7 lines

---
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 <[email protected]>

Acked-by Simon Glass <[email protected]>

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 <[email protected]>

---
Changes V2:
- cleanup commit message

---
 arch/arm/mach-exynos/clock.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Hi,

Quoted 13 lines · 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.

Quoted 7 lines

Signed-off-by Przemyslaw Marczak <[email protected]>

---
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 <[email protected]>

Regars,
Simon

Hello Simon,

Quoted 27 lines · Simon Glass

Hi,

Quoted 13 lines · 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.

Quoted 7 lines

Signed-off-by Przemyslaw Marczak <[email protected]>

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

Quoted 5 lines

Acked-by Simon Glass <[email protected]>

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,

Hello Simon,

Quoted 25 lines · Simon Glass

Hi,

Quoted 13 lines · 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.

Quoted 7 lines

Signed-off-by Przemyslaw Marczak <[email protected]>

---
Changes V2:
- cleanup commit message

---
  arch/arm/mach-exynos/clock.c | 10 ++++------
  1 file changed, 4 insertions(+), 6 deletions(-)

Quoted 8 lines

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 <[email protected]>

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,

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 <[email protected]>

Acked-by Stephen Warren <[email protected]>

Acked-by Simon Glass <[email protected]>

Tested-by Simon Glass <[email protected]>

---
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(-)

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 <[email protected]>

Acked-by Stephen Warren <[email protected]>

Acked-by Simon Glass <[email protected]>

Tested-by Simon Glass <[email protected]>

---
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(-)

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 <[email protected]>

Acked-by Jaehoon Chung <[email protected]>

Acked-by Simon Glass <[email protected]>

Tested-by Simon Glass <[email protected]>

---
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(-)

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 <[email protected]>

Cc ?ukasz Majewski <[email protected]>

Cc Minkyu Kang <[email protected]>

--
Changes V3:
- new commit

---
 arch/arm/dts/exynos4210-trats.dts | 4 ++++
 1 file changed, 4 insertions(+)

arch/arm/dts/exynos4210-trats.dts +0 -0

index 36d02df..f3fac80 100644

arch/arm/dts/exynos4210-trats.dts +0 -0

 };

Hi, Przemyslaw.

Quoted 4 lines · 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.

Quoted 1 line

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.

Quoted 13 lines

Tested-on: Trats

Signed-off-by Przemyslaw Marczak <[email protected]>

Cc ?ukasz Majewski <[email protected]>

Cc Minkyu Kang <[email protected]>

--
Changes V3:
- new commit

---
  arch/arm/dts/exynos4210-trats.dts | 4 ++++
  1 file changed, 4 insertions(+)

arch/arm/dts/exynos4210-trats.dts line 123

Quoted 12 lines

arch/arm/dts/exynos4210-trats.dts +4 -0

index 36d02df..f3fac80 100644
@@ -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

Quoted 2 lines

arch/arm/dts/exynos4210-trats.dts +0 -0

  };

Hello Jaehoon,

Quoted 9 lines · Jaehoon Chung

Hi, Przemyslaw.

Quoted 5 lines · 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.

Quoted 5 lines

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.

Quoted 13 lines

Quoted 13 lines

Tested-on: Trats

Signed-off-by Przemyslaw Marczak <[email protected]>

Cc ?ukasz Majewski <[email protected]>

Cc Minkyu Kang <[email protected]>

Signature · 5 lines

Changes V3:
- new commit
---
  arch/arm/dts/exynos4210-trats.dts | 4 ++++
  1 file changed, 4 insertions(+)

Quoted 14 lines

arch/arm/dts/exynos4210-trats.dts line 123

Quoted 12 lines

arch/arm/dts/exynos4210-trats.dts +4 -0

index 36d02df..f3fac80 100644
@@ -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.

Quoted 8 lines

Best Regards,
Jaehoon Chung

Quoted 2 lines

arch/arm/dts/exynos4210-trats.dts +0 -0

  };

Best regards,

Hi,

Quoted 12 lines · Przemyslaw Marczak

Hello Jaehoon,

Quoted 9 lines · Jaehoon Chung

Hi, Przemyslaw.

Quoted 5 lines · 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.

Quoted 7 lines

Quoted 5 lines

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.

Quoted 1 line

Quoted 1 line

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

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(-)

Quoted 28 lines · 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

Quoted 5 lines

  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!

Hello Tom, Simon,

Quoted 42 lines · Tom Rini

Quoted 37 lines · 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(-)

Quoted 3 lines

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?

Hi Przemyslaw,

Quoted 50 lines · Przemyslaw Marczak

Hello Tom, Simon,

Quoted 45 lines · Tom Rini

Quoted 32 lines · 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

Quoted 5 lines

  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!

Quoted 4 lines

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

Simon,
Is that good to you?

Yes, thank you both.

Hi,

Quoted 58 lines · Simon Glass

Hi Przemyslaw,

Quoted 54 lines · Przemyslaw Marczak

Hello Tom, Simon,

Quoted 42 lines · Tom Rini

Quoted 32 lines · 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

Quoted 5 lines

   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(-)

Quoted 3 lines

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?

Quoted 1 line

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