[PATCH net-next 0/2] ravb: Support 1Gbps on R-Car H3 ES1.1+ and R-Car M3-W

[PATCH net-next 0/2] ravb: Support 1Gbps on R-Car H3 ES1.1+ and R-Car M3-W

[Simon Horman]

Hi,

this series adds support for gigabit communication to the Renesas EthernetAVB
controller when used in conjunction with  R-Car Gen3 H3 ES1.1+ and M3-W SoCs.
Gigabit is already supported with R-Car Gen 2 SoCs.

The patch from Geert was previously posted for inclusion in v4.10 and
acked by Dave for that purpose. It was, however, not accepted by the
ARM SoC maintainers.

The path from Mizuguchi-san is to address timing problems observed with
gigabit transfers. I would like it considered although my own testing on
M3-W did not show any timing problems.


Geert Uytterhoeven (1):
  ravb: Support 1Gbps on R-Car H3 ES1.1+ and R-Car M3-W

Kazuya Mizuguchi (1):
  ravb: Add tx and rx clock internal delays mode of APSR

 drivers/net/ethernet/renesas/ravb.h      | 10 ++++++++
 drivers/net/ethernet/renesas/ravb_main.c | 39 ++++++++++++++++++++++++++++++--
 2 files changed, 47 insertions(+), 2 deletions(-)

-- 
2.7.0.rc3.207.g0ac5344

[PATCH net-next 1/2] ravb: Add tx and rx clock internal delays mode of APSR

[Simon Horman]

From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>

This patch enables tx and rx clock internal delay modes (TDM and RDM).

This is to address a failure in the case of 1Gbps communication using the
by salvator-x board with the KSZ9031RNX phy. This has been reported to
occur with both the r8a7795 (H3) and r8a7796 (M3-W) SoCs.

With this change APSR internal delay modes are enabled for
"rgmii-id", "rgmii-rxid" and "rgmii-txid" phy modes as follows:

phy mode   | ASPR delay mode
-----------+----------------
rgmii-id   | TDM and RDM
rgmii-rxid | RDM
rgmii-txid | TDM

Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

---
v1 [Simon Horman]
- Combined patches
- Reworded changelog

v0 [Kazuya Mizuguchi]
---
 drivers/net/ethernet/renesas/ravb.h      | 10 ++++++++++
 drivers/net/ethernet/renesas/ravb_main.c | 29 +++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index f1109661a533..d7c91d48cc48 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -76,6 +76,7 @@ enum ravb_reg {
 	CDAR20	= 0x0060,
 	CDAR21	= 0x0064,
 	ESR	= 0x0088,
+	APSR	= 0x008C,	/* R-Car Gen3 only */
 	RCR	= 0x0090,
 	RQC0	= 0x0094,
 	RQC1	= 0x0098,
@@ -248,6 +249,15 @@ enum ESR_BIT {
 	ESR_EIL		= 0x00001000,
 };
 
+/* APSR */
+enum APSR_BIT {
+	APSR_MEMS		= 0x00000002,
+	APSR_CMSW		= 0x00000010,
+	APSR_DM			= 0x00006000,
+	APSR_DM_RDM		= 0x00002000,
+	APSR_DM_TDM		= 0x00004000,
+};
+
 /* RCR */
 enum RCR_BIT {
 	RCR_EFFS	= 0x00000001,
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 89ac1e3f6175..9fb4c04c5885 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1904,6 +1904,29 @@ static void ravb_set_config_mode(struct net_device *ndev)
 	}
 }
 
+static void ravb_set_delay_mode(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	if (priv->chip_id != RCAR_GEN2) {
+		switch (priv->phy_interface) {
+		case PHY_INTERFACE_MODE_RGMII_ID:
+			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM |
+				   APSR_DM_TDM);
+			break;
+		case PHY_INTERFACE_MODE_RGMII_RXID:
+			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM);
+			break;
+		case PHY_INTERFACE_MODE_RGMII_TXID:
+			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_TDM);
+			break;
+		default:
+			ravb_modify(ndev, APSR, APSR_DM, 0);
+			break;
+		}
+	}
+}
+
 static int ravb_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -2016,6 +2039,9 @@ static int ravb_probe(struct platform_device *pdev)
 	/* Request GTI loading */
 	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
 
+	/* Set APSR */
+	ravb_set_delay_mode(ndev);
+
 	/* Allocate descriptor base address table */
 	priv->desc_bat_size = sizeof(struct ravb_desc) * DBAT_ENTRY_NUM;
 	priv->desc_bat = dma_alloc_coherent(ndev->dev.parent, priv->desc_bat_size,
@@ -2152,6 +2178,9 @@ static int __maybe_unused ravb_resume(struct device *dev)
 	/* Request GTI loading */
 	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
 
+	/* Set APSR */
+	ravb_set_delay_mode(ndev);
+
 	/* Restore descriptor base address table */
 	ravb_write(ndev, priv->desc_bat_dma, DBAT);
 
-- 
2.7.0.rc3.207.g0ac5344

[PATCH net-next 2/2] ravb: Support 1Gbps on R-Car H3 ES1.1+ and R-Car M3-W

[Simon Horman]

From: Geert Uytterhoeven <geert+renesas@glider.be>

The limitation to 10/100Mbit speeds on R-Car Gen3 is valid for R-Car H3
ES1.0 only. Check for the exact SoC model to allow 1Gbps on newer
revisions of R-Car H3, and on R-Car M3-W.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 drivers/net/ethernet/renesas/ravb_main.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 9fb4c04c5885..93cdadeae354 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -31,6 +31,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/sys_soc.h>
 
 #include <asm/div64.h>
 
@@ -973,6 +974,11 @@ static void ravb_adjust_link(struct net_device *ndev)
 		phy_print_status(phydev);
 }
 
+static const struct soc_device_attribute r8a7795es10[] = {
+	{ .soc_id = "r8a7795", .revision = "ES1.0", },
+	{ /* sentinel */ }
+};
+
 /* PHY init function */
 static int ravb_phy_init(struct net_device *ndev)
 {
@@ -1008,10 +1014,10 @@ static int ravb_phy_init(struct net_device *ndev)
 		goto err_deregister_fixed_link;
 	}
 
-	/* This driver only support 10/100Mbit speeds on Gen3
+	/* This driver only support 10/100Mbit speeds on R-Car H3 ES1.0
 	 * at this time.
 	 */
-	if (priv->chip_id == RCAR_GEN3) {
+	if (soc_device_match(r8a7795es10)) {
 		err = phy_set_max_speed(phydev, SPEED_100);
 		if (err) {
 			netdev_err(ndev, "failed to limit PHY to 100Mbit/s\n");
-- 
2.7.0.rc3.207.g0ac5344

Re: [PATCH net-next 0/2] ravb: Support 1Gbps on R-Car H3 ES1.1+ and R-Car M3-W

[Geert Uytterhoeven]

Hi Simon,

On Fri, Jan 27, 2017 at 12:04 PM, Simon Horman
<horms+renesas@verge.net.au> wrote:
> The path from Mizuguchi-san is to address timing problems observed with
> gigabit transfers. I would like it considered although my own testing on
> M3-W did not show any timing problems.

Is there any relation with the *-skew-ps properties in the DTS in the BSP?

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 1/2] ravb: Add tx and rx clock internal delays mode of APSR

[Sergei Shtylyov]

Hello!

On 01/27/2017 02:04 PM, Simon Horman wrote:

> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>
> This patch enables tx and rx clock internal delay modes (TDM and RDM).
>
> This is to address a failure in the case of 1Gbps communication using the
> by salvator-x board with the KSZ9031RNX phy. This has been reported to
> occur with both the r8a7795 (H3) and r8a7796 (M3-W) SoCs.
>
> With this change APSR internal delay modes are enabled for
> "rgmii-id", "rgmii-rxid" and "rgmii-txid" phy modes as follows:
>
> phy mode   | ASPR delay mode
> -----------+----------------
> rgmii-id   | TDM and RDM
> rgmii-rxid | RDM
> rgmii-txid | TDM
>
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>
> ---
> v1 [Simon Horman]
> - Combined patches
> - Reworded changelog
>
> v0 [Kazuya Mizuguchi]
> ---
>  drivers/net/ethernet/renesas/ravb.h      | 10 ++++++++++
>  drivers/net/ethernet/renesas/ravb_main.c | 29 +++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
>
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index f1109661a533..d7c91d48cc48 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
[...]
> @@ -248,6 +249,15 @@ enum ESR_BIT {
>  	ESR_EIL		= 0x00001000,
>  };
>
> +/* APSR */
> +enum APSR_BIT {
> +	APSR_MEMS		= 0x00000002,

    Not documented in the revision 0.5 of the gen3 manual...

> +	APSR_CMSW		= 0x00000010,
> +	APSR_DM			= 0x00006000,

    ... and neither this field. Are these documented in the latter revs?

> +	APSR_DM_RDM		= 0x00002000,
> +	APSR_DM_TDM		= 0x00004000,
> +};
> +
>  /* RCR */
>  enum RCR_BIT {
>  	RCR_EFFS	= 0x00000001,
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 89ac1e3f6175..9fb4c04c5885 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1904,6 +1904,29 @@ static void ravb_set_config_mode(struct net_device *ndev)
>  	}
>  }
>
> +static void ravb_set_delay_mode(struct net_device *ndev)
> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +
> +	if (priv->chip_id != RCAR_GEN2) {
> +		switch (priv->phy_interface) {
> +		case PHY_INTERFACE_MODE_RGMII_ID:
> +			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM |
> +				   APSR_DM_TDM);
> +			break;
> +		case PHY_INTERFACE_MODE_RGMII_RXID:
> +			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM);
> +			break;
> +		case PHY_INTERFACE_MODE_RGMII_TXID:
> +			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_TDM);
> +			break;
> +		default:
> +			ravb_modify(ndev, APSR, APSR_DM, 0);
> +			break;
> +		}

    How about doing ravb_modify() only once?

[...]

MNR, Sergei

Re: [PATCH net-next 2/2] ravb: Support 1Gbps on R-Car H3 ES1.1+ and R-Car M3-W

[Sergei Shtylyov]

On 01/27/2017 02:04 PM, Simon Horman wrote:

> From: Geert Uytterhoeven <geert+renesas@glider.be>
>
> The limitation to 10/100Mbit speeds on R-Car Gen3 is valid for R-Car H3
> ES1.0 only. Check for the exact SoC model to allow 1Gbps on newer
> revisions of R-Car H3, and on R-Car M3-W.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

MBR, Sergei

Re: [PATCH net-next 1/2] ravb: Add tx and rx clock internal delays mode of APSR

[Sergei Shtylyov]

On 01/27/2017 02:04 PM, Simon Horman wrote:

> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>
> This patch enables tx and rx clock internal delay modes (TDM and RDM).
>
> This is to address a failure in the case of 1Gbps communication using the
> by salvator-x board with the KSZ9031RNX phy. This has been reported to
> occur with both the r8a7795 (H3) and r8a7796 (M3-W) SoCs.
>
> With this change APSR internal delay modes are enabled for
> "rgmii-id", "rgmii-rxid" and "rgmii-txid" phy modes as follows:
>
> phy mode   | ASPR delay mode
> -----------+----------------
> rgmii-id   | TDM and RDM
> rgmii-rxid | RDM
> rgmii-txid | TDM
>
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>
> ---
> v1 [Simon Horman]
> - Combined patches
> - Reworded changelog
>
> v0 [Kazuya Mizuguchi]
[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 89ac1e3f6175..9fb4c04c5885 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1904,6 +1904,29 @@ static void ravb_set_config_mode(struct net_device *ndev)
>  	}
>  }
>
> +static void ravb_set_delay_mode(struct net_device *ndev)
> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +
> +	if (priv->chip_id != RCAR_GEN2) {
> +		switch (priv->phy_interface) {
> +		case PHY_INTERFACE_MODE_RGMII_ID:
> +			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM |
> +				   APSR_DM_TDM);
> +			break;
> +		case PHY_INTERFACE_MODE_RGMII_RXID:
> +			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM);
> +			break;
> +		case PHY_INTERFACE_MODE_RGMII_TXID:
> +			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_TDM);
> +			break;
> +		default:
> +			ravb_modify(ndev, APSR, APSR_DM, 0);
> +			break;
> +		}
> +	}
> +}
> +
>  static int ravb_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> @@ -2016,6 +2039,9 @@ static int ravb_probe(struct platform_device *pdev)
>  	/* Request GTI loading */
>  	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
>
> +	/* Set APSR */
> +	ravb_set_delay_mode(ndev);
> +
>  	/* Allocate descriptor base address table */
>  	priv->desc_bat_size = sizeof(struct ravb_desc) * DBAT_ENTRY_NUM;
>  	priv->desc_bat = dma_alloc_coherent(ndev->dev.parent, priv->desc_bat_size,
> @@ -2152,6 +2178,9 @@ static int __maybe_unused ravb_resume(struct device *dev)
>  	/* Request GTI loading */
>  	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
>
> +	/* Set APSR */
> +	ravb_set_delay_mode(ndev);
> +

    So far the pattern was to first check if gen != 2, then calling a gen3 
specific function. I'd like to keep it, rather than checking in the function 
itself.

[...]

MBR, Sergei

Re: [PATCH net-next 0/2] ravb: Support 1Gbps on R-Car H3 ES1.1+ and R-Car M3-W

[Simon Horman]

On Fri, Jan 27, 2017 at 01:16:43PM +0100, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Fri, Jan 27, 2017 at 12:04 PM, Simon Horman
> <horms+renesas@verge.net.au> wrote:
> > The path from Mizuguchi-san is to address timing problems observed with
> > gigabit transfers. I would like it considered although my own testing on
> > M3-W did not show any timing problems.
> 
> Is there any relation with the *-skew-ps properties in the DTS in the BSP?

Yes, I believe they are also intended to address timeouts.
I plan to post them too.

Re: [PATCH net-next 1/2] ravb: Add tx and rx clock internal delays mode of APSR

[Simon Horman]

On Fri, Jan 27, 2017 at 03:42:05PM +0300, Sergei Shtylyov wrote:
> Hello!
> 
> On 01/27/2017 02:04 PM, Simon Horman wrote:
> 
> >From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> >
> >This patch enables tx and rx clock internal delay modes (TDM and RDM).
> >
> >This is to address a failure in the case of 1Gbps communication using the
> >by salvator-x board with the KSZ9031RNX phy. This has been reported to
> >occur with both the r8a7795 (H3) and r8a7796 (M3-W) SoCs.
> >
> >With this change APSR internal delay modes are enabled for
> >"rgmii-id", "rgmii-rxid" and "rgmii-txid" phy modes as follows:
> >
> >phy mode   | ASPR delay mode
> >-----------+----------------
> >rgmii-id   | TDM and RDM
> >rgmii-rxid | RDM
> >rgmii-txid | TDM
> >
> >Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> >Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> >
> >---
> >v1 [Simon Horman]
> >- Combined patches
> >- Reworded changelog
> >
> >v0 [Kazuya Mizuguchi]
> >---
> > drivers/net/ethernet/renesas/ravb.h      | 10 ++++++++++
> > drivers/net/ethernet/renesas/ravb_main.c | 29 +++++++++++++++++++++++++++++
> > 2 files changed, 39 insertions(+)
> >
> >diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> >index f1109661a533..d7c91d48cc48 100644
> >--- a/drivers/net/ethernet/renesas/ravb.h
> >+++ b/drivers/net/ethernet/renesas/ravb.h
> [...]
> >@@ -248,6 +249,15 @@ enum ESR_BIT {
> > 	ESR_EIL		= 0x00001000,
> > };
> >
> >+/* APSR */
> >+enum APSR_BIT {
> >+	APSR_MEMS		= 0x00000002,
> 
>    Not documented in the revision 0.5 of the gen3 manual...

It is in version 0.53 of the documentation but I think it can be dropped
from this patch as its unused.

> >+	APSR_CMSW		= 0x00000010,

I think the above can also be dropped as it is unused.

> >+	APSR_DM			= 0x00006000,
> 
>    ... and neither this field. Are these documented in the latter revs?

This one is not. I can ask Mizuguchi-san to confirm it exists (with himself).

> >+	APSR_DM_RDM		= 0x00002000,
> >+	APSR_DM_TDM		= 0x00004000,
> >+};
> >+
> > /* RCR */
> > enum RCR_BIT {
> > 	RCR_EFFS	= 0x00000001,
> >diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >index 89ac1e3f6175..9fb4c04c5885 100644
> >--- a/drivers/net/ethernet/renesas/ravb_main.c
> >+++ b/drivers/net/ethernet/renesas/ravb_main.c
> >@@ -1904,6 +1904,29 @@ static void ravb_set_config_mode(struct net_device *ndev)
> > 	}
> > }
> >
> >+static void ravb_set_delay_mode(struct net_device *ndev)
> >+{
> >+	struct ravb_private *priv = netdev_priv(ndev);
> >+
> >+	if (priv->chip_id != RCAR_GEN2) {
> >+		switch (priv->phy_interface) {
> >+		case PHY_INTERFACE_MODE_RGMII_ID:
> >+			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM |
> >+				   APSR_DM_TDM);
> >+			break;
> >+		case PHY_INTERFACE_MODE_RGMII_RXID:
> >+			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM);
> >+			break;
> >+		case PHY_INTERFACE_MODE_RGMII_TXID:
> >+			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_TDM);
> >+			break;
> >+		default:
> >+			ravb_modify(ndev, APSR, APSR_DM, 0);
> >+			break;
> >+		}
> 
>    How about doing ravb_modify() only once?

Something like this?

		int set = 0;

		if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
		    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_RXID)
		    set |= APSR_DM_RDM;

		if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
		    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID)
		    set |= APSR_DM_TDM;

		ravb_modify(ndev, APSR, APSR_DM, set);

I don't really see any advantage to it but I can change things around
however you like.

Re: [PATCH net-next 1/2] ravb: Add tx and rx clock internal delays mode of APSR

[Simon Horman]

On Fri, Jan 27, 2017 at 04:00:27PM +0300, Sergei Shtylyov wrote:
> On 01/27/2017 02:04 PM, Simon Horman wrote:
> 
> >From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> >
> >This patch enables tx and rx clock internal delay modes (TDM and RDM).
> >
> >This is to address a failure in the case of 1Gbps communication using the
> >by salvator-x board with the KSZ9031RNX phy. This has been reported to
> >occur with both the r8a7795 (H3) and r8a7796 (M3-W) SoCs.
> >
> >With this change APSR internal delay modes are enabled for
> >"rgmii-id", "rgmii-rxid" and "rgmii-txid" phy modes as follows:
> >
> >phy mode   | ASPR delay mode
> >-----------+----------------
> >rgmii-id   | TDM and RDM
> >rgmii-rxid | RDM
> >rgmii-txid | TDM
> >
> >Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> >Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> >
> >---
> >v1 [Simon Horman]
> >- Combined patches
> >- Reworded changelog
> >
> >v0 [Kazuya Mizuguchi]
> [...]
> >diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >index 89ac1e3f6175..9fb4c04c5885 100644
> >--- a/drivers/net/ethernet/renesas/ravb_main.c
> >+++ b/drivers/net/ethernet/renesas/ravb_main.c
> >@@ -1904,6 +1904,29 @@ static void ravb_set_config_mode(struct net_device *ndev)
> > 	}
> > }
> >
> >+static void ravb_set_delay_mode(struct net_device *ndev)
> >+{
> >+	struct ravb_private *priv = netdev_priv(ndev);
> >+
> >+	if (priv->chip_id != RCAR_GEN2) {
> >+		switch (priv->phy_interface) {
> >+		case PHY_INTERFACE_MODE_RGMII_ID:
> >+			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM |
> >+				   APSR_DM_TDM);
> >+			break;
> >+		case PHY_INTERFACE_MODE_RGMII_RXID:
> >+			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM);
> >+			break;
> >+		case PHY_INTERFACE_MODE_RGMII_TXID:
> >+			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_TDM);
> >+			break;
> >+		default:
> >+			ravb_modify(ndev, APSR, APSR_DM, 0);
> >+			break;
> >+		}
> >+	}
> >+}
> >+
> > static int ravb_probe(struct platform_device *pdev)
> > {
> > 	struct device_node *np = pdev->dev.of_node;
> >@@ -2016,6 +2039,9 @@ static int ravb_probe(struct platform_device *pdev)
> > 	/* Request GTI loading */
> > 	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> >
> >+	/* Set APSR */
> >+	ravb_set_delay_mode(ndev);
> >+
> > 	/* Allocate descriptor base address table */
> > 	priv->desc_bat_size = sizeof(struct ravb_desc) * DBAT_ENTRY_NUM;
> > 	priv->desc_bat = dma_alloc_coherent(ndev->dev.parent, priv->desc_bat_size,
> >@@ -2152,6 +2178,9 @@ static int __maybe_unused ravb_resume(struct device *dev)
> > 	/* Request GTI loading */
> > 	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> >
> >+	/* Set APSR */
> >+	ravb_set_delay_mode(ndev);
> >+
> 
>    So far the pattern was to first check if gen != 2, then calling a gen3
> specific function. I'd like to keep it, rather than checking in the function
> itself.

So you would like something like?

	/* Set APSR */
	if (priv->chip_id != RCAR_GEN2)
		 ravb_set_delay_mode(ndev);

That does not seem better to me but I can make it so if that is your
preference.

Re: [PATCH net-next 1/2] ravb: Add tx and rx clock internal delays mode of APSR

[Sergei Shtylyov]

On 01/27/2017 07:49 PM, Simon Horman wrote:

>>> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>>>
>>> This patch enables tx and rx clock internal delay modes (TDM and RDM).
>>>
>>> This is to address a failure in the case of 1Gbps communication using the
>>> by salvator-x board with the KSZ9031RNX phy. This has been reported to
>>> occur with both the r8a7795 (H3) and r8a7796 (M3-W) SoCs.
>>>
>>> With this change APSR internal delay modes are enabled for
>>> "rgmii-id", "rgmii-rxid" and "rgmii-txid" phy modes as follows:
>>>
>>> phy mode   | ASPR delay mode
>>> -----------+----------------
>>> rgmii-id   | TDM and RDM
>>> rgmii-rxid | RDM
>>> rgmii-txid | TDM
>>>
>>> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>>> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>>>
>>> ---
>>> v1 [Simon Horman]
>>> - Combined patches
>>> - Reworded changelog
>>>
>>> v0 [Kazuya Mizuguchi]
>>> ---
>>> drivers/net/ethernet/renesas/ravb.h      | 10 ++++++++++
>>> drivers/net/ethernet/renesas/ravb_main.c | 29 +++++++++++++++++++++++++++++
>>> 2 files changed, 39 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
>>> index f1109661a533..d7c91d48cc48 100644
>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>> [...]
>>> @@ -248,6 +249,15 @@ enum ESR_BIT {
>>> 	ESR_EIL		= 0x00001000,
>>> };
>>>
>>> +/* APSR */
>>> +enum APSR_BIT {
>>> +	APSR_MEMS		= 0x00000002,
>>
>>    Not documented in the revision 0.5 of the gen3 manual...
>
> It is in version 0.53 of the documentation but I think it can be dropped
> from this patch as its unused.

    No need to drop, let it be. :-)

>>> +	APSR_CMSW		= 0x00000010,
>
> I think the above can also be dropped as it is unused.

    No need.

>>> +	APSR_DM			= 0x00006000,
>>
>>    ... and neither this field. Are these documented in the latter revs?
>
> This one is not. I can ask Mizuguchi-san to confirm it exists (with himself).

    The whole patch depends on it! If it's not documented, I'd like a comment 
added, like I did for the other undocumented things in this driver...

[...]
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 89ac1e3f6175..9fb4c04c5885 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -1904,6 +1904,29 @@ static void ravb_set_config_mode(struct net_device *ndev)
>>> 	}
>>> }
>>>
>>> +static void ravb_set_delay_mode(struct net_device *ndev)
>>> +{
>>> +	struct ravb_private *priv = netdev_priv(ndev);
>>> +
>>> +	if (priv->chip_id != RCAR_GEN2) {
>>> +		switch (priv->phy_interface) {
>>> +		case PHY_INTERFACE_MODE_RGMII_ID:
>>> +			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM |
>>> +				   APSR_DM_TDM);
>>> +			break;
>>> +		case PHY_INTERFACE_MODE_RGMII_RXID:
>>> +			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM);
>>> +			break;
>>> +		case PHY_INTERFACE_MODE_RGMII_TXID:
>>> +			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_TDM);
>>> +			break;
>>> +		default:
>>> +			ravb_modify(ndev, APSR, APSR_DM, 0);
>>> +			break;
>>> +		}
>>
>>    How about doing ravb_modify() only once?
>
> Something like this?
>
> 		int set = 0;
>
> 		if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> 		    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_RXID)
> 		    set |= APSR_DM_RDM;
>
> 		if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> 		    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID)
> 		    set |= APSR_DM_TDM;
>
> 		ravb_modify(ndev, APSR, APSR_DM, set);
>
> I don't really see any advantage to it but I can change things around
> however you like.

    I didn't mean to replace *switch*, actually -- just to move ravb_modify() 
out of it.

MBR, Sergei

Re: [PATCH net-next 1/2] ravb: Add tx and rx clock internal delays mode of APSR

[Sergei Shtylyov]

On 01/27/2017 07:51 PM, Simon Horman wrote:

>>> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>>>
>>> This patch enables tx and rx clock internal delay modes (TDM and RDM).
>>>
>>> This is to address a failure in the case of 1Gbps communication using the
>>> by salvator-x board with the KSZ9031RNX phy. This has been reported to
>>> occur with both the r8a7795 (H3) and r8a7796 (M3-W) SoCs.
>>>
>>> With this change APSR internal delay modes are enabled for
>>> "rgmii-id", "rgmii-rxid" and "rgmii-txid" phy modes as follows:
>>>
>>> phy mode   | ASPR delay mode
>>> -----------+----------------
>>> rgmii-id   | TDM and RDM
>>> rgmii-rxid | RDM
>>> rgmii-txid | TDM
>>>
>>> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>>> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>>>
>>> ---
>>> v1 [Simon Horman]
>>> - Combined patches
>>> - Reworded changelog
>>>
>>> v0 [Kazuya Mizuguchi]
>> [...]
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 89ac1e3f6175..9fb4c04c5885 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
>>> @@ -2016,6 +2039,9 @@ static int ravb_probe(struct platform_device *pdev)
>>> 	/* Request GTI loading */
>>> 	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
>>>
>>> +	/* Set APSR */
>>> +	ravb_set_delay_mode(ndev);
>>> +
>>> 	/* Allocate descriptor base address table */
>>> 	priv->desc_bat_size = sizeof(struct ravb_desc) * DBAT_ENTRY_NUM;
>>> 	priv->desc_bat = dma_alloc_coherent(ndev->dev.parent, priv->desc_bat_size,
>>> @@ -2152,6 +2178,9 @@ static int __maybe_unused ravb_resume(struct device *dev)
>>> 	/* Request GTI loading */
>>> 	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
>>>
>>> +	/* Set APSR */
>>> +	ravb_set_delay_mode(ndev);
>>> +
>>
>>    So far the pattern was to first check if gen != 2, then calling a gen3
>> specific function. I'd like to keep it, rather than checking in the function
>> itself.
>
> So you would like something like?
>
> 	/* Set APSR */
> 	if (priv->chip_id != RCAR_GEN2)
> 		 ravb_set_delay_mode(ndev);

    Yes (except the comment).

>
> That does not seem better to me but I can make it so if that is your
> preference.

    Yes, not better code-wise but more consistent.

MBR, Sergei

Re: [PATCH net-next 1/2] ravb: Add tx and rx clock internal delays mode of APSR

[Simon Horman]

On Fri, Jan 27, 2017 at 07:55:25PM +0300, Sergei Shtylyov wrote:
> On 01/27/2017 07:49 PM, Simon Horman wrote:
> 
> >>>From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> >>>
> >>>This patch enables tx and rx clock internal delay modes (TDM and RDM).
> >>>
> >>>This is to address a failure in the case of 1Gbps communication using the
> >>>by salvator-x board with the KSZ9031RNX phy. This has been reported to
> >>>occur with both the r8a7795 (H3) and r8a7796 (M3-W) SoCs.
> >>>
> >>>With this change APSR internal delay modes are enabled for
> >>>"rgmii-id", "rgmii-rxid" and "rgmii-txid" phy modes as follows:
> >>>
> >>>phy mode   | ASPR delay mode
> >>>-----------+----------------
> >>>rgmii-id   | TDM and RDM
> >>>rgmii-rxid | RDM
> >>>rgmii-txid | TDM
> >>>
> >>>Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> >>>Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> >>>
> >>>---
> >>>v1 [Simon Horman]
> >>>- Combined patches
> >>>- Reworded changelog
> >>>
> >>>v0 [Kazuya Mizuguchi]
> >>>---
> >>>drivers/net/ethernet/renesas/ravb.h      | 10 ++++++++++
> >>>drivers/net/ethernet/renesas/ravb_main.c | 29 +++++++++++++++++++++++++++++
> >>>2 files changed, 39 insertions(+)
> >>>
> >>>diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> >>>index f1109661a533..d7c91d48cc48 100644
> >>>--- a/drivers/net/ethernet/renesas/ravb.h
> >>>+++ b/drivers/net/ethernet/renesas/ravb.h
> >>[...]
> >>>@@ -248,6 +249,15 @@ enum ESR_BIT {
> >>>	ESR_EIL		= 0x00001000,
> >>>};
> >>>
> >>>+/* APSR */
> >>>+enum APSR_BIT {
> >>>+	APSR_MEMS		= 0x00000002,
> >>
> >>   Not documented in the revision 0.5 of the gen3 manual...
> >
> >It is in version 0.53 of the documentation but I think it can be dropped
> >from this patch as its unused.
> 
>    No need to drop, let it be. :-)
> 
> >>>+	APSR_CMSW		= 0x00000010,
> >
> >I think the above can also be dropped as it is unused.
> 
>    No need.

Ok, lets leave it and MEMS.

> >>>+	APSR_DM			= 0x00006000,
> >>
> >>   ... and neither this field. Are these documented in the latter revs?
> >
> >This one is not. I can ask Mizuguchi-san to confirm it exists (with himself).
> 
>    The whole patch depends on it! If it's not documented, I'd like a comment
> added, like I did for the other undocumented things in this driver...

Sure.

> [...]
> >>>diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >>>index 89ac1e3f6175..9fb4c04c5885 100644
> >>>--- a/drivers/net/ethernet/renesas/ravb_main.c
> >>>+++ b/drivers/net/ethernet/renesas/ravb_main.c
> >>>@@ -1904,6 +1904,29 @@ static void ravb_set_config_mode(struct net_device *ndev)
> >>>	}
> >>>}
> >>>
> >>>+static void ravb_set_delay_mode(struct net_device *ndev)
> >>>+{
> >>>+	struct ravb_private *priv = netdev_priv(ndev);
> >>>+
> >>>+	if (priv->chip_id != RCAR_GEN2) {
> >>>+		switch (priv->phy_interface) {
> >>>+		case PHY_INTERFACE_MODE_RGMII_ID:
> >>>+			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM |
> >>>+				   APSR_DM_TDM);
> >>>+			break;
> >>>+		case PHY_INTERFACE_MODE_RGMII_RXID:
> >>>+			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM);
> >>>+			break;
> >>>+		case PHY_INTERFACE_MODE_RGMII_TXID:
> >>>+			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_TDM);
> >>>+			break;
> >>>+		default:
> >>>+			ravb_modify(ndev, APSR, APSR_DM, 0);
> >>>+			break;
> >>>+		}
> >>
> >>   How about doing ravb_modify() only once?
> >
> >Something like this?
> >
> >		int set = 0;
> >
> >		if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> >		    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_RXID)
> >		    set |= APSR_DM_RDM;
> >
> >		if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> >		    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID)
> >		    set |= APSR_DM_TDM;
> >
> >		ravb_modify(ndev, APSR, APSR_DM, set);
> >
> >I don't really see any advantage to it but I can change things around
> >however you like.
> 
>    I didn't mean to replace *switch*, actually -- just to move ravb_modify()
> out of it.

Ok, I will make it so.

Re: [PATCH net-next 1/2] ravb: Add tx and rx clock internal delays mode of APSR

[Sergei Shtylyov]

On 01/27/2017 09:07 PM, Simon Horman wrote:

>>>>> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>>>>>
>>>>> This patch enables tx and rx clock internal delay modes (TDM and RDM).
>>>>>
>>>>> This is to address a failure in the case of 1Gbps communication using the
>>>>> by salvator-x board with the KSZ9031RNX phy. This has been reported to
>>>>> occur with both the r8a7795 (H3) and r8a7796 (M3-W) SoCs.
>>>>>
>>>>> With this change APSR internal delay modes are enabled for
>>>>> "rgmii-id", "rgmii-rxid" and "rgmii-txid" phy modes as follows:
>>>>>
>>>>> phy mode   | ASPR delay mode
>>>>> -----------+----------------
>>>>> rgmii-id   | TDM and RDM
>>>>> rgmii-rxid | RDM
>>>>> rgmii-txid | TDM
>>>>>
>>>>> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>>>>> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>>>>>
>>>>> ---
>>>>> v1 [Simon Horman]
>>>>> - Combined patches
>>>>> - Reworded changelog
>>>>>
>>>>> v0 [Kazuya Mizuguchi]
>>>>> ---
>>>>> drivers/net/ethernet/renesas/ravb.h      | 10 ++++++++++
>>>>> drivers/net/ethernet/renesas/ravb_main.c | 29 +++++++++++++++++++++++++++++
>>>>> 2 files changed, 39 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
>>>>> index f1109661a533..d7c91d48cc48 100644
>>>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>>> [...]
>>>>> @@ -248,6 +249,15 @@ enum ESR_BIT {
>>>>> 	ESR_EIL		= 0x00001000,
>>>>> };
>>>>>
>>>>> +/* APSR */
>>>>> +enum APSR_BIT {
>>>>> +	APSR_MEMS		= 0x00000002,
>>>>
>>>>   Not documented in the revision 0.5 of the gen3 manual...
>>>
>>> It is in version 0.53 of the documentation but I think it can be dropped
>> >from this patch as its unused.
>>
>>    No need to drop, let it be. :-)
>>
>>>>> +	APSR_CMSW		= 0x00000010,
>>>
>>> I think the above can also be dropped as it is unused.
>>
>>    No need.
>
> Ok, lets leave it and MEMS.
>
>>>>> +	APSR_DM			= 0x00006000,
>>>>
>>>>   ... and neither this field. Are these documented in the latter revs?
>>>
>>> This one is not. I can ask Mizuguchi-san to confirm it exists (with himself).
>>
>>    The whole patch depends on it! If it's not documented, I'd like a comment
>> added, like I did for the other undocumented things in this driver...
>
> Sure.
>
>> [...]
>>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>>>> index 89ac1e3f6175..9fb4c04c5885 100644
>>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>>> @@ -1904,6 +1904,29 @@ static void ravb_set_config_mode(struct net_device *ndev)
>>>>> 	}
>>>>> }
>>>>>
>>>>> +static void ravb_set_delay_mode(struct net_device *ndev)
>>>>> +{
>>>>> +	struct ravb_private *priv = netdev_priv(ndev);
>>>>> +
>>>>> +	if (priv->chip_id != RCAR_GEN2) {
>>>>> +		switch (priv->phy_interface) {
>>>>> +		case PHY_INTERFACE_MODE_RGMII_ID:
>>>>> +			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM |
>>>>> +				   APSR_DM_TDM);
>>>>> +			break;
>>>>> +		case PHY_INTERFACE_MODE_RGMII_RXID:
>>>>> +			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM);
>>>>> +			break;
>>>>> +		case PHY_INTERFACE_MODE_RGMII_TXID:
>>>>> +			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_TDM);
>>>>> +			break;
>>>>> +		default:
>>>>> +			ravb_modify(ndev, APSR, APSR_DM, 0);
>>>>> +			break;
>>>>> +		}
>>>>
>>>>   How about doing ravb_modify() only once?
>>>
>>> Something like this?
>>>
>>> 		int set = 0;
>>>
>>> 		if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
>>> 		    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_RXID)
>>> 		    set |= APSR_DM_RDM;
>>>
>>> 		if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
>>> 		    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID)
>>> 		    set |= APSR_DM_TDM;
>>>
>>> 		ravb_modify(ndev, APSR, APSR_DM, set);
>>>
>>> I don't really see any advantage to it but I can change things around
>>> however you like.
>>
>>    I didn't mean to replace *switch*, actually -- just to move ravb_modify()
>> out of it.
>
> Ok, I will make it so.

    Your variant is probably even better! :-)
    I'm not sure how good gcc is at merging the similar code paths, 
simplifying its work seems worth it to me. The less repetitive code, the better.

MBR, Sergei

Re: [PATCH net-next 1/2] ravb: Add tx and rx clock internal delays mode of APSR

[Simon Horman]

On Fri, Jan 27, 2017 at 09:11:35PM +0300, Sergei Shtylyov wrote:
> On 01/27/2017 09:07 PM, Simon Horman wrote:
> 
> >>>>>From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> >>>>>
> >>>>>This patch enables tx and rx clock internal delay modes (TDM and RDM).
> >>>>>
> >>>>>This is to address a failure in the case of 1Gbps communication using the
> >>>>>by salvator-x board with the KSZ9031RNX phy. This has been reported to
> >>>>>occur with both the r8a7795 (H3) and r8a7796 (M3-W) SoCs.
> >>>>>
> >>>>>With this change APSR internal delay modes are enabled for
> >>>>>"rgmii-id", "rgmii-rxid" and "rgmii-txid" phy modes as follows:
> >>>>>
> >>>>>phy mode   | ASPR delay mode
> >>>>>-----------+----------------
> >>>>>rgmii-id   | TDM and RDM
> >>>>>rgmii-rxid | RDM
> >>>>>rgmii-txid | TDM
> >>>>>
> >>>>>Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> >>>>>Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> >>>>>
> >>>>>---
> >>>>>v1 [Simon Horman]
> >>>>>- Combined patches
> >>>>>- Reworded changelog
> >>>>>
> >>>>>v0 [Kazuya Mizuguchi]
> >>>>>---
> >>>>>drivers/net/ethernet/renesas/ravb.h      | 10 ++++++++++
> >>>>>drivers/net/ethernet/renesas/ravb_main.c | 29 +++++++++++++++++++++++++++++
> >>>>>2 files changed, 39 insertions(+)
> >>>>>
> >>>>>diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> >>>>>index f1109661a533..d7c91d48cc48 100644
> >>>>>--- a/drivers/net/ethernet/renesas/ravb.h
> >>>>>+++ b/drivers/net/ethernet/renesas/ravb.h
> >>>>[...]
> >>>>>@@ -248,6 +249,15 @@ enum ESR_BIT {
> >>>>>	ESR_EIL		= 0x00001000,
> >>>>>};
> >>>>>
> >>>>>+/* APSR */
> >>>>>+enum APSR_BIT {
> >>>>>+	APSR_MEMS		= 0x00000002,
> >>>>
> >>>>  Not documented in the revision 0.5 of the gen3 manual...
> >>>
> >>>It is in version 0.53 of the documentation but I think it can be dropped
> >>>from this patch as its unused.
> >>
> >>   No need to drop, let it be. :-)
> >>
> >>>>>+	APSR_CMSW		= 0x00000010,
> >>>
> >>>I think the above can also be dropped as it is unused.
> >>
> >>   No need.
> >
> >Ok, lets leave it and MEMS.
> >
> >>>>>+	APSR_DM			= 0x00006000,
> >>>>
> >>>>  ... and neither this field. Are these documented in the latter revs?
> >>>
> >>>This one is not. I can ask Mizuguchi-san to confirm it exists (with himself).
> >>
> >>   The whole patch depends on it! If it's not documented, I'd like a comment
> >>added, like I did for the other undocumented things in this driver...
> >
> >Sure.
> >
> >>[...]
> >>>>>diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >>>>>index 89ac1e3f6175..9fb4c04c5885 100644
> >>>>>--- a/drivers/net/ethernet/renesas/ravb_main.c
> >>>>>+++ b/drivers/net/ethernet/renesas/ravb_main.c
> >>>>>@@ -1904,6 +1904,29 @@ static void ravb_set_config_mode(struct net_device *ndev)
> >>>>>	}
> >>>>>}
> >>>>>
> >>>>>+static void ravb_set_delay_mode(struct net_device *ndev)
> >>>>>+{
> >>>>>+	struct ravb_private *priv = netdev_priv(ndev);
> >>>>>+
> >>>>>+	if (priv->chip_id != RCAR_GEN2) {
> >>>>>+		switch (priv->phy_interface) {
> >>>>>+		case PHY_INTERFACE_MODE_RGMII_ID:
> >>>>>+			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM |
> >>>>>+				   APSR_DM_TDM);
> >>>>>+			break;
> >>>>>+		case PHY_INTERFACE_MODE_RGMII_RXID:
> >>>>>+			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM);
> >>>>>+			break;
> >>>>>+		case PHY_INTERFACE_MODE_RGMII_TXID:
> >>>>>+			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_TDM);
> >>>>>+			break;
> >>>>>+		default:
> >>>>>+			ravb_modify(ndev, APSR, APSR_DM, 0);
> >>>>>+			break;
> >>>>>+		}
> >>>>
> >>>>  How about doing ravb_modify() only once?
> >>>
> >>>Something like this?
> >>>
> >>>		int set = 0;
> >>>
> >>>		if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> >>>		    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_RXID)
> >>>		    set |= APSR_DM_RDM;
> >>>
> >>>		if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
> >>>		    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID)
> >>>		    set |= APSR_DM_TDM;
> >>>
> >>>		ravb_modify(ndev, APSR, APSR_DM, set);
> >>>
> >>>I don't really see any advantage to it but I can change things around
> >>>however you like.
> >>
> >>   I didn't mean to replace *switch*, actually -- just to move ravb_modify()
> >>out of it.
> >
> >Ok, I will make it so.
> 
>    Your variant is probably even better! :-)
>    I'm not sure how good gcc is at merging the similar code paths,
> simplifying its work seems worth it to me. The less repetitive code, the
> better.

Ok, thanks. I'll go with my version.