[08/10] net: ax88796: Make reset more robust on AX88796B

Message ID 1523916285-6057-9-git-send-email-schmitzmic@gmail.com
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • New network driver for Amiga X-Surf 100 (m68k)
Related show

Commit Message

Michael Schmitz April 16, 2018, 10:04 p.m.
From: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

The AX88796B as installed on the X-Surf-100 does not recognize a MII reset
request if the previous write to the MII control register also was a reset
request. So a dummy write to the control register makes the soft reset in
the PHY initialization code work.

Signed-off-by: Michael Karcher <kernel@mkarcher.dialup.fu-berlin.de>
---
 drivers/net/ethernet/8390/ax88796.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

Comments

Andrew Lunn April 16, 2018, 11:12 p.m. | #1
On Tue, Apr 17, 2018 at 10:04:43AM +1200, Michael Schmitz wrote:
> From: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> 
> The AX88796B as installed on the X-Surf-100 does not recognize a MII reset
> request if the previous write to the MII control register also was a reset
> request. So a dummy write to the control register makes the soft reset in
> the PHY initialization code work.
> 
> Signed-off-by: Michael Karcher <kernel@mkarcher.dialup.fu-berlin.de>
> ---
>  drivers/net/ethernet/8390/ax88796.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/ethernet/8390/ax88796.c b/drivers/net/ethernet/8390/ax88796.c
> index 6af9aca..a2f9a09 100644
> --- a/drivers/net/ethernet/8390/ax88796.c
> +++ b/drivers/net/ethernet/8390/ax88796.c
> @@ -374,6 +374,10 @@ static int ax_mii_probe(struct net_device *dev)
>  		return -ENODEV;
>  	}
>  
> +	/* write a non-reset pattern to the control register to
> +	 * re-arm the reset request detection logic (needed on AX88796B)
> +	 */
> +	phy_write(phy_dev, MII_BMCR, 0);

This should really be fixed in the PHY driver, not the MAC.

     Andrew
Michael Schmitz April 17, 2018, 12:14 a.m. | #2
Hi Andrew,

On Tue, Apr 17, 2018 at 11:12 AM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Tue, Apr 17, 2018 at 10:04:43AM +1200, Michael Schmitz wrote:
>> From: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
>>
>> The AX88796B as installed on the X-Surf-100 does not recognize a MII reset
>> request if the previous write to the MII control register also was a reset
>> request. So a dummy write to the control register makes the soft reset in
>> the PHY initialization code work.
>>
>> Signed-off-by: Michael Karcher <kernel@mkarcher.dialup.fu-berlin.de>
>> ---
>>  drivers/net/ethernet/8390/ax88796.c |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/8390/ax88796.c b/drivers/net/ethernet/8390/ax88796.c
>> index 6af9aca..a2f9a09 100644
>> --- a/drivers/net/ethernet/8390/ax88796.c
>> +++ b/drivers/net/ethernet/8390/ax88796.c
>> @@ -374,6 +374,10 @@ static int ax_mii_probe(struct net_device *dev)
>>               return -ENODEV;
>>       }
>>
>> +     /* write a non-reset pattern to the control register to
>> +      * re-arm the reset request detection logic (needed on AX88796B)
>> +      */
>> +     phy_write(phy_dev, MII_BMCR, 0);
>
> This should really be fixed in the PHY driver, not the MAC.

OK - do you want this separate, or as part of this series? Might have
a few side effects on more commonly used hardware, perhaps?

Cheers,

  Michael

>
>      Andrew
Andrew Lunn April 17, 2018, 1:32 a.m. | #3
> > This should really be fixed in the PHY driver, not the MAC.
> 
> OK - do you want this separate, or as part of this series? Might have
> a few side effects on more commonly used hardware, perhaps?

Hi Michael

What PHY driver is used? In the driver you can implement a .soft_reset
function which first does the dummy write, and then uses
genphy_soft_reset() to do the actual reset.

     Andrew
Michael Karcher April 17, 2018, 5:18 a.m. | #4
[Andrew, sorry for the dup. I did hit reply-to-auhor instead of
reply-to-all first.]

Andrew Lunn schrieb:
>> > This should really be fixed in the PHY driver, not the MAC.
>>
>> OK - do you want this separate, or as part of this series? Might have
>> a few side effects on more commonly used hardware, perhaps?
>
> Hi Michael
>
> What PHY driver is used?
The ax88796b comes with its own integrated (buggy) PHY needing this
workaround. This PHY has its own ID which is not known by Linux, so it is
using the genphy driver as fallback.

> In the driver you can implement a .soft_reset
> function which first does the dummy write, and then uses
> genphy_soft_reset() to do the actual reset.
We could do that - but I dont't see the point in creating a PHY driver
that is only ever used by this MAC driver, just to add a single line to
the genphy driver. If the same PHY might be used with a different MAC,
you definitely would have a point there, though.

Kind regards,
  Michael Karcher
Andrew Lunn April 17, 2018, 1:01 p.m. | #5
On Tue, Apr 17, 2018 at 07:18:10AM +0200, Michael Karcher wrote:
> [Andrew, sorry for the dup. I did hit reply-to-auhor instead of
> reply-to-all first.]
> 
> Andrew Lunn schrieb:
> >> > This should really be fixed in the PHY driver, not the MAC.
> >>
> >> OK - do you want this separate, or as part of this series? Might have
> >> a few side effects on more commonly used hardware, perhaps?
> >
> > Hi Michael
> >
> > What PHY driver is used?
> The ax88796b comes with its own integrated (buggy) PHY needing this
> workaround. This PHY has its own ID which is not known by Linux, so it is
> using the genphy driver as fallback.
> 
> > In the driver you can implement a .soft_reset
> > function which first does the dummy write, and then uses
> > genphy_soft_reset() to do the actual reset.
> We could do that - but I dont't see the point in creating a PHY driver
> that is only ever used by this MAC driver, just to add a single line to
> the genphy driver. If the same PHY might be used with a different MAC,
> you definitely would have a point there, though.


Hi Michael

We try to keep the core code clean, and put all workarounds for buggy
hardware in drivers specific to them. It just helps keep the core code
maintainable.

I would prefer a driver specific to this PHY with the workaround. But
lets see what Florian says.

     Andrew
Florian Fainelli April 17, 2018, 6:08 p.m. | #6
On 04/17/2018 06:01 AM, Andrew Lunn wrote:
> On Tue, Apr 17, 2018 at 07:18:10AM +0200, Michael Karcher wrote:
>> [Andrew, sorry for the dup. I did hit reply-to-auhor instead of
>> reply-to-all first.]
>>
>> Andrew Lunn schrieb:
>>>>> This should really be fixed in the PHY driver, not the MAC.
>>>>
>>>> OK - do you want this separate, or as part of this series? Might have
>>>> a few side effects on more commonly used hardware, perhaps?
>>>
>>> Hi Michael
>>>
>>> What PHY driver is used?
>> The ax88796b comes with its own integrated (buggy) PHY needing this
>> workaround. This PHY has its own ID which is not known by Linux, so it is
>> using the genphy driver as fallback.
>>
>>> In the driver you can implement a .soft_reset
>>> function which first does the dummy write, and then uses
>>> genphy_soft_reset() to do the actual reset.
>> We could do that - but I dont't see the point in creating a PHY driver
>> that is only ever used by this MAC driver, just to add a single line to
>> the genphy driver. If the same PHY might be used with a different MAC,
>> you definitely would have a point there, though.
> 
> 
> Hi Michael
> 
> We try to keep the core code clean, and put all workarounds for buggy
> hardware in drivers specific to them. It just helps keep the core code
> maintainable.
> 
> I would prefer a driver specific to this PHY with the workaround. But
> lets see what Florian says.

If you are already using the generic PHY driver, coming up with a custom
one that only overrides the soft_reset and/or config_init callback is
really not that much work, and as Andrew says, it helps make things
clearer and properly isolated. As far as where to place that driver, you
can either create a new file under drivers/net/phy/* or you can even
register a phy_driver instance from within ax88796 if that makes it any
clearer.

FWIW, there are plenty of examples where there is a PHY driver used by a
single MAC, and that is perfectly fine, because the abstraction is still
preserved.
Michael Schmitz April 17, 2018, 7:25 p.m. | #7
Thanks Florian,

I'll keep the Asix PHY driver separate from ax88796 for now. Mainly to
simplify testing. Let's see whether it can be used by any other MAC -
can still fold it into ax88796 later.

Cheers,

  Michael


On Wed, Apr 18, 2018 at 6:08 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 04/17/2018 06:01 AM, Andrew Lunn wrote:
>> On Tue, Apr 17, 2018 at 07:18:10AM +0200, Michael Karcher wrote:
>>> [Andrew, sorry for the dup. I did hit reply-to-auhor instead of
>>> reply-to-all first.]
>>>
>>> Andrew Lunn schrieb:
>>>>>> This should really be fixed in the PHY driver, not the MAC.
>>>>>
>>>>> OK - do you want this separate, or as part of this series? Might have
>>>>> a few side effects on more commonly used hardware, perhaps?
>>>>
>>>> Hi Michael
>>>>
>>>> What PHY driver is used?
>>> The ax88796b comes with its own integrated (buggy) PHY needing this
>>> workaround. This PHY has its own ID which is not known by Linux, so it is
>>> using the genphy driver as fallback.
>>>
>>>> In the driver you can implement a .soft_reset
>>>> function which first does the dummy write, and then uses
>>>> genphy_soft_reset() to do the actual reset.
>>> We could do that - but I dont't see the point in creating a PHY driver
>>> that is only ever used by this MAC driver, just to add a single line to
>>> the genphy driver. If the same PHY might be used with a different MAC,
>>> you definitely would have a point there, though.
>>
>>
>> Hi Michael
>>
>> We try to keep the core code clean, and put all workarounds for buggy
>> hardware in drivers specific to them. It just helps keep the core code
>> maintainable.
>>
>> I would prefer a driver specific to this PHY with the workaround. But
>> lets see what Florian says.
>
> If you are already using the generic PHY driver, coming up with a custom
> one that only overrides the soft_reset and/or config_init callback is
> really not that much work, and as Andrew says, it helps make things
> clearer and properly isolated. As far as where to place that driver, you
> can either create a new file under drivers/net/phy/* or you can even
> register a phy_driver instance from within ax88796 if that makes it any
> clearer.
>
> FWIW, there are plenty of examples where there is a PHY driver used by a
> single MAC, and that is perfectly fine, because the abstraction is still
> preserved.
> --
> Florian

Patch

diff --git a/drivers/net/ethernet/8390/ax88796.c b/drivers/net/ethernet/8390/ax88796.c
index 6af9aca..a2f9a09 100644
--- a/drivers/net/ethernet/8390/ax88796.c
+++ b/drivers/net/ethernet/8390/ax88796.c
@@ -374,6 +374,10 @@  static int ax_mii_probe(struct net_device *dev)
 		return -ENODEV;
 	}
 
+	/* write a non-reset pattern to the control register to
+	 * re-arm the reset request detection logic (needed on AX88796B)
+	 */
+	phy_write(phy_dev, MII_BMCR, 0);
 	ret = phy_connect_direct(dev, phy_dev, ax_handle_link_change,
 				 PHY_INTERFACE_MODE_MII);
 	if (ret) {