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

Tue Aug 16 21:38:51 CEST 2022

On Tuesday 16 August 2022 20:17:08 Pali Rohár wrote:
> 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.

Meanwhile I have found following email thread:
https://lore.kernel.org/linux-arm-kernel/20121020113800.GC21046@lunn.ch/t/#u

Where are more 6281 boards with 166 MHz TCLK and there is interesting output:
$ dmesg | grep -i tclk
[    5.851008] Kirkwood: MV88F6281-A0, TCLK=200000000

Which means that old kernel version had TCLK detection code. I grepped
git linux history and I successfully found it:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm/mach-kirkwood/common.c?h=v3.15#n542

#define  SAMPLE_AT_RESET   (DEV_BUS_VIRT_BASE + 0x0030)

static int __init kirkwood_find_tclk(void)
{
	u32 dev, rev;

	kirkwood_pcie_id(&dev, &rev);

	if (dev == MV88F6281_DEV_ID || dev == MV88F6282_DEV_ID)
		if (((readl(SAMPLE_AT_RESET) >> 21) & 1) == 0)
			return 200000000;

	return 166666667;
}

So for TCLK detection is used BIT 21 of already mentioned 0x10030 Sample
at Reset Register.

I'm going to prepare a patch which will fix this issue for 88F6281 SoCs.

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