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

[Michael Walle <michael@walle.cc>]

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

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

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

-michael

> > Signed-off-by: Pali Rohár <pali@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
> >