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

Thu Sep 22 00:08:55 CEST 2022

On Wednesday 21 September 2022 15:54:52 Chris Packham wrote:
> On Tue, Sep 20, 2022 at 9:22 PM Pali Rohár <pali at 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 at 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.

Ok! I know that there is Marvell **e**fuse API and U-Boot fuse API
(without leading e). For A3720 Marvell U-Boot has only **e**fuse
command and I have already ported it to U-Boot fuse and it is in
mainline.

So I think that same is for AC5 and if you need it, you would have to
port it to u-boot fuse API.

> > > +
> > >  # 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.

Maybe because of this fix?
https://lore.kernel.org/u-boot/20220506180140.141049-1-robert.marko@sartura.hr/

> > > +
> > >       /* 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.

Yea, A70x0/A80x0/CN913x U-Boot code is not cleaned yet, contains lot of
unsystematic parts and its DTS is not compatible with Linux... So I'm
not surprised and even you already wrote that original code is based on
older u-boot version.

> 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
> > >