[net-next 0/3] ravb: Remove use of undocumented registers

[net-next 0/3] ravb: Remove use of undocumented registers

[Simon Horman]

Hi,

this short series cleans up the RAVB driver a little.

The first patch corrects the spelling of the FBP field of SFO register.
This register field is unused and should have no run-time effect.

The remaining two patches remove the use of undocumented registers
after some consultation with the internal Renesas BSP team.

All patches have been lightly tested on:
* E3 Ebisu
* H3 Salvator-XS (ES2.0)
* M3-W Salvator-XS
* M3-N Salvator-XS

Kazuya Mizuguchi (2):
  ravb: correct typo in FBP field of SFO register
  ravb: Remove undocumented processing

Simon Horman (1):
  ravb: TROCR register is only present on R-Car Gen3

 drivers/net/ethernet/renesas/ravb.h      |  9 ++-------
 drivers/net/ethernet/renesas/ravb_main.c | 21 ++++-----------------
 2 files changed, 6 insertions(+), 24 deletions(-)

-- 
2.11.0

[net-next 1/3] ravb: correct typo in FBP field of SFO register

[Simon Horman]

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

The field name is FBP rather than FPB.

This field is unused and could equally be removed from the driver entirely.
But there seems no harm in leaving as documentation of the presence of the
field.

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

v1 - Simon Horman
* Extracted from larger patch
* Wrote changelog

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index ac9195add811..bdb051f04b0c 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -317,7 +312,7 @@ enum UFCD_BIT {

 /* SFO */
 enum SFO_BIT {
-	SFO_FPB		= 0x0000003F,
+	SFO_FBP		= 0x0000003F,
 };

 /* RTC */
---
 drivers/net/ethernet/renesas/ravb.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index ac9195add811..2596a95a4300 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -317,7 +317,7 @@ enum UFCD_BIT {
 
 /* SFO */
 enum SFO_BIT {
-	SFO_FPB		= 0x0000003F,
+	SFO_FBP		= 0x0000003F,
 };
 
 /* RTC */
-- 
2.11.0

[net-next 2/3] ravb: Remove undocumented processing

[Simon Horman]

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

This patch removes the use of the undocumented registers
CDCR, LCCR, CERCR, CEECR and the undocumented BOC bit of the CCC register.

Current documentation for EtherAVB (ravb) describes the offset of
what the driver uses as the BOC bit as reserved and that only a value of
0 should be written. Furthermore, the offsets used for the undocumented
registers are also considered reserved nd should not be written to.

After some internal investigation with Renesas it remains unclear
why this driver accesses these fields but regardless of what the historical
reasons are the current code is considered incorrect.

Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 drivers/net/ethernet/renesas/ravb.h      |  5 -----
 drivers/net/ethernet/renesas/ravb_main.c | 15 ---------------
 2 files changed, 20 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 2596a95a4300..bdb051f04b0c 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -194,15 +194,11 @@ enum ravb_reg {
 	MAHR	= 0x05c0,
 	MALR	= 0x05c8,
 	TROCR	= 0x0700,	/* Undocumented? */
-	CDCR	= 0x0708,	/* Undocumented? */
-	LCCR	= 0x0710,	/* Undocumented? */
 	CEFCR	= 0x0740,
 	FRECR	= 0x0748,
 	TSFRCR	= 0x0750,
 	TLFRCR	= 0x0758,
 	RFCR	= 0x0760,
-	CERCR	= 0x0768,	/* Undocumented? */
-	CEECR	= 0x0770,	/* Undocumented? */
 	MAFCR	= 0x0778,
 };
 
@@ -220,7 +216,6 @@ enum CCC_BIT {
 	CCC_CSEL_HPB	= 0x00010000,
 	CCC_CSEL_ETH_TX	= 0x00020000,
 	CCC_CSEL_GMII_REF = 0x00030000,
-	CCC_BOC		= 0x00100000,	/* Undocumented? */
 	CCC_LBME	= 0x01000000,
 };
 
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 6cacd5e893ac..b538cc6fdbb7 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -447,12 +447,6 @@ static int ravb_dmac_init(struct net_device *ndev)
 	ravb_ring_format(ndev, RAVB_BE);
 	ravb_ring_format(ndev, RAVB_NC);
 
-#if defined(__LITTLE_ENDIAN)
-	ravb_modify(ndev, CCC, CCC_BOC, 0);
-#else
-	ravb_modify(ndev, CCC, CCC_BOC, CCC_BOC);
-#endif
-
 	/* Set AVB RX */
 	ravb_write(ndev,
 		   RCR_EFFS | RCR_ENCF | RCR_ETS0 | RCR_ESF | 0x18000000, RCR);
@@ -1629,15 +1623,6 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
 
 	nstats->tx_dropped += ravb_read(ndev, TROCR);
 	ravb_write(ndev, 0, TROCR);	/* (write clear) */
-	nstats->collisions += ravb_read(ndev, CDCR);
-	ravb_write(ndev, 0, CDCR);	/* (write clear) */
-	nstats->tx_carrier_errors += ravb_read(ndev, LCCR);
-	ravb_write(ndev, 0, LCCR);	/* (write clear) */
-
-	nstats->tx_carrier_errors += ravb_read(ndev, CERCR);
-	ravb_write(ndev, 0, CERCR);	/* (write clear) */
-	nstats->tx_carrier_errors += ravb_read(ndev, CEECR);
-	ravb_write(ndev, 0, CEECR);	/* (write clear) */
 
 	nstats->rx_packets = stats0->rx_packets + stats1->rx_packets;
 	nstats->tx_packets = stats0->tx_packets + stats1->tx_packets;
-- 
2.11.0

[net-next 3/3] ravb: TROCR register is only present on R-Car Gen3

[Simon Horman]

Only use the TROCR register on R-Car Gen3.
It is not present on other SoCs.

Offsets used for the undocumented registers are also considered reserved
and should not be written to.

After some internal investigation with Renesas it remains unclear why this
driver accesses these fields on R-Car Gen2 but regardless of what the
historical reasons are the current code is considered incorrect.

Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 drivers/net/ethernet/renesas/ravb.h      | 2 +-
 drivers/net/ethernet/renesas/ravb_main.c | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index bdb051f04b0c..a9c89d5d8898 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -193,7 +193,7 @@ enum ravb_reg {
 	GECMR	= 0x05b0,
 	MAHR	= 0x05c0,
 	MALR	= 0x05c8,
-	TROCR	= 0x0700,	/* Undocumented? */
+	TROCR	= 0x0700,	/* R-Car Gen3 only */
 	CEFCR	= 0x0740,
 	FRECR	= 0x0748,
 	TSFRCR	= 0x0750,
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index b538cc6fdbb7..de9aa8c47f1c 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1621,8 +1621,10 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
 	stats0 = &priv->stats[RAVB_BE];
 	stats1 = &priv->stats[RAVB_NC];
 
-	nstats->tx_dropped += ravb_read(ndev, TROCR);
-	ravb_write(ndev, 0, TROCR);	/* (write clear) */
+	if (priv->chip_id == RCAR_GEN3) {
+		nstats->tx_dropped += ravb_read(ndev, TROCR);
+		ravb_write(ndev, 0, TROCR);	/* (write clear) */
+	}
 
 	nstats->rx_packets = stats0->rx_packets + stats1->rx_packets;
 	nstats->tx_packets = stats0->tx_packets + stats1->tx_packets;
-- 
2.11.0

Re: [net-next 0/3] ravb: Remove use of undocumented registers

[Geert Uytterhoeven]

Hi Simon, Biju, Fabrizio,

On Mon, Sep 2, 2019 at 10:06 AM Simon Horman <horms+renesas@verge.net.au> wrote:
> this short series cleans up the RAVB driver a little.
>
> The first patch corrects the spelling of the FBP field of SFO register.
> This register field is unused and should have no run-time effect.
>
> The remaining two patches remove the use of undocumented registers
> after some consultation with the internal Renesas BSP team.
>
> All patches have been lightly tested on:
> * E3 Ebisu
> * H3 Salvator-XS (ES2.0)
> * M3-W Salvator-XS
> * M3-N Salvator-XS

It would be good if someone could test this on an R-Car Gen2 board
that uses ravb (iwg22d or iwg23s).

Thanks!

> Kazuya Mizuguchi (2):
>   ravb: correct typo in FBP field of SFO register
>   ravb: Remove undocumented processing
>
> Simon Horman (1):
>   ravb: TROCR register is only present on R-Car Gen3

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: [net-next 1/3] ravb: correct typo in FBP field of SFO register

[Yoshihiro Shimoda]

Hi Simon-san,

Thank you for the patch!

> From: Simon Horman, Sent: Monday, September 2, 2019 5:06 PM
> 
> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> 
> The field name is FBP rather than FPB.
> 
> This field is unused and could equally be removed from the driver entirely.
> But there seems no harm in leaving as documentation of the presence of the
> field.
> 
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
> v0 - Kazuya Mizuguchi
> 
> v1 - Simon Horman
> * Extracted from larger patch
> * Wrote changelog
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index ac9195add811..bdb051f04b0c 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -317,7 +312,7 @@ enum UFCD_BIT {
> 
>  /* SFO */
>  enum SFO_BIT {
> -	SFO_FPB		= 0x0000003F,
> +	SFO_FBP		= 0x0000003F,
>  };
> 
>  /* RTC */
> ---
>  drivers/net/ethernet/renesas/ravb.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h

This patch has two same diff. After removed either one of them,

Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Best regards,
Yoshihiro Shimoda


> index ac9195add811..2596a95a4300 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -317,7 +317,7 @@ enum UFCD_BIT {
> 
>  /* SFO */
>  enum SFO_BIT {
> -	SFO_FPB		= 0x0000003F,
> +	SFO_FBP		= 0x0000003F,
>  };
> 
>  /* RTC */
> --
> 2.11.0

RE: [net-next 2/3] ravb: Remove undocumented processing

[Yoshihiro Shimoda]

Hi Simon-san,

> From: Simon Horman, Sent: Monday, September 2, 2019 5:06 PM
> 
> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> 
> This patch removes the use of the undocumented registers
> CDCR, LCCR, CERCR, CEECR and the undocumented BOC bit of the CCC register.
> 
> Current documentation for EtherAVB (ravb) describes the offset of
> what the driver uses as the BOC bit as reserved and that only a value of
> 0 should be written. Furthermore, the offsets used for the undocumented
> registers are also considered reserved nd should not be written to.
> 
> After some internal investigation with Renesas it remains unclear
> why this driver accesses these fields but regardless of what the historical
> reasons are the current code is considered incorrect.
> 
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

Thank you for the patch!

Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Best regards,
Yoshihiro Shimoda

RE: [net-next 3/3] ravb: TROCR register is only present on R-Car Gen3

[Yoshihiro Shimoda]

Hi Simon-san,

> From: Simon Horman, Sent: Monday, September 2, 2019 5:06 PM
> 
> Only use the TROCR register on R-Car Gen3.
> It is not present on other SoCs.
> 
> Offsets used for the undocumented registers are also considered reserved
> and should not be written to.
> 
> After some internal investigation with Renesas it remains unclear why this
> driver accesses these fields on R-Car Gen2 but regardless of what the
> historical reasons are the current code is considered incorrect.
> 
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

Thank you for the patch!

Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Best regards,
Yoshihiro Shimoda

RE: [net-next 0/3] ravb: Remove use of undocumented registers

[Chris Paterson]

Hello Geert,

> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org>
> On Behalf Of Geert Uytterhoeven
> Sent: 02 September 2019 09:16
> 
> Hi Simon, Biju, Fabrizio,
> 
> On Mon, Sep 2, 2019 at 10:06 AM Simon Horman
> <horms+renesas@verge.net.au> wrote:
> > this short series cleans up the RAVB driver a little.
> >
> > The first patch corrects the spelling of the FBP field of SFO register.
> > This register field is unused and should have no run-time effect.
> >
> > The remaining two patches remove the use of undocumented registers
> > after some consultation with the internal Renesas BSP team.
> >
> > All patches have been lightly tested on:
> > * E3 Ebisu
> > * H3 Salvator-XS (ES2.0)
> > * M3-W Salvator-XS
> > * M3-N Salvator-XS
> 
> It would be good if someone could test this on an R-Car Gen2 board
> that uses ravb (iwg22d or iwg23s).

I've tried this series + net-next on the iwg23s haven't seen any issues from a quick sanity test.

Kind regards, Chris

> 
> Thanks!
> 
> > Kazuya Mizuguchi (2):
> >   ravb: correct typo in FBP field of SFO register
> >   ravb: Remove undocumented processing
> >
> > Simon Horman (1):
> >   ravb: TROCR register is only present on R-Car Gen3
> 
> 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: [net-next 1/3] ravb: correct typo in FBP field of SFO register

[Sergei Shtylyov]

Hello!

On 09/02/2019 11:06 AM, Simon Horman wrote:

> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> 
> The field name is FBP rather than FPB.
> 
> This field is unused and could equally be removed from the driver entirely.
> But there seems no harm in leaving as documentation of the presence of the
> field.
> 
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

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

> ---
> v0 - Kazuya Mizuguchi
> 
> v1 - Simon Horman
> * Extracted from larger patch

   I'd just claim the authorship in this case (and mentioned that it's based
on Mizuguchi-san's large patch right in the change log).

> * Wrote changelog
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index ac9195add811..bdb051f04b0c 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -317,7 +312,7 @@ enum UFCD_BIT {
> 
>  /* SFO */
>  enum SFO_BIT {
> -	SFO_FPB		= 0x0000003F,
> +	SFO_FBP		= 0x0000003F,
>  };
> 
>  /* RTC */
> ---

   This is where the actual patch starts, right?

>  drivers/net/ethernet/renesas/ravb.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index ac9195add811..2596a95a4300 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -317,7 +317,7 @@ enum UFCD_BIT {
>  
>  /* SFO */
>  enum SFO_BIT {
> -	SFO_FPB		= 0x0000003F,
> +	SFO_FBP		= 0x0000003F,
>  };
>  
>  /* RTC */

MBR, Sergei

Re: [net-next 2/3] ravb: Remove undocumented processing

[Sergei Shtylyov]

On 09/02/2019 11:06 AM, Simon Horman wrote:

> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> 
> This patch removes the use of the undocumented registers
> CDCR, LCCR, CERCR, CEECR and the undocumented BOC bit of the CCC register.

   The driver has many more #define's marked as undocumented. It's not clear
why you crammed the counters and the endianness bit in one patch. It clearly
needs to be split -- one patch for the MAC counters and one patch for the
AVB-DMAC bit.

> Current documentation for EtherAVB (ravb) describes the offset of
> what the driver uses as the BOC bit as reserved and that only a value of
> 0 should be written. Furthermore, the offsets used for the undocumented
> registers are also considered reserved nd should not be written to.
> 
> After some internal investigation with Renesas it remains unclear
> why this driver accesses these fields but regardless of what the historical
> reasons are the current code is considered incorrect.
> 
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
[...]

MBR, Sergei

Re: [net-next 3/3] ravb: TROCR register is only present on R-Car Gen3

[Sergei Shtylyov]

On 09/02/2019 11:06 AM, Simon Horman wrote:

> Only use the TROCR register on R-Car Gen3.
> It is not present on other SoCs.
> 
> Offsets used for the undocumented registers are also considered reserved
> and should not be written to.
> 
> After some internal investigation with Renesas it remains unclear why this
> driver accesses these fields on R-Car Gen2 but regardless of what the
> historical reasons are the current code is considered incorrect.

  Most probably copy&paste from sh_eth.c...

> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
[...]

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

MBR, Sergei

Re: [net-next 1/3] ravb: correct typo in FBP field of SFO register

[David Miller]

From: Simon Horman <horms+renesas@verge.net.au>
Date: Mon,  2 Sep 2019 10:06:01 +0200

> -	SFO_FPB		= 0x0000003F,
> +	SFO_FBP		= 0x0000003F,
>  };
> 
>  /* RTC */
> ---
>  drivers/net/ethernet/renesas/ravb.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h

Simon please clean this up, I don't know what happened here :-)

Re: [net-next 2/3] ravb: Remove undocumented processing

[Simon Horman]

On Mon, Sep 02, 2019 at 08:41:14PM +0300, Sergei Shtylyov wrote:
> On 09/02/2019 11:06 AM, Simon Horman wrote:
> 
> > From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> > 
> > This patch removes the use of the undocumented registers
> > CDCR, LCCR, CERCR, CEECR and the undocumented BOC bit of the CCC register.
> 
>    The driver has many more #define's marked as undocumented. It's not clear
> why you crammed the counters and the endianness bit in one patch. It clearly
> needs to be split -- one patch for the MAC counters and one patch for the
> AVB-DMAC bit.

Thanks for the suggestion, I will split the patch.

> > Current documentation for EtherAVB (ravb) describes the offset of
> > what the driver uses as the BOC bit as reserved and that only a value of
> > 0 should be written. Furthermore, the offsets used for the undocumented
> > registers are also considered reserved nd should not be written to.
> > 
> > After some internal investigation with Renesas it remains unclear
> > why this driver accesses these fields but regardless of what the historical
> > reasons are the current code is considered incorrect.
> > 
> > Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> [...]
> 
> MBR, Sergei
> 

Re: [net-next 1/3] ravb: correct typo in FBP field of SFO register

[Simon Horman]

On Mon, Sep 02, 2019 at 11:33:55AM -0700, David Miller wrote:
> From: Simon Horman <horms+renesas@verge.net.au>
> Date: Mon,  2 Sep 2019 10:06:01 +0200
> 
> > -	SFO_FPB		= 0x0000003F,
> > +	SFO_FBP		= 0x0000003F,
> >  };
> > 
> >  /* RTC */
> > ---
> >  drivers/net/ethernet/renesas/ravb.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> 
> Simon please clean this up, I don't know what happened here :-)

Yeah, sorry about that. I don't know how it happened either.