| Message ID | 20231208-palpitate-passable-c79bacf2036c@spud (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Conor Dooley |
| Headers | show |
| Series | MPFS clock fixes required for correct CAN clock modeling | expand |
On Fri, 08 Dec 2023 17:12:24 +0000, Conor Dooley wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > The CAN controller on PolarFire SoC has an AHB peripheral clock _and_ a > CAN bus clock. The bus clock was omitted when the binding was written, > but is required for operation. Make up for lost time and add it. > > Cautionary tale in adding bindings without having implemented a real > user for them perhaps. > > Fixes: c878d518d7b6 ("dt-bindings: can: mpfs: document the mpfs CAN controller") > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > .../devicetree/bindings/net/can/microchip,mpfs-can.yaml | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml: properties:clocks: {'maxItems': 2, 'items': [{'description': 'AHB peripheral clock'}, {'description': 'CAN bus clock'}]} should not be valid under {'required': ['maxItems']} hint: "maxItems" is not needed with an "items" list from schema $id: http://devicetree.org/meta-schemas/items.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231208-palpitate-passable-c79bacf2036c@spud The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Fri, Dec 08, 2023 at 12:31:00PM -0600, Rob Herring wrote: > > On Fri, 08 Dec 2023 17:12:24 +0000, Conor Dooley wrote: > > From: Conor Dooley <conor.dooley@microchip.com> > > > > The CAN controller on PolarFire SoC has an AHB peripheral clock _and_ a > > CAN bus clock. The bus clock was omitted when the binding was written, > > but is required for operation. Make up for lost time and add it. > > > > Cautionary tale in adding bindings without having implemented a real > > user for them perhaps. > > > > Fixes: c878d518d7b6 ("dt-bindings: can: mpfs: document the mpfs CAN controller") > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > --- > > .../devicetree/bindings/net/can/microchip,mpfs-can.yaml | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml: properties:clocks: {'maxItems': 2, 'items': [{'description': 'AHB peripheral clock'}, {'description': 'CAN bus clock'}]} should not be valid under {'required': ['maxItems']} > hint: "maxItems" is not needed with an "items" list > from schema $id: http://devicetree.org/meta-schemas/items.yaml# Oh dear, me of all people.
On Fri, Dec 08, 2023 at 07:25:39PM +0000, Conor Dooley wrote: > On Fri, Dec 08, 2023 at 12:31:00PM -0600, Rob Herring wrote: > > > > On Fri, 08 Dec 2023 17:12:24 +0000, Conor Dooley wrote: > > > From: Conor Dooley <conor.dooley@microchip.com> > > > > > > The CAN controller on PolarFire SoC has an AHB peripheral clock _and_ a > > > CAN bus clock. The bus clock was omitted when the binding was written, > > > but is required for operation. Make up for lost time and add it. > > > > > > Cautionary tale in adding bindings without having implemented a real > > > user for them perhaps. > > > > > > Fixes: c878d518d7b6 ("dt-bindings: can: mpfs: document the mpfs CAN controller") > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > > --- > > > .../devicetree/bindings/net/can/microchip,mpfs-can.yaml | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' > > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > > > yamllint warnings/errors: > > > > dtschema/dtc warnings/errors: > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml: properties:clocks: {'maxItems': 2, 'items': [{'description': 'AHB peripheral clock'}, {'description': 'CAN bus clock'}]} should not be valid under {'required': ['maxItems']} > > hint: "maxItems" is not needed with an "items" list > > from schema $id: http://devicetree.org/meta-schemas/items.yaml# > > > Oh dear, me of all people. Happens to the best of us. :)
On 08.12.2023 17:12:24, Conor Dooley wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > The CAN controller on PolarFire SoC has an AHB peripheral clock _and_ a > CAN bus clock. The bus clock was omitted when the binding was written, > but is required for operation. Make up for lost time and add it. > > Cautionary tale in adding bindings without having implemented a real > user for them perhaps. > > Fixes: c878d518d7b6 ("dt-bindings: can: mpfs: document the mpfs CAN controller") > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > .../devicetree/bindings/net/can/microchip,mpfs-can.yaml | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml b/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml > index 45aa3de7cf01..05f680f15b17 100644 > --- a/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml > +++ b/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml > @@ -24,7 +24,10 @@ properties: > maxItems: 1 > > clocks: > - maxItems: 1 > + maxItems: 2 > + items: > + - description: AHB peripheral clock > + - description: CAN bus clock Do we we want to have a "clock-names" property, as we need the clock rate of the CAN bus clock. Marc > > required: > - compatible > @@ -39,7 +42,7 @@ examples: > can@2010c000 { > compatible = "microchip,mpfs-can"; > reg = <0x2010c000 0x1000>; > - clocks = <&clkcfg 17>; > + clocks = <&clkcfg 17>, <&clkcfg 37>; > interrupt-parent = <&plic>; > interrupts = <56>; > }; Marc
On Tue, Dec 12, 2023 at 09:49:41PM +0100, Marc Kleine-Budde wrote: > On 08.12.2023 17:12:24, Conor Dooley wrote: > > From: Conor Dooley <conor.dooley@microchip.com> > > > > The CAN controller on PolarFire SoC has an AHB peripheral clock _and_ a > > CAN bus clock. The bus clock was omitted when the binding was written, > > but is required for operation. Make up for lost time and add it. > > > > Cautionary tale in adding bindings without having implemented a real > > user for them perhaps. > > > > Fixes: c878d518d7b6 ("dt-bindings: can: mpfs: document the mpfs CAN controller") > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > --- > > .../devicetree/bindings/net/can/microchip,mpfs-can.yaml | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml b/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml > > index 45aa3de7cf01..05f680f15b17 100644 > > --- a/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml > > +++ b/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml > > @@ -24,7 +24,10 @@ properties: > > maxItems: 1 > > > > clocks: > > - maxItems: 1 > > + maxItems: 2 > > + items: > > + - description: AHB peripheral clock > > + - description: CAN bus clock > > Do we we want to have a "clock-names" property, as we need the clock > rate of the CAN bus clock. We should not need the clock-names property to be able to get both of the clocks. clk_bulk_get_all() for example should be usable here. Cheers, Conor.
On 13.12.2023 13:02:49, Conor Dooley wrote: > On Tue, Dec 12, 2023 at 09:49:41PM +0100, Marc Kleine-Budde wrote: > > On 08.12.2023 17:12:24, Conor Dooley wrote: > > > From: Conor Dooley <conor.dooley@microchip.com> > > > > > > The CAN controller on PolarFire SoC has an AHB peripheral clock _and_ a > > > CAN bus clock. The bus clock was omitted when the binding was written, > > > but is required for operation. Make up for lost time and add it. > > > > > > Cautionary tale in adding bindings without having implemented a real > > > user for them perhaps. > > > > > > Fixes: c878d518d7b6 ("dt-bindings: can: mpfs: document the mpfs CAN controller") > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > > --- > > > .../devicetree/bindings/net/can/microchip,mpfs-can.yaml | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml b/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml > > > index 45aa3de7cf01..05f680f15b17 100644 > > > --- a/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml > > > +++ b/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml > > > @@ -24,7 +24,10 @@ properties: > > > maxItems: 1 > > > > > > clocks: > > > - maxItems: 1 > > > + maxItems: 2 > > > + items: > > > + - description: AHB peripheral clock > > > + - description: CAN bus clock > > > > Do we we want to have a "clock-names" property, as we need the clock > > rate of the CAN bus clock. > > We should not need the clock-names property to be able to get both of > the clocks. clk_bulk_get_all() for example should be usable here. ACK, but we need the clock rate of CAN clock. Does this binding check that the CAN clock rate is the 2nd one? regards, Marc
On Thu, Dec 14, 2023 at 12:31:04PM +0100, Marc Kleine-Budde wrote: > On 13.12.2023 13:02:49, Conor Dooley wrote: > > On Tue, Dec 12, 2023 at 09:49:41PM +0100, Marc Kleine-Budde wrote: > > > On 08.12.2023 17:12:24, Conor Dooley wrote: > > > > From: Conor Dooley <conor.dooley@microchip.com> > > > > > > > > The CAN controller on PolarFire SoC has an AHB peripheral clock _and_ a > > > > CAN bus clock. The bus clock was omitted when the binding was written, > > > > but is required for operation. Make up for lost time and add it. > > > > > > > > Cautionary tale in adding bindings without having implemented a real > > > > user for them perhaps. > > > > > > > > Fixes: c878d518d7b6 ("dt-bindings: can: mpfs: document the mpfs CAN controller") > > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > > > --- > > > > .../devicetree/bindings/net/can/microchip,mpfs-can.yaml | 7 +++++-- > > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml b/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml > > > > index 45aa3de7cf01..05f680f15b17 100644 > > > > --- a/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml > > > > +++ b/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml > > > > @@ -24,7 +24,10 @@ properties: > > > > maxItems: 1 > > > > > > > > clocks: > > > > - maxItems: 1 > > > > + maxItems: 2 > > > > + items: > > > > + - description: AHB peripheral clock > > > > + - description: CAN bus clock > > > > > > Do we we want to have a "clock-names" property, as we need the clock > > > rate of the CAN bus clock. > > > > We should not need the clock-names property to be able to get both of > > the clocks. clk_bulk_get_all() for example should be usable here. > > ACK, but we need the clock rate of CAN clock. Does this binding check > that the CAN clock rate is the 2nd one? The items list requires that the can clock be the second one, so drivers etc can rely on that ordering.
On 14.12.2023 13:16:55, Conor Dooley wrote: > > > > > --- a/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml > > > > > +++ b/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml > > > > > @@ -24,7 +24,10 @@ properties: > > > > > maxItems: 1 > > > > > > > > > > clocks: > > > > > - maxItems: 1 > > > > > + maxItems: 2 > > > > > + items: > > > > > + - description: AHB peripheral clock > > > > > + - description: CAN bus clock > > > > > > > > Do we we want to have a "clock-names" property, as we need the clock > > > > rate of the CAN bus clock. > > > > > > We should not need the clock-names property to be able to get both of > > > the clocks. clk_bulk_get_all() for example should be usable here. > > > > ACK, but we need the clock rate of CAN clock. Does this binding check > > that the CAN clock rate is the 2nd one? > > The items list requires that the can clock be the second one, so drivers > etc can rely on that ordering. Thanks for the clarification, Marc
diff --git a/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml b/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml index 45aa3de7cf01..05f680f15b17 100644 --- a/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml +++ b/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml @@ -24,7 +24,10 @@ properties: maxItems: 1 clocks: - maxItems: 1 + maxItems: 2 + items: + - description: AHB peripheral clock + - description: CAN bus clock required: - compatible @@ -39,7 +42,7 @@ examples: can@2010c000 { compatible = "microchip,mpfs-can"; reg = <0x2010c000 0x1000>; - clocks = <&clkcfg 17>; + clocks = <&clkcfg 17>, <&clkcfg 37>; interrupt-parent = <&plic>; interrupts = <56>; };