[PATCH net-next 0/5] net: renesas: rswitch: Improve performance of TX

[PATCH net-next 0/5] net: renesas: rswitch: Improve performance of TX

[Yoshihiro Shimoda]

This patch series is based on next-20230525. This patch series can improve
performance of TX in a specific condition. The previous code used
"global rate limiter" feature so that this is possible to cause performance
down if we use multiple ports at the same time. To resolve this issue,
use "per-queue rate limiter" feature instead. To use the feature, we need
to refactor the rswitch driver, especially got the internal bus clock
rate and calculate the value for the feature.

Yoshihiro Shimoda (5):
  dt-bindings: net: r8a779f0-ether-switch: Add ACLK
  net: renesas: rswitch: Rename GWCA related definitions
  net: renesas: rswitch: Alloc all 128 queues
  net: renesas: rswitch: Use AXI_TLIM_N queues if a TX queue
  net: renesas: rswitch: Use per-queue rate limiter

 .../net/renesas,r8a779f0-ether-switch.yaml    | 10 ++-
 drivers/net/ethernet/renesas/rswitch.c        | 86 ++++++++++++-------
 drivers/net/ethernet/renesas/rswitch.h        | 30 +++++--
 3 files changed, 82 insertions(+), 44 deletions(-)

-- 
2.25.1

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

[Yoshihiro Shimoda]

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
 
   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

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1688 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Andrew Lunn]

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

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2576 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Yoshihiro Shimoda]

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.

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4033 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Yoshihiro Shimoda]

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.

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

[Krzysztof Kozlowski]

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

[PATCH net-next 2/5] net: renesas: rswitch: Rename GWCA related definitions

[Yoshihiro Shimoda]

Rename GWCA related definitions to improve readability.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 10 +++++-----
 drivers/net/ethernet/renesas/rswitch.h | 12 ++++++------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index 29afaddb598d..51df96de6fd5 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -95,7 +95,7 @@ static void rswitch_top_init(struct rswitch_private *priv)
 {
 	int i;
 
-	for (i = 0; i < RSWITCH_MAX_NUM_QUEUES; i++)
+	for (i = 0; i < GWCA_AXI_CHAIN_N; i++)
 		iowrite32((i / 16) << (GWCA_INDEX * 8), priv->addr + TPEMIMC7(i));
 }
 
@@ -179,7 +179,7 @@ static bool rswitch_is_any_data_irq(struct rswitch_private *priv, u32 *dis, bool
 	u32 *mask = tx ? priv->gwca.tx_irq_bits : priv->gwca.rx_irq_bits;
 	int i;
 
-	for (i = 0; i < RSWITCH_NUM_IRQ_REGS; i++) {
+	for (i = 0; i < GWCA_NUM_IRQ_REGS; i++) {
 		if (dis[i] & mask[i])
 			return true;
 	}
@@ -191,7 +191,7 @@ static void rswitch_get_data_irq_status(struct rswitch_private *priv, u32 *dis)
 {
 	int i;
 
-	for (i = 0; i < RSWITCH_NUM_IRQ_REGS; i++) {
+	for (i = 0; i < GWCA_NUM_IRQ_REGS; i++) {
 		dis[i] = ioread32(priv->addr + GWDIS(i));
 		dis[i] &= ioread32(priv->addr + GWDIE(i));
 	}
@@ -863,7 +863,7 @@ static irqreturn_t rswitch_data_irq(struct rswitch_private *priv, u32 *dis)
 static irqreturn_t rswitch_gwca_irq(int irq, void *dev_id)
 {
 	struct rswitch_private *priv = dev_id;
-	u32 dis[RSWITCH_NUM_IRQ_REGS];
+	u32 dis[GWCA_NUM_IRQ_REGS];
 	irqreturn_t ret = IRQ_NONE;
 
 	rswitch_get_data_irq_status(priv, dis);
@@ -1891,7 +1891,7 @@ static int renesas_eth_sw_probe(struct platform_device *pdev)
 
 	priv->gwca.index = AGENT_INDEX_GWCA;
 	priv->gwca.num_queues = min(RSWITCH_NUM_PORTS * NUM_QUEUES_PER_NDEV,
-				    RSWITCH_MAX_NUM_QUEUES);
+				    GWCA_AXI_CHAIN_N);
 	priv->gwca.queues = devm_kcalloc(&pdev->dev, priv->gwca.num_queues,
 					 sizeof(*priv->gwca.queues), GFP_KERNEL);
 	if (!priv->gwca.queues)
diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h
index b3e0411b408e..550a6bff9078 100644
--- a/drivers/net/ethernet/renesas/rswitch.h
+++ b/drivers/net/ethernet/renesas/rswitch.h
@@ -10,8 +10,6 @@
 #include <linux/platform_device.h>
 #include "rcar_gen4_ptp.h"
 
-#define RSWITCH_MAX_NUM_QUEUES	128
-
 #define RSWITCH_NUM_PORTS	3
 #define rswitch_for_each_enabled_port(priv, i)		\
 	for (i = 0; i < RSWITCH_NUM_PORTS; i++)		\
@@ -50,6 +48,9 @@
 #define AGENT_INDEX_GWCA	3
 #define GWRO			RSWITCH_GWCA0_OFFSET
 
+#define GWCA_AXI_CHAIN_N	128
+#define GWCA_NUM_IRQ_REGS	(GWCA_AXI_CHAIN_N / BITS_PER_TYPE(u32))
+
 #define GWCA_TS_IRQ_RESOURCE_NAME	"gwca0_rxts0"
 #define GWCA_TS_IRQ_NAME		"rswitch: gwca0_rxts0"
 #define GWCA_TS_IRQ_BIT			BIT(0)
@@ -949,7 +950,6 @@ struct rswitch_gwca_ts_info {
 	u8 tag;
 };
 
-#define RSWITCH_NUM_IRQ_REGS	(RSWITCH_MAX_NUM_QUEUES / BITS_PER_TYPE(u32))
 struct rswitch_gwca {
 	int index;
 	struct rswitch_desc *linkfix_table;
@@ -959,9 +959,9 @@ struct rswitch_gwca {
 	int num_queues;
 	struct rswitch_gwca_queue ts_queue;
 	struct list_head ts_info_list;
-	DECLARE_BITMAP(used, RSWITCH_MAX_NUM_QUEUES);
-	u32 tx_irq_bits[RSWITCH_NUM_IRQ_REGS];
-	u32 rx_irq_bits[RSWITCH_NUM_IRQ_REGS];
+	DECLARE_BITMAP(used, GWCA_AXI_CHAIN_N);
+	u32 tx_irq_bits[GWCA_NUM_IRQ_REGS];
+	u32 rx_irq_bits[GWCA_NUM_IRQ_REGS];
 	int speed;
 };
 
-- 
2.25.1

Re: [PATCH net-next 2/5] net: renesas: rswitch: Rename GWCA related definitions

[Geert Uytterhoeven]

Hi Shimoda-san,

On Mon, May 29, 2023 at 10:08 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> Rename GWCA related definitions to improve readability.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Thanks for your patch!

> --- a/drivers/net/ethernet/renesas/rswitch.h
> +++ b/drivers/net/ethernet/renesas/rswitch.h

> @@ -959,9 +959,9 @@ struct rswitch_gwca {
>         int num_queues;
>         struct rswitch_gwca_queue ts_queue;
>         struct list_head ts_info_list;
> -       DECLARE_BITMAP(used, RSWITCH_MAX_NUM_QUEUES);
> -       u32 tx_irq_bits[RSWITCH_NUM_IRQ_REGS];
> -       u32 rx_irq_bits[RSWITCH_NUM_IRQ_REGS];
> +       DECLARE_BITMAP(used, GWCA_AXI_CHAIN_N);
> +       u32 tx_irq_bits[GWCA_NUM_IRQ_REGS];
> +       u32 rx_irq_bits[GWCA_NUM_IRQ_REGS];

Not directly related to this patch, but is there a specific reason why
tx_irq_bits and rx_irq_bits are arrays instead of bitmaps declared
using DECLARE_BITMAP()?  I think you can simplify the code that accesses
them by using the bitmap APIs.

>         int speed;
>  };

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH net-next 2/5] net: renesas: rswitch: Rename GWCA related definitions

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Tuesday, May 30, 2023 4:18 PM
> 
> Hi Shimoda-san,
> 
> On Mon, May 29, 2023 at 10:08 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > Rename GWCA related definitions to improve readability.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/net/ethernet/renesas/rswitch.h
> > +++ b/drivers/net/ethernet/renesas/rswitch.h
> 
> > @@ -959,9 +959,9 @@ struct rswitch_gwca {
> >         int num_queues;
> >         struct rswitch_gwca_queue ts_queue;
> >         struct list_head ts_info_list;
> > -       DECLARE_BITMAP(used, RSWITCH_MAX_NUM_QUEUES);
> > -       u32 tx_irq_bits[RSWITCH_NUM_IRQ_REGS];
> > -       u32 rx_irq_bits[RSWITCH_NUM_IRQ_REGS];
> > +       DECLARE_BITMAP(used, GWCA_AXI_CHAIN_N);
> > +       u32 tx_irq_bits[GWCA_NUM_IRQ_REGS];
> > +       u32 rx_irq_bits[GWCA_NUM_IRQ_REGS];
> 
> Not directly related to this patch, but is there a specific reason why
> tx_irq_bits and rx_irq_bits are arrays instead of bitmaps declared
> using DECLARE_BITMAP()?  I think you can simplify the code that accesses
> them by using the bitmap APIs.

Using arrays is easy to understand to me about GWDI[ES]i registers' handling
in the following functions:
- rswitch_is_any_data_irq()
- rswitch_get_data_irq_status()
- rswitch_data_irq()

However, using bitmaps can avoid calculation of index and bit by division and modulo.
So, it seems better.

And, this is also not related to this patch though, I realized that separating
tx_irq_bits and gwca.rx_irq_bits is not needed.

Best regards,
Yoshihiro Shimoda

> >         int speed;
> >  };
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

[PATCH net-next 3/5] net: renesas: rswitch: Alloc all 128 queues

[Yoshihiro Shimoda]

To use per-queue rate limiter feature in the future, alloc all 128
queues (GWCA_AXI_CHAIN_N) of GWCA so that drop num_queues from
struct rswitch_gwca. Notes that add a condition of gwca.used flag
in rswitch_data_irq() because the previous code always set the flag
of all queues.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 19 ++++++++++---------
 drivers/net/ethernet/renesas/rswitch.h |  1 -
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index 51df96de6fd5..4aab5d8aad2f 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -508,16 +508,16 @@ static int rswitch_gwca_queue_ext_ts_format(struct net_device *ndev,
 
 static int rswitch_gwca_linkfix_alloc(struct rswitch_private *priv)
 {
-	int i, num_queues = priv->gwca.num_queues;
 	struct rswitch_gwca *gwca = &priv->gwca;
 	struct device *dev = &priv->pdev->dev;
+	int i;
 
-	gwca->linkfix_table_size = sizeof(struct rswitch_desc) * num_queues;
+	gwca->linkfix_table_size = sizeof(struct rswitch_desc) * GWCA_AXI_CHAIN_N;
 	gwca->linkfix_table = dma_alloc_coherent(dev, gwca->linkfix_table_size,
 						 &gwca->linkfix_table_dma, GFP_KERNEL);
 	if (!gwca->linkfix_table)
 		return -ENOMEM;
-	for (i = 0; i < num_queues; i++)
+	for (i = 0; i < GWCA_AXI_CHAIN_N; i++)
 		gwca->linkfix_table[i].die_dt = DT_EOS;
 
 	return 0;
@@ -538,8 +538,8 @@ static struct rswitch_gwca_queue *rswitch_gwca_get(struct rswitch_private *priv)
 	struct rswitch_gwca_queue *gq;
 	int index;
 
-	index = find_first_zero_bit(priv->gwca.used, priv->gwca.num_queues);
-	if (index >= priv->gwca.num_queues)
+	index = find_first_zero_bit(priv->gwca.used, GWCA_AXI_CHAIN_N);
+	if (index >= GWCA_AXI_CHAIN_N)
 		return NULL;
 	set_bit(index, priv->gwca.used);
 	gq = &priv->gwca.queues[index];
@@ -846,7 +846,10 @@ static irqreturn_t rswitch_data_irq(struct rswitch_private *priv, u32 *dis)
 	struct rswitch_gwca_queue *gq;
 	int i, index, bit;
 
-	for (i = 0; i < priv->gwca.num_queues; i++) {
+	for (i = 0; i < GWCA_AXI_CHAIN_N; i++) {
+		if (!test_bit(i, priv->gwca.used))
+			continue;
+
 		gq = &priv->gwca.queues[i];
 		index = gq->index / 32;
 		bit = BIT(gq->index % 32);
@@ -1890,9 +1893,7 @@ static int renesas_eth_sw_probe(struct platform_device *pdev)
 	}
 
 	priv->gwca.index = AGENT_INDEX_GWCA;
-	priv->gwca.num_queues = min(RSWITCH_NUM_PORTS * NUM_QUEUES_PER_NDEV,
-				    GWCA_AXI_CHAIN_N);
-	priv->gwca.queues = devm_kcalloc(&pdev->dev, priv->gwca.num_queues,
+	priv->gwca.queues = devm_kcalloc(&pdev->dev, GWCA_AXI_CHAIN_N,
 					 sizeof(*priv->gwca.queues), GFP_KERNEL);
 	if (!priv->gwca.queues)
 		return -ENOMEM;
diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h
index 550a6bff9078..c3c2c92c2a1e 100644
--- a/drivers/net/ethernet/renesas/rswitch.h
+++ b/drivers/net/ethernet/renesas/rswitch.h
@@ -956,7 +956,6 @@ struct rswitch_gwca {
 	dma_addr_t linkfix_table_dma;
 	u32 linkfix_table_size;
 	struct rswitch_gwca_queue *queues;
-	int num_queues;
 	struct rswitch_gwca_queue ts_queue;
 	struct list_head ts_info_list;
 	DECLARE_BITMAP(used, GWCA_AXI_CHAIN_N);
-- 
2.25.1

[PATCH net-next 4/5] net: renesas: rswitch: Use AXI_TLIM_N queues if a TX queue

[Yoshihiro Shimoda]

To use per-queue rate limiter feature in the future, use AXI_TLIM_N
queues if a TX queue.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 10 ++++++----
 drivers/net/ethernet/renesas/rswitch.h |  2 ++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index 4aab5d8aad2f..4ae34b0206cd 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -533,12 +533,14 @@ static void rswitch_gwca_linkfix_free(struct rswitch_private *priv)
 	gwca->linkfix_table = NULL;
 }
 
-static struct rswitch_gwca_queue *rswitch_gwca_get(struct rswitch_private *priv)
+static struct rswitch_gwca_queue *rswitch_gwca_get(struct rswitch_private *priv,
+						   bool dir_tx)
 {
 	struct rswitch_gwca_queue *gq;
 	int index;
 
-	index = find_first_zero_bit(priv->gwca.used, GWCA_AXI_CHAIN_N);
+	index = find_next_zero_bit(priv->gwca.used, GWCA_AXI_CHAIN_N,
+				   dir_tx ? GWCA_AXI_TRIM_BASE : 0);
 	if (index >= GWCA_AXI_CHAIN_N)
 		return NULL;
 	set_bit(index, priv->gwca.used);
@@ -561,7 +563,7 @@ static int rswitch_txdmac_alloc(struct net_device *ndev)
 	struct rswitch_private *priv = rdev->priv;
 	int err;
 
-	rdev->tx_queue = rswitch_gwca_get(priv);
+	rdev->tx_queue = rswitch_gwca_get(priv, true);
 	if (!rdev->tx_queue)
 		return -EBUSY;
 
@@ -595,7 +597,7 @@ static int rswitch_rxdmac_alloc(struct net_device *ndev)
 	struct rswitch_private *priv = rdev->priv;
 	int err;
 
-	rdev->rx_queue = rswitch_gwca_get(priv);
+	rdev->rx_queue = rswitch_gwca_get(priv, false);
 	if (!rdev->rx_queue)
 		return -EBUSY;
 
diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h
index c3c2c92c2a1e..7ba45ddab42a 100644
--- a/drivers/net/ethernet/renesas/rswitch.h
+++ b/drivers/net/ethernet/renesas/rswitch.h
@@ -49,7 +49,9 @@
 #define GWRO			RSWITCH_GWCA0_OFFSET
 
 #define GWCA_AXI_CHAIN_N	128
+#define GWCA_AXI_TLIM_N		32
 #define GWCA_NUM_IRQ_REGS	(GWCA_AXI_CHAIN_N / BITS_PER_TYPE(u32))
+#define GWCA_AXI_TRIM_BASE	(GWCA_AXI_CHAIN_N - GWCA_AXI_TLIM_N)
 
 #define GWCA_TS_IRQ_RESOURCE_NAME	"gwca0_rxts0"
 #define GWCA_TS_IRQ_NAME		"rswitch: gwca0_rxts0"
-- 
2.25.1

[PATCH net-next 5/5] net: renesas: rswitch: Use per-queue rate limiter

[Yoshihiro Shimoda]

Use per-queue rate limiter instead of global rate limiter. Otherwise
TX performance will be low when we use multiple ports at the same time.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 51 +++++++++++++++++---------
 drivers/net/ethernet/renesas/rswitch.h | 15 +++++++-
 2 files changed, 47 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index 4ae34b0206cd..a7195625a2c7 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -156,22 +156,31 @@ static int rswitch_gwca_axi_ram_reset(struct rswitch_private *priv)
 	return rswitch_reg_wait(priv->addr, GWARIRM, GWARIRM_ARR, GWARIRM_ARR);
 }
 
-static void rswitch_gwca_set_rate_limit(struct rswitch_private *priv, int rate)
+static void rswitch_gwca_set_rate_limit(struct rswitch_private *priv,
+					struct rswitch_gwca_queue *txq)
 {
-	u32 gwgrlulc, gwgrlc;
+	u64 period_ps;
+	unsigned long rate;
+	u32 gwrlc;
 
-	switch (rate) {
-	case 1000:
-		gwgrlulc = 0x0000005f;
-		gwgrlc = 0x00010260;
-		break;
-	default:
-		dev_err(&priv->pdev->dev, "%s: This rate is not supported (%d)\n", __func__, rate);
-		return;
-	}
+	rate = clk_get_rate(priv->aclk);
+	if (!rate)
+		rate = RSWITCH_ACLK_DEFAULT;
+
+	period_ps = div64_u64(1000000000000ULL, rate);
+
+	/* GWRLC value = 256 * ACLK_period[ns] * maxBandwidth[Gbps] */
+	gwrlc = 256 * period_ps * txq->speed / 1000000;
+
+	/* To avoid overflow internally, the value should be 97% */
+	gwrlc = gwrlc * 97 / 100;
 
-	iowrite32(gwgrlulc, priv->addr + GWGRLULC);
-	iowrite32(gwgrlc, priv->addr + GWGRLC);
+	dev_dbg(&priv->pdev->dev,
+		"%s: index = %d, speed = %d, rate = %ld, gwrlc = %08x\n",
+		__func__, txq->index_trim, txq->speed, rate, gwrlc);
+
+	iowrite32(GWRLULC_NOT_REQUIRED, priv->addr + GWRLULC(txq->index_trim));
+	iowrite32(gwrlc | GWRLC_RLE, priv->addr + GWRLC(txq->index_trim));
 }
 
 static bool rswitch_is_any_data_irq(struct rswitch_private *priv, u32 *dis, bool tx)
@@ -548,6 +557,10 @@ static struct rswitch_gwca_queue *rswitch_gwca_get(struct rswitch_private *priv,
 	memset(gq, 0, sizeof(*gq));
 	gq->index = index;
 
+	/* The first "Rate limiter" queue is located at GWCA_AXI_CHAIN_N - 1 */
+	if (dir_tx)
+		gq->index_trim = GWCA_AXI_CHAIN_N - index - 1;
+
 	return gq;
 }
 
@@ -651,7 +664,6 @@ static int rswitch_gwca_hw_init(struct rswitch_private *priv)
 	iowrite32(lower_32_bits(priv->gwca.ts_queue.ring_dma), priv->addr + GWTDCAC10);
 	iowrite32(upper_32_bits(priv->gwca.ts_queue.ring_dma), priv->addr + GWTDCAC00);
 	iowrite32(GWCA_TS_IRQ_BIT, priv->addr + GWTSDCC0);
-	rswitch_gwca_set_rate_limit(priv, priv->gwca.speed);
 
 	for (i = 0; i < RSWITCH_NUM_PORTS; i++) {
 		err = rswitch_rxdmac_init(priv, i);
@@ -1441,6 +1453,8 @@ static int rswitch_open(struct net_device *ndev)
 	napi_enable(&rdev->napi);
 	netif_start_queue(ndev);
 
+	rswitch_gwca_set_rate_limit(rdev->priv, rdev->tx_queue);
+
 	rswitch_enadis_data_irq(rdev->priv, rdev->tx_queue->index, true);
 	rswitch_enadis_data_irq(rdev->priv, rdev->rx_queue->index, true);
 
@@ -1723,9 +1737,6 @@ static int rswitch_device_alloc(struct rswitch_private *priv, int index)
 	if (err < 0)
 		goto out_get_params;
 
-	if (rdev->priv->gwca.speed < rdev->etha->speed)
-		rdev->priv->gwca.speed = rdev->etha->speed;
-
 	err = rswitch_rxdmac_alloc(ndev);
 	if (err < 0)
 		goto out_rxdmac;
@@ -1734,6 +1745,8 @@ static int rswitch_device_alloc(struct rswitch_private *priv, int index)
 	if (err < 0)
 		goto out_txdmac;
 
+	rdev->tx_queue->speed = rdev->etha->speed;
+
 	return 0;
 
 out_txdmac:
@@ -1903,6 +1916,10 @@ static int renesas_eth_sw_probe(struct platform_device *pdev)
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 
+	priv->aclk = devm_clk_get_optional(&pdev->dev, "aclk");
+	if (IS_ERR(priv->aclk))
+		return PTR_ERR(priv->aclk);
+
 	ret = rswitch_init(priv);
 	if (ret < 0) {
 		pm_runtime_put(&pdev->dev);
diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h
index 7ba45ddab42a..741f45266c29 100644
--- a/drivers/net/ethernet/renesas/rswitch.h
+++ b/drivers/net/ethernet/renesas/rswitch.h
@@ -7,9 +7,11 @@
 #ifndef __RSWITCH_H__
 #define __RSWITCH_H__
 
+#include <linux/clk.h>
 #include <linux/platform_device.h>
 #include "rcar_gen4_ptp.h"
 
+#define RSWITCH_ACLK_DEFAULT	400000000UL
 #define RSWITCH_NUM_PORTS	3
 #define rswitch_for_each_enabled_port(priv, i)		\
 	for (i = 0; i < RSWITCH_NUM_PORTS; i++)		\
@@ -676,7 +678,7 @@ enum rswitch_reg {
 	GWIDACAM10	= GWRO + 0x0984,
 	GWGRLC		= GWRO + 0x0a00,
 	GWGRLULC	= GWRO + 0x0a04,
-	GWRLIVC0	= GWRO + 0x0a80,
+	GWRLC0		= GWRO + 0x0a80,
 	GWRLULC0	= GWRO + 0x0a84,
 	GWIDPC		= GWRO + 0x0b00,
 	GWIDC0		= GWRO + 0x0c00,
@@ -778,6 +780,11 @@ enum rswitch_gwca_mode {
 #define GWTRC(queue)		(GWTRC0 + (queue) / 32 * 4)
 #define GWDCC_OFFS(queue)	(GWDCC0 + (queue) * 4)
 
+#define GWRLC(trim)		(GWRLC0 + (trim) * 8)
+#define GWRLC_RLE		BIT(16)
+#define GWRLULC(trim)		(GWRLULC0 + (trim) * 8)
+#define GWRLULC_NOT_REQUIRED	0x00800000
+
 #define GWDIS(i)		(GWDIS0 + (i) * 0x10)
 #define GWDIE(i)		(GWDIE0 + (i) * 0x10)
 #define GWDID(i)		(GWDID0 + (i) * 0x10)
@@ -942,6 +949,10 @@ struct rswitch_gwca_queue {
 	bool dir_tx;
 	struct sk_buff **skbs;
 	struct net_device *ndev;	/* queue to ndev for irq */
+
+	/* For tx_ring */
+	int index_trim;
+	int speed;
 };
 
 struct rswitch_gwca_ts_info {
@@ -963,7 +974,6 @@ struct rswitch_gwca {
 	DECLARE_BITMAP(used, GWCA_AXI_CHAIN_N);
 	u32 tx_irq_bits[GWCA_NUM_IRQ_REGS];
 	u32 rx_irq_bits[GWCA_NUM_IRQ_REGS];
-	int speed;
 };
 
 #define NUM_QUEUES_PER_NDEV	2
@@ -997,6 +1007,7 @@ struct rswitch_private {
 	struct platform_device *pdev;
 	void __iomem *addr;
 	struct rcar_gen4_ptp_private *ptp_priv;
+	struct clk *aclk;
 
 	struct rswitch_device *rdev[RSWITCH_NUM_PORTS];
 	DECLARE_BITMAP(opened_ports, RSWITCH_NUM_PORTS);
-- 
2.25.1

Re: [PATCH net-next 5/5] net: renesas: rswitch: Use per-queue rate limiter

[kernel test robot]

Hi Yoshihiro,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Yoshihiro-Shimoda/dt-bindings-net-r8a779f0-ether-switch-Add-ACLK/20230529-161010
base:   net-next/main
patch link:    https://lore.kernel.org/r/20230529080840.1156458-6-yoshihiro.shimoda.uh%40renesas.com
patch subject: [PATCH net-next 5/5] net: renesas: rswitch: Use per-queue rate limiter
config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20230530/202305300454.Kyjag1oy-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/64502944471d5b6fac76f49c06c29edfbbe43935
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Yoshihiro-Shimoda/dt-bindings-net-r8a779f0-ether-switch-Add-ACLK/20230529-161010
        git checkout 64502944471d5b6fac76f49c06c29edfbbe43935
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 ~/bin/make.cross W=1 O=build_dir ARCH=mips olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 ~/bin/make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305300454.Kyjag1oy-lkp@intel.com/

All errors (new ones prefixed by >>):

   arch/mips/kernel/head.o: in function `kernel_entry':
   (.ref.text+0xac): relocation truncated to fit: R_MIPS_26 against `start_kernel'
   init/main.o: in function `set_reset_devices':
   main.c:(.init.text+0x20): relocation truncated to fit: R_MIPS_26 against `_mcount'
   main.c:(.init.text+0x30): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc'
   init/main.o: in function `debug_kernel':
   main.c:(.init.text+0xa4): relocation truncated to fit: R_MIPS_26 against `_mcount'
   main.c:(.init.text+0xb4): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc'
   init/main.o: in function `quiet_kernel':
   main.c:(.init.text+0x128): relocation truncated to fit: R_MIPS_26 against `_mcount'
   main.c:(.init.text+0x138): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc'
   init/main.o: in function `warn_bootconfig':
   main.c:(.init.text+0x1ac): relocation truncated to fit: R_MIPS_26 against `_mcount'
   main.c:(.init.text+0x1bc): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc'
   init/main.o: in function `init_setup':
   main.c:(.init.text+0x234): relocation truncated to fit: R_MIPS_26 against `_mcount'
   main.c:(.init.text+0x254): additional relocation overflows omitted from the output
   mips-linux-ld: drivers/net/ethernet/renesas/rswitch.o: in function `rswitch_open':
>> rswitch.c:(.text.rswitch_open+0x1a4): undefined reference to `__udivdi3'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next 5/5] net: renesas: rswitch: Use per-queue rate limiter

[kernel test robot]

Hi Yoshihiro,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Yoshihiro-Shimoda/dt-bindings-net-r8a779f0-ether-switch-Add-ACLK/20230529-161010
base:   net-next/main
patch link:    https://lore.kernel.org/r/20230529080840.1156458-6-yoshihiro.shimoda.uh%40renesas.com
patch subject: [PATCH net-next 5/5] net: renesas: rswitch: Use per-queue rate limiter
config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20230530/202305300718.pdF0iaxU-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/64502944471d5b6fac76f49c06c29edfbbe43935
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Yoshihiro-Shimoda/dt-bindings-net-r8a779f0-ether-switch-Add-ACLK/20230529-161010
        git checkout 64502944471d5b6fac76f49c06c29edfbbe43935
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 ~/bin/make.cross W=1 O=build_dir ARCH=powerpc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 ~/bin/make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305300718.pdF0iaxU-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "__udivdi3" [drivers/net/ethernet/renesas/rswitch_drv.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next 5/5] net: renesas: rswitch: Use per-queue rate limiter

[Geert Uytterhoeven]

Hi Shimoda-san,

On Mon, May 29, 2023 at 10:08 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> Use per-queue rate limiter instead of global rate limiter. Otherwise
> TX performance will be low when we use multiple ports at the same time.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Thanks for your patch!

> --- a/drivers/net/ethernet/renesas/rswitch.c
> +++ b/drivers/net/ethernet/renesas/rswitch.c
> @@ -156,22 +156,31 @@ static int rswitch_gwca_axi_ram_reset(struct rswitch_private *priv)
>         return rswitch_reg_wait(priv->addr, GWARIRM, GWARIRM_ARR, GWARIRM_ARR);
>  }
>
> -static void rswitch_gwca_set_rate_limit(struct rswitch_private *priv, int rate)
> +static void rswitch_gwca_set_rate_limit(struct rswitch_private *priv,
> +                                       struct rswitch_gwca_queue *txq)
>  {
> -       u32 gwgrlulc, gwgrlc;
> +       u64 period_ps;
> +       unsigned long rate;
> +       u32 gwrlc;
>
> -       switch (rate) {
> -       case 1000:
> -               gwgrlulc = 0x0000005f;
> -               gwgrlc = 0x00010260;
> -               break;
> -       default:
> -               dev_err(&priv->pdev->dev, "%s: This rate is not supported (%d)\n", __func__, rate);
> -               return;
> -       }
> +       rate = clk_get_rate(priv->aclk);
> +       if (!rate)
> +               rate = RSWITCH_ACLK_DEFAULT;
> +
> +       period_ps = div64_u64(1000000000000ULL, rate);

div64_ul, as rate is unsigned long.

> +
> +       /* GWRLC value = 256 * ACLK_period[ns] * maxBandwidth[Gbps] */
> +       gwrlc = 256 * period_ps * txq->speed / 1000000;

This contains an open-coded 64-by-32 division, causing link failures
on 32-bit platforms, so you should use div_u64() instead.  However,
because of the premultiplication by speed, which is 32-bit, you can
use the mul_u64_u32_div() helper.

Combining this with the calculation of period_ps above, you can simplify
this to:

    gwrlc = div64_ul(256000000ULL * txq->speed, rate);

> +
> +       /* To avoid overflow internally, the value should be 97% */
> +       gwrlc = gwrlc * 97 / 100;
>
> -       iowrite32(gwgrlulc, priv->addr + GWGRLULC);
> -       iowrite32(gwgrlc, priv->addr + GWGRLC);
> +       dev_dbg(&priv->pdev->dev,
> +               "%s: index = %d, speed = %d, rate = %ld, gwrlc = %08x\n",
> +               __func__, txq->index_trim, txq->speed, rate, gwrlc);
> +
> +       iowrite32(GWRLULC_NOT_REQUIRED, priv->addr + GWRLULC(txq->index_trim));
> +       iowrite32(gwrlc | GWRLC_RLE, priv->addr + GWRLC(txq->index_trim));
>  }
>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH net-next 5/5] net: renesas: rswitch: Use per-queue rate limiter

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Tuesday, May 30, 2023 4:33 PM
> 
> Hi Shimoda-san,
> 
> On Mon, May 29, 2023 at 10:08 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > Use per-queue rate limiter instead of global rate limiter. Otherwise
> > TX performance will be low when we use multiple ports at the same time.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> Thanks for your patch!

Thank you for your review!

> > --- a/drivers/net/ethernet/renesas/rswitch.c
> > +++ b/drivers/net/ethernet/renesas/rswitch.c
> > @@ -156,22 +156,31 @@ static int rswitch_gwca_axi_ram_reset(struct rswitch_private *priv)
> >         return rswitch_reg_wait(priv->addr, GWARIRM, GWARIRM_ARR, GWARIRM_ARR);
> >  }
> >
> > -static void rswitch_gwca_set_rate_limit(struct rswitch_private *priv, int rate)
> > +static void rswitch_gwca_set_rate_limit(struct rswitch_private *priv,
> > +                                       struct rswitch_gwca_queue *txq)
> >  {
> > -       u32 gwgrlulc, gwgrlc;
> > +       u64 period_ps;
> > +       unsigned long rate;
> > +       u32 gwrlc;
> >
> > -       switch (rate) {
> > -       case 1000:
> > -               gwgrlulc = 0x0000005f;
> > -               gwgrlc = 0x00010260;
> > -               break;
> > -       default:
> > -               dev_err(&priv->pdev->dev, "%s: This rate is not supported (%d)\n", __func__, rate);
> > -               return;
> > -       }
> > +       rate = clk_get_rate(priv->aclk);
> > +       if (!rate)
> > +               rate = RSWITCH_ACLK_DEFAULT;
> > +
> > +       period_ps = div64_u64(1000000000000ULL, rate);
> 
> div64_ul, as rate is unsigned long.

I see.

> > +
> > +       /* GWRLC value = 256 * ACLK_period[ns] * maxBandwidth[Gbps] */
> > +       gwrlc = 256 * period_ps * txq->speed / 1000000;
> 
> This contains an open-coded 64-by-32 division, causing link failures
> on 32-bit platforms, so you should use div_u64() instead.  However,
> because of the premultiplication by speed, which is 32-bit, you can
> use the mul_u64_u32_div() helper.

Thank you for your comment! After I got emails from kernel test robot,
I realized that I should use such a macro here.

> Combining this with the calculation of period_ps above, you can simplify
> this to:
> 
>     gwrlc = div64_ul(256000000ULL * txq->speed, rate);

Thank you for your suggestion! I'll fix it on v2.

Best regards,
Yoshihiro Shimoda

> > +
> > +       /* To avoid overflow internally, the value should be 97% */
> > +       gwrlc = gwrlc * 97 / 100;
> >
> > -       iowrite32(gwgrlulc, priv->addr + GWGRLULC);
> > -       iowrite32(gwgrlc, priv->addr + GWGRLC);
> > +       dev_dbg(&priv->pdev->dev,
> > +               "%s: index = %d, speed = %d, rate = %ld, gwrlc = %08x\n",
> > +               __func__, txq->index_trim, txq->speed, rate, gwrlc);
> > +
> > +       iowrite32(GWRLULC_NOT_REQUIRED, priv->addr + GWRLULC(txq->index_trim));
> > +       iowrite32(gwrlc | GWRLC_RLE, priv->addr + GWRLC(txq->index_trim));
> >  }
> >
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH net-next 5/5] net: renesas: rswitch: Use per-queue rate limiter

[Simon Horman]

On Mon, May 29, 2023 at 05:08:40PM +0900, Yoshihiro Shimoda wrote:
> Use per-queue rate limiter instead of global rate limiter. Otherwise
> TX performance will be low when we use multiple ports at the same time.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/net/ethernet/renesas/rswitch.c | 51 +++++++++++++++++---------
>  drivers/net/ethernet/renesas/rswitch.h | 15 +++++++-
>  2 files changed, 47 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
> index 4ae34b0206cd..a7195625a2c7 100644
> --- a/drivers/net/ethernet/renesas/rswitch.c
> +++ b/drivers/net/ethernet/renesas/rswitch.c
> @@ -156,22 +156,31 @@ static int rswitch_gwca_axi_ram_reset(struct rswitch_private *priv)
>  	return rswitch_reg_wait(priv->addr, GWARIRM, GWARIRM_ARR, GWARIRM_ARR);
>  }
>  
> -static void rswitch_gwca_set_rate_limit(struct rswitch_private *priv, int rate)
> +static void rswitch_gwca_set_rate_limit(struct rswitch_private *priv,
> +					struct rswitch_gwca_queue *txq)
>  {
> -	u32 gwgrlulc, gwgrlc;
> +	u64 period_ps;
> +	unsigned long rate;
> +	u32 gwrlc;

Hi Shimoda-san,

a minor not from my side: please use reverse xmas tree order - longest line
to shortest - for local variable declarations in networking code.

	unsigned long rate;
	u64 period_ps;
	u32 gwrlc;

-- 
pw-bot: cr

RE: [PATCH net-next 5/5] net: renesas: rswitch: Use per-queue rate limiter

[Yoshihiro Shimoda]

Hi Simon-san,

> From: Simon Horman, Sent: Wednesday, May 31, 2023 4:29 AM
> 
> On Mon, May 29, 2023 at 05:08:40PM +0900, Yoshihiro Shimoda wrote:
> > Use per-queue rate limiter instead of global rate limiter. Otherwise
> > TX performance will be low when we use multiple ports at the same time.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/net/ethernet/renesas/rswitch.c | 51 +++++++++++++++++---------
> >  drivers/net/ethernet/renesas/rswitch.h | 15 +++++++-
> >  2 files changed, 47 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
> > index 4ae34b0206cd..a7195625a2c7 100644
> > --- a/drivers/net/ethernet/renesas/rswitch.c
> > +++ b/drivers/net/ethernet/renesas/rswitch.c
> > @@ -156,22 +156,31 @@ static int rswitch_gwca_axi_ram_reset(struct rswitch_private *priv)
> >  	return rswitch_reg_wait(priv->addr, GWARIRM, GWARIRM_ARR, GWARIRM_ARR);
> >  }
> >
> > -static void rswitch_gwca_set_rate_limit(struct rswitch_private *priv, int rate)
> > +static void rswitch_gwca_set_rate_limit(struct rswitch_private *priv,
> > +					struct rswitch_gwca_queue *txq)
> >  {
> > -	u32 gwgrlulc, gwgrlc;
> > +	u64 period_ps;
> > +	unsigned long rate;
> > +	u32 gwrlc;
> 
> Hi Shimoda-san,
> 
> a minor not from my side: please use reverse xmas tree order - longest line
> to shortest - for local variable declarations in networking code.
> 
> 	unsigned long rate;
> 	u64 period_ps;
> 	u32 gwrlc;

Thank you for your comment! I completely forgot that network driver should use
reverse xmas tree order...

JFYI, I found another way to improve the performance. So, I dropped this patch on v2 [1].

[1] https://lore.kernel.org/all/20230606085558.1708766-1-yoshihiro.shimoda.uh@renesas.com/

Best regards,
Yoshihiro Shimoda

> --
> pw-bot: cr

Re: [PATCH net-next 0/5] net: renesas: rswitch: Improve performance of TX

[Andrew Lunn]

On Mon, May 29, 2023 at 05:08:35PM +0900, Yoshihiro Shimoda wrote:
> This patch series is based on next-20230525.

Hi Yoshihiro

Patches for networking should be based on the HEAD of net-next/main.

	Andrew

RE: [PATCH net-next 0/5] net: renesas: rswitch: Improve performance of TX

[Yoshihiro Shimoda]

Hi Andrew,

> From: Andrew Lunn, Sent: Tuesday, May 30, 2023 1:23 AM
> 
> On Mon, May 29, 2023 at 05:08:35PM +0900, Yoshihiro Shimoda wrote:
> > This patch series is based on next-20230525.
> 
> Hi Yoshihiro
> 
> Patches for networking should be based on the HEAD of net-next/main.

Oops. I got it. I'll rebase on the HEAD of net-next/main.

Best regards,
Yoshihiro Shimoda

> 	Andrew