Re: [U-Boot] [PATCH v3 2/2] arm: add initial support for Amlogic Meson and ODROID-C2

[Alexander Graf]

On 14.04.16 01:12, Marek Vasut wrote:
> On 04/14/2016 12:53 AM, Alexander Graf wrote:
>>
>>
>> On 14.04.16 00:51, Tom Rini wrote:
>>> On Thu, Apr 14, 2016 at 12:42:41AM +0200, Alexander Graf wrote:


 On 14.04.16 00:34, Tom Rini wrote:
> On Wed, Apr 13, 2016 at 11:38:51PM +0200, Alexander Graf wrote:
>> On 13.04.16 13:52, Marek Vasut wrote:
>>> On 04/13/2016 01:22 PM, Beniamino Galvani wrote:
 On Wed, Apr 13, 2016 at 12:26:43AM +0200, Marek Vasut wrote:
>> So, after some investigation, the reason is that the code runs when
>> caches are still disabled and thus all the memory is treated as
>> Device-nGnRnE, requiring aligned accesses.
>
> You mean 8-byte aligned accesses, correct ?

 Yes.

>> The return value of
>> fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but
>> not 8)
>
> The return value of fdt_getprop() is a pointer, thus 8byte long on
> aarch64 and thus aligned to 8 bytes on the stack unless there is
> some real problem.

 Right, however I'm not talking about the alignment of the pointer on
 the stack, but about the value of the pointer, which depends on the
 offset inside the device tree blob of the property. If I use this:

 val = fdt_getprop(gd->fdt_blob, offset, "reg", )
 gd->ram_size = fdt64_to_cpu(*(fdt64_t *)val)

 when the CPU tries to dereference val (which is something like
 0x010429e4) an alignment fault is generated for the reason
 stated above.
>>>
>>> Oh, now it's clear what the problem is, thanks. But then, we'd need such
>>> fixups all over the place I'm afraid. Isn't there some way to enable
>>> support for "unaligned" accesses instead?
>>
>> Yes, and it's called "enable the MMU". You could probably do this in the
>> early dram init stage already, but I'm not sure it's worth it. The NXP
>> people are the only ones doing it really early today FWIW.
>>
>> Also, if you find it more readable, you could just use
>> get_unaligned_be64(). It gets you down to byte accesses rather than
>> 32bit fetches, but the function name makes it pretty obvious what we're
>> looking at.
>
> OK, now I'm starting to get nightmares back to our last unaligned access
> discussion.  Is ARMv8 doing something radically different from ARMv7
> here, wrt unaligned accesses?

 No, it does the same. To handle not naturally memory accesses you need
 to have dcache enabled and to enable the dcache the MMU needs to be
 turned on. ARMv7 is the same for all I'm aware of.
>>>
>>> Ah, OK, so we just need to get the MMU on for ARMv8, and that's more
>>> complex?  Or am I just flat out missing something?
>>>
>>
>> It's not terribly complex, but the code in question runs in very early
>> init (it initializes dram). I guess people usually like their caches off
>> that early.
> 
> Caches and MMU are two orthogonal things though.

The way I understood this is that the default cachability of non MMU
mapped pages is Device-nGnRnE. To have anything marked as cached, the
only chance you have is to set the caching mode in the page table. So
enabling the MMU is a requirement to get data accessed via the dcache
which is a requirement to allow unaligned accesses.


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

Re: [U-Boot] [PATCH v3 2/2] arm: add initial support for Amlogic Meson and ODROID-C2

[Marek Vasut]

On 04/14/2016 12:53 AM, Alexander Graf wrote:
> 
> 
> On 14.04.16 00:51, Tom Rini wrote:
>> On Thu, Apr 14, 2016 at 12:42:41AM +0200, Alexander Graf wrote:
>>>
>>>
>>> On 14.04.16 00:34, Tom Rini wrote:
 On Wed, Apr 13, 2016 at 11:38:51PM +0200, Alexander Graf wrote:
> On 13.04.16 13:52, Marek Vasut wrote:
>> On 04/13/2016 01:22 PM, Beniamino Galvani wrote:
>>> On Wed, Apr 13, 2016 at 12:26:43AM +0200, Marek Vasut wrote:
> So, after some investigation, the reason is that the code runs when
> caches are still disabled and thus all the memory is treated as
> Device-nGnRnE, requiring aligned accesses.

 You mean 8-byte aligned accesses, correct ?
>>>
>>> Yes.
>>>
> The return value of
> fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but
> not 8)

 The return value of fdt_getprop() is a pointer, thus 8byte long on
 aarch64 and thus aligned to 8 bytes on the stack unless there is
 some real problem.
>>>
>>> Right, however I'm not talking about the alignment of the pointer on
>>> the stack, but about the value of the pointer, which depends on the
>>> offset inside the device tree blob of the property. If I use this:
>>>
>>> val = fdt_getprop(gd->fdt_blob, offset, "reg", )
>>> gd->ram_size = fdt64_to_cpu(*(fdt64_t *)val)
>>>
>>> when the CPU tries to dereference val (which is something like
>>> 0x010429e4) an alignment fault is generated for the reason
>>> stated above.
>>
>> Oh, now it's clear what the problem is, thanks. But then, we'd need such
>> fixups all over the place I'm afraid. Isn't there some way to enable
>> support for "unaligned" accesses instead?
>
> Yes, and it's called "enable the MMU". You could probably do this in the
> early dram init stage already, but I'm not sure it's worth it. The NXP
> people are the only ones doing it really early today FWIW.
>
> Also, if you find it more readable, you could just use
> get_unaligned_be64(). It gets you down to byte accesses rather than
> 32bit fetches, but the function name makes it pretty obvious what we're
> looking at.

 OK, now I'm starting to get nightmares back to our last unaligned access
 discussion.  Is ARMv8 doing something radically different from ARMv7
 here, wrt unaligned accesses?
>>>
>>> No, it does the same. To handle not naturally memory accesses you need
>>> to have dcache enabled and to enable the dcache the MMU needs to be
>>> turned on. ARMv7 is the same for all I'm aware of.
>>
>> Ah, OK, so we just need to get the MMU on for ARMv8, and that's more
>> complex?  Or am I just flat out missing something?
>>
> 
> It's not terribly complex, but the code in question runs in very early
> init (it initializes dram). I guess people usually like their caches off
> that early.

Caches and MMU are two orthogonal things though.


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 2/2] arm: add initial support for Amlogic Meson and ODROID-C2

[Alexander Graf]

On 14.04.16 00:51, Tom Rini wrote:
> On Thu, Apr 14, 2016 at 12:42:41AM +0200, Alexander Graf wrote:
>>
>>
>> On 14.04.16 00:34, Tom Rini wrote:
>>> On Wed, Apr 13, 2016 at 11:38:51PM +0200, Alexander Graf wrote:
 On 13.04.16 13:52, Marek Vasut wrote:
> On 04/13/2016 01:22 PM, Beniamino Galvani wrote:
>> On Wed, Apr 13, 2016 at 12:26:43AM +0200, Marek Vasut wrote:
 So, after some investigation, the reason is that the code runs when
 caches are still disabled and thus all the memory is treated as
 Device-nGnRnE, requiring aligned accesses.
>>>
>>> You mean 8-byte aligned accesses, correct ?
>>
>> Yes.
>>
 The return value of
 fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but
 not 8)
>>>
>>> The return value of fdt_getprop() is a pointer, thus 8byte long on
>>> aarch64 and thus aligned to 8 bytes on the stack unless there is
>>> some real problem.
>>
>> Right, however I'm not talking about the alignment of the pointer on
>> the stack, but about the value of the pointer, which depends on the
>> offset inside the device tree blob of the property. If I use this:
>>
>> val = fdt_getprop(gd->fdt_blob, offset, "reg", )
>> gd->ram_size = fdt64_to_cpu(*(fdt64_t *)val)
>>
>> when the CPU tries to dereference val (which is something like
>> 0x010429e4) an alignment fault is generated for the reason
>> stated above.
>
> Oh, now it's clear what the problem is, thanks. But then, we'd need such
> fixups all over the place I'm afraid. Isn't there some way to enable
> support for "unaligned" accesses instead?

 Yes, and it's called "enable the MMU". You could probably do this in the
 early dram init stage already, but I'm not sure it's worth it. The NXP
 people are the only ones doing it really early today FWIW.

 Also, if you find it more readable, you could just use
 get_unaligned_be64(). It gets you down to byte accesses rather than
 32bit fetches, but the function name makes it pretty obvious what we're
 looking at.
>>>
>>> OK, now I'm starting to get nightmares back to our last unaligned access
>>> discussion.  Is ARMv8 doing something radically different from ARMv7
>>> here, wrt unaligned accesses?
>>
>> No, it does the same. To handle not naturally memory accesses you need
>> to have dcache enabled and to enable the dcache the MMU needs to be
>> turned on. ARMv7 is the same for all I'm aware of.
> 
> Ah, OK, so we just need to get the MMU on for ARMv8, and that's more
> complex?  Or am I just flat out missing something?
> 

It's not terribly complex, but the code in question runs in very early
init (it initializes dram). I guess people usually like their caches off
that early.


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

Re: [U-Boot] [PATCH v3 2/2] arm: add initial support for Amlogic Meson and ODROID-C2

[Tom Rini]

On Thu, Apr 14, 2016 at 12:42:41AM +0200, Alexander Graf wrote:
> 
> 
> On 14.04.16 00:34, Tom Rini wrote:
> > On Wed, Apr 13, 2016 at 11:38:51PM +0200, Alexander Graf wrote:
> >> On 13.04.16 13:52, Marek Vasut wrote:
> >>> On 04/13/2016 01:22 PM, Beniamino Galvani wrote:
>  On Wed, Apr 13, 2016 at 12:26:43AM +0200, Marek Vasut wrote:
> >> So, after some investigation, the reason is that the code runs when
> >> caches are still disabled and thus all the memory is treated as
> >> Device-nGnRnE, requiring aligned accesses.
> >
> > You mean 8-byte aligned accesses, correct ?
> 
>  Yes.
> 
> >> The return value of
> >> fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but
> >> not 8)
> >
> > The return value of fdt_getprop() is a pointer, thus 8byte long on
> > aarch64 and thus aligned to 8 bytes on the stack unless there is
> > some real problem.
> 
>  Right, however I'm not talking about the alignment of the pointer on
>  the stack, but about the value of the pointer, which depends on the
>  offset inside the device tree blob of the property. If I use this:
> 
>  val = fdt_getprop(gd->fdt_blob, offset, "reg", )
>  gd->ram_size = fdt64_to_cpu(*(fdt64_t *)val)
> 
>  when the CPU tries to dereference val (which is something like
>  0x010429e4) an alignment fault is generated for the reason
>  stated above.
> >>>
> >>> Oh, now it's clear what the problem is, thanks. But then, we'd need such
> >>> fixups all over the place I'm afraid. Isn't there some way to enable
> >>> support for "unaligned" accesses instead?
> >>
> >> Yes, and it's called "enable the MMU". You could probably do this in the
> >> early dram init stage already, but I'm not sure it's worth it. The NXP
> >> people are the only ones doing it really early today FWIW.
> >>
> >> Also, if you find it more readable, you could just use
> >> get_unaligned_be64(). It gets you down to byte accesses rather than
> >> 32bit fetches, but the function name makes it pretty obvious what we're
> >> looking at.
> > 
> > OK, now I'm starting to get nightmares back to our last unaligned access
> > discussion.  Is ARMv8 doing something radically different from ARMv7
> > here, wrt unaligned accesses?
> 
> No, it does the same. To handle not naturally memory accesses you need
> to have dcache enabled and to enable the dcache the MMU needs to be
> turned on. ARMv7 is the same for all I'm aware of.

Ah, OK, so we just need to get the MMU on for ARMv8, and that's more
complex?  Or am I just flat out missing something?

-- 
Tom


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

Re: [U-Boot] [PATCH v3 2/2] arm: add initial support for Amlogic Meson and ODROID-C2

[Alexander Graf]

On 14.04.16 00:34, Tom Rini wrote:
> On Wed, Apr 13, 2016 at 11:38:51PM +0200, Alexander Graf wrote:
>> On 13.04.16 13:52, Marek Vasut wrote:
>>> On 04/13/2016 01:22 PM, Beniamino Galvani wrote:
 On Wed, Apr 13, 2016 at 12:26:43AM +0200, Marek Vasut wrote:
>> So, after some investigation, the reason is that the code runs when
>> caches are still disabled and thus all the memory is treated as
>> Device-nGnRnE, requiring aligned accesses.
>
> You mean 8-byte aligned accesses, correct ?

 Yes.

>> The return value of
>> fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but
>> not 8)
>
> The return value of fdt_getprop() is a pointer, thus 8byte long on
> aarch64 and thus aligned to 8 bytes on the stack unless there is
> some real problem.

 Right, however I'm not talking about the alignment of the pointer on
 the stack, but about the value of the pointer, which depends on the
 offset inside the device tree blob of the property. If I use this:

 val = fdt_getprop(gd->fdt_blob, offset, "reg", )
 gd->ram_size = fdt64_to_cpu(*(fdt64_t *)val)

 when the CPU tries to dereference val (which is something like
 0x010429e4) an alignment fault is generated for the reason
 stated above.
>>>
>>> Oh, now it's clear what the problem is, thanks. But then, we'd need such
>>> fixups all over the place I'm afraid. Isn't there some way to enable
>>> support for "unaligned" accesses instead?
>>
>> Yes, and it's called "enable the MMU". You could probably do this in the
>> early dram init stage already, but I'm not sure it's worth it. The NXP
>> people are the only ones doing it really early today FWIW.
>>
>> Also, if you find it more readable, you could just use
>> get_unaligned_be64(). It gets you down to byte accesses rather than
>> 32bit fetches, but the function name makes it pretty obvious what we're
>> looking at.
> 
> OK, now I'm starting to get nightmares back to our last unaligned access
> discussion.  Is ARMv8 doing something radically different from ARMv7
> here, wrt unaligned accesses?

No, it does the same. To handle not naturally memory accesses you need
to have dcache enabled and to enable the dcache the MMU needs to be
turned on. ARMv7 is the same for all I'm aware of.


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

Re: [U-Boot] [PATCH v3 2/2] arm: add initial support for Amlogic Meson and ODROID-C2

[Tom Rini]

On Wed, Apr 13, 2016 at 11:38:51PM +0200, Alexander Graf wrote:
> On 13.04.16 13:52, Marek Vasut wrote:
> > On 04/13/2016 01:22 PM, Beniamino Galvani wrote:
> >> On Wed, Apr 13, 2016 at 12:26:43AM +0200, Marek Vasut wrote:
>  So, after some investigation, the reason is that the code runs when
>  caches are still disabled and thus all the memory is treated as
>  Device-nGnRnE, requiring aligned accesses.
> >>>
> >>> You mean 8-byte aligned accesses, correct ?
> >>
> >> Yes.
> >>
>  The return value of
>  fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but
>  not 8)
> >>>
> >>> The return value of fdt_getprop() is a pointer, thus 8byte long on
> >>> aarch64 and thus aligned to 8 bytes on the stack unless there is
> >>> some real problem.
> >>
> >> Right, however I'm not talking about the alignment of the pointer on
> >> the stack, but about the value of the pointer, which depends on the
> >> offset inside the device tree blob of the property. If I use this:
> >>
> >> val = fdt_getprop(gd->fdt_blob, offset, "reg", )
> >> gd->ram_size = fdt64_to_cpu(*(fdt64_t *)val)
> >>
> >> when the CPU tries to dereference val (which is something like
> >> 0x010429e4) an alignment fault is generated for the reason
> >> stated above.
> > 
> > Oh, now it's clear what the problem is, thanks. But then, we'd need such
> > fixups all over the place I'm afraid. Isn't there some way to enable
> > support for "unaligned" accesses instead?
> 
> Yes, and it's called "enable the MMU". You could probably do this in the
> early dram init stage already, but I'm not sure it's worth it. The NXP
> people are the only ones doing it really early today FWIW.
> 
> Also, if you find it more readable, you could just use
> get_unaligned_be64(). It gets you down to byte accesses rather than
> 32bit fetches, but the function name makes it pretty obvious what we're
> looking at.

OK, now I'm starting to get nightmares back to our last unaligned access
discussion.  Is ARMv8 doing something radically different from ARMv7
here, wrt unaligned accesses?

-- 
Tom


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

Re: [U-Boot] [PATCH v3 2/2] arm: add initial support for Amlogic Meson and ODROID-C2

[Alexander Graf]

On 13.04.16 13:52, Marek Vasut wrote:
> On 04/13/2016 01:22 PM, Beniamino Galvani wrote:
>> On Wed, Apr 13, 2016 at 12:26:43AM +0200, Marek Vasut wrote:
 So, after some investigation, the reason is that the code runs when
 caches are still disabled and thus all the memory is treated as
 Device-nGnRnE, requiring aligned accesses.
>>>
>>> You mean 8-byte aligned accesses, correct ?
>>
>> Yes.
>>
 The return value of
 fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but
 not 8)
>>>
>>> The return value of fdt_getprop() is a pointer, thus 8byte long on
>>> aarch64 and thus aligned to 8 bytes on the stack unless there is
>>> some real problem.
>>
>> Right, however I'm not talking about the alignment of the pointer on
>> the stack, but about the value of the pointer, which depends on the
>> offset inside the device tree blob of the property. If I use this:
>>
>> val = fdt_getprop(gd->fdt_blob, offset, "reg", )
>> gd->ram_size = fdt64_to_cpu(*(fdt64_t *)val)
>>
>> when the CPU tries to dereference val (which is something like
>> 0x010429e4) an alignment fault is generated for the reason
>> stated above.
> 
> Oh, now it's clear what the problem is, thanks. But then, we'd need such
> fixups all over the place I'm afraid. Isn't there some way to enable
> support for "unaligned" accesses instead?

Yes, and it's called "enable the MMU". You could probably do this in the
early dram init stage already, but I'm not sure it's worth it. The NXP
people are the only ones doing it really early today FWIW.

Also, if you find it more readable, you could just use
get_unaligned_be64(). It gets you down to byte accesses rather than
32bit fetches, but the function name makes it pretty obvious what we're
looking at.


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

Re: [U-Boot] [PATCH v3 2/2] arm: add initial support for Amlogic Meson and ODROID-C2

[Marek Vasut]

On 04/13/2016 01:22 PM, Beniamino Galvani wrote:
> On Wed, Apr 13, 2016 at 12:26:43AM +0200, Marek Vasut wrote:
>>> So, after some investigation, the reason is that the code runs when
>>> caches are still disabled and thus all the memory is treated as
>>> Device-nGnRnE, requiring aligned accesses.
>>
>> You mean 8-byte aligned accesses, correct ?
> 
> Yes.
> 
>>> The return value of
>>> fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but
>>> not 8)
>>
>> The return value of fdt_getprop() is a pointer, thus 8byte long on
>> aarch64 and thus aligned to 8 bytes on the stack unless there is
>> some real problem.
> 
> Right, however I'm not talking about the alignment of the pointer on
> the stack, but about the value of the pointer, which depends on the
> offset inside the device tree blob of the property. If I use this:
> 
> val = fdt_getprop(gd->fdt_blob, offset, "reg", )
> gd->ram_size = fdt64_to_cpu(*(fdt64_t *)val)
> 
> when the CPU tries to dereference val (which is something like
> 0x010429e4) an alignment fault is generated for the reason
> stated above.

Oh, now it's clear what the problem is, thanks. But then, we'd need such
fixups all over the place I'm afraid. Isn't there some way to enable
support for "unaligned" accesses instead?

>>> and therefore a 32-bit type must be used to avoid alignment
>>> faults. Probably the comment should be updated to explain this better.
>>
>> Take a look at what uniphier does : arch/arm/mach-uniphier/dram_init.c
>> Does that approach with fdt64_t work for you?
> 
> Nope, that's the first thing I've tried :(
> 
> Beniamino
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 2/2] arm: add initial support for Amlogic Meson and ODROID-C2

[Beniamino Galvani]

On Wed, Apr 13, 2016 at 12:26:43AM +0200, Marek Vasut wrote:
> > So, after some investigation, the reason is that the code runs when
> > caches are still disabled and thus all the memory is treated as
> > Device-nGnRnE, requiring aligned accesses.
> 
> You mean 8-byte aligned accesses, correct ?

Yes.

> > The return value of
> > fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but
> > not 8)
> 
> The return value of fdt_getprop() is a pointer, thus 8byte long on
> aarch64 and thus aligned to 8 bytes on the stack unless there is
> some real problem.

Right, however I'm not talking about the alignment of the pointer on
the stack, but about the value of the pointer, which depends on the
offset inside the device tree blob of the property. If I use this:

val = fdt_getprop(gd->fdt_blob, offset, "reg", )
gd->ram_size = fdt64_to_cpu(*(fdt64_t *)val)

when the CPU tries to dereference val (which is something like
0x010429e4) an alignment fault is generated for the reason
stated above.

> > and therefore a 32-bit type must be used to avoid alignment
> > faults. Probably the comment should be updated to explain this better.
> 
> Take a look at what uniphier does : arch/arm/mach-uniphier/dram_init.c
> Does that approach with fdt64_t work for you?

Nope, that's the first thing I've tried :(

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

Re: [U-Boot] [PATCH v3 2/2] arm: add initial support for Amlogic Meson and ODROID-C2

[Marek Vasut]

On 04/12/2016 11:50 PM, Beniamino Galvani wrote:
> On Mon, Apr 11, 2016 at 11:08:02PM +0200, Marek Vasut wrote:
> + val = fdt_getprop(gd->fdt_blob, offset, "reg", );
> + if (len < sizeof(*val) * 4)
> + return -EINVAL;
> +
> + /* Don't use fdt64_t to avoid unaligned access */

 This looks iffy, can you elaborate on this issue ?
>>>
>>> I was getting a "Synchronous Abort handler, esr 0x9621" which
>>> seemed to indicate a alignment fault, but thinking again about it I'm
>>> not sure anymore of the real cause. fdt64_t and fdt64_to_cpu() don't
>>> work here, I will try to investigate better why. Suggestions are
>>> welcome :)
>>
>> Toolchain issues ? Stack alignment issue ?
> 
> So, after some investigation, the reason is that the code runs when
> caches are still disabled and thus all the memory is treated as
> Device-nGnRnE, requiring aligned accesses.

You mean 8-byte aligned accesses, correct ?

> The return value of
> fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but
> not 8)

The return value of fdt_getprop() is a pointer, thus 8byte long on
aarch64 and thus aligned to 8 bytes on the stack unless there is
some real problem.

> and therefore a 32-bit type must be used to avoid alignment
> faults. Probably the comment should be updated to explain this better.

Take a look at what uniphier does : arch/arm/mach-uniphier/dram_init.c
Does that approach with fdt64_t work for you?

> Beniamino
> 
Code in question:

+int dram_init(void)
+{
+   const fdt32_t *val;
+   int offset;
+   int len;
+
+   offset = fdt_path_offset(gd->fdt_blob, "/memory");
+   if (offset < 0)
+   return -EINVAL;
+
+   val = fdt_getprop(gd->fdt_blob, offset, "reg", );
+   if (len < sizeof(*val) * 4)
+   return -EINVAL;
+
+   /* Don't use fdt64_t to avoid unaligned access */
+   gd->ram_size = (uint64_t)fdt32_to_cpu(val[2]) << 32;
+   gd->ram_size |= fdt32_to_cpu(val[3]);
+
+   return 0;
+}

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 2/2] arm: add initial support for Amlogic Meson and ODROID-C2

[Beniamino Galvani]

On Mon, Apr 11, 2016 at 11:08:02PM +0200, Marek Vasut wrote:
> >>> + val = fdt_getprop(gd->fdt_blob, offset, "reg", );
> >>> + if (len < sizeof(*val) * 4)
> >>> + return -EINVAL;
> >>> +
> >>> + /* Don't use fdt64_t to avoid unaligned access */
> >>
> >> This looks iffy, can you elaborate on this issue ?
> > 
> > I was getting a "Synchronous Abort handler, esr 0x9621" which
> > seemed to indicate a alignment fault, but thinking again about it I'm
> > not sure anymore of the real cause. fdt64_t and fdt64_to_cpu() don't
> > work here, I will try to investigate better why. Suggestions are
> > welcome :)
> 
> Toolchain issues ? Stack alignment issue ?

So, after some investigation, the reason is that the code runs when
caches are still disabled and thus all the memory is treated as
Device-nGnRnE, requiring aligned accesses. The return value of
fdt_getprop() is guaranteed to be aligned to a 4 byte boundary (but
not 8) and therefore a 32-bit type must be used to avoid alignment
faults. Probably the comment should be updated to explain this better.

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

Re: [U-Boot] [PATCH v3 2/2] arm: add initial support for Amlogic Meson and ODROID-C2

[Marek Vasut]

On 04/11/2016 11:02 PM, Beniamino Galvani wrote:
> On Mon, Apr 11, 2016 at 02:08:56AM +0200, Marek Vasut wrote:
>>
>>> +#define GXBB_GPIO_0_EN GXBB_PERIPHS_ADDR(0x0c)
>>> +#define GXBB_GPIO_0_OUTGXBB_PERIPHS_ADDR(0x0d)
>>> +#define GXBB_GPIO_0_IN GXBB_PERIPHS_ADDR(0x0e)
>>
>> You can also define this as
>> GXBB_GPIO_EN(n)  (0xc + 3 * (n) + 0)
>> GXBB_GPIO_OUT(n) (0xc + 3 * (n) + 1)
>> GXBB_GPIO_IN(n)  (0xc + 3 * (n) + 2)
> 
> This would work well if GPIO_6 didn't exist :)

Hrm, good point. You can probably special-case the gpio 6 with ternary
operator then .

>>> +#define GXBB_GPIO_1_EN GXBB_PERIPHS_ADDR(0x0f)
>>> +#define GXBB_GPIO_1_OUTGXBB_PERIPHS_ADDR(0x10)
>>> +#define GXBB_GPIO_1_IN GXBB_PERIPHS_ADDR(0x11)
>>> +#define GXBB_GPIO_2_EN GXBB_PERIPHS_ADDR(0x12)
>>> +#define GXBB_GPIO_2_OUTGXBB_PERIPHS_ADDR(0x13)
>>> +#define GXBB_GPIO_2_IN GXBB_PERIPHS_ADDR(0x14)
>>> +#define GXBB_GPIO_3_EN GXBB_PERIPHS_ADDR(0x15)
>>> +#define GXBB_GPIO_3_OUTGXBB_PERIPHS_ADDR(0x16)
>>> +#define GXBB_GPIO_3_IN GXBB_PERIPHS_ADDR(0x17)
>>> +#define GXBB_GPIO_4_EN GXBB_PERIPHS_ADDR(0x18)
>>> +#define GXBB_GPIO_4_OUTGXBB_PERIPHS_ADDR(0x19)
>>> +#define GXBB_GPIO_4_IN GXBB_PERIPHS_ADDR(0x1a)
>>> +#define GXBB_GPIO_5_EN GXBB_PERIPHS_ADDR(0x1b)
>>> +#define GXBB_GPIO_5_OUTGXBB_PERIPHS_ADDR(0x1c)
>>> +#define GXBB_GPIO_5_IN GXBB_PERIPHS_ADDR(0x1d)
>>> +#define GXBB_GPIO_6_EN GXBB_PERIPHS_ADDR(0x08)
>>> +#define GXBB_GPIO_6_OUTGXBB_PERIPHS_ADDR(0x09)
>>> +#define GXBB_GPIO_6_IN GXBB_PERIPHS_ADDR(0x0a)
> 
>> It'd be nice to have base addresses somewhere at the beginning instead
>> of having them mixed with the bit macros, but that's a matter of taste.
> 
> I agree, I'll change this.
> 
>>> +   val = fdt_getprop(gd->fdt_blob, offset, "reg", );
>>> +   if (len < sizeof(*val) * 4)
>>> +   return -EINVAL;
>>> +
>>> +   /* Don't use fdt64_t to avoid unaligned access */
>>
>> This looks iffy, can you elaborate on this issue ?
> 
> I was getting a "Synchronous Abort handler, esr 0x9621" which
> seemed to indicate a alignment fault, but thinking again about it I'm
> not sure anymore of the real cause. fdt64_t and fdt64_to_cpu() don't
> work here, I will try to investigate better why. Suggestions are
> welcome :)

Toolchain issues ? Stack alignment issue ?

>>> +   /* Reserve first 16 MiB of RAM */
>>
>> Why ?
> 
> I'll add a comment, first 16 MiB seems to be reserved for firmware.

Thanks.

>>> +void reset_cpu(ulong addr)
>>> +{
>>
>> How does the system reboot then ?
> 
> The system reboots through a call to secure monitor, which is not
> implemented in this submission.

Ha, either implement the call or add comment please.

>>> +struct mm_region *mem_map = gxbb_mem_map;
>>
>> This looks super-iffy, I wouldn't be surprised if this started being
>> optimized away at some point.
> 
> I don't understand, why that should happen?

If you link the file alone, nothing references the symbols, so they can
be optimized away. But this is luckily not the case here.

>>> +   /* Reset PHY on GPIOZ_14 */
>>> +   clrbits_le32(GXBB_GPIO_3_EN, BIT(14));
>>> +   clrbits_le32(GXBB_GPIO_3_OUT, BIT(14));
>>> +   udelay(10);
>>
>> mdelay(100); , though that is quite some wait.
> 
> Will change and decrease the timeout.
> 
>>> diff --git a/drivers/serial/serial_meson.c b/drivers/serial/serial_meson.c
>>
>> This should go in a separate patch.
> 
> I originally submitted the driver as separate patch, then Tom
> suggested that the initial platform patch should contain everything
> needed to perform a boot, so UART, DDR, board file and DTS.

Meh, I politely disagree with Tom ;-)

> Thanks for the review,
> 
> Beniamino
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 2/2] arm: add initial support for Amlogic Meson and ODROID-C2

[Beniamino Galvani]

On Mon, Apr 11, 2016 at 02:08:56AM +0200, Marek Vasut wrote:
>
> > +#define GXBB_GPIO_0_EN GXBB_PERIPHS_ADDR(0x0c)
> > +#define GXBB_GPIO_0_OUTGXBB_PERIPHS_ADDR(0x0d)
> > +#define GXBB_GPIO_0_IN GXBB_PERIPHS_ADDR(0x0e)
> 
> You can also define this as
> GXBB_GPIO_EN(n)  (0xc + 3 * (n) + 0)
> GXBB_GPIO_OUT(n) (0xc + 3 * (n) + 1)
> GXBB_GPIO_IN(n)  (0xc + 3 * (n) + 2)

This would work well if GPIO_6 didn't exist :)

> > +#define GXBB_GPIO_1_EN GXBB_PERIPHS_ADDR(0x0f)
> > +#define GXBB_GPIO_1_OUTGXBB_PERIPHS_ADDR(0x10)
> > +#define GXBB_GPIO_1_IN GXBB_PERIPHS_ADDR(0x11)
> > +#define GXBB_GPIO_2_EN GXBB_PERIPHS_ADDR(0x12)
> > +#define GXBB_GPIO_2_OUTGXBB_PERIPHS_ADDR(0x13)
> > +#define GXBB_GPIO_2_IN GXBB_PERIPHS_ADDR(0x14)
> > +#define GXBB_GPIO_3_EN GXBB_PERIPHS_ADDR(0x15)
> > +#define GXBB_GPIO_3_OUTGXBB_PERIPHS_ADDR(0x16)
> > +#define GXBB_GPIO_3_IN GXBB_PERIPHS_ADDR(0x17)
> > +#define GXBB_GPIO_4_EN GXBB_PERIPHS_ADDR(0x18)
> > +#define GXBB_GPIO_4_OUTGXBB_PERIPHS_ADDR(0x19)
> > +#define GXBB_GPIO_4_IN GXBB_PERIPHS_ADDR(0x1a)
> > +#define GXBB_GPIO_5_EN GXBB_PERIPHS_ADDR(0x1b)
> > +#define GXBB_GPIO_5_OUTGXBB_PERIPHS_ADDR(0x1c)
> > +#define GXBB_GPIO_5_IN GXBB_PERIPHS_ADDR(0x1d)
> > +#define GXBB_GPIO_6_EN GXBB_PERIPHS_ADDR(0x08)
> > +#define GXBB_GPIO_6_OUTGXBB_PERIPHS_ADDR(0x09)
> > +#define GXBB_GPIO_6_IN GXBB_PERIPHS_ADDR(0x0a)

> It'd be nice to have base addresses somewhere at the beginning instead
> of having them mixed with the bit macros, but that's a matter of taste.

I agree, I'll change this.

> > +   val = fdt_getprop(gd->fdt_blob, offset, "reg", );
> > +   if (len < sizeof(*val) * 4)
> > +   return -EINVAL;
> > +
> > +   /* Don't use fdt64_t to avoid unaligned access */
> 
> This looks iffy, can you elaborate on this issue ?

I was getting a "Synchronous Abort handler, esr 0x9621" which
seemed to indicate a alignment fault, but thinking again about it I'm
not sure anymore of the real cause. fdt64_t and fdt64_to_cpu() don't
work here, I will try to investigate better why. Suggestions are
welcome :)

> > +   /* Reserve first 16 MiB of RAM */
> 
> Why ?

I'll add a comment, first 16 MiB seems to be reserved for firmware.

> > +void reset_cpu(ulong addr)
> > +{
> 
> How does the system reboot then ?

The system reboots through a call to secure monitor, which is not
implemented in this submission.

> > +struct mm_region *mem_map = gxbb_mem_map;
> 
> This looks super-iffy, I wouldn't be surprised if this started being
> optimized away at some point.

I don't understand, why that should happen?

> > +   /* Reset PHY on GPIOZ_14 */
> > +   clrbits_le32(GXBB_GPIO_3_EN, BIT(14));
> > +   clrbits_le32(GXBB_GPIO_3_OUT, BIT(14));
> > +   udelay(10);
> 
> mdelay(100); , though that is quite some wait.

Will change and decrease the timeout.

> > diff --git a/drivers/serial/serial_meson.c b/drivers/serial/serial_meson.c
> 
> This should go in a separate patch.

I originally submitted the driver as separate patch, then Tom
suggested that the initial platform patch should contain everything
needed to perform a boot, so UART, DDR, board file and DTS.

Thanks for the review,

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

Re: [U-Boot] [PATCH v3 2/2] arm: add initial support for Amlogic Meson and ODROID-C2

[Marek Vasut]

On 04/10/2016 05:30 PM, Beniamino Galvani wrote:
> This adds platform code for the Amlogic Meson GXBaby (S905) SoC and a
> board definition for ODROID-C2. This initial submission only supports
> UART and Ethernet (through the existing Designware driver). DTS files
> are the ones submitted to Linux arm-soc for 4.7 [1].
> 
> [1] https://patchwork.ozlabs.org/patch/603583/
> 
> Signed-off-by: Beniamino Galvani 

[...]

> diff --git a/arch/arm/include/asm/arch-meson/gxbb.h 
> b/arch/arm/include/asm/arch-meson/gxbb.h
> new file mode 100644
> index 000..cec06d7
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-meson/gxbb.h
> @@ -0,0 +1,77 @@
> +/*
> + * (C) Copyright 2016 - Beniamino Galvani 
> + *
> + * SPDX-License-Identifier:  GPL-2.0+
> + */
> +
> +#ifndef __GXBB_H__
> +#define __GXBB_H__
> +
> +#define GXBB_PERIPHS_BASE0xc8834400
> +#define GXBB_PERIPHS_ADDR(off)   (GXBB_PERIPHS_BASE + ((off) << 2))
> +
> +#define GXBB_GPIO_0_EN   GXBB_PERIPHS_ADDR(0x0c)
> +#define GXBB_GPIO_0_OUT  GXBB_PERIPHS_ADDR(0x0d)
> +#define GXBB_GPIO_0_IN   GXBB_PERIPHS_ADDR(0x0e)

You can also define this as
GXBB_GPIO_EN(n)  (0xc + 3 * (n) + 0)
GXBB_GPIO_OUT(n) (0xc + 3 * (n) + 1)
GXBB_GPIO_IN(n)  (0xc + 3 * (n) + 2)

> +#define GXBB_GPIO_1_EN   GXBB_PERIPHS_ADDR(0x0f)
> +#define GXBB_GPIO_1_OUT  GXBB_PERIPHS_ADDR(0x10)
> +#define GXBB_GPIO_1_IN   GXBB_PERIPHS_ADDR(0x11)
> +#define GXBB_GPIO_2_EN   GXBB_PERIPHS_ADDR(0x12)
> +#define GXBB_GPIO_2_OUT  GXBB_PERIPHS_ADDR(0x13)
> +#define GXBB_GPIO_2_IN   GXBB_PERIPHS_ADDR(0x14)
> +#define GXBB_GPIO_3_EN   GXBB_PERIPHS_ADDR(0x15)
> +#define GXBB_GPIO_3_OUT  GXBB_PERIPHS_ADDR(0x16)
> +#define GXBB_GPIO_3_IN   GXBB_PERIPHS_ADDR(0x17)
> +#define GXBB_GPIO_4_EN   GXBB_PERIPHS_ADDR(0x18)
> +#define GXBB_GPIO_4_OUT  GXBB_PERIPHS_ADDR(0x19)
> +#define GXBB_GPIO_4_IN   GXBB_PERIPHS_ADDR(0x1a)
> +#define GXBB_GPIO_5_EN   GXBB_PERIPHS_ADDR(0x1b)
> +#define GXBB_GPIO_5_OUT  GXBB_PERIPHS_ADDR(0x1c)
> +#define GXBB_GPIO_5_IN   GXBB_PERIPHS_ADDR(0x1d)
> +#define GXBB_GPIO_6_EN   GXBB_PERIPHS_ADDR(0x08)
> +#define GXBB_GPIO_6_OUT  GXBB_PERIPHS_ADDR(0x09)
> +#define GXBB_GPIO_6_IN   GXBB_PERIPHS_ADDR(0x0a)
> +
> +#define GXBB_PINMUX_0GXBB_PERIPHS_ADDR(0x2c)

DTTO, #define ... (0x2c + (n)) here

> +#define GXBB_PINMUX_1GXBB_PERIPHS_ADDR(0x2d)
> +#define GXBB_PINMUX_2GXBB_PERIPHS_ADDR(0x2e)
> +#define GXBB_PINMUX_3GXBB_PERIPHS_ADDR(0x2f)
> +#define GXBB_PINMUX_4GXBB_PERIPHS_ADDR(0x30)
> +#define GXBB_PINMUX_5GXBB_PERIPHS_ADDR(0x31)
> +#define GXBB_PINMUX_6GXBB_PERIPHS_ADDR(0x32)
> +#define GXBB_PINMUX_7GXBB_PERIPHS_ADDR(0x33)
> +#define GXBB_PINMUX_8GXBB_PERIPHS_ADDR(0x34)
> +#define GXBB_PINMUX_9GXBB_PERIPHS_ADDR(0x35)
> +#define GXBB_PINMUX_10   GXBB_PERIPHS_ADDR(0x36)
> +#define GXBB_PINMUX_11   GXBB_PERIPHS_ADDR(0x37)
> +#define GXBB_PINMUX_12   GXBB_PERIPHS_ADDR(0x38)
> +
> +#define GXBB_ETH_REG_0   GXBB_PERIPHS_ADDR(0x50)
> +#define GXBB_ETH_REG_1   GXBB_PERIPHS_ADDR(0x51)
> +
> +#define GXBB_ETH_REG_0_PHY_INTF  BIT(0)
> +#define GXBB_ETH_REG_0_TX_PHASE(x)   (((x) & 3) << 5)
> +#define GXBB_ETH_REG_0_TX_RATIO(x)   (((x) & 7) << 7)
> +#define GXBB_ETH_REG_0_PHY_CLK_ENBIT(10)
> +#define GXBB_ETH_REG_0_CLK_ENBIT(12)
> +
> +#define GXBB_HIU_BASE0xc883c000

It'd be nice to have base addresses somewhere at the beginning instead
of having them mixed with the bit macros, but that's a matter of taste.

> +#define GXBB_HIU_ADDR(off)   (GXBB_HIU_BASE + ((off) << 2))
> +
> +#define GXBB_MEM_PD_REG_0GXBB_HIU_ADDR(0x40)
> +
> +/* Ethernet memory power domain */
> +#define GXBB_MEM_PD_REG_0_ETH_MASK   (BIT(2) | BIT(3))
> +
> +/* Clock gates */
> +#define GXBB_GCLK_MPEG_0 GXBB_HIU_ADDR(0x50)
> +#define GXBB_GCLK_MPEG_1 GXBB_HIU_ADDR(0x51)
> +#define GXBB_GCLK_MPEG_2 GXBB_HIU_ADDR(0x52)
> +#define GXBB_GCLK_MPEG_OTHER GXBB_HIU_ADDR(0x53)
> +#define GXBB_GCLK_MPEG_AOGXBB_HIU_ADDR(0x54)
> +
> +#define GXBB_GCLK_MPEG_1_ETH BIT(3)
> +
> +#define GXBB_ETH_BASE0xc941
> +
> +#endif /* __GXBB_H__ */

[...]

> diff --git a/arch/arm/mach-meson/board.c b/arch/arm/mach-meson/board.c
> new file mode 100644
> index 000..1d4d32e
> --- /dev/null
> +++ b/arch/arm/mach-meson/board.c
> @@ -0,0 +1,65 @@
> +/*
> + * (C) Copyright 2016 Beniamino Galvani 
> + *
> + * SPDX-License-Identifier:  GPL-2.0+
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +DECLARE_GLOBAL_DATA_PTR;
>