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

[Marek Vasut]

On 3/11/22 20:33, Angus Ainslie wrote:
> On 2022-03-11 11:19, Marek Vasut wrote:
>> 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 ?
>>
> 
> Ok I can do that for the imx8m* clock drivers.

You can do that as a separate patch too, possibly afterward as well.
If something breaks, it would be easy to bisect.

>>> 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.
>>
> 
> It's a good thing I didn't just copy the kernel drivers because I found 
> an error there. After this driver accepted there will very few if any 
> updates to the mux tables as I think most if not all of the clocks 
> you'll need in u-boot are defined. If there are any future updates those 
> could be cut and pasted.

I had one more look at the kernel and u-boot drivers and now I finally 
see what you mean with the tables being disparate ... all right.