[U-Boot] [PATCH v2] drivers: core: Add translation in live tree case

[U-Boot] [PATCH v2] drivers: core: Add translation in live tree case

[Mario Six]

The function dev_read_addr calls ofnode_get_addr_index in the live tree
case, which does not apply bus translations to the address read from the
device tree. This results in illegal addresses on boards that rely on
bus translations being applied.

Fix this situation by applying bus translations in the live tree case as
well.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---

Changes v1 -> v2:
* Added IS_ENABLED(CONFIG_OF_TRANSLATE) case distinction

---
 drivers/core/ofnode.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
index 0030ab962e..513bebd32f 100644
--- a/drivers/core/ofnode.c
+++ b/drivers/core/ofnode.c
@@ -200,13 +200,19 @@ fdt_addr_t ofnode_get_addr_index(ofnode node, int index)
 		uint flags;
 		u64 size;
 		int na;
+		__be32 addr;

 		prop_val = of_get_address(ofnode_to_np(node), index, &size,
 					  &flags);
 		if (!prop_val)
 			return FDT_ADDR_T_NONE;
 		na = of_n_addr_cells(ofnode_to_np(node));
-		return of_read_number(prop_val, na);
+		addr = of_read_number(prop_val, na);
+
+		if (IS_ENABLED(CONFIG_OF_TRANSLATE))
+			return of_translate_address(ofnode_to_np(node), &addr);
+		else
+			return addr;
 	} else {
 		return fdt_get_base_address(gd->fdt_blob,
 					    ofnode_to_offset(node));
--
2.11.0

[U-Boot] [PATCH v2] drivers: core: Add translation in live tree case

[Simon Glass]

On 23 November 2017 at 23:51, Mario Six <mario.six@gdsys.cc> wrote:
> The function dev_read_addr calls ofnode_get_addr_index in the live tree
> case, which does not apply bus translations to the address read from the
> device tree. This results in illegal addresses on boards that rely on
> bus translations being applied.
>
> Fix this situation by applying bus translations in the live tree case as
> well.
>
> Signed-off-by: Mario Six <mario.six@gdsys.cc>
> ---
>
> Changes v1 -> v2:
> * Added IS_ENABLED(CONFIG_OF_TRANSLATE) case distinction
>
> ---
>  drivers/core/ofnode.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

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

[U-Boot] [PATCH v2] drivers: core: Add translation in live tree case

[sjg at google.com]

On 23 November 2017 at 23:51, Mario Six <mario.six@gdsys.cc> wrote:
> The function dev_read_addr calls ofnode_get_addr_index in the live tree
> case, which does not apply bus translations to the address read from the
> device tree. This results in illegal addresses on boards that rely on
> bus translations being applied.
>
> Fix this situation by applying bus translations in the live tree case as
> well.
>
> Signed-off-by: Mario Six <mario.six@gdsys.cc>
> ---
>
> Changes v1 -> v2:
> * Added IS_ENABLED(CONFIG_OF_TRANSLATE) case distinction
>
> ---
>  drivers/core/ofnode.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

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

Applied to u-boot-dm thanks!

[U-Boot] [PATCH v2] drivers: core: Add translation in live tree case

[Simon Glass]

+Stephen, Tom

Hi Mario,

I've had to drop this since it breaks tegra. Stephen feels that this
is likely a bug in the patch rather than anything wrong with Tegra. Do
you have any thoughts? I can potentially try things out on the Tegra
boards I have.

Regards,
Simon


On 8 December 2017 at 10:11,  <sjg@google.com> wrote:
> On 23 November 2017 at 23:51, Mario Six <mario.six@gdsys.cc> wrote:
>> The function dev_read_addr calls ofnode_get_addr_index in the live tree
>> case, which does not apply bus translations to the address read from the
>> device tree. This results in illegal addresses on boards that rely on
>> bus translations being applied.
>>
>> Fix this situation by applying bus translations in the live tree case as
>> well.
>>
>> Signed-off-by: Mario Six <mario.six@gdsys.cc>
>> ---
>>
>> Changes v1 -> v2:
>> * Added IS_ENABLED(CONFIG_OF_TRANSLATE) case distinction
>>
>> ---
>>  drivers/core/ofnode.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Applied to u-boot-dm thanks!

[U-Boot] [PATCH v2] drivers: core: Add translation in live tree case

[Mario Six]

Hi Simon,

On Thu, Dec 14, 2017 at 9:36 PM, Simon Glass <sjg@chromium.org> wrote:
> +Stephen, Tom
>
> Hi Mario,
>
> I've had to drop this since it breaks tegra. Stephen feels that this
> is likely a bug in the patch rather than anything wrong with Tegra. Do
> you have any thoughts? I can potentially try things out on the Tegra
> boards I have.
>
> Regards,
> Simon
>

I think I've found the problem: It's an endianness issue. The
of_translate_address function seems to return a raw big-endian address, while
the of_read_number function translates it to CPU endianness before returning
it. On the MPC8308 I tested on this made no difference, but little-endian
systems will have problems. When I put a bogus dev_read_addr in the sandbox
serial driver, I see the effect as well.

Here's a hopefully fixed version (just the diff; I'll post a proper patch when
your tests succeeded); it corrects the issue on sandbox, so I hope it will work
on tegra as well:

----- >8 -----

diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
index 0030ab962e..ca22d09ff2 100644
--- a/drivers/core/ofnode.c
+++ b/drivers/core/ofnode.c
@@ -200,13 +200,22 @@ fdt_addr_t ofnode_get_addr_index(ofnode node, int index)
                uint flags;
                u64 size;
                int na;
+               __be32 addr;

                prop_val = of_get_address(ofnode_to_np(node), index, &size,
                                          &flags);
                if (!prop_val)
                        return FDT_ADDR_T_NONE;
                na = of_n_addr_cells(ofnode_to_np(node));
-               return of_read_number(prop_val, na);
+               addr = of_read_number(prop_val, na);
+
+               if (IS_ENABLED(CONFIG_OF_TRANSLATE)) {
+                       u64 paddr =
of_translate_address(ofnode_to_np(node), &addr);
+
+                       return (fdt_addr_t)(be64_to_cpu(paddr) >> 32);
+               } else {
+                       return addr;
+               }
        } else {
                return fdt_get_base_address(gd->fdt_blob,
                                            ofnode_to_offset(node));
----- >8 -----

Thanks for the help, and best regards,

Mario

>
> On 8 December 2017 at 10:11,  <sjg@google.com> wrote:
>> On 23 November 2017 at 23:51, Mario Six <mario.six@gdsys.cc> wrote:
>>> The function dev_read_addr calls ofnode_get_addr_index in the live tree
>>> case, which does not apply bus translations to the address read from the
>>> device tree. This results in illegal addresses on boards that rely on
>>> bus translations being applied.
>>>
>>> Fix this situation by applying bus translations in the live tree case as
>>> well.
>>>
>>> Signed-off-by: Mario Six <mario.six@gdsys.cc>
>>> ---
>>>
>>> Changes v1 -> v2:
>>> * Added IS_ENABLED(CONFIG_OF_TRANSLATE) case distinction
>>>
>>> ---
>>>  drivers/core/ofnode.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> Applied to u-boot-dm thanks!
>

[U-Boot] [PATCH v2] drivers: core: Add translation in live tree case

[Mario Six]

On Fri, Dec 15, 2017 at 9:05 AM, Mario Six <mario.six@gdsys.cc> wrote:
> Hi Simon,
>
> On Thu, Dec 14, 2017 at 9:36 PM, Simon Glass <sjg@chromium.org> wrote:
>> +Stephen, Tom
>>
>> Hi Mario,
>>
>> I've had to drop this since it breaks tegra. Stephen feels that this
>> is likely a bug in the patch rather than anything wrong with Tegra. Do
>> you have any thoughts? I can potentially try things out on the Tegra
>> boards I have.
>>
>> Regards,
>> Simon
>>
>
> I think I've found the problem: It's an endianness issue. The
> of_translate_address function seems to return a raw big-endian address, while
> the of_read_number function translates it to CPU endianness before returning
> it. On the MPC8308 I tested on this made no difference, but little-endian
> systems will have problems. When I put a bogus dev_read_addr in the sandbox
> serial driver, I see the effect as well.
>
> Here's a hopefully fixed version (just the diff; I'll post a proper patch when
> your tests succeeded); it corrects the issue on sandbox, so I hope it will work
> on tegra as well:
>
> ----- >8 -----
>
> diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
> index 0030ab962e..ca22d09ff2 100644
> --- a/drivers/core/ofnode.c
> +++ b/drivers/core/ofnode.c
> @@ -200,13 +200,22 @@ fdt_addr_t ofnode_get_addr_index(ofnode node, int index)
>                 uint flags;
>                 u64 size;
>                 int na;
> +               __be32 addr;
>
>                 prop_val = of_get_address(ofnode_to_np(node), index, &size,
>                                           &flags);
>                 if (!prop_val)
>                         return FDT_ADDR_T_NONE;
>                 na = of_n_addr_cells(ofnode_to_np(node));
> -               return of_read_number(prop_val, na);
> +               addr = of_read_number(prop_val, na);
> +
> +               if (IS_ENABLED(CONFIG_OF_TRANSLATE)) {
> +                       u64 paddr =
> of_translate_address(ofnode_to_np(node), &addr);
> +
> +                       return (fdt_addr_t)(be64_to_cpu(paddr) >> 32);
> +               } else {
> +                       return addr;
> +               }
>         } else {
>                 return fdt_get_base_address(gd->fdt_blob,
>                                             ofnode_to_offset(node));
> ----- >8 -----
>
> Thanks for the help, and best regards,
>
> Mario
>

Small correction:

----- >8 -----

diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
index 0030ab962e..09f0aeab4f 100644
--- a/drivers/core/ofnode.c
+++ b/drivers/core/ofnode.c
@@ -200,13 +200,22 @@ fdt_addr_t ofnode_get_addr_index(ofnode node, int index)
                uint flags;
                u64 size;
                int na;
+               __be32 addr;

                prop_val = of_get_address(ofnode_to_np(node), index, &size,
                                          &flags);
                if (!prop_val)
                        return FDT_ADDR_T_NONE;
                na = of_n_addr_cells(ofnode_to_np(node));
-               return of_read_number(prop_val, na);
+               addr = of_read_number(prop_val, na);
+
+               if (IS_ENABLED(CONFIG_OF_TRANSLATE)) {
+                       u64 paddr =
of_translate_address(ofnode_to_np(node), &addr);
+
+                       return be32_to_cpu((fdt_addr_t)paddr);
+               } else {
+                       return addr;
+               }
        } else {
                return fdt_get_base_address(gd->fdt_blob,
                                            ofnode_to_offset(node));

----- >8 -----

Regards,

Mario

>>
>> On 8 December 2017 at 10:11,  <sjg@google.com> wrote:
>>> On 23 November 2017 at 23:51, Mario Six <mario.six@gdsys.cc> wrote:
>>>> The function dev_read_addr calls ofnode_get_addr_index in the live tree
>>>> case, which does not apply bus translations to the address read from the
>>>> device tree. This results in illegal addresses on boards that rely on
>>>> bus translations being applied.
>>>>
>>>> Fix this situation by applying bus translations in the live tree case as
>>>> well.
>>>>
>>>> Signed-off-by: Mario Six <mario.six@gdsys.cc>
>>>> ---
>>>>
>>>> Changes v1 -> v2:
>>>> * Added IS_ENABLED(CONFIG_OF_TRANSLATE) case distinction
>>>>
>>>> ---
>>>>  drivers/core/ofnode.c | 8 +++++++-
>>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>
>>> Applied to u-boot-dm thanks!
>>

[U-Boot] [PATCH v2] drivers: core: Add translation in live tree case

[Stephen Warren]

On 12/15/2017 01:19 AM, Mario Six wrote:
> On Fri, Dec 15, 2017 at 9:05 AM, Mario Six <mario.six@gdsys.cc> wrote:
>> Hi Simon,
>>
>> On Thu, Dec 14, 2017 at 9:36 PM, Simon Glass <sjg@chromium.org> wrote:
>>> +Stephen, Tom
>>>
>>> Hi Mario,
>>>
>>> I've had to drop this since it breaks tegra. Stephen feels that this
>>> is likely a bug in the patch rather than anything wrong with Tegra. Do
>>> you have any thoughts? I can potentially try things out on the Tegra
>>> boards I have.
>>>
>>> Regards,
>>> Simon
>>>
>>
>> I think I've found the problem: It's an endianness issue. The
>> of_translate_address function seems to return a raw big-endian address, while
>> the of_read_number function translates it to CPU endianness before returning
>> it. On the MPC8308 I tested on this made no difference, but little-endian
>> systems will have problems. When I put a bogus dev_read_addr in the sandbox
>> serial driver, I see the effect as well.
>>
>> Here's a hopefully fixed version (just the diff; I'll post a proper patch when
>> your tests succeeded); it corrects the issue on sandbox, so I hope it will work
>> on tegra as well:
...
> Small correction:

Yes, that seems to work. I tested:

a) u-boot-dm works fine on Jetson TK1.
b) Applying your original patch reproduces the failure.
c) Applying this latest patch make everything work again.

Thanks.