| Message ID | 20241031-mbly-clk-v1-1-89d8b28e3006@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Usable clocks on Mobileye EyeQ5 & EyeQ6H | expand |
On Thu, Oct 31, 2024 at 04:52:51PM +0100, Théo Lebrun wrote: > Some compatibles expose a single clock. For those, we used to let them > using `#clock-cells = <0>` (ie <&olb> reference rather than <&olb 0>). > > Switch away from that: enforce a cell for all compatibles. This is more > straight forward, and avoids devicetree changes whenever a compatible > goes from exposing a single clock to multiple ones. Your reasoning is flawed. Changing #clock-cells is an ABI break. So you should only be changing this if it was just wrong. And if it's not wrong in some cases, you shouldn't be changing those. The h/w either has 1 clock or multiple and #clocks-cells should match. > > Side benefit: dt-bindings is simpler. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml | 24 +--------------------- > 1 file changed, 1 insertion(+), 23 deletions(-) > > diff --git a/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml > index f7e606d45ebcfc46ffe076e23a2ed514bfff9b8f..6d11472ba5a704d5d20f43776e5867f507a39242 100644 > --- a/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml > +++ b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml > @@ -41,9 +41,7 @@ properties: > enum: [ 1, 2 ] > > '#clock-cells': > - description: > - Cell is clock index. Optional if compatible has a single clock. > - enum: [ 0, 1 ] > + const: 1 > > clocks: > maxItems: 1 > @@ -312,26 +310,6 @@ allOf: > properties: > '#reset-cells': false > > - # Compatibles exposing a single clock. > - - if: > - properties: > - compatible: > - contains: > - enum: > - - mobileye,eyeq6h-central-olb > - - mobileye,eyeq6h-east-olb > - - mobileye,eyeq6h-west-olb > - - mobileye,eyeq6h-ddr0-olb > - - mobileye,eyeq6h-ddr1-olb > - then: > - properties: > - '#clock-cells': > - const: 0 > - else: > - properties: > - '#clock-cells': > - const: 1 > - > # Only EyeQ5 has pinctrl in OLB. > - if: > not: > > -- > 2.47.0 >
On Mon Nov 4, 2024 at 4:37 PM CET, Rob Herring wrote: > On Thu, Oct 31, 2024 at 04:52:51PM +0100, Théo Lebrun wrote: > > Some compatibles expose a single clock. For those, we used to let them > > using `#clock-cells = <0>` (ie <&olb> reference rather than <&olb 0>). > > > > Switch away from that: enforce a cell for all compatibles. This is more > > straight forward, and avoids devicetree changes whenever a compatible > > goes from exposing a single clock to multiple ones. > > Your reasoning is flawed. Changing #clock-cells is an ABI break. So you > should only be changing this if it was just wrong. And if it's not wrong > in some cases, you shouldn't be changing those. The h/w either has 1 > clock or multiple and #clocks-cells should match. I see your reasoning, and I agree that changing #clock-cells is an ABI break. However, there are two things to take into account: - We do not (yet?) have an omniscient view of the hardware. We do not know what every single register in those memory regions do. Some clocks might be lurking in the shadows, especially as we don't support many HW capabilities yet. - The earlier the better. If we discover later down the road that, indeed, some more clocks were hiding, we'll have to do an ABI break. At that point, some people might actually be using the platform. Seeing what we currently have supported upstream versus the amount of HW blocks available in the SoC, I cannot imagine anyone using the platform with an upstream kernel. So the choice is: - potential ABI break in the future, once people use the platform, or, - guaranteed ABI break now, when no one is using it. I pick option two! Do you agree with the thought process? Thanks Rob, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Mon, Nov 04, 2024 at 05:46:10PM +0100, Théo Lebrun wrote: > On Mon Nov 4, 2024 at 4:37 PM CET, Rob Herring wrote: > > On Thu, Oct 31, 2024 at 04:52:51PM +0100, Théo Lebrun wrote: > > > Some compatibles expose a single clock. For those, we used to let them > > > using `#clock-cells = <0>` (ie <&olb> reference rather than <&olb 0>). > > > > > > Switch away from that: enforce a cell for all compatibles. This is more > > > straight forward, and avoids devicetree changes whenever a compatible > > > goes from exposing a single clock to multiple ones. > > > > Your reasoning is flawed. Changing #clock-cells is an ABI break. So you > > should only be changing this if it was just wrong. And if it's not wrong > > in some cases, you shouldn't be changing those. The h/w either has 1 > > clock or multiple and #clocks-cells should match. > > I see your reasoning, and I agree that changing #clock-cells is an ABI > break. However, there are two things to take into account: > > - We do not (yet?) have an omniscient view of the hardware. We do not > know what every single register in those memory regions do. > > Some clocks might be lurking in the shadows, especially as we don't > support many HW capabilities yet. > > - The earlier the better. If we discover later down the road that, > indeed, some more clocks were hiding, we'll have to do an ABI break. > > At that point, some people might actually be using the platform. > Seeing what we currently have supported upstream versus the amount > of HW blocks available in the SoC, I cannot imagine anyone using the > platform with an upstream kernel. > > So the choice is: > - potential ABI break in the future, once people use the platform, or, > - guaranteed ABI break now, when no one is using it. > > I pick option two! Do you agree with the thought process? Ultimately, it is up to you and the maintainers for the platform to decide. I only ask that ABI breaks are called out as ABI breaks with reasoning given for the ABI break. I had no clue whether you have access to RTL or are reverse engineering this or something in between. Please summarize the above explanation for the commit msg. Rob
diff --git a/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml index f7e606d45ebcfc46ffe076e23a2ed514bfff9b8f..6d11472ba5a704d5d20f43776e5867f507a39242 100644 --- a/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml +++ b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml @@ -41,9 +41,7 @@ properties: enum: [ 1, 2 ] '#clock-cells': - description: - Cell is clock index. Optional if compatible has a single clock. - enum: [ 0, 1 ] + const: 1 clocks: maxItems: 1 @@ -312,26 +310,6 @@ allOf: properties: '#reset-cells': false - # Compatibles exposing a single clock. - - if: - properties: - compatible: - contains: - enum: - - mobileye,eyeq6h-central-olb - - mobileye,eyeq6h-east-olb - - mobileye,eyeq6h-west-olb - - mobileye,eyeq6h-ddr0-olb - - mobileye,eyeq6h-ddr1-olb - then: - properties: - '#clock-cells': - const: 0 - else: - properties: - '#clock-cells': - const: 1 - # Only EyeQ5 has pinctrl in OLB. - if: not:
Some compatibles expose a single clock. For those, we used to let them using `#clock-cells = <0>` (ie <&olb> reference rather than <&olb 0>). Switch away from that: enforce a cell for all compatibles. This is more straight forward, and avoids devicetree changes whenever a compatible goes from exposing a single clock to multiple ones. Side benefit: dt-bindings is simpler. Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml | 24 +--------------------- 1 file changed, 1 insertion(+), 23 deletions(-)