| Message ID | 20230529080840.1156458-2-yoshihiro.shimoda.uh@renesas.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | net: renesas: rswitch: Improve performance of TX | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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 >
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
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.
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.
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.
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.
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 --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>;
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(-)