Re: [PATCH] net: macb: do not disable MDIO bus when closing interface

[Anssi Hannula <anssi.hannula@bitwise.fi>]

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