[PATCH net] gianfar: Don't force RGMII mode after reset, use defaults

[PATCH net] gianfar: Don't force RGMII mode after reset, use defaults

[Claudiu Manoil]

We received reports that forcing the MAC into RGMII (1 Gbps)
interface mode after MAC reset occasionally disrupts operation
of PHYs capable only of 100Mbps, even after adjust_link kicks
in and re-adjusts the interface mode in MACCFG2 accordingly.
Instead of forcing MACCFG2 into RGMII mode, let's use the default
reset value of MACCFG2 (that leaves the IF_Mode field unset) and
let adjust_link configure the correct mode from the beginning.
MACCFG2_INIT_SETTINGS is dropped, only the PAD_CRC bit is preserved,
the remaining fields (IF_Mode and Duplex) are left for adjust_link.
Tested on boards with gigabit PHYs.

MACCFG2_INIT_SETTINGS is there since day one, but the issue
got visible after introducing the MAC reset and reconfig support,
which added MAC reset at runtime, at interface open.

Fixes: a328ac92d314 ("gianfar: Implement MAC reset and reconfig procedure")

Reported-by: Daniel Walker <danielwa@cisco.com>
Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 3 ++-
 drivers/net/ethernet/freescale/gianfar.h | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 51ad864..0f4d13d 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -3173,7 +3173,8 @@ void gfar_mac_reset(struct gfar_private *priv)
 	gfar_write(&regs->minflr, MINFLR_INIT_SETTINGS);
 
 	/* Initialize MACCFG2. */
-	tempval = MACCFG2_INIT_SETTINGS;
+	tempval = gfar_read(&regs->maccfg2);
+	tempval |= MACCFG2_PAD_CRC;
 
 	/* eTSEC74 erratum: Rx frames of length MAXFRM or MAXFRM-1
 	 * are marked as truncated.  Avoid this by MACCFG2[Huge Frame]=1,
diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
index f472a6d..cc70e03 100644
--- a/drivers/net/ethernet/freescale/gianfar.h
+++ b/drivers/net/ethernet/freescale/gianfar.h
@@ -150,8 +150,8 @@ extern const char gfar_driver_version[];
 #define MACCFG1_SYNCD_TX_EN	0x00000002
 #define MACCFG1_TX_EN		0x00000001
 
-#define MACCFG2_INIT_SETTINGS	0x00007205
 #define MACCFG2_FULL_DUPLEX	0x00000001
+#define MACCFG2_PAD_CRC         0x00000004
 #define MACCFG2_IF              0x00000300
 #define MACCFG2_MII             0x00000100
 #define MACCFG2_GMII            0x00000200
-- 
2.7.4

Re: [PATCH net] gianfar: Don't force RGMII mode after reset, use defaults

[Daniel Walker]

Just adding HEMANT who was involved in the original debug.
Also Sathish did the current debug.


On Tue, Nov 12, 2019 at 04:55:11PM +0200, Claudiu Manoil wrote:
> We received reports that forcing the MAC into RGMII (1 Gbps)
> interface mode after MAC reset occasionally disrupts operation
> of PHYs capable only of 100Mbps, even after adjust_link kicks
> in and re-adjusts the interface mode in MACCFG2 accordingly.
> Instead of forcing MACCFG2 into RGMII mode, let's use the default
> reset value of MACCFG2 (that leaves the IF_Mode field unset) and
> let adjust_link configure the correct mode from the beginning.
> MACCFG2_INIT_SETTINGS is dropped, only the PAD_CRC bit is preserved,
> the remaining fields (IF_Mode and Duplex) are left for adjust_link.
> Tested on boards with gigabit PHYs.
> 
> MACCFG2_INIT_SETTINGS is there since day one, but the issue
> got visible after introducing the MAC reset and reconfig support,
> which added MAC reset at runtime, at interface open.
> 
> Fixes: a328ac92d314 ("gianfar: Implement MAC reset and reconfig procedure")
> 
> Reported-by: Daniel Walker <danielwa@cisco.com>
> Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
> ---
>  drivers/net/ethernet/freescale/gianfar.c | 3 ++-
>  drivers/net/ethernet/freescale/gianfar.h | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index 51ad864..0f4d13d 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -3173,7 +3173,8 @@ void gfar_mac_reset(struct gfar_private *priv)
>  	gfar_write(&regs->minflr, MINFLR_INIT_SETTINGS);
>  
>  	/* Initialize MACCFG2. */
> -	tempval = MACCFG2_INIT_SETTINGS;
> +	tempval = gfar_read(&regs->maccfg2);
> +	tempval |= MACCFG2_PAD_CRC;
>  
>  	/* eTSEC74 erratum: Rx frames of length MAXFRM or MAXFRM-1
>  	 * are marked as truncated.  Avoid this by MACCFG2[Huge Frame]=1,
> diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
> index f472a6d..cc70e03 100644
> --- a/drivers/net/ethernet/freescale/gianfar.h
> +++ b/drivers/net/ethernet/freescale/gianfar.h
> @@ -150,8 +150,8 @@ extern const char gfar_driver_version[];
>  #define MACCFG1_SYNCD_TX_EN	0x00000002
>  #define MACCFG1_TX_EN		0x00000001
>  
> -#define MACCFG2_INIT_SETTINGS	0x00007205
>  #define MACCFG2_FULL_DUPLEX	0x00000001
> +#define MACCFG2_PAD_CRC         0x00000004
>  #define MACCFG2_IF              0x00000300
>  #define MACCFG2_MII             0x00000100
>  #define MACCFG2_GMII            0x00000200
> -- 
> 2.7.4
> 

Re: [PATCH net] gianfar: Don't force RGMII mode after reset, use defaults

[HEMANT RAMDASI (hramdasi)]

    > Reported-by: Daniel Walker <danielwa@cisco.com>
    > Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
    > ---
    >  drivers/net/ethernet/freescale/gianfar.c | 3 ++-
    >  drivers/net/ethernet/freescale/gianfar.h | 2 +-
    >  2 files changed, 3 insertions(+), 2 deletions(-)
    > 
    > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
    > index 51ad864..0f4d13d 100644
    > --- a/drivers/net/ethernet/freescale/gianfar.c
    > +++ b/drivers/net/ethernet/freescale/gianfar.c
    > @@ -3173,7 +3173,8 @@ void gfar_mac_reset(struct gfar_private *priv)
    >  	gfar_write(&regs->minflr, MINFLR_INIT_SETTINGS);
    >  
    >  	/* Initialize MACCFG2. */
    > -	tempval = MACCFG2_INIT_SETTINGS;
    > +	tempval = gfar_read(&regs->maccfg2);
    > +	tempval |= MACCFG2_PAD_CRC;

This is not in sync with PAD/CRC definition of maccfg2 mentioned in p202 rm.
 

RE: [PATCH net] gianfar: Don't force RGMII mode after reset, use defaults

[Claudiu Manoil]

>-----Original Message-----
>From: HEMANT RAMDASI (hramdasi) <hramdasi@cisco.com>
>Sent: Tuesday, November 12, 2019 6:56 PM
>To: Daniel Walker (danielwa) <danielwa@cisco.com>; Claudiu Manoil
><claudiu.manoil@nxp.com>
>Cc: David S . Miller <davem@davemloft.net>; netdev@vger.kernel.org;
>Sathish Jarugumalli -X (sjarugum - ARICENT TECHNOLOGIES HOLDINGS
>LIMITED at Cisco) <sjarugum@cisco.com>
>Subject: Re: [PATCH net] gianfar: Don't force RGMII mode after reset, use
>defaults
>
>    > Reported-by: Daniel Walker <danielwa@cisco.com>
>    > Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
>    > ---
>    >  drivers/net/ethernet/freescale/gianfar.c | 3 ++-
>    >  drivers/net/ethernet/freescale/gianfar.h | 2 +-
>    >  2 files changed, 3 insertions(+), 2 deletions(-)
>    >
>    > diff --git a/drivers/net/ethernet/freescale/gianfar.c
>b/drivers/net/ethernet/freescale/gianfar.c
>    > index 51ad864..0f4d13d 100644
>    > --- a/drivers/net/ethernet/freescale/gianfar.c
>    > +++ b/drivers/net/ethernet/freescale/gianfar.c
>    > @@ -3173,7 +3173,8 @@ void gfar_mac_reset(struct gfar_private *priv)
>    >  	gfar_write(&regs->minflr, MINFLR_INIT_SETTINGS);
>    >
>    >  	/* Initialize MACCFG2. */
>    > -	tempval = MACCFG2_INIT_SETTINGS;
>    > +	tempval = gfar_read(&regs->maccfg2);
>    > +	tempval |= MACCFG2_PAD_CRC;
>
>This is not in sync with PAD/CRC definition of maccfg2 mentioned in p202 rm.
>

I don know what you mean.  The definition of this bit is:
" Pad and append CRC . This bit is cleared by default.
This bit must be set when in half-duplex mode (MACCFG2[Full_Duplex] is cleared).
0 Frames presented to the MAC have a valid length and contain a CRC.
1 The MAC pads all transmitted short frames and appends a CRC to every frame regardless of padding
requirement."

So the driver sets this bit to have small frames padded. It always worked this way,
and I retested on P2020RDB and LS1021RDB and works.
Are you saying that padding does not work on your board with the current upstream code?

-Claudiu

Re: [PATCH net] gianfar: Don't force RGMII mode after reset, use defaults

[HEMANT RAMDASI (hramdasi)]

    >
    >This is not in sync with PAD/CRC definition of maccfg2 mentioned in p202 rm.
    >
    
    > I don know what you mean.  The definition of this bit is:
    > " Pad and append CRC . This bit is cleared by default.
    > This bit must be set when in half-duplex mode (MACCFG2[Full_Duplex] is cleared).

Should the bit be clear when in full duplex or it does not matter?   

    > 0 Frames presented to the MAC have a valid length and contain a CRC.
    > 1 The MAC pads all transmitted short frames and appends a CRC to every frame regardless of padding
    > requirement."
    
    > So the driver sets this bit to have small frames padded. It always worked this way,
    > and I retested on P2020RDB and LS1021RDB and works.
    > Are you saying that padding does not work on your board with the current upstream code?

It works but the settings does not match with what's mentioned in p2020 rm and the bit 29 becomes redundant.
    
-Hemant
    
    

RE: [PATCH net] gianfar: Don't force RGMII mode after reset, use defaults

[Claudiu Manoil]

>-----Original Message-----
>From: HEMANT RAMDASI (hramdasi) <hramdasi@cisco.com>
[...]
>> This bit must be set when in half-duplex mode (MACCFG2[Full_Duplex] is cleared).
>
> Should the bit be clear when in full duplex or it does not matter?
>

From my tests, in full duplex mode small frames won't get padded if this bit is disabled,
and will be counted as undersize frames and dropped. So this bit needs to be set
in full duplex mode to get packets smaller than 64B past the MAC (w/o software padding).
The statement above likely means that for half-duplex mode packets cannot egress
the MAC regardless of their size if the PAD_CRC bit is not set.  At least this is consistent
with my experiments.

>> 0 Frames presented to the MAC have a valid length and contain a CRC.
>> 1 The MAC pads all transmitted short frames and appends a CRC to every frame
>> regardless of padding requirement."
>>
>> So the driver sets this bit to have small frames padded. It always worked
>> this way, and I retested on P2020RDB and LS1021RDB and works.
>> Are you saying that padding does not work on your board with the current
>> upstream code?
>
>It works but the settings does not match with what's mentioned in p2020 rm
>and the bit 29 becomes redundant.
>

So, the PAD_CRC bit is not redundant, and for half-duplex mode it looks like this bit is
even mandatory to have Tx traffic at all.

This patch is however not about the PAD_CRC bit. It's about default interface mode
setting in the MACCFG2 register.  And I just noticed to my surprise that with the default
reset value for interface mode (00b) the SGMII link won't get up on my P2020RDB-PC
board, while the RGMII links don't have this problem.  On the newer LS1021ATWR board
there's no such issue (both sgmii and rgmii links work with default IF_Mode of 00).
So looks like IF_Mode was being initialized to "byte mode" (10b, aka RGMII 1G mode)
with a reason, so that older boards have functional links in all cases (i.e. sgmii).

Better to drop this patch for now to avoid such regressions for older boards.  This is what
happens when legacy driver code like this gets changed.  I'll have to ask h/w ppl for
clarification on this.

Thanks,
Claudiu

Re: [PATCH net] gianfar: Don't force RGMII mode after reset, use defaults

[HEMANT RAMDASI (hramdasi)]

    >> This bit must be set when in half-duplex mode (MACCFG2[Full_Duplex] is cleared).
    >
    > Should the bit be clear when in full duplex or it does not matter?
    >
    
    > From my tests, in full duplex mode small frames won't get padded if this bit is disabled,
    > and will be counted as undersize frames and dropped. So this bit needs to be set
    > in full duplex mode to get packets smaller than 64B past the MAC (w/o software padding).

This is little strange as we do not see this problem on all pkt type, icmp passes well and we observed issue with tftp ack. 
So there is no link issue.

    > The statement above likely means that for half-duplex mode packets cannot egress
    > the MAC regardless of their size if the PAD_CRC bit is not set.  At least this is consistent
    > with my experiments.
    
    >> 0 Frames presented to the MAC have a valid length and contain a CRC.
    >> 1 The MAC pads all transmitted short frames and appends a CRC to every frame
    >> regardless of padding requirement."
    >>
    >> So the driver sets this bit to have small frames padded. It always worked
    >> this way, and I retested on P2020RDB and LS1021RDB and works.
    >> Are you saying that padding does not work on your board with the current
    >> upstream code?
    >
    >It works but the settings does not match with what's mentioned in p2020 rm
    >and the bit 29 becomes redundant.
    >
    
    > So, the PAD_CRC bit is not redundant, and for half-duplex mode it looks like this bit is
    > even mandatory to have Tx traffic at all.
    
    > This patch is however not about the PAD_CRC bit. It's about default interface mode

This patch looks like doing the same though..

    > setting in the MACCFG2 register.  And I just noticed to my surprise that with the default
    > reset value for interface mode (00b) the SGMII link won't get up on my P2020RDB-PC
    > board, while the RGMII links don't have this problem.  On the newer LS1021ATWR board
    > there's no such issue (both sgmii and rgmii links work with default IF_Mode of 00).
    > So looks like IF_Mode was being initialized to "byte mode" (10b, aka RGMII 1G mode)
    > with a reason, so that older boards have functional links in all cases (i.e. sgmii).

    > Better to drop this patch for now to avoid such regressions for older boards.  This is what
    > happens when legacy driver code like this gets changed.  I'll have to ask h/w ppl for
    > clarification on this.

Strangely reported issue is observed only on few P2020 devices.
    
-hemant
    
    

RE: [PATCH net] gianfar: Don't force RGMII mode after reset, use defaults

[Claudiu Manoil]

>-----Original Message-----
>From: HEMANT RAMDASI (hramdasi) <hramdasi@cisco.com>
[..]
>
>>> This bit must be set when in half-duplex mode (MACCFG2[Full_Duplex] is cleared).
>>
>> Should the bit be clear when in full duplex or it does not matter?
>>
>
>> From my tests, in full duplex mode small frames won't get padded if this bit is disabled,
>> and will be counted as undersize frames and dropped. So this bit needs to be set
>> in full duplex mode to get packets smaller than 64B past the MAC (w/o software padding).
>
>This is little strange as we do not see this problem on all pkt type, icmp passes
>well and we observed issue with tftp ack.

I tested on a 1Gbit (full duplex) link, and ARP and small ICMP ipv4 packets were not passing
with the PAD_CRC bit disabled.

Re: [PATCH net] gianfar: Don't force RGMII mode after reset, use defaults

[Daniel Walker (danielwa)]

On Wed, Nov 13, 2019 at 04:01:39PM +0000, Claudiu Manoil wrote:
> >-----Original Message-----
> >From: HEMANT RAMDASI (hramdasi) <hramdasi@cisco.com>
> [..]
> >
> >>> This bit must be set when in half-duplex mode (MACCFG2[Full_Duplex] is cleared).
> >>
> >> Should the bit be clear when in full duplex or it does not matter?
> >>
> >
> >> From my tests, in full duplex mode small frames won't get padded if this bit is disabled,
> >> and will be counted as undersize frames and dropped. So this bit needs to be set
> >> in full duplex mode to get packets smaller than 64B past the MAC (w/o software padding).
> >
> >This is little strange as we do not see this problem on all pkt type, icmp passes
> >well and we observed issue with tftp ack.
> 
> I tested on a 1Gbit (full duplex) link, and ARP and small ICMP ipv4 packets were not passing
> with the PAD_CRC bit disabled.


Have you looked into this patch any further ?

Daniel

RE: [PATCH net] gianfar: Don't force RGMII mode after reset, use defaults

[Claudiu Manoil]

>-----Original Message-----
>From: Daniel Walker (danielwa) <danielwa@cisco.com>
>Sent: Wednesday, February 19, 2020 8:58 PM
>To: Claudiu Manoil <claudiu.manoil@nxp.com>
>Cc: HEMANT RAMDASI (hramdasi) <hramdasi@cisco.com>; David S . Miller
><davem@davemloft.net>; netdev@vger.kernel.org; Sathish Jarugumalli -X
>(sjarugum - ARICENT TECHNOLOGIES MAURIITIUS LIMITED at Cisco)
><sjarugum@cisco.com>
>Subject: Re: [PATCH net] gianfar: Don't force RGMII mode after reset, use
>defaults
>
>On Wed, Nov 13, 2019 at 04:01:39PM +0000, Claudiu Manoil wrote:
>> >-----Original Message-----
>> >From: HEMANT RAMDASI (hramdasi) <hramdasi@cisco.com>
>> [..]
>> >
>> >>> This bit must be set when in half-duplex mode (MACCFG2[Full_Duplex]
>is cleared).
>> >>
>> >> Should the bit be clear when in full duplex or it does not matter?
>> >>
>> >
>> >> From my tests, in full duplex mode small frames won't get padded if
>> >> this bit is disabled, and will be counted as undersize frames and
>> >> dropped. So this bit needs to be set in full duplex mode to get packets
>smaller than 64B past the MAC (w/o software padding).
>> >
>> >This is little strange as we do not see this problem on all pkt type,
>> >icmp passes well and we observed issue with tftp ack.
>>
>> I tested on a 1Gbit (full duplex) link, and ARP and small ICMP ipv4
>> packets were not passing with the PAD_CRC bit disabled.
>
>
>Have you looked into this patch any further ?
>

Hi,
I have an idea on how to parameterize the interface mode setting.
Let me try a patch and if it passes on my boards I'll send it as v2.

Re: [PATCH net] gianfar: Don't force RGMII mode after reset, use defaults

[Daniel Walker (danielwa)]

On Tue, Nov 12, 2019 at 04:55:11PM +0200, Claudiu Manoil wrote:
> We received reports that forcing the MAC into RGMII (1 Gbps)
> interface mode after MAC reset occasionally disrupts operation
> of PHYs capable only of 100Mbps, even after adjust_link kicks
> in and re-adjusts the interface mode in MACCFG2 accordingly.
> Instead of forcing MACCFG2 into RGMII mode, let's use the default
> reset value of MACCFG2 (that leaves the IF_Mode field unset) and
> let adjust_link configure the correct mode from the beginning.
> MACCFG2_INIT_SETTINGS is dropped, only the PAD_CRC bit is preserved,
> the remaining fields (IF_Mode and Duplex) are left for adjust_link.
> Tested on boards with gigabit PHYs.
> 
> MACCFG2_INIT_SETTINGS is there since day one, but the issue
> got visible after introducing the MAC reset and reconfig support,
> which added MAC reset at runtime, at interface open.
> 
> Fixes: a328ac92d314 ("gianfar: Implement MAC reset and reconfig procedure")
> 


We tested these changes on our side, and it appears to solve the problem.
Have been able to resolve the issues which you have seen on your side?


Daniel