[PATCH 5/5] arm: kirkwood: Do not overwrite CONFIG_SYS_TCLK

[Pali Rohár]

Hello!

On Tuesday 16 August 2022 11:37:48 Michael Walle wrote:
> Hi!
> 
> > On Sunday 01 August 2021 20:07:16 Chris Packham wrote:
> > > On Sun, Aug 1, 2021 at 12:23 AM Pali Rohár <pali at kernel.org> wrote:
> > > >
> > > > Config option CONFIG_SYS_TCLK is set by kw88f6281.h and kw88f6192.h files
> > > > to correct SOC/platform value. So do not overwrite it in board config
> > > > include files.
> > > >
> > > > Kirkwood 88F6180 and 88F6192 uses 166 MHz TCLK and Kirkwood 88F6281 uses
> > > > 200 MHz TCLK.
> > > >
> > > 
> > > It's been a while since I worked with kirkwood but I thought that
> > > there was hardware strapping for the TCLK.
> > 
> > Interesting... Because I took above information from Kirkwood hardware specifications...
> > 
> > 88F6180: https://web.archive.org/web/20130730091654/http://www.marvell.com/embedded-processors/kirkwood/assets/HW_88F6180_OpenSource.pdf
> > 88F6192: https://web.archive.org/web/20121021182835/http://www.marvell.com/embedded-processors/kirkwood/assets/HW_88F619x_OpenSource.pdf
> > 88F6281: https://web.archive.org/web/20120620073511/http://www.marvell.com/embedded-processors/kirkwood/assets/HW_88F6281_OpenSource.pdf
> > 
> > And there are specified fixed TCLK values.
> 
> Nope, this breaks my lsxl (specifically the LSCHLv2) board. The TCLK is
> 166MHz there.

Ou, sorry for that.

> > > If I understand correctly
> > > the defines in kw88f6281.h/kw88f6192.h were sensible defaults but
> > > boards were able to override it to reflect the hardware configuration.
> > 
> > Anyway, I think that this patch should not cause issue as it is changing
> > only two board config files and removing redefinition of CONFIG_SYS_TCLK
> > macro which is set to the same value as in kw88f61**.h files.
> 
> At least for the lsxl and the NET2BIG_V2 this is not correct. Both have
> the 88F6281 and both use have a 166MHz TCLK clock.

Interesting... because this contradicts publicly available
documentation. Maybe in NDA doc is some more details?

> Anyway, I'm reverting this patch. The only open question is, should I
> convert the TCLK to a Kconfig option?

In this case it would be better to detect TCLK from some SAR register,
like it is already implemented for other Armada SoCs.

Just by a chance, do you have some "better" 88F6281 documentation? If
there is some TCLK information or SAR register description. In publicly
available FS_88F6180_9x_6281_OpenSource.pdf there is 0x10030 Sample at
Reset Register, but nothing TCLK related.

At least BootROM has to detect TCLK because UART clock is derived from
TCLK and BootROM supports UART booting via 115200 baudrate. In case you
can provide me 88F6281 BootROM dump from your board, I can try to find
code which configures UART and detect TCLK. I have already did it for
88F6820 (A385) to verify that U-Boot code detects TCLK in the same way
as BootROM.

> -michael
> 
> > > Signed-off-by: Pali Rohár <pali at kernel.org>
> > > ---
> > >  arch/arm/mach-kirkwood/include/mach/kw88f6281.h | 2 --
> > >  include/configs/lacie_kw.h                      | 5 -----
> > >  include/configs/lsxl.h                          | 2 --
> > >  3 files changed, 9 deletions(-)
> > >
> > > diff --git a/arch/arm/mach-kirkwood/include/mach/kw88f6281.h b/arch/arm/mach-kirkwood/include/mach/kw88f6281.h
> > > index 33e741420781..87406081cf54 100644
> > > --- a/arch/arm/mach-kirkwood/include/mach/kw88f6281.h
> > > +++ b/arch/arm/mach-kirkwood/include/mach/kw88f6281.h
> > > @@ -15,8 +15,6 @@
> > >  #define KW_REGS_PHY_BASE               KW88F6281_REGS_PHYS_BASE
> > >
> > >  /* TCLK Core Clock definition */
> > > -#ifndef CONFIG_SYS_TCLK
> > >  #define CONFIG_SYS_TCLK        200000000 /* 200MHz */
> > > -#endif
> > >
> > >  #endif /* _ASM_ARCH_KW88F6281_H */
> > > diff --git a/include/configs/lacie_kw.h b/include/configs/lacie_kw.h
> > > index 420c1d49b08e..88f784f1f0fd 100644
> > > --- a/include/configs/lacie_kw.h
> > > +++ b/include/configs/lacie_kw.h
> > > @@ -39,11 +39,6 @@
> > >  #endif
> > >  #define CONFIG_SKIP_LOWLEVEL_INIT      /* disable board lowlevel_init */
> > >
> > > -/*
> > > - * Core clock definition
> > > - */
> > > -#define CONFIG_SYS_TCLK                        166000000 /* 166MHz */
> > > -
> > >  /*
> > >   * SDRAM configuration
> > >   */
> > > diff --git a/include/configs/lsxl.h b/include/configs/lsxl.h
> > > index 0c0ab2486e23..a4a4739d0dd7 100644
> > > --- a/include/configs/lsxl.h
> > > +++ b/include/configs/lsxl.h
> > > @@ -13,11 +13,9 @@
> > >  #if defined(CONFIG_LSCHLV2)
> > >  #define CONFIG_SYS_KWD_CONFIG $(CONFIG_BOARDDIR)/kwbimage-lschl.cfg
> > >  #define CONFIG_MACH_TYPE 3006
> > > -#define CONFIG_SYS_TCLK 166666667 /* 166 MHz */
> > >  #elif defined(CONFIG_LSXHL)
> > >  #define CONFIG_SYS_KWD_CONFIG $(CONFIG_BOARDDIR)/kwbimage-lsxhl.cfg
> > >  #define CONFIG_MACH_TYPE 2663
> > > -/* CONFIG_SYS_TCLK is 200000000 by default */
> > >  #else
> > >  #error "unknown board"
> > >  #endif
> > > --
> > > 2.20.1
> > >
>