[PATCH] arm: mvebu: AC5/AC5X: use fixed page table size

[PATCH] arm: mvebu: AC5/AC5X: use fixed page table size

[Chris Packham]

Since commit 6cdf6b7a340d ("arm64: Use FEAT_HAFDBS to track dirty pages
when available") the default get_page_table_size() sets some flags for
more efficient handling of dirty page table entries. This causes
problems on the AC5/AC5X SoC (specifically a lockup when calling
__asm_switch_ttbr() via mmu_set_region_dcache_behaviour()).

The reason for the lockup isn't at all clear but it can be avoided if we
provide our own get_page_table_size() which stops gd->arch.has_hafdbs
from being set to true effectively returning the AC5/AC5X to the older
behaviour. This also opts for a simple fixed page table size rather than
trying to duplicate the more complicated logic to optimise the table
size.

Signed-off-by: Chris Packham <judge.packham@gmail.com>
---

 arch/arm/mach-mvebu/alleycat5/cpu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/mach-mvebu/alleycat5/cpu.c b/arch/arm/mach-mvebu/alleycat5/cpu.c
index 8204d9627515..7ba57ae75e76 100644
--- a/arch/arm/mach-mvebu/alleycat5/cpu.c
+++ b/arch/arm/mach-mvebu/alleycat5/cpu.c
@@ -63,6 +63,11 @@ static struct mm_region ac5_mem_map[] = {
 
 struct mm_region *mem_map = ac5_mem_map;
 
+u64 get_page_table_size(void)
+{
+	return 0x80000;
+}
+
 void reset_cpu(void)
 {
 }
-- 
2.42.0

Re: [PATCH] arm: mvebu: AC5/AC5X: use fixed page table size

[Stefan Roese]

Hi Chris,

On 10/18/23 22:53, Chris Packham wrote:
> Since commit 6cdf6b7a340d ("arm64: Use FEAT_HAFDBS to track dirty pages
> when available") the default get_page_table_size() sets some flags for
> more efficient handling of dirty page table entries. This causes
> problems on the AC5/AC5X SoC (specifically a lockup when calling
> __asm_switch_ttbr() via mmu_set_region_dcache_behaviour()).
> 
> The reason for the lockup isn't at all clear but it can be avoided if we
> provide our own get_page_table_size() which stops gd->arch.has_hafdbs
> from being set to true effectively returning the AC5/AC5X to the older
> behaviour. This also opts for a simple fixed page table size rather than
> trying to duplicate the more complicated logic to optimise the table
> size.
> 
> Signed-off-by: Chris Packham <judge.packham@gmail.com>

I'm not 100% happy with this approach / workaround - still it fixes an
issue on your board, so:

Reviewed-by: Stefan Roese <sr@denx.de>

Perhaps you (or someone else) can look into this in more depth to get
to the root of this issue at a later time.

Thanks,
Stefan

> ---
> 
>   arch/arm/mach-mvebu/alleycat5/cpu.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/mach-mvebu/alleycat5/cpu.c b/arch/arm/mach-mvebu/alleycat5/cpu.c
> index 8204d9627515..7ba57ae75e76 100644
> --- a/arch/arm/mach-mvebu/alleycat5/cpu.c
> +++ b/arch/arm/mach-mvebu/alleycat5/cpu.c
> @@ -63,6 +63,11 @@ static struct mm_region ac5_mem_map[] = {
>   
>   struct mm_region *mem_map = ac5_mem_map;
>   
> +u64 get_page_table_size(void)
> +{
> +	return 0x80000;
> +}
> +
>   void reset_cpu(void)
>   {
>   }

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH] arm: mvebu: AC5/AC5X: use fixed page table size

[Chris Packham]

On Fri, 20 Oct 2023, 7:18 pm Stefan Roese, <sr@denx.de> wrote:

> Hi Chris,
>
> On 10/18/23 22:53, Chris Packham wrote:
> > Since commit 6cdf6b7a340d ("arm64: Use FEAT_HAFDBS to track dirty pages
> > when available") the default get_page_table_size() sets some flags for
> > more efficient handling of dirty page table entries. This causes
> > problems on the AC5/AC5X SoC (specifically a lockup when calling
> > __asm_switch_ttbr() via mmu_set_region_dcache_behaviour()).
> >
> > The reason for the lockup isn't at all clear but it can be avoided if we
> > provide our own get_page_table_size() which stops gd->arch.has_hafdbs
> > from being set to true effectively returning the AC5/AC5X to the older
> > behaviour. This also opts for a simple fixed page table size rather than
> > trying to duplicate the more complicated logic to optimise the table
> > size.
> >
> > Signed-off-by: Chris Packham <judge.packham@gmail.com>
>
> I'm not 100% happy with this approach / workaround - still it fixes an
> issue on your board, so:
>
> Reviewed-by: Stefan Roese <sr@denx.de>
>

Yeah I'm not super happy about this either. But this is about the best I
could come up with.


> Perhaps you (or someone else) can look into this in more depth to get
> to the root of this issue at a later time.
>

I did spend a reasonable amount of time tracking down where things were
going wrong, even if I couldn't figure out why. The commit message
basically reflects what I found.


> Thanks,
> Stefan
>
> > ---
> >
> >   arch/arm/mach-mvebu/alleycat5/cpu.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/arch/arm/mach-mvebu/alleycat5/cpu.c
> b/arch/arm/mach-mvebu/alleycat5/cpu.c
> > index 8204d9627515..7ba57ae75e76 100644
> > --- a/arch/arm/mach-mvebu/alleycat5/cpu.c
> > +++ b/arch/arm/mach-mvebu/alleycat5/cpu.c
> > @@ -63,6 +63,11 @@ static struct mm_region ac5_mem_map[] = {
> >
> >   struct mm_region *mem_map = ac5_mem_map;
> >
> > +u64 get_page_table_size(void)
> > +{
> > +     return 0x80000;
> > +}
> > +
> >   void reset_cpu(void)
> >   {
> >   }
>
> Viele Grüße,
> Stefan Roese
>
> --
> DENX Software Engineering GmbH,      Managing Director: Erika Unter
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
>

Re: [PATCH] arm: mvebu: AC5/AC5X: use fixed page table size

[Stefan Roese]

On 10/20/23 10:21, Chris Packham wrote:
> 
> 
> On Fri, 20 Oct 2023, 7:18 pm Stefan Roese, <sr@denx.de 
> <mailto:sr@denx.de>> wrote:
> 
>     Hi Chris,
> 
>     On 10/18/23 22:53, Chris Packham wrote:
>      > Since commit 6cdf6b7a340d ("arm64: Use FEAT_HAFDBS to track dirty
>     pages
>      > when available") the default get_page_table_size() sets some
>     flags for
>      > more efficient handling of dirty page table entries. This causes
>      > problems on the AC5/AC5X SoC (specifically a lockup when calling
>      > __asm_switch_ttbr() via mmu_set_region_dcache_behaviour()).
>      >
>      > The reason for the lockup isn't at all clear but it can be
>     avoided if we
>      > provide our own get_page_table_size() which stops gd->arch.has_hafdbs
>      > from being set to true effectively returning the AC5/AC5X to the
>     older
>      > behaviour. This also opts for a simple fixed page table size
>     rather than
>      > trying to duplicate the more complicated logic to optimise the table
>      > size.
>      >
>      > Signed-off-by: Chris Packham <judge.packham@gmail.com
>     <mailto:judge.packham@gmail.com>>
> 
>     I'm not 100% happy with this approach / workaround - still it fixes an
>     issue on your board, so:
> 
>     Reviewed-by: Stefan Roese <sr@denx.de <mailto:sr@denx.de>>
> 
> 
> Yeah I'm not super happy about this either. But this is about the best I 
> could come up with.
> 
> 
>     Perhaps you (or someone else) can look into this in more depth to get
>     to the root of this issue at a later time.
> 
> 
> I did spend a reasonable amount of time tracking down where things were 
> going wrong, even if I couldn't figure out why. The commit message 
> basically reflects what I found.

Applied to u-boot-marvell/master

Thanks,
Stefan

> 
> 
>     Thanks,
>     Stefan
> 
>      > ---
>      >
>      >   arch/arm/mach-mvebu/alleycat5/cpu.c | 5 +++++
>      >   1 file changed, 5 insertions(+)
>      >
>      > diff --git a/arch/arm/mach-mvebu/alleycat5/cpu.c
>     b/arch/arm/mach-mvebu/alleycat5/cpu.c
>      > index 8204d9627515..7ba57ae75e76 100644
>      > --- a/arch/arm/mach-mvebu/alleycat5/cpu.c
>      > +++ b/arch/arm/mach-mvebu/alleycat5/cpu.c
>      > @@ -63,6 +63,11 @@ static struct mm_region ac5_mem_map[] = {
>      >
>      >   struct mm_region *mem_map = ac5_mem_map;
>      >
>      > +u64 get_page_table_size(void)
>      > +{
>      > +     return 0x80000;
>      > +}
>      > +
>      >   void reset_cpu(void)
>      >   {
>      >   }
> 
>     Viele Grüße,
>     Stefan Roese
> 
>     -- 
>     DENX Software Engineering GmbH,      Managing Director: Erika Unter
>     HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>     Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email:
>     sr@denx.de <mailto:sr@denx.de>
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH] arm: mvebu: AC5/AC5X: use fixed page table size

[Marc Zyngier]

On 2023-10-18 21:53, Chris Packham wrote:
> Since commit 6cdf6b7a340d ("arm64: Use FEAT_HAFDBS to track dirty pages
> when available") the default get_page_table_size() sets some flags for
> more efficient handling of dirty page table entries. This causes
> problems on the AC5/AC5X SoC (specifically a lockup when calling
> __asm_switch_ttbr() via mmu_set_region_dcache_behaviour()).
> 
> The reason for the lockup isn't at all clear but it can be avoided if 
> we
> provide our own get_page_table_size() which stops gd->arch.has_hafdbs
> from being set to true effectively returning the AC5/AC5X to the older
> behaviour. This also opts for a simple fixed page table size rather 
> than
> trying to duplicate the more complicated logic to optimise the table
> size.
> 
> Signed-off-by: Chris Packham <judge.packham@gmail.com>
> ---
> 
>  arch/arm/mach-mvebu/alleycat5/cpu.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/mach-mvebu/alleycat5/cpu.c
> b/arch/arm/mach-mvebu/alleycat5/cpu.c
> index 8204d9627515..7ba57ae75e76 100644
> --- a/arch/arm/mach-mvebu/alleycat5/cpu.c
> +++ b/arch/arm/mach-mvebu/alleycat5/cpu.c
> @@ -63,6 +63,11 @@ static struct mm_region ac5_mem_map[] = {
> 
>  struct mm_region *mem_map = ac5_mem_map;
> 
> +u64 get_page_table_size(void)
> +{
> +	return 0x80000;
> +}
> +
>  void reset_cpu(void)
>  {
>  }

This looks terribly wrong. In all honesty, if the original
patch creates problems and that people add this sort of stuff
to paper over it, I'd rather your *revert* my patch altogether.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] arm: mvebu: AC5/AC5X: use fixed page table size

[Chris Packham]

On Sat, 21 Oct 2023, 2:04 am Marc Zyngier, <maz@kernel.org> wrote:

> On 2023-10-18 21:53, Chris Packham wrote:
> > Since commit 6cdf6b7a340d ("arm64: Use FEAT_HAFDBS to track dirty pages
> > when available") the default get_page_table_size() sets some flags for
> > more efficient handling of dirty page table entries. This causes
> > problems on the AC5/AC5X SoC (specifically a lockup when calling
> > __asm_switch_ttbr() via mmu_set_region_dcache_behaviour()).
> >
> > The reason for the lockup isn't at all clear but it can be avoided if
> > we
> > provide our own get_page_table_size() which stops gd->arch.has_hafdbs
> > from being set to true effectively returning the AC5/AC5X to the older
> > behaviour. This also opts for a simple fixed page table size rather
> > than
> > trying to duplicate the more complicated logic to optimise the table
> > size.
> >
> > Signed-off-by: Chris Packham <judge.packham@gmail.com>
> > ---
> >
> >  arch/arm/mach-mvebu/alleycat5/cpu.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/arch/arm/mach-mvebu/alleycat5/cpu.c
> > b/arch/arm/mach-mvebu/alleycat5/cpu.c
> > index 8204d9627515..7ba57ae75e76 100644
> > --- a/arch/arm/mach-mvebu/alleycat5/cpu.c
> > +++ b/arch/arm/mach-mvebu/alleycat5/cpu.c
> > @@ -63,6 +63,11 @@ static struct mm_region ac5_mem_map[] = {
> >
> >  struct mm_region *mem_map = ac5_mem_map;
> >
> > +u64 get_page_table_size(void)
> > +{
> > +     return 0x80000;
> > +}
> > +
> >  void reset_cpu(void)
> >  {
> >  }
>
> This looks terribly wrong. In all honesty, if the original
> patch creates problems and that people add this sort of stuff
> to paper over it, I'd rather your *revert* my patch altogether.
>

There are other platforms that define a get_page_table_size(). That may
mean that they are just inadvertently avoiding the issues I'm seeing.

I'm happy to prepare a series reverting the problematic changes. But I'm
sure that there's something about the AC5 that's the actual problem. I'm
just out of my depth in terms of my ability to progress it.


>          M.
> --
> Jazz is not dead. It just smells funny...
>