All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] net: macb: do not disable MDIO bus when closing interface
@ 2018-08-08 12:19 Anssi Hannula
  2018-08-09  8:26 ` Claudiu Beznea
  0 siblings, 1 reply; 11+ messages in thread
From: Anssi Hannula @ 2018-08-08 12:19 UTC (permalink / raw)
  To: Nicolas Ferre, David S. Miller; +Cc: netdev

macb_close() calls macb_reset_hw() which zeroes NCR register, including
the MPE (Management Port Enable) bit.

This will prevent accessing any other PHYs for other Ethernet MACs on
the MDIO bus which is still registered.

Fix that by keeping the MPE bit set.

Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---
 drivers/net/ethernet/cadence/macb_main.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index dc09f9a8a49b..3ca98fc32144 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -2030,12 +2030,13 @@ static void macb_reset_hw(struct macb *bp)
 	unsigned int q;
 
 	/* Disable RX and TX (XXX: Should we halt the transmission
-	 * more gracefully?)
+	 * more gracefully?) but keep management port open since there
+	 * may be other users of the mdio bus
 	 */
-	macb_writel(bp, NCR, 0);
+	macb_writel(bp, NCR, MACB_BIT(MPE));
 
 	/* Clear the stats registers (XXX: Update stats first?) */
-	macb_writel(bp, NCR, MACB_BIT(CLRSTAT));
+	macb_writel(bp, NCR, MACB_BIT(CLRSTAT) | MACB_BIT(MPE));
 
 	/* Clear all status flags */
 	macb_writel(bp, TSR, -1);
-- 
2.16.3

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: macb: do not disable MDIO bus when closing interface
  2018-08-08 12:19 [PATCH] net: macb: do not disable MDIO bus when closing interface Anssi Hannula
@ 2018-08-09  8:26 ` Claudiu Beznea
  2018-08-09 14:54   ` Anssi Hannula
  0 siblings, 1 reply; 11+ messages in thread
From: Claudiu Beznea @ 2018-08-09  8:26 UTC (permalink / raw)
  To: Anssi Hannula, Nicolas Ferre, David S. Miller; +Cc: netdev



On 08.08.2018 15:19, Anssi Hannula wrote:
> macb_close() calls macb_reset_hw() which zeroes NCR register, including
> the MPE (Management Port Enable) bit.
> 
> This will prevent accessing any other PHYs for other Ethernet MACs on
> the MDIO bus which is still registered.
> 
> Fix that by keeping the MPE bit set.
> 
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index dc09f9a8a49b..3ca98fc32144 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2030,12 +2030,13 @@ static void macb_reset_hw(struct macb *bp)
>  	unsigned int q;
>  
>  	/* Disable RX and TX (XXX: Should we halt the transmission
> -	 * more gracefully?)
> +	 * more gracefully?) but keep management port open since there
> +	 * may be other users of the mdio bus
>  	 */
> -	macb_writel(bp, NCR, 0);
> +	macb_writel(bp, NCR, MACB_BIT(MPE));

Would be better to read the NCR and clear only RX and TX bits, something like:
	val = macb_readl(bp, NCR);

	/* Disable TX and RX. */
	val &= ~(MACB_BIT(TE) | MACB_BIT(RE));

	/* Clear statistics */
	val |= MACB_BIT(CLRSTAT);

	macb_writel(bp, NCR, val);

MPE should have been enabled by previous operations.

Thank you,
Claudiu Beznea

>  
>  	/* Clear the stats registers (XXX: Update stats first?) */
> -	macb_writel(bp, NCR, MACB_BIT(CLRSTAT));
> +	macb_writel(bp, NCR, MACB_BIT(CLRSTAT) | MACB_BIT(MPE));
>  
>  	/* Clear all status flags */
>  	macb_writel(bp, TSR, -1);
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: macb: do not disable MDIO bus when closing interface
  2018-08-09  8:26 ` Claudiu Beznea
@ 2018-08-09 14:54   ` Anssi Hannula
  2018-08-09 15:14     ` Andrew Lunn
  0 siblings, 1 reply; 11+ messages in thread
From: Anssi Hannula @ 2018-08-09 14:54 UTC (permalink / raw)
  To: Claudiu Beznea; +Cc: Nicolas Ferre, David S. Miller, netdev

On 9.8.2018 11:26, Claudiu Beznea wrote:
> On 08.08.2018 15:19, Anssi Hannula wrote:
>> macb_close() calls macb_reset_hw() which zeroes NCR register, including
>> the MPE (Management Port Enable) bit.
>>
>> This will prevent accessing any other PHYs for other Ethernet MACs on
>> the MDIO bus which is still registered.
>>
>> Fix that by keeping the MPE bit set.
>>
>> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
>> ---
>>  drivers/net/ethernet/cadence/macb_main.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index dc09f9a8a49b..3ca98fc32144 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -2030,12 +2030,13 @@ static void macb_reset_hw(struct macb *bp)
>>  	unsigned int q;
>>  
>>  	/* Disable RX and TX (XXX: Should we halt the transmission
>> -	 * more gracefully?)
>> +	 * more gracefully?) but keep management port open since there
>> +	 * may be other users of the mdio bus
>>  	 */
>> -	macb_writel(bp, NCR, 0);
>> +	macb_writel(bp, NCR, MACB_BIT(MPE));
> Would be better to read the NCR and clear only RX and TX bits, something like:
> 	val = macb_readl(bp, NCR);
>
> 	/* Disable TX and RX. */
> 	val &= ~(MACB_BIT(TE) | MACB_BIT(RE));
>
> 	/* Clear statistics */
> 	val |= MACB_BIT(CLRSTAT);
>
> 	macb_writel(bp, NCR, val);
>
> MPE should have been enabled by previous operations.

macb_reset_hw() is called in init path too, though, so maybe clearing
all bits is intentional / wanted to get the controller to a known state,
even though the comment only mentions TX/RX?

If so, maybe the below would be better? But if you think just clearing
TE/RE is better I'll just do it like you suggested.

    val = macb_readl(bp, NCR);

    /* Keep only MDIO port active */
    val &= MACB_BIT(MPE);

    /* Clear statistics */
    val |= MACB_BIT(CLRSTAT);

    macb_writel(bp, NCR, val);



> Thank you,
> Claudiu Beznea
>
>>  
>>  	/* Clear the stats registers (XXX: Update stats first?) */
>> -	macb_writel(bp, NCR, MACB_BIT(CLRSTAT));
>> +	macb_writel(bp, NCR, MACB_BIT(CLRSTAT) | MACB_BIT(MPE));
>>  
>>  	/* Clear all status flags */
>>  	macb_writel(bp, TSR, -1);
>>

-- 
Anssi Hannula / Bitwise Oy
+358 503803997

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: macb: do not disable MDIO bus when closing interface
  2018-08-09 14:54   ` Anssi Hannula
@ 2018-08-09 15:14     ` Andrew Lunn
  2018-08-10  6:22       ` Anssi Hannula
  0 siblings, 1 reply; 11+ messages in thread
From: Andrew Lunn @ 2018-08-09 15:14 UTC (permalink / raw)
  To: Anssi Hannula; +Cc: Claudiu Beznea, Nicolas Ferre, David S. Miller, netdev

Hi Anssi

> macb_reset_hw() is called in init path too, though, so maybe clearing
> all bits is intentional / wanted to get the controller to a known state,
> even though the comment only mentions TX/RX?

You need to be careful here. Once of_mdiobus_register() is called, the
MDIO should be usable. If you happen to have an Ethernet switch on the
bus, it could be probed then. The DSA driver will start using the bus.
Or if you have a second PHY, connected to some other MAC, it could be
used by the other MAC.  This all happens in the macb_probe function.

Sometime later, the interface will be up'ed. At this point macb_open()
is called, which calls macb_init_hw(), which calls
macb_reset_hw(). What you don't want happening is changes to the NCR
at this point breaking an MDIO transaction which might be going on.

Ideally, the MPE should be enabled before of_mdiobus_register(), and
left alone until mdiobus_unregister() is called in macb_remove().

     Andrew

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: macb: do not disable MDIO bus when closing interface
  2018-08-09 15:14     ` Andrew Lunn
@ 2018-08-10  6:22       ` Anssi Hannula
  2018-08-13  9:08         ` Claudiu Beznea
  0 siblings, 1 reply; 11+ messages in thread
From: Anssi Hannula @ 2018-08-10  6:22 UTC (permalink / raw)
  To: Andrew Lunn; +Cc: Claudiu Beznea, Nicolas Ferre, David S. Miller, netdev

On 9.8.2018 18:14, Andrew Lunn wrote:
> Hi Anssi

Hi!

>> macb_reset_hw() is called in init path too, though, so maybe clearing
>> all bits is intentional / wanted to get the controller to a known state,
>> even though the comment only mentions TX/RX?
> You need to be careful here. Once of_mdiobus_register() is called, the
> MDIO should be usable. If you happen to have an Ethernet switch on the
> bus, it could be probed then. The DSA driver will start using the bus.
> Or if you have a second PHY, connected to some other MAC, it could be
> used by the other MAC.  This all happens in the macb_probe function.
>
> Sometime later, the interface will be up'ed. At this point macb_open()
> is called, which calls macb_init_hw(), which calls
> macb_reset_hw(). What you don't want happening is changes to the NCR
> at this point breaking an MDIO transaction which might be going on.
>
> Ideally, the MPE should be enabled before of_mdiobus_register(), and
> left alone until mdiobus_unregister() is called in macb_remove().

Yep, fixing the use case of having PHYs of other MACs is why I wrote the
patch :)

Currently the reset code disables MPE while other MACs are using PHYs on
the bus.

-- 
Anssi Hannula / Bitwise Oy
+358 503803997

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: macb: do not disable MDIO bus when closing interface
  2018-08-10  6:22       ` Anssi Hannula
@ 2018-08-13  9:08         ` Claudiu Beznea
  2018-08-20 14:55           ` [PATCH] net: macb: do not disable MDIO bus at open/close time Anssi Hannula
  0 siblings, 1 reply; 11+ messages in thread
From: Claudiu Beznea @ 2018-08-13  9:08 UTC (permalink / raw)
  To: Anssi Hannula, Andrew Lunn; +Cc: Nicolas Ferre, David S. Miller, netdev



On 10.08.2018 09:22, Anssi Hannula wrote:
> On 9.8.2018 18:14, Andrew Lunn wrote:
>> Hi Anssi
> 
> Hi!
> 
>>> macb_reset_hw() is called in init path too,

I only see it in macb_close() and macb_open() called from macb_init_hw().

though, so maybe clearing
>>> all bits is intentional / wanted to get the controller to a known state,

As far as I know the NCR is used in this driver only to control transmit
enable, receive enable, MPE and tx start.

Not clearing NCR in the init phase should not have any impact on IP. Same
for not clearing Tx start on "if down" path (if tx is in progress).

>>> even though the comment only mentions TX/RX?
>> You need to be careful here. Once of_mdiobus_register() is called, the
>> MDIO should be usable. If you happen to have an Ethernet switch on the
>> bus, it could be probed then. The DSA driver will start using the bus.
>> Or if you have a second PHY, connected to some other MAC, it could be
>> used by the other MAC.  This all happens in the macb_probe function.
>>
>> Sometime later, the interface will be up'ed. At this point macb_open()
>> is called, which calls macb_init_hw(), which calls
>> macb_reset_hw(). What you don't want happening is changes to the NCR
>> at this point breaking an MDIO transaction which might be going on.
>>
>> Ideally, the MPE should be enabled before of_mdiobus_register(), and
>> left alone until mdiobus_unregister() is called in macb_remove().
> 
> Yep, fixing the use case of having PHYs of other MACs is why I wrote the
> patch :)
> 
> Currently the reset code disables MPE while other MACs are using PHYs on
> the bus.

MPE is set in the early phase of initialization, in macb_mii_init(), called
from probe() function. Due to the fact that NCR is cleared in
macb_reset_hw() it is set again at the end of macb_init_hw(). So, as per
Andrew comments in this thread, setting it in macb_mii_init() and letting
it unchanged until remove would be the best. So, if you clear only TE and
RE bits in macb_reset_hw() please also remove MACB_BIT(MPE) from below line
at the end of macb_init_hw():

        macb_writel(bp, NCR, MACB_BIT(RE) | MACB_BIT(TE) | MACB_BIT(MPE));


Thank you,
Claudiu Beznea

> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] net: macb: do not disable MDIO bus at open/close time
  2018-08-13  9:08         ` Claudiu Beznea
@ 2018-08-20 14:55           ` Anssi Hannula
  2018-08-22 15:33             ` Claudiu Beznea
  0 siblings, 1 reply; 11+ messages in thread
From: Anssi Hannula @ 2018-08-20 14:55 UTC (permalink / raw)
  To: netdev, Nicolas Ferre, Claudiu Beznea; +Cc: David S. Miller, Andrew Lunn

macb_reset_hw() is called from macb_close() and indirectly from
macb_open(). macb_reset_hw() zeroes the NCR register, including the MPE
(Management Port Enable) bit.

This will prevent accessing any other PHYs for other Ethernet MACs on
the MDIO bus, which remains registered at macb_reset_hw() time, until
macb_init_hw() is called from macb_open() which sets the MPE bit again.

I.e. currently the MDIO bus has a short disruption at open time and is
disabled at close time until the interface is opened again.

Fix that by only touching the RE and TE bits when enabling and disabling
RX/TX.

Fixes: 6c36a7074436 ("macb: Use generic PHY layer")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---

Claudiu Beznea wrote:
> On 10.08.2018 09:22, Anssi Hannula wrote:
>>
>> macb_reset_hw() is called in init path too,
>
> I only see it in macb_close() and macb_open() called from macb_init_hw().

Yeah, macb_init_hw() is what I meant :)


 drivers/net/ethernet/cadence/macb_main.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index dc09f9a8a49b..6501e9b3785a 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -2028,14 +2028,17 @@ static void macb_reset_hw(struct macb *bp)
 {
 	struct macb_queue *queue;
 	unsigned int q;
+	u32 ctrl = macb_readl(bp, NCR);
 
 	/* Disable RX and TX (XXX: Should we halt the transmission
 	 * more gracefully?)
 	 */
-	macb_writel(bp, NCR, 0);
+	ctrl &= ~(MACB_BIT(RE) | MACB_BIT(TE));
 
 	/* Clear the stats registers (XXX: Update stats first?) */
-	macb_writel(bp, NCR, MACB_BIT(CLRSTAT));
+	ctrl |= MACB_BIT(CLRSTAT);
+
+	macb_writel(bp, NCR, ctrl);
 
 	/* Clear all status flags */
 	macb_writel(bp, TSR, -1);
@@ -2170,6 +2173,7 @@ static void macb_init_hw(struct macb *bp)
 	unsigned int q;
 
 	u32 config;
+	u32 ctrl;
 
 	macb_reset_hw(bp);
 	macb_set_hwaddr(bp);
@@ -2223,7 +2227,9 @@ static void macb_init_hw(struct macb *bp)
 	}
 
 	/* Enable TX and RX */
-	macb_writel(bp, NCR, MACB_BIT(RE) | MACB_BIT(TE) | MACB_BIT(MPE));
+	ctrl = macb_readl(bp, NCR);
+	ctrl |= MACB_BIT(RE) | MACB_BIT(TE);
+	macb_writel(bp, NCR, ctrl);
 }
 
 /* The hash address register is 64 bits long and takes up two
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: macb: do not disable MDIO bus at open/close time
  2018-08-20 14:55           ` [PATCH] net: macb: do not disable MDIO bus at open/close time Anssi Hannula
@ 2018-08-22 15:33             ` Claudiu Beznea
  2018-08-23  7:45               ` [PATCH v2] " Anssi Hannula
  0 siblings, 1 reply; 11+ messages in thread
From: Claudiu Beznea @ 2018-08-22 15:33 UTC (permalink / raw)
  To: Anssi Hannula, netdev, Nicolas Ferre; +Cc: David S. Miller, Andrew Lunn



On 20.08.2018 17:55, Anssi Hannula wrote:
> macb_reset_hw() is called from macb_close() and indirectly from
> macb_open(). macb_reset_hw() zeroes the NCR register, including the MPE
> (Management Port Enable) bit.
> 
> This will prevent accessing any other PHYs for other Ethernet MACs on
> the MDIO bus, which remains registered at macb_reset_hw() time, until
> macb_init_hw() is called from macb_open() which sets the MPE bit again.
> 
> I.e. currently the MDIO bus has a short disruption at open time and is
> disabled at close time until the interface is opened again.
> 
> Fix that by only touching the RE and TE bits when enabling and disabling
> RX/TX.
> 
> Fixes: 6c36a7074436 ("macb: Use generic PHY layer")
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
> ---
> 
> Claudiu Beznea wrote:
>> On 10.08.2018 09:22, Anssi Hannula wrote:
>>>
>>> macb_reset_hw() is called in init path too,
>>
>> I only see it in macb_close() and macb_open() called from macb_init_hw().
> 
> Yeah, macb_init_hw() is what I meant :)
> 
> 
>  drivers/net/ethernet/cadence/macb_main.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index dc09f9a8a49b..6501e9b3785a 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2028,14 +2028,17 @@ static void macb_reset_hw(struct macb *bp)
>  {
>  	struct macb_queue *queue;
>  	unsigned int q;
> +	u32 ctrl = macb_readl(bp, NCR);
>  
>  	/* Disable RX and TX (XXX: Should we halt the transmission
>  	 * more gracefully?)
>  	 */
> -	macb_writel(bp, NCR, 0);
> +	ctrl &= ~(MACB_BIT(RE) | MACB_BIT(TE));
>  
>  	/* Clear the stats registers (XXX: Update stats first?) */
> -	macb_writel(bp, NCR, MACB_BIT(CLRSTAT));
> +	ctrl |= MACB_BIT(CLRSTAT);
> +
> +	macb_writel(bp, NCR, ctrl);
>  
>  	/* Clear all status flags */
>  	macb_writel(bp, TSR, -1);
> @@ -2170,6 +2173,7 @@ static void macb_init_hw(struct macb *bp)
>  	unsigned int q;
>  
>  	u32 config;
> +	u32 ctrl;
>  
>  	macb_reset_hw(bp);
>  	macb_set_hwaddr(bp);
> @@ -2223,7 +2227,9 @@ static void macb_init_hw(struct macb *bp)
>  	}
>  
>  	/* Enable TX and RX */
> -	macb_writel(bp, NCR, MACB_BIT(RE) | MACB_BIT(TE) | MACB_BIT(MPE));
> +	ctrl = macb_readl(bp, NCR);
> +	ctrl |= MACB_BIT(RE) | MACB_BIT(TE);
> +	macb_writel(bp, NCR, ctrl);

I would keep it as:

	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(RE) | MACB_BIT(TE));

>  }
>  
>  /* The hash address register is 64 bits long and takes up two
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] net: macb: do not disable MDIO bus at open/close time
  2018-08-22 15:33             ` Claudiu Beznea
@ 2018-08-23  7:45               ` Anssi Hannula
  2018-08-24 14:47                 ` Claudiu Beznea
  2018-08-26  0:35                 ` David Miller
  0 siblings, 2 replies; 11+ messages in thread
From: Anssi Hannula @ 2018-08-23  7:45 UTC (permalink / raw)
  To: netdev, Nicolas Ferre, Claudiu Beznea; +Cc: David S. Miller, Andrew Lunn

macb_reset_hw() is called from macb_close() and indirectly from
macb_open(). macb_reset_hw() zeroes the NCR register, including the MPE
(Management Port Enable) bit.

This will prevent accessing any other PHYs for other Ethernet MACs on
the MDIO bus, which remains registered at macb_reset_hw() time, until
macb_init_hw() is called from macb_open() which sets the MPE bit again.

I.e. currently the MDIO bus has a short disruption at open time and is
disabled at close time until the interface is opened again.

Fix that by only touching the RE and TE bits when enabling and disabling
RX/TX.

v2: Make macb_init_hw() NCR write a single statement.

Fixes: 6c36a7074436 ("macb: Use generic PHY layer")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---
 drivers/net/ethernet/cadence/macb_main.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index dc09f9a8a49b..225a7c8bad2d 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -2028,14 +2028,17 @@ static void macb_reset_hw(struct macb *bp)
 {
 	struct macb_queue *queue;
 	unsigned int q;
+	u32 ctrl = macb_readl(bp, NCR);
 
 	/* Disable RX and TX (XXX: Should we halt the transmission
 	 * more gracefully?)
 	 */
-	macb_writel(bp, NCR, 0);
+	ctrl &= ~(MACB_BIT(RE) | MACB_BIT(TE));
 
 	/* Clear the stats registers (XXX: Update stats first?) */
-	macb_writel(bp, NCR, MACB_BIT(CLRSTAT));
+	ctrl |= MACB_BIT(CLRSTAT);
+
+	macb_writel(bp, NCR, ctrl);
 
 	/* Clear all status flags */
 	macb_writel(bp, TSR, -1);
@@ -2223,7 +2226,7 @@ static void macb_init_hw(struct macb *bp)
 	}
 
 	/* Enable TX and RX */
-	macb_writel(bp, NCR, MACB_BIT(RE) | MACB_BIT(TE) | MACB_BIT(MPE));
+	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(RE) | MACB_BIT(TE));
 }
 
 /* The hash address register is 64 bits long and takes up two
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] net: macb: do not disable MDIO bus at open/close time
  2018-08-23  7:45               ` [PATCH v2] " Anssi Hannula
@ 2018-08-24 14:47                 ` Claudiu Beznea
  2018-08-26  0:35                 ` David Miller
  1 sibling, 0 replies; 11+ messages in thread
From: Claudiu Beznea @ 2018-08-24 14:47 UTC (permalink / raw)
  To: Anssi Hannula, netdev, Nicolas Ferre; +Cc: David S. Miller, Andrew Lunn



On 23.08.2018 10:45, Anssi Hannula wrote:
> macb_reset_hw() is called from macb_close() and indirectly from
> macb_open(). macb_reset_hw() zeroes the NCR register, including the MPE
> (Management Port Enable) bit.
> 
> This will prevent accessing any other PHYs for other Ethernet MACs on
> the MDIO bus, which remains registered at macb_reset_hw() time, until
> macb_init_hw() is called from macb_open() which sets the MPE bit again.
> 
> I.e. currently the MDIO bus has a short disruption at open time and is
> disabled at close time until the interface is opened again.
> 
> Fix that by only touching the RE and TE bits when enabling and disabling
> RX/TX.
> 
> v2: Make macb_init_hw() NCR write a single statement.

This usually goes after the tree '-' below.

> 
> Fixes: 6c36a7074436 ("macb: Use generic PHY layer")
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
> ---

Here:

v2: Make macb_init_hw() NCR write a single statement.

Other than this: 

Reviewed-by: Claudiu Beznea <claudiu.beznea@microchip.com>

Checked that link goes up and ping is working on a SAMA5D2 Xplained:

Tested-by: Claudiu Beznea <claudiu.beznea@microchip.com>

>  drivers/net/ethernet/cadence/macb_main.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index dc09f9a8a49b..225a7c8bad2d 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2028,14 +2028,17 @@ static void macb_reset_hw(struct macb *bp)
>  {
>  	struct macb_queue *queue;
>  	unsigned int q;
> +	u32 ctrl = macb_readl(bp, NCR);
>  
>  	/* Disable RX and TX (XXX: Should we halt the transmission
>  	 * more gracefully?)
>  	 */
> -	macb_writel(bp, NCR, 0);
> +	ctrl &= ~(MACB_BIT(RE) | MACB_BIT(TE));
>  
>  	/* Clear the stats registers (XXX: Update stats first?) */
> -	macb_writel(bp, NCR, MACB_BIT(CLRSTAT));
> +	ctrl |= MACB_BIT(CLRSTAT);
> +
> +	macb_writel(bp, NCR, ctrl);
>  
>  	/* Clear all status flags */
>  	macb_writel(bp, TSR, -1);
> @@ -2223,7 +2226,7 @@ static void macb_init_hw(struct macb *bp)
>  	}
>  
>  	/* Enable TX and RX */
> -	macb_writel(bp, NCR, MACB_BIT(RE) | MACB_BIT(TE) | MACB_BIT(MPE));
> +	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(RE) | MACB_BIT(TE));
>  }
>  
>  /* The hash address register is 64 bits long and takes up two
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] net: macb: do not disable MDIO bus at open/close time
  2018-08-23  7:45               ` [PATCH v2] " Anssi Hannula
  2018-08-24 14:47                 ` Claudiu Beznea
@ 2018-08-26  0:35                 ` David Miller
  1 sibling, 0 replies; 11+ messages in thread
From: David Miller @ 2018-08-26  0:35 UTC (permalink / raw)
  To: anssi.hannula; +Cc: netdev, nicolas.ferre, Claudiu.Beznea, andrew

From: Anssi Hannula <anssi.hannula@bitwise.fi>
Date: Thu, 23 Aug 2018 10:45:22 +0300

> macb_reset_hw() is called from macb_close() and indirectly from
> macb_open(). macb_reset_hw() zeroes the NCR register, including the MPE
> (Management Port Enable) bit.
> 
> This will prevent accessing any other PHYs for other Ethernet MACs on
> the MDIO bus, which remains registered at macb_reset_hw() time, until
> macb_init_hw() is called from macb_open() which sets the MPE bit again.
> 
> I.e. currently the MDIO bus has a short disruption at open time and is
> disabled at close time until the interface is opened again.
> 
> Fix that by only touching the RE and TE bits when enabling and disabling
> RX/TX.
> 
> v2: Make macb_init_hw() NCR write a single statement.
> 
> Fixes: 6c36a7074436 ("macb: Use generic PHY layer")
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>

Applied and queued up for -stable.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-08-26  4:16 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-08 12:19 [PATCH] net: macb: do not disable MDIO bus when closing interface Anssi Hannula
2018-08-09  8:26 ` Claudiu Beznea
2018-08-09 14:54   ` Anssi Hannula
2018-08-09 15:14     ` Andrew Lunn
2018-08-10  6:22       ` Anssi Hannula
2018-08-13  9:08         ` Claudiu Beznea
2018-08-20 14:55           ` [PATCH] net: macb: do not disable MDIO bus at open/close time Anssi Hannula
2018-08-22 15:33             ` Claudiu Beznea
2018-08-23  7:45               ` [PATCH v2] " Anssi Hannula
2018-08-24 14:47                 ` Claudiu Beznea
2018-08-26  0:35                 ` David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.