[PATCH net-next 00/18] Add Gigabit Ethernet driver support

[PATCH net-next 00/18] Add Gigabit Ethernet driver support

[Biju Das]

The DMAC and EMAC blocks of Gigabit Ethernet IP is almost similar to Ethernet AVB.

The Gigabit Etherner IP consists of Ethernet controller (E-MAC), Internal TCP/IP Offload Engine (TOE) and Dedicated Direct memory access controller (DMAC).

With few changes in driver, we can support Gigabit ethernet driver as well.

This patch series is aims to support the same

RFC->V1
  * Incorporated feedback from Andrew, Sergei, Geert and Prabhakar
  * https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=515525

Biju Das (18):
  dt-bindings: net: renesas,etheravb: Document Gigabit Ethernet IP
  drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks
  drivers: clk: renesas: r9a07g044-cpg: Add ethernet clock sources
  drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset
  ravb: Replace chip type with a structure for driver data
  ravb: Factorise ptp feature
  ravb: Add features specific to R-Car Gen3
  ravb: Add R-Car common features
  ravb: Factorise ravb_ring_free function
  ravb: Factorise ravb_ring_format function
  ravb: Factorise ravb_ring_init function
  ravb: Factorise {emac,dmac} init function
  ravb: Factorise ravb_rx function
  ravb: Factorise ravb_adjust_link function
  ravb: Factorise ravb_set_features
  ravb: Add reset support
  ravb: Add GbEthernet driver support
  arm64: dts: renesas: r9a07g044: Add GbEther nodes

 .../bindings/net/renesas,etheravb.yaml        |  57 +-
 arch/arm64/boot/dts/renesas/r9a07g044.dtsi    |  42 +
 drivers/clk/renesas/r9a07g044-cpg.c           |  27 +
 drivers/clk/renesas/rzg2l-cpg.c               |  24 +
 drivers/clk/renesas/rzg2l-cpg.h               |  15 +
 drivers/net/ethernet/renesas/ravb.h           | 112 ++-
 drivers/net/ethernet/renesas/ravb_main.c      | 922 +++++++++++++++---
 7 files changed, 1031 insertions(+), 168 deletions(-)

-- 
2.17.1

[PATCH net-next 01/18] dt-bindings: net: renesas,etheravb: Document Gigabit Ethernet IP

[Biju Das]

Document Gigabit Ethernet IP found on RZ/G2L SoC.

Gigabit Ethernet Interface includes Ethernet controller (E-MAC),
Internal TCP/IP Offload Engine (TOE) and Dedicated Direct memory
access controller (DMAC) for transferring transmitted Ethernet
frames to and received Ethernet frames from respective storage
areas in the URAM at high speed.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 .../bindings/net/renesas,etheravb.yaml        | 57 +++++++++++++++----
 1 file changed, 45 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/renesas,etheravb.yaml b/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
index 005868f703a6..5e12a759004f 100644
--- a/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
+++ b/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
@@ -43,23 +43,20 @@ properties:
               - renesas,etheravb-r8a779a0     # R-Car V3U
           - const: renesas,etheravb-rcar-gen3 # R-Car Gen3 and RZ/G2
 
+      - items:
+          - enum:
+              - renesas,r9a07g044-gbeth # RZ/G2{L,LC}
+          - const: renesas,rzg2l-gbeth  # RZ/G2L
+
   reg: true
 
   interrupts: true
 
   interrupt-names: true
 
-  clocks:
-    minItems: 1
-    items:
-      - description: AVB functional clock
-      - description: Optional TXC reference clock
+  clocks: true
 
-  clock-names:
-    minItems: 1
-    items:
-      - const: fck
-      - const: refclk
+  clock-names: true
 
   iommus:
     maxItems: 1
@@ -145,14 +142,20 @@ allOf:
       properties:
         compatible:
           contains:
-            const: renesas,etheravb-rcar-gen2
+            enum:
+              - renesas,etheravb-rcar-gen2
+              - renesas,rzg2l-gbeth
     then:
       properties:
         interrupts:
-          maxItems: 1
+          minItems: 1
+          maxItems: 3
         interrupt-names:
+          minItems: 1
           items:
             - const: mux
+            - const: int_fil_n
+            - const: int_arp_ns_n
         rx-internal-delay-ps: false
     else:
       properties:
@@ -208,6 +211,36 @@ allOf:
         tx-internal-delay-ps:
           const: 2000
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: renesas,rzg2l-gbeth
+    then:
+      properties:
+        clocks:
+          items:
+            - description: Main clock
+            - description: Register access clock
+            - description: Reference clock for RGMII
+        clock-names:
+          items:
+            - const: axi
+            - const: chi
+            - const: refclk
+    else:
+      properties:
+        clocks:
+          minItems: 1
+          items:
+            - description: AVB functional clock
+            - description: Optional TXC reference clock
+        clock-names:
+          minItems: 1
+          items:
+            - const: fck
+            - const: refclk
+
 additionalProperties: false
 
 examples:
-- 
2.17.1

[PATCH net-next 02/18] drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks

[Biju Das]

Add support to handle mux clocks inorder to select a clock source
from multiple sources.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/clk/renesas/rzg2l-cpg.c | 24 ++++++++++++++++++++++++
 drivers/clk/renesas/rzg2l-cpg.h |  9 +++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index 3b3b2c3347f3..491b10da5766 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -130,6 +130,27 @@ rzg2l_cpg_div_clk_register(const struct cpg_core_clk *core,
 	return clk_hw->clk;
 }
 
+static struct clk * __init
+rzg2l_cpg_mux_clk_register(const struct cpg_core_clk *core,
+			   void __iomem *base,
+			   struct rzg2l_cpg_priv *priv)
+{
+	const struct clk_hw *clk_hw;
+
+	clk_hw = devm_clk_hw_register_mux(priv->dev, core->name,
+					  core->parent_names, core->num_parents,
+					  core->flag,
+					  base + GET_REG_OFFSET(core->conf),
+					  GET_SHIFT(core->conf),
+					  GET_WIDTH(core->conf),
+					  core->mux_flags, &priv->rmw_lock);
+
+	if (IS_ERR(clk_hw))
+		return ERR_CAST(clk_hw);
+
+	return clk_hw->clk;
+}
+
 struct pll_clk {
 	struct clk_hw hw;
 	unsigned int conf;
@@ -288,6 +309,9 @@ rzg2l_cpg_register_core_clk(const struct cpg_core_clk *core,
 		clk = rzg2l_cpg_div_clk_register(core, priv->clks,
 						 priv->base, priv);
 		break;
+	case CLK_TYPE_MUX:
+		clk = rzg2l_cpg_mux_clk_register(core, priv->base, priv);
+		break;
 	default:
 		goto fail;
 	}
diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
index 63695280ce8b..148db5de253b 100644
--- a/drivers/clk/renesas/rzg2l-cpg.h
+++ b/drivers/clk/renesas/rzg2l-cpg.h
@@ -43,6 +43,7 @@ struct cpg_core_clk {
 	const struct clk_div_table *dtable;
 	const char * const *parent_names;
 	int flag;
+	int mux_flags;
 	int num_parents;
 };
 
@@ -54,6 +55,9 @@ enum clk_types {
 
 	/* Clock with divider */
 	CLK_TYPE_DIV,
+
+	/* Clock with clock source selector */
+	CLK_TYPE_MUX,
 };
 
 #define DEF_TYPE(_name, _id, _type...) \
@@ -69,6 +73,11 @@ enum clk_types {
 #define DEF_DIV(_name, _id, _parent, _conf, _dtable, _flag) \
 	DEF_TYPE(_name, _id, CLK_TYPE_DIV, .conf = _conf, \
 		 .parent = _parent, .dtable = _dtable, .flag = _flag)
+#define DEF_MUX(_name, _id, _conf, _parent_names, _num_parents, _flag, \
+		_mux_flags) \
+	DEF_TYPE(_name, _id, CLK_TYPE_MUX, .conf = _conf, \
+		 .parent_names = _parent_names, .num_parents = _num_parents, \
+		 .flag = _flag, .mux_flags = _mux_flags)
 
 /**
  * struct rzg2l_mod_clk - Module Clocks definitions
-- 
2.17.1

Re: [PATCH net-next 02/18] drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks

[Sergei Shtylyov]

Hello!

On 22.07.2021 17:13, Biju Das wrote:

> Add support to handle mux clocks inorder to select a clock source

    In order.

> from multiple sources.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>   drivers/clk/renesas/rzg2l-cpg.c | 24 ++++++++++++++++++++++++
>   drivers/clk/renesas/rzg2l-cpg.h |  9 +++++++++
>   2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
> index 3b3b2c3347f3..491b10da5766 100644
> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -130,6 +130,27 @@ rzg2l_cpg_div_clk_register(const struct cpg_core_clk *core,
>   	return clk_hw->clk;
>   }
>   
> +static struct clk * __init
> +rzg2l_cpg_mux_clk_register(const struct cpg_core_clk *core,
> +			   void __iomem *base,
> +			   struct rzg2l_cpg_priv *priv)
> +{
> +	const struct clk_hw *clk_hw;
> +
> +	clk_hw = devm_clk_hw_register_mux(priv->dev, core->name,
> +					  core->parent_names, core->num_parents,
> +					  core->flag,
> +					  base + GET_REG_OFFSET(core->conf),
> +					  GET_SHIFT(core->conf),
> +					  GET_WIDTH(core->conf),
> +					  core->mux_flags, &priv->rmw_lock);
> +

    Empty line is hardly needed here...

> +	if (IS_ERR(clk_hw))
> +		return ERR_CAST(clk_hw);
> +
> +	return clk_hw->clk;
> +}
> +
>   struct pll_clk {
>   	struct clk_hw hw;
>   	unsigned int conf;
[...]

MBR, Sergei

RE: [PATCH net-next 02/18] drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks

[Biju Das]

Hi Sergei,

Thanks for the review.

> Subject: Re: [PATCH net-next 02/18] drivers: clk: renesas: rzg2l-cpg: Add
> support to handle MUX clocks
> 
> Hello!
> 
> On 22.07.2021 17:13, Biju Das wrote:
> 
> > Add support to handle mux clocks inorder to select a clock source
> 
>     In order.

Ok.
> 
> > from multiple sources.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >   drivers/clk/renesas/rzg2l-cpg.c | 24 ++++++++++++++++++++++++
> >   drivers/clk/renesas/rzg2l-cpg.h |  9 +++++++++
> >   2 files changed, 33 insertions(+)
> >
> > diff --git a/drivers/clk/renesas/rzg2l-cpg.c
> > b/drivers/clk/renesas/rzg2l-cpg.c index 3b3b2c3347f3..491b10da5766
> > 100644
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > @@ -130,6 +130,27 @@ rzg2l_cpg_div_clk_register(const struct
> cpg_core_clk *core,
> >   	return clk_hw->clk;
> >   }
> >
> > +static struct clk * __init
> > +rzg2l_cpg_mux_clk_register(const struct cpg_core_clk *core,
> > +			   void __iomem *base,
> > +			   struct rzg2l_cpg_priv *priv)
> > +{
> > +	const struct clk_hw *clk_hw;
> > +
> > +	clk_hw = devm_clk_hw_register_mux(priv->dev, core->name,
> > +					  core->parent_names, core->num_parents,
> > +					  core->flag,
> > +					  base + GET_REG_OFFSET(core->conf),
> > +					  GET_SHIFT(core->conf),
> > +					  GET_WIDTH(core->conf),
> > +					  core->mux_flags, &priv->rmw_lock);
> > +
> 
>     Empty line is hardly needed here...

OK. Will fix it in next version.

Cheers,
Biju

Re: [PATCH net-next 02/18] drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks

[Geert Uytterhoeven]

Hi Biju,


On Thu, Jul 22, 2021 at 4:14 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Add support to handle mux clocks inorder to select a clock source
> from multiple sources.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> --- a/drivers/clk/renesas/rzg2l-cpg.h
> +++ b/drivers/clk/renesas/rzg2l-cpg.h
> @@ -43,6 +43,7 @@ struct cpg_core_clk {
>         const struct clk_div_table *dtable;
>         const char * const *parent_names;
>         int flag;
> +       int mux_flags;
>         int num_parents;
>  };
>

I'd move SEL_PLL_PACK() from [PATCH 03/18] here, as it applies to
"_conf" in DEF_MUX() below.

> @@ -54,6 +55,9 @@ enum clk_types {
>
>         /* Clock with divider */
>         CLK_TYPE_DIV,
> +
> +       /* Clock with clock source selector */
> +       CLK_TYPE_MUX,
>  };
>
>  #define DEF_TYPE(_name, _id, _type...) \
> @@ -69,6 +73,11 @@ enum clk_types {
>  #define DEF_DIV(_name, _id, _parent, _conf, _dtable, _flag) \
>         DEF_TYPE(_name, _id, CLK_TYPE_DIV, .conf = _conf, \
>                  .parent = _parent, .dtable = _dtable, .flag = _flag)
> +#define DEF_MUX(_name, _id, _conf, _parent_names, _num_parents, _flag, \
> +               _mux_flags) \
> +       DEF_TYPE(_name, _id, CLK_TYPE_MUX, .conf = _conf, \
> +                .parent_names = _parent_names, .num_parents = _num_parents, \
> +                .flag = _flag, .mux_flags = _mux_flags)
>
>  /**
>   * struct rzg2l_mod_clk - Module Clocks definitions

With the above fixed:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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 02/18] drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH net-next 02/18] drivers: clk: renesas: rzg2l-cpg: Add
> support to handle MUX clocks
> 
> Hi Biju,
> 
> 
> On Thu, Jul 22, 2021 at 4:14 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > Add support to handle mux clocks inorder to select a clock source from
> > multiple sources.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> > --- a/drivers/clk/renesas/rzg2l-cpg.h
> > +++ b/drivers/clk/renesas/rzg2l-cpg.h
> > @@ -43,6 +43,7 @@ struct cpg_core_clk {
> >         const struct clk_div_table *dtable;
> >         const char * const *parent_names;
> >         int flag;
> > +       int mux_flags;
> >         int num_parents;
> >  };
> >
> 
> I'd move SEL_PLL_PACK() from [PATCH 03/18] here, as it applies to "_conf"
> in DEF_MUX() below.

OK. Will move in this patch

Regards,
Biju

> 
> > @@ -54,6 +55,9 @@ enum clk_types {
> >
> >         /* Clock with divider */
> >         CLK_TYPE_DIV,
> > +
> > +       /* Clock with clock source selector */
> > +       CLK_TYPE_MUX,
> >  };
> >
> >  #define DEF_TYPE(_name, _id, _type...) \ @@ -69,6 +73,11 @@ enum
> > clk_types {  #define DEF_DIV(_name, _id, _parent, _conf, _dtable,
> > _flag) \
> >         DEF_TYPE(_name, _id, CLK_TYPE_DIV, .conf = _conf, \
> >                  .parent = _parent, .dtable = _dtable, .flag = _flag)
> > +#define DEF_MUX(_name, _id, _conf, _parent_names, _num_parents, _flag,
> \
> > +               _mux_flags) \
> > +       DEF_TYPE(_name, _id, CLK_TYPE_MUX, .conf = _conf, \
> > +                .parent_names = _parent_names, .num_parents =
> _num_parents, \
> > +                .flag = _flag, .mux_flags = _mux_flags)
> >
> >  /**
> >   * struct rzg2l_mod_clk - Module Clocks definitions
> 
> With the above fixed:
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> 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 03/18] drivers: clk: renesas: r9a07g044-cpg: Add ethernet clock sources

[Biju Das]

Ethernet reference clock can be sourced from PLL5_2 or PLL6_2. Add support
for ethernet source clock selection using SEL_PLL_6_2 mux.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/clk/renesas/r9a07g044-cpg.c | 17 +++++++++++++++++
 drivers/clk/renesas/rzg2l-cpg.h     |  6 ++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c
index 9e9e8fb6d00d..c78bea2f6ea8 100644
--- a/drivers/clk/renesas/r9a07g044-cpg.c
+++ b/drivers/clk/renesas/r9a07g044-cpg.c
@@ -35,8 +35,10 @@ enum clk_ids {
 	CLK_PLL3_DIV4,
 	CLK_PLL4,
 	CLK_PLL5,
+	CLK_PLL5_2,
 	CLK_PLL5_DIV2,
 	CLK_PLL6,
+	CLK_PLL6_DIV2,
 	CLK_P1_DIV2,
 
 	/* Module Clocks */
@@ -53,6 +55,9 @@ static const struct clk_div_table dtable_1_32[] = {
 	{0, 0},
 };
 
+/* Mux clock tables */
+static const char * const sel_pll6_2[]	= { ".pll6_2_div2", ".pll5_2_div2" };
+
 static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
 	/* External Clock Inputs */
 	DEF_INPUT("extal", CLK_EXTAL),
@@ -64,6 +69,11 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
 	DEF_FIXED(".pll2", CLK_PLL2, CLK_EXTAL, 133, 2),
 	DEF_FIXED(".pll3", CLK_PLL3, CLK_EXTAL, 133, 2),
 
+	DEF_FIXED(".pll5", CLK_PLL5, CLK_EXTAL, 125, 1),
+	DEF_FIXED(".pll5_2", CLK_PLL5_2, CLK_PLL5, 1, 6),
+
+	DEF_FIXED(".pll6", CLK_PLL6, CLK_EXTAL, 125, 6),
+
 	DEF_FIXED(".pll2_div2", CLK_PLL2_DIV2, CLK_PLL2, 1, 2),
 	DEF_FIXED(".pll2_div16", CLK_PLL2_DIV16, CLK_PLL2, 1, 16),
 	DEF_FIXED(".pll2_div20", CLK_PLL2_DIV20, CLK_PLL2, 1, 20),
@@ -73,6 +83,9 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
 	DEF_FIXED(".pll3_div2_4_2", CLK_PLL3_DIV2_4_2, CLK_PLL3_DIV2_4, 1, 2),
 	DEF_FIXED(".pll3_div4", CLK_PLL3_DIV4, CLK_PLL3, 1, 4),
 
+	DEF_FIXED(".pll5_2_div2", CLK_PLL5_DIV2, CLK_PLL5_2, 1, 2),
+	DEF_FIXED(".pll6_2_div2", CLK_PLL6_DIV2, CLK_PLL6, 1, 2),
+
 	/* Core output clk */
 	DEF_FIXED("I", R9A07G044_CLK_I, CLK_PLL1, 1, 1),
 	DEF_DIV("P0", R9A07G044_CLK_P0, CLK_PLL2_DIV16, DIVPL2A,
@@ -83,6 +96,10 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
 	DEF_FIXED("P1_DIV2", CLK_P1_DIV2, R9A07G044_CLK_P1, 1, 2),
 	DEF_DIV("P2", R9A07G044_CLK_P2, CLK_PLL3_DIV2_4_2,
 		DIVPL3A, dtable_1_32, CLK_DIVIDER_HIWORD_MASK),
+	DEF_FIXED("M0", R9A07G044_CLK_M0, CLK_PLL3_DIV2_4, 1, 1),
+	DEF_FIXED("ZT", R9A07G044_CLK_ZT, CLK_PLL3_DIV2_4_2, 1, 1),
+	DEF_MUX("HP", R9A07G044_CLK_HP, SEL_PLL6_2,
+		sel_pll6_2, ARRAY_SIZE(sel_pll6_2), 0, CLK_MUX_HIWORD_MASK),
 };
 
 static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
index 148db5de253b..5202c0512483 100644
--- a/drivers/clk/renesas/rzg2l-cpg.h
+++ b/drivers/clk/renesas/rzg2l-cpg.h
@@ -11,6 +11,7 @@
 
 #define CPG_PL2_DDIV		(0x204)
 #define CPG_PL3A_DDIV		(0x208)
+#define CPG_PL6_ETH_SSEL	(0x418)
 
 /* n = 0/1/2 for PLL1/4/6 */
 #define CPG_SAMPLL_CLK1(n)	(0x04 + (16 * n))
@@ -24,6 +25,11 @@
 #define DIVPL3A		DDIV_PACK(CPG_PL3A_DDIV, 0, 3)
 #define DIVPL3B		DDIV_PACK(CPG_PL3A_DDIV, 4, 3)
 
+#define SEL_PLL_PACK(offset, bitpos, size) \
+		(((offset) << 20) | ((bitpos) << 12) | ((size) << 8))
+
+#define SEL_PLL6_2	SEL_PLL_PACK(CPG_PL6_ETH_SSEL, 0, 1)
+
 /**
  * Definitions of CPG Core Clocks
  *
-- 
2.17.1

Re: [PATCH net-next 03/18] drivers: clk: renesas: r9a07g044-cpg: Add ethernet clock sources

[Geert Uytterhoeven]

Hi Biju,

On Thu, Jul 22, 2021 at 4:14 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Ethernet reference clock can be sourced from PLL5_2 or PLL6_2. Add support
> for ethernet source clock selection using SEL_PLL_6_2 mux.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/clk/renesas/r9a07g044-cpg.c
> +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> @@ -35,8 +35,10 @@ enum clk_ids {
>         CLK_PLL3_DIV4,
>         CLK_PLL4,
>         CLK_PLL5,
> +       CLK_PLL5_2,

Why do you need this? We already have CLK_PLL5_DIV2?

>         CLK_PLL5_DIV2,
>         CLK_PLL6,
> +       CLK_PLL6_DIV2,
>         CLK_P1_DIV2,
>
>         /* Module Clocks */
> @@ -53,6 +55,9 @@ static const struct clk_div_table dtable_1_32[] = {
>         {0, 0},
>  };
>
> +/* Mux clock tables */
> +static const char * const sel_pll6_2[] = { ".pll6_2_div2", ".pll5_2_div2" };

".pll6_div2", ".pll5_div2"

> +
>  static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
>         /* External Clock Inputs */
>         DEF_INPUT("extal", CLK_EXTAL),
> @@ -64,6 +69,11 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
>         DEF_FIXED(".pll2", CLK_PLL2, CLK_EXTAL, 133, 2),
>         DEF_FIXED(".pll3", CLK_PLL3, CLK_EXTAL, 133, 2),
>
> +       DEF_FIXED(".pll5", CLK_PLL5, CLK_EXTAL, 125, 1),
> +       DEF_FIXED(".pll5_2", CLK_PLL5_2, CLK_PLL5, 1, 6),

Suffix "_2" but divided by 6?
Why do you need this clock?

> +
> +       DEF_FIXED(".pll6", CLK_PLL6, CLK_EXTAL, 125, 6),
> +
>         DEF_FIXED(".pll2_div2", CLK_PLL2_DIV2, CLK_PLL2, 1, 2),
>         DEF_FIXED(".pll2_div16", CLK_PLL2_DIV16, CLK_PLL2, 1, 16),
>         DEF_FIXED(".pll2_div20", CLK_PLL2_DIV20, CLK_PLL2, 1, 20),
> @@ -73,6 +83,9 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
>         DEF_FIXED(".pll3_div2_4_2", CLK_PLL3_DIV2_4_2, CLK_PLL3_DIV2_4, 1, 2),
>         DEF_FIXED(".pll3_div4", CLK_PLL3_DIV4, CLK_PLL3, 1, 4),
>
> +       DEF_FIXED(".pll5_2_div2", CLK_PLL5_DIV2, CLK_PLL5_2, 1, 2),

".pll5_div2"
Figure 8.3 ("Clock System Diagram (2)") says the parent is CLK_PLL5?

> +       DEF_FIXED(".pll6_2_div2", CLK_PLL6_DIV2, CLK_PLL6, 1, 2),

".pll6_div2"

> +
>         /* Core output clk */
>         DEF_FIXED("I", R9A07G044_CLK_I, CLK_PLL1, 1, 1),
>         DEF_DIV("P0", R9A07G044_CLK_P0, CLK_PLL2_DIV16, DIVPL2A,
> @@ -83,6 +96,10 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
>         DEF_FIXED("P1_DIV2", CLK_P1_DIV2, R9A07G044_CLK_P1, 1, 2),
>         DEF_DIV("P2", R9A07G044_CLK_P2, CLK_PLL3_DIV2_4_2,
>                 DIVPL3A, dtable_1_32, CLK_DIVIDER_HIWORD_MASK),
> +       DEF_FIXED("M0", R9A07G044_CLK_M0, CLK_PLL3_DIV2_4, 1, 1),
> +       DEF_FIXED("ZT", R9A07G044_CLK_ZT, CLK_PLL3_DIV2_4_2, 1, 1),
> +       DEF_MUX("HP", R9A07G044_CLK_HP, SEL_PLL6_2,
> +               sel_pll6_2, ARRAY_SIZE(sel_pll6_2), 0, CLK_MUX_HIWORD_MASK),

OK.

>  };
>
>  static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
> diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
> index 148db5de253b..5202c0512483 100644
> --- a/drivers/clk/renesas/rzg2l-cpg.h
> +++ b/drivers/clk/renesas/rzg2l-cpg.h
> @@ -11,6 +11,7 @@
>
>  #define CPG_PL2_DDIV           (0x204)
>  #define CPG_PL3A_DDIV          (0x208)
> +#define CPG_PL6_ETH_SSEL       (0x418)
>
>  /* n = 0/1/2 for PLL1/4/6 */
>  #define CPG_SAMPLL_CLK1(n)     (0x04 + (16 * n))
> @@ -24,6 +25,11 @@
>  #define DIVPL3A                DDIV_PACK(CPG_PL3A_DDIV, 0, 3)
>  #define DIVPL3B                DDIV_PACK(CPG_PL3A_DDIV, 4, 3)
>
> +#define SEL_PLL_PACK(offset, bitpos, size) \
> +               (((offset) << 20) | ((bitpos) << 12) | ((size) << 8))

I think the addition of SEL_PLL_PACK() should be moved to
[PATCH 02/18].

> +
> +#define SEL_PLL6_2     SEL_PLL_PACK(CPG_PL6_ETH_SSEL, 0, 1)
> +
>  /**
>   * Definitions of CPG Core Clocks
>   *

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 03/18] drivers: clk: renesas: r9a07g044-cpg: Add ethernet clock sources

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH net-next 03/18] drivers: clk: renesas: r9a07g044-cpg:
> Add ethernet clock sources
> 
> Hi Biju,
> 
> On Thu, Jul 22, 2021 at 4:14 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > Ethernet reference clock can be sourced from PLL5_2 or PLL6_2. Add
> > support for ethernet source clock selection using SEL_PLL_6_2 mux.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/clk/renesas/r9a07g044-cpg.c
> > +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> > @@ -35,8 +35,10 @@ enum clk_ids {
> >         CLK_PLL3_DIV4,
> >         CLK_PLL4,
> >         CLK_PLL5,
> > +       CLK_PLL5_2,
> 
> Why do you need this? We already have CLK_PLL5_DIV2?

As per clock list HP =250 MHz.

With PLL5_DIV2 the frequency, what we get is 3000 x 1/2 = 1500 MHz, which is incorrect

There is some issue with HW manual. It is not clear. I have requested to fix those issue.

Actually PLL5 generates 3 clk outputs (As per page 403 of manual)

FOUTPOSTDIV= 3000
FOUT3 = 500
FOUT1PH0 = 1500

To match the clock list document and Figure 8.3 Clock system diagram for HP clock.
This clock has to be FOUT3.

FOUT3(second PLL5 clock source)* 1/2 =250 MHz(HP clock).

On the HW manual, Divider name for PLL5 is mentioned as PLL_5_2_DIV12 and for PLL6 as PLL6_2_DIV2.

> 
> >         CLK_PLL5_DIV2,
> >         CLK_PLL6,
> > +       CLK_PLL6_DIV2,
> >         CLK_P1_DIV2,
> >
> >         /* Module Clocks */
> > @@ -53,6 +55,9 @@ static const struct clk_div_table dtable_1_32[] = {
> >         {0, 0},
> >  };
> >
> > +/* Mux clock tables */
> > +static const char * const sel_pll6_2[] = { ".pll6_2_div2",
> > +".pll5_2_div2" };
> 
> ".pll6_div2", ".pll5_div2"

Basically PLL6 generates 2 clock outputs. That is the reason it is termed as PLL6_2 for second clock
And pll5 generates 3 clock outputs and pll5_2(second clock source) is used for HP.

To match the register definition, I will change this to ".pll6_2_div2" and ".pll5_2_div12". Is it ok?

> 
> > +
> >  static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
> >         /* External Clock Inputs */
> >         DEF_INPUT("extal", CLK_EXTAL), @@ -64,6 +69,11 @@ static const
> > struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
> >         DEF_FIXED(".pll2", CLK_PLL2, CLK_EXTAL, 133, 2),
> >         DEF_FIXED(".pll3", CLK_PLL3, CLK_EXTAL, 133, 2),
> >
> > +       DEF_FIXED(".pll5", CLK_PLL5, CLK_EXTAL, 125, 1),
> > +       DEF_FIXED(".pll5_2", CLK_PLL5_2, CLK_PLL5, 1, 6),
> 
> Suffix "_2" but divided by 6?
> Why do you need this clock?

To generate 500 MHz Clock, so that it can match with clock list document.

> 
> > +
> > +       DEF_FIXED(".pll6", CLK_PLL6, CLK_EXTAL, 125, 6),
> > +
> >         DEF_FIXED(".pll2_div2", CLK_PLL2_DIV2, CLK_PLL2, 1, 2),
> >         DEF_FIXED(".pll2_div16", CLK_PLL2_DIV16, CLK_PLL2, 1, 16),
> >         DEF_FIXED(".pll2_div20", CLK_PLL2_DIV20, CLK_PLL2, 1, 20), @@
> > -73,6 +83,9 @@ static const struct cpg_core_clk r9a07g044_core_clks[]
> __initconst = {
> >         DEF_FIXED(".pll3_div2_4_2", CLK_PLL3_DIV2_4_2, CLK_PLL3_DIV2_4,
> 1, 2),
> >         DEF_FIXED(".pll3_div4", CLK_PLL3_DIV4, CLK_PLL3, 1, 4),
> >
> > +       DEF_FIXED(".pll5_2_div2", CLK_PLL5_DIV2, CLK_PLL5_2, 1, 2),
> 
> ".pll5_div2"
> Figure 8.3 ("Clock System Diagram (2)") says the parent is CLK_PLL5?

There is some issue in HW manual. Actually parent is second clock out from PLL5. See page 403.


> 
> > +       DEF_FIXED(".pll6_2_div2", CLK_PLL6_DIV2, CLK_PLL6, 1, 2),
> 
> ".pll6_div2"
> 
> > +
> >         /* Core output clk */
> >         DEF_FIXED("I", R9A07G044_CLK_I, CLK_PLL1, 1, 1),
> >         DEF_DIV("P0", R9A07G044_CLK_P0, CLK_PLL2_DIV16, DIVPL2A, @@
> > -83,6 +96,10 @@ static const struct cpg_core_clk r9a07g044_core_clks[]
> __initconst = {
> >         DEF_FIXED("P1_DIV2", CLK_P1_DIV2, R9A07G044_CLK_P1, 1, 2),
> >         DEF_DIV("P2", R9A07G044_CLK_P2, CLK_PLL3_DIV2_4_2,
> >                 DIVPL3A, dtable_1_32, CLK_DIVIDER_HIWORD_MASK),
> > +       DEF_FIXED("M0", R9A07G044_CLK_M0, CLK_PLL3_DIV2_4, 1, 1),
> > +       DEF_FIXED("ZT", R9A07G044_CLK_ZT, CLK_PLL3_DIV2_4_2, 1, 1),
> > +       DEF_MUX("HP", R9A07G044_CLK_HP, SEL_PLL6_2,
> > +               sel_pll6_2, ARRAY_SIZE(sel_pll6_2), 0,
> > + CLK_MUX_HIWORD_MASK),
> 
> OK.
> 
> >  };
> >
> >  static struct rzg2l_mod_clk r9a07g044_mod_clks[] = { diff --git
> > a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
> > index 148db5de253b..5202c0512483 100644
> > --- a/drivers/clk/renesas/rzg2l-cpg.h
> > +++ b/drivers/clk/renesas/rzg2l-cpg.h
> > @@ -11,6 +11,7 @@
> >
> >  #define CPG_PL2_DDIV           (0x204)
> >  #define CPG_PL3A_DDIV          (0x208)
> > +#define CPG_PL6_ETH_SSEL       (0x418)
> >
> >  /* n = 0/1/2 for PLL1/4/6 */
> >  #define CPG_SAMPLL_CLK1(n)     (0x04 + (16 * n))
> > @@ -24,6 +25,11 @@
> >  #define DIVPL3A                DDIV_PACK(CPG_PL3A_DDIV, 0, 3)
> >  #define DIVPL3B                DDIV_PACK(CPG_PL3A_DDIV, 4, 3)
> >
> > +#define SEL_PLL_PACK(offset, bitpos, size) \
> > +               (((offset) << 20) | ((bitpos) << 12) | ((size) << 8))
> 
> I think the addition of SEL_PLL_PACK() should be moved to [PATCH 02/18].
> 

OK.

Regards,
Biju

> > +
> > +#define SEL_PLL6_2     SEL_PLL_PACK(CPG_PL6_ETH_SSEL, 0, 1)
> > +
> >  /**
> >   * Definitions of CPG Core Clocks
> >   *
> 
> 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 03/18] drivers: clk: renesas: r9a07g044-cpg: Add ethernet clock sources

[Geert Uytterhoeven]

Hi Biju,

On Mon, Jul 26, 2021 at 1:43 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH net-next 03/18] drivers: clk: renesas: r9a07g044-cpg:
> > Add ethernet clock sources
> > On Thu, Jul 22, 2021 at 4:14 PM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > Ethernet reference clock can be sourced from PLL5_2 or PLL6_2. Add
> > > support for ethernet source clock selection using SEL_PLL_6_2 mux.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/clk/renesas/r9a07g044-cpg.c
> > > +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> > > @@ -35,8 +35,10 @@ enum clk_ids {
> > >         CLK_PLL3_DIV4,
> > >         CLK_PLL4,
> > >         CLK_PLL5,
> > > +       CLK_PLL5_2,
> >
> > Why do you need this? We already have CLK_PLL5_DIV2?
>
> As per clock list HP =250 MHz.
>
> With PLL5_DIV2 the frequency, what we get is 3000 x 1/2 = 1500 MHz, which is incorrect
>
> There is some issue with HW manual. It is not clear. I have requested to fix those issue.
>
> Actually PLL5 generates 3 clk outputs (As per page 403 of manual)
>
> FOUTPOSTDIV= 3000
> FOUT3 = 500
> FOUT1PH0 = 1500
>
> To match the clock list document and Figure 8.3 Clock system diagram for HP clock.
> This clock has to be FOUT3.
>
> FOUT3(second PLL5 clock source)* 1/2 =250 MHz(HP clock).
>
> On the HW manual, Divider name for PLL5 is mentioned as PLL_5_2_DIV12 and for PLL6 as PLL6_2_DIV2.
>
> >
> > >         CLK_PLL5_DIV2,
> > >         CLK_PLL6,
> > > +       CLK_PLL6_DIV2,
> > >         CLK_P1_DIV2,
> > >
> > >         /* Module Clocks */
> > > @@ -53,6 +55,9 @@ static const struct clk_div_table dtable_1_32[] = {
> > >         {0, 0},
> > >  };
> > >
> > > +/* Mux clock tables */
> > > +static const char * const sel_pll6_2[] = { ".pll6_2_div2",
> > > +".pll5_2_div2" };
> >
> > ".pll6_div2", ".pll5_div2"
>
> Basically PLL6 generates 2 clock outputs. That is the reason it is termed as PLL6_2 for second clock
> And pll5 generates 3 clock outputs and pll5_2(second clock source) is used for HP.
>
> To match the register definition, I will change this to ".pll6_2_div2" and ".pll5_2_div12". Is it ok?

OK, thanks for the explanation!

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 04/18] drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset

[Biju Das]

Add ETH{0,1} clock/reset entries to CPG driver.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/clk/renesas/r9a07g044-cpg.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c
index c78bea2f6ea8..8be09bcdf0f6 100644
--- a/drivers/clk/renesas/r9a07g044-cpg.c
+++ b/drivers/clk/renesas/r9a07g044-cpg.c
@@ -137,6 +137,14 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
 				0x578, 2),
 	DEF_MOD("usb_pclk",	R9A07G044_USB_PCLK, R9A07G044_CLK_P1,
 				0x578, 3),
+	DEF_MOD("eth0_axi",	R9A07G044_ETH0_CLK_AXI, R9A07G044_CLK_M0,
+				0x57c, 0),
+	DEF_MOD("eth0_chi",	R9A07G044_ETH0_CLK_CHI, R9A07G044_CLK_ZT,
+				0x57c, 0),
+	DEF_MOD("eth1_axi",	R9A07G044_ETH1_CLK_AXI, R9A07G044_CLK_M0,
+				0x57c, 1),
+	DEF_MOD("eth1_chi",	R9A07G044_ETH1_CLK_CHI, R9A07G044_CLK_ZT,
+				0x57c, 1),
 	DEF_MOD("i2c0",		R9A07G044_I2C0_PCLK, R9A07G044_CLK_P0,
 				0x580, 0),
 	DEF_MOD("i2c1",		R9A07G044_I2C1_PCLK, R9A07G044_CLK_P0,
@@ -181,6 +189,8 @@ static struct rzg2l_reset r9a07g044_resets[] = {
 	DEF_RST(R9A07G044_USB_U2H1_HRESETN, 0x878, 1),
 	DEF_RST(R9A07G044_USB_U2P_EXL_SYSRST, 0x878, 2),
 	DEF_RST(R9A07G044_USB_PRESETN, 0x878, 3),
+	DEF_RST(R9A07G044_ETH0_RST_HW_N, 0x87c, 0),
+	DEF_RST(R9A07G044_ETH1_RST_HW_N, 0x87c, 1),
 	DEF_RST(R9A07G044_I2C0_MRST, 0x880, 0),
 	DEF_RST(R9A07G044_I2C1_MRST, 0x880, 1),
 	DEF_RST(R9A07G044_I2C2_MRST, 0x880, 2),
-- 
2.17.1

Re: [PATCH net-next 04/18] drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset

[Geert Uytterhoeven]

Hi Biju,

On Thu, Jul 22, 2021 at 4:14 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Add ETH{0,1} clock/reset entries to CPG driver.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/clk/renesas/r9a07g044-cpg.c
> +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> @@ -137,6 +137,14 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
>                                 0x578, 2),
>         DEF_MOD("usb_pclk",     R9A07G044_USB_PCLK, R9A07G044_CLK_P1,
>                                 0x578, 3),
> +       DEF_MOD("eth0_axi",     R9A07G044_ETH0_CLK_AXI, R9A07G044_CLK_M0,
> +                               0x57c, 0),
> +       DEF_MOD("eth0_chi",     R9A07G044_ETH0_CLK_CHI, R9A07G044_CLK_ZT,
> +                               0x57c, 0),
> +       DEF_MOD("eth1_axi",     R9A07G044_ETH1_CLK_AXI, R9A07G044_CLK_M0,
> +                               0x57c, 1),
> +       DEF_MOD("eth1_chi",     R9A07G044_ETH1_CLK_CHI, R9A07G044_CLK_ZT,
> +                               0x57c, 1),

The AXI and CHI clocks use the same register bits, so this won't work
as expected. E.g. when disabling one clock, the other clock will be
disabled, too. The correct way to handle this is to create a new clock
type for coupled clocks, which sets the CPG_CLKON_ETH.CLK[01]_ON bit
when at least one clock is enabled, and clears the bit only when both
clocks are disabled.

>         DEF_MOD("i2c0",         R9A07G044_I2C0_PCLK, R9A07G044_CLK_P0,
>                                 0x580, 0),
>         DEF_MOD("i2c1",         R9A07G044_I2C1_PCLK, R9A07G044_CLK_P0,
> @@ -181,6 +189,8 @@ static struct rzg2l_reset r9a07g044_resets[] = {
>         DEF_RST(R9A07G044_USB_U2H1_HRESETN, 0x878, 1),
>         DEF_RST(R9A07G044_USB_U2P_EXL_SYSRST, 0x878, 2),
>         DEF_RST(R9A07G044_USB_PRESETN, 0x878, 3),
> +       DEF_RST(R9A07G044_ETH0_RST_HW_N, 0x87c, 0),
> +       DEF_RST(R9A07G044_ETH1_RST_HW_N, 0x87c, 1),
>         DEF_RST(R9A07G044_I2C0_MRST, 0x880, 0),
>         DEF_RST(R9A07G044_I2C1_MRST, 0x880, 1),
>         DEF_RST(R9A07G044_I2C2_MRST, 0x880, 2),

This part is OK.

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 04/18] drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH net-next 04/18] drivers: clk: renesas: r9a07g044-cpg:
> Add GbEthernet clock/reset
> 
> Hi Biju,
> 
> On Thu, Jul 22, 2021 at 4:14 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > Add ETH{0,1} clock/reset entries to CPG driver.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/clk/renesas/r9a07g044-cpg.c
> > +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> > @@ -137,6 +137,14 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] =
> {
> >                                 0x578, 2),
> >         DEF_MOD("usb_pclk",     R9A07G044_USB_PCLK, R9A07G044_CLK_P1,
> >                                 0x578, 3),
> > +       DEF_MOD("eth0_axi",     R9A07G044_ETH0_CLK_AXI,
> R9A07G044_CLK_M0,
> > +                               0x57c, 0),
> > +       DEF_MOD("eth0_chi",     R9A07G044_ETH0_CLK_CHI,
> R9A07G044_CLK_ZT,
> > +                               0x57c, 0),
> > +       DEF_MOD("eth1_axi",     R9A07G044_ETH1_CLK_AXI,
> R9A07G044_CLK_M0,
> > +                               0x57c, 1),
> > +       DEF_MOD("eth1_chi",     R9A07G044_ETH1_CLK_CHI,
> R9A07G044_CLK_ZT,
> > +                               0x57c, 1),
> 
> The AXI and CHI clocks use the same register bits, so this won't work as
> expected. E.g. when disabling one clock, the other clock will be disabled,
> too. The correct way to handle this is to create a new clock type for
> coupled clocks, which sets the CPG_CLKON_ETH.CLK[01]_ON bit when at least
> one clock is enabled, and clears the bit only when both clocks are
> disabled.

OK. Will create new clk type and add the logic to handle the same.

Regards,
Biju

> 
> >         DEF_MOD("i2c0",         R9A07G044_I2C0_PCLK, R9A07G044_CLK_P0,
> >                                 0x580, 0),
> >         DEF_MOD("i2c1",         R9A07G044_I2C1_PCLK, R9A07G044_CLK_P0,
> > @@ -181,6 +189,8 @@ static struct rzg2l_reset r9a07g044_resets[] = {
> >         DEF_RST(R9A07G044_USB_U2H1_HRESETN, 0x878, 1),
> >         DEF_RST(R9A07G044_USB_U2P_EXL_SYSRST, 0x878, 2),
> >         DEF_RST(R9A07G044_USB_PRESETN, 0x878, 3),
> > +       DEF_RST(R9A07G044_ETH0_RST_HW_N, 0x87c, 0),
> > +       DEF_RST(R9A07G044_ETH1_RST_HW_N, 0x87c, 1),
> >         DEF_RST(R9A07G044_I2C0_MRST, 0x880, 0),
> >         DEF_RST(R9A07G044_I2C1_MRST, 0x880, 1),
> >         DEF_RST(R9A07G044_I2C2_MRST, 0x880, 2),
> 
> This part is OK.
> 
> 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 05/18] ravb: Replace chip type with a structure for driver data

[Biju Das]

The DMAC and EMAC blocks of Gigabit Ethernet IP is almost similar to
Ethernet AVB. With few changes in driver we can support both the IP.

This patch is in preparation for supporting the same by replacing chip
type by a structure with values, feature bits and function pointers.

Currently only values is added to structure and later patches will add
features and function pointers.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      | 14 +++++
 drivers/net/ethernet/renesas/ravb_main.c | 76 +++++++++++++++++-------
 2 files changed, 67 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 80e62ca2e3d3..0ed21262f26b 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -988,6 +988,18 @@ enum ravb_chip_id {
 	RCAR_GEN3,
 };
 
+struct ravb_drv_data {
+	netdev_features_t net_features;
+	netdev_features_t net_hw_features;
+	const char (*gstrings_stats)[ETH_GSTRING_LEN];
+	size_t gstrings_size;
+	size_t stats_len;
+	u32 num_gstat_queue;
+	size_t skb_sz;
+	u8 num_tx_desc;
+	enum ravb_chip_id chip_id;
+};
+
 struct ravb_private {
 	struct net_device *ndev;
 	struct platform_device *pdev;
@@ -1040,6 +1052,8 @@ struct ravb_private {
 	unsigned txcidm:1;		/* TX Clock Internal Delay Mode */
 	unsigned rgmii_override:1;	/* Deprecated rgmii-*id behavior */
 	int num_tx_desc;		/* TX descriptors per packet */
+
+	const struct ravb_drv_data *info;
 };
 
 static inline u32 ravb_read(struct net_device *ndev, enum ravb_reg reg)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 805397088850..84ebd6fef711 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -339,6 +339,7 @@ static void ravb_ring_format(struct net_device *ndev, int q)
 static int ravb_ring_init(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 	int num_tx_desc = priv->num_tx_desc;
 	struct sk_buff *skb;
 	int ring_size;
@@ -353,7 +354,7 @@ static int ravb_ring_init(struct net_device *ndev, int q)
 		goto error;
 
 	for (i = 0; i < priv->num_rx_ring[q]; i++) {
-		skb = netdev_alloc_skb(ndev, RX_BUF_SZ + RAVB_ALIGN - 1);
+		skb = netdev_alloc_skb(ndev, info->skb_sz);
 		if (!skb)
 			goto error;
 		ravb_set_buffer_align(skb);
@@ -1133,13 +1134,14 @@ static const char ravb_gstrings_stats[][ETH_GSTRING_LEN] = {
 	"rx_queue_1_over_errors",
 };
 
-#define RAVB_STATS_LEN	ARRAY_SIZE(ravb_gstrings_stats)
-
 static int ravb_get_sset_count(struct net_device *netdev, int sset)
 {
+	struct ravb_private *priv = netdev_priv(netdev);
+	const struct ravb_drv_data *info = priv->info;
+
 	switch (sset) {
 	case ETH_SS_STATS:
-		return RAVB_STATS_LEN;
+		return info->stats_len;
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -1149,11 +1151,12 @@ static void ravb_get_ethtool_stats(struct net_device *ndev,
 				   struct ethtool_stats *estats, u64 *data)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 	int i = 0;
 	int q;
 
 	/* Device-specific stats */
-	for (q = RAVB_BE; q < NUM_RX_QUEUE; q++) {
+	for (q = RAVB_BE; q < info->num_gstat_queue; q++) {
 		struct net_device_stats *stats = &priv->stats[q];
 
 		data[i++] = priv->cur_rx[q];
@@ -1176,9 +1179,12 @@ static void ravb_get_ethtool_stats(struct net_device *ndev,
 
 static void ravb_get_strings(struct net_device *ndev, u32 stringset, u8 *data)
 {
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
+
 	switch (stringset) {
 	case ETH_SS_STATS:
-		memcpy(data, ravb_gstrings_stats, sizeof(ravb_gstrings_stats));
+		memcpy(data, info->gstrings_stats, info->gstrings_size);
 		break;
 	}
 }
@@ -1924,12 +1930,36 @@ static int ravb_mdio_release(struct ravb_private *priv)
 	return 0;
 }
 
+static const struct ravb_drv_data ravb_gen3_data = {
+	.net_features = NETIF_F_RXCSUM,
+	.net_hw_features = NETIF_F_RXCSUM,
+	.gstrings_stats = ravb_gstrings_stats,
+	.gstrings_size = sizeof(ravb_gstrings_stats),
+	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
+	.num_gstat_queue = NUM_RX_QUEUE,
+	.skb_sz = RX_BUF_SZ + RAVB_ALIGN - 1,
+	.num_tx_desc = NUM_TX_DESC_GEN3,
+	.chip_id = RCAR_GEN3,
+};
+
+static const struct ravb_drv_data ravb_gen2_data = {
+	.net_features = NETIF_F_RXCSUM,
+	.net_hw_features = NETIF_F_RXCSUM,
+	.gstrings_stats = ravb_gstrings_stats,
+	.gstrings_size = sizeof(ravb_gstrings_stats),
+	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
+	.num_gstat_queue = NUM_RX_QUEUE,
+	.skb_sz = RX_BUF_SZ + RAVB_ALIGN - 1,
+	.num_tx_desc = NUM_TX_DESC_GEN2,
+	.chip_id = RCAR_GEN2,
+};
+
 static const struct of_device_id ravb_match_table[] = {
-	{ .compatible = "renesas,etheravb-r8a7790", .data = (void *)RCAR_GEN2 },
-	{ .compatible = "renesas,etheravb-r8a7794", .data = (void *)RCAR_GEN2 },
-	{ .compatible = "renesas,etheravb-rcar-gen2", .data = (void *)RCAR_GEN2 },
-	{ .compatible = "renesas,etheravb-r8a7795", .data = (void *)RCAR_GEN3 },
-	{ .compatible = "renesas,etheravb-rcar-gen3", .data = (void *)RCAR_GEN3 },
+	{ .compatible = "renesas,etheravb-r8a7790", .data = &ravb_gen2_data },
+	{ .compatible = "renesas,etheravb-r8a7794", .data = &ravb_gen2_data },
+	{ .compatible = "renesas,etheravb-rcar-gen2", .data = &ravb_gen2_data },
+	{ .compatible = "renesas,etheravb-r8a7795", .data = &ravb_gen3_data },
+	{ .compatible = "renesas,etheravb-rcar-gen3", .data = &ravb_gen3_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ravb_match_table);
@@ -2034,8 +2064,8 @@ static void ravb_set_delay_mode(struct net_device *ndev)
 static int ravb_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
+	const struct ravb_drv_data *info;
 	struct ravb_private *priv;
-	enum ravb_chip_id chip_id;
 	struct net_device *ndev;
 	int error, irq, q;
 	struct resource *res;
@@ -2052,15 +2082,15 @@ static int ravb_probe(struct platform_device *pdev)
 	if (!ndev)
 		return -ENOMEM;
 
-	ndev->features = NETIF_F_RXCSUM;
-	ndev->hw_features = NETIF_F_RXCSUM;
+	info = of_device_get_match_data(&pdev->dev);
+
+	ndev->features = info->net_features;
+	ndev->hw_features = info->net_hw_features;
 
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 
-	chip_id = (enum ravb_chip_id)of_device_get_match_data(&pdev->dev);
-
-	if (chip_id == RCAR_GEN3)
+	if (info->chip_id == RCAR_GEN3)
 		irq = platform_get_irq_byname(pdev, "ch22");
 	else
 		irq = platform_get_irq(pdev, 0);
@@ -2073,6 +2103,7 @@ static int ravb_probe(struct platform_device *pdev)
 	SET_NETDEV_DEV(ndev, &pdev->dev);
 
 	priv = netdev_priv(ndev);
+	priv->info = info;
 	priv->ndev = ndev;
 	priv->pdev = pdev;
 	priv->num_tx_ring[RAVB_BE] = BE_TX_RING_SIZE;
@@ -2099,7 +2130,7 @@ static int ravb_probe(struct platform_device *pdev)
 	priv->avb_link_active_low =
 		of_property_read_bool(np, "renesas,ether-link-active-low");
 
-	if (chip_id == RCAR_GEN3) {
+	if (info->chip_id == RCAR_GEN3) {
 		irq = platform_get_irq_byname(pdev, "ch24");
 		if (irq < 0) {
 			error = irq;
@@ -2124,7 +2155,7 @@ static int ravb_probe(struct platform_device *pdev)
 		}
 	}
 
-	priv->chip_id = chip_id;
+	priv->chip_id = info->chip_id;
 
 	priv->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(priv->clk)) {
@@ -2142,8 +2173,7 @@ static int ravb_probe(struct platform_device *pdev)
 	ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
 	ndev->min_mtu = ETH_MIN_MTU;
 
-	priv->num_tx_desc = chip_id == RCAR_GEN2 ?
-		NUM_TX_DESC_GEN2 : NUM_TX_DESC_GEN3;
+	priv->num_tx_desc = info->num_tx_desc;
 
 	/* Set function */
 	ndev->netdev_ops = &ravb_netdev_ops;
@@ -2184,7 +2214,7 @@ static int ravb_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&priv->ts_skb_list);
 
 	/* Initialise PTP Clock driver */
-	if (chip_id != RCAR_GEN2)
+	if (info->chip_id != RCAR_GEN2)
 		ravb_ptp_init(ndev, pdev);
 
 	/* Debug message level */
@@ -2232,7 +2262,7 @@ static int ravb_probe(struct platform_device *pdev)
 			  priv->desc_bat_dma);
 
 	/* Stop PTP Clock driver */
-	if (chip_id != RCAR_GEN2)
+	if (info->chip_id != RCAR_GEN2)
 		ravb_ptp_stop(ndev);
 out_disable_refclk:
 	clk_disable_unprepare(priv->refclk);
-- 
2.17.1

Re: [PATCH net-next 05/18] ravb: Replace chip type with a structure for driver data

[Sergei Shtylyov]

Hello!

On 7/22/21 5:13 PM, Biju Das wrote:

> The DMAC and EMAC blocks of Gigabit Ethernet IP is almost similar to
> Ethernet AVB. With few changes in driver we can support both the IP.
> 
> This patch is in preparation for supporting the same by replacing chip
> type by a structure with values, feature bits and function pointers.
> 
> Currently only values is added to structure and later patches will add
> features and function pointers.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb.h      | 14 +++++
>  drivers/net/ethernet/renesas/ravb_main.c | 76 +++++++++++++++++-------
>  2 files changed, 67 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 80e62ca2e3d3..0ed21262f26b 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -988,6 +988,18 @@ enum ravb_chip_id {
>  	RCAR_GEN3,
>  };
>  
> +struct ravb_drv_data {

   I'd rather suggest *struct* ravb_hw_info... This is hardly a driver data, more like
hwrdware's one. :-)

> +	netdev_features_t net_features;
> +	netdev_features_t net_hw_features;
> +	const char (*gstrings_stats)[ETH_GSTRING_LEN];
> +	size_t gstrings_size;
> +	size_t stats_len;
> +	u32 num_gstat_queue;
> +	size_t skb_sz;
> +	u8 num_tx_desc;
> +	enum ravb_chip_id chip_id;

   Mhm, I'd expect that chip_id is no longer needed with the feature structs... 

[...]
> @@ -1040,6 +1052,8 @@ struct ravb_private {
>  	unsigned txcidm:1;		/* TX Clock Internal Delay Mode */
>  	unsigned rgmii_override:1;	/* Deprecated rgmii-*id behavior */
>  	int num_tx_desc;		/* TX descriptors per packet */
> +
> +	const struct ravb_drv_data *info;

   So data or info? :-)

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 805397088850..84ebd6fef711 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -1176,9 +1179,12 @@ static void ravb_get_ethtool_stats(struct net_device *ndev,
>  
>  static void ravb_get_strings(struct net_device *ndev, u32 stringset, u8 *data)
>  {
> +	struct ravb_private *priv = netdev_priv(ndev);
> +	const struct ravb_drv_data *info = priv->info;
> +
>  	switch (stringset) {
>  	case ETH_SS_STATS:
> -		memcpy(data, ravb_gstrings_stats, sizeof(ravb_gstrings_stats));
> +		memcpy(data, info->gstrings_stats, info->gstrings_size);
>  		break;
>  	}
>  }
> @@ -1924,12 +1930,36 @@ static int ravb_mdio_release(struct ravb_private *priv)
>  	return 0;
>  }
>  
> +static const struct ravb_drv_data ravb_gen3_data = {
> +	.net_features = NETIF_F_RXCSUM,
> +	.net_hw_features = NETIF_F_RXCSUM,
> +	.gstrings_stats = ravb_gstrings_stats,
> +	.gstrings_size = sizeof(ravb_gstrings_stats),
> +	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
> +	.num_gstat_queue = NUM_RX_QUEUE,
> +	.skb_sz = RX_BUF_SZ + RAVB_ALIGN - 1,
> +	.num_tx_desc = NUM_TX_DESC_GEN3,
> +	.chip_id = RCAR_GEN3,
> +};
> +
> +static const struct ravb_drv_data ravb_gen2_data = {
> +	.net_features = NETIF_F_RXCSUM,
> +	.net_hw_features = NETIF_F_RXCSUM,

   Mhm, why have the fields that don't change from SoC to SoC anyway?
I do think they should be added when a new SoC support is added...

> +	.gstrings_stats = ravb_gstrings_stats,
> +	.gstrings_size = sizeof(ravb_gstrings_stats),
> +	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),

   Same question...

> +	.num_gstat_queue = NUM_RX_QUEUE,
> +	.skb_sz = RX_BUF_SZ + RAVB_ALIGN - 1,

   Again why?

> +	.num_tx_desc = NUM_TX_DESC_GEN2,
> +	.chip_id = RCAR_GEN2,
> +};
> +
[...]
> @@ -2052,15 +2082,15 @@ static int ravb_probe(struct platform_device *pdev)
>  	if (!ndev)
>  		return -ENOMEM;
>  
> -	ndev->features = NETIF_F_RXCSUM;
> -	ndev->hw_features = NETIF_F_RXCSUM;
> +	info = of_device_get_match_data(&pdev->dev);
> +
> +	ndev->features = info->net_features;
> +	ndev->hw_features = info->net_hw_features;
>  
>  	pm_runtime_enable(&pdev->dev);
>  	pm_runtime_get_sync(&pdev->dev);
>  
> -	chip_id = (enum ravb_chip_id)of_device_get_match_data(&pdev->dev);
> -
> -	if (chip_id == RCAR_GEN3)
> +	if (info->chip_id == RCAR_GEN3)

   Ugh...

>  		irq = platform_get_irq_byname(pdev, "ch22");
>  	else
>  		irq = platform_get_irq(pdev, 0);
[...]
> @@ -2099,7 +2130,7 @@ static int ravb_probe(struct platform_device *pdev)
>  	priv->avb_link_active_low =
>  		of_property_read_bool(np, "renesas,ether-link-active-low");
>  
> -	if (chip_id == RCAR_GEN3) {
> +	if (info->chip_id == RCAR_GEN3) {

   Ugh...

>  		irq = platform_get_irq_byname(pdev, "ch24");
>  		if (irq < 0) {
>  			error = irq;
[...]
> @@ -2184,7 +2214,7 @@ static int ravb_probe(struct platform_device *pdev)
>  	INIT_LIST_HEAD(&priv->ts_skb_list);
>  
>  	/* Initialise PTP Clock driver */
> -	if (chip_id != RCAR_GEN2)
> +	if (info->chip_id != RCAR_GEN2)
>  		ravb_ptp_init(ndev, pdev);

   Ugh...

>  
>  	/* Debug message level */
> @@ -2232,7 +2262,7 @@ static int ravb_probe(struct platform_device *pdev)
>  			  priv->desc_bat_dma);
>  
>  	/* Stop PTP Clock driver */
> -	if (chip_id != RCAR_GEN2)
> +	if (info->chip_id != RCAR_GEN2)
>  		ravb_ptp_stop(ndev);

  Ugh...


>  out_disable_refclk:
>  	clk_disable_unprepare(priv->refclk);

MBR, Sergei

RE: [PATCH net-next 05/18] ravb: Replace chip type with a structure for driver data

[Biju Das]

Hi Sergei,

Thanks for the feedback.

> Biju Das <biju.das@bp.renesas.com>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Subject: Re: [PATCH net-next 05/18] ravb: Replace chip type with a
> structure for driver data
> 
> Hello!
> 
> On 7/22/21 5:13 PM, Biju Das wrote:
> 
> > The DMAC and EMAC blocks of Gigabit Ethernet IP is almost similar to
> > Ethernet AVB. With few changes in driver we can support both the IP.
> >
> > This patch is in preparation for supporting the same by replacing chip
> > type by a structure with values, feature bits and function pointers.
> >
> > Currently only values is added to structure and later patches will add
> > features and function pointers.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/net/ethernet/renesas/ravb.h      | 14 +++++
> >  drivers/net/ethernet/renesas/ravb_main.c | 76
> > +++++++++++++++++-------
> >  2 files changed, 67 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb.h
> > b/drivers/net/ethernet/renesas/ravb.h
> > index 80e62ca2e3d3..0ed21262f26b 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> > @@ -988,6 +988,18 @@ enum ravb_chip_id {
> >  	RCAR_GEN3,
> >  };
> >
> > +struct ravb_drv_data {
> 
>    I'd rather suggest *struct* ravb_hw_info... This is hardly a driver
> data, more like hwrdware's one. :-)

OK.

> 
> > +	netdev_features_t net_features;
> > +	netdev_features_t net_hw_features;
> > +	const char (*gstrings_stats)[ETH_GSTRING_LEN];
> > +	size_t gstrings_size;
> > +	size_t stats_len;
> > +	u32 num_gstat_queue;
> > +	size_t skb_sz;
> > +	u8 num_tx_desc;
> > +	enum ravb_chip_id chip_id;
> 
>    Mhm, I'd expect that chip_id is no longer needed with the feature
> structs...

Yes,If you see the subsequent 3 patches, chip_id usage is completely removed from
ravb_main.c. but it is still required for ravb_ptp.c. Please let me know do you want
me take out from there as well. Then as part of [1], I can take out chipid completely.

[1]
https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210722141351.13668-7-biju.das.jz@bp.renesas.com/


> 
> [...]
> > @@ -1040,6 +1052,8 @@ struct ravb_private {
> >  	unsigned txcidm:1;		/* TX Clock Internal Delay Mode */
> >  	unsigned rgmii_override:1;	/* Deprecated rgmii-*id behavior */
> >  	int num_tx_desc;		/* TX descriptors per packet */
> > +
> > +	const struct ravb_drv_data *info;
> 
>    So data or info? :-)

As we are going to use ravb_hw_info, it will be info.

> 
> [...]
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index 805397088850..84ebd6fef711 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
> > @@ -1176,9 +1179,12 @@ static void ravb_get_ethtool_stats(struct
> > net_device *ndev,
> >
> >  static void ravb_get_strings(struct net_device *ndev, u32 stringset,
> > u8 *data)  {
> > +	struct ravb_private *priv = netdev_priv(ndev);
> > +	const struct ravb_drv_data *info = priv->info;
> > +
> >  	switch (stringset) {
> >  	case ETH_SS_STATS:
> > -		memcpy(data, ravb_gstrings_stats,
> sizeof(ravb_gstrings_stats));
> > +		memcpy(data, info->gstrings_stats, info->gstrings_size);
> >  		break;
> >  	}
> >  }
> > @@ -1924,12 +1930,36 @@ static int ravb_mdio_release(struct ravb_private
> *priv)
> >  	return 0;
> >  }
> >
> > +static const struct ravb_drv_data ravb_gen3_data = {
> > +	.net_features = NETIF_F_RXCSUM,
> > +	.net_hw_features = NETIF_F_RXCSUM,
> > +	.gstrings_stats = ravb_gstrings_stats,
> > +	.gstrings_size = sizeof(ravb_gstrings_stats),
> > +	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
> > +	.num_gstat_queue = NUM_RX_QUEUE,
> > +	.skb_sz = RX_BUF_SZ + RAVB_ALIGN - 1,
> > +	.num_tx_desc = NUM_TX_DESC_GEN3,
> > +	.chip_id = RCAR_GEN3,
> > +};
> > +
> > +static const struct ravb_drv_data ravb_gen2_data = {
> > +	.net_features = NETIF_F_RXCSUM,
> > +	.net_hw_features = NETIF_F_RXCSUM,
> 
>    Mhm, why have the fields that don't change from SoC to SoC anyway?
> I do think they should be added when a new SoC support is added...

This is a preparation patch for supporting RZ/G2L and there is a difference w.r.to RZ/G2L
See [2]. That is the reason to isolate the new SoC changes w.r.to existing one it is added
Here. Andrew also suggested to make smaller changes.

[2]
https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210722141351.13668-18-biju.das.jz@bp.renesas.com/

> 
> > +	.gstrings_stats = ravb_gstrings_stats,
> > +	.gstrings_size = sizeof(ravb_gstrings_stats),
> > +	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
> 
>    Same question...

Same as above.

> > +	.num_gstat_queue = NUM_RX_QUEUE,
> > +	.skb_sz = RX_BUF_SZ + RAVB_ALIGN - 1,
> 
>    Again why?

Same as above.

> 
> > +	.num_tx_desc = NUM_TX_DESC_GEN2,
> > +	.chip_id = RCAR_GEN2,
> > +};
> > +
> [...]
> > @@ -2052,15 +2082,15 @@ static int ravb_probe(struct platform_device
> *pdev)
> >  	if (!ndev)
> >  		return -ENOMEM;
> >
> > -	ndev->features = NETIF_F_RXCSUM;
> > -	ndev->hw_features = NETIF_F_RXCSUM;
> > +	info = of_device_get_match_data(&pdev->dev);
> > +
> > +	ndev->features = info->net_features;
> > +	ndev->hw_features = info->net_hw_features;
> >
> >  	pm_runtime_enable(&pdev->dev);
> >  	pm_runtime_get_sync(&pdev->dev);
> >
> > -	chip_id = (enum ravb_chip_id)of_device_get_match_data(&pdev->dev);
> > -
> > -	if (chip_id == RCAR_GEN3)
> > +	if (info->chip_id == RCAR_GEN3)
> 
>    Ugh...
This chip id is replaced in subsequent patch [3]

[3]
https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210722141351.13668-8-biju.das.jz@bp.renesas.com/

> 
> >  		irq = platform_get_irq_byname(pdev, "ch22");
> >  	else
> >  		irq = platform_get_irq(pdev, 0);
> [...]
> > @@ -2099,7 +2130,7 @@ static int ravb_probe(struct platform_device
> *pdev)
> >  	priv->avb_link_active_low =
> >  		of_property_read_bool(np, "renesas,ether-link-active-low");
> >
> > -	if (chip_id == RCAR_GEN3) {
> > +	if (info->chip_id == RCAR_GEN3) {
> 
>    Ugh...
Same as above.

> 
> >  		irq = platform_get_irq_byname(pdev, "ch24");
> >  		if (irq < 0) {
> >  			error = irq;
> [...]
> > @@ -2184,7 +2214,7 @@ static int ravb_probe(struct platform_device
> *pdev)
> >  	INIT_LIST_HEAD(&priv->ts_skb_list);
> >
> >  	/* Initialise PTP Clock driver */
> > -	if (chip_id != RCAR_GEN2)
> > +	if (info->chip_id != RCAR_GEN2)
> >  		ravb_ptp_init(ndev, pdev);
> 
>    Ugh...
Same as above.
> 
> >
> >  	/* Debug message level */
> > @@ -2232,7 +2262,7 @@ static int ravb_probe(struct platform_device
> *pdev)
> >  			  priv->desc_bat_dma);
> >
> >  	/* Stop PTP Clock driver */
> > -	if (chip_id != RCAR_GEN2)
> > +	if (info->chip_id != RCAR_GEN2)
> >  		ravb_ptp_stop(ndev);
> 
>   Ugh...

Same as above.

Regards,
Biju

> 
> 
> >  out_disable_refclk:
> >  	clk_disable_unprepare(priv->refclk);
> 
> MBR, Sergei

[PATCH net-next 06/18] ravb: Factorise ptp feature

[Biju Das]

Gptp is active in CONFIG mode for R-Car Gen3, where as it is not
active in CONFIG mode for R-Car Gen2. Add feature bits to handle
both cases.

RZ/G2L does not support ptp feature. Factorise ptp feature
specific to R-Car.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  1 +
 drivers/net/ethernet/renesas/ravb_main.c | 81 ++++++++++++++++--------
 2 files changed, 56 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 0ed21262f26b..a474ed68db22 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -998,6 +998,7 @@ struct ravb_drv_data {
 	size_t skb_sz;
 	u8 num_tx_desc;
 	enum ravb_chip_id chip_id;
+	u32 features;
 };
 
 struct ravb_private {
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 84ebd6fef711..e966b76df32c 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -40,6 +40,14 @@
 		 NETIF_MSG_RX_ERR | \
 		 NETIF_MSG_TX_ERR)
 
+#define RAVB_PTP_CONFIG_ACTIVE		BIT(0)
+#define RAVB_PTP_CONFIG_INACTIVE	BIT(1)
+
+#define RAVB_PTP	(RAVB_PTP_CONFIG_ACTIVE | RAVB_PTP_CONFIG_INACTIVE)
+
+#define RAVB_RCAR_GEN3_FEATURES	RAVB_PTP_CONFIG_ACTIVE
+#define RAVB_RCAR_GEN2_FEATURES	RAVB_PTP_CONFIG_INACTIVE
+
 static const char *ravb_rx_irqs[NUM_RX_QUEUE] = {
 	"ch0", /* RAVB_BE */
 	"ch1", /* RAVB_NC */
@@ -804,6 +812,7 @@ static irqreturn_t ravb_interrupt(int irq, void *dev_id)
 {
 	struct net_device *ndev = dev_id;
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 	irqreturn_t result = IRQ_NONE;
 	u32 iss;
 
@@ -839,7 +848,7 @@ static irqreturn_t ravb_interrupt(int irq, void *dev_id)
 	}
 
 	/* gPTP interrupt status summary */
-	if (iss & ISS_CGIS) {
+	if ((info->features & RAVB_PTP) && (iss & ISS_CGIS)) {
 		ravb_ptp_interrupt(ndev);
 		result = IRQ_HANDLED;
 	}
@@ -1204,6 +1213,7 @@ static int ravb_set_ringparam(struct net_device *ndev,
 			      struct ethtool_ringparam *ring)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 	int error;
 
 	if (ring->tx_pending > BE_TX_RING_MAX ||
@@ -1217,7 +1227,7 @@ static int ravb_set_ringparam(struct net_device *ndev,
 	if (netif_running(ndev)) {
 		netif_device_detach(ndev);
 		/* Stop PTP Clock driver */
-		if (priv->chip_id == RCAR_GEN2)
+		if (info->features & RAVB_PTP_CONFIG_INACTIVE)
 			ravb_ptp_stop(ndev);
 		/* Wait for DMA stopping */
 		error = ravb_stop_dma(ndev);
@@ -1249,7 +1259,7 @@ static int ravb_set_ringparam(struct net_device *ndev,
 		ravb_emac_init(ndev);
 
 		/* Initialise PTP Clock driver */
-		if (priv->chip_id == RCAR_GEN2)
+		if (info->features & RAVB_PTP_CONFIG_INACTIVE)
 			ravb_ptp_init(ndev, priv->pdev);
 
 		netif_device_attach(ndev);
@@ -1262,6 +1272,7 @@ static int ravb_get_ts_info(struct net_device *ndev,
 			    struct ethtool_ts_info *info)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *data = priv->info;
 
 	info->so_timestamping =
 		SOF_TIMESTAMPING_TX_SOFTWARE |
@@ -1275,7 +1286,8 @@ static int ravb_get_ts_info(struct net_device *ndev,
 		(1 << HWTSTAMP_FILTER_NONE) |
 		(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
 		(1 << HWTSTAMP_FILTER_ALL);
-	info->phc_index = ptp_clock_index(priv->ptp.clock);
+	if (data->features & RAVB_PTP)
+		info->phc_index = ptp_clock_index(priv->ptp.clock);
 
 	return 0;
 }
@@ -1340,6 +1352,7 @@ static inline int ravb_hook_irq(unsigned int irq, irq_handler_t handler,
 static int ravb_open(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 	struct platform_device *pdev = priv->pdev;
 	struct device *dev = &pdev->dev;
 	int error;
@@ -1388,7 +1401,7 @@ static int ravb_open(struct net_device *ndev)
 	ravb_emac_init(ndev);
 
 	/* Initialise PTP Clock driver */
-	if (priv->chip_id == RCAR_GEN2)
+	if (info->features & RAVB_PTP_CONFIG_INACTIVE)
 		ravb_ptp_init(ndev, priv->pdev);
 
 	netif_tx_start_all_queues(ndev);
@@ -1402,7 +1415,7 @@ static int ravb_open(struct net_device *ndev)
 
 out_ptp_stop:
 	/* Stop PTP Clock driver */
-	if (priv->chip_id == RCAR_GEN2)
+	if (info->features & RAVB_PTP_CONFIG_INACTIVE)
 		ravb_ptp_stop(ndev);
 out_free_irq_nc_tx:
 	if (priv->chip_id == RCAR_GEN2)
@@ -1443,13 +1456,14 @@ static void ravb_tx_timeout_work(struct work_struct *work)
 {
 	struct ravb_private *priv = container_of(work, struct ravb_private,
 						 work);
+	const struct ravb_drv_data *info = priv->info;
 	struct net_device *ndev = priv->ndev;
 	int error;
 
 	netif_tx_stop_all_queues(ndev);
 
 	/* Stop PTP Clock driver */
-	if (priv->chip_id == RCAR_GEN2)
+	if (info->features & RAVB_PTP_CONFIG_INACTIVE)
 		ravb_ptp_stop(ndev);
 
 	/* Wait for DMA stopping */
@@ -1484,7 +1498,7 @@ static void ravb_tx_timeout_work(struct work_struct *work)
 
 out:
 	/* Initialise PTP Clock driver */
-	if (priv->chip_id == RCAR_GEN2)
+	if (info->features & RAVB_PTP_CONFIG_INACTIVE)
 		ravb_ptp_init(ndev, priv->pdev);
 
 	netif_tx_start_all_queues(ndev);
@@ -1681,6 +1695,7 @@ static int ravb_close(struct net_device *ndev)
 {
 	struct device_node *np = ndev->dev.parent->of_node;
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 	struct ravb_tstamp_skb *ts_skb, *ts_skb2;
 
 	netif_tx_stop_all_queues(ndev);
@@ -1691,7 +1706,7 @@ static int ravb_close(struct net_device *ndev)
 	ravb_write(ndev, 0, TIC);
 
 	/* Stop PTP Clock driver */
-	if (priv->chip_id == RCAR_GEN2)
+	if (info->features & RAVB_PTP_CONFIG_INACTIVE)
 		ravb_ptp_stop(ndev);
 
 	/* Set the config mode to stop the AVB-DMAC's processes */
@@ -1940,6 +1955,7 @@ static const struct ravb_drv_data ravb_gen3_data = {
 	.skb_sz = RX_BUF_SZ + RAVB_ALIGN - 1,
 	.num_tx_desc = NUM_TX_DESC_GEN3,
 	.chip_id = RCAR_GEN3,
+	.features = RAVB_RCAR_GEN3_FEATURES,
 };
 
 static const struct ravb_drv_data ravb_gen2_data = {
@@ -1952,6 +1968,7 @@ static const struct ravb_drv_data ravb_gen2_data = {
 	.skb_sz = RX_BUF_SZ + RAVB_ALIGN - 1,
 	.num_tx_desc = NUM_TX_DESC_GEN2,
 	.chip_id = RCAR_GEN2,
+	.features = RAVB_RCAR_GEN2_FEATURES,
 };
 
 static const struct of_device_id ravb_match_table[] = {
@@ -1992,14 +2009,20 @@ static int ravb_set_gti(struct net_device *ndev)
 static void ravb_set_config_mode(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 
-	if (priv->chip_id == RCAR_GEN2) {
+	switch (info->features & RAVB_PTP) {
+	case RAVB_PTP_CONFIG_INACTIVE:
 		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
 		/* Set CSEL value */
 		ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
-	} else {
+		break;
+	case RAVB_PTP_CONFIG_ACTIVE:
 		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG |
 			    CCC_GAC | CCC_CSEL_HPB);
+		break;
+	default:
+		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
 	}
 }
 
@@ -2182,13 +2205,15 @@ static int ravb_probe(struct platform_device *pdev)
 	/* Set AVB config mode */
 	ravb_set_config_mode(ndev);
 
-	/* Set GTI value */
-	error = ravb_set_gti(ndev);
-	if (error)
-		goto out_disable_refclk;
+	if (info->features & RAVB_PTP) {
+		/* Set GTI value */
+		error = ravb_set_gti(ndev);
+		if (error)
+			goto out_disable_refclk;
 
-	/* Request GTI loading */
-	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
+		/* Request GTI loading */
+		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
+	}
 
 	if (priv->chip_id != RCAR_GEN2) {
 		ravb_parse_delay_mode(np, ndev);
@@ -2214,7 +2239,7 @@ static int ravb_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&priv->ts_skb_list);
 
 	/* Initialise PTP Clock driver */
-	if (info->chip_id != RCAR_GEN2)
+	if (info->features & RAVB_PTP_CONFIG_ACTIVE)
 		ravb_ptp_init(ndev, pdev);
 
 	/* Debug message level */
@@ -2262,7 +2287,7 @@ static int ravb_probe(struct platform_device *pdev)
 			  priv->desc_bat_dma);
 
 	/* Stop PTP Clock driver */
-	if (info->chip_id != RCAR_GEN2)
+	if (info->features & RAVB_PTP_CONFIG_ACTIVE)
 		ravb_ptp_stop(ndev);
 out_disable_refclk:
 	clk_disable_unprepare(priv->refclk);
@@ -2278,9 +2303,10 @@ static int ravb_remove(struct platform_device *pdev)
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 
 	/* Stop PTP Clock driver */
-	if (priv->chip_id != RCAR_GEN2)
+	if (info->features & RAVB_PTP_CONFIG_ACTIVE)
 		ravb_ptp_stop(ndev);
 
 	clk_disable_unprepare(priv->refclk);
@@ -2363,6 +2389,7 @@ static int __maybe_unused ravb_resume(struct device *dev)
 {
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 	int ret = 0;
 
 	/* If WoL is enabled set reset mode to rearm the WoL logic */
@@ -2377,13 +2404,15 @@ static int __maybe_unused ravb_resume(struct device *dev)
 	/* Set AVB config mode */
 	ravb_set_config_mode(ndev);
 
-	/* Set GTI value */
-	ret = ravb_set_gti(ndev);
-	if (ret)
-		return ret;
+	if (info->features & RAVB_PTP) {
+		/* Set GTI value */
+		ret = ravb_set_gti(ndev);
+		if (ret)
+			return ret;
 
-	/* Request GTI loading */
-	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
+		/* Request GTI loading */
+		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
+	}
 
 	if (priv->chip_id != RCAR_GEN2)
 		ravb_set_delay_mode(ndev);
-- 
2.17.1

Re: [PATCH net-next 06/18] ravb: Factorise ptp feature

[Sergei Shtylyov]

HEllo!

On 7/22/21 5:13 PM, Biju Das wrote:

> Gptp is active in CONFIG mode for R-Car Gen3, where as it is not

   It's gPTP, the manuals say. :-)

> active in CONFIG mode for R-Car Gen2. Add feature bits to handle
> both cases.

   I have no idea why this single diff requires 2 fetaure bits....

> RZ/G2L does not support ptp feature.

   Ah, that explains it. :-)
   It doesn't explain why we should bother with the 2nd bit in the same patch tho...

> Factorise ptp feature
> specific to R-Car.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb.h      |  1 +
>  drivers/net/ethernet/renesas/ravb_main.c | 81 ++++++++++++++++--------
>  2 files changed, 56 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 0ed21262f26b..a474ed68db22 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -998,6 +998,7 @@ struct ravb_drv_data {
>  	size_t skb_sz;
>  	u8 num_tx_desc;
>  	enum ravb_chip_id chip_id;
> +	u32 features;

   You didn't like bitfelds (in sh_eth) so much? :-)

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 84ebd6fef711..e966b76df32c 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -40,6 +40,14 @@
>  		 NETIF_MSG_RX_ERR | \
>  		 NETIF_MSG_TX_ERR)
>  
> +#define RAVB_PTP_CONFIG_ACTIVE		BIT(0)
> +#define RAVB_PTP_CONFIG_INACTIVE	BIT(1)

   If both bits are 0, it means GbEth?

> +
> +#define RAVB_PTP	(RAVB_PTP_CONFIG_ACTIVE | RAVB_PTP_CONFIG_INACTIVE)

   Hm?

> +
> +#define RAVB_RCAR_GEN3_FEATURES	RAVB_PTP_CONFIG_ACTIVE
> +#define RAVB_RCAR_GEN2_FEATURES	RAVB_PTP_CONFIG_INACTIVE

   Not sure whtehr these are necessary...

[...]
>  	}
>  
>  	/* gPTP interrupt status summary */
> -	if (iss & ISS_CGIS) {
> +	if ((info->features & RAVB_PTP) && (iss & ISS_CGIS)) {

   This is not a transparent change -- the fearture check came fromn nownere...

>  		ravb_ptp_interrupt(ndev);
>  		result = IRQ_HANDLED;
>  	}
[...]
> @@ -1275,7 +1286,8 @@ static int ravb_get_ts_info(struct net_device *ndev,
>  		(1 << HWTSTAMP_FILTER_NONE) |
>  		(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
>  		(1 << HWTSTAMP_FILTER_ALL);
> -	info->phc_index = ptp_clock_index(priv->ptp.clock);
> +	if (data->features & RAVB_PTP)

   Again, not transparent...

> +		info->phc_index = ptp_clock_index(priv->ptp.clock);
>  
>  	return 0;
>  }
[...]
> @@ -1992,14 +2009,20 @@ static int ravb_set_gti(struct net_device *ndev)
>  static void ravb_set_config_mode(struct net_device *ndev)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
> +	const struct ravb_drv_data *info = priv->info;
>  
> -	if (priv->chip_id == RCAR_GEN2) {
> +	switch (info->features & RAVB_PTP) {
> +	case RAVB_PTP_CONFIG_INACTIVE:
>  		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
>  		/* Set CSEL value */
>  		ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
> -	} else {
> +		break;
> +	case RAVB_PTP_CONFIG_ACTIVE:
>  		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG |
>  			    CCC_GAC | CCC_CSEL_HPB);
> +		break;
> +	default:
> +		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);

   Not trasparent again...

[...]
> @@ -2182,13 +2205,15 @@ static int ravb_probe(struct platform_device *pdev)
>  	/* Set AVB config mode */
>  	ravb_set_config_mode(ndev);
>  
> -	/* Set GTI value */
> -	error = ravb_set_gti(ndev);
> -	if (error)
> -		goto out_disable_refclk;
> +	if (info->features & RAVB_PTP) {

   Not transparent enough yet again...

> +		/* Set GTI value */
> +		error = ravb_set_gti(ndev);
> +		if (error)
> +			goto out_disable_refclk;
>  
> -	/* Request GTI loading */
> -	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> +		/* Request GTI loading */
> +		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> +	}
>  
>  	if (priv->chip_id != RCAR_GEN2) {
>  		ravb_parse_delay_mode(np, ndev);
[...]
> @@ -2377,13 +2404,15 @@ static int __maybe_unused ravb_resume(struct device *dev)
>  	/* Set AVB config mode */
>  	ravb_set_config_mode(ndev);
>  
> -	/* Set GTI value */
> -	ret = ravb_set_gti(ndev);
> -	if (ret)
> -		return ret;
> +	if (info->features & RAVB_PTP) {

   Not transparent enough again...

> +		/* Set GTI value */
> +		ret = ravb_set_gti(ndev);
> +		if (ret)
> +			return ret;
>  
> -	/* Request GTI loading */
> -	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> +		/* Request GTI loading */
> +		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> +	}
>  
>  	if (priv->chip_id != RCAR_GEN2)
>  		ravb_set_delay_mode(ndev);
> 

MBR, Sergei

RE: [PATCH net-next 06/18] ravb: Factorise ptp feature

[Biju Das]

Hi Sergei,

Thanks for the feedback.

> Subject: Re: [PATCH net-next 06/18] ravb: Factorise ptp feature
> 
> HEllo!
> 
> On 7/22/21 5:13 PM, Biju Das wrote:
> 
> > Gptp is active in CONFIG mode for R-Car Gen3, where as it is not
> 
>    It's gPTP, the manuals say. :-)

Ok.

> 
> > active in CONFIG mode for R-Car Gen2. Add feature bits to handle both
> > cases.
> 
>    I have no idea why this single diff requires 2 fetaure bits....

Basically this is a HW feature.

1) for R-Car Gen3, gPTP is active in config mode (R-Car Gen3)
2) for R-Car Gen2, gPTP is not active in config mode (R-Car Gen2)
3) RZ/G2L does not support ptp feature.

> 
> > RZ/G2L does not support ptp feature.
> 
>    Ah, that explains it. :-)
>    It doesn't explain why we should bother with the 2nd bit in the same
> patch tho...

See above it is HW feature diff between R-Car Gen3 and R-Car Gen2.

> 
> > Factorise ptp feature
> > specific to R-Car.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/net/ethernet/renesas/ravb.h      |  1 +
> >  drivers/net/ethernet/renesas/ravb_main.c | 81
> > ++++++++++++++++--------
> >  2 files changed, 56 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb.h
> > b/drivers/net/ethernet/renesas/ravb.h
> > index 0ed21262f26b..a474ed68db22 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> > @@ -998,6 +998,7 @@ struct ravb_drv_data {
> >  	size_t skb_sz;
> >  	u8 num_tx_desc;
> >  	enum ravb_chip_id chip_id;
> > +	u32 features;
> 
>    You didn't like bitfelds (in sh_eth) so much? :-)

OK. I will change to bit fields, if there is no objection.

> 
> [...]
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index 84ebd6fef711..e966b76df32c 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -40,6 +40,14 @@
> >  		 NETIF_MSG_RX_ERR | \
> >  		 NETIF_MSG_TX_ERR)
> >
> > +#define RAVB_PTP_CONFIG_ACTIVE		BIT(0)
> > +#define RAVB_PTP_CONFIG_INACTIVE	BIT(1)
> 
>    If both bits are 0, it means GbEth?

Yes that is correct. ptp is not supported in GbEth.

> 
> > +
> > +#define RAVB_PTP	(RAVB_PTP_CONFIG_ACTIVE |
> RAVB_PTP_CONFIG_INACTIVE)
> 
>    Hm?
> 
> > +
> > +#define RAVB_RCAR_GEN3_FEATURES	RAVB_PTP_CONFIG_ACTIVE
> > +#define RAVB_RCAR_GEN2_FEATURES	RAVB_PTP_CONFIG_INACTIVE
> 
>    Not sure whtehr these are necessary...

If there is no objection for using bit fields, then the above definitions not required.

> 
> [...]
> >  	}
> >
> >  	/* gPTP interrupt status summary */
> > -	if (iss & ISS_CGIS) {
> > +	if ((info->features & RAVB_PTP) && (iss & ISS_CGIS)) {
> 
>    This is not a transparent change -- the fearture check came fromn
> nownere...

I have added commit statement to make it clear, "RZ/G2L does not support ptp feature. Factorise ptp feature
specific to R-Car".

Do you see any issues with this? If needed we can factorize this portion of the code again to make 
It simpler. First patch is ptp feature for r-car differences(R-Car Gen3 and R-Car Gen2) and second patch( with "transparent" comments you have mentioned in this patch)

Please let me know.

> 
> >  		ravb_ptp_interrupt(ndev);
> >  		result = IRQ_HANDLED;
> >  	}
> [...]
> > @@ -1275,7 +1286,8 @@ static int ravb_get_ts_info(struct net_device
> *ndev,
> >  		(1 << HWTSTAMP_FILTER_NONE) |
> >  		(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
> >  		(1 << HWTSTAMP_FILTER_ALL);
> > -	info->phc_index = ptp_clock_index(priv->ptp.clock);
> > +	if (data->features & RAVB_PTP)
> 
>    Again, not transparent...

See above.

> 
> > +		info->phc_index = ptp_clock_index(priv->ptp.clock);
> >
> >  	return 0;
> >  }
> [...]
> > @@ -1992,14 +2009,20 @@ static int ravb_set_gti(struct net_device
> > *ndev)  static void ravb_set_config_mode(struct net_device *ndev)  {
> >  	struct ravb_private *priv = netdev_priv(ndev);
> > +	const struct ravb_drv_data *info = priv->info;
> >
> > -	if (priv->chip_id == RCAR_GEN2) {
> > +	switch (info->features & RAVB_PTP) {
> > +	case RAVB_PTP_CONFIG_INACTIVE:
> >  		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
> >  		/* Set CSEL value */
> >  		ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
> > -	} else {
> > +		break;
> > +	case RAVB_PTP_CONFIG_ACTIVE:
> >  		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG |
> >  			    CCC_GAC | CCC_CSEL_HPB);
> > +		break;
> > +	default:
> > +		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
>    Not trasparent again...

See above.

First Case is for R-Car Gen3 where ptp is active in config mode.
Second Case is for R-Car Gen2 where ptp is not active in config mode.
Third Case is default for RZ/G2L where ptp is not present.

Do you see any issues with this?

> 
> [...]
> > @@ -2182,13 +2205,15 @@ static int ravb_probe(struct platform_device
> *pdev)
> >  	/* Set AVB config mode */
> >  	ravb_set_config_mode(ndev);
> >
> > -	/* Set GTI value */
> > -	error = ravb_set_gti(ndev);
> > -	if (error)
> > -		goto out_disable_refclk;
> > +	if (info->features & RAVB_PTP) {
> 
>    Not transparent enough yet again...

See above.

> 
> > +		/* Set GTI value */
> > +		error = ravb_set_gti(ndev);
> > +		if (error)
> > +			goto out_disable_refclk;
> >
> > -	/* Request GTI loading */
> > -	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> > +		/* Request GTI loading */
> > +		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> > +	}
> >
> >  	if (priv->chip_id != RCAR_GEN2) {
> >  		ravb_parse_delay_mode(np, ndev);
> [...]
> > @@ -2377,13 +2404,15 @@ static int __maybe_unused ravb_resume(struct
> device *dev)
> >  	/* Set AVB config mode */
> >  	ravb_set_config_mode(ndev);
> >
> > -	/* Set GTI value */
> > -	ret = ravb_set_gti(ndev);
> > -	if (ret)
> > -		return ret;
> > +	if (info->features & RAVB_PTP) {
> 
>    Not transparent enough again...

See above.

Cheers,
Biju

Re: [PATCH net-next 06/18] ravb: Factorise ptp feature

[Andrew Lunn]

On Mon, Jul 26, 2021 at 09:01:29AM +0000, Biju Das wrote:
> Hi Sergei,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH net-next 06/18] ravb: Factorise ptp feature
> > 
> > HEllo!
> > 
> > On 7/22/21 5:13 PM, Biju Das wrote:
> > 
> > > Gptp is active in CONFIG mode for R-Car Gen3, where as it is not
> > 
> >    It's gPTP, the manuals say. :-)
> 
> Ok.
> 
> > 
> > > active in CONFIG mode for R-Car Gen2. Add feature bits to handle both
> > > cases.
> > 
> >    I have no idea why this single diff requires 2 fetaure bits....
> 
> Basically this is a HW feature.
> 
> 1) for R-Car Gen3, gPTP is active in config mode (R-Car Gen3)
> 2) for R-Car Gen2, gPTP is not active in config mode (R-Car Gen2)
> 3) RZ/G2L does not support ptp feature.

This is useful information to put in he commit message. The commit
message should answer the question "Why is this change being made?",
since what is being changed should be obvious from the patch.

      Andrew

RE: [PATCH net-next 06/18] ravb: Factorise ptp feature

[Biju Das]

Hi Andrew,

Thanks for the feedback.

> Subject: Re: [PATCH net-next 06/18] ravb: Factorise ptp feature
> 
> On Mon, Jul 26, 2021 at 09:01:29AM +0000, Biju Das wrote:
> > Hi Sergei,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH net-next 06/18] ravb: Factorise ptp feature
> > >
> > > HEllo!
> > >
> > > On 7/22/21 5:13 PM, Biju Das wrote:
> > >
> > > > Gptp is active in CONFIG mode for R-Car Gen3, where as it is not
> > >
> > >    It's gPTP, the manuals say. :-)
> >
> > Ok.
> >
> > >
> > > > active in CONFIG mode for R-Car Gen2. Add feature bits to handle
> > > > both cases.
> > >
> > >    I have no idea why this single diff requires 2 fetaure bits....
> >
> > Basically this is a HW feature.
> >
> > 1) for R-Car Gen3, gPTP is active in config mode (R-Car Gen3)
> > 2) for R-Car Gen2, gPTP is not active in config mode (R-Car Gen2)
> > 3) RZ/G2L does not support ptp feature.
> 
> This is useful information to put in he commit message. The commit message
> should answer the question "Why is this change being made?", since what is
> being changed should be obvious from the patch.

OK. will add this in commit description.

Regards,
Biju

> 
>       Andrew

[PATCH net-next 07/18] ravb: Add features specific to R-Car Gen3

[Biju Das]

Multiple irqs, internal delay and tx drop counter is present only
in R-Car Gen3. Add feature bits to support the same.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 36 ++++++++++++++++--------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index e966b76df32c..b3c99f974632 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -42,10 +42,18 @@
 
 #define RAVB_PTP_CONFIG_ACTIVE		BIT(0)
 #define RAVB_PTP_CONFIG_INACTIVE	BIT(1)
+#define RAVB_MULTI_IRQS			BIT(2)
+#define RAVB_INTERNAL_DELAY		BIT(3)
+#define RAVB_TX_DROP_COUNTER		BIT(4)
 
 #define RAVB_PTP	(RAVB_PTP_CONFIG_ACTIVE | RAVB_PTP_CONFIG_INACTIVE)
 
-#define RAVB_RCAR_GEN3_FEATURES	RAVB_PTP_CONFIG_ACTIVE
+#define RAVB_RCAR_GEN3_FEATURES \
+		(RAVB_PTP_CONFIG_ACTIVE		| \
+		 RAVB_MULTI_IRQS		| \
+		 RAVB_INTERNAL_DELAY		| \
+		 RAVB_TX_DROP_COUNTER)
+
 #define RAVB_RCAR_GEN2_FEATURES	RAVB_PTP_CONFIG_INACTIVE
 
 static const char *ravb_rx_irqs[NUM_RX_QUEUE] = {
@@ -435,6 +443,7 @@ static void ravb_emac_init(struct net_device *ndev)
 static int ravb_dmac_init(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 	int error;
 
 	/* Set CONFIG mode */
@@ -466,7 +475,7 @@ static int ravb_dmac_init(struct net_device *ndev)
 	ravb_write(ndev, TCCR_TFEN, TCCR);
 
 	/* Interrupt init: */
-	if (priv->chip_id == RCAR_GEN3) {
+	if (info->features & RAVB_MULTI_IRQS) {
 		/* Clear DIL.DPLx */
 		ravb_write(ndev, 0, DIL);
 		/* Set queue specific interrupt */
@@ -767,6 +776,7 @@ static void ravb_error_interrupt(struct net_device *ndev)
 static bool ravb_queue_interrupt(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 	u32 ris0 = ravb_read(ndev, RIS0);
 	u32 ric0 = ravb_read(ndev, RIC0);
 	u32 tis  = ravb_read(ndev, TIS);
@@ -775,7 +785,7 @@ static bool ravb_queue_interrupt(struct net_device *ndev, int q)
 	if (((ris0 & ric0) & BIT(q)) || ((tis  & tic)  & BIT(q))) {
 		if (napi_schedule_prep(&priv->napi[q])) {
 			/* Mask RX and TX interrupts */
-			if (priv->chip_id == RCAR_GEN2) {
+			if (!(info->features & RAVB_MULTI_IRQS)) {
 				ravb_write(ndev, ric0 & ~BIT(q), RIC0);
 				ravb_write(ndev, tic & ~BIT(q), TIC);
 			} else {
@@ -919,6 +929,7 @@ static int ravb_poll(struct napi_struct *napi, int budget)
 {
 	struct net_device *ndev = napi->dev;
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 	unsigned long flags;
 	int q = napi - priv->napi;
 	int mask = BIT(q);
@@ -942,7 +953,7 @@ static int ravb_poll(struct napi_struct *napi, int budget)
 
 	/* Re-enable RX/TX interrupts */
 	spin_lock_irqsave(&priv->lock, flags);
-	if (priv->chip_id == RCAR_GEN2) {
+	if (!(info->features & RAVB_MULTI_IRQS)) {
 		ravb_modify(ndev, RIC0, mask, mask);
 		ravb_modify(ndev, TIC,  mask, mask);
 	} else {
@@ -1360,7 +1371,7 @@ static int ravb_open(struct net_device *ndev)
 	napi_enable(&priv->napi[RAVB_BE]);
 	napi_enable(&priv->napi[RAVB_NC]);
 
-	if (priv->chip_id == RCAR_GEN2) {
+	if (!(info->features & RAVB_MULTI_IRQS)) {
 		error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED,
 				    ndev->name, ndev);
 		if (error) {
@@ -1418,7 +1429,7 @@ static int ravb_open(struct net_device *ndev)
 	if (info->features & RAVB_PTP_CONFIG_INACTIVE)
 		ravb_ptp_stop(ndev);
 out_free_irq_nc_tx:
-	if (priv->chip_id == RCAR_GEN2)
+	if (!(info->features & RAVB_MULTI_IRQS))
 		goto out_free_irq;
 	free_irq(priv->tx_irqs[RAVB_NC], ndev);
 out_free_irq_nc_rx:
@@ -1648,13 +1659,14 @@ static u16 ravb_select_queue(struct net_device *ndev, struct sk_buff *skb,
 static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 	struct net_device_stats *nstats, *stats0, *stats1;
 
 	nstats = &ndev->stats;
 	stats0 = &priv->stats[RAVB_BE];
 	stats1 = &priv->stats[RAVB_NC];
 
-	if (priv->chip_id == RCAR_GEN3) {
+	if (info->features & RAVB_TX_DROP_COUNTER) {
 		nstats->tx_dropped += ravb_read(ndev, TROCR);
 		ravb_write(ndev, 0, TROCR);	/* (write clear) */
 	}
@@ -1729,7 +1741,7 @@ static int ravb_close(struct net_device *ndev)
 			of_phy_deregister_fixed_link(np);
 	}
 
-	if (priv->chip_id != RCAR_GEN2) {
+	if (info->features & RAVB_MULTI_IRQS) {
 		free_irq(priv->tx_irqs[RAVB_NC], ndev);
 		free_irq(priv->rx_irqs[RAVB_NC], ndev);
 		free_irq(priv->tx_irqs[RAVB_BE], ndev);
@@ -2113,7 +2125,7 @@ static int ravb_probe(struct platform_device *pdev)
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 
-	if (info->chip_id == RCAR_GEN3)
+	if (info->features & RAVB_MULTI_IRQS)
 		irq = platform_get_irq_byname(pdev, "ch22");
 	else
 		irq = platform_get_irq(pdev, 0);
@@ -2153,7 +2165,7 @@ static int ravb_probe(struct platform_device *pdev)
 	priv->avb_link_active_low =
 		of_property_read_bool(np, "renesas,ether-link-active-low");
 
-	if (info->chip_id == RCAR_GEN3) {
+	if (info->features & RAVB_MULTI_IRQS) {
 		irq = platform_get_irq_byname(pdev, "ch24");
 		if (irq < 0) {
 			error = irq;
@@ -2215,7 +2227,7 @@ static int ravb_probe(struct platform_device *pdev)
 		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
 	}
 
-	if (priv->chip_id != RCAR_GEN2) {
+	if (info->features & RAVB_INTERNAL_DELAY) {
 		ravb_parse_delay_mode(np, ndev);
 		ravb_set_delay_mode(ndev);
 	}
@@ -2414,7 +2426,7 @@ static int __maybe_unused ravb_resume(struct device *dev)
 		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
 	}
 
-	if (priv->chip_id != RCAR_GEN2)
+	if (info->features & RAVB_INTERNAL_DELAY)
 		ravb_set_delay_mode(ndev);
 
 	/* Restore descriptor base address table */
-- 
2.17.1

[PATCH net-next 08/18] ravb: Add R-Car common features

[Biju Das]

The below features are supported by both R-Car Gen2 and Gen3.

1) magic packet detection
2) multiple TSRQ support
3) extended descriptor in rx
4) No half duplex support
5) override mtu change

Add features bits to support the same.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 110 +++++++++++++++--------
 1 file changed, 71 insertions(+), 39 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index b3c99f974632..4ef2565534d2 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -45,16 +45,30 @@
 #define RAVB_MULTI_IRQS			BIT(2)
 #define RAVB_INTERNAL_DELAY		BIT(3)
 #define RAVB_TX_DROP_COUNTER		BIT(4)
+#define RAVB_MAGIC			BIT(5)
+#define RAVB_MULTI_TSRQ			BIT(6)
+#define RAVB_NO_HALF_DUPLEX		BIT(7)
+#define RAVB_OVERRIDE_MTU_CHANGE	BIT(8)
+#define RAVB_EX_RX_DESC			BIT(9)
 
 #define RAVB_PTP	(RAVB_PTP_CONFIG_ACTIVE | RAVB_PTP_CONFIG_INACTIVE)
+#define RAVB_RCAR_COMMON \
+		(RAVB_MAGIC			| \
+		 RAVB_MULTI_TSRQ		| \
+		 RAVB_NO_HALF_DUPLEX		| \
+		 RAVB_OVERRIDE_MTU_CHANGE	| \
+		 RAVB_EX_RX_DESC)
 
 #define RAVB_RCAR_GEN3_FEATURES \
 		(RAVB_PTP_CONFIG_ACTIVE		| \
 		 RAVB_MULTI_IRQS		| \
 		 RAVB_INTERNAL_DELAY		| \
-		 RAVB_TX_DROP_COUNTER)
+		 RAVB_TX_DROP_COUNTER		| \
+		 RAVB_RCAR_COMMON)
 
-#define RAVB_RCAR_GEN2_FEATURES	RAVB_PTP_CONFIG_INACTIVE
+#define RAVB_RCAR_GEN2_FEATURES \
+		(RAVB_PTP_CONFIG_INACTIVE	| \
+		 RAVB_RCAR_COMMON)
 
 static const char *ravb_rx_irqs[NUM_RX_QUEUE] = {
 	"ch0", /* RAVB_BE */
@@ -680,11 +694,14 @@ static void ravb_rcv_snd_enable(struct net_device *ndev)
 /* function for waiting dma process finished */
 static int ravb_stop_dma(struct net_device *ndev)
 {
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 	int error;
 
 	/* Wait for stopping the hardware TX process */
-	error = ravb_wait(ndev, TCCR,
-			  TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3, 0);
+	if (info->features & RAVB_MULTI_TSRQ)
+		error = ravb_wait(ndev, TCCR,
+				  TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3, 0);
 	if (error)
 		return error;
 
@@ -709,12 +726,13 @@ static int ravb_stop_dma(struct net_device *ndev)
 static void ravb_emac_interrupt_unlocked(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 	u32 ecsr, psr;
 
 	ecsr = ravb_read(ndev, ECSR);
 	ravb_write(ndev, ecsr, ECSR);	/* clear interrupt */
 
-	if (ecsr & ECSR_MPD)
+	if ((info->features & RAVB_MAGIC) && (ecsr & ECSR_MPD))
 		pm_wakeup_event(&priv->pdev->dev, 0);
 	if (ecsr & ECSR_ICD)
 		ndev->stats.tx_carrier_errors++;
@@ -808,11 +826,14 @@ static bool ravb_queue_interrupt(struct net_device *ndev, int q)
 
 static bool ravb_timestamp_interrupt(struct net_device *ndev)
 {
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 	u32 tis = ravb_read(ndev, TIS);
 
 	if (tis & TIS_TFUF) {
 		ravb_write(ndev, ~(TIS_TFUF | TIS_RESERVED), TIS);
-		ravb_get_tx_tstamp(ndev);
+		if (info->features & RAVB_EX_RX_DESC)
+			ravb_get_tx_tstamp(ndev);
 		return true;
 	}
 	return false;
@@ -1024,6 +1045,7 @@ static int ravb_phy_init(struct net_device *ndev)
 {
 	struct device_node *np = ndev->dev.parent->of_node;
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 	struct phy_device *phydev;
 	struct device_node *pn;
 	phy_interface_t iface;
@@ -1069,15 +1091,17 @@ static int ravb_phy_init(struct net_device *ndev)
 		netdev_info(ndev, "limited PHY to 100Mbit/s\n");
 	}
 
-	/* 10BASE, Pause and Asym Pause is not supported */
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Pause_BIT);
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Asym_Pause_BIT);
+	if (info->features & RAVB_NO_HALF_DUPLEX) {
+		/* 10BASE, Pause and Asym Pause is not supported */
+		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
+		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
+		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Pause_BIT);
+		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Asym_Pause_BIT);
 
-	/* Half Duplex is not supported */
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
+		/* Half Duplex is not supported */
+		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
+		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
+	}
 
 	phy_attached_info(phydev);
 
@@ -1314,8 +1338,9 @@ static void ravb_get_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
 static int ravb_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 
-	if (wol->wolopts & ~WAKE_MAGIC)
+	if ((wol->wolopts & ~WAKE_MAGIC) || (!(info->features & RAVB_MAGIC)))
 		return -EOPNOTSUPP;
 
 	priv->wol_enabled = !!(wol->wolopts & WAKE_MAGIC);
@@ -1519,6 +1544,7 @@ static void ravb_tx_timeout_work(struct work_struct *work)
 static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 	int num_tx_desc = priv->num_tx_desc;
 	u16 q = skb_get_queue_mapping(skb);
 	struct ravb_tstamp_skb *ts_skb;
@@ -1595,28 +1621,30 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	desc->dptr = cpu_to_le32(dma_addr);
 
 	/* TX timestamp required */
-	if (q == RAVB_NC) {
-		ts_skb = kmalloc(sizeof(*ts_skb), GFP_ATOMIC);
-		if (!ts_skb) {
-			if (num_tx_desc > 1) {
-				desc--;
-				dma_unmap_single(ndev->dev.parent, dma_addr,
-						 len, DMA_TO_DEVICE);
+	if (info->features & RAVB_EX_RX_DESC) {
+		if (q == RAVB_NC) {
+			ts_skb = kmalloc(sizeof(*ts_skb), GFP_ATOMIC);
+			if (!ts_skb) {
+				if (num_tx_desc > 1) {
+					desc--;
+					dma_unmap_single(ndev->dev.parent, dma_addr,
+							 len, DMA_TO_DEVICE);
+				}
+				goto unmap;
 			}
-			goto unmap;
+			ts_skb->skb = skb_get(skb);
+			ts_skb->tag = priv->ts_skb_tag++;
+			priv->ts_skb_tag &= 0x3ff;
+			list_add_tail(&ts_skb->list, &priv->ts_skb_list);
+
+			/* TAG and timestamp required flag */
+			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+			desc->tagh_tsr = (ts_skb->tag >> 4) | TX_TSR;
+			desc->ds_tagl |= cpu_to_le16(ts_skb->tag << 12);
 		}
-		ts_skb->skb = skb_get(skb);
-		ts_skb->tag = priv->ts_skb_tag++;
-		priv->ts_skb_tag &= 0x3ff;
-		list_add_tail(&ts_skb->list, &priv->ts_skb_list);
 
-		/* TAG and timestamp required flag */
-		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
-		desc->tagh_tsr = (ts_skb->tag >> 4) | TX_TSR;
-		desc->ds_tagl |= cpu_to_le16(ts_skb->tag << 12);
+		skb_tx_timestamp(skb);
 	}
-
-	skb_tx_timestamp(skb);
 	/* Descriptor type must be set after all the above writes */
 	dma_wmb();
 	if (num_tx_desc > 1) {
@@ -1727,10 +1755,12 @@ static int ravb_close(struct net_device *ndev)
 			   "device will be stopped after h/w processes are done.\n");
 
 	/* Clear the timestamp list */
-	list_for_each_entry_safe(ts_skb, ts_skb2, &priv->ts_skb_list, list) {
-		list_del(&ts_skb->list);
-		kfree_skb(ts_skb->skb);
-		kfree(ts_skb);
+	if (info->features & RAVB_EX_RX_DESC) {
+		list_for_each_entry_safe(ts_skb, ts_skb2, &priv->ts_skb_list, list) {
+			list_del(&ts_skb->list);
+			kfree_skb(ts_skb->skb);
+			kfree(ts_skb);
+		}
 	}
 
 	/* PHY disconnect */
@@ -2205,8 +2235,10 @@ static int ravb_probe(struct platform_device *pdev)
 	}
 	clk_prepare_enable(priv->refclk);
 
-	ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
-	ndev->min_mtu = ETH_MIN_MTU;
+	if (info->features & RAVB_OVERRIDE_MTU_CHANGE) {
+		ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
+		ndev->min_mtu = ETH_MIN_MTU;
+	}
 
 	priv->num_tx_desc = info->num_tx_desc;
 
-- 
2.17.1

Re: [PATCH net-next 08/18] ravb: Add R-Car common features

[Sergei Shtylyov]

Hello!

On 7/22/21 5:13 PM, Biju Das wrote:

> The below features are supported by both R-Car Gen2 and Gen3.
> 
> 1) magic packet detection
> 2) multiple TSRQ support
> 3) extended descriptor in rx

   I think this one should better be called timestamping...

> 4) No half duplex support

   Couldn't we avoid the "negative" features?

> 5) override mtu change

   Hm, I'd vote for the individual patches covering only single feature...

> Add features bits to support the same.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 110 +++++++++++++++--------
>  1 file changed, 71 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index b3c99f974632..4ef2565534d2 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -680,11 +694,14 @@ static void ravb_rcv_snd_enable(struct net_device *ndev)
>  /* function for waiting dma process finished */
>  static int ravb_stop_dma(struct net_device *ndev)
>  {
> +	struct ravb_private *priv = netdev_priv(ndev);
> +	const struct ravb_drv_data *info = priv->info;
>  	int error;
>  
>  	/* Wait for stopping the hardware TX process */
> -	error = ravb_wait(ndev, TCCR,
> -			  TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3, 0);
> +	if (info->features & RAVB_MULTI_TSRQ)
> +		error = ravb_wait(ndev, TCCR,
> +				  TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3, 0);
>  	if (error)

   What if the above *if* skips the ravb_wait() call -- didn't you get a complaint from gcc
about the unnintialized variable?

[...]
> @@ -808,11 +826,14 @@ static bool ravb_queue_interrupt(struct net_device *ndev, int q)
>  
>  static bool ravb_timestamp_interrupt(struct net_device *ndev)
>  {
> +	struct ravb_private *priv = netdev_priv(ndev);
> +	const struct ravb_drv_data *info = priv->info;
>  	u32 tis = ravb_read(ndev, TIS);
>  
>  	if (tis & TIS_TFUF) {
>  		ravb_write(ndev, ~(TIS_TFUF | TIS_RESERVED), TIS);
> -		ravb_get_tx_tstamp(ndev);
> +		if (info->features & RAVB_EX_RX_DESC)

   Yeah, definitely a bad feature name...

> +			ravb_get_tx_tstamp(ndev);
>  		return true;
>  	}
>  	return false;
[...]
> @@ -1069,15 +1091,17 @@ static int ravb_phy_init(struct net_device *ndev)
>  		netdev_info(ndev, "limited PHY to 100Mbit/s\n");
>  	}
>  
> -	/* 10BASE, Pause and Asym Pause is not supported */
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Pause_BIT);
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Asym_Pause_BIT);
> +	if (info->features & RAVB_NO_HALF_DUPLEX) {
> +		/* 10BASE, Pause and Asym Pause is not supported */
> +		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
> +		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
> +		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Pause_BIT);
> +		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Asym_Pause_BIT);
>  
> -	/* Half Duplex is not supported */
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
> +		/* Half Duplex is not supported */
> +		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
> +		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);

    Mhm? Some of the half-duplex modes sre unsupported still?

[...]
> @@ -1314,8 +1338,9 @@ static void ravb_get_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
>  static int ravb_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
> +	const struct ravb_drv_data *info = priv->info;
>  
> -	if (wol->wolopts & ~WAKE_MAGIC)
> +	if ((wol->wolopts & ~WAKE_MAGIC) || (!(info->features & RAVB_MAGIC)))

   Parens about !x not needed. And I think the second operand should come first instead...

>  		return -EOPNOTSUPP;
>  
>  	priv->wol_enabled = !!(wol->wolopts & WAKE_MAGIC);
[...]
> @@ -1595,28 +1621,30 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	desc->dptr = cpu_to_le32(dma_addr);
>  
>  	/* TX timestamp required */
> -	if (q == RAVB_NC) {
> -		ts_skb = kmalloc(sizeof(*ts_skb), GFP_ATOMIC);
> -		if (!ts_skb) {
> -			if (num_tx_desc > 1) {
> -				desc--;
> -				dma_unmap_single(ndev->dev.parent, dma_addr,
> -						 len, DMA_TO_DEVICE);
> +	if (info->features & RAVB_EX_RX_DESC) {

   Definitely a bad name...

[...]
> @@ -2205,8 +2235,10 @@ static int ravb_probe(struct platform_device *pdev)
>  	}
>  	clk_prepare_enable(priv->refclk);
>  
> -	ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
> -	ndev->min_mtu = ETH_MIN_MTU;
> +	if (info->features & RAVB_OVERRIDE_MTU_CHANGE) {

   Why? :-/ Could you tell me more details?

> +		ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
> +		ndev->min_mtu = ETH_MIN_MTU;
> +	}
>  
>  	priv->num_tx_desc = info->num_tx_desc;
>  

MBR, Sergei

RE: [PATCH net-next 08/18] ravb: Add R-Car common features

[Biju Das]

Hi Sergei,

Thanks for the feedback.

> Subject: Re: [PATCH net-next 08/18] ravb: Add R-Car common features
> 
> Hello!
> 
> On 7/22/21 5:13 PM, Biju Das wrote:
> 
> > The below features are supported by both R-Car Gen2 and Gen3.
> >
> > 1) magic packet detection
> > 2) multiple TSRQ support
> > 3) extended descriptor in rx
> 
>    I think this one should better be called timestamping...

OK. Will change it to timestamp.

> 
> > 4) No half duplex support
> 
>    Couldn't we avoid the "negative" features?

10 Mbps/Half duplex(As per the comment below) is not supported in R-Car. May be it should be a "positive" feature for
RZ/G2L. probably we need to add this as part of RZ/G2L feature.

> 
> > 5) override mtu change
> 
>    Hm, I'd vote for the individual patches covering only single feature...

OK. That will make patches again simpler.

> 
> > Add features bits to support the same.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/net/ethernet/renesas/ravb_main.c | 110
> > +++++++++++++++--------
> >  1 file changed, 71 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index b3c99f974632..4ef2565534d2 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
> > @@ -680,11 +694,14 @@ static void ravb_rcv_snd_enable(struct
> > net_device *ndev)
> >  /* function for waiting dma process finished */  static int
> > ravb_stop_dma(struct net_device *ndev)  {
> > +	struct ravb_private *priv = netdev_priv(ndev);
> > +	const struct ravb_drv_data *info = priv->info;
> >  	int error;
> >
> >  	/* Wait for stopping the hardware TX process */
> > -	error = ravb_wait(ndev, TCCR,
> > -			  TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3, 0);
> > +	if (info->features & RAVB_MULTI_TSRQ)
> > +		error = ravb_wait(ndev, TCCR,
> > +				  TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 |
> TCCR_TSRQ3, 0);
> >  	if (error)
> 
>    What if the above *if* skips the ravb_wait() call -- didn't you get a
> complaint from gcc about the unnintialized variable?

Good catch will initialize with error = 0 and later patch when we add RZ/G2L, will remove this unnecessary initialization.

> 
> [...]
> > @@ -808,11 +826,14 @@ static bool ravb_queue_interrupt(struct
> > net_device *ndev, int q)
> >
> >  static bool ravb_timestamp_interrupt(struct net_device *ndev)  {
> > +	struct ravb_private *priv = netdev_priv(ndev);
> > +	const struct ravb_drv_data *info = priv->info;
> >  	u32 tis = ravb_read(ndev, TIS);
> >
> >  	if (tis & TIS_TFUF) {
> >  		ravb_write(ndev, ~(TIS_TFUF | TIS_RESERVED), TIS);
> > -		ravb_get_tx_tstamp(ndev);
> > +		if (info->features & RAVB_EX_RX_DESC)
> 
>    Yeah, definitely a bad feature name...
OK. AS you suggested, will change the macro RAVB_EX_RX_DESC to ravb_timestamp feature bit like sh_eth.

> 
> > +			ravb_get_tx_tstamp(ndev);
> >  		return true;
> >  	}
> >  	return false;
> [...]
> > @@ -1069,15 +1091,17 @@ static int ravb_phy_init(struct net_device
> *ndev)
> >  		netdev_info(ndev, "limited PHY to 100Mbit/s\n");
> >  	}
> >
> > -	/* 10BASE, Pause and Asym Pause is not supported */
> > -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
> > -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
> > -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Pause_BIT);
> > -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Asym_Pause_BIT);
> > +	if (info->features & RAVB_NO_HALF_DUPLEX) {
> > +		/* 10BASE, Pause and Asym Pause is not supported */
> > +		phy_remove_link_mode(phydev,
> ETHTOOL_LINK_MODE_10baseT_Half_BIT);
> > +		phy_remove_link_mode(phydev,
> ETHTOOL_LINK_MODE_10baseT_Full_BIT);
> > +		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Pause_BIT);
> > +		phy_remove_link_mode(phydev,
> ETHTOOL_LINK_MODE_Asym_Pause_BIT);
> >
> > -	/* Half Duplex is not supported */
> > -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
> > -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
> > +		/* Half Duplex is not supported */
> > +		phy_remove_link_mode(phydev,
> ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
> > +		phy_remove_link_mode(phydev,
> ETHTOOL_LINK_MODE_100baseT_Half_BIT);
> 
>     Mhm? Some of the half-duplex modes sre unsupported still?
> 
> [...]
> > @@ -1314,8 +1338,9 @@ static void ravb_get_wol(struct net_device
> > *ndev, struct ethtool_wolinfo *wol)  static int ravb_set_wol(struct
> > net_device *ndev, struct ethtool_wolinfo *wol)  {
> >  	struct ravb_private *priv = netdev_priv(ndev);
> > +	const struct ravb_drv_data *info = priv->info;
> >
> > -	if (wol->wolopts & ~WAKE_MAGIC)
> > +	if ((wol->wolopts & ~WAKE_MAGIC) || (!(info->features &
> > +RAVB_MAGIC)))
> 
>    Parens about !x not needed. And I think the second operand should come
> first instead...

OK.

> 
> >  		return -EOPNOTSUPP;
> >
> >  	priv->wol_enabled = !!(wol->wolopts & WAKE_MAGIC);
> [...]
> > @@ -1595,28 +1621,30 @@ static netdev_tx_t ravb_start_xmit(struct
> sk_buff *skb, struct net_device *ndev)
> >  	desc->dptr = cpu_to_le32(dma_addr);
> >
> >  	/* TX timestamp required */
> > -	if (q == RAVB_NC) {
> > -		ts_skb = kmalloc(sizeof(*ts_skb), GFP_ATOMIC);
> > -		if (!ts_skb) {
> > -			if (num_tx_desc > 1) {
> > -				desc--;
> > -				dma_unmap_single(ndev->dev.parent, dma_addr,
> > -						 len, DMA_TO_DEVICE);
> > +	if (info->features & RAVB_EX_RX_DESC) {
> 
>    Definitely a bad name...

OK.

> 
> [...]
> > @@ -2205,8 +2235,10 @@ static int ravb_probe(struct platform_device
> *pdev)
> >  	}
> >  	clk_prepare_enable(priv->refclk);
> >
> > -	ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
> > -	ndev->min_mtu = ETH_MIN_MTU;
> > +	if (info->features & RAVB_OVERRIDE_MTU_CHANGE) {
> 
>    Why? :-/ Could you tell me more details?

RX buff size = 2048 for R-Car where as it is 8K for RZ/G2L.
So Do you think we should use the same max_mtu for RZ/G2L as well?

Please let us know your recommendation here in this case?  The original bsp code used default "eth_mtu_change"
which is now deprecated.

Regards,
Biju

> 
> > +		ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
> > +		ndev->min_mtu = ETH_MIN_MTU;
> > +	}
> >
> >  	priv->num_tx_desc = info->num_tx_desc;
> >
> 
> MBR, Sergei

Re: [PATCH net-next 08/18] ravb: Add R-Car common features

[Andrew Lunn]

> > > @@ -2205,8 +2235,10 @@ static int ravb_probe(struct platform_device
> > *pdev)
> > >  	}
> > >  	clk_prepare_enable(priv->refclk);
> > >
> > > -	ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
> > > -	ndev->min_mtu = ETH_MIN_MTU;
> > > +	if (info->features & RAVB_OVERRIDE_MTU_CHANGE) {
> > 
> >    Why? :-/ Could you tell me more details?
> 
> RX buff size = 2048 for R-Car where as it is 8K for RZ/G2L.

RAVB_OVERRIDE_MTU_CHANGE is not the most descriptive name. You are not
overriding, you are setting the correct value for the hardware
variant.

Maybe name the feature RAVB_8K_BUFFERS or RAVB_2K_BUFFERS.

Also, putting more details in the commit message will help, and lots
of small patches, each patch doing one thing. It is much better to
have 40 simple, well documented, obviously correct patches, than 20
hard to understand patches. But please do submit them in small
batches, no more than 15 at once.

	 Andrew

RE: [PATCH net-next 08/18] ravb: Add R-Car common features

[Biju Das]

Hi Andrew,

Thanks for the feedback.

> Subject: Re: [PATCH net-next 08/18] ravb: Add R-Car common features
> 
> > > > @@ -2205,8 +2235,10 @@ static int ravb_probe(struct
> > > > platform_device
> > > *pdev)
> > > >  	}
> > > >  	clk_prepare_enable(priv->refclk);
> > > >
> > > > -	ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
> > > > -	ndev->min_mtu = ETH_MIN_MTU;
> > > > +	if (info->features & RAVB_OVERRIDE_MTU_CHANGE) {
> > >
> > >    Why? :-/ Could you tell me more details?
> >
> > RX buff size = 2048 for R-Car where as it is 8K for RZ/G2L.
> 
> RAVB_OVERRIDE_MTU_CHANGE is not the most descriptive name. You are not
> overriding, you are setting the correct value for the hardware variant.

Thanks for correcting me.

> Maybe name the feature RAVB_8K_BUFFERS or RAVB_2K_BUFFERS.

OK.

> 
> Also, putting more details in the commit message will help, and lots of
> small patches, each patch doing one thing. 

Agreed. Will send smaller patches with more details on commit message.

It is much better to have 40
> simple, well documented, obviously correct patches, than 20 hard to
> understand patches. But please do submit them in small batches, no more
> than 15 at once.

OK. 

Cheers,
Biju

[PATCH net-next 09/18] ravb: Factorise ravb_ring_free function

[Biju Das]

Extended descriptor support in RX is available for R-Car where as it
is a normal descriptor for RZ/G2L. Factorise ravb_ring_free function
so that it can support later SoC.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  5 +++
 drivers/net/ethernet/renesas/ravb_main.c | 49 ++++++++++++++++--------
 2 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index a474ed68db22..3a9cf6e8671a 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -988,7 +988,12 @@ enum ravb_chip_id {
 	RCAR_GEN3,
 };
 
+struct ravb_ops {
+	void (*ring_free)(struct net_device *ndev, int q);
+};
+
 struct ravb_drv_data {
+	const struct ravb_ops *ravb_ops;
 	netdev_features_t net_features;
 	netdev_features_t net_hw_features;
 	const char (*gstrings_stats)[ETH_GSTRING_LEN];
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 4ef2565534d2..a3b8b243fd54 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -247,30 +247,39 @@ static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
 }
 
 /* Free skb's and DMA buffers for Ethernet AVB */
-static void ravb_ring_free(struct net_device *ndev, int q)
+static void ravb_ring_free_rx(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
-	int num_tx_desc = priv->num_tx_desc;
 	int ring_size;
 	int i;
 
-	if (priv->rx_ring[q]) {
-		for (i = 0; i < priv->num_rx_ring[q]; i++) {
-			struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
+	for (i = 0; i < priv->num_rx_ring[q]; i++) {
+		struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
 
-			if (!dma_mapping_error(ndev->dev.parent,
-					       le32_to_cpu(desc->dptr)))
-				dma_unmap_single(ndev->dev.parent,
-						 le32_to_cpu(desc->dptr),
-						 RX_BUF_SZ,
-						 DMA_FROM_DEVICE);
-		}
-		ring_size = sizeof(struct ravb_ex_rx_desc) *
-			    (priv->num_rx_ring[q] + 1);
-		dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q],
-				  priv->rx_desc_dma[q]);
-		priv->rx_ring[q] = NULL;
+		if (!dma_mapping_error(ndev->dev.parent,
+				       le32_to_cpu(desc->dptr)))
+			dma_unmap_single(ndev->dev.parent,
+					 le32_to_cpu(desc->dptr),
+					 RX_BUF_SZ,
+					 DMA_FROM_DEVICE);
 	}
+	ring_size = sizeof(struct ravb_ex_rx_desc) *
+		    (priv->num_rx_ring[q] + 1);
+	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q],
+			  priv->rx_desc_dma[q]);
+	priv->rx_ring[q] = NULL;
+}
+
+static void ravb_ring_free(struct net_device *ndev, int q)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
+	int num_tx_desc = priv->num_tx_desc;
+	int ring_size;
+	int i;
+
+	if (priv->rx_ring[q])
+		info->ravb_ops->ring_free(ndev, q);
 
 	if (priv->tx_ring[q]) {
 		ravb_tx_free(ndev, q, false);
@@ -1987,7 +1996,12 @@ static int ravb_mdio_release(struct ravb_private *priv)
 	return 0;
 }
 
+static const struct ravb_ops ravb_gen3_ops = {
+	.ring_free = ravb_ring_free_rx,
+};
+
 static const struct ravb_drv_data ravb_gen3_data = {
+	.ravb_ops = &ravb_gen3_ops,
 	.net_features = NETIF_F_RXCSUM,
 	.net_hw_features = NETIF_F_RXCSUM,
 	.gstrings_stats = ravb_gstrings_stats,
@@ -2001,6 +2015,7 @@ static const struct ravb_drv_data ravb_gen3_data = {
 };
 
 static const struct ravb_drv_data ravb_gen2_data = {
+	.ravb_ops = &ravb_gen3_ops,
 	.net_features = NETIF_F_RXCSUM,
 	.net_hw_features = NETIF_F_RXCSUM,
 	.gstrings_stats = ravb_gstrings_stats,
-- 
2.17.1

Re: [PATCH net-next 09/18] ravb: Factorise ravb_ring_free function

[Sergei Shtylyov]

Hello!

On 7/22/21 5:13 PM, Biju Das wrote:

> Extended descriptor support in RX is available for R-Car where as it
> is a normal descriptor for RZ/G2L. Factorise ravb_ring_free function
> so that it can support later SoC.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb.h      |  5 +++
>  drivers/net/ethernet/renesas/ravb_main.c | 49 ++++++++++++++++--------
>  2 files changed, 37 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index a474ed68db22..3a9cf6e8671a 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -988,7 +988,12 @@ enum ravb_chip_id {
>  	RCAR_GEN3,
>  };
>  
> +struct ravb_ops {
> +	void (*ring_free)(struct net_device *ndev, int q);

   Hmm, why not store it right in the *struct* ravb_drv_data?

> +};
> +
>  struct ravb_drv_data {
> +	const struct ravb_ops *ravb_ops;
>  	netdev_features_t net_features;
>  	netdev_features_t net_hw_features;
>  	const char (*gstrings_stats)[ETH_GSTRING_LEN];
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 4ef2565534d2..a3b8b243fd54 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -247,30 +247,39 @@ static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
>  }
>  
>  /* Free skb's and DMA buffers for Ethernet AVB */
> -static void ravb_ring_free(struct net_device *ndev, int q)
> +static void ravb_ring_free_rx(struct net_device *ndev, int q)

   How about ravb_rx_ring_free() instead?
 
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
> -	int num_tx_desc = priv->num_tx_desc;
>  	int ring_size;
>  	int i;
>  
> -	if (priv->rx_ring[q]) {
> -		for (i = 0; i < priv->num_rx_ring[q]; i++) {
> -			struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
> +	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> +		struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
>  
> -			if (!dma_mapping_error(ndev->dev.parent,
> -					       le32_to_cpu(desc->dptr)))
> -				dma_unmap_single(ndev->dev.parent,
> -						 le32_to_cpu(desc->dptr),
> -						 RX_BUF_SZ,
> -						 DMA_FROM_DEVICE);
> -		}
> -		ring_size = sizeof(struct ravb_ex_rx_desc) *
> -			    (priv->num_rx_ring[q] + 1);
> -		dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q],
> -				  priv->rx_desc_dma[q]);
> -		priv->rx_ring[q] = NULL;
> +		if (!dma_mapping_error(ndev->dev.parent,
> +				       le32_to_cpu(desc->dptr)))
> +			dma_unmap_single(ndev->dev.parent,
> +					 le32_to_cpu(desc->dptr),
> +					 RX_BUF_SZ,
> +					 DMA_FROM_DEVICE);
>  	}
> +	ring_size = sizeof(struct ravb_ex_rx_desc) *
> +		    (priv->num_rx_ring[q] + 1);
> +	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q],
> +			  priv->rx_desc_dma[q]);
> +	priv->rx_ring[q] = NULL;

   Couldn't this be moved into the new ravb_ring_free(), like the initial NULL check?

> +}
> +
> +static void ravb_ring_free(struct net_device *ndev, int q)
> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +	const struct ravb_drv_data *info = priv->info;
> +	int num_tx_desc = priv->num_tx_desc;
> +	int ring_size;
> +	int i;
> +
> +	if (priv->rx_ring[q])
> +		info->ravb_ops->ring_free(ndev, q);

   ... here?

[...]

MBR, Sergei

RE: [PATCH net-next 09/18] ravb: Factorise ravb_ring_free function

[Biju Das]

Hi Sergei,

Thanks for the feedback.

> Subject: Re: [PATCH net-next 09/18] ravb: Factorise ravb_ring_free
> function
> 
> Hello!
> 
> On 7/22/21 5:13 PM, Biju Das wrote:
> 
> > Extended descriptor support in RX is available for R-Car where as it
> > is a normal descriptor for RZ/G2L. Factorise ravb_ring_free function
> > so that it can support later SoC.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/net/ethernet/renesas/ravb.h      |  5 +++
> >  drivers/net/ethernet/renesas/ravb_main.c | 49
> > ++++++++++++++++--------
> >  2 files changed, 37 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb.h
> > b/drivers/net/ethernet/renesas/ravb.h
> > index a474ed68db22..3a9cf6e8671a 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> > @@ -988,7 +988,12 @@ enum ravb_chip_id {
> >  	RCAR_GEN3,
> >  };
> >
> > +struct ravb_ops {
> > +	void (*ring_free)(struct net_device *ndev, int q);
> 
>    Hmm, why not store it right in the *struct* ravb_drv_data?

OK.

> 
> > +};
> > +
> >  struct ravb_drv_data {
> > +	const struct ravb_ops *ravb_ops;
> >  	netdev_features_t net_features;
> >  	netdev_features_t net_hw_features;
> >  	const char (*gstrings_stats)[ETH_GSTRING_LEN];
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index 4ef2565534d2..a3b8b243fd54 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -247,30 +247,39 @@ static int ravb_tx_free(struct net_device *ndev,
> > int q, bool free_txed_only)  }
> >
> >  /* Free skb's and DMA buffers for Ethernet AVB */ -static void
> > ravb_ring_free(struct net_device *ndev, int q)
> > +static void ravb_ring_free_rx(struct net_device *ndev, int q)
> 
>    How about ravb_rx_ring_free() instead?
Agreed.

> 
> >  {
> >  	struct ravb_private *priv = netdev_priv(ndev);
> > -	int num_tx_desc = priv->num_tx_desc;
> >  	int ring_size;
> >  	int i;
> >
> > -	if (priv->rx_ring[q]) {
> > -		for (i = 0; i < priv->num_rx_ring[q]; i++) {
> > -			struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
> > +	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> > +		struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
> >
> > -			if (!dma_mapping_error(ndev->dev.parent,
> > -					       le32_to_cpu(desc->dptr)))
> > -				dma_unmap_single(ndev->dev.parent,
> > -						 le32_to_cpu(desc->dptr),
> > -						 RX_BUF_SZ,
> > -						 DMA_FROM_DEVICE);
> > -		}
> > -		ring_size = sizeof(struct ravb_ex_rx_desc) *
> > -			    (priv->num_rx_ring[q] + 1);
> > -		dma_free_coherent(ndev->dev.parent, ring_size, priv-
> >rx_ring[q],
> > -				  priv->rx_desc_dma[q]);
> > -		priv->rx_ring[q] = NULL;
> > +		if (!dma_mapping_error(ndev->dev.parent,
> > +				       le32_to_cpu(desc->dptr)))
> > +			dma_unmap_single(ndev->dev.parent,
> > +					 le32_to_cpu(desc->dptr),
> > +					 RX_BUF_SZ,
> > +					 DMA_FROM_DEVICE);
> >  	}
> > +	ring_size = sizeof(struct ravb_ex_rx_desc) *
> > +		    (priv->num_rx_ring[q] + 1);
> > +	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q],
> > +			  priv->rx_desc_dma[q]);
> > +	priv->rx_ring[q] = NULL;
> 
>    Couldn't this be moved into the new ravb_ring_free(), like the initial
> NULL check?

For RZ/G2L, it is priv->rgeth_rx_ring, that is the reason.

I can move the initial NULL check here, so the generic ravb_ring_free does not differentiate between
priv->rx_ring and priv->rgeth_rx_ring. See below.

> 
> > +}
> > +
> > +static void ravb_ring_free(struct net_device *ndev, int q) {
> > +	struct ravb_private *priv = netdev_priv(ndev);
> > +	const struct ravb_drv_data *info = priv->info;
> > +	int num_tx_desc = priv->num_tx_desc;
> > +	int ring_size;
> > +	int i;
> > +
> > +	if (priv->rx_ring[q])
> > +		info->ravb_ops->ring_free(ndev, q);
> 
>    ... here?

     It will be just  info->ravb_ops->ring_free(ndev, q);
And NULL check will be handled respective rx helper function.

What do you think?

Cheers,
Biju

> 
> [...]
> 
> MBR, Sergei

RE: [PATCH net-next 09/18] ravb: Factorise ravb_ring_free function

[Biju Das]

Hi Sergei,


> Subject: RE: [PATCH net-next 09/18] ravb: Factorise ravb_ring_free
> function
> 
> Hi Sergei,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH net-next 09/18] ravb: Factorise ravb_ring_free
> > function
> >
> > Hello!
> >
> > On 7/22/21 5:13 PM, Biju Das wrote:
> >
> > > Extended descriptor support in RX is available for R-Car where as it
> > > is a normal descriptor for RZ/G2L. Factorise ravb_ring_free function
> > > so that it can support later SoC.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  drivers/net/ethernet/renesas/ravb.h      |  5 +++
> > >  drivers/net/ethernet/renesas/ravb_main.c | 49
> > > ++++++++++++++++--------
> > >  2 files changed, 37 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/renesas/ravb.h
> > > b/drivers/net/ethernet/renesas/ravb.h
> > > index a474ed68db22..3a9cf6e8671a 100644
> > > --- a/drivers/net/ethernet/renesas/ravb.h
> > > +++ b/drivers/net/ethernet/renesas/ravb.h
> > > @@ -988,7 +988,12 @@ enum ravb_chip_id {
> > >  	RCAR_GEN3,
> > >  };
> > >
> > > +struct ravb_ops {
> > > +	void (*ring_free)(struct net_device *ndev, int q);
> >
> >    Hmm, why not store it right in the *struct* ravb_drv_data?
> 
> OK.
> 
> >
> > > +};
> > > +
> > >  struct ravb_drv_data {
> > > +	const struct ravb_ops *ravb_ops;
> > >  	netdev_features_t net_features;
> > >  	netdev_features_t net_hw_features;
> > >  	const char (*gstrings_stats)[ETH_GSTRING_LEN];
> > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > > b/drivers/net/ethernet/renesas/ravb_main.c
> > > index 4ef2565534d2..a3b8b243fd54 100644
> > > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > > @@ -247,30 +247,39 @@ static int ravb_tx_free(struct net_device
> > > *ndev, int q, bool free_txed_only)  }
> > >
> > >  /* Free skb's and DMA buffers for Ethernet AVB */ -static void
> > > ravb_ring_free(struct net_device *ndev, int q)
> > > +static void ravb_ring_free_rx(struct net_device *ndev, int q)
> >
> >    How about ravb_rx_ring_free() instead?
> Agreed.
> 
> >
> > >  {
> > >  	struct ravb_private *priv = netdev_priv(ndev);
> > > -	int num_tx_desc = priv->num_tx_desc;
> > >  	int ring_size;
> > >  	int i;
> > >
> > > -	if (priv->rx_ring[q]) {
> > > -		for (i = 0; i < priv->num_rx_ring[q]; i++) {
> > > -			struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
> > > +	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> > > +		struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
> > >
> > > -			if (!dma_mapping_error(ndev->dev.parent,
> > > -					       le32_to_cpu(desc->dptr)))
> > > -				dma_unmap_single(ndev->dev.parent,
> > > -						 le32_to_cpu(desc->dptr),
> > > -						 RX_BUF_SZ,
> > > -						 DMA_FROM_DEVICE);
> > > -		}
> > > -		ring_size = sizeof(struct ravb_ex_rx_desc) *
> > > -			    (priv->num_rx_ring[q] + 1);
> > > -		dma_free_coherent(ndev->dev.parent, ring_size, priv-
> > >rx_ring[q],
> > > -				  priv->rx_desc_dma[q]);
> > > -		priv->rx_ring[q] = NULL;
> > > +		if (!dma_mapping_error(ndev->dev.parent,
> > > +				       le32_to_cpu(desc->dptr)))
> > > +			dma_unmap_single(ndev->dev.parent,
> > > +					 le32_to_cpu(desc->dptr),
> > > +					 RX_BUF_SZ,
> > > +					 DMA_FROM_DEVICE);
> > >  	}
> > > +	ring_size = sizeof(struct ravb_ex_rx_desc) *
> > > +		    (priv->num_rx_ring[q] + 1);
> > > +	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q],
> > > +			  priv->rx_desc_dma[q]);
> > > +	priv->rx_ring[q] = NULL;
> >
> >    Couldn't this be moved into the new ravb_ring_free(), like the
> > initial NULL check?
> 
> For RZ/G2L, it is priv->rgeth_rx_ring, that is the reason.
> 
> I can move the initial NULL check here, so the generic ravb_ring_free does
> not differentiate between
> priv->rx_ring and priv->rgeth_rx_ring. See below.
> 
> >
> > > +}
> > > +
> > > +static void ravb_ring_free(struct net_device *ndev, int q) {
> > > +	struct ravb_private *priv = netdev_priv(ndev);
> > > +	const struct ravb_drv_data *info = priv->info;
> > > +	int num_tx_desc = priv->num_tx_desc;
> > > +	int ring_size;
> > > +	int i;
> > > +
> > > +	if (priv->rx_ring[q])
> > > +		info->ravb_ops->ring_free(ndev, q);
> >
> >    ... here?
> 
>      It will be just  info->ravb_ops->ring_free(ndev, q); And NULL check
> will be handled respective rx helper function.

I will be sending next set of patches incorporating the above comments
within couple of days.

Regards,
Biju

[PATCH net-next 10/18] ravb: Factorise ravb_ring_format function

[Biju Das]

The ravb_ring_format function uses extended descriptor in rx for
R-Car where as it use normal descriptor for RZ/G2L. Factorise
rx ring buffer buildup to extend the support for later SoC.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  1 +
 drivers/net/ethernet/renesas/ravb_main.c | 34 +++++++++++++++---------
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 3a9cf6e8671a..a3258c5d0c3d 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -990,6 +990,7 @@ enum ravb_chip_id {
 
 struct ravb_ops {
 	void (*ring_free)(struct net_device *ndev, int q);
+	void (*ring_format)(struct net_device *ndev, int q);
 };
 
 struct ravb_drv_data {
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index a3b8b243fd54..c23f0d420c70 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -311,26 +311,15 @@ static void ravb_ring_free(struct net_device *ndev, int q)
 }
 
 /* Format skb and descriptor buffer for Ethernet AVB */
-static void ravb_ring_format(struct net_device *ndev, int q)
+static void ravb_ring_format_rx(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
-	int num_tx_desc = priv->num_tx_desc;
 	struct ravb_ex_rx_desc *rx_desc;
-	struct ravb_tx_desc *tx_desc;
-	struct ravb_desc *desc;
 	int rx_ring_size = sizeof(*rx_desc) * priv->num_rx_ring[q];
-	int tx_ring_size = sizeof(*tx_desc) * priv->num_tx_ring[q] *
-			   num_tx_desc;
 	dma_addr_t dma_addr;
 	int i;
 
-	priv->cur_rx[q] = 0;
-	priv->cur_tx[q] = 0;
-	priv->dirty_rx[q] = 0;
-	priv->dirty_tx[q] = 0;
-
 	memset(priv->rx_ring[q], 0, rx_ring_size);
-	/* Build RX ring buffer */
 	for (i = 0; i < priv->num_rx_ring[q]; i++) {
 		/* RX descriptor */
 		rx_desc = &priv->rx_ring[q][i];
@@ -349,6 +338,26 @@ static void ravb_ring_format(struct net_device *ndev, int q)
 	rx_desc = &priv->rx_ring[q][i];
 	rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
 	rx_desc->die_dt = DT_LINKFIX; /* type */
+}
+
+static void ravb_ring_format(struct net_device *ndev, int q)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
+	int num_tx_desc = priv->num_tx_desc;
+	struct ravb_tx_desc *tx_desc;
+	struct ravb_desc *desc;
+	int tx_ring_size = sizeof(*tx_desc) * priv->num_tx_ring[q] *
+			   num_tx_desc;
+	int i;
+
+	priv->cur_rx[q] = 0;
+	priv->cur_tx[q] = 0;
+	priv->dirty_rx[q] = 0;
+	priv->dirty_tx[q] = 0;
+
+	/* Build RX ring buffer */
+	info->ravb_ops->ring_format(ndev, q);
 
 	memset(priv->tx_ring[q], 0, tx_ring_size);
 	/* Build TX ring buffer */
@@ -1998,6 +2007,7 @@ static int ravb_mdio_release(struct ravb_private *priv)
 
 static const struct ravb_ops ravb_gen3_ops = {
 	.ring_free = ravb_ring_free_rx,
+	.ring_format = ravb_ring_format_rx,
 };
 
 static const struct ravb_drv_data ravb_gen3_data = {
-- 
2.17.1

Re: [PATCH net-next 10/18] ravb: Factorise ravb_ring_format function

[Sergei Shtylyov]

On 7/22/21 5:13 PM, Biju Das wrote:

> The ravb_ring_format function uses extended descriptor in rx for
> R-Car where as it use normal descriptor for RZ/G2L. Factorise
> rx ring buffer buildup to extend the support for later SoC.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb.h      |  1 +
>  drivers/net/ethernet/renesas/ravb_main.c | 34 +++++++++++++++---------
>  2 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 3a9cf6e8671a..a3258c5d0c3d 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -990,6 +990,7 @@ enum ravb_chip_id {
>  
>  struct ravb_ops {
>  	void (*ring_free)(struct net_device *ndev, int q);
> +	void (*ring_format)(struct net_device *ndev, int q);

   Like I said, we don't need another indirection.... also both ops are for RX.

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index a3b8b243fd54..c23f0d420c70 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -311,26 +311,15 @@ static void ravb_ring_free(struct net_device *ndev, int q)
>  }
>  
>  /* Format skb and descriptor buffer for Ethernet AVB */
> -static void ravb_ring_format(struct net_device *ndev, int q)
> +static void ravb_ring_format_rx(struct net_device *ndev, int rxq)

   How about ravb_rx_ring_format(struct net_device *ndev, int q)?

[...]

MBR, Sergei

RE: [PATCH net-next 10/18] ravb: Factorise ravb_ring_format function

[Biju Das]

HI Sergei,

Thanks for the feedback.

> Subject: Re: [PATCH net-next 10/18] ravb: Factorise ravb_ring_format
> function
> 
> On 7/22/21 5:13 PM, Biju Das wrote:
> 
> > The ravb_ring_format function uses extended descriptor in rx for R-Car
> > where as it use normal descriptor for RZ/G2L. Factorise rx ring buffer
> > buildup to extend the support for later SoC.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/net/ethernet/renesas/ravb.h      |  1 +
> >  drivers/net/ethernet/renesas/ravb_main.c | 34
> > +++++++++++++++---------
> >  2 files changed, 23 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb.h
> > b/drivers/net/ethernet/renesas/ravb.h
> > index 3a9cf6e8671a..a3258c5d0c3d 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> > @@ -990,6 +990,7 @@ enum ravb_chip_id {
> >
> >  struct ravb_ops {
> >  	void (*ring_free)(struct net_device *ndev, int q);
> > +	void (*ring_format)(struct net_device *ndev, int q);
> 
>    Like I said, we don't need another indirection.... also both ops are
> for RX.

OK. Will add this as part of ravb_drv_data.

> 
> [...]
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index a3b8b243fd54..c23f0d420c70 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -311,26 +311,15 @@ static void ravb_ring_free(struct net_device
> > *ndev, int q)  }
> >
> >  /* Format skb and descriptor buffer for Ethernet AVB */ -static void
> > ravb_ring_format(struct net_device *ndev, int q)
> > +static void ravb_ring_format_rx(struct net_device *ndev, int rxq)
> 
>    How about ravb_rx_ring_format(struct net_device *ndev, int q)?

Agreed.

Cheers,
Biju

> 
> [...]
> 
> MBR, Sergei

[PATCH net-next 11/18] ravb: Factorise ravb_ring_init function

[Biju Das]

The ravb_ring_init function uses extended descriptor in rx for
R-Car and normal descriptor for RZ/G2L. Factorise rx ring buffer
allocation so that it can support later SoC.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  1 +
 drivers/net/ethernet/renesas/ravb_main.c | 20 +++++++++++++++-----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index a3258c5d0c3d..d82bfa6e57c1 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -991,6 +991,7 @@ enum ravb_chip_id {
 struct ravb_ops {
 	void (*ring_free)(struct net_device *ndev, int q);
 	void (*ring_format)(struct net_device *ndev, int q);
+	bool (*alloc_rx_desc)(struct net_device *ndev, int q);
 };
 
 struct ravb_drv_data {
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index c23f0d420c70..3d0f6598b936 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -384,6 +384,19 @@ static void ravb_ring_format(struct net_device *ndev, int q)
 }
 
 /* Init skb and descriptor buffer for Ethernet AVB */
+static bool ravb_alloc_rx_desc(struct net_device *ndev, int q)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int ring_size;
+
+	ring_size = sizeof(struct ravb_ex_rx_desc) * (priv->num_rx_ring[q] + 1);
+
+	priv->rx_ring[q] = dma_alloc_coherent(ndev->dev.parent, ring_size,
+					      &priv->rx_desc_dma[q],
+					      GFP_KERNEL);
+	return priv->rx_ring[q];
+}
+
 static int ravb_ring_init(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -418,11 +431,7 @@ static int ravb_ring_init(struct net_device *ndev, int q)
 	}
 
 	/* Allocate all RX descriptors. */
-	ring_size = sizeof(struct ravb_ex_rx_desc) * (priv->num_rx_ring[q] + 1);
-	priv->rx_ring[q] = dma_alloc_coherent(ndev->dev.parent, ring_size,
-					      &priv->rx_desc_dma[q],
-					      GFP_KERNEL);
-	if (!priv->rx_ring[q])
+	if (!info->ravb_ops->alloc_rx_desc(ndev, q))
 		goto error;
 
 	priv->dirty_rx[q] = 0;
@@ -2008,6 +2017,7 @@ static int ravb_mdio_release(struct ravb_private *priv)
 static const struct ravb_ops ravb_gen3_ops = {
 	.ring_free = ravb_ring_free_rx,
 	.ring_format = ravb_ring_format_rx,
+	.alloc_rx_desc = ravb_alloc_rx_desc,
 };
 
 static const struct ravb_drv_data ravb_gen3_data = {
-- 
2.17.1

Re: [PATCH net-next 11/18] ravb: Factorise ravb_ring_init function

[Sergei Shtylyov]

On 7/22/21 5:13 PM, Biju Das wrote:

> The ravb_ring_init function uses extended descriptor in rx for
> R-Car and normal descriptor for RZ/G2L. Factorise rx ring buffer
> allocation so that it can support later SoC.

   In this case I think you factored out the function in question... but my ENglish is possibly
too weak. :-)

> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb.h      |  1 +
>  drivers/net/ethernet/renesas/ravb_main.c | 20 +++++++++++++++-----
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index a3258c5d0c3d..d82bfa6e57c1 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -991,6 +991,7 @@ enum ravb_chip_id {
>  struct ravb_ops {
>  	void (*ring_free)(struct net_device *ndev, int q);
>  	void (*ring_format)(struct net_device *ndev, int q);
> +	bool (*alloc_rx_desc)(struct net_device *ndev, int q);

  Aha, rx_ appears at last! :-)


> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index c23f0d420c70..3d0f6598b936 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -384,6 +384,19 @@ static void ravb_ring_format(struct net_device *ndev, int q)
>  }
>  
>  /* Init skb and descriptor buffer for Ethernet AVB */
> +static bool ravb_alloc_rx_desc(struct net_device *ndev, int q)

    Why *bool*? I think we shold just return a pointer allocated.

> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +	int ring_size;
> +
> +	ring_size = sizeof(struct ravb_ex_rx_desc) * (priv->num_rx_ring[q] + 1);
> +
> +	priv->rx_ring[q] = dma_alloc_coherent(ndev->dev.parent, ring_size,
> +					      &priv->rx_desc_dma[q],
> +					      GFP_KERNEL);
> +	return priv->rx_ring[q];
> +}
> +
>  static int ravb_ring_init(struct net_device *ndev, int q)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
> @@ -418,11 +431,7 @@ static int ravb_ring_init(struct net_device *ndev, int q)
>  	}
>  
>  	/* Allocate all RX descriptors. */
> -	ring_size = sizeof(struct ravb_ex_rx_desc) * (priv->num_rx_ring[q] + 1);
> -	priv->rx_ring[q] = dma_alloc_coherent(ndev->dev.parent, ring_size,
> -					      &priv->rx_desc_dma[q],
> -					      GFP_KERNEL);
> -	if (!priv->rx_ring[q])
> +	if (!info->ravb_ops->alloc_rx_desc(ndev, q))
>  		goto error;
>  
>  	priv->dirty_rx[q] = 0;
> @@ -2008,6 +2017,7 @@ static int ravb_mdio_release(struct ravb_private *priv)
>  static const struct ravb_ops ravb_gen3_ops = {
>  	.ring_free = ravb_ring_free_rx,
>  	.ring_format = ravb_ring_format_rx,
> +	.alloc_rx_desc = ravb_alloc_rx_desc,
>  };
>  
>  static const struct ravb_drv_data ravb_gen3_data = {
> 

RE: [PATCH net-next 11/18] ravb: Factorise ravb_ring_init function

[Biju Das]

Hi Sergei,

Thanks for the feedback.

> Subject: Re: [PATCH net-next 11/18] ravb: Factorise ravb_ring_init
> function
> 
> On 7/22/21 5:13 PM, Biju Das wrote:
> 
> > The ravb_ring_init function uses extended descriptor in rx for R-Car
> > and normal descriptor for RZ/G2L. Factorise rx ring buffer allocation
> > so that it can support later SoC.
> 
>    In this case I think you factored out the function in question... but
> my ENglish is possibly too weak. :-)

OK. I meant add a helper function for rx ring buffer allocation.

> 
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/net/ethernet/renesas/ravb.h      |  1 +
> >  drivers/net/ethernet/renesas/ravb_main.c | 20 +++++++++++++++-----
> >  2 files changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb.h
> > b/drivers/net/ethernet/renesas/ravb.h
> > index a3258c5d0c3d..d82bfa6e57c1 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> > @@ -991,6 +991,7 @@ enum ravb_chip_id {  struct ravb_ops {
> >  	void (*ring_free)(struct net_device *ndev, int q);
> >  	void (*ring_format)(struct net_device *ndev, int q);
> > +	bool (*alloc_rx_desc)(struct net_device *ndev, int q);
> 
>   Aha, rx_ appears at last! :-)
> 
> 
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index c23f0d420c70..3d0f6598b936 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -384,6 +384,19 @@ static void ravb_ring_format(struct net_device
> > *ndev, int q)  }
> >
> >  /* Init skb and descriptor buffer for Ethernet AVB */
> > +static bool ravb_alloc_rx_desc(struct net_device *ndev, int q)
> 
>     Why *bool*? I think we shold just return a pointer allocated.

Ah OK.

Cheers,
Biju

> 
> > +{
> > +	struct ravb_private *priv = netdev_priv(ndev);
> > +	int ring_size;
> > +
> > +	ring_size = sizeof(struct ravb_ex_rx_desc) * (priv->num_rx_ring[q] +
> > +1);
> > +
> > +	priv->rx_ring[q] = dma_alloc_coherent(ndev->dev.parent, ring_size,
> > +					      &priv->rx_desc_dma[q],
> > +					      GFP_KERNEL);
> > +	return priv->rx_ring[q];
> > +}
> > +
> >  static int ravb_ring_init(struct net_device *ndev, int q)  {
> >  	struct ravb_private *priv = netdev_priv(ndev); @@ -418,11 +431,7 @@
> > static int ravb_ring_init(struct net_device *ndev, int q)
> >  	}
> >
> >  	/* Allocate all RX descriptors. */
> > -	ring_size = sizeof(struct ravb_ex_rx_desc) * (priv->num_rx_ring[q] +
> 1);
> > -	priv->rx_ring[q] = dma_alloc_coherent(ndev->dev.parent, ring_size,
> > -					      &priv->rx_desc_dma[q],
> > -					      GFP_KERNEL);
> > -	if (!priv->rx_ring[q])
> > +	if (!info->ravb_ops->alloc_rx_desc(ndev, q))
> >  		goto error;
> >
> >  	priv->dirty_rx[q] = 0;
> > @@ -2008,6 +2017,7 @@ static int ravb_mdio_release(struct ravb_private
> > *priv)  static const struct ravb_ops ravb_gen3_ops = {
> >  	.ring_free = ravb_ring_free_rx,
> >  	.ring_format = ravb_ring_format_rx,
> > +	.alloc_rx_desc = ravb_alloc_rx_desc,
> >  };
> >
> >  static const struct ravb_drv_data ravb_gen3_data = {
> >

[PATCH net-next 12/18] ravb: Factorise {emac,dmac} init function

[Biju Das]

The R-Car AVB module has Magic packet detection, multiple irq's and
timestamp enable features which is not present on RZ/G2L Gigabit
Ethernet module. Factorise emac and dmac initialization function to
support the later SoC.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  2 +
 drivers/net/ethernet/renesas/ravb_main.c | 58 ++++++++++++++++--------
 2 files changed, 40 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index d82bfa6e57c1..4d5910dcda86 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -992,6 +992,8 @@ struct ravb_ops {
 	void (*ring_free)(struct net_device *ndev, int q);
 	void (*ring_format)(struct net_device *ndev, int q);
 	bool (*alloc_rx_desc)(struct net_device *ndev, int q);
+	void (*emac_init)(struct net_device *ndev);
+	void (*dmac_init)(struct net_device *ndev);
 };
 
 struct ravb_drv_data {
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 3d0f6598b936..e200114376e4 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -454,7 +454,7 @@ static int ravb_ring_init(struct net_device *ndev, int q)
 }
 
 /* E-MAC init function */
-static void ravb_emac_init(struct net_device *ndev)
+static void ravb_emac_init_ex(struct net_device *ndev)
 {
 	/* Receive frame limit set register */
 	ravb_write(ndev, ndev->mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN, RFLR);
@@ -480,30 +480,19 @@ static void ravb_emac_init(struct net_device *ndev)
 	ravb_write(ndev, ECSIPR_ICDIP | ECSIPR_MPDIP | ECSIPR_LCHNGIP, ECSIPR);
 }
 
-/* Device init function for Ethernet AVB */
-static int ravb_dmac_init(struct net_device *ndev)
+static void ravb_emac_init(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_drv_data *info = priv->info;
-	int error;
 
-	/* Set CONFIG mode */
-	error = ravb_config(ndev);
-	if (error)
-		return error;
-
-	error = ravb_ring_init(ndev, RAVB_BE);
-	if (error)
-		return error;
-	error = ravb_ring_init(ndev, RAVB_NC);
-	if (error) {
-		ravb_ring_free(ndev, RAVB_BE);
-		return error;
-	}
+	info->ravb_ops->emac_init(ndev);
+}
 
-	/* Descriptor format */
-	ravb_ring_format(ndev, RAVB_BE);
-	ravb_ring_format(ndev, RAVB_NC);
+/* Device init function for Ethernet AVB */
+static void ravb_dmac_init_ex(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 
 	/* Set AVB RX */
 	ravb_write(ndev,
@@ -530,6 +519,33 @@ static int ravb_dmac_init(struct net_device *ndev)
 	ravb_write(ndev, RIC2_QFE0 | RIC2_QFE1 | RIC2_RFFE, RIC2);
 	/* Frame transmitted, timestamp FIFO updated */
 	ravb_write(ndev, TIC_FTE0 | TIC_FTE1 | TIC_TFUE, TIC);
+}
+
+static int ravb_dmac_init(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
+	int error;
+
+	/* Set CONFIG mode */
+	error = ravb_config(ndev);
+	if (error)
+		return error;
+
+	error = ravb_ring_init(ndev, RAVB_BE);
+	if (error)
+		return error;
+	error = ravb_ring_init(ndev, RAVB_NC);
+	if (error) {
+		ravb_ring_free(ndev, RAVB_BE);
+		return error;
+	}
+
+	/* Descriptor format */
+	ravb_ring_format(ndev, RAVB_BE);
+	ravb_ring_format(ndev, RAVB_NC);
+
+	info->ravb_ops->dmac_init(ndev);
 
 	/* Setting the control will start the AVB-DMAC process. */
 	ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_OPERATION);
@@ -2018,6 +2034,8 @@ static const struct ravb_ops ravb_gen3_ops = {
 	.ring_free = ravb_ring_free_rx,
 	.ring_format = ravb_ring_format_rx,
 	.alloc_rx_desc = ravb_alloc_rx_desc,
+	.emac_init = ravb_emac_init_ex,
+	.dmac_init = ravb_dmac_init_ex,
 };
 
 static const struct ravb_drv_data ravb_gen3_data = {
-- 
2.17.1

Re: [PATCH net-next 12/18] ravb: Factorise {emac,dmac} init function

[Sergei Shtylyov]

Hello!

On 7/22/21 5:13 PM, Biju Das wrote:

> The R-Car AVB module has Magic packet detection, multiple irq's and
> timestamp enable features which is not present on RZ/G2L Gigabit
                                   ^ are

> Ethernet module. Factorise emac and dmac initialization function to
> support the later SoC.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb.h      |  2 +
>  drivers/net/ethernet/renesas/ravb_main.c | 58 ++++++++++++++++--------
>  2 files changed, 40 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index d82bfa6e57c1..4d5910dcda86 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -992,6 +992,8 @@ struct ravb_ops {
>  	void (*ring_free)(struct net_device *ndev, int q);
>  	void (*ring_format)(struct net_device *ndev, int q);
>  	bool (*alloc_rx_desc)(struct net_device *ndev, int q);
> +	void (*emac_init)(struct net_device *ndev);
> +	void (*dmac_init)(struct net_device *ndev);
>  };
>  
>  struct ravb_drv_data {
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 3d0f6598b936..e200114376e4 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -454,7 +454,7 @@ static int ravb_ring_init(struct net_device *ndev, int q)
>  }
>  
>  /* E-MAC init function */
> -static void ravb_emac_init(struct net_device *ndev)
> +static void ravb_emac_init_ex(struct net_device *ndev)
>  {
>  	/* Receive frame limit set register */
>  	ravb_write(ndev, ndev->mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN, RFLR);
> @@ -480,30 +480,19 @@ static void ravb_emac_init(struct net_device *ndev)
>  	ravb_write(ndev, ECSIPR_ICDIP | ECSIPR_MPDIP | ECSIPR_LCHNGIP, ECSIPR);
>  }
>  
> -/* Device init function for Ethernet AVB */

   Grr, this comment seems oudated...

> -static int ravb_dmac_init(struct net_device *ndev)
> +static void ravb_emac_init(struct net_device *ndev)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	const struct ravb_drv_data *info = priv->info;
> -	int error;
>  
> -	/* Set CONFIG mode */
> -	error = ravb_config(ndev);
> -	if (error)
> -		return error;
> -
> -	error = ravb_ring_init(ndev, RAVB_BE);
> -	if (error)
> -		return error;
> -	error = ravb_ring_init(ndev, RAVB_NC);
> -	if (error) {
> -		ravb_ring_free(ndev, RAVB_BE);
> -		return error;
> -	}
> +	info->ravb_ops->emac_init(ndev);
> +}

   The whole ravb_emac_init() now consists only of a single method call?
Why do we need it at all?

>  
> -	/* Descriptor format */
> -	ravb_ring_format(ndev, RAVB_BE);
> -	ravb_ring_format(ndev, RAVB_NC);
> +/* Device init function for Ethernet AVB */

   s/Device/DMAC/. Or this comment shouldn't have been moved.

> +static void ravb_dmac_init_ex(struct net_device *ndev)

   Please no _ex suffixes -- reminds me of Windoze too much. :-)

> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +	const struct ravb_drv_data *info = priv->info;
>  
>  	/* Set AVB RX */
>  	ravb_write(ndev,
> @@ -530,6 +519,33 @@ static int ravb_dmac_init(struct net_device *ndev)
>  	ravb_write(ndev, RIC2_QFE0 | RIC2_QFE1 | RIC2_RFFE, RIC2);
>  	/* Frame transmitted, timestamp FIFO updated */
>  	ravb_write(ndev, TIC_FTE0 | TIC_FTE1 | TIC_TFUE, TIC);
> +}
> +
> +static int ravb_dmac_init(struct net_device *ndev)
> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +	const struct ravb_drv_data *info = priv->info;
> +	int error;
> +
> +	/* Set CONFIG mode */
> +	error = ravb_config(ndev);
> +	if (error)
> +		return error;
> +
> +	error = ravb_ring_init(ndev, RAVB_BE);
> +	if (error)
> +		return error;
> +	error = ravb_ring_init(ndev, RAVB_NC);
> +	if (error) {
> +		ravb_ring_free(ndev, RAVB_BE);
> +		return error;
> +	}
> +
> +	/* Descriptor format */
> +	ravb_ring_format(ndev, RAVB_BE);
> +	ravb_ring_format(ndev, RAVB_NC);
> +
> +	info->ravb_ops->dmac_init(ndev);
>  
>  	/* Setting the control will start the AVB-DMAC process. */
>  	ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_OPERATION);
> @@ -2018,6 +2034,8 @@ static const struct ravb_ops ravb_gen3_ops = {
>  	.ring_free = ravb_ring_free_rx,
>  	.ring_format = ravb_ring_format_rx,
>  	.alloc_rx_desc = ravb_alloc_rx_desc,
> +	.emac_init = ravb_emac_init_ex,
> +	.dmac_init = ravb_dmac_init_ex,

   Hmm, why not also gen2?!

>  };
[...]

MBR, Sergei

RE: [PATCH net-next 12/18] ravb: Factorise {emac,dmac} init function

[Biju Das]

Hi Sergei,

Thanks for the feedback.

> <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Subject: Re: [PATCH net-next 12/18] ravb: Factorise {emac,dmac} init
> function
> 
> Hello!
> 
> On 7/22/21 5:13 PM, Biju Das wrote:
> 
> > The R-Car AVB module has Magic packet detection, multiple irq's and
> > timestamp enable features which is not present on RZ/G2L Gigabit
>                                    ^ are

OK. Will fix this in next patch set.

> 
> > Ethernet module. Factorise emac and dmac initialization function to
> > support the later SoC.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/net/ethernet/renesas/ravb.h      |  2 +
> >  drivers/net/ethernet/renesas/ravb_main.c | 58
> > ++++++++++++++++--------
> >  2 files changed, 40 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb.h
> > b/drivers/net/ethernet/renesas/ravb.h
> > index d82bfa6e57c1..4d5910dcda86 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> > @@ -992,6 +992,8 @@ struct ravb_ops {
> >  	void (*ring_free)(struct net_device *ndev, int q);
> >  	void (*ring_format)(struct net_device *ndev, int q);
> >  	bool (*alloc_rx_desc)(struct net_device *ndev, int q);
> > +	void (*emac_init)(struct net_device *ndev);
> > +	void (*dmac_init)(struct net_device *ndev);
> >  };
> >
> >  struct ravb_drv_data {
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index 3d0f6598b936..e200114376e4 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -454,7 +454,7 @@ static int ravb_ring_init(struct net_device *ndev,
> > int q)  }
> >
> >  /* E-MAC init function */
> > -static void ravb_emac_init(struct net_device *ndev)
> > +static void ravb_emac_init_ex(struct net_device *ndev)
> >  {
> >  	/* Receive frame limit set register */
> >  	ravb_write(ndev, ndev->mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN,
> > RFLR); @@ -480,30 +480,19 @@ static void ravb_emac_init(struct
> net_device *ndev)
> >  	ravb_write(ndev, ECSIPR_ICDIP | ECSIPR_MPDIP | ECSIPR_LCHNGIP,
> > ECSIPR);  }
> >
> > -/* Device init function for Ethernet AVB */
> 
>    Grr, this comment seems oudated...

OK.
> 
> > -static int ravb_dmac_init(struct net_device *ndev)
> > +static void ravb_emac_init(struct net_device *ndev)
> >  {
> >  	struct ravb_private *priv = netdev_priv(ndev);
> >  	const struct ravb_drv_data *info = priv->info;
> > -	int error;
> >
> > -	/* Set CONFIG mode */
> > -	error = ravb_config(ndev);
> > -	if (error)
> > -		return error;
> > -
> > -	error = ravb_ring_init(ndev, RAVB_BE);
> > -	if (error)
> > -		return error;
> > -	error = ravb_ring_init(ndev, RAVB_NC);
> > -	if (error) {
> > -		ravb_ring_free(ndev, RAVB_BE);
> > -		return error;
> > -	}
> > +	info->ravb_ops->emac_init(ndev);
> > +}
> 
>    The whole ravb_emac_init() now consists only of a single method call?
> Why do we need it at all?

OK will assign info->emac_init with ravb_emac_init, so GbEthernet just need to
fill emac_init function. I will remove the function "ravb_emac_init_ex".


> 
> >
> > -	/* Descriptor format */
> > -	ravb_ring_format(ndev, RAVB_BE);
> > -	ravb_ring_format(ndev, RAVB_NC);
> > +/* Device init function for Ethernet AVB */
> 
>    s/Device/DMAC/. Or this comment shouldn't have been moved.

OK.

> 
> > +static void ravb_dmac_init_ex(struct net_device *ndev)
> 
>    Please no _ex suffixes -- reminds me of Windoze too much. :-)

OK. Will change it to ravb_device_init

Regards,
Biju

> 
> > +{
> > +	struct ravb_private *priv = netdev_priv(ndev);
> > +	const struct ravb_drv_data *info = priv->info;
> >
> >  	/* Set AVB RX */
> >  	ravb_write(ndev,
> > @@ -530,6 +519,33 @@ static int ravb_dmac_init(struct net_device *ndev)
> >  	ravb_write(ndev, RIC2_QFE0 | RIC2_QFE1 | RIC2_RFFE, RIC2);
> >  	/* Frame transmitted, timestamp FIFO updated */
> >  	ravb_write(ndev, TIC_FTE0 | TIC_FTE1 | TIC_TFUE, TIC);
> > +}
> > +
> > +static int ravb_dmac_init(struct net_device *ndev) {
> > +	struct ravb_private *priv = netdev_priv(ndev);
> > +	const struct ravb_drv_data *info = priv->info;
> > +	int error;
> > +
> > +	/* Set CONFIG mode */
> > +	error = ravb_config(ndev);
> > +	if (error)
> > +		return error;
> > +
> > +	error = ravb_ring_init(ndev, RAVB_BE);
> > +	if (error)
> > +		return error;
> > +	error = ravb_ring_init(ndev, RAVB_NC);
> > +	if (error) {
> > +		ravb_ring_free(ndev, RAVB_BE);
> > +		return error;
> > +	}
> > +
> > +	/* Descriptor format */
> > +	ravb_ring_format(ndev, RAVB_BE);
> > +	ravb_ring_format(ndev, RAVB_NC);
> > +
> > +	info->ravb_ops->dmac_init(ndev);
> >
> >  	/* Setting the control will start the AVB-DMAC process. */
> >  	ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_OPERATION); @@ -2018,6
> > +2034,8 @@ static const struct ravb_ops ravb_gen3_ops = {
> >  	.ring_free = ravb_ring_free_rx,
> >  	.ring_format = ravb_ring_format_rx,
> >  	.alloc_rx_desc = ravb_alloc_rx_desc,
> > +	.emac_init = ravb_emac_init_ex,
> > +	.dmac_init = ravb_dmac_init_ex,
> 
>    Hmm, why not also gen2?!

Re: [PATCH net-next 12/18] ravb: Factorise {emac,dmac} init function

[Sergey Shtylyov]

On 8/20/21 6:42 PM, Biju Das wrote:

[...]
>>> The R-Car AVB module has Magic packet detection, multiple irq's and
>>> timestamp enable features which is not present on RZ/G2L Gigabit
>>                                    ^ are
> 
> OK. Will fix this in next patch set.
> 
>>
>>> Ethernet module. Factorise emac and dmac initialization function to
>>> support the later SoC.
>>>
>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>>> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>> ---
>>>  drivers/net/ethernet/renesas/ravb.h      |  2 +
>>>  drivers/net/ethernet/renesas/ravb_main.c | 58
>>> ++++++++++++++++--------
>>>  2 files changed, 40 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb.h
>>> b/drivers/net/ethernet/renesas/ravb.h
>>> index d82bfa6e57c1..4d5910dcda86 100644
>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>> @@ -992,6 +992,8 @@ struct ravb_ops {
>>>  	void (*ring_free)(struct net_device *ndev, int q);
>>>  	void (*ring_format)(struct net_device *ndev, int q);
>>>  	bool (*alloc_rx_desc)(struct net_device *ndev, int q);
>>> +	void (*emac_init)(struct net_device *ndev);
>>> +	void (*dmac_init)(struct net_device *ndev);
>>>  };
>>>
>>>  struct ravb_drv_data {
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 3d0f6598b936..e200114376e4 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -454,7 +454,7 @@ static int ravb_ring_init(struct net_device *ndev,
>>> int q)  }
>>>
>>>  /* E-MAC init function */
>>> -static void ravb_emac_init(struct net_device *ndev)
>>> +static void ravb_emac_init_ex(struct net_device *ndev)
>>>  {
>>>  	/* Receive frame limit set register */
>>>  	ravb_write(ndev, ndev->mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN,
>>> RFLR); @@ -480,30 +480,19 @@ static void ravb_emac_init(struct
>> net_device *ndev)
>>>  	ravb_write(ndev, ECSIPR_ICDIP | ECSIPR_MPDIP | ECSIPR_LCHNGIP,
>>> ECSIPR);  }
>>>
>>> -/* Device init function for Ethernet AVB */
>>
>>    Grr, this comment seems oudated...
> 
> OK.

   Just don't move the comment. :-)

>>> -static int ravb_dmac_init(struct net_device *ndev)
>>> +static void ravb_emac_init(struct net_device *ndev)
>>>  {
>>>  	struct ravb_private *priv = netdev_priv(ndev);
>>>  	const struct ravb_drv_data *info = priv->info;
>>> -	int error;
>>>
>>> -	/* Set CONFIG mode */
>>> -	error = ravb_config(ndev);
>>> -	if (error)
>>> -		return error;
>>> -
>>> -	error = ravb_ring_init(ndev, RAVB_BE);
>>> -	if (error)
>>> -		return error;
>>> -	error = ravb_ring_init(ndev, RAVB_NC);
>>> -	if (error) {
>>> -		ravb_ring_free(ndev, RAVB_BE);
>>> -		return error;
>>> -	}
>>> +	info->ravb_ops->emac_init(ndev);
>>> +}
>>
>>    The whole ravb_emac_init() now consists only of a single method call?
>> Why do we need it at all?
> 
> OK will assign info->emac_init with ravb_emac_init, so GbEthernet just need to
> fill emac_init function. I will remove the function "ravb_emac_init_ex".

  Will the EMAC init methods differ so much as to we should provide 2 separate
implementations?

[...]
>>> +static void ravb_dmac_init_ex(struct net_device *ndev)
>>
>>    Please no _ex suffixes -- reminds me of Windoze too much. :-)
> 
> OK. Will change it to ravb_device_init

   Ugh! Why not leave it named ravb_dmac_init()?

> Regards,
> Biju
> 
>>
>>> +{
>>> +	struct ravb_private *priv = netdev_priv(ndev);
>>> +	const struct ravb_drv_data *info = priv->info;
>>>
>>>  	/* Set AVB RX */
>>>  	ravb_write(ndev,
>>> @@ -530,6 +519,33 @@ static int ravb_dmac_init(struct net_device *ndev)
>>>  	ravb_write(ndev, RIC2_QFE0 | RIC2_QFE1 | RIC2_RFFE, RIC2);
>>>  	/* Frame transmitted, timestamp FIFO updated */
>>>  	ravb_write(ndev, TIC_FTE0 | TIC_FTE1 | TIC_TFUE, TIC);
>>> +}
>>> +
>>> +static int ravb_dmac_init(struct net_device *ndev) {
>>> +	struct ravb_private *priv = netdev_priv(ndev);
>>> +	const struct ravb_drv_data *info = priv->info;
>>> +	int error;
>>> +
>>> +	/* Set CONFIG mode */
>>> +	error = ravb_config(ndev);
>>> +	if (error)
>>> +		return error;
>>> +
>>> +	error = ravb_ring_init(ndev, RAVB_BE);
>>> +	if (error)
>>> +		return error;
>>> +	error = ravb_ring_init(ndev, RAVB_NC);
>>> +	if (error) {
>>> +		ravb_ring_free(ndev, RAVB_BE);
>>> +		return error;
>>> +	}
>>> +
>>> +	/* Descriptor format */
>>> +	ravb_ring_format(ndev, RAVB_BE);
>>> +	ravb_ring_format(ndev, RAVB_NC);
>>> +
>>> +	info->ravb_ops->dmac_init(ndev);
>>>
>>>  	/* Setting the control will start the AVB-DMAC process. */
>>>  	ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_OPERATION); @@ -2018,6
>>> +2034,8 @@ static const struct ravb_ops ravb_gen3_ops = {
>>>  	.ring_free = ravb_ring_free_rx,
>>>  	.ring_format = ravb_ring_format_rx,
>>>  	.alloc_rx_desc = ravb_alloc_rx_desc,
>>> +	.emac_init = ravb_emac_init_ex,
>>> +	.dmac_init = ravb_dmac_init_ex,
>>
>>    Hmm, why not also gen2?!

   The question remained unreplied?... :-/

MBR, Sergey 

RE: [PATCH net-next 12/18] ravb: Factorise {emac,dmac} init function

[Biju Das]

Hi Sergei,

> Subject: Re: [PATCH net-next 12/18] ravb: Factorise {emac,dmac} init
> function
> 
> On 8/20/21 6:42 PM, Biju Das wrote:
> 
> [...]
> >>> The R-Car AVB module has Magic packet detection, multiple irq's and
> >>> timestamp enable features which is not present on RZ/G2L Gigabit
> >>                                    ^ are
> >
> > OK. Will fix this in next patch set.
> >
> >>
> >>> Ethernet module. Factorise emac and dmac initialization function to
> >>> support the later SoC.
> >>>
> >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >>> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>> ---
> >>>  drivers/net/ethernet/renesas/ravb.h      |  2 +
> >>>  drivers/net/ethernet/renesas/ravb_main.c | 58
> >>> ++++++++++++++++--------
> >>>  2 files changed, 40 insertions(+), 20 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/renesas/ravb.h
> >>> b/drivers/net/ethernet/renesas/ravb.h
> >>> index d82bfa6e57c1..4d5910dcda86 100644
> >>> --- a/drivers/net/ethernet/renesas/ravb.h
> >>> +++ b/drivers/net/ethernet/renesas/ravb.h
> >>> @@ -992,6 +992,8 @@ struct ravb_ops {
> >>>  	void (*ring_free)(struct net_device *ndev, int q);
> >>>  	void (*ring_format)(struct net_device *ndev, int q);
> >>>  	bool (*alloc_rx_desc)(struct net_device *ndev, int q);
> >>> +	void (*emac_init)(struct net_device *ndev);
> >>> +	void (*dmac_init)(struct net_device *ndev);
> >>>  };
> >>>
> >>>  struct ravb_drv_data {
> >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> >>> b/drivers/net/ethernet/renesas/ravb_main.c
> >>> index 3d0f6598b936..e200114376e4 100644
> >>> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >>> @@ -454,7 +454,7 @@ static int ravb_ring_init(struct net_device
> >>> *ndev, int q)  }
> >>>
> >>>  /* E-MAC init function */
> >>> -static void ravb_emac_init(struct net_device *ndev)
> >>> +static void ravb_emac_init_ex(struct net_device *ndev)
> >>>  {
> >>>  	/* Receive frame limit set register */
> >>>  	ravb_write(ndev, ndev->mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN,
> >>> RFLR); @@ -480,30 +480,19 @@ static void ravb_emac_init(struct
> >> net_device *ndev)
> >>>  	ravb_write(ndev, ECSIPR_ICDIP | ECSIPR_MPDIP | ECSIPR_LCHNGIP,
> >>> ECSIPR);  }
> >>>
> >>> -/* Device init function for Ethernet AVB */
> >>
> >>    Grr, this comment seems oudated...
> >
> > OK.
> 
>    Just don't move the comment. :-)
> 
> >>> -static int ravb_dmac_init(struct net_device *ndev)
> >>> +static void ravb_emac_init(struct net_device *ndev)
> >>>  {
> >>>  	struct ravb_private *priv = netdev_priv(ndev);
> >>>  	const struct ravb_drv_data *info = priv->info;
> >>> -	int error;
> >>>
> >>> -	/* Set CONFIG mode */
> >>> -	error = ravb_config(ndev);
> >>> -	if (error)
> >>> -		return error;
> >>> -
> >>> -	error = ravb_ring_init(ndev, RAVB_BE);
> >>> -	if (error)
> >>> -		return error;
> >>> -	error = ravb_ring_init(ndev, RAVB_NC);
> >>> -	if (error) {
> >>> -		ravb_ring_free(ndev, RAVB_BE);
> >>> -		return error;
> >>> -	}
> >>> +	info->ravb_ops->emac_init(ndev);
> >>> +}
> >>
> >>    The whole ravb_emac_init() now consists only of a single method
> call?
> >> Why do we need it at all?
> >
> > OK will assign info->emac_init with ravb_emac_init, so GbEthernet just
> > need to fill emac_init function. I will remove the function
> "ravb_emac_init_ex".
> 
>   Will the EMAC init methods differ so much as to we should provide 2
> separate implementations?
> 
> [...]
> >>> +static void ravb_dmac_init_ex(struct net_device *ndev)
> >>
> >>    Please no _ex suffixes -- reminds me of Windoze too much. :-)
> >
> > OK. Will change it to ravb_device_init
> 
>    Ugh! Why not leave it named ravb_dmac_init()?

Please see [1] below and also planning to send another 10 small incremental patches,
So that it is clear to every one.

> 
> > Regards,
> > Biju
> >
> >>
> >>> +{
> >>> +	struct ravb_private *priv = netdev_priv(ndev);
> >>> +	const struct ravb_drv_data *info = priv->info;
> >>>
> >>>  	/* Set AVB RX */
> >>>  	ravb_write(ndev,
> >>> @@ -530,6 +519,33 @@ static int ravb_dmac_init(struct net_device
> *ndev)
> >>>  	ravb_write(ndev, RIC2_QFE0 | RIC2_QFE1 | RIC2_RFFE, RIC2);
> >>>  	/* Frame transmitted, timestamp FIFO updated */
> >>>  	ravb_write(ndev, TIC_FTE0 | TIC_FTE1 | TIC_TFUE, TIC);
> >>> +}
> >>> +
> >>> +static int ravb_dmac_init(struct net_device *ndev) {
> >>> +	struct ravb_private *priv = netdev_priv(ndev);
> >>> +	const struct ravb_drv_data *info = priv->info;
> >>> +	int error;
> >>> +
> >>> +	/* Set CONFIG mode */
> >>> +	error = ravb_config(ndev);
> >>> +	if (error)
> >>> +		return error;
> >>> +
> >>> +	error = ravb_ring_init(ndev, RAVB_BE);
> >>> +	if (error)
> >>> +		return error;
> >>> +	error = ravb_ring_init(ndev, RAVB_NC);
> >>> +	if (error) {
> >>> +		ravb_ring_free(ndev, RAVB_BE);
> >>> +		return error;
> >>> +	}
> >>> +
> >>> +	/* Descriptor format */
> >>> +	ravb_ring_format(ndev, RAVB_BE);
> >>> +	ravb_ring_format(ndev, RAVB_NC);
> >>> +
> >>> +	info->ravb_ops->dmac_init(ndev);
> >>>
> >>>  	/* Setting the control will start the AVB-DMAC process. */
> >>>  	ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_OPERATION); @@ -2018,6
> >>> +2034,8 @@ static const struct ravb_ops ravb_gen3_ops = {
> >>>  	.ring_free = ravb_ring_free_rx,
> >>>  	.ring_format = ravb_ring_format_rx,
> >>>  	.alloc_rx_desc = ravb_alloc_rx_desc,
> >>> +	.emac_init = ravb_emac_init_ex,
> >>> +	.dmac_init = ravb_dmac_init_ex,
> >>
> >>    Hmm, why not also gen2?!
> 
>    The question remained unreplied?... :-/

ravb_ops for gen3 and gen2 same. But RZ/G2L have different function pointers[1].

As agreed on next patchset, we are avoiding indirection and it will be hw_ifo.
In that case there will be 1 entry in gen3 and 1 entry in gen2. Hope this clears
Your doubts.

[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210722141351.13668-18-biju.das.jz@bp.renesas.com/

Cheers,
Biju

[PATCH net-next 13/18] ravb: Factorise ravb_rx function

[Biju Das]

R-Car uses extended descriptor where as RZ/G2L uses normal
descriptor. Factorise ravb_rx function to support the later.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  1 +
 drivers/net/ethernet/renesas/ravb_main.c | 11 ++++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 4d5910dcda86..8a35b0ca1183 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -994,6 +994,7 @@ struct ravb_ops {
 	bool (*alloc_rx_desc)(struct net_device *ndev, int q);
 	void (*emac_init)(struct net_device *ndev);
 	void (*dmac_init)(struct net_device *ndev);
+	bool (*receive)(struct net_device *ndev, int *quota, int q);
 };
 
 struct ravb_drv_data {
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index e200114376e4..a0f19c6f8833 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -607,7 +607,7 @@ static void ravb_rx_csum(struct sk_buff *skb)
 }
 
 /* Packet receive function for Ethernet AVB */
-static bool ravb_rx(struct net_device *ndev, int *quota, int q)
+static bool ravb_ex_rx(struct net_device *ndev, int *quota, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	int entry = priv->cur_rx[q] % priv->num_rx_ring[q];
@@ -722,6 +722,14 @@ static bool ravb_rx(struct net_device *ndev, int *quota, int q)
 	return boguscnt <= 0;
 }
 
+static bool ravb_rx(struct net_device *ndev, int *quota, int q)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
+
+	return info->ravb_ops->receive(ndev, quota, q);
+}
+
 static void ravb_rcv_snd_disable(struct net_device *ndev)
 {
 	/* Disable TX and RX */
@@ -2036,6 +2044,7 @@ static const struct ravb_ops ravb_gen3_ops = {
 	.alloc_rx_desc = ravb_alloc_rx_desc,
 	.emac_init = ravb_emac_init_ex,
 	.dmac_init = ravb_dmac_init_ex,
+	.receive = ravb_ex_rx,
 };
 
 static const struct ravb_drv_data ravb_gen3_data = {
-- 
2.17.1

[PATCH net-next 14/18] ravb: Factorise ravb_adjust_link function

[Biju Das]

R-Car supports 100 and 1000 Mbs transfer speed where as RZ/G2L
in addition support 10Mbps. Factorise ravb_adjust_link function
in order to support 10Mbps speed.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      | 1 +
 drivers/net/ethernet/renesas/ravb_main.c | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 8a35b0ca1183..4725be4e85d0 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -995,6 +995,7 @@ struct ravb_ops {
 	void (*emac_init)(struct net_device *ndev);
 	void (*dmac_init)(struct net_device *ndev);
 	bool (*receive)(struct net_device *ndev, int *quota, int q);
+	void (*set_rate)(struct net_device *ndev);
 };
 
 struct ravb_drv_data {
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index a0f19c6f8833..9d5ccd8f9bb1 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1049,6 +1049,7 @@ static int ravb_poll(struct napi_struct *napi, int budget)
 static void ravb_adjust_link(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
 	struct phy_device *phydev = ndev->phydev;
 	bool new_state = false;
 	unsigned long flags;
@@ -1063,7 +1064,7 @@ static void ravb_adjust_link(struct net_device *ndev)
 		if (phydev->speed != priv->speed) {
 			new_state = true;
 			priv->speed = phydev->speed;
-			ravb_set_rate(ndev);
+			info->ravb_ops->set_rate(ndev);
 		}
 		if (!priv->link) {
 			ravb_modify(ndev, ECMR, ECMR_TXF, 0);
@@ -2045,6 +2046,7 @@ static const struct ravb_ops ravb_gen3_ops = {
 	.emac_init = ravb_emac_init_ex,
 	.dmac_init = ravb_dmac_init_ex,
 	.receive = ravb_ex_rx,
+	.set_rate = ravb_set_rate,
 };
 
 static const struct ravb_drv_data ravb_gen3_data = {
-- 
2.17.1

[PATCH net-next 15/18] ravb: Factorise ravb_set_features

[Biju Das]

RZ/G2L supports HW checksum. Factorise ravb_set_features
to support this feature.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  1 +
 drivers/net/ethernet/renesas/ravb_main.c | 14 ++++++++++++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 4725be4e85d0..f1de095f21d9 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -996,6 +996,7 @@ struct ravb_ops {
 	void (*dmac_init)(struct net_device *ndev);
 	bool (*receive)(struct net_device *ndev, int *quota, int q);
 	void (*set_rate)(struct net_device *ndev);
+	int (*set_features)(struct net_device *ndev, netdev_features_t features);
 };
 
 struct ravb_drv_data {
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 9d5ccd8f9bb1..5a375ac962a0 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1966,8 +1966,8 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
 	spin_unlock_irqrestore(&priv->lock, flags);
 }
 
-static int ravb_set_features(struct net_device *ndev,
-			     netdev_features_t features)
+static int ravb_set_features_rx_csum(struct net_device *ndev,
+				     netdev_features_t features)
 {
 	netdev_features_t changed = ndev->features ^ features;
 
@@ -1979,6 +1979,15 @@ static int ravb_set_features(struct net_device *ndev,
 	return 0;
 }
 
+static int ravb_set_features(struct net_device *ndev,
+			     netdev_features_t features)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
+
+	return info->ravb_ops->set_features(ndev, features);
+}
+
 static const struct net_device_ops ravb_netdev_ops = {
 	.ndo_open		= ravb_open,
 	.ndo_stop		= ravb_close,
@@ -2047,6 +2056,7 @@ static const struct ravb_ops ravb_gen3_ops = {
 	.dmac_init = ravb_dmac_init_ex,
 	.receive = ravb_ex_rx,
 	.set_rate = ravb_set_rate,
+	.set_features = ravb_set_features_rx_csum,
 };
 
 static const struct ravb_drv_data ravb_gen3_data = {
-- 
2.17.1

[PATCH net-next 16/18] ravb: Add reset support

[Biju Das]

Reset support is present on R-Car. Let's support it, if it is
available.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  1 +
 drivers/net/ethernet/renesas/ravb_main.c | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index f1de095f21d9..af06e849db47 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1067,6 +1067,7 @@ struct ravb_private {
 	int num_tx_desc;		/* TX descriptors per packet */
 
 	const struct ravb_drv_data *info;
+	struct reset_control *rstc;
 };
 
 static inline u32 ravb_read(struct net_device *ndev, enum ravb_reg reg)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 5a375ac962a0..5a83dd83c635 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -29,6 +29,7 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/sys_soc.h>
+#include <linux/reset.h>
 
 #include <asm/div64.h>
 
@@ -2204,6 +2205,7 @@ static int ravb_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
 	const struct ravb_drv_data *info;
+	struct reset_control *rstc;
 	struct ravb_private *priv;
 	struct net_device *ndev;
 	int error, irq, q;
@@ -2216,6 +2218,11 @@ static int ravb_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	rstc = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
+	if (IS_ERR(rstc))
+		return dev_err_probe(&pdev->dev, PTR_ERR(rstc),
+				     "failed to get cpg reset\n");
+
 	ndev = alloc_etherdev_mqs(sizeof(struct ravb_private),
 				  NUM_TX_QUEUE, NUM_RX_QUEUE);
 	if (!ndev)
@@ -2226,6 +2233,7 @@ static int ravb_probe(struct platform_device *pdev)
 	ndev->features = info->net_features;
 	ndev->hw_features = info->net_hw_features;
 
+	reset_control_deassert(rstc);
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 
@@ -2243,6 +2251,7 @@ static int ravb_probe(struct platform_device *pdev)
 
 	priv = netdev_priv(ndev);
 	priv->info = info;
+	priv->rstc = rstc;
 	priv->ndev = ndev;
 	priv->pdev = pdev;
 	priv->num_tx_ring[RAVB_BE] = BE_TX_RING_SIZE;
@@ -2414,6 +2423,7 @@ static int ravb_probe(struct platform_device *pdev)
 
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
+	reset_control_assert(rstc);
 	return error;
 }
 
@@ -2439,6 +2449,7 @@ static int ravb_remove(struct platform_device *pdev)
 	netif_napi_del(&priv->napi[RAVB_BE]);
 	ravb_mdio_release(priv);
 	pm_runtime_disable(&pdev->dev);
+	reset_control_assert(priv->rstc);
 	free_netdev(ndev);
 	platform_set_drvdata(pdev, NULL);
 
-- 
2.17.1

[PATCH net-next 17/18] ravb: Add GbEthernet driver support

[Biju Das]

Add Gigabit Ethernet driver support found on RZ/G2L.

The Gigabit Etherner IP consists of Ethernet controller (E-MAC),
Internal TCP/IP Offload Engine (TOE) and Dedicated Direct memory
access controller (DMAC).

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  84 ++++-
 drivers/net/ethernet/renesas/ravb_main.c | 436 ++++++++++++++++++++++-
 2 files changed, 513 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index af06e849db47..6d730f479a37 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -81,6 +81,7 @@ enum ravb_reg {
 	RQC3	= 0x00A0,
 	RQC4	= 0x00A4,
 	RPC	= 0x00B0,
+	RTC	= 0x00B4,	/* RZ/G2L only */
 	UFCW	= 0x00BC,
 	UFCS	= 0x00C0,
 	UFCV0	= 0x00C4,
@@ -156,6 +157,7 @@ enum ravb_reg {
 	TIS	= 0x037C,
 	ISS	= 0x0380,
 	CIE	= 0x0384,	/* R-Car Gen3 only */
+	RIC3	= 0x0388,	/* RZ/G2L only */
 	GCCR	= 0x0390,
 	GMTT	= 0x0394,
 	GPTC	= 0x0398,
@@ -187,19 +189,28 @@ enum ravb_reg {
 	PIR	= 0x0520,
 	PSR	= 0x0528,
 	PIPR	= 0x052c,
+	CXR31	= 0x0530,	/* Documented for RZ/G2L only */
+	CXR35	= 0x0540,	/* Documented for RZ/G2L only */
 	MPR	= 0x0558,
 	PFTCR	= 0x055c,
 	PFRCR	= 0x0560,
 	GECMR	= 0x05b0,
 	MAHR	= 0x05c0,
 	MALR	= 0x05c8,
-	TROCR	= 0x0700,	/* R-Car Gen3 only */
+	TROCR	= 0x0700,	/* R-Car Gen3 and RZ/G2L only */
+	CXR41	= 0x0708,	/* Documented for RZ/G2L only */
+	CXR42	= 0x0710,	/* Documented for RZ/G2L only */
 	CEFCR	= 0x0740,
 	FRECR	= 0x0748,
 	TSFRCR	= 0x0750,
 	TLFRCR	= 0x0758,
 	RFCR	= 0x0760,
+	CXR55	= 0x0768,	/* Documented for RZ/G2L only */
+	CXR56	= 0x0770,	/* Documented for RZ/G2L only */
 	MAFCR	= 0x0778,
+	CSR0     = 0x0800,	/* Documented for RZ/G2L only */
+	CSR1     = 0x0804,	/* Documented for RZ/G2L only */
+	CSR2     = 0x0808,	/* Documented for RZ/G2L only */
 };
 
 
@@ -804,16 +815,21 @@ enum TID_BIT {
 enum ECMR_BIT {
 	ECMR_PRM	= 0x00000001,
 	ECMR_DM		= 0x00000002,
+	ECMR_LPM	= 0x00000010,	/* Documented for RZ/G2L only */
 	ECMR_TE		= 0x00000020,
 	ECMR_RE		= 0x00000040,
 	ECMR_MPDE	= 0x00000200,
+	ECMR_CER	= 0x00001000,	/* Documented for RZ/G2L only */
 	ECMR_TXF	= 0x00010000,	/* Documented for R-Car Gen3 only */
 	ECMR_RXF	= 0x00020000,
 	ECMR_PFR	= 0x00040000,
 	ECMR_ZPF	= 0x00080000,	/* Documented for R-Car Gen3 only */
 	ECMR_RZPF	= 0x00100000,
 	ECMR_DPAD	= 0x00200000,
+	ECMR_CXSER	= 0x00400000,	/* Documented for RZ/G2L only */
 	ECMR_RCSC	= 0x00800000,
+	ECMR_TCPT	= 0x01000000,	/* Documented for RZ/G2L only */
+	ECMR_RCPT	= 0x02000000,	/* Documented for RZ/G2L only */
 	ECMR_TRCCM	= 0x04000000,
 };
 
@@ -823,6 +839,7 @@ enum ECSR_BIT {
 	ECSR_MPD	= 0x00000002,
 	ECSR_LCHNG	= 0x00000004,
 	ECSR_PHYI	= 0x00000008,
+	ECSR_PFRI	= 0x00000010,
 };
 
 /* ECSIPR */
@@ -857,9 +874,13 @@ enum MPR_BIT {
 
 /* GECMR */
 enum GECMR_BIT {
-	GECMR_SPEED	= 0x00000001,
-	GECMR_SPEED_100	= 0x00000000,
-	GECMR_SPEED_1000 = 0x00000001,
+	GECMR_SPEED		= 0x00000001,
+	GECMR_SPEED_100		= 0x00000000,
+	GECMR_SPEED_1000	= 0x00000001,
+	RGETH_GECMR_SPEED	= 0x00000030,
+	RGETH_GECMR_SPEED_10	= 0x00000000,
+	RGETH_GECMR_SPEED_100	= 0x00000010,
+	RGETH_GECMR_SPEED_1000	= 0x00000020,
 };
 
 /* The Ethernet AVB descriptor definitions. */
@@ -949,6 +970,54 @@ enum RAVB_QUEUE {
 	RAVB_NC,	/* Network Control Queue */
 };
 
+enum CXR31_BIT {
+	CXR31_SEL_LINK0	= 0x00000001,
+	CXR31_SEL_LINK1	= 0x00000008,
+};
+
+enum CXR35_BIT {
+	CXR35_SEL_MODIN	= 0x00000100,
+};
+
+enum CSR0_BIT {
+	CSR0_CCM	= 0x00000001,
+	CSR0_TPE	= 0x00000010,
+	CSR0_RPE	= 0x00000020,
+	CSR0_TBP	= 0x00000100,
+	CSR0_RBP	= 0x00000200,
+	CSR0_FIFOCAP	= 0x00003000,
+};
+
+enum CSR1_BIT {
+	CSR1_TIP4	= 0x00000001,
+	CSR1_TTCP4	= 0x00000010,
+	CSR1_TUDP4	= 0x00000020,
+	CSR1_TICMP4	= 0x00000040,
+	CSR1_TTCP6	= 0x00100000,
+	CSR1_TUDP6	= 0x00200000,
+	CSR1_TICMP6	= 0x00400000,
+	CSR1_THOP	= 0x01000000,
+	CSR1_TROUT	= 0x02000000,
+	CSR1_TAHD	= 0x04000000,
+	CSR1_TDHD	= 0x08000000,
+	CSR1_ALL	= 0x0F700071,
+};
+
+enum CSR2_BIT {
+	CSR2_RIP4	= 0x00000001,
+	CSR2_RTCP4	= 0x00000010,
+	CSR2_RUDP4	= 0x00000020,
+	CSR2_RICMP4	= 0x00000040,
+	CSR2_RTCP6	= 0x00100000,
+	CSR2_RUDP6	= 0x00200000,
+	CSR2_RICMP6	= 0x00400000,
+	CSR2_RHOP	= 0x01000000,
+	CSR2_RROUT	= 0x02000000,
+	CSR2_RAHD	= 0x04000000,
+	CSR2_RDHD	= 0x08000000,
+	CSR2_ALL	= 0x0F700071,
+};
+
 #define DBAT_ENTRY_NUM	22
 #define RX_QUEUE_OFFSET	4
 #define NUM_RX_QUEUE	2
@@ -956,6 +1025,9 @@ enum RAVB_QUEUE {
 
 #define RX_BUF_SZ	(2048 - ETH_FCS_LEN + sizeof(__sum16))
 
+#define RGETH_RX_BUFF_MAX 8192
+#define RGETH_RX_DESC_DATA_SIZE 4080
+
 /* TX descriptors per packet */
 #define NUM_TX_DESC_GEN2	2
 #define NUM_TX_DESC_GEN3	1
@@ -1068,6 +1140,10 @@ struct ravb_private {
 
 	const struct ravb_drv_data *info;
 	struct reset_control *rstc;
+
+	int duplex;
+	struct ravb_rx_desc *rgeth_rx_ring[NUM_RX_QUEUE];
+	struct sk_buff *rxtop_skb;
 };
 
 static inline u32 ravb_read(struct net_device *ndev, enum ravb_reg reg)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 5a83dd83c635..0378b2d26b8c 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -51,6 +51,8 @@
 #define RAVB_NO_HALF_DUPLEX		BIT(7)
 #define RAVB_OVERRIDE_MTU_CHANGE	BIT(8)
 #define RAVB_EX_RX_DESC			BIT(9)
+#define RAVB_MII_RGMII_SELECTION	BIT(10)
+#define RAVB_CARRIER_COUNTER		BIT(11)
 
 #define RAVB_PTP	(RAVB_PTP_CONFIG_ACTIVE | RAVB_PTP_CONFIG_INACTIVE)
 #define RAVB_RCAR_COMMON \
@@ -71,6 +73,10 @@
 		(RAVB_PTP_CONFIG_INACTIVE	| \
 		 RAVB_RCAR_COMMON)
 
+#define RAVB_RZ_G2L_FEATURES \
+		(RAVB_MII_RGMII_SELECTION	| \
+		 RAVB_CARRIER_COUNTER)
+
 static const char *ravb_rx_irqs[NUM_RX_QUEUE] = {
 	"ch0", /* RAVB_BE */
 	"ch1", /* RAVB_NC */
@@ -113,6 +119,23 @@ static int ravb_config(struct net_device *ndev)
 	return error;
 }
 
+static void ravb_set_rate_rgeth(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	switch (priv->speed) {
+	case 10:                /* 10BASE */
+		ravb_write(ndev, RGETH_GECMR_SPEED_10, GECMR);
+		break;
+	case 100:               /* 100BASE */
+		ravb_write(ndev, RGETH_GECMR_SPEED_100, GECMR);
+		break;
+	case 1000:              /* 1000BASE */
+		ravb_write(ndev, RGETH_GECMR_SPEED_1000, GECMR);
+		break;
+	}
+}
+
 static void ravb_set_rate(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -248,6 +271,28 @@ static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
 }
 
 /* Free skb's and DMA buffers for Ethernet AVB */
+static void ravb_ring_free_rx_rgeth(struct net_device *ndev, int q)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int ring_size;
+	int i;
+
+	for (i = 0; i < priv->num_rx_ring[q]; i++) {
+		struct ravb_rx_desc *desc = &priv->rgeth_rx_ring[q][i];
+
+		if (!dma_mapping_error(ndev->dev.parent,
+				       le32_to_cpu(desc->dptr)))
+			dma_unmap_single(ndev->dev.parent,
+					 le32_to_cpu(desc->dptr),
+					 RGETH_RX_BUFF_MAX,
+					 DMA_FROM_DEVICE);
+	}
+	ring_size = sizeof(struct ravb_rx_desc) * (priv->num_rx_ring[q] + 1);
+	dma_free_coherent(ndev->dev.parent, ring_size, priv->rgeth_rx_ring[q],
+			  priv->rx_desc_dma[q]);
+	priv->rgeth_rx_ring[q] = NULL;
+}
+
 static void ravb_ring_free_rx(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -279,7 +324,7 @@ static void ravb_ring_free(struct net_device *ndev, int q)
 	int ring_size;
 	int i;
 
-	if (priv->rx_ring[q])
+	if (priv->rx_ring[q] || priv->rgeth_rx_ring[q])
 		info->ravb_ops->ring_free(ndev, q);
 
 	if (priv->tx_ring[q]) {
@@ -312,6 +357,36 @@ static void ravb_ring_free(struct net_device *ndev, int q)
 }
 
 /* Format skb and descriptor buffer for Ethernet AVB */
+static void ravb_ring_format_rx_rgeth(struct net_device *ndev, int q)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	struct ravb_rx_desc *rx_desc;
+	int rx_ring_size = sizeof(*rx_desc) * priv->num_rx_ring[q];
+	dma_addr_t dma_addr;
+	int i;
+
+	memset(priv->rgeth_rx_ring[q], 0, rx_ring_size);
+	/* Build RX ring buffer */
+	for (i = 0; i < priv->num_rx_ring[q]; i++) {
+		/* RX descriptor */
+		rx_desc = &priv->rgeth_rx_ring[q][i];
+		rx_desc->ds_cc = cpu_to_le16(RGETH_RX_DESC_DATA_SIZE);
+		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
+					  RGETH_RX_BUFF_MAX,
+					  DMA_FROM_DEVICE);
+		/* We just set the data size to 0 for a failed mapping which
+		 * should prevent DMA from happening...
+		 */
+		if (dma_mapping_error(ndev->dev.parent, dma_addr))
+			rx_desc->ds_cc = cpu_to_le16(0);
+		rx_desc->dptr = cpu_to_le32(dma_addr);
+		rx_desc->die_dt = DT_FEMPTY;
+	}
+	rx_desc = &priv->rgeth_rx_ring[q][i];
+	rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
+	rx_desc->die_dt = DT_LINKFIX; /* type */
+}
+
 static void ravb_ring_format_rx(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -385,6 +460,19 @@ static void ravb_ring_format(struct net_device *ndev, int q)
 }
 
 /* Init skb and descriptor buffer for Ethernet AVB */
+static bool ravb_alloc_rx_desc_rgeth(struct net_device *ndev, int q)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int ring_size;
+
+	ring_size = sizeof(struct ravb_rx_desc) * (priv->num_rx_ring[q] + 1);
+
+	priv->rgeth_rx_ring[q] = dma_alloc_coherent(ndev->dev.parent, ring_size,
+						    &priv->rx_desc_dma[q],
+						    GFP_KERNEL);
+	return priv->rgeth_rx_ring[q];
+}
+
 static bool ravb_alloc_rx_desc(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -455,6 +543,37 @@ static int ravb_ring_init(struct net_device *ndev, int q)
 }
 
 /* E-MAC init function */
+static void ravb_emac_init_rgeth(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	/* Receive frame limit set register */
+	ravb_write(ndev, RGETH_RX_BUFF_MAX + ETH_FCS_LEN, RFLR);
+
+	/* PAUSE prohibition */
+	ravb_write(ndev, ECMR_ZPF | ((priv->duplex > 0) ? ECMR_DM : 0) |
+			 ECMR_TE | ECMR_RE | ECMR_RCPT |
+			 ECMR_TXF | ECMR_RXF | ECMR_PRM, ECMR);
+
+	ravb_set_rate_rgeth(ndev);
+
+	/* Set MAC address */
+	ravb_write(ndev,
+		   (ndev->dev_addr[0] << 24) | (ndev->dev_addr[1] << 16) |
+		   (ndev->dev_addr[2] << 8)  | (ndev->dev_addr[3]), MAHR);
+	ravb_write(ndev, (ndev->dev_addr[4] << 8)  | (ndev->dev_addr[5]), MALR);
+
+	/* E-MAC status register clear */
+	ravb_write(ndev, ECSR_ICD | ECSR_LCHNG | ECSR_PFRI, ECSR);
+	ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0);
+
+	/* E-MAC interrupt enable register */
+	ravb_write(ndev, ECSIPR_ICDIP, ECSIPR);
+
+	ravb_write(ndev, ravb_read(ndev, CXR31) & ~CXR31_SEL_LINK1, CXR31);
+	ravb_write(ndev, ravb_read(ndev, CXR31) | CXR31_SEL_LINK0, CXR31);
+}
+
 static void ravb_emac_init_ex(struct net_device *ndev)
 {
 	/* Receive frame limit set register */
@@ -490,6 +609,31 @@ static void ravb_emac_init(struct net_device *ndev)
 }
 
 /* Device init function for Ethernet AVB */
+static void ravb_dmac_init_rgeth(struct net_device *ndev)
+{
+	/* Set AVB RX */
+	ravb_write(ndev, 0x60000000, RCR);
+
+	/* Set Max Frame Length (RTC) */
+	ravb_write(ndev, 0x7ffc0000 | RGETH_RX_BUFF_MAX, RTC);
+
+	/* Set FIFO size */
+	ravb_write(ndev, 0x00222200, TGC);
+
+	ravb_write(ndev, 0, TCCR);
+
+	/* Frame receive */
+	ravb_write(ndev, RIC0_FRE0, RIC0);
+	/* Disable FIFO full warning */
+	ravb_write(ndev, 0x0, RIC1);
+	/* Receive FIFO full error, descriptor empty */
+	ravb_write(ndev, RIC2_QFE0 | RIC2_RFFE, RIC2);
+
+	ravb_write(ndev, 0x0, RIC3);
+
+	ravb_write(ndev, TIC_FTE0, TIC);
+}
+
 static void ravb_dmac_init_ex(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -592,6 +736,23 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)
 	}
 }
 
+static void ravb_rx_csum_rgeth(struct sk_buff *skb)
+{
+	u8 *hw_csum;
+
+	/* The hardware checksum is contained in sizeof(__sum16) (2) bytes
+	 * appended to packet data
+	 */
+	if (unlikely(skb->len < sizeof(__sum16)))
+		return;
+	hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
+
+	if (*hw_csum == 0)
+		skb->ip_summed = CHECKSUM_UNNECESSARY;
+	else
+		skb->ip_summed = CHECKSUM_NONE;
+}
+
 static void ravb_rx_csum(struct sk_buff *skb)
 {
 	u8 *hw_csum;
@@ -608,6 +769,148 @@ static void ravb_rx_csum(struct sk_buff *skb)
 }
 
 /* Packet receive function for Ethernet AVB */
+static struct sk_buff *ravb_get_skb_rgeth(struct net_device *ndev,  int q,
+					  int entry, struct ravb_rx_desc *desc)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	struct sk_buff *skb;
+
+	skb = priv->rx_skb[q][entry];
+	priv->rx_skb[q][entry] = NULL;
+	dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
+			 ALIGN(RGETH_RX_BUFF_MAX, 16), DMA_FROM_DEVICE);
+
+	return skb;
+}
+
+static bool ravb_rx_rgeth(struct net_device *ndev, int *quota, int q)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int entry = priv->cur_rx[q] % priv->num_rx_ring[q];
+	int boguscnt = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
+	struct net_device_stats *stats = &priv->stats[q];
+	struct ravb_rx_desc *desc;
+	struct sk_buff *skb;
+	dma_addr_t dma_addr;
+	u8  desc_status;
+	u8  die_dt;
+	u16 pkt_len;
+	int limit;
+
+	boguscnt = min(boguscnt, *quota);
+	limit = boguscnt;
+	desc = &priv->rgeth_rx_ring[q][entry];
+	while (desc->die_dt != DT_FEMPTY) {
+		/* Descriptor type must be checked before all other reads */
+		dma_rmb();
+		desc_status = desc->msc;
+		pkt_len = le16_to_cpu(desc->ds_cc) & RX_DS;
+
+		if (--boguscnt < 0)
+			break;
+
+		/* We use 0-byte descriptors to mark the DMA mapping errors */
+		if (!pkt_len)
+			continue;
+
+		if (desc_status & MSC_MC)
+			stats->multicast++;
+
+		if (desc_status & (MSC_CRC | MSC_RFE | MSC_RTSF | MSC_RTLF | MSC_CEEF)) {
+			stats->rx_errors++;
+			if (desc_status & MSC_CRC)
+				stats->rx_crc_errors++;
+			if (desc_status & MSC_RFE)
+				stats->rx_frame_errors++;
+			if (desc_status & (MSC_RTLF | MSC_RTSF))
+				stats->rx_length_errors++;
+			if (desc_status & MSC_CEEF)
+				stats->rx_missed_errors++;
+		} else {
+			die_dt = desc->die_dt & 0xF0;
+			switch (die_dt) {
+			case DT_FSINGLE:
+				skb = ravb_get_skb_rgeth(ndev, q, entry, desc);
+				skb_put(skb, pkt_len);
+				skb->protocol = eth_type_trans(skb, ndev);
+				if (ndev->features & NETIF_F_RXCSUM)
+					ravb_rx_csum_rgeth(skb);
+				napi_gro_receive(&priv->napi[q], skb);
+				stats->rx_packets++;
+				stats->rx_bytes += pkt_len;
+				break;
+			case DT_FSTART:
+				priv->rxtop_skb = ravb_get_skb_rgeth(ndev, q, entry, desc);
+				skb_put(priv->rxtop_skb, pkt_len);
+				break;
+			case DT_FMID:
+				skb = ravb_get_skb_rgeth(ndev, q, entry, desc);
+				skb_copy_to_linear_data_offset(priv->rxtop_skb,
+							       priv->rxtop_skb->len,
+							       skb->data,
+							       pkt_len);
+				skb_put(priv->rxtop_skb, pkt_len);
+				dev_kfree_skb(skb);
+				break;
+			case DT_FEND:
+				skb = ravb_get_skb_rgeth(ndev, q, entry, desc);
+				skb_copy_to_linear_data_offset(priv->rxtop_skb,
+							       priv->rxtop_skb->len,
+							       skb->data,
+							       pkt_len);
+				skb_put(priv->rxtop_skb, pkt_len);
+				dev_kfree_skb(skb);
+				priv->rxtop_skb->protocol =
+					eth_type_trans(priv->rxtop_skb, ndev);
+				if (ndev->features & NETIF_F_RXCSUM)
+					ravb_rx_csum_rgeth(skb);
+				napi_gro_receive(&priv->napi[q],
+						 priv->rxtop_skb);
+				stats->rx_packets++;
+				stats->rx_bytes += priv->rxtop_skb->len;
+				break;
+			}
+		}
+
+		entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
+		desc = &priv->rgeth_rx_ring[q][entry];
+	}
+
+	/* Refill the RX ring buffers. */
+	for (; priv->cur_rx[q] - priv->dirty_rx[q] > 0; priv->dirty_rx[q]++) {
+		entry = priv->dirty_rx[q] % priv->num_rx_ring[q];
+		desc = &priv->rgeth_rx_ring[q][entry];
+		desc->ds_cc = cpu_to_le16(RGETH_RX_DESC_DATA_SIZE);
+
+		if (!priv->rx_skb[q][entry]) {
+			skb = netdev_alloc_skb(ndev,
+					       RGETH_RX_BUFF_MAX + RAVB_ALIGN - 1);
+			if (!skb)
+				break;
+			ravb_set_buffer_align(skb);
+			dma_addr = dma_map_single(ndev->dev.parent,
+						  skb->data,
+						  le16_to_cpu(RGETH_RX_BUFF_MAX),
+						  DMA_FROM_DEVICE);
+			skb_checksum_none_assert(skb);
+			/* We just set the data size to 0 for a failed mapping
+			 * which should prevent DMA  from happening...
+			 */
+			if (dma_mapping_error(ndev->dev.parent, dma_addr))
+				desc->ds_cc = cpu_to_le16(0);
+			desc->dptr = cpu_to_le32(dma_addr);
+			priv->rx_skb[q][entry] = skb;
+		}
+		/* Descriptor type must be set after all the above writes */
+		dma_wmb();
+		desc->die_dt = DT_FEMPTY;
+	}
+
+	*quota -= limit - (++boguscnt);
+
+	return boguscnt <= 0;
+}
+
 static bool ravb_ex_rx(struct net_device *ndev, int *quota, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -754,6 +1057,9 @@ static int ravb_stop_dma(struct net_device *ndev)
 	if (info->features & RAVB_MULTI_TSRQ)
 		error = ravb_wait(ndev, TCCR,
 				  TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3, 0);
+	else
+		error = ravb_wait(ndev, TCCR, TCCR_TSRQ0, 0);
+
 	if (error)
 		return error;
 
@@ -1003,16 +1309,24 @@ static int ravb_poll(struct napi_struct *napi, int budget)
 	struct net_device *ndev = napi->dev;
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_drv_data *info = priv->info;
+	struct ravb_rx_desc *desc;
 	unsigned long flags;
 	int q = napi - priv->napi;
 	int mask = BIT(q);
 	int quota = budget;
+	int entry;
 
+	if (!(info->features & RAVB_EX_RX_DESC)) {
+		entry = priv->cur_rx[q] % priv->num_rx_ring[q];
+		desc = &priv->rgeth_rx_ring[q][entry];
+	}
 	/* Processing RX Descriptor Ring */
 	/* Clear RX interrupt */
 	ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0);
-	if (ravb_rx(ndev, &quota, q))
-		goto out;
+	if ((info->features & RAVB_EX_RX_DESC) || desc->die_dt != DT_FEMPTY) {
+		if (ravb_rx(ndev, &quota, q))
+			goto out;
+	}
 
 	/* Processing TX Descriptor Ring */
 	spin_lock_irqsave(&priv->lock, flags);
@@ -1046,6 +1360,18 @@ static int ravb_poll(struct napi_struct *napi, int budget)
 	return budget - quota;
 }
 
+static void ravb_set_duplex_rgeth(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	u32 ecmr = ravb_read(ndev, ECMR);
+
+	if (priv->duplex > 0)	/* Full */
+		ecmr |=  ECMR_DM;
+	else			/* Half */
+		ecmr &= ~ECMR_DM;
+	ravb_write(ndev, ecmr, ECMR);
+}
+
 /* PHY state control function */
 static void ravb_adjust_link(struct net_device *ndev)
 {
@@ -1062,6 +1388,12 @@ static void ravb_adjust_link(struct net_device *ndev)
 		ravb_rcv_snd_disable(ndev);
 
 	if (phydev->link) {
+		if (!(info->features & RAVB_NO_HALF_DUPLEX) && phydev->duplex != priv->duplex) {
+			new_state = true;
+			priv->duplex = phydev->duplex;
+			ravb_set_duplex_rgeth(ndev);
+		}
+
 		if (phydev->speed != priv->speed) {
 			new_state = true;
 			priv->speed = phydev->speed;
@@ -1076,6 +1408,8 @@ static void ravb_adjust_link(struct net_device *ndev)
 		new_state = true;
 		priv->link = 0;
 		priv->speed = 0;
+		if (!(info->features & RAVB_NO_HALF_DUPLEX))
+			priv->duplex = -1;
 	}
 
 	/* Enable TX and RX right over here, if E-MAC change is ignored */
@@ -1106,6 +1440,7 @@ static int ravb_phy_init(struct net_device *ndev)
 
 	priv->link = 0;
 	priv->speed = 0;
+	priv->duplex = -1;
 
 	/* Try connecting to PHY */
 	pn = of_parse_phandle(np, "phy-handle", 0);
@@ -1144,6 +1479,9 @@ static int ravb_phy_init(struct net_device *ndev)
 		netdev_info(ndev, "limited PHY to 100Mbit/s\n");
 	}
 
+	if (info->features & RAVB_MII_RGMII_SELECTION)
+		ravb_write(ndev, ravb_read(ndev, CXR35) | CXR35_SEL_MODIN, CXR35);
+
 	if (info->features & RAVB_NO_HALF_DUPLEX) {
 		/* 10BASE, Pause and Asym Pause is not supported */
 		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
@@ -1197,6 +1535,24 @@ static void ravb_set_msglevel(struct net_device *ndev, u32 value)
 	priv->msg_enable = value;
 }
 
+static const char ravb_gstrings_stats_rgeth[][ETH_GSTRING_LEN] = {
+	"rx_queue_0_current",
+	"tx_queue_0_current",
+	"rx_queue_0_dirty",
+	"tx_queue_0_dirty",
+	"rx_queue_0_packets",
+	"tx_queue_0_packets",
+	"rx_queue_0_bytes",
+	"tx_queue_0_bytes",
+	"rx_queue_0_mcast_packets",
+	"rx_queue_0_errors",
+	"rx_queue_0_crc_errors",
+	"rx_queue_0_frame_errors",
+	"rx_queue_0_length_errors",
+	"rx_queue_0_csum_offload_errors",
+	"rx_queue_0_over_errors",
+};
+
 static const char ravb_gstrings_stats[][ETH_GSTRING_LEN] = {
 	"rx_queue_0_current",
 	"tx_queue_0_current",
@@ -1752,6 +2108,18 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
 		ravb_write(ndev, 0, TROCR);	/* (write clear) */
 	}
 
+	if (info->features & RAVB_CARRIER_COUNTER) {
+		nstats->collisions += ravb_read(ndev, CXR41);
+		ravb_write(ndev, 0, CXR41);	/* (write clear) */
+		nstats->tx_carrier_errors += ravb_read(ndev, CXR42);
+		ravb_write(ndev, 0, CXR42);	/* (write clear) */
+
+		nstats->tx_carrier_errors += ravb_read(ndev, CXR55);
+		ravb_write(ndev, 0, CXR55);	/* (write clear) */
+		nstats->tx_carrier_errors += ravb_read(ndev, CXR56);
+		ravb_write(ndev, 0, CXR56);	/* (write clear) */
+	}
+
 	nstats->rx_packets = stats0->rx_packets + stats1->rx_packets;
 	nstats->tx_packets = stats0->tx_packets + stats1->tx_packets;
 	nstats->rx_bytes = stats0->rx_bytes + stats1->rx_bytes;
@@ -1967,6 +2335,44 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
 	spin_unlock_irqrestore(&priv->lock, flags);
 }
 
+static int ravb_set_features_rx_csum_rgeth(struct net_device *ndev,
+					   netdev_features_t features)
+{
+	netdev_features_t changed = features ^ ndev->features;
+	unsigned int reg;
+	int error;
+
+	reg = ravb_read(ndev, CSR0);
+
+	ravb_write(ndev, reg & ~(CSR0_RPE | CSR0_TPE), CSR0);
+	error = ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0);
+	if (error) {
+		ravb_write(ndev, reg, CSR0);
+		return error;
+	}
+
+	if (changed & NETIF_F_RXCSUM) {
+		if (features & NETIF_F_RXCSUM)
+			ravb_write(ndev, CSR2_ALL, CSR2);
+		else
+			ravb_write(ndev, 0, CSR2);
+	}
+
+	if (changed & NETIF_F_HW_CSUM) {
+		if (features & NETIF_F_HW_CSUM) {
+			ravb_write(ndev, CSR1_ALL, CSR1);
+			ndev->features |= NETIF_F_CSUM_MASK;
+		} else {
+			ravb_write(ndev, 0, CSR1);
+		}
+	}
+	ravb_write(ndev, reg, CSR0);
+
+	ndev->features = features;
+
+	return 0;
+}
+
 static int ravb_set_features_rx_csum(struct net_device *ndev,
 				     netdev_features_t features)
 {
@@ -2060,6 +2466,17 @@ static const struct ravb_ops ravb_gen3_ops = {
 	.set_features = ravb_set_features_rx_csum,
 };
 
+static const struct ravb_ops ravb_ops_rgeth = {
+	.ring_free = ravb_ring_free_rx_rgeth,
+	.ring_format = ravb_ring_format_rx_rgeth,
+	.alloc_rx_desc = ravb_alloc_rx_desc_rgeth,
+	.emac_init = ravb_emac_init_rgeth,
+	.dmac_init = ravb_dmac_init_rgeth,
+	.receive = ravb_rx_rgeth,
+	.set_rate = ravb_set_rate_rgeth,
+	.set_features = ravb_set_features_rx_csum_rgeth,
+};
+
 static const struct ravb_drv_data ravb_gen3_data = {
 	.ravb_ops = &ravb_gen3_ops,
 	.net_features = NETIF_F_RXCSUM,
@@ -2088,12 +2505,25 @@ static const struct ravb_drv_data ravb_gen2_data = {
 	.features = RAVB_RCAR_GEN2_FEATURES,
 };
 
+static const struct ravb_drv_data rgeth_data = {
+	.ravb_ops = &ravb_ops_rgeth,
+	.net_hw_features = (NETIF_F_HW_CSUM | NETIF_F_RXCSUM),
+	.gstrings_stats = ravb_gstrings_stats_rgeth,
+	.gstrings_size = sizeof(ravb_gstrings_stats_rgeth),
+	.stats_len = ARRAY_SIZE(ravb_gstrings_stats_rgeth),
+	.num_gstat_queue = 1,
+	.skb_sz = RGETH_RX_BUFF_MAX + RAVB_ALIGN - 1,
+	.num_tx_desc = 2,
+	.features = RAVB_RZ_G2L_FEATURES,
+};
+
 static const struct of_device_id ravb_match_table[] = {
 	{ .compatible = "renesas,etheravb-r8a7790", .data = &ravb_gen2_data },
 	{ .compatible = "renesas,etheravb-r8a7794", .data = &ravb_gen2_data },
 	{ .compatible = "renesas,etheravb-rcar-gen2", .data = &ravb_gen2_data },
 	{ .compatible = "renesas,etheravb-r8a7795", .data = &ravb_gen3_data },
 	{ .compatible = "renesas,etheravb-rcar-gen3", .data = &ravb_gen3_data },
+	{ .compatible = "renesas,rzg2l-gbeth", .data = &rgeth_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ravb_match_table);
-- 
2.17.1

[PATCH net-next 18/18] arm64: dts: renesas: r9a07g044: Add GbEther nodes

[Biju Das]

Add Gigabit Ethernet{0,1} nodes to SoC DTSI.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 42 ++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
index 17bd09641529..816bf5ff5d54 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
@@ -331,6 +331,48 @@
 			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_LOW>;
 		};
 
+		eth0: ethernet@11c20000 {
+			compatible = "renesas,r9a07g044-gbeth",
+				     "renesas,rzg2l-gbeth";
+			reg = <0 0x11c20000 0 0x10000>;
+			interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 85 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "mux", "int_fil_n",
+					  "int_arp_ns_n";
+			phy-mode = "rgmii";
+			clocks = <&cpg CPG_MOD R9A07G044_ETH0_CLK_AXI>,
+				 <&cpg CPG_MOD R9A07G044_ETH0_CLK_CHI>,
+				 <&cpg CPG_CORE R9A07G044_CLK_HP>;
+			clock-names = "axi", "chi", "refclk";
+			resets = <&cpg R9A07G044_ETH0_RST_HW_N>;
+			power-domains = <&cpg>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
+
+		eth1: ethernet@11c30000 {
+			compatible = "renesas,r9a07g044-gbeth",
+				     "renesas,rzg2l-gbeth";
+			reg = <0 0x11c30000 0 0x10000>;
+			interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "mux", "int_fil_n",
+					  "int_arp_ns_n";
+			phy-mode = "rgmii";
+			clocks = <&cpg CPG_MOD R9A07G044_ETH1_CLK_AXI>,
+				 <&cpg CPG_MOD R9A07G044_ETH1_CLK_CHI>,
+				 <&cpg CPG_CORE R9A07G044_CLK_HP>;
+			clock-names = "axi", "chi", "refclk";
+			resets = <&cpg R9A07G044_ETH1_RST_HW_N>;
+			power-domains = <&cpg>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
+
 		phyrst: usbphy-ctrl@11c40000 {
 			compatible = "renesas,r9a07g044-usbphy-ctrl",
 				     "renesas,rzg2l-usbphy-ctrl";
-- 
2.17.1

Re: [PATCH net-next 00/18] Add Gigabit Ethernet driver support

[Sergei Shtylyov]

On 7/22/21 5:13 PM, Biju Das wrote:

> The DMAC and EMAC blocks of Gigabit Ethernet IP is almost similar to Ethernet AVB.
> 
> The Gigabit Etherner IP consists of Ethernet controller (E-MAC), Internal TCP/IP Offload Engine (TOE) and Dedicated Direct memory access controller (DMAC).
> 
> With few changes in driver, we can support Gigabit ethernet driver as well.
> 
> This patch series is aims to support the same
> 
> RFC->V1
>   * Incorporated feedback from Andrew, Sergei, Geert and Prabhakar
>   * https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=515525
> 
> Biju Das (18):
>   dt-bindings: net: renesas,etheravb: Document Gigabit Ethernet IP
>   drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks
>   drivers: clk: renesas: r9a07g044-cpg: Add ethernet clock sources
>   drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset


   It's not a good idea to have the patch to the defferent subsystems lumped
all together in a single series...

>   ravb: Replace chip type with a structure for driver data

   I was expecting some real changes on how the gen2/3 diff. features in this patch,
but I only saw new info having no real changes where they were needed and having the
changes that did not need to be converted yet...
   Anwyay, I have stopped here for today.

>   ravb: Factorise ptp feature
>   ravb: Add features specific to R-Car Gen3
>   ravb: Add R-Car common features
>   ravb: Factorise ravb_ring_free function
>   ravb: Factorise ravb_ring_format function
>   ravb: Factorise ravb_ring_init function
>   ravb: Factorise {emac,dmac} init function
>   ravb: Factorise ravb_rx function
>   ravb: Factorise ravb_adjust_link function
>   ravb: Factorise ravb_set_features
>   ravb: Add reset support
>   ravb: Add GbEthernet driver support
>   arm64: dts: renesas: r9a07g044: Add GbEther nodes
> 
[...]

MBR, Sergei

Re: [PATCH net-next 00/18] Add Gigabit Ethernet driver support

[Andrew Lunn]

On Thu, Jul 22, 2021 at 11:53:59PM +0300, Sergei Shtylyov wrote:
> On 7/22/21 5:13 PM, Biju Das wrote:
> 
> > The DMAC and EMAC blocks of Gigabit Ethernet IP is almost similar to Ethernet AVB.
> > 
> > The Gigabit Etherner IP consists of Ethernet controller (E-MAC), Internal TCP/IP Offload Engine (TOE) and Dedicated Direct memory access controller (DMAC).
> > 
> > With few changes in driver, we can support Gigabit ethernet driver as well.
> > 
> > This patch series is aims to support the same
> > 
> > RFC->V1
> >   * Incorporated feedback from Andrew, Sergei, Geert and Prabhakar
> >   * https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=515525
> > 
> > Biju Das (18):
> >   dt-bindings: net: renesas,etheravb: Document Gigabit Ethernet IP
> >   drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks
> >   drivers: clk: renesas: r9a07g044-cpg: Add ethernet clock sources
> >   drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset
> 
> 
>    It's not a good idea to have the patch to the defferent subsystems lumped
> all together in a single series...

Agreed.

Are these changes inseparable? If so, you need to be up front on this,
and you need an agreement with the subsystem maintainers how the
patches are going to be merged? Through which tree. And you need
Acked-by from the other tree maintainers.

Ideally you submit multiple patchsets. This assumes all sets will
compile independently.

	Andrew

RE: [PATCH net-next 00/18] Add Gigabit Ethernet driver support

[Biju Das]

Hi Andrew and Sergei,


> Subject: Re: [PATCH net-next 00/18] Add Gigabit Ethernet driver support
> 
> On Thu, Jul 22, 2021 at 11:53:59PM +0300, Sergei Shtylyov wrote:
> > On 7/22/21 5:13 PM, Biju Das wrote:
> >
> > > The DMAC and EMAC blocks of Gigabit Ethernet IP is almost similar to
> Ethernet AVB.
> > >
> > > The Gigabit Etherner IP consists of Ethernet controller (E-MAC),
> Internal TCP/IP Offload Engine (TOE) and Dedicated Direct memory access
> controller (DMAC).
> > >
> > > With few changes in driver, we can support Gigabit ethernet driver as
> well.
> > >
> > > This patch series is aims to support the same
> > >
> > > RFC->V1
> > >   * Incorporated feedback from Andrew, Sergei, Geert and Prabhakar
> > >   *
> > > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
> > > tchwork.kernel.org%2Fproject%2Flinux-renesas-soc%2Flist%2F%3Fseries%
> > > 3D515525&data=04%7C01%7Cbiju.das.jz%40bp.renesas.com%7C6fe3922cc
> > > 35d4178cb1d08d94d54bc75%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7
> > > C637625848601442706%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQ
> > > IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=bOpIqV1g
> > > lMUXqz9rsX0UK3Oqap2J1cY86TGVOJvzYe4%3D&reserved=0
> > >
> > > Biju Das (18):
> > >   dt-bindings: net: renesas,etheravb: Document Gigabit Ethernet IP
> > >   drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks
> > >   drivers: clk: renesas: r9a07g044-cpg: Add ethernet clock sources
> > >   drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset
> >
> >
> >    It's not a good idea to have the patch to the defferent subsystems
> > lumped all together in a single series...
> 
> Agreed.
> 
> Are these changes inseparable? If so, you need to be up front on this, and
> you need an agreement with the subsystem maintainers how the patches are
> going to be merged? Through which tree. And you need Acked-by from the
> other tree maintainers.
> 
> Ideally you submit multiple patchsets. This assumes all sets will compile
> independently.

Agreed. Will split this patch series in 3 patchsets

1) single binding patch 

2) Clock patchset

3) ravb driver patchset.

Cheers,
Biju

Re: [PATCH net-next 00/18] Add Gigabit Ethernet driver support

[Geert Uytterhoeven]

Hi Biju,

On Fri, Jul 23, 2021 at 8:28 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH net-next 00/18] Add Gigabit Ethernet driver support
> >
> > On Thu, Jul 22, 2021 at 11:53:59PM +0300, Sergei Shtylyov wrote:
> > > On 7/22/21 5:13 PM, Biju Das wrote:
> > >
> > > > The DMAC and EMAC blocks of Gigabit Ethernet IP is almost similar to
> > Ethernet AVB.
> > > >
> > > > The Gigabit Etherner IP consists of Ethernet controller (E-MAC),
> > Internal TCP/IP Offload Engine (TOE) and Dedicated Direct memory access
> > controller (DMAC).
> > > >
> > > > With few changes in driver, we can support Gigabit ethernet driver as
> > well.
> > > >
> > > > This patch series is aims to support the same
> > > >
> > > > RFC->V1
> > > >   * Incorporated feedback from Andrew, Sergei, Geert and Prabhakar
> > > >   *
> > > > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
> > > > tchwork.kernel.org%2Fproject%2Flinux-renesas-soc%2Flist%2F%3Fseries%
> > > > 3D515525&amp;data=04%7C01%7Cbiju.das.jz%40bp.renesas.com%7C6fe3922cc
> > > > 35d4178cb1d08d94d54bc75%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7
> > > > C637625848601442706%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQ
> > > > IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=bOpIqV1g
> > > > lMUXqz9rsX0UK3Oqap2J1cY86TGVOJvzYe4%3D&amp;reserved=0
> > > >
> > > > Biju Das (18):
> > > >   dt-bindings: net: renesas,etheravb: Document Gigabit Ethernet IP
> > > >   drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks
> > > >   drivers: clk: renesas: r9a07g044-cpg: Add ethernet clock sources
> > > >   drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset
> > >
> > >
> > >    It's not a good idea to have the patch to the defferent subsystems
> > > lumped all together in a single series...
> >
> > Agreed.
> >
> > Are these changes inseparable? If so, you need to be up front on this, and
> > you need an agreement with the subsystem maintainers how the patches are
> > going to be merged? Through which tree. And you need Acked-by from the
> > other tree maintainers.
> >
> > Ideally you submit multiple patchsets. This assumes all sets will compile
> > independently.
>
> Agreed. Will split this patch series in 3 patchsets
>
> 1) single binding patch
>
> 2) Clock patchset
>
> 3) ravb driver patchset.

4) dts part.

Part 2 should pass through renesas-clk.
Part 4 should pass through renesas-devel.

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 00/18] Add Gigabit Ethernet driver support

[Arnd Bergmann]

On Mon, Jul 26, 2021 at 12:56 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Fri, Jul 23, 2021 at 8:28 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > >
> > > Are these changes inseparable? If so, you need to be up front on this, and
> > > you need an agreement with the subsystem maintainers how the patches are
> > > going to be merged? Through which tree. And you need Acked-by from the
> > > other tree maintainers.
> > >
> > > Ideally you submit multiple patchsets. This assumes all sets will compile
> > > independently.
> >
> > Agreed. Will split this patch series in 3 patchsets
> >
> > 1) single binding patch
> >
> > 2) Clock patchset
> >
> > 3) ravb driver patchset.
>
> 4) dts part.
>
> Part 2 should pass through renesas-clk.
> Part 4 should pass through renesas-devel.

Sounds good. To clarify: the changes should not just compile, but
each branch should be usable independently with no loss of
functionality. Using the feature depends on having all branches
merged, but they should not introduced regressions when applied
into some other tree.

I hope this was already obvious to everyone involved.

       Arnd

RE: [PATCH net-next 00/18] Add Gigabit Ethernet driver support

[Biju Das]

Hi Sergei,

Thanks for the feedback.

> Subject: Re: [PATCH net-next 00/18] Add Gigabit Ethernet driver support
> 
> On 7/22/21 5:13 PM, Biju Das wrote:
> 
> > The DMAC and EMAC blocks of Gigabit Ethernet IP is almost similar to
> Ethernet AVB.
> >
> > The Gigabit Etherner IP consists of Ethernet controller (E-MAC),
> Internal TCP/IP Offload Engine (TOE) and Dedicated Direct memory access
> controller (DMAC).
> >
> > With few changes in driver, we can support Gigabit ethernet driver as
> well.
> >
> > This patch series is aims to support the same
> >
> > RFC->V1
> >   * Incorporated feedback from Andrew, Sergei, Geert and Prabhakar
> >   *
> > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > hwork.kernel.org%2Fproject%2Flinux-renesas-soc%2Flist%2F%3Fseries%3D51
> > 5525&data=04%7C01%7Cbiju.das.jz%40bp.renesas.com%7Cb01d51eb4442476
> > 149d608d94d52d7c9%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C6376258
> > 40484693261%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMz
> > IiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=M6biLbregS1y2R%2BMNb
> > b5PRRvgQxympZfHZkbuH0ZrXI%3D&reserved=0
> >
> > Biju Das (18):
> >   dt-bindings: net: renesas,etheravb: Document Gigabit Ethernet IP
> >   drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks
> >   drivers: clk: renesas: r9a07g044-cpg: Add ethernet clock sources
> >   drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset
> 
> 
>    It's not a good idea to have the patch to the defferent subsystems
> lumped all together in a single series...
> 
> >   ravb: Replace chip type with a structure for driver data
> 
>    I was expecting some real changes on how the gen2/3 diff. features in
> this patch, but I only saw new info having no real changes where they were
> needed and having the changes that did not need to be converted yet...
>    Anwyay, I have stopped here for today.

This patch is a preparation patch. On the subsequent patches[1] [2] and [3] you will see the real hw changes wr.to gen2/gen3. Feature bit is not added in this patch, but on the subsequent patches. I believe you haven't reviewed that patches yet.

PTP feature and the diff between gen2/gen3
------------------------------------------
[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210722141351.13668-7-biju.das.jz@bp.renesas.com/


Features specific to R-Car Gen3
--------------------------------
[2] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210722141351.13668-8-biju.das.jz@bp.renesas.com/


R-Car common features
---------------------

[3] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210722141351.13668-9-biju.das.jz@bp.renesas.com/

There is a review comment for making smaller changes. 
what do you recommend after going through [1],[2] and [3]. 
Please let us know.

Regards,
Biju


> 
> >   ravb: Factorise ptp feature
> >   ravb: Add features specific to R-Car Gen3
> >   ravb: Add R-Car common features
> >   ravb: Factorise ravb_ring_free function
> >   ravb: Factorise ravb_ring_format function
> >   ravb: Factorise ravb_ring_init function
> >   ravb: Factorise {emac,dmac} init function
> >   ravb: Factorise ravb_rx function
> >   ravb: Factorise ravb_adjust_link function
> >   ravb: Factorise ravb_set_features
> >   ravb: Add reset support
> >   ravb: Add GbEthernet driver support
> >   arm64: dts: renesas: r9a07g044: Add GbEther nodes
> >
> [...]
> 
> MBR, Sergei