Re: [PATCH] dt-bindings: clock: imx8mq: Fix numbering overlaps and gaps

[Abel Vesa <abel.vesa@nxp.com>]

On 19-03-05 10:38:29, Stephen Boyd wrote:
> Quoting Abel Vesa (2019-03-05 01:49:16)
> > IMX8MQ_CLK_USB_PHY_REF changes from 163 to 153, this way removing the gap.
> > All the following clock ids are now decreased by 10 to keep the numbering
> > right. Doing this, the IMX8MQ_CLK_CSI2_CORE is not overlapped with
> > IMX8MQ_CLK_GPT1 anymore. IMX8MQ_CLK_GPT1_ROOT changes from 193 to 183 and
> > all the following ids are updated accordingly.
> 
> Why do the numbers need to be consecutive? This looks difficult to merge
> given that the commit that's being "fixed" is in v5.0 and thus the
> integer value of these defines is pretty much an ABI. So if there are
> holes in the space, I suggest we leave them there unless something is
> really wrong or unworkable with that solution.
> 

I would've ignored it but there are a few overlaps.

#define IMX8MQ_CLK_GPT1                        170
overlaps with:
#define IMX8MQ_CLK_CSI2_CORE           170

#define IMX8MQ_CLK_CSI2_PHY_REF                181
overlaps with:
#define IMX8MQ_CLK_ECSPI3_ROOT                 181

#define IMX8MQ_CLK_CSI2_ESC            182
overlaps with:
#define IMX8MQ_CLK_ENET1_ROOT                  182

By removing the gaps, some of the overlaps are also removed.

I can just get rid of the overlaps and keep the gaps if that makes it more ok
for the stability of the ABI.

> BTW, it would be great if the binding header was generated once and then
> never changed again so that we don't have to spend time on these sorts
> of patches in the future. Please try to fully describe each possible clk
> that might be used with a particular binding instead of adding new clk
> ids over time, especially if you have access to the silicon manufacturer
> documentation and can easily figure out all the clks that are there
> beforehand.
> 

Here is an example of why this is not really doable: clk-sccg-pll.c.
The original design was adding the intermediary clocks like:

IMX8MQ_SYS1_PLL1_OUT        
IMX8MQ_SYS1_PLL1_OUT_DIV    
IMX8MQ_SYS1_PLL1_REF_DIV    
IMX8MQ_SYS1_PLL2
IMX8MQ_SYS1_PLL2_DIV  
IMX8MQ_SYS1_PLL2_OUT  

And these were just for SYS1_PLL. There are 2 more SYSx_PLL.
Plus the DRAM_PLL, the VIDEO2_PLL and the AUDIO_PLL.

Since the refactoring of clk-sccg-pll.c, these are not used anymore (or should
not be used). So I would have to remove them, but as you said, it would break
the ABI. And this example goes even further with the fact that the PHY_27M
and the mux between the PHY_27M and the OSC_27 are to be controlled by the
display controller driver itself (at this point the PHY_27M is not yet upstream
and I can't say for sure it will ever be). Basically, what this means is that 
the PHY_27M will have to be exposed as a standalone clock even if it is hidden
behind a mux which has another clock that can provide the same rate. That
is only because there is some difference in phase (AFAIU) between the OSC_27M
and the PHY_27M, at the same rate. And this is just one example.

Point being, there is no way of knowing beforehand what intermediary clocks
are needed, even with the silicon manufacturer documentation, until the
driver that makes use of a specific clock actually works.