* [PATCH] dt-bindings: clock: imx8mq: Fix numbering overlaps and gaps
@ 2019-03-05 9:49 Abel Vesa
2019-03-05 18:38 ` Stephen Boyd
0 siblings, 1 reply; 9+ messages in thread
From: Abel Vesa @ 2019-03-05 9:49 UTC (permalink / raw)
To: Rob Herring, Stephen Boyd, Mark Rutland, Shawn Guo, Sascha Hauer,
Lucas Stach, Patrick Wildt, Fabio Estevam
Cc: dl-linux-imx, devicetree, linux-arm-kernel,
Linux Kernel Mailing List, Abel Vesa
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.
Reported-by: Patrick Wildt <patrick@blueri.se>
Fixes: 1cf3817b ("dt-bindings: Add binding for i.MX8MQ CCM")
Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
---
include/dt-bindings/clock/imx8mq-clock.h | 220 +++++++++++++++----------------
1 file changed, 110 insertions(+), 110 deletions(-)
diff --git a/include/dt-bindings/clock/imx8mq-clock.h b/include/dt-bindings/clock/imx8mq-clock.h
index b58cc64..6677e92 100644
--- a/include/dt-bindings/clock/imx8mq-clock.h
+++ b/include/dt-bindings/clock/imx8mq-clock.h
@@ -245,160 +245,160 @@
/* USB_CORE_REF */
#define IMX8MQ_CLK_USB_CORE_REF 152
/* USB_PHY_REF */
-#define IMX8MQ_CLK_USB_PHY_REF 163
+#define IMX8MQ_CLK_USB_PHY_REF 153
/* ECSPI1 */
-#define IMX8MQ_CLK_ECSPI1 164
+#define IMX8MQ_CLK_ECSPI1 154
/* ECSPI2 */
-#define IMX8MQ_CLK_ECSPI2 165
+#define IMX8MQ_CLK_ECSPI2 155
/* PWM1 */
-#define IMX8MQ_CLK_PWM1 166
+#define IMX8MQ_CLK_PWM1 156
/* PWM2 */
-#define IMX8MQ_CLK_PWM2 167
+#define IMX8MQ_CLK_PWM2 157
/* PWM3 */
-#define IMX8MQ_CLK_PWM3 168
+#define IMX8MQ_CLK_PWM3 158
/* PWM4 */
-#define IMX8MQ_CLK_PWM4 169
+#define IMX8MQ_CLK_PWM4 159
/* GPT1 */
-#define IMX8MQ_CLK_GPT1 170
+#define IMX8MQ_CLK_GPT1 160
/* WDOG */
-#define IMX8MQ_CLK_WDOG 171
+#define IMX8MQ_CLK_WDOG 161
/* WRCLK */
-#define IMX8MQ_CLK_WRCLK 172
+#define IMX8MQ_CLK_WRCLK 162
/* DSI_CORE */
-#define IMX8MQ_CLK_DSI_CORE 173
+#define IMX8MQ_CLK_DSI_CORE 163
/* DSI_PHY */
-#define IMX8MQ_CLK_DSI_PHY_REF 174
+#define IMX8MQ_CLK_DSI_PHY_REF 164
/* DSI_DBI */
-#define IMX8MQ_CLK_DSI_DBI 175
+#define IMX8MQ_CLK_DSI_DBI 165
/*DSI_ESC */
-#define IMX8MQ_CLK_DSI_ESC 176
+#define IMX8MQ_CLK_DSI_ESC 166
/* CSI1_CORE */
-#define IMX8MQ_CLK_CSI1_CORE 177
+#define IMX8MQ_CLK_CSI1_CORE 167
/* CSI1_PHY */
-#define IMX8MQ_CLK_CSI1_PHY_REF 178
+#define IMX8MQ_CLK_CSI1_PHY_REF 168
/* CSI_ESC */
-#define IMX8MQ_CLK_CSI1_ESC 179
+#define IMX8MQ_CLK_CSI1_ESC 169
/* CSI2_CORE */
#define IMX8MQ_CLK_CSI2_CORE 170
/* CSI2_PHY */
-#define IMX8MQ_CLK_CSI2_PHY_REF 181
+#define IMX8MQ_CLK_CSI2_PHY_REF 171
/* CSI2_ESC */
-#define IMX8MQ_CLK_CSI2_ESC 182
+#define IMX8MQ_CLK_CSI2_ESC 172
/* PCIE2_CTRL */
-#define IMX8MQ_CLK_PCIE2_CTRL 183
+#define IMX8MQ_CLK_PCIE2_CTRL 173
/* PCIE2_PHY */
-#define IMX8MQ_CLK_PCIE2_PHY 184
+#define IMX8MQ_CLK_PCIE2_PHY 174
/* PCIE2_AUX */
-#define IMX8MQ_CLK_PCIE2_AUX 185
+#define IMX8MQ_CLK_PCIE2_AUX 175
/* ECSPI3 */
-#define IMX8MQ_CLK_ECSPI3 186
+#define IMX8MQ_CLK_ECSPI3 176
/* CCGR clocks */
-#define IMX8MQ_CLK_A53_ROOT 187
-#define IMX8MQ_CLK_DRAM_ROOT 188
-#define IMX8MQ_CLK_ECSPI1_ROOT 189
+#define IMX8MQ_CLK_A53_ROOT 177
+#define IMX8MQ_CLK_DRAM_ROOT 178
+#define IMX8MQ_CLK_ECSPI1_ROOT 179
#define IMX8MQ_CLK_ECSPI2_ROOT 180
#define IMX8MQ_CLK_ECSPI3_ROOT 181
#define IMX8MQ_CLK_ENET1_ROOT 182
-#define IMX8MQ_CLK_GPT1_ROOT 193
-#define IMX8MQ_CLK_I2C1_ROOT 194
-#define IMX8MQ_CLK_I2C2_ROOT 195
-#define IMX8MQ_CLK_I2C3_ROOT 196
-#define IMX8MQ_CLK_I2C4_ROOT 197
-#define IMX8MQ_CLK_M4_ROOT 198
-#define IMX8MQ_CLK_PCIE1_ROOT 199
-#define IMX8MQ_CLK_PCIE2_ROOT 200
-#define IMX8MQ_CLK_PWM1_ROOT 201
-#define IMX8MQ_CLK_PWM2_ROOT 202
-#define IMX8MQ_CLK_PWM3_ROOT 203
-#define IMX8MQ_CLK_PWM4_ROOT 204
-#define IMX8MQ_CLK_QSPI_ROOT 205
-#define IMX8MQ_CLK_SAI1_ROOT 206
-#define IMX8MQ_CLK_SAI2_ROOT 207
-#define IMX8MQ_CLK_SAI3_ROOT 208
-#define IMX8MQ_CLK_SAI4_ROOT 209
-#define IMX8MQ_CLK_SAI5_ROOT 210
-#define IMX8MQ_CLK_SAI6_ROOT 212
-#define IMX8MQ_CLK_UART1_ROOT 213
-#define IMX8MQ_CLK_UART2_ROOT 214
-#define IMX8MQ_CLK_UART3_ROOT 215
-#define IMX8MQ_CLK_UART4_ROOT 216
-#define IMX8MQ_CLK_USB1_CTRL_ROOT 217
-#define IMX8MQ_CLK_USB2_CTRL_ROOT 218
-#define IMX8MQ_CLK_USB1_PHY_ROOT 219
-#define IMX8MQ_CLK_USB2_PHY_ROOT 220
-#define IMX8MQ_CLK_USDHC1_ROOT 221
-#define IMX8MQ_CLK_USDHC2_ROOT 222
-#define IMX8MQ_CLK_WDOG1_ROOT 223
-#define IMX8MQ_CLK_WDOG2_ROOT 224
-#define IMX8MQ_CLK_WDOG3_ROOT 225
-#define IMX8MQ_CLK_GPU_ROOT 226
-#define IMX8MQ_CLK_HEVC_ROOT 227
-#define IMX8MQ_CLK_AVC_ROOT 228
-#define IMX8MQ_CLK_VP9_ROOT 229
-#define IMX8MQ_CLK_HEVC_INTER_ROOT 230
-#define IMX8MQ_CLK_DISP_ROOT 231
-#define IMX8MQ_CLK_HDMI_ROOT 232
-#define IMX8MQ_CLK_HDMI_PHY_ROOT 233
-#define IMX8MQ_CLK_VPU_DEC_ROOT 234
-#define IMX8MQ_CLK_CSI1_ROOT 235
-#define IMX8MQ_CLK_CSI2_ROOT 236
-#define IMX8MQ_CLK_RAWNAND_ROOT 237
-#define IMX8MQ_CLK_SDMA1_ROOT 238
-#define IMX8MQ_CLK_SDMA2_ROOT 239
-#define IMX8MQ_CLK_VPU_G1_ROOT 240
-#define IMX8MQ_CLK_VPU_G2_ROOT 241
+#define IMX8MQ_CLK_GPT1_ROOT 183
+#define IMX8MQ_CLK_I2C1_ROOT 184
+#define IMX8MQ_CLK_I2C2_ROOT 185
+#define IMX8MQ_CLK_I2C3_ROOT 186
+#define IMX8MQ_CLK_I2C4_ROOT 187
+#define IMX8MQ_CLK_M4_ROOT 188
+#define IMX8MQ_CLK_PCIE1_ROOT 189
+#define IMX8MQ_CLK_PCIE2_ROOT 190
+#define IMX8MQ_CLK_PWM1_ROOT 191
+#define IMX8MQ_CLK_PWM2_ROOT 192
+#define IMX8MQ_CLK_PWM3_ROOT 193
+#define IMX8MQ_CLK_PWM4_ROOT 194
+#define IMX8MQ_CLK_QSPI_ROOT 195
+#define IMX8MQ_CLK_SAI1_ROOT 196
+#define IMX8MQ_CLK_SAI2_ROOT 197
+#define IMX8MQ_CLK_SAI3_ROOT 198
+#define IMX8MQ_CLK_SAI4_ROOT 199
+#define IMX8MQ_CLK_SAI5_ROOT 200
+#define IMX8MQ_CLK_SAI6_ROOT 201
+#define IMX8MQ_CLK_UART1_ROOT 202
+#define IMX8MQ_CLK_UART2_ROOT 203
+#define IMX8MQ_CLK_UART3_ROOT 204
+#define IMX8MQ_CLK_UART4_ROOT 205
+#define IMX8MQ_CLK_USB1_CTRL_ROOT 206
+#define IMX8MQ_CLK_USB2_CTRL_ROOT 207
+#define IMX8MQ_CLK_USB1_PHY_ROOT 208
+#define IMX8MQ_CLK_USB2_PHY_ROOT 209
+#define IMX8MQ_CLK_USDHC1_ROOT 210
+#define IMX8MQ_CLK_USDHC2_ROOT 211
+#define IMX8MQ_CLK_WDOG1_ROOT 212
+#define IMX8MQ_CLK_WDOG2_ROOT 213
+#define IMX8MQ_CLK_WDOG3_ROOT 214
+#define IMX8MQ_CLK_GPU_ROOT 215
+#define IMX8MQ_CLK_HEVC_ROOT 216
+#define IMX8MQ_CLK_AVC_ROOT 217
+#define IMX8MQ_CLK_VP9_ROOT 218
+#define IMX8MQ_CLK_HEVC_INTER_ROOT 219
+#define IMX8MQ_CLK_DISP_ROOT 220
+#define IMX8MQ_CLK_HDMI_ROOT 221
+#define IMX8MQ_CLK_HDMI_PHY_ROOT 222
+#define IMX8MQ_CLK_VPU_DEC_ROOT 223
+#define IMX8MQ_CLK_CSI1_ROOT 224
+#define IMX8MQ_CLK_CSI2_ROOT 225
+#define IMX8MQ_CLK_RAWNAND_ROOT 226
+#define IMX8MQ_CLK_SDMA1_ROOT 227
+#define IMX8MQ_CLK_SDMA2_ROOT 228
+#define IMX8MQ_CLK_VPU_G1_ROOT 229
+#define IMX8MQ_CLK_VPU_G2_ROOT 230
/* SCCG PLL GATE */
-#define IMX8MQ_SYS1_PLL_OUT 242
-#define IMX8MQ_SYS2_PLL_OUT 243
-#define IMX8MQ_SYS3_PLL_OUT 244
-#define IMX8MQ_DRAM_PLL_OUT 245
-
-#define IMX8MQ_GPT_3M_CLK 246
-
-#define IMX8MQ_CLK_IPG_ROOT 247
-#define IMX8MQ_CLK_IPG_AUDIO_ROOT 248
-#define IMX8MQ_CLK_SAI1_IPG 249
-#define IMX8MQ_CLK_SAI2_IPG 250
-#define IMX8MQ_CLK_SAI3_IPG 251
-#define IMX8MQ_CLK_SAI4_IPG 252
-#define IMX8MQ_CLK_SAI5_IPG 253
-#define IMX8MQ_CLK_SAI6_IPG 254
+#define IMX8MQ_SYS1_PLL_OUT 231
+#define IMX8MQ_SYS2_PLL_OUT 232
+#define IMX8MQ_SYS3_PLL_OUT 233
+#define IMX8MQ_DRAM_PLL_OUT 234
+
+#define IMX8MQ_GPT_3M_CLK 235
+
+#define IMX8MQ_CLK_IPG_ROOT 236
+#define IMX8MQ_CLK_IPG_AUDIO_ROOT 237
+#define IMX8MQ_CLK_SAI1_IPG 238
+#define IMX8MQ_CLK_SAI2_IPG 239
+#define IMX8MQ_CLK_SAI3_IPG 240
+#define IMX8MQ_CLK_SAI4_IPG 241
+#define IMX8MQ_CLK_SAI5_IPG 242
+#define IMX8MQ_CLK_SAI6_IPG 243
/* DSI AHB/IPG clocks */
/* rxesc clock */
-#define IMX8MQ_CLK_DSI_AHB 255
+#define IMX8MQ_CLK_DSI_AHB 244
/* txesc clock */
-#define IMX8MQ_CLK_DSI_IPG_DIV 256
+#define IMX8MQ_CLK_DSI_IPG_DIV 245
-#define IMX8MQ_CLK_TMU_ROOT 257
+#define IMX8MQ_CLK_TMU_ROOT 246
/* Display root clocks */
-#define IMX8MQ_CLK_DISP_AXI_ROOT 258
-#define IMX8MQ_CLK_DISP_APB_ROOT 259
-#define IMX8MQ_CLK_DISP_RTRM_ROOT 260
+#define IMX8MQ_CLK_DISP_AXI_ROOT 247
+#define IMX8MQ_CLK_DISP_APB_ROOT 248
+#define IMX8MQ_CLK_DISP_RTRM_ROOT 249
-#define IMX8MQ_CLK_OCOTP_ROOT 261
+#define IMX8MQ_CLK_OCOTP_ROOT 250
-#define IMX8MQ_CLK_DRAM_ALT_ROOT 262
-#define IMX8MQ_CLK_DRAM_CORE 263
+#define IMX8MQ_CLK_DRAM_ALT_ROOT 251
+#define IMX8MQ_CLK_DRAM_CORE 252
-#define IMX8MQ_CLK_MU_ROOT 264
-#define IMX8MQ_VIDEO2_PLL_OUT 265
+#define IMX8MQ_CLK_MU_ROOT 253
+#define IMX8MQ_VIDEO2_PLL_OUT 254
-#define IMX8MQ_CLK_CLKO2 266
+#define IMX8MQ_CLK_CLKO2 255
-#define IMX8MQ_CLK_NAND_USDHC_BUS_RAWNAND_CLK 267
+#define IMX8MQ_CLK_NAND_USDHC_BUS_RAWNAND_CLK 256
-#define IMX8MQ_CLK_CLKO1 268
-#define IMX8MQ_CLK_ARM 269
+#define IMX8MQ_CLK_CLKO1 257
+#define IMX8MQ_CLK_ARM 258
-#define IMX8MQ_CLK_GPIO1_ROOT 270
-#define IMX8MQ_CLK_GPIO2_ROOT 271
-#define IMX8MQ_CLK_GPIO3_ROOT 272
-#define IMX8MQ_CLK_GPIO4_ROOT 273
-#define IMX8MQ_CLK_GPIO5_ROOT 274
+#define IMX8MQ_CLK_GPIO1_ROOT 259
+#define IMX8MQ_CLK_GPIO2_ROOT 260
+#define IMX8MQ_CLK_GPIO3_ROOT 261
+#define IMX8MQ_CLK_GPIO4_ROOT 262
+#define IMX8MQ_CLK_GPIO5_ROOT 263
-#define IMX8MQ_CLK_END 275
+#define IMX8MQ_CLK_END 264
#endif /* __DT_BINDINGS_CLOCK_IMX8MQ_H */
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] dt-bindings: clock: imx8mq: Fix numbering overlaps and gaps
2019-03-05 9:49 [PATCH] dt-bindings: clock: imx8mq: Fix numbering overlaps and gaps Abel Vesa
@ 2019-03-05 18:38 ` Stephen Boyd
2019-03-06 13:09 ` Abel Vesa
0 siblings, 1 reply; 9+ messages in thread
From: Stephen Boyd @ 2019-03-05 18:38 UTC (permalink / raw)
To: Abel Vesa, Fabio Estevam, Lucas Stach, Mark Rutland,
Patrick Wildt, Rob Herring, Sascha Hauer, Shawn Guo
Cc: dl-linux-imx, devicetree, linux-arm-kernel,
Linux Kernel Mailing List, Abel Vesa
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.
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dt-bindings: clock: imx8mq: Fix numbering overlaps and gaps
2019-03-05 18:38 ` Stephen Boyd
@ 2019-03-06 13:09 ` Abel Vesa
2019-03-08 15:29 ` Stephen Boyd
0 siblings, 1 reply; 9+ messages in thread
From: Abel Vesa @ 2019-03-06 13:09 UTC (permalink / raw)
To: Stephen Boyd
Cc: Fabio Estevam, Lucas Stach, Mark Rutland, Patrick Wildt,
Rob Herring, Sascha Hauer, Shawn Guo, dl-linux-imx, devicetree,
linux-arm-kernel, Linux Kernel Mailing List
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dt-bindings: clock: imx8mq: Fix numbering overlaps and gaps
2019-03-06 13:09 ` Abel Vesa
@ 2019-03-08 15:29 ` Stephen Boyd
2019-03-12 7:36 ` Patrick Wildt
0 siblings, 1 reply; 9+ messages in thread
From: Stephen Boyd @ 2019-03-08 15:29 UTC (permalink / raw)
To: Abel Vesa
Cc: Fabio Estevam, Lucas Stach, Mark Rutland, Patrick Wildt,
Rob Herring, Sascha Hauer, Shawn Guo, dl-linux-imx, devicetree,
linux-arm-kernel, Linux Kernel Mailing List
Quoting Abel Vesa (2019-03-06 05:09:42)
> 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
Ok. Are any of the overlaps in use by dtbs right now?
>
> 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.
It's mostly about making sure that any existing dtbs don't have their
numbers shifted around. So hopefully any overlapping identifiers aren't
in use yet and then those ids can be changed while leaving the ones that
are in use how they are.
>
> > 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.
Why do you need to remove them? Why can't they just return an error if
some driver tries to get those clks after they shouldn't be use anymore?
> 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.
It sounds like these are two independent clks that could or couldn't be
exposed into the DT numberspace?
>
> 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.
I agree that we don't know what clks are needed beforehand, predicting
the future is hard, but that doesn't mean they can't be exposed into the
header file if they exist. It doesn't really matter if there are numbers
in there or not for clks that shouldn't be used. The provider and the
driver implementation need to make the decision to provide them or not
to the consumer drivers when they ask for them. It's perfectly valid for
new code to start failing those requests because "they shouldn't be
used". The header file (really the DT bindings) is the place that
decides what the numbers are and what they correspond to.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dt-bindings: clock: imx8mq: Fix numbering overlaps and gaps
2019-03-08 15:29 ` Stephen Boyd
@ 2019-03-12 7:36 ` Patrick Wildt
2019-03-12 20:39 ` Stephen Boyd
0 siblings, 1 reply; 9+ messages in thread
From: Patrick Wildt @ 2019-03-12 7:36 UTC (permalink / raw)
To: Stephen Boyd
Cc: Abel Vesa, Fabio Estevam, Lucas Stach, Mark Rutland, Rob Herring,
Sascha Hauer, Shawn Guo, dl-linux-imx, devicetree,
linux-arm-kernel, Linux Kernel Mailing List
On Fri, Mar 08, 2019 at 07:29:05AM -0800, Stephen Boyd wrote:
> It's mostly about making sure that any existing dtbs don't have their
> numbers shifted around. So hopefully any overlapping identifiers aren't
> in use yet and then those ids can be changed while leaving the ones that
> are in use how they are.
In practice I bet no one uses Linux 5.0's i.MX8M device trees since they
lack too much support. It's so basic it's not useful. You'd still run
your existing non-mainline bindings until it is. Thus I would argue
changing the ABI right now would be the only chance there is.
If you think that chance is gone, then I guess the reasonable thing is
to keep the numbers and only move those (to the end) which overlap. Or
put them into that erreneous number gap.
Patrick
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dt-bindings: clock: imx8mq: Fix numbering overlaps and gaps
2019-03-12 7:36 ` Patrick Wildt
@ 2019-03-12 20:39 ` Stephen Boyd
2019-03-12 20:59 ` Patrick Wildt
2019-03-15 13:33 ` Aisheng Dong
0 siblings, 2 replies; 9+ messages in thread
From: Stephen Boyd @ 2019-03-12 20:39 UTC (permalink / raw)
To: Patrick Wildt
Cc: Abel Vesa, Fabio Estevam, Lucas Stach, Mark Rutland, Rob Herring,
Sascha Hauer, Shawn Guo, dl-linux-imx, devicetree,
linux-arm-kernel, Linux Kernel Mailing List
Quoting Patrick Wildt (2019-03-12 00:36:54)
> On Fri, Mar 08, 2019 at 07:29:05AM -0800, Stephen Boyd wrote:
> > It's mostly about making sure that any existing dtbs don't have their
> > numbers shifted around. So hopefully any overlapping identifiers aren't
> > in use yet and then those ids can be changed while leaving the ones that
> > are in use how they are.
>
> In practice I bet no one uses Linux 5.0's i.MX8M device trees since they
> lack too much support. It's so basic it's not useful. You'd still run
> your existing non-mainline bindings until it is. Thus I would argue
> changing the ABI right now would be the only chance there is.
>
> If you think that chance is gone, then I guess the reasonable thing is
> to keep the numbers and only move those (to the end) which overlap. Or
> put them into that erreneous number gap.
>
The chance is quickly slipping away because we're going to be at -rc1
soon. I'm not the one to decide what is and isn't being used by people
out there, so I'm happy to apply this patch now before the next -rc1
comes out as long as it doesn't break anything in arm-soc area. The
confidence I'm getting isn't high though. Has anyone from NXP reviewed
this change? Maybe I can get an ack from someone else that normally
looks after the arm-soc/dts side of things here indicating that nothing
should go wrong? That would increase my confidence levels.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dt-bindings: clock: imx8mq: Fix numbering overlaps and gaps
2019-03-12 20:39 ` Stephen Boyd
@ 2019-03-12 20:59 ` Patrick Wildt
2019-03-12 22:02 ` Stephen Boyd
2019-03-15 13:33 ` Aisheng Dong
1 sibling, 1 reply; 9+ messages in thread
From: Patrick Wildt @ 2019-03-12 20:59 UTC (permalink / raw)
To: Stephen Boyd
Cc: Abel Vesa, Fabio Estevam, Lucas Stach, Mark Rutland, Rob Herring,
Sascha Hauer, Shawn Guo, dl-linux-imx, devicetree,
linux-arm-kernel, Linux Kernel Mailing List
On Tue, Mar 12, 2019 at 01:39:50PM -0700, Stephen Boyd wrote:
> Quoting Patrick Wildt (2019-03-12 00:36:54)
> > On Fri, Mar 08, 2019 at 07:29:05AM -0800, Stephen Boyd wrote:
> > > It's mostly about making sure that any existing dtbs don't have their
> > > numbers shifted around. So hopefully any overlapping identifiers aren't
> > > in use yet and then those ids can be changed while leaving the ones that
> > > are in use how they are.
> >
> > In practice I bet no one uses Linux 5.0's i.MX8M device trees since they
> > lack too much support. It's so basic it's not useful. You'd still run
> > your existing non-mainline bindings until it is. Thus I would argue
> > changing the ABI right now would be the only chance there is.
> >
> > If you think that chance is gone, then I guess the reasonable thing is
> > to keep the numbers and only move those (to the end) which overlap. Or
> > put them into that erreneous number gap.
> >
>
> The chance is quickly slipping away because we're going to be at -rc1
> soon. I'm not the one to decide what is and isn't being used by people
> out there, so I'm happy to apply this patch now before the next -rc1
> comes out as long as it doesn't break anything in arm-soc area. The
> confidence I'm getting isn't high though. Has anyone from NXP reviewed
> this change? Maybe I can get an ack from someone else that normally
> looks after the arm-soc/dts side of things here indicating that nothing
> should go wrong? That would increase my confidence levels.
The person that supplied the diff apparently is from NXP, which should
be enough to say that NXP reviewed it?
It's a bit of a shame that the ones that are CC'd keep quiet. I would
take this chance and go ahead with it. After 5.1/rc1 there will be no
chance to rectify this in a sane way.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dt-bindings: clock: imx8mq: Fix numbering overlaps and gaps
2019-03-12 20:59 ` Patrick Wildt
@ 2019-03-12 22:02 ` Stephen Boyd
0 siblings, 0 replies; 9+ messages in thread
From: Stephen Boyd @ 2019-03-12 22:02 UTC (permalink / raw)
To: Patrick Wildt
Cc: Abel Vesa, Fabio Estevam, Lucas Stach, Mark Rutland, Rob Herring,
Sascha Hauer, Shawn Guo, dl-linux-imx, devicetree,
linux-arm-kernel, Linux Kernel Mailing List
Quoting Patrick Wildt (2019-03-12 13:59:22)
> On Tue, Mar 12, 2019 at 01:39:50PM -0700, Stephen Boyd wrote:
> > Quoting Patrick Wildt (2019-03-12 00:36:54)
> > > On Fri, Mar 08, 2019 at 07:29:05AM -0800, Stephen Boyd wrote:
> > > > It's mostly about making sure that any existing dtbs don't have their
> > > > numbers shifted around. So hopefully any overlapping identifiers aren't
> > > > in use yet and then those ids can be changed while leaving the ones that
> > > > are in use how they are.
> > >
> > > In practice I bet no one uses Linux 5.0's i.MX8M device trees since they
> > > lack too much support. It's so basic it's not useful. You'd still run
> > > your existing non-mainline bindings until it is. Thus I would argue
> > > changing the ABI right now would be the only chance there is.
> > >
> > > If you think that chance is gone, then I guess the reasonable thing is
> > > to keep the numbers and only move those (to the end) which overlap. Or
> > > put them into that erreneous number gap.
> > >
> >
> > The chance is quickly slipping away because we're going to be at -rc1
> > soon. I'm not the one to decide what is and isn't being used by people
> > out there, so I'm happy to apply this patch now before the next -rc1
> > comes out as long as it doesn't break anything in arm-soc area. The
> > confidence I'm getting isn't high though. Has anyone from NXP reviewed
> > this change? Maybe I can get an ack from someone else that normally
> > looks after the arm-soc/dts side of things here indicating that nothing
> > should go wrong? That would increase my confidence levels.
>
> The person that supplied the diff apparently is from NXP, which should
> be enough to say that NXP reviewed it?
>
> It's a bit of a shame that the ones that are CC'd keep quiet. I would
> take this chance and go ahead with it. After 5.1/rc1 there will be no
> chance to rectify this in a sane way.
Ok. I'm just going to merge it and see if anyone complains. I'll send
the PR tomorrow.
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] dt-bindings: clock: imx8mq: Fix numbering overlaps and gaps
2019-03-12 20:39 ` Stephen Boyd
2019-03-12 20:59 ` Patrick Wildt
@ 2019-03-15 13:33 ` Aisheng Dong
1 sibling, 0 replies; 9+ messages in thread
From: Aisheng Dong @ 2019-03-15 13:33 UTC (permalink / raw)
To: Stephen Boyd, Patrick Wildt
Cc: Abel Vesa, Fabio Estevam, Lucas Stach, Mark Rutland, Rob Herring,
Sascha Hauer, Shawn Guo, dl-linux-imx, devicetree,
linux-arm-kernel, Linux Kernel Mailing List
> From: Stephen Boyd [mailto:sboyd@kernel.org]
>
> Quoting Patrick Wildt (2019-03-12 00:36:54)
> > On Fri, Mar 08, 2019 at 07:29:05AM -0800, Stephen Boyd wrote:
> > > It's mostly about making sure that any existing dtbs don't have
> > > their numbers shifted around. So hopefully any overlapping
> > > identifiers aren't in use yet and then those ids can be changed
> > > while leaving the ones that are in use how they are.
> >
> > In practice I bet no one uses Linux 5.0's i.MX8M device trees since
> > they lack too much support. It's so basic it's not useful. You'd
> > still run your existing non-mainline bindings until it is. Thus I
> > would argue changing the ABI right now would be the only chance there is.
> >
> > If you think that chance is gone, then I guess the reasonable thing is
> > to keep the numbers and only move those (to the end) which overlap.
> > Or put them into that erreneous number gap.
> >
>
> The chance is quickly slipping away because we're going to be at -rc1 soon. I'm
> not the one to decide what is and isn't being used by people out there, so I'm
> happy to apply this patch now before the next -rc1 comes out as long as it
> doesn't break anything in arm-soc area. The confidence I'm getting isn't high
> though. Has anyone from NXP reviewed this change? Maybe I can get an ack
> from someone else that normally looks after the arm-soc/dts side of things
> here indicating that nothing should go wrong? That would increase my
> confidence levels.
AFAIK no one out there using it for product without being able to update accordingly,
as it still has a very preliminary support.
So I agree we need to fix it at this early time
Tested-and-Acked-by: Dong Aisheng <aisheng.dong@nxp.com>
Regards
Dong Aisheng
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-03-15 13:33 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-05 9:49 [PATCH] dt-bindings: clock: imx8mq: Fix numbering overlaps and gaps Abel Vesa
2019-03-05 18:38 ` Stephen Boyd
2019-03-06 13:09 ` Abel Vesa
2019-03-08 15:29 ` Stephen Boyd
2019-03-12 7:36 ` Patrick Wildt
2019-03-12 20:39 ` Stephen Boyd
2019-03-12 20:59 ` Patrick Wildt
2019-03-12 22:02 ` Stephen Boyd
2019-03-15 13:33 ` Aisheng Dong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).