| Message ID | 20190902080603.5636-3-horms+renesas@verge.net.au (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | ravb: Remove use of undocumented registers | expand |
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
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
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 >
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;