Re: [PATCH v2 5/6] arm: mvebu: Support for 98DX25xx/98DX35xx SoC

[Chris Packham <judge.packham@gmail.com>]

On Tue, Sep 20, 2022 at 9:22 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Tuesday 20 September 2022 20:31:52 Chris Packham wrote:
> > Add support for the Allecat5/Alleycat5X SoC. These are L3 switches with
> > an integrated CPU (referred to as the CnM block in Marvell's
> > documentation). These have dual ARMv8.2 CPUs (Cortex-A55). This support
> > has been ported from Marvell's SDK which is based on a much older
> > version of U-Boot.
> >
> > Signed-off-by: Chris Packham <judge.packham@gmail.com>
> > ---
> >
> > (no changes since v1)
> >
> >  arch/arm/dts/ac5-98dx25xx.dtsi           | 292 +++++++++++++++++++++++
> >  arch/arm/dts/ac5-98dx35xx.dtsi           |  17 ++
> >  arch/arm/mach-mvebu/Kconfig              |   5 +
> >  arch/arm/mach-mvebu/Makefile             |   1 +
> >  arch/arm/mach-mvebu/alleycat5/Makefile   |   9 +
> >  arch/arm/mach-mvebu/alleycat5/clock.c    |  49 ++++
> >  arch/arm/mach-mvebu/alleycat5/cpu.c      | 129 ++++++++++
> >  arch/arm/mach-mvebu/alleycat5/soc.c      | 229 ++++++++++++++++++
> >  arch/arm/mach-mvebu/arm64-common.c       |  15 ++
> >  arch/arm/mach-mvebu/include/mach/clock.h |  11 +
> >  arch/arm/mach-mvebu/include/mach/cpu.h   |   4 +
> >  arch/arm/mach-mvebu/include/mach/soc.h   |   4 +
> >  12 files changed, 765 insertions(+)
> >  create mode 100644 arch/arm/dts/ac5-98dx25xx.dtsi
> >  create mode 100644 arch/arm/dts/ac5-98dx35xx.dtsi
> >  create mode 100644 arch/arm/mach-mvebu/alleycat5/Makefile
> >  create mode 100644 arch/arm/mach-mvebu/alleycat5/clock.c
> >  create mode 100644 arch/arm/mach-mvebu/alleycat5/cpu.c
> >  create mode 100644 arch/arm/mach-mvebu/alleycat5/soc.c
> >  create mode 100644 arch/arm/mach-mvebu/include/mach/clock.h
> >
> ...
> > diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
> > index a81b8e2b0d..400a308985 100644
> > --- a/arch/arm/mach-mvebu/Kconfig
> > +++ b/arch/arm/mach-mvebu/Kconfig
> > @@ -49,6 +49,11 @@ config ARMADA_8K
> >       bool
> >       select ARM64
> >
> > +config ALLEYCAT_5
> > +     bool
> > +     select ARM64
> > +     select HAVE_MVEBU_EFUSE
>
> Hello! You are not adding any efuse implementation for AC5 platform,
> so selecting this symbol seems to be wrong.
>
> Or are you reusing A3720 or A38x OTP implementation for AC5? This is not
> clear from the patch.
>

The original code did have some EFUSE stuff in it but I've dropped
most of it out. I think this can go.

> > +
> >  # Armada PLL frequency (used for NAND clock generation)
> >  config SYS_MVEBU_PLL_CLOCK
> >       int
> ...
> > diff --git a/arch/arm/mach-mvebu/arm64-common.c b/arch/arm/mach-mvebu/arm64-common.c
> > index 238edbe6ba..c9b6f16c63 100644
> > --- a/arch/arm/mach-mvebu/arm64-common.c
> > +++ b/arch/arm/mach-mvebu/arm64-common.c
> > @@ -53,6 +53,8 @@ __weak int dram_init_banksize(void)
> >               return a8k_dram_init_banksize();
> >       else if (CONFIG_IS_ENABLED(ARMADA_3700))
> >               return a3700_dram_init_banksize();
> > +     else if (CONFIG_IS_ENABLED(ALLEYCAT_5))
> > +             return alleycat5_dram_init_banksize();
> >       else
> >               return fdtdec_setup_memory_banksize();
> >  }
> > @@ -68,6 +70,9 @@ __weak int dram_init(void)
> >       if (CONFIG_IS_ENABLED(ARMADA_3700))
> >               return a3700_dram_init();
> >
> > +     if (CONFIG_IS_ENABLED(ALLEYCAT_5))
> > +             return alleycat5_dram_init();
> > +
> >       if (fdtdec_setup_mem_size_base() != 0)
> >               return -EINVAL;
> >
> > @@ -100,6 +105,16 @@ int arch_early_init_r(void)
> >                       break;
> >       }
> >
> > +     i = 0;
> > +     while (1) {
> > +             /* Call the pinctrl code via the PINCTRL uclass driver */
> > +             ret = uclass_get_device(UCLASS_PINCTRL, i++, &dev);
> > +
> > +             /* We're done, once no further CP110 device is found */
> > +             if (ret)
> > +                     break;
> > +     }
>
> This code is unconditionally called for all 64-bit mvebu platforms, not
> only for new AC5. So if this is some fix, it should be in separate
> commit. If not then it should be marked AC5 specific and explained why.
>

Yeah I can't see why it's needed. The pinctrl device still gets probed
without it so I suspect it's just old cruft left over from a time that
wasn't the case.

> > +
> >       /* Cause the SATA device to do its early init */
> >       uclass_first_device(UCLASS_AHCI, &dev);
> >
> > diff --git a/arch/arm/mach-mvebu/include/mach/clock.h b/arch/arm/mach-mvebu/include/mach/clock.h
> > new file mode 100644
> > index 0000000000..93d965ad5a
> > --- /dev/null
> > +++ b/arch/arm/mach-mvebu/include/mach/clock.h
> > @@ -0,0 +1,11 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (C) 2018 Marvell International Ltd.
> > + */
> > +
> > +#ifndef _MVEBU_CLOCK_H_
> > +#define _MVEBU_CLOCK_H_
> > +
> > +void soc_print_clock_info(void);
>
> I'm not sure if this function needs to be exported. You are using it
> only in AC5 cpu.c file, in function print_cpuinfo(). So probably you can
> move soc_print_clock_info() into that file and then global mvebu clock.h
> would not be needed at all.
>

I'd need to fold all of the stuff in alleycat5/clock.c into
alleycat5/cpu.c but it should be doable. Alternatively I could keep it
separate but have a local .h file for this (and soc.h) to avoid
polluting the mvebu generic files.

> > +
> > +#endif /* _MVEBU_CLOCK_H_ */
> > diff --git a/arch/arm/mach-mvebu/include/mach/cpu.h b/arch/arm/mach-mvebu/include/mach/cpu.h
> > index b127fce865..c17c2440f1 100644
> > --- a/arch/arm/mach-mvebu/include/mach/cpu.h
> > +++ b/arch/arm/mach-mvebu/include/mach/cpu.h
> > @@ -174,6 +174,10 @@ int a3700_dram_init_banksize(void);
> >  /* A3700 PCIe regions fixer for device tree */
> >  int a3700_fdt_fix_pcie_regions(void *blob);
> >
> > +/* Alleycat5 dram functions */
> > +int alleycat5_dram_init(void);
> > +int alleycat5_dram_init_banksize(void);
> > +
> >  /*
> >   * get_ref_clk
> >   *
> > diff --git a/arch/arm/mach-mvebu/include/mach/soc.h b/arch/arm/mach-mvebu/include/mach/soc.h
> > index 3b9618852c..b9312e37dc 100644
> > --- a/arch/arm/mach-mvebu/include/mach/soc.h
> > +++ b/arch/arm/mach-mvebu/include/mach/soc.h
> > @@ -212,4 +212,8 @@
> >  #define CONFIG_SYS_TCLK              250000000       /* 250MHz */
> >  #endif
> >
> > +#ifndef __ASSEMBLY__
> > +void soc_print_device_info(void);
> > +int soc_early_init_f(void);
>
> These two functions are not available on existing mvebu platforms,
> so I think functions should not be declared in global mvebu soc.h file.
>

This one is a bit weird. There is a weak version defined in the
board.c (same for the octeontx2_cn913x, possibly copied from similar
vendor code). But the actual implementation that we need is in
arch/arm/mach-mvebu/alleycat5/soc.c so my attempt to put a definition
in soc.h was to make this less mysterious but I forgot to remove the
weak definition.

The strong implementation is the thing that triggers the init for the
mvebu_sar driver from the previous patch. So I think I need to unpick
the SAR driver and figure out how to do it "properly" (addressing the
issues you've pointed out). What I intend to do is send a v3 series
without addressing this specific concern and then try and figure out
how much (or how little) of the SAR code I really need.

> > +#endif /* __ASSEMBLY__ */
> >  #endif /* _MVEBU_SOC_H */
> > --
> > 2.37.3
> >