From: Conor Dooley <[email protected]>
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 <[email protected]>
---
.../devicetree/bindings/net/can/microchip,mpfs-can.yaml | 6 ++++--
1 file changed, 4 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..01e4d4a54df6 100644
--- a/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml
+++ b/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml
@@ -24,7 +24,9 @@ properties:
maxItems: 1
clocks:
- maxItems: 1
+ items:
+ - description: AHB peripheral clock
+ - description: CAN bus clock
required:
- compatible
@@ -39,7 +41,7 @@ examples:
can@2010c000 {
compatible = "microchip,mpfs-can";
reg = <0x2010c000 0x1000>;
- clocks = <&clkcfg 17>;
+ clocks = <&clkcfg 17>, <&clkcfg 37>;
interrupt-parent = <&plic>;
interrupts = <56>;
};
--
2.43.0
On 22.01.2024 12:19:50, Conor Dooley wrote:
> From: Conor Dooley <[email protected]>
>
> 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 <[email protected]>
> ---
> .../devicetree/bindings/net/can/microchip,mpfs-can.yaml | 6 ++++--
> 1 file changed, 4 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..01e4d4a54df6 100644
> --- a/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml
> +++ b/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml
> @@ -24,7 +24,9 @@ properties:
> maxItems: 1
>
> clocks:
> - maxItems: 1
> + items:
> + - description: AHB peripheral clock
> + - description: CAN bus clock
What about adding clock-names, so that the order can be checked
automatically?
>
> required:
> - compatible
> @@ -39,7 +41,7 @@ examples:
> can@2010c000 {
> compatible = "microchip,mpfs-can";
> reg = <0x2010c000 0x1000>;
> - clocks = <&clkcfg 17>;
> + clocks = <&clkcfg 17>, <&clkcfg 37>;
> interrupt-parent = <&plic>;
> interrupts = <56>;
> };
> --
> 2.43.0
>
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
On Mon, Jan 22, 2024 at 02:13:16PM +0100, Marc Kleine-Budde wrote:
> On 22.01.2024 12:19:50, Conor Dooley wrote:
> > From: Conor Dooley <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > .../devicetree/bindings/net/can/microchip,mpfs-can.yaml | 6 ++++--
> > 1 file changed, 4 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..01e4d4a54df6 100644
> > --- a/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml
> > +++ b/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml
> > @@ -24,7 +24,9 @@ properties:
> > maxItems: 1
> >
> > clocks:
> > - maxItems: 1
> > + items:
> > + - description: AHB peripheral clock
> > + - description: CAN bus clock
>
> What about adding clock-names, so that the order can be checked
> automatically?
I don't personally care for doing so, but if your heart is set on having
them, then sure.
Cheers,
Conor.
On 22.01.2024 14:21:04, Conor Dooley wrote:
> On Mon, Jan 22, 2024 at 02:13:16PM +0100, Marc Kleine-Budde wrote:
> > On 22.01.2024 12:19:50, Conor Dooley wrote:
> > > From: Conor Dooley <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > .../devicetree/bindings/net/can/microchip,mpfs-can.yaml | 6 ++++--
> > > 1 file changed, 4 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..01e4d4a54df6 100644
> > > --- a/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml
> > > +++ b/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml
> > > @@ -24,7 +24,9 @@ properties:
> > > maxItems: 1
> > >
> > > clocks:
> > > - maxItems: 1
> > > + items:
> > > + - description: AHB peripheral clock
> > > + - description: CAN bus clock
> >
> > What about adding clock-names, so that the order can be checked
> > automatically?
>
> I don't personally care for doing so, but if your heart is set on having
> them, then sure.
Usually the CAN driver needs to have the clock rate of the clocks that
the basis for the CAN bus clock. Looking at the clocks description it's
probably the 2nd one.
With clock-names we can automatically check that the 2nd clock is always
the CAN clock.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
On Mon, Jan 22, 2024 at 03:46:04PM +0100, Marc Kleine-Budde wrote:
> On 22.01.2024 14:21:04, Conor Dooley wrote:
> > On Mon, Jan 22, 2024 at 02:13:16PM +0100, Marc Kleine-Budde wrote:
> > > On 22.01.2024 12:19:50, Conor Dooley wrote:
> > > > From: Conor Dooley <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > ---
> > > > .../devicetree/bindings/net/can/microchip,mpfs-can.yaml | 6 ++++--
> > > > 1 file changed, 4 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..01e4d4a54df6 100644
> > > > --- a/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml
> > > > +++ b/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml
> > > > @@ -24,7 +24,9 @@ properties:
> > > > maxItems: 1
> > > >
> > > > clocks:
> > > > - maxItems: 1
> > > > + items:
> > > > + - description: AHB peripheral clock
> > > > + - description: CAN bus clock
> > >
> > > What about adding clock-names, so that the order can be checked
> > > automatically?
> >
> > I don't personally care for doing so, but if your heart is set on having
> > them, then sure.
>
> Usually the CAN driver needs to have the clock rate of the clocks that
> the basis for the CAN bus clock. Looking at the clocks description it's
> probably the 2nd one.
>
> With clock-names we can automatically check that the 2nd clock is always
> the CAN clock.
I think we already had this discussion on v1, where I said that the
binding requires the clocks to be in that order, regardless of whether
or not clock-names is provided. You feel more strongly about it than I
do, so I will add them when I get around to sending a v3.
Cheers,
Conor.
On 22.01.2024 14:56:09, Conor Dooley wrote:
> On Mon, Jan 22, 2024 at 03:46:04PM +0100, Marc Kleine-Budde wrote:
> > On 22.01.2024 14:21:04, Conor Dooley wrote:
> > > On Mon, Jan 22, 2024 at 02:13:16PM +0100, Marc Kleine-Budde wrote:
> > > > On 22.01.2024 12:19:50, Conor Dooley wrote:
> > > > > From: Conor Dooley <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > ---
> > > > > .../devicetree/bindings/net/can/microchip,mpfs-can.yaml | 6 ++++--
> > > > > 1 file changed, 4 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..01e4d4a54df6 100644
> > > > > --- a/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml
> > > > > +++ b/Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml
> > > > > @@ -24,7 +24,9 @@ properties:
> > > > > maxItems: 1
> > > > >
> > > > > clocks:
> > > > > - maxItems: 1
> > > > > + items:
> > > > > + - description: AHB peripheral clock
> > > > > + - description: CAN bus clock
> > > >
> > > > What about adding clock-names, so that the order can be checked
> > > > automatically?
> > >
> > > I don't personally care for doing so, but if your heart is set on having
> > > them, then sure.
> >
> > Usually the CAN driver needs to have the clock rate of the clocks that
> > the basis for the CAN bus clock. Looking at the clocks description it's
> > probably the 2nd one.
> >
> > With clock-names we can automatically check that the 2nd clock is always
> > the CAN clock.
>
> I think we already had this discussion on v1, where I said that the
> binding requires the clocks to be in that order, regardless of whether
> or not clock-names is provided. You feel more strongly about it than I
> do, so I will add them when I get around to sending a v3.
Yes, this discussion sounded very familiar to me, never mind. Keep it as
is, and let's get this binding and the CAN driver upstream!
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
On Mon, Jan 22, 2024 at 04:31:32PM +0100, Marc Kleine-Budde wrote:
> let's get this binding and the CAN driver upstream!
FWIW, the driver seems to be in a good state (or as good as it can to
someone unaware of the goings on of CAN). Hopefully the author gets
a chance to send it out soon.
Cheers,
Conor.
On 22/01/2024 13:19, Conor Dooley wrote:
> From: Conor Dooley <[email protected]>
>
> 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 <[email protected]>
> ---
> .../devicetree/bindings/net/can/microchip,mpfs-can.yaml | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On Mon, Jan 22, 2024 at 04:31:32PM +0100, Marc Kleine-Budde wrote:
> On 22.01.2024 14:56:09, Conor Dooley wrote:
> > I think we already had this discussion on v1, where I said that the
> > binding requires the clocks to be in that order, regardless of whether
> > or not clock-names is provided. You feel more strongly about it than I
> > do, so I will add them when I get around to sending a v3.
>
> Yes, this discussion sounded very familiar to me, never mind. Keep it as
> is, and let's get this binding and the CAN driver upstream!
BTW, I didn't see an ack on this nor do I see it in linux-next (yet).
Are you expecting the patch to go with the rest via the clock tree,
via the DT tree or will you be taking it with CAN stuff via netdev?
I can resend this one patch with a netdev appropriate subject prefix
if you like.
Thanks,
Conor.
On 30.01.2024 12:11:44, Conor Dooley wrote:
> On Mon, Jan 22, 2024 at 04:31:32PM +0100, Marc Kleine-Budde wrote:
> > On 22.01.2024 14:56:09, Conor Dooley wrote:
>
> > > I think we already had this discussion on v1, where I said that the
> > > binding requires the clocks to be in that order, regardless of whether
> > > or not clock-names is provided. You feel more strongly about it than I
> > > do, so I will add them when I get around to sending a v3.
> >
> > Yes, this discussion sounded very familiar to me, never mind. Keep it as
> > is, and let's get this binding and the CAN driver upstream!
>
> BTW, I didn't see an ack on this nor do I see it in linux-next (yet).
> Are you expecting the patch to go with the rest via the clock tree,
> via the DT tree or will you be taking it with CAN stuff via netdev?
>
> I can resend this one patch with a netdev appropriate subject prefix
> if you like.
Feel free to take the whole series via the clock tree.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
On Tue, Jan 30, 2024 at 02:01:09PM +0100, Marc Kleine-Budde wrote:
> On 30.01.2024 12:11:44, Conor Dooley wrote:
> > On Mon, Jan 22, 2024 at 04:31:32PM +0100, Marc Kleine-Budde wrote:
> > > On 22.01.2024 14:56:09, Conor Dooley wrote:
> >
> > > > I think we already had this discussion on v1, where I said that the
> > > > binding requires the clocks to be in that order, regardless of whether
> > > > or not clock-names is provided. You feel more strongly about it than I
> > > > do, so I will add them when I get around to sending a v3.
> > >
> > > Yes, this discussion sounded very familiar to me, never mind. Keep it as
> > > is, and let's get this binding and the CAN driver upstream!
> >
> > BTW, I didn't see an ack on this nor do I see it in linux-next (yet).
> > Are you expecting the patch to go with the rest via the clock tree,
> > via the DT tree or will you be taking it with CAN stuff via netdev?
> >
> > I can resend this one patch with a netdev appropriate subject prefix
> > if you like.
>
> Feel free to take the whole series via the clock tree.
Okay, I've gone and applied all of the binding and driver patches to the
branch we will send to Stephen. Thanks!