diff mbox

[U-Boot,Drivers,14/19] net/macb: workaround for transmission hang issue

Message ID a0a27b1d7bfcb3a1c8e3bb7cdeffb6969934f9d7.1351877124.git.vipin.kumar@st.com
State Superseded
Delegated to: Albert ARIBAUD
Headers show

Commit Message

Vipin Kumar Nov. 2, 2012, 5:39 p.m. UTC
From: Shiraz Hashim <shiraz.hashim@st.com>

It is observed on SPEAr320S RMII#1 interface that on transmitting
packets the MAC dma hangs randomly and constantly showing busy tx-go
state.

It comes out if this situation only when Transmission is disabled and
enabled again.

Since it happens randomly and u-boot doesn't require high performance we
disable TE and re-enable it on each transmission. We also change number
of transmit descriptor to 1 as we would not require more than it, further
it would not alter GMAC notion of transmit descriptor start queue as it
always point to same descriptor.

Signed-off-by: Shiraz Hashim <shiraz.hashim@st.com>
---
 drivers/net/macb.c | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

Comments

Albert ARIBAUD Feb. 3, 2013, 11:19 a.m. UTC | #1
Hi Vipin,

On Fri, 2 Nov 2012 23:09:59 +0530, Vipin Kumar <vipin.kumar@st.com>
wrote:

> From: Shiraz Hashim <shiraz.hashim@st.com>
> 
> It is observed on SPEAr320S RMII#1 interface that on transmitting
> packets the MAC dma hangs randomly and constantly showing busy tx-go
> state.
> 
> It comes out if this situation only when Transmission is disabled and
> enabled again.
> 
> Since it happens randomly and u-boot doesn't require high performance we
> disable TE and re-enable it on each transmission. We also change number
> of transmit descriptor to 1 as we would not require more than it, further
> it would not alter GMAC notion of transmit descriptor start queue as it
> always point to same descriptor.
> 
> Signed-off-by: Shiraz Hashim <shiraz.hashim@st.com>
> ---
>  drivers/net/macb.c | 38 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> index ac25b52..17bad33 100644
> --- a/drivers/net/macb.c
> +++ b/drivers/net/macb.c
> @@ -55,7 +55,7 @@
>  
>  #define CONFIG_SYS_MACB_RX_BUFFER_SIZE		4096
>  #define CONFIG_SYS_MACB_RX_RING_SIZE		(CONFIG_SYS_MACB_RX_BUFFER_SIZE / 128)
> -#define CONFIG_SYS_MACB_TX_RING_SIZE		16
> +#define CONFIG_SYS_MACB_TX_RING_SIZE		1
>  #define CONFIG_SYS_MACB_TX_TIMEOUT		1000
>  #define CONFIG_SYS_MACB_AUTONEG_TIMEOUT	5000000
>  
> @@ -226,7 +226,13 @@ static int macb_send(struct eth_device *netdev, void *packet, int length)
>  	macb->tx_ring[tx_head].ctrl = ctrl;
>  	macb->tx_ring[tx_head].addr = paddr;
>  	barrier();
> -	macb_writel(macb, NCR, MACB_BIT(TE) | MACB_BIT(RE) | MACB_BIT(TSTART));
> +	/*
> +	 * Due to issues on SPEAr320 RMII, disable TE first so that
> +	 * controller can come out if it is hanged during transmission
> +	 */
> +	macb_writel(macb, NCR, macb_readl(macb, NCR) & ~MACB_BIT(TE));
> +	macb_writel(macb, NCR, macb_readl(macb, NCR) |
> +			MACB_BIT(TE) | MACB_BIT(TSTART));
>  
>  	/*
>  	 * I guess this is necessary because the networking core may
> @@ -444,6 +450,31 @@ static int macb_phy_init(struct macb_device *macb)
>  	}
>  }
>  
> +static void macb_reset_hw(struct macb_device *bp)
> +{
> +	/* Make sure we have the write buffer for ourselves */
> +	barrier();
> +	/*
> +	 * Disable RX and TX (XXX: Should we halt the transmission
> +	 * more gracefully?) and we should not close the mdio port
> +	 */
> +	macb_writel(bp, NCR, 0);
> +
> +	/* Clear the stats registers (XXX: Update stats first?) */
> +	macb_writel(bp, NCR, MACB_BIT(CLRSTAT));
> +
> +	/* keep the mdio port , otherwise other eth will not work */
> +	macb_writel(bp, NCR, MACB_BIT(MPE));
> +
> +	/* Clear all status flags */
> +	macb_writel(bp, TSR, ~0UL);
> +	macb_writel(bp, RSR, ~0UL);
> +
> +	/* Disable all interrupts */
> +	macb_writel(bp, IDR, ~0UL);
> +	macb_readl(bp, ISR);
> +}
> +
>  static int macb_init(struct eth_device *netdev, bd_t *bd)
>  {
>  	struct macb_device *macb = to_macb(netdev);
> @@ -520,8 +551,7 @@ static void macb_halt(struct eth_device *netdev)
>  		tsr = macb_readl(macb, TSR);
>  	} while (tsr & MACB_BIT(TGO));
>  
> -	/* Disable TX and RX, and clear statistics */
> -	macb_writel(macb, NCR, MACB_BIT(CLRSTAT));
> +	macb_reset_hw(macb);
>  }
>  
>  static int macb_write_hwaddr(struct eth_device *dev)

This patch did not reappear in later versions of the series, and no
other standalone patch seems to match it. Was it dropped?

Amicalement,
Albert ARIBAUD Feb. 28, 2013, 12:59 p.m. UTC | #2
On Sun, 3 Feb 2013 12:19:26 +0100, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:

> Hi Vipin,
> 
> On Fri, 2 Nov 2012 23:09:59 +0530, Vipin Kumar <vipin.kumar@st.com>
> wrote:
> 
> > From: Shiraz Hashim <shiraz.hashim@st.com>
> > 
> > It is observed on SPEAr320S RMII#1 interface that on transmitting
> > packets the MAC dma hangs randomly and constantly showing busy tx-go
> > state.
> > 
> > It comes out if this situation only when Transmission is disabled and
> > enabled again.
> > 
> > Since it happens randomly and u-boot doesn't require high performance we
> > disable TE and re-enable it on each transmission. We also change number
> > of transmit descriptor to 1 as we would not require more than it, further
> > it would not alter GMAC notion of transmit descriptor start queue as it
> > always point to same descriptor.
> > 
> > Signed-off-by: Shiraz Hashim <shiraz.hashim@st.com>
> > ---
> >  drivers/net/macb.c | 38 ++++++++++++++++++++++++++++++++++----
> >  1 file changed, 34 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> > index ac25b52..17bad33 100644
> > --- a/drivers/net/macb.c
> > +++ b/drivers/net/macb.c
> > @@ -55,7 +55,7 @@
> >  
> >  #define CONFIG_SYS_MACB_RX_BUFFER_SIZE		4096
> >  #define CONFIG_SYS_MACB_RX_RING_SIZE		(CONFIG_SYS_MACB_RX_BUFFER_SIZE / 128)
> > -#define CONFIG_SYS_MACB_TX_RING_SIZE		16
> > +#define CONFIG_SYS_MACB_TX_RING_SIZE		1
> >  #define CONFIG_SYS_MACB_TX_TIMEOUT		1000
> >  #define CONFIG_SYS_MACB_AUTONEG_TIMEOUT	5000000
> >  
> > @@ -226,7 +226,13 @@ static int macb_send(struct eth_device *netdev, void *packet, int length)
> >  	macb->tx_ring[tx_head].ctrl = ctrl;
> >  	macb->tx_ring[tx_head].addr = paddr;
> >  	barrier();
> > -	macb_writel(macb, NCR, MACB_BIT(TE) | MACB_BIT(RE) | MACB_BIT(TSTART));
> > +	/*
> > +	 * Due to issues on SPEAr320 RMII, disable TE first so that
> > +	 * controller can come out if it is hanged during transmission
> > +	 */
> > +	macb_writel(macb, NCR, macb_readl(macb, NCR) & ~MACB_BIT(TE));
> > +	macb_writel(macb, NCR, macb_readl(macb, NCR) |
> > +			MACB_BIT(TE) | MACB_BIT(TSTART));
> >  
> >  	/*
> >  	 * I guess this is necessary because the networking core may
> > @@ -444,6 +450,31 @@ static int macb_phy_init(struct macb_device *macb)
> >  	}
> >  }
> >  
> > +static void macb_reset_hw(struct macb_device *bp)
> > +{
> > +	/* Make sure we have the write buffer for ourselves */
> > +	barrier();
> > +	/*
> > +	 * Disable RX and TX (XXX: Should we halt the transmission
> > +	 * more gracefully?) and we should not close the mdio port
> > +	 */
> > +	macb_writel(bp, NCR, 0);
> > +
> > +	/* Clear the stats registers (XXX: Update stats first?) */
> > +	macb_writel(bp, NCR, MACB_BIT(CLRSTAT));
> > +
> > +	/* keep the mdio port , otherwise other eth will not work */
> > +	macb_writel(bp, NCR, MACB_BIT(MPE));
> > +
> > +	/* Clear all status flags */
> > +	macb_writel(bp, TSR, ~0UL);
> > +	macb_writel(bp, RSR, ~0UL);
> > +
> > +	/* Disable all interrupts */
> > +	macb_writel(bp, IDR, ~0UL);
> > +	macb_readl(bp, ISR);
> > +}
> > +
> >  static int macb_init(struct eth_device *netdev, bd_t *bd)
> >  {
> >  	struct macb_device *macb = to_macb(netdev);
> > @@ -520,8 +551,7 @@ static void macb_halt(struct eth_device *netdev)
> >  		tsr = macb_readl(macb, TSR);
> >  	} while (tsr & MACB_BIT(TGO));
> >  
> > -	/* Disable TX and RX, and clear statistics */
> > -	macb_writel(macb, NCR, MACB_BIT(CLRSTAT));
> > +	macb_reset_hw(macb);
> >  }
> >  
> >  static int macb_write_hwaddr(struct eth_device *dev)
> 
> This patch did not reappear in later versions of the series, and no
> other standalone patch seems to match it. Was it dropped?

Ping?

Amicalement,
Bo Shen March 1, 2013, 3:08 a.m. UTC | #3
Hi All,

On 02/28/2013 08:59 PM, Albert ARIBAUD wrote:
> On Sun, 3 Feb 2013 12:19:26 +0100, Albert ARIBAUD
> <albert.u.boot@aribaud.net>  wrote:
>
>> Hi Vipin,
>>
>> On Fri, 2 Nov 2012 23:09:59 +0530, Vipin Kumar<vipin.kumar@st.com>
>> wrote:
>>
>>> From: Shiraz Hashim<shiraz.hashim@st.com>
>>>
>>> It is observed on SPEAr320S RMII#1 interface that on transmitting
>>> packets the MAC dma hangs randomly and constantly showing busy tx-go
>>> state.
>>>
>>> It comes out if this situation only when Transmission is disabled and
>>> enabled again.
>>>
>>> Since it happens randomly and u-boot doesn't require high performance we
>>> disable TE and re-enable it on each transmission. We also change number
>>> of transmit descriptor to 1 as we would not require more than it, further
>>> it would not alter GMAC notion of transmit descriptor start queue as it
>>> always point to same descriptor.
>>>
>>> Signed-off-by: Shiraz Hashim<shiraz.hashim@st.com>
>>> ---
>>>   drivers/net/macb.c | 38 ++++++++++++++++++++++++++++++++++----
>>>   1 file changed, 34 insertions(+), 4 deletions(-)

Tested on Atmel EK board. It works.

Tested-by: Bo Shen <voice.shen@atmel.com>

BTW, would this be implemented as a workaround only for SPEAr320S?

Best Regards,
Bo Shen

>>> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
>>> index ac25b52..17bad33 100644
>>> --- a/drivers/net/macb.c
>>> +++ b/drivers/net/macb.c
>>> @@ -55,7 +55,7 @@
>>>
>>>   #define CONFIG_SYS_MACB_RX_BUFFER_SIZE		4096
>>>   #define CONFIG_SYS_MACB_RX_RING_SIZE		(CONFIG_SYS_MACB_RX_BUFFER_SIZE / 128)
>>> -#define CONFIG_SYS_MACB_TX_RING_SIZE		16
>>> +#define CONFIG_SYS_MACB_TX_RING_SIZE		1
>>>   #define CONFIG_SYS_MACB_TX_TIMEOUT		1000
>>>   #define CONFIG_SYS_MACB_AUTONEG_TIMEOUT	5000000
>>>
>>> @@ -226,7 +226,13 @@ static int macb_send(struct eth_device *netdev, void *packet, int length)
>>>   	macb->tx_ring[tx_head].ctrl = ctrl;
>>>   	macb->tx_ring[tx_head].addr = paddr;
>>>   	barrier();
>>> -	macb_writel(macb, NCR, MACB_BIT(TE) | MACB_BIT(RE) | MACB_BIT(TSTART));
>>> +	/*
>>> +	 * Due to issues on SPEAr320 RMII, disable TE first so that
>>> +	 * controller can come out if it is hanged during transmission
>>> +	 */
>>> +	macb_writel(macb, NCR, macb_readl(macb, NCR)&  ~MACB_BIT(TE));
>>> +	macb_writel(macb, NCR, macb_readl(macb, NCR) |
>>> +			MACB_BIT(TE) | MACB_BIT(TSTART));
>>>
>>>   	/*
>>>   	 * I guess this is necessary because the networking core may
>>> @@ -444,6 +450,31 @@ static int macb_phy_init(struct macb_device *macb)
>>>   	}
>>>   }
>>>
>>> +static void macb_reset_hw(struct macb_device *bp)
>>> +{
>>> +	/* Make sure we have the write buffer for ourselves */
>>> +	barrier();
>>> +	/*
>>> +	 * Disable RX and TX (XXX: Should we halt the transmission
>>> +	 * more gracefully?) and we should not close the mdio port
>>> +	 */
>>> +	macb_writel(bp, NCR, 0);
>>> +
>>> +	/* Clear the stats registers (XXX: Update stats first?) */
>>> +	macb_writel(bp, NCR, MACB_BIT(CLRSTAT));
>>> +
>>> +	/* keep the mdio port , otherwise other eth will not work */
>>> +	macb_writel(bp, NCR, MACB_BIT(MPE));
>>> +
>>> +	/* Clear all status flags */
>>> +	macb_writel(bp, TSR, ~0UL);
>>> +	macb_writel(bp, RSR, ~0UL);
>>> +
>>> +	/* Disable all interrupts */
>>> +	macb_writel(bp, IDR, ~0UL);
>>> +	macb_readl(bp, ISR);
>>> +}
>>> +
>>>   static int macb_init(struct eth_device *netdev, bd_t *bd)
>>>   {
>>>   	struct macb_device *macb = to_macb(netdev);
>>> @@ -520,8 +551,7 @@ static void macb_halt(struct eth_device *netdev)
>>>   		tsr = macb_readl(macb, TSR);
>>>   	} while (tsr&  MACB_BIT(TGO));
>>>
>>> -	/* Disable TX and RX, and clear statistics */
>>> -	macb_writel(macb, NCR, MACB_BIT(CLRSTAT));
>>> +	macb_reset_hw(macb);
>>>   }
>>>
>>>   static int macb_write_hwaddr(struct eth_device *dev)
>>
>> This patch did not reappear in later versions of the series, and no
>> other standalone patch seems to match it. Was it dropped?
>
> Ping?
>
> Amicalement,
Vipin Kumar March 1, 2013, 3:40 a.m. UTC | #4
On 3/1/2013 8:38 AM, Bo Shen wrote:
> Hi All,
>
> On 02/28/2013 08:59 PM, Albert ARIBAUD wrote:
>> On Sun, 3 Feb 2013 12:19:26 +0100, Albert ARIBAUD
>> <albert.u.boot@aribaud.net> wrote:
>>
>>> Hi Vipin,
>>>
>>> On Fri, 2 Nov 2012 23:09:59 +0530, Vipin Kumar<vipin.kumar@st.com>
>>> wrote:
>>>
>>>> From: Shiraz Hashim<shiraz.hashim@st.com>
>>>>
>>>> It is observed on SPEAr320S RMII#1 interface that on transmitting
>>>> packets the MAC dma hangs randomly and constantly showing busy tx-go
>>>> state.
>>>>
>>>> It comes out if this situation only when Transmission is disabled and
>>>> enabled again.
>>>>
>>>> Since it happens randomly and u-boot doesn't require high
>>>> performance we
>>>> disable TE and re-enable it on each transmission. We also change number
>>>> of transmit descriptor to 1 as we would not require more than it,
>>>> further
>>>> it would not alter GMAC notion of transmit descriptor start queue as it
>>>> always point to same descriptor.
>>>>
>>>> Signed-off-by: Shiraz Hashim<shiraz.hashim@st.com>
>>>> ---
>>>> drivers/net/macb.c | 38 ++++++++++++++++++++++++++++++++++----
>>>> 1 file changed, 34 insertions(+), 4 deletions(-)
>
> Tested on Atmel EK board. It works.
>
> Tested-by: Bo Shen <voice.shen@atmel.com>
>
> BTW, would this be implemented as a workaround only for SPEAr320S?
>

Yes, The idea was to implement this only for spear320s
After your tested-by, I am thinking may be I can make a separate 
workaround config which can be enabled by any board using this peripheral.

Is that OK?

Regards
Vipin
Vipin Kumar March 1, 2013, 3:41 a.m. UTC | #5
On 2/28/2013 6:29 PM, Albert ARIBAUD wrote:
> On Sun, 3 Feb 2013 12:19:26 +0100, Albert ARIBAUD
> <albert.u.boot@aribaud.net>  wrote:
>
>> Hi Vipin,
>>
>> On Fri, 2 Nov 2012 23:09:59 +0530, Vipin Kumar<vipin.kumar@st.com>
>> wrote:
>>
>>> From: Shiraz Hashim<shiraz.hashim@st.com>
>>>
>>> It is observed on SPEAr320S RMII#1 interface that on transmitting
>>> packets the MAC dma hangs randomly and constantly showing busy tx-go
>>> state.
>>>
>>> It comes out if this situation only when Transmission is disabled and
>>> enabled again.
>>>
>>> Since it happens randomly and u-boot doesn't require high performance we
>>> disable TE and re-enable it on each transmission. We also change number
>>> of transmit descriptor to 1 as we would not require more than it, further
>>> it would not alter GMAC notion of transmit descriptor start queue as it
>>> always point to same descriptor.
>>>
>>> Signed-off-by: Shiraz Hashim<shiraz.hashim@st.com>
>>> ---
>>>   drivers/net/macb.c | 38 ++++++++++++++++++++++++++++++++++----
>>>   1 file changed, 34 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
>>> index ac25b52..17bad33 100644
>>> --- a/drivers/net/macb.c
>>> +++ b/drivers/net/macb.c
>>> @@ -55,7 +55,7 @@
>>>
>>>   #define CONFIG_SYS_MACB_RX_BUFFER_SIZE		4096
>>>   #define CONFIG_SYS_MACB_RX_RING_SIZE		(CONFIG_SYS_MACB_RX_BUFFER_SIZE / 128)
>>> -#define CONFIG_SYS_MACB_TX_RING_SIZE		16
>>> +#define CONFIG_SYS_MACB_TX_RING_SIZE		1
>>>   #define CONFIG_SYS_MACB_TX_TIMEOUT		1000
>>>   #define CONFIG_SYS_MACB_AUTONEG_TIMEOUT	5000000
>>>
>>> @@ -226,7 +226,13 @@ static int macb_send(struct eth_device *netdev, void *packet, int length)
>>>   	macb->tx_ring[tx_head].ctrl = ctrl;
>>>   	macb->tx_ring[tx_head].addr = paddr;
>>>   	barrier();
>>> -	macb_writel(macb, NCR, MACB_BIT(TE) | MACB_BIT(RE) | MACB_BIT(TSTART));
>>> +	/*
>>> +	 * Due to issues on SPEAr320 RMII, disable TE first so that
>>> +	 * controller can come out if it is hanged during transmission
>>> +	 */
>>> +	macb_writel(macb, NCR, macb_readl(macb, NCR)&  ~MACB_BIT(TE));
>>> +	macb_writel(macb, NCR, macb_readl(macb, NCR) |
>>> +			MACB_BIT(TE) | MACB_BIT(TSTART));
>>>
>>>   	/*
>>>   	 * I guess this is necessary because the networking core may
>>> @@ -444,6 +450,31 @@ static int macb_phy_init(struct macb_device *macb)
>>>   	}
>>>   }
>>>
>>> +static void macb_reset_hw(struct macb_device *bp)
>>> +{
>>> +	/* Make sure we have the write buffer for ourselves */
>>> +	barrier();
>>> +	/*
>>> +	 * Disable RX and TX (XXX: Should we halt the transmission
>>> +	 * more gracefully?) and we should not close the mdio port
>>> +	 */
>>> +	macb_writel(bp, NCR, 0);
>>> +
>>> +	/* Clear the stats registers (XXX: Update stats first?) */
>>> +	macb_writel(bp, NCR, MACB_BIT(CLRSTAT));
>>> +
>>> +	/* keep the mdio port , otherwise other eth will not work */
>>> +	macb_writel(bp, NCR, MACB_BIT(MPE));
>>> +
>>> +	/* Clear all status flags */
>>> +	macb_writel(bp, TSR, ~0UL);
>>> +	macb_writel(bp, RSR, ~0UL);
>>> +
>>> +	/* Disable all interrupts */
>>> +	macb_writel(bp, IDR, ~0UL);
>>> +	macb_readl(bp, ISR);
>>> +}
>>> +
>>>   static int macb_init(struct eth_device *netdev, bd_t *bd)
>>>   {
>>>   	struct macb_device *macb = to_macb(netdev);
>>> @@ -520,8 +551,7 @@ static void macb_halt(struct eth_device *netdev)
>>>   		tsr = macb_readl(macb, TSR);
>>>   	} while (tsr&  MACB_BIT(TGO));
>>>
>>> -	/* Disable TX and RX, and clear statistics */
>>> -	macb_writel(macb, NCR, MACB_BIT(CLRSTAT));
>>> +	macb_reset_hw(macb);
>>>   }
>>>
>>>   static int macb_write_hwaddr(struct eth_device *dev)
>>
>> This patch did not reappear in later versions of the series, and no
>> other standalone patch seems to match it. Was it dropped?
>
> Ping?
>

No, I have been busy with something. I would come back to the u-boot 
development soon

Vipin

> Amicalement,
Bo Shen March 1, 2013, 3:48 a.m. UTC | #6
Hi Vipin,

On 3/1/2013 11:40, Vipin Kumar wrote:
[Snip]
>>>>> Signed-off-by: Shiraz Hashim<shiraz.hashim@st.com>
>>>>> ---
>>>>> drivers/net/macb.c | 38 ++++++++++++++++++++++++++++++++++----
>>>>> 1 file changed, 34 insertions(+), 4 deletions(-)
>>
>> Tested on Atmel EK board. It works.
>>
>> Tested-by: Bo Shen <voice.shen@atmel.com>
>>
>> BTW, would this be implemented as a workaround only for SPEAr320S?
>>
>
> Yes, The idea was to implement this only for spear320s
> After your tested-by, I am thinking may be I can make a separate
> workaround config which can be enabled by any board using this peripheral.
>
> Is that OK?

Agree.

Best Regards,
Bo Shen
Albert ARIBAUD March 1, 2013, 7:28 a.m. UTC | #7
Hi Vipin,

On Fri, 1 Mar 2013 09:11:33 +0530, Vipin Kumar <vipin.kumar@st.com>
wrote:

> On 2/28/2013 6:29 PM, Albert ARIBAUD wrote:
> > On Sun, 3 Feb 2013 12:19:26 +0100, Albert ARIBAUD
> > <albert.u.boot@aribaud.net>  wrote:
> >
> >> Hi Vipin,
> >>
> >> On Fri, 2 Nov 2012 23:09:59 +0530, Vipin Kumar<vipin.kumar@st.com>
> >> wrote:
> >>
> >>> From: Shiraz Hashim<shiraz.hashim@st.com>
> >>>
> >>> It is observed on SPEAr320S RMII#1 interface that on transmitting
> >>> packets the MAC dma hangs randomly and constantly showing busy tx-go
> >>> state.
> >>>
> >>> It comes out if this situation only when Transmission is disabled and
> >>> enabled again.
> >>>
> >>> Since it happens randomly and u-boot doesn't require high performance we
> >>> disable TE and re-enable it on each transmission. We also change number
> >>> of transmit descriptor to 1 as we would not require more than it, further
> >>> it would not alter GMAC notion of transmit descriptor start queue as it
> >>> always point to same descriptor.
> >>>
> >>> Signed-off-by: Shiraz Hashim<shiraz.hashim@st.com>
> >>> ---
> >>>   drivers/net/macb.c | 38 ++++++++++++++++++++++++++++++++++----
> >>>   1 file changed, 34 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> >>> index ac25b52..17bad33 100644
> >>> --- a/drivers/net/macb.c
> >>> +++ b/drivers/net/macb.c
> >>> @@ -55,7 +55,7 @@
> >>>
> >>>   #define CONFIG_SYS_MACB_RX_BUFFER_SIZE		4096
> >>>   #define CONFIG_SYS_MACB_RX_RING_SIZE		(CONFIG_SYS_MACB_RX_BUFFER_SIZE / 128)
> >>> -#define CONFIG_SYS_MACB_TX_RING_SIZE		16
> >>> +#define CONFIG_SYS_MACB_TX_RING_SIZE		1
> >>>   #define CONFIG_SYS_MACB_TX_TIMEOUT		1000
> >>>   #define CONFIG_SYS_MACB_AUTONEG_TIMEOUT	5000000
> >>>
> >>> @@ -226,7 +226,13 @@ static int macb_send(struct eth_device *netdev, void *packet, int length)
> >>>   	macb->tx_ring[tx_head].ctrl = ctrl;
> >>>   	macb->tx_ring[tx_head].addr = paddr;
> >>>   	barrier();
> >>> -	macb_writel(macb, NCR, MACB_BIT(TE) | MACB_BIT(RE) | MACB_BIT(TSTART));
> >>> +	/*
> >>> +	 * Due to issues on SPEAr320 RMII, disable TE first so that
> >>> +	 * controller can come out if it is hanged during transmission
> >>> +	 */
> >>> +	macb_writel(macb, NCR, macb_readl(macb, NCR)&  ~MACB_BIT(TE));
> >>> +	macb_writel(macb, NCR, macb_readl(macb, NCR) |
> >>> +			MACB_BIT(TE) | MACB_BIT(TSTART));
> >>>
> >>>   	/*
> >>>   	 * I guess this is necessary because the networking core may
> >>> @@ -444,6 +450,31 @@ static int macb_phy_init(struct macb_device *macb)
> >>>   	}
> >>>   }
> >>>
> >>> +static void macb_reset_hw(struct macb_device *bp)
> >>> +{
> >>> +	/* Make sure we have the write buffer for ourselves */
> >>> +	barrier();
> >>> +	/*
> >>> +	 * Disable RX and TX (XXX: Should we halt the transmission
> >>> +	 * more gracefully?) and we should not close the mdio port
> >>> +	 */
> >>> +	macb_writel(bp, NCR, 0);
> >>> +
> >>> +	/* Clear the stats registers (XXX: Update stats first?) */
> >>> +	macb_writel(bp, NCR, MACB_BIT(CLRSTAT));
> >>> +
> >>> +	/* keep the mdio port , otherwise other eth will not work */
> >>> +	macb_writel(bp, NCR, MACB_BIT(MPE));
> >>> +
> >>> +	/* Clear all status flags */
> >>> +	macb_writel(bp, TSR, ~0UL);
> >>> +	macb_writel(bp, RSR, ~0UL);
> >>> +
> >>> +	/* Disable all interrupts */
> >>> +	macb_writel(bp, IDR, ~0UL);
> >>> +	macb_readl(bp, ISR);
> >>> +}
> >>> +
> >>>   static int macb_init(struct eth_device *netdev, bd_t *bd)
> >>>   {
> >>>   	struct macb_device *macb = to_macb(netdev);
> >>> @@ -520,8 +551,7 @@ static void macb_halt(struct eth_device *netdev)
> >>>   		tsr = macb_readl(macb, TSR);
> >>>   	} while (tsr&  MACB_BIT(TGO));
> >>>
> >>> -	/* Disable TX and RX, and clear statistics */
> >>> -	macb_writel(macb, NCR, MACB_BIT(CLRSTAT));
> >>> +	macb_reset_hw(macb);
> >>>   }
> >>>
> >>>   static int macb_write_hwaddr(struct eth_device *dev)
> >>
> >> This patch did not reappear in later versions of the series, and no
> >> other standalone patch seems to match it. Was it dropped?
> >
> > Ping?
> >
> 
> No, I have been busy with something. I would come back to the u-boot 
> development soon

Thanks. Following your discussion with Bo Shen, should I expect a new
version of this patch in the future, or does it stand as it is?

> Vipin

Amicalement,
Vipin Kumar March 1, 2013, 7:55 a.m. UTC | #8
On 3/1/2013 12:58 PM, Albert ARIBAUD wrote:
> Hi Vipin,
>
> On Fri, 1 Mar 2013 09:11:33 +0530, Vipin Kumar<vipin.kumar@st.com>
> wrote:
>
>> On 2/28/2013 6:29 PM, Albert ARIBAUD wrote:
>>> On Sun, 3 Feb 2013 12:19:26 +0100, Albert ARIBAUD
>>> <albert.u.boot@aribaud.net>   wrote:
>>>
>>>> Hi Vipin,
>>>>
>>>> On Fri, 2 Nov 2012 23:09:59 +0530, Vipin Kumar<vipin.kumar@st.com>
>>>> wrote:
>>>>
>>>>> From: Shiraz Hashim<shiraz.hashim@st.com>
>>>>>
>>>>> It is observed on SPEAr320S RMII#1 interface that on transmitting
>>>>> packets the MAC dma hangs randomly and constantly showing busy tx-go
>>>>> state.
>>>>>
>>>>> It comes out if this situation only when Transmission is disabled and
>>>>> enabled again.
>>>>>
>>>>> Since it happens randomly and u-boot doesn't require high performance we
>>>>> disable TE and re-enable it on each transmission. We also change number
>>>>> of transmit descriptor to 1 as we would not require more than it, further
>>>>> it would not alter GMAC notion of transmit descriptor start queue as it
>>>>> always point to same descriptor.
>>>>>
>>>>> Signed-off-by: Shiraz Hashim<shiraz.hashim@st.com>
>>>>> ---
>>>>>    drivers/net/macb.c | 38 ++++++++++++++++++++++++++++++++++----
>>>>>    1 file changed, 34 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
>>>>> index ac25b52..17bad33 100644
>>>>> --- a/drivers/net/macb.c
>>>>> +++ b/drivers/net/macb.c
>>>>> @@ -55,7 +55,7 @@
>>>>>
>>>>>    #define CONFIG_SYS_MACB_RX_BUFFER_SIZE		4096
>>>>>    #define CONFIG_SYS_MACB_RX_RING_SIZE		(CONFIG_SYS_MACB_RX_BUFFER_SIZE / 128)
>>>>> -#define CONFIG_SYS_MACB_TX_RING_SIZE		16
>>>>> +#define CONFIG_SYS_MACB_TX_RING_SIZE		1
>>>>>    #define CONFIG_SYS_MACB_TX_TIMEOUT		1000
>>>>>    #define CONFIG_SYS_MACB_AUTONEG_TIMEOUT	5000000
>>>>>
>>>>> @@ -226,7 +226,13 @@ static int macb_send(struct eth_device *netdev, void *packet, int length)
>>>>>    	macb->tx_ring[tx_head].ctrl = ctrl;
>>>>>    	macb->tx_ring[tx_head].addr = paddr;
>>>>>    	barrier();
>>>>> -	macb_writel(macb, NCR, MACB_BIT(TE) | MACB_BIT(RE) | MACB_BIT(TSTART));
>>>>> +	/*
>>>>> +	 * Due to issues on SPEAr320 RMII, disable TE first so that
>>>>> +	 * controller can come out if it is hanged during transmission
>>>>> +	 */
>>>>> +	macb_writel(macb, NCR, macb_readl(macb, NCR)&   ~MACB_BIT(TE));
>>>>> +	macb_writel(macb, NCR, macb_readl(macb, NCR) |
>>>>> +			MACB_BIT(TE) | MACB_BIT(TSTART));
>>>>>
>>>>>    	/*
>>>>>    	 * I guess this is necessary because the networking core may
>>>>> @@ -444,6 +450,31 @@ static int macb_phy_init(struct macb_device *macb)
>>>>>    	}
>>>>>    }
>>>>>
>>>>> +static void macb_reset_hw(struct macb_device *bp)
>>>>> +{
>>>>> +	/* Make sure we have the write buffer for ourselves */
>>>>> +	barrier();
>>>>> +	/*
>>>>> +	 * Disable RX and TX (XXX: Should we halt the transmission
>>>>> +	 * more gracefully?) and we should not close the mdio port
>>>>> +	 */
>>>>> +	macb_writel(bp, NCR, 0);
>>>>> +
>>>>> +	/* Clear the stats registers (XXX: Update stats first?) */
>>>>> +	macb_writel(bp, NCR, MACB_BIT(CLRSTAT));
>>>>> +
>>>>> +	/* keep the mdio port , otherwise other eth will not work */
>>>>> +	macb_writel(bp, NCR, MACB_BIT(MPE));
>>>>> +
>>>>> +	/* Clear all status flags */
>>>>> +	macb_writel(bp, TSR, ~0UL);
>>>>> +	macb_writel(bp, RSR, ~0UL);
>>>>> +
>>>>> +	/* Disable all interrupts */
>>>>> +	macb_writel(bp, IDR, ~0UL);
>>>>> +	macb_readl(bp, ISR);
>>>>> +}
>>>>> +
>>>>>    static int macb_init(struct eth_device *netdev, bd_t *bd)
>>>>>    {
>>>>>    	struct macb_device *macb = to_macb(netdev);
>>>>> @@ -520,8 +551,7 @@ static void macb_halt(struct eth_device *netdev)
>>>>>    		tsr = macb_readl(macb, TSR);
>>>>>    	} while (tsr&   MACB_BIT(TGO));
>>>>>
>>>>> -	/* Disable TX and RX, and clear statistics */
>>>>> -	macb_writel(macb, NCR, MACB_BIT(CLRSTAT));
>>>>> +	macb_reset_hw(macb);
>>>>>    }
>>>>>
>>>>>    static int macb_write_hwaddr(struct eth_device *dev)
>>>>
>>>> This patch did not reappear in later versions of the series, and no
>>>> other standalone patch seems to match it. Was it dropped?
>>>
>>> Ping?
>>>
>>
>> No, I have been busy with something. I would come back to the u-boot
>> development soon
>
> Thanks. Following your discussion with Bo Shen, should I expect a new
> version of this patch in the future, or does it stand as it is?
>

A new version

Vipin

>> Vipin
>
> Amicalement,
diff mbox

Patch

diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index ac25b52..17bad33 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -55,7 +55,7 @@ 
 
 #define CONFIG_SYS_MACB_RX_BUFFER_SIZE		4096
 #define CONFIG_SYS_MACB_RX_RING_SIZE		(CONFIG_SYS_MACB_RX_BUFFER_SIZE / 128)
-#define CONFIG_SYS_MACB_TX_RING_SIZE		16
+#define CONFIG_SYS_MACB_TX_RING_SIZE		1
 #define CONFIG_SYS_MACB_TX_TIMEOUT		1000
 #define CONFIG_SYS_MACB_AUTONEG_TIMEOUT	5000000
 
@@ -226,7 +226,13 @@  static int macb_send(struct eth_device *netdev, void *packet, int length)
 	macb->tx_ring[tx_head].ctrl = ctrl;
 	macb->tx_ring[tx_head].addr = paddr;
 	barrier();
-	macb_writel(macb, NCR, MACB_BIT(TE) | MACB_BIT(RE) | MACB_BIT(TSTART));
+	/*
+	 * Due to issues on SPEAr320 RMII, disable TE first so that
+	 * controller can come out if it is hanged during transmission
+	 */
+	macb_writel(macb, NCR, macb_readl(macb, NCR) & ~MACB_BIT(TE));
+	macb_writel(macb, NCR, macb_readl(macb, NCR) |
+			MACB_BIT(TE) | MACB_BIT(TSTART));
 
 	/*
 	 * I guess this is necessary because the networking core may
@@ -444,6 +450,31 @@  static int macb_phy_init(struct macb_device *macb)
 	}
 }
 
+static void macb_reset_hw(struct macb_device *bp)
+{
+	/* Make sure we have the write buffer for ourselves */
+	barrier();
+	/*
+	 * Disable RX and TX (XXX: Should we halt the transmission
+	 * more gracefully?) and we should not close the mdio port
+	 */
+	macb_writel(bp, NCR, 0);
+
+	/* Clear the stats registers (XXX: Update stats first?) */
+	macb_writel(bp, NCR, MACB_BIT(CLRSTAT));
+
+	/* keep the mdio port , otherwise other eth will not work */
+	macb_writel(bp, NCR, MACB_BIT(MPE));
+
+	/* Clear all status flags */
+	macb_writel(bp, TSR, ~0UL);
+	macb_writel(bp, RSR, ~0UL);
+
+	/* Disable all interrupts */
+	macb_writel(bp, IDR, ~0UL);
+	macb_readl(bp, ISR);
+}
+
 static int macb_init(struct eth_device *netdev, bd_t *bd)
 {
 	struct macb_device *macb = to_macb(netdev);
@@ -520,8 +551,7 @@  static void macb_halt(struct eth_device *netdev)
 		tsr = macb_readl(macb, TSR);
 	} while (tsr & MACB_BIT(TGO));
 
-	/* Disable TX and RX, and clear statistics */
-	macb_writel(macb, NCR, MACB_BIT(CLRSTAT));
+	macb_reset_hw(macb);
 }
 
 static int macb_write_hwaddr(struct eth_device *dev)