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

From: Michael Walle <michael@walle.cc>
To: "Pali Rohár" <pali@kernel.org>
Cc: baruch@tkos.co.il, dgilmore@redhat.com, jon@solid-run.com,
	judge.packham@gmail.com, marek.behun@nic.cz, mario.six@gdsys.cc,
	sr@denx.de, u-boot@lists.denx.de
Subject: Re: [PATCH 5/5] arm: kirkwood: Do not overwrite CONFIG_SYS_TCLK
Date: Tue, 16 Aug 2022 22:06:22 +0200	[thread overview]
Message-ID: <f382439a1a35537a6fd6f2d438bf6bb4@walle.cc> (raw)
In-Reply-To: <20220816193851.t5gxdrpavkef62c5@pali>

Am 2022-08-16 21:38, schrieb Pali Rohár:
> 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@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.

Seems like you beat me, while I was typing my former mail ;)

-michael