[U-Boot] [PATCH 0/3] Fix fdt 'reg' parsing and unbreak Odroid U3
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(-)
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@0x0 { #address-cells = <1>; #size-cells = <0>; children@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; }
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@0x0 { #address-cells = <1>; #size-cells = <0>; children@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.)
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@0x0 { #address-cells = <1>; #size-cells = <0>; children@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,
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.
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;
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.
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,
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.
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
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)
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(-)
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,
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,
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,
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(-)
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@0x100 { #address-cells = <1>; #size-cells = <0>; children@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; }
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@0x100 { #address-cells = <1>; #size-cells = <0>; children@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
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@0x100 { #address-cells = <1>; #size-cells = <0>; children@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.
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;
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)
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
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,
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,
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,
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(-)
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@0x0 { #address-cells = <1>; #size-cells = <0>; children@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; }
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@0x0 { #address-cells = <1>; #size-cells = <0>; children@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!
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;
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!
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)
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!
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@12540000 { status = "disabled"; }; + + dwmmc@12550000 { + status = "disabled"; + }; };
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@12540000 { status = "disabled"; };
- dwmmc@12550000 {
status = "disabled";- };
It seems to support dwmmc controller. 12550000 addr is for sdhci controller.
Best Regards, Jaehoon Chung
};
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@12540000 { status = "disabled"; };
- dwmmc@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,
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@12540000 { status = "disabled"; };
- dwmmc@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,
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!
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!
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,
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
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
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,
participants (6)
-
Jaehoon Chung -
Minkyu Kang -
Przemyslaw Marczak -
Simon Glass -
Stephen Warren -
Tom Rini