| Message ID | 20240924141247.132455-2-ciprianmarian.costea@oss.nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | add NXP LINFlexD UART clock support for S32G2/S32G3 SoC | expand |
On Tue, Sep 24, 2024 at 05:12:46PM +0300, Ciprian Costea wrote: > From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> > > Add clock definitions for NXP LINFlexD UART bindings > and update the binding examples with S32G2 node. > > Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> > --- > .../bindings/serial/fsl,s32-linflexuart.yaml | 21 +++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > index 4171f524a928..45fcab9e186d 100644 > --- a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > +++ b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > @@ -34,6 +34,14 @@ properties: > interrupts: > maxItems: 1 > > + clocks: > + maxItems: 2 > + > + clock-names: > + items: > + - const: ipg > + - const: lin Can all devices have 2 clocks, or just the s32g2? > + > required: > - compatible > - reg > @@ -48,3 +56,16 @@ examples: > reg = <0x40053000 0x1000>; > interrupts = <0 59 4>; > }; > + > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + > + serial@401c8000 { > + compatible = "nxp,s32g2-linflexuart", > + "fsl,s32v234-linflexuart"; > + reg = <0x401c8000 0x3000>; > + interrupts = <GIC_SPI 82 IRQ_TYPE_EDGE_RISING>; > + clocks = <&clks 14>, <&clks 13>; > + clock-names = "lin", "ipg"; > + }; > -- > 2.45.2 >
On 9/24/2024 5:24 PM, Conor Dooley wrote: > On Tue, Sep 24, 2024 at 05:12:46PM +0300, Ciprian Costea wrote: >> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> >> >> Add clock definitions for NXP LINFlexD UART bindings >> and update the binding examples with S32G2 node. >> >> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> >> --- >> .../bindings/serial/fsl,s32-linflexuart.yaml | 21 +++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml >> index 4171f524a928..45fcab9e186d 100644 >> --- a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml >> +++ b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml >> @@ -34,6 +34,14 @@ properties: >> interrupts: >> maxItems: 1 >> >> + clocks: >> + maxItems: 2 >> + >> + clock-names: >> + items: >> + - const: ipg >> + - const: lin > > Can all devices have 2 clocks, or just the s32g2? > Hello Conor, All devices (S32G2, S32G3 and S32V234) have 2 clocks for LINFlexD module. They are: "lin" which is the frequency of the baud clock and "ipg" which drives the access to the LINFlexD iomapped registers. Best Regards, Ciprian >> + >> required: >> - compatible >> - reg >> @@ -48,3 +56,16 @@ examples: >> reg = <0x40053000 0x1000>; >> interrupts = <0 59 4>; >> }; >> + >> + - | >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + #include <dt-bindings/interrupt-controller/irq.h> >> + >> + serial@401c8000 { >> + compatible = "nxp,s32g2-linflexuart", >> + "fsl,s32v234-linflexuart"; >> + reg = <0x401c8000 0x3000>; >> + interrupts = <GIC_SPI 82 IRQ_TYPE_EDGE_RISING>; >> + clocks = <&clks 14>, <&clks 13>; >> + clock-names = "lin", "ipg"; >> + }; >> -- >> 2.45.2 >>
On Tue, Sep 24, 2024 at 05:52:13PM +0300, Ciprian Marian Costea wrote: > On 9/24/2024 5:24 PM, Conor Dooley wrote: > > On Tue, Sep 24, 2024 at 05:12:46PM +0300, Ciprian Costea wrote: > > > From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> > > > > > > Add clock definitions for NXP LINFlexD UART bindings > > > and update the binding examples with S32G2 node. > > > > > > Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> > > > --- > > > .../bindings/serial/fsl,s32-linflexuart.yaml | 21 +++++++++++++++++++ > > > 1 file changed, 21 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > > > index 4171f524a928..45fcab9e186d 100644 > > > --- a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > > > +++ b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > > > @@ -34,6 +34,14 @@ properties: > > > interrupts: > > > maxItems: 1 > > > + clocks: > > > + maxItems: 2 > > > + > > > + clock-names: > > > + items: > > > + - const: ipg > > > + - const: lin > > > > Can all devices have 2 clocks, or just the s32g2? > > > > All devices (S32G2, S32G3 and S32V234) have 2 clocks for LINFlexD module. I see. How come the driver is capable of working without them? > They are: "lin" which is the frequency of the baud clock and "ipg" which > drives the access to the LINFlexD iomapped registers. It would be good to have an items list in the clocks property with that information.
On 9/24/2024 6:01 PM, Conor Dooley wrote: > On Tue, Sep 24, 2024 at 05:52:13PM +0300, Ciprian Marian Costea wrote: >> On 9/24/2024 5:24 PM, Conor Dooley wrote: >>> On Tue, Sep 24, 2024 at 05:12:46PM +0300, Ciprian Costea wrote: >>>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> >>>> >>>> Add clock definitions for NXP LINFlexD UART bindings >>>> and update the binding examples with S32G2 node. >>>> >>>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> >>>> --- >>>> .../bindings/serial/fsl,s32-linflexuart.yaml | 21 +++++++++++++++++++ >>>> 1 file changed, 21 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml >>>> index 4171f524a928..45fcab9e186d 100644 >>>> --- a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml >>>> +++ b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml >>>> @@ -34,6 +34,14 @@ properties: >>>> interrupts: >>>> maxItems: 1 >>>> + clocks: >>>> + maxItems: 2 >>>> + >>>> + clock-names: >>>> + items: >>>> + - const: ipg >>>> + - const: lin >>> >>> Can all devices have 2 clocks, or just the s32g2? >>> >> >> All devices (S32G2, S32G3 and S32V234) have 2 clocks for LINFlexD module. > > I see. How come the driver is capable of working without them? > The driver was working because the LINFlexD clocks were configured and kept enabled by the downstream bootloader (TF-A and U-Boot). This is not ideal since LINFlexD Linux driver should manage those clocks independently and not rely on a previous bootloader configuration (hence the need for this current patchset). >> They are: "lin" which is the frequency of the baud clock and "ipg" which >> drives the access to the LINFlexD iomapped registers. > > It would be good to have an items list in the clocks property with that > information. Thanks for this suggestion. I would add such information in V2.
On Tue, Sep 24, 2024 at 06:17:11PM +0300, Ciprian Marian Costea wrote: > On 9/24/2024 6:01 PM, Conor Dooley wrote: > > On Tue, Sep 24, 2024 at 05:52:13PM +0300, Ciprian Marian Costea wrote: > > > On 9/24/2024 5:24 PM, Conor Dooley wrote: > > > > On Tue, Sep 24, 2024 at 05:12:46PM +0300, Ciprian Costea wrote: > > > > > From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> > > > > > > > > > > Add clock definitions for NXP LINFlexD UART bindings > > > > > and update the binding examples with S32G2 node. > > > > > > > > > > Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> > > > > > --- > > > > > .../bindings/serial/fsl,s32-linflexuart.yaml | 21 +++++++++++++++++++ > > > > > 1 file changed, 21 insertions(+) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > > > > > index 4171f524a928..45fcab9e186d 100644 > > > > > --- a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > > > > > +++ b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > > > > > @@ -34,6 +34,14 @@ properties: > > > > > interrupts: > > > > > maxItems: 1 > > > > > + clocks: > > > > > + maxItems: 2 > > > > > + > > > > > + clock-names: > > > > > + items: > > > > > + - const: ipg > > > > > + - const: lin > > > > > > > > Can all devices have 2 clocks, or just the s32g2? > > > > > > > > > > All devices (S32G2, S32G3 and S32V234) have 2 clocks for LINFlexD module. > > > > I see. How come the driver is capable of working without them? > > > > The driver was working because the LINFlexD clocks were configured and kept > enabled by the downstream bootloader (TF-A and U-Boot). This is not ideal > since LINFlexD Linux driver should manage those clocks independently and not > rely on a previous bootloader configuration (hence the need for this current > patchset). I'd also mark them as required in the binding too, but the driver will still have to account for them being missing, for backwards compatibility reasons. Add a comment explaining the history to the commit message when you do that.
On 9/24/2024 6:27 PM, Conor Dooley wrote: > On Tue, Sep 24, 2024 at 06:17:11PM +0300, Ciprian Marian Costea wrote: >> On 9/24/2024 6:01 PM, Conor Dooley wrote: >>> On Tue, Sep 24, 2024 at 05:52:13PM +0300, Ciprian Marian Costea wrote: >>>> On 9/24/2024 5:24 PM, Conor Dooley wrote: >>>>> On Tue, Sep 24, 2024 at 05:12:46PM +0300, Ciprian Costea wrote: >>>>>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> >>>>>> >>>>>> Add clock definitions for NXP LINFlexD UART bindings >>>>>> and update the binding examples with S32G2 node. >>>>>> >>>>>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> >>>>>> --- >>>>>> .../bindings/serial/fsl,s32-linflexuart.yaml | 21 +++++++++++++++++++ >>>>>> 1 file changed, 21 insertions(+) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml >>>>>> index 4171f524a928..45fcab9e186d 100644 >>>>>> --- a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml >>>>>> +++ b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml >>>>>> @@ -34,6 +34,14 @@ properties: >>>>>> interrupts: >>>>>> maxItems: 1 >>>>>> + clocks: >>>>>> + maxItems: 2 >>>>>> + >>>>>> + clock-names: >>>>>> + items: >>>>>> + - const: ipg >>>>>> + - const: lin >>>>> >>>>> Can all devices have 2 clocks, or just the s32g2? >>>>> >>>> >>>> All devices (S32G2, S32G3 and S32V234) have 2 clocks for LINFlexD module. >>> >>> I see. How come the driver is capable of working without them? >>> >> >> The driver was working because the LINFlexD clocks were configured and kept >> enabled by the downstream bootloader (TF-A and U-Boot). This is not ideal >> since LINFlexD Linux driver should manage those clocks independently and not >> rely on a previous bootloader configuration (hence the need for this current >> patchset). > > I'd also mark them as required in the binding too, but the driver will > still have to account for them being missing, for backwards > compatibility reasons. Add a comment explaining the history to the > commit message when you do that. Already in the second patch from this patchset the clocking support was added in the LINFlexD driver as optional, indeed for backwards compatibility. I presumed that because of this optional clock enablement, I should not add the clocks as required in the bindings, but I will do so in V2. Thanks.
On Tue, Sep 24, 2024 at 06:32:30PM +0300, Ciprian Marian Costea wrote: > On 9/24/2024 6:27 PM, Conor Dooley wrote: > > On Tue, Sep 24, 2024 at 06:17:11PM +0300, Ciprian Marian Costea wrote: > > > On 9/24/2024 6:01 PM, Conor Dooley wrote: > > > > On Tue, Sep 24, 2024 at 05:52:13PM +0300, Ciprian Marian Costea wrote: > > > > > On 9/24/2024 5:24 PM, Conor Dooley wrote: > > > > > > On Tue, Sep 24, 2024 at 05:12:46PM +0300, Ciprian Costea wrote: > > > > > > > From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> > > > > > > > > > > > > > > Add clock definitions for NXP LINFlexD UART bindings > > > > > > > and update the binding examples with S32G2 node. > > > > > > > > > > > > > > Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> > > > > > > > --- > > > > > > > .../bindings/serial/fsl,s32-linflexuart.yaml | 21 +++++++++++++++++++ > > > > > > > 1 file changed, 21 insertions(+) > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > > > > > > > index 4171f524a928..45fcab9e186d 100644 > > > > > > > --- a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > > > > > > > +++ b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > > > > > > > @@ -34,6 +34,14 @@ properties: > > > > > > > interrupts: > > > > > > > maxItems: 1 > > > > > > > + clocks: > > > > > > > + maxItems: 2 > > > > > > > + > > > > > > > + clock-names: > > > > > > > + items: > > > > > > > + - const: ipg > > > > > > > + - const: lin > > > > > > > > > > > > Can all devices have 2 clocks, or just the s32g2? > > > > > > > > > > > > > > > > All devices (S32G2, S32G3 and S32V234) have 2 clocks for LINFlexD module. > > > > > > > > I see. How come the driver is capable of working without them? > > > > > > > > > > The driver was working because the LINFlexD clocks were configured and kept > > > enabled by the downstream bootloader (TF-A and U-Boot). This is not ideal > > > since LINFlexD Linux driver should manage those clocks independently and not > > > rely on a previous bootloader configuration (hence the need for this current > > > patchset). > > > > I'd also mark them as required in the binding too, but the driver will > > still have to account for them being missing, for backwards > > compatibility reasons. Add a comment explaining the history to the > > commit message when you do that. > > Already in the second patch from this patchset the clocking support was > added in the LINFlexD driver as optional, indeed for backwards > compatibility. Oh great. > I presumed that because of this optional clock enablement, I should not add > the clocks as required in the bindings, but I will do so in V2. Thanks. IMO required is correct, since it would not have worked properly if the bootloader hadn't done the setup.
diff --git a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml index 4171f524a928..45fcab9e186d 100644 --- a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml +++ b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml @@ -34,6 +34,14 @@ properties: interrupts: maxItems: 1 + clocks: + maxItems: 2 + + clock-names: + items: + - const: ipg + - const: lin + required: - compatible - reg @@ -48,3 +56,16 @@ examples: reg = <0x40053000 0x1000>; interrupts = <0 59 4>; }; + + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/interrupt-controller/irq.h> + + serial@401c8000 { + compatible = "nxp,s32g2-linflexuart", + "fsl,s32v234-linflexuart"; + reg = <0x401c8000 0x3000>; + interrupts = <GIC_SPI 82 IRQ_TYPE_EDGE_RISING>; + clocks = <&clks 14>, <&clks 13>; + clock-names = "lin", "ipg"; + };