Re: [PATCH v2 2/2] clk: imx8mq: Add a clock driver for the imx8mq

From: Marek Vasut <marex@denx.de>
To: Angus Ainslie <angus@akkea.ca>
Cc: Angus Ainslie <angus.ainslie@puri.sm>,
	u-boot@lists.denx.de, lukma@denx.de, seanga2@gmail.com,
	sbabic@denx.de, festevam@gmail.com, kernel@puri.sm,
	uboot-imx@nxp.com
Subject: Re: [PATCH v2 2/2] clk: imx8mq: Add a clock driver for the imx8mq
Date: Fri, 11 Mar 2022 20:19:14 +0100	[thread overview]
Message-ID: <26c0b552-b13c-d48c-04f3-b47c9f72c9b5@denx.de> (raw)
In-Reply-To: <d442f3b7696343e03deb73352b5fc25f@akkea.ca>

On 3/11/22 19:41, Angus Ainslie wrote:
> On 2022-03-11 10:05, Marek Vasut wrote:
>> On 3/11/22 18:02, Angus Ainslie wrote:
>>> On 2022-03-11 08:57, Marek Vasut wrote:
>>>> On 3/11/22 17:35, Angus Ainslie wrote:
>>>>> All of the PLLs and clocks are initialized so the subsystems below are
>>>>> functional and tested.
>>>>>
>>>>> 1) USB host and peripheral
>>>>> 2) ECSPI
>>>>> 3) UART
>>>>> 4) I2C all busses
>>>>> 5) USDHC for eMMC support
>>>>> 6) USB storage
>>>>> 7) GPIO
>>>>> 8) DRAM
>>>>>
>>>>> The PLL rate tables are from the kernel
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=43cdaa1567ad3931fbde438853947d45238cc040 
>>>>>
>>>>
>>>> That patch is three years old.
>>>> That patch is for MX8M Mini clock, not for MX8M(Q).
>>>>
>>>> You can use the abbreviated commit ID instead:
>>>> 43cdaa1567ad3 ("clk: imx8mm: Move 1443X/1416X PLL clock structure to 
>>>> common place")
>>>> But that seems to be the wrong commit.
>>>
>>> That's the commit where the imx8m PLL frequency table is moved to a 
>>> common file for use by all of the imx8m variants. The imx8mq linux 
>>> driver does not even use the frequency tables so there is not a 
>>> specific commit for it.
>>
>> Isn't large part of this driver coming from these tables ?
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/clk/imx/clk-imx8mq.c 
>>
> 
> When I said tables I was referring to the PLL frequency tables.

Ahh, hmmm, I see that we now have three copies of those PLL tables in 
each MX8M{M,N,P} driver and Linux instead has this in common code. Can 
you deduplicate the PLL tables before we add fourth copy ?

> The driver is modelled after the u-boot imx8mm u-boot driver with 
> register and mux updates from the imx8mq reference manual. Very little 
> comes from the imx8mq kernel driver. Mainly I just verified mux naming 
> and register offsets against that driver.

Would it make sense to pick the Linux kernel tables instead then, 
instead of hand-writing them from scratch ? That seems error prone.

> That reminds me that I need to send a kernel patch for the monitor muxes.

[...]