On Thu, Sep 22, 2022 at 9:40 AM Pali Rohár <p...@kernel.org> wrote:
>
> On Thursday 22 September 2022 09:25:37 Chris Packham wrote:
> > On Wed, Sep 21, 2022 at 5:58 PM Stefan Roese <s...@denx.de> wrote:
> > >
> > > On 21.09.22 06:59, 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.pack...@gmail.com>
> > > > ---
> > > >
> >
> > <snip>
> >
> > > > diff --git a/arch/arm/mach-mvebu/alleycat5/soc.c
> > > > b/arch/arm/mach-mvebu/alleycat5/soc.c
> > > > new file mode 100644
> > > > index 0000000000..f388d4ee40
> > > > --- /dev/null
> > > > +++ b/arch/arm/mach-mvebu/alleycat5/soc.c
> >
> > <snip>
> >
> > > > +int soc_early_init_f(void)
> > > > +{
> > > > +#ifdef CONFIG_MVEBU_SAR
> > > > +/* Sample at reset register init */
> > > > + mvebu_sar_init();
> > > > +#endif
> > >
> > > Won't CONFIG_MVEBU_SAR always be enabled? Remove the #ifdef in this
> > > case.
> > >
> > > > + return 0;
> > > > +}
> >
> > Currently it is possible to turn it off. As I've said I think I do
> > need to look at the whole SAR business and see if it can be done
> > differently.
> >
> > One useful thing it does do is tell me about how the hardware has been
> > strapped. U-Boot itself doesn't care about that specifically as the
> > separate mv_ddr blob that runs before ATF is the thing that actually
> > uses the SAR values. But U-
> > Boot is the first place that can give me some friendly output about
> > the board and knowing the CPU/DDR clock speed is useful at that level.
>
> I agree that the last thing - print information about CPU/DDR clock
> speed is useful [1]. But I do not see reason why to complicate it such
> much via new uclass OOP model for other things in DTS, which moreover
> use already deprecated structure and requires hack in fdtdec.c file.
>
Agreed. The fact that I can turn the driver off and (after fixing some
code that calls into it directly) everything still works (except for
the reporting of the clock speeds) it does indicate that I don't
really need this.
> I think that the whole SAR code can be simplified and written
> straightforward for AC5 to be easier for developer to read... but this
> step is not probably such simple as it is required to first understand
> what is this "complicated" code doing and it is probably boring job :-(
Not boring but it doesn't help that Marvell don't document the SAR
registers so I can only go by following their overly complicated code.
>
> For 32-bit Armada there is already very simple SAR register handling to
> check and detect TCLK speed. So I think AC5 is similar in this area, but
> more has more complicated code logic and "very simple 32-bit SAR" is not
> suitable to reuse.
>
I think I can come up with something that replaces the sar-uclass
business with code that just accesses the relevant register(s)
directly in the alleycat5/soc.c code.
>
> [1] - Btw, I added this print in A3720 secure firmware, because it is
> really useful and important (!) and secure firmware is who configures it
