diff mbox series

[net-next,1/5] dt-bindings: net: r8a779f0-ether-switch: Add ACLK

Message ID 20230529080840.1156458-2-yoshihiro.shimoda.uh@renesas.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: renesas: rswitch: Improve performance of TX | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers success CCed 14 of 14 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 22 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Yoshihiro Shimoda May 29, 2023, 8:08 a.m. UTC
Add ACLK of GWCA which needs to calculate registers' values for
rate limiter feature.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 .../bindings/net/renesas,r8a779f0-ether-switch.yaml    | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Conor Dooley May 29, 2023, 6:36 p.m. UTC | #1
Hey,

On Mon, May 29, 2023 at 05:08:36PM +0900, Yoshihiro Shimoda wrote:
> Add ACLK of GWCA which needs to calculate registers' values for
> rate limiter feature.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  .../bindings/net/renesas,r8a779f0-ether-switch.yaml    | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml b/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> index e933a1e48d67..cbe05fdcadaf 100644
> --- a/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> +++ b/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> @@ -75,7 +75,12 @@ properties:
>        - const: rmac2_phy
>  
>    clocks:
> -    maxItems: 1
> +    maxItems: 2
> +
> +  clock-names:
> +    items:
> +      - const: fck
> +      - const: aclk

Since having both clocks is now required, please add some detail in the
commit message about why that is the case. Reading it sounds like this
is an optional new feature & not something that is required.

Thanks,
Conor.

>  
>    resets:
>      maxItems: 1
> @@ -221,7 +226,8 @@ examples:
>                            "rmac2_mdio",
>                            "rmac0_phy", "rmac1_phy",
>                            "rmac2_phy";
> -        clocks = <&cpg CPG_MOD 1505>;
> +        clocks = <&cpg CPG_MOD 1505>, <&cpg CPG_CORE R8A779F0_CLK_S0D2_HSC>;
> +        clock-names = "fck", "aclk";
>          power-domains = <&sysc R8A779F0_PD_ALWAYS_ON>;
>          resets = <&cpg 1505>;
>  
> -- 
> 2.25.1
>
Andrew Lunn May 29, 2023, 8:11 p.m. UTC | #2
On Mon, May 29, 2023 at 07:36:03PM +0100, Conor Dooley wrote:
> Hey,
> 
> On Mon, May 29, 2023 at 05:08:36PM +0900, Yoshihiro Shimoda wrote:
> > Add ACLK of GWCA which needs to calculate registers' values for
> > rate limiter feature.
> > 
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  .../bindings/net/renesas,r8a779f0-ether-switch.yaml    | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml b/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > index e933a1e48d67..cbe05fdcadaf 100644
> > --- a/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > +++ b/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > @@ -75,7 +75,12 @@ properties:
> >        - const: rmac2_phy
> >  
> >    clocks:
> > -    maxItems: 1
> > +    maxItems: 2
> > +
> > +  clock-names:
> > +    items:
> > +      - const: fck
> > +      - const: aclk
> 
> Since having both clocks is now required, please add some detail in the
> commit message about why that is the case. Reading it sounds like this
> is an optional new feature & not something that is required.

This is something i wondered about, backwards compatibility with old
DT blobs. In the C code it is optional, and has a default clock rate
if the clock is not present. So the yaml should not enforce an aclk
member.

	Andrew
Conor Dooley May 29, 2023, 8:44 p.m. UTC | #3
On Mon, May 29, 2023 at 10:11:12PM +0200, Andrew Lunn wrote:
> On Mon, May 29, 2023 at 07:36:03PM +0100, Conor Dooley wrote:
> > On Mon, May 29, 2023 at 05:08:36PM +0900, Yoshihiro Shimoda wrote:
> > > Add ACLK of GWCA which needs to calculate registers' values for
> > > rate limiter feature.
> > > 
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > ---
> > >  .../bindings/net/renesas,r8a779f0-ether-switch.yaml    | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml b/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > > index e933a1e48d67..cbe05fdcadaf 100644
> > > --- a/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > > +++ b/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > > @@ -75,7 +75,12 @@ properties:
> > >        - const: rmac2_phy
> > >  
> > >    clocks:
> > > -    maxItems: 1
> > > +    maxItems: 2
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: fck
> > > +      - const: aclk
> > 
> > Since having both clocks is now required, please add some detail in the
> > commit message about why that is the case. Reading it sounds like this
> > is an optional new feature & not something that is required.
> 
> This is something i wondered about, backwards compatibility with old
> DT blobs. In the C code it is optional, and has a default clock rate
> if the clock is not present.

Yeah, I did the cursory check of the code to make sure that an old dtb
would still function, which is part of why I am asking for the
explanation of the enforcement here. I'm not clear on what the
consequences of getting the default rate is. Perhaps if I read the whole
series and understood the code I would be, but this commit should
explain the why anyway & save me the trouble ;)

> So the yaml should not enforce an aclk member.

This however I could go either way on. If the thing isn't going to
function properly with the fallback rate, but would just limp on on
in whatever broken way it has always done, I would agree with making
the second clock required so that no new devicetrees are written in a
way that would put the hardware into that broken state.
On the other hand, if it works perfectly fine for some use cases without
the second clock & just using the default rathe then I don't think the
presence of the second clock should be enforced.

Cheers,
Conor.
Yoshihiro Shimoda May 30, 2023, 12:19 a.m. UTC | #4
Hello Conor, Andrew,

> From: Conor Dooley, Sent: Tuesday, May 30, 2023 5:44 AM
> On Mon, May 29, 2023 at 10:11:12PM +0200, Andrew Lunn wrote:
> > On Mon, May 29, 2023 at 07:36:03PM +0100, Conor Dooley wrote:
> > > On Mon, May 29, 2023 at 05:08:36PM +0900, Yoshihiro Shimoda wrote:
> > > > Add ACLK of GWCA which needs to calculate registers' values for
> > > > rate limiter feature.
> > > >
> > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > > ---
> > > >  .../bindings/net/renesas,r8a779f0-ether-switch.yaml    | 10 ++++++++--
> > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> b/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > > > index e933a1e48d67..cbe05fdcadaf 100644
> > > > --- a/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > > > +++ b/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > > > @@ -75,7 +75,12 @@ properties:
> > > >        - const: rmac2_phy
> > > >
> > > >    clocks:
> > > > -    maxItems: 1
> > > > +    maxItems: 2
> > > > +
> > > > +  clock-names:
> > > > +    items:
> > > > +      - const: fck
> > > > +      - const: aclk
> > >
> > > Since having both clocks is now required, please add some detail in the
> > > commit message about why that is the case. Reading it sounds like this
> > > is an optional new feature & not something that is required.
> >
> > This is something i wondered about, backwards compatibility with old
> > DT blobs. In the C code it is optional, and has a default clock rate
> > if the clock is not present.

I'm sorry for lacking explanation. You're correct. this is backwards
compatibility with old DT blobs.

> Yeah, I did the cursory check of the code to make sure that an old dtb
> would still function, which is part of why I am asking for the
> explanation of the enforcement here. I'm not clear on what the
> consequences of getting the default rate is. Perhaps if I read the whole
> series and understood the code I would be, but this commit should
> explain the why anyway & save me the trouble ;)

The following clock rates are the same (400MHz):
 - default rate (RSWITCH_ACLK_DEFAULT) in the C code
 - R8A779F0_CLK_S0D2_HSC from dtb

Only for backwards compatibility with old DT blobs, I added
the RSWITCH_ACLK_DEFAULT, and got the aclk as optional.

By the way, R8A779F0_CLK_S0D2_HSC is fixed rate, and the r8a779f0-ether-switch
only uses the rswitch driver. Therefore, the clock rate is always 400MHz.
So, I'm thinking that the following implementation is enough.
 - no dt-bindings change. (In other words, don't add aclk in the dt-bindings.)
 - hardcoded the clock rate in the C code as 400MHz.

> > So the yaml should not enforce an aclk member.
> 
> This however I could go either way on. If the thing isn't going to
> function properly with the fallback rate, but would just limp on on
> in whatever broken way it has always done, I would agree with making
> the second clock required so that no new devicetrees are written in a
> way that would put the hardware into that broken state.
> On the other hand, if it works perfectly fine for some use cases without
> the second clock & just using the default rathe then I don't think the
> presence of the second clock should be enforced.

Thank you very much for your comments! The it works perfectly fine for
all use cases without the second clock & just using the default rate.
That's why I'm now thinking that adding aclk into the dt-bindings is not
a good way...

Best regards,
Yoshihiro Shimoda

> Cheers,
> Conor.
Conor Dooley May 30, 2023, 7:22 a.m. UTC | #5
Hey,

On Tue, May 30, 2023 at 12:19:46AM +0000, Yoshihiro Shimoda wrote:
> > From: Conor Dooley, Sent: Tuesday, May 30, 2023 5:44 AM
> > On Mon, May 29, 2023 at 10:11:12PM +0200, Andrew Lunn wrote:
> > > On Mon, May 29, 2023 at 07:36:03PM +0100, Conor Dooley wrote:
> > > > On Mon, May 29, 2023 at 05:08:36PM +0900, Yoshihiro Shimoda wrote:
> > > > > Add ACLK of GWCA which needs to calculate registers' values for
> > > > > rate limiter feature.
> > > > >
> > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > > > ---
> > > > >  .../bindings/net/renesas,r8a779f0-ether-switch.yaml    | 10 ++++++++--
> > > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > b/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > > > > index e933a1e48d67..cbe05fdcadaf 100644
> > > > > --- a/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > > > > +++ b/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > > > > @@ -75,7 +75,12 @@ properties:
> > > > >        - const: rmac2_phy
> > > > >
> > > > >    clocks:
> > > > > -    maxItems: 1
> > > > > +    maxItems: 2
> > > > > +
> > > > > +  clock-names:
> > > > > +    items:
> > > > > +      - const: fck
> > > > > +      - const: aclk
> > > >
> > > > Since having both clocks is now required, please add some detail in the
> > > > commit message about why that is the case. Reading it sounds like this
> > > > is an optional new feature & not something that is required.
> > >
> > > This is something i wondered about, backwards compatibility with old
> > > DT blobs. In the C code it is optional, and has a default clock rate
> > > if the clock is not present.
> 
> I'm sorry for lacking explanation. You're correct. this is backwards
> compatibility with old DT blobs.
> 
> > Yeah, I did the cursory check of the code to make sure that an old dtb
> > would still function, which is part of why I am asking for the
> > explanation of the enforcement here. I'm not clear on what the
> > consequences of getting the default rate is. Perhaps if I read the whole
> > series and understood the code I would be, but this commit should
> > explain the why anyway & save me the trouble ;)
> 
> The following clock rates are the same (400MHz):
>  - default rate (RSWITCH_ACLK_DEFAULT) in the C code
>  - R8A779F0_CLK_S0D2_HSC from dtb
> 
> Only for backwards compatibility with old DT blobs, I added
> the RSWITCH_ACLK_DEFAULT, and got the aclk as optional.
> 
> By the way, R8A779F0_CLK_S0D2_HSC is fixed rate, and the r8a779f0-ether-switch
> only uses the rswitch driver. Therefore, the clock rate is always 400MHz.
> So, I'm thinking that the following implementation is enough.
>  - no dt-bindings change. (In other words, don't add aclk in the dt-bindings.)
>  - hardcoded the clock rate in the C code as 400MHz.
> 
> > > So the yaml should not enforce an aclk member.
> > 
> > This however I could go either way on. If the thing isn't going to
> > function properly with the fallback rate, but would just limp on on
> > in whatever broken way it has always done, I would agree with making
> > the second clock required so that no new devicetrees are written in a
> > way that would put the hardware into that broken state.
> > On the other hand, if it works perfectly fine for some use cases without
> > the second clock & just using the default rathe then I don't think the
> > presence of the second clock should be enforced.
> 
> Thank you very much for your comments! The it works perfectly fine for
> all use cases without the second clock & just using the default rate.
> That's why I'm now thinking that adding aclk into the dt-bindings is not
> a good way...

I am biased, but I think the binding should describe the hardware &
therefore the additional clock should be added.

Cheers,
Conor.
Yoshihiro Shimoda May 30, 2023, 11:42 a.m. UTC | #6
Hello Conor,

> From: Conor Dooley, Sent: Tuesday, May 30, 2023 4:22 PM
> 
> Hey,
> 
> On Tue, May 30, 2023 at 12:19:46AM +0000, Yoshihiro Shimoda wrote:
> > > From: Conor Dooley, Sent: Tuesday, May 30, 2023 5:44 AM
> > > On Mon, May 29, 2023 at 10:11:12PM +0200, Andrew Lunn wrote:
> > > > On Mon, May 29, 2023 at 07:36:03PM +0100, Conor Dooley wrote:
> > > > > On Mon, May 29, 2023 at 05:08:36PM +0900, Yoshihiro Shimoda wrote:
> > > > > > Add ACLK of GWCA which needs to calculate registers' values for
> > > > > > rate limiter feature.
> > > > > >
> > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > > > > ---
> > > > > >  .../bindings/net/renesas,r8a779f0-ether-switch.yaml    | 10 ++++++++--
> > > > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > > b/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > > > > > index e933a1e48d67..cbe05fdcadaf 100644
> > > > > > --- a/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
> > > > > > @@ -75,7 +75,12 @@ properties:
> > > > > >        - const: rmac2_phy
> > > > > >
> > > > > >    clocks:
> > > > > > -    maxItems: 1
> > > > > > +    maxItems: 2
> > > > > > +
> > > > > > +  clock-names:
> > > > > > +    items:
> > > > > > +      - const: fck
> > > > > > +      - const: aclk
> > > > >
> > > > > Since having both clocks is now required, please add some detail in the
> > > > > commit message about why that is the case. Reading it sounds like this
> > > > > is an optional new feature & not something that is required.
> > > >
> > > > This is something i wondered about, backwards compatibility with old
> > > > DT blobs. In the C code it is optional, and has a default clock rate
> > > > if the clock is not present.
> >
> > I'm sorry for lacking explanation. You're correct. this is backwards
> > compatibility with old DT blobs.
> >
> > > Yeah, I did the cursory check of the code to make sure that an old dtb
> > > would still function, which is part of why I am asking for the
> > > explanation of the enforcement here. I'm not clear on what the
> > > consequences of getting the default rate is. Perhaps if I read the whole
> > > series and understood the code I would be, but this commit should
> > > explain the why anyway & save me the trouble ;)
> >
> > The following clock rates are the same (400MHz):
> >  - default rate (RSWITCH_ACLK_DEFAULT) in the C code
> >  - R8A779F0_CLK_S0D2_HSC from dtb
> >
> > Only for backwards compatibility with old DT blobs, I added
> > the RSWITCH_ACLK_DEFAULT, and got the aclk as optional.
> >
> > By the way, R8A779F0_CLK_S0D2_HSC is fixed rate, and the r8a779f0-ether-switch
> > only uses the rswitch driver. Therefore, the clock rate is always 400MHz.
> > So, I'm thinking that the following implementation is enough.
> >  - no dt-bindings change. (In other words, don't add aclk in the dt-bindings.)
> >  - hardcoded the clock rate in the C code as 400MHz.
> >
> > > > So the yaml should not enforce an aclk member.
> > >
> > > This however I could go either way on. If the thing isn't going to
> > > function properly with the fallback rate, but would just limp on on
> > > in whatever broken way it has always done, I would agree with making
> > > the second clock required so that no new devicetrees are written in a
> > > way that would put the hardware into that broken state.
> > > On the other hand, if it works perfectly fine for some use cases without
> > > the second clock & just using the default rathe then I don't think the
> > > presence of the second clock should be enforced.
> >
> > Thank you very much for your comments! The it works perfectly fine for
> > all use cases without the second clock & just using the default rate.
> > That's why I'm now thinking that adding aclk into the dt-bindings is not
> > a good way...
> 
> I am biased, but I think the binding should describe the hardware &
> therefore the additional clock should be added.

I got it. I'll fix this patch on v2.

Best regards,
Yoshihiro Shimoda

> Cheers,
> Conor.
Krzysztof Kozlowski May 30, 2023, 12:27 p.m. UTC | #7
On Mon, 29 May 2023 17:08:36 +0900, Yoshihiro Shimoda wrote:
> Add ACLK of GWCA which needs to calculate registers' values for
> rate limiter feature.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  .../bindings/net/renesas,r8a779f0-ether-switch.yaml    | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 

Running 'make dtbs_check' with the schema in this patch gives the
following warnings. Consider if they are expected or the schema is
incorrect. These may not be new warnings.

Note that it is not yet a requirement to have 0 warnings for dtbs_check.
This will change in the future.

Full log is available here: https://patchwork.ozlabs.org/patch/1786990


ethernet@e6880000: clocks: [[12, 1, 1505]] is too short
	arch/arm64/boot/dts/renesas/r8a779f0-spider.dtb
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml b/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
index e933a1e48d67..cbe05fdcadaf 100644
--- a/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
+++ b/Documentation/devicetree/bindings/net/renesas,r8a779f0-ether-switch.yaml
@@ -75,7 +75,12 @@  properties:
       - const: rmac2_phy
 
   clocks:
-    maxItems: 1
+    maxItems: 2
+
+  clock-names:
+    items:
+      - const: fck
+      - const: aclk
 
   resets:
     maxItems: 1
@@ -221,7 +226,8 @@  examples:
                           "rmac2_mdio",
                           "rmac0_phy", "rmac1_phy",
                           "rmac2_phy";
-        clocks = <&cpg CPG_MOD 1505>;
+        clocks = <&cpg CPG_MOD 1505>, <&cpg CPG_CORE R8A779F0_CLK_S0D2_HSC>;
+        clock-names = "fck", "aclk";
         power-domains = <&sysc R8A779F0_PD_ALWAYS_ON>;
         resets = <&cpg 1505>;