diff mbox series

[v6,1/6] net: phy: at803x: Export at803x_debug_reg_mask()

Message ID 20180510231657.28503-2-paul.burton@mips.com
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series net: pch_gbe: MIPS support | expand

Commit Message

Paul Burton May 10, 2018, 11:16 p.m. UTC 
From: Andrew Lunn <andrew@lunn.ch>

On some boards, this PHY has a problem when it hibernates. Export this
function to a board can register a PHY fixup to disable hibernation.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Paul Burton <paul.burton@mips.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org

---

Changes in v6:
- New patch.

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/net/phy/at803x.c   |  5 +++--
 include/linux/at803x_phy.h | 16 ++++++++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)
 create mode 100644 include/linux/at803x_phy.h

Comments

Andrew Lunn May 11, 2018, 12:26 a.m. UTC | #1 
On Thu, May 10, 2018 at 04:16:52PM -0700, Paul Burton wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> 
> On some boards, this PHY has a problem when it hibernates. Export this
> function to a board can register a PHY fixup to disable hibernation.

What do you know about the problem?

https://patchwork.ozlabs.org/patch/686371/

I don't remember how it was solved, but you should probably do the
same.

	Andrew
Paul Burton May 11, 2018, 6:25 p.m. UTC | #2 
Hi Andrew,

On Fri, May 11, 2018 at 02:26:19AM +0200, Andrew Lunn wrote:
> On Thu, May 10, 2018 at 04:16:52PM -0700, Paul Burton wrote:
> > From: Andrew Lunn <andrew@lunn.ch>
> > 
> > On some boards, this PHY has a problem when it hibernates. Export this
> > function to a board can register a PHY fixup to disable hibernation.
> 
> What do you know about the problem?
> 
> https://patchwork.ozlabs.org/patch/686371/
> 
> I don't remember how it was solved, but you should probably do the
> same.
> 
> 	Andrew

I'm afraid I don't know much about the problem - this one is your patch
entirely unchanged, and I don't have access to the hardware in question
(my board uses a Realtek RTL8211E PHY).

I presume you did this because the pch_gbe driver as-is in mainline
disables hibernation for the AR803X PHY found on the MinnowBoard, so
this would be preserving the existing behaviour of the driver?

That behaviour was introduced by commit f1a26fdf5944f ("pch_gbe: Add
MinnowBoard support"), so perhaps Darren as its author might know more?

My presumption would be that this is done to ensure that the PHY is
always providing the RX clock, which the EG20T manual says is required
for the MAC reset register RX_RST & ALL_RST bits to clear. We wait for
those using the call to pch_gbe_wait_clr_bit() in
pch_gbe_mac_reset_hw(), which happens before we initialize the PHY.

I could reorder the probe function a little to initialize the PHY before
performing the MAC reset, drop this patch and the AR803X hibernation
stuff from patch 2 if you like. But again, I can't actually test the
result on the affected hardware.

Thanks,
    Paul
Paul Burton May 11, 2018, 6:38 p.m. UTC | #3 
On Fri, May 11, 2018 at 11:25:02AM -0700, Paul Burton wrote:
> Hi Andrew,
> 
> On Fri, May 11, 2018 at 02:26:19AM +0200, Andrew Lunn wrote:
> > On Thu, May 10, 2018 at 04:16:52PM -0700, Paul Burton wrote:
> > > From: Andrew Lunn <andrew@lunn.ch>
> > > 
> > > On some boards, this PHY has a problem when it hibernates. Export this
> > > function to a board can register a PHY fixup to disable hibernation.
> > 
> > What do you know about the problem?
> > 
> > https://patchwork.ozlabs.org/patch/686371/
> > 
> > I don't remember how it was solved, but you should probably do the
> > same.
> > 
> > 	Andrew
> 
> I'm afraid I don't know much about the problem - this one is your patch
> entirely unchanged, and I don't have access to the hardware in question
> (my board uses a Realtek RTL8211E PHY).
> 
> I presume you did this because the pch_gbe driver as-is in mainline
> disables hibernation for the AR803X PHY found on the MinnowBoard, so
> this would be preserving the existing behaviour of the driver?
> 
> That behaviour was introduced by commit f1a26fdf5944f ("pch_gbe: Add
> MinnowBoard support"), so perhaps Darren as its author might know more?
> 
> My presumption would be that this is done to ensure that the PHY is
> always providing the RX clock, which the EG20T manual says is required
> for the MAC reset register RX_RST & ALL_RST bits to clear. We wait for
> those using the call to pch_gbe_wait_clr_bit() in
> pch_gbe_mac_reset_hw(), which happens before we initialize the PHY.
> 
> I could reorder the probe function a little to initialize the PHY before
> performing the MAC reset, drop this patch and the AR803X hibernation
> stuff from patch 2 if you like. But again, I can't actually test the
> result on the affected hardware.
> 
> Thanks,
>     Paul

I got an undeliverable response using Darren's email address from the
commit referenced above, so updating to the latest address I see for him
in git history.

Thanks,
    Paul
Andrew Lunn May 11, 2018, 7:24 p.m. UTC | #4 
> I could reorder the probe function a little to initialize the PHY before
> performing the MAC reset, drop this patch and the AR803X hibernation
> stuff from patch 2 if you like. But again, I can't actually test the
> result on the affected hardware.

Hi Paul

I don't like a MAC driver poking around in PHY registers.

So if you can rearrange the code, that would be great.

   Thanks
	Andrew
Paul Burton May 11, 2018, 10:22 p.m. UTC | #5 
Hi Andrew,

On Fri, May 11, 2018 at 09:24:46PM +0200, Andrew Lunn wrote:
> > I could reorder the probe function a little to initialize the PHY before
> > performing the MAC reset, drop this patch and the AR803X hibernation
> > stuff from patch 2 if you like. But again, I can't actually test the
> > result on the affected hardware.
> 
> Hi Paul
> 
> I don't like a MAC driver poking around in PHY registers.
> 
> So if you can rearrange the code, that would be great.
> 
>    Thanks
> 	Andrew

Sure, I'll give it a shot.

After digging into it I see 2 ways to go here:

  1) We could just always reset the PHY before we reset the MAC. That
     would give us a window of however long the PHY takes to enter its
     low power state & stop providing the RX clock during which we'd
     need the MAC reset to complete. In the case of the AR8031 that's
     "about 10 seconds" according to its data sheet. In this particular
     case that feels like plenty, but it does also feel a bit icky to
     rely on the timing chosen by the PHY manufacturer to line up with
     that of the MAC reset.

  2) We could introduce a couple of new phy_* functions to disable &
     enable low power states like the AR8031's hibernation feature, by
     calling new function pointers in struct phy_driver. Then pch_gbe &
     other MACs could call those to have the PHY driver disable
     hibernation at times where we know we'll need the RX clock and
     re-enable it afterwards.

I'm currently leaning towards option 2. How does that sound to you? Or
can you see another way to handle this?

Thanks,
    Paul
Andrew Lunn May 12, 2018, 9:37 p.m. UTC | #6 
On Fri, May 11, 2018 at 03:22:39PM -0700, Paul Burton wrote:
> Hi Andrew,
> 
> On Fri, May 11, 2018 at 09:24:46PM +0200, Andrew Lunn wrote:
> > > I could reorder the probe function a little to initialize the PHY before
> > > performing the MAC reset, drop this patch and the AR803X hibernation
> > > stuff from patch 2 if you like. But again, I can't actually test the
> > > result on the affected hardware.
> > 
> > Hi Paul
> > 
> > I don't like a MAC driver poking around in PHY registers.
> > 
> > So if you can rearrange the code, that would be great.
> > 
> >    Thanks
> > 	Andrew
> 
> Sure, I'll give it a shot.
> 
> After digging into it I see 2 ways to go here:
> 
>   1) We could just always reset the PHY before we reset the MAC. That
>      would give us a window of however long the PHY takes to enter its
>      low power state & stop providing the RX clock during which we'd
>      need the MAC reset to complete. In the case of the AR8031 that's
>      "about 10 seconds" according to its data sheet. In this particular
>      case that feels like plenty, but it does also feel a bit icky to
>      rely on the timing chosen by the PHY manufacturer to line up with
>      that of the MAC reset.
> 
>   2) We could introduce a couple of new phy_* functions to disable &
>      enable low power states like the AR8031's hibernation feature, by
>      calling new function pointers in struct phy_driver. Then pch_gbe &
>      other MACs could call those to have the PHY driver disable
>      hibernation at times where we know we'll need the RX clock and
>      re-enable it afterwards.

Hi Paul

When there is no link, you don't need the MAC running. My assumption
is that the PHY is designed around that idea, you leave the MAC idle
until there is a link. When the phylib calls the link_change handler,
the MAC should then be started/stopped depending on the state of the
link. You are then guaranteed to have the clock when you need it.

I've no idea how easy this is to implement given the current code...

     Andrew
diff mbox series

Patch

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 411cf1072bae..5aede5708abf 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -18,6 +18,7 @@ 
 #include <linux/etherdevice.h>
 #include <linux/of_gpio.h>
 #include <linux/gpio/consumer.h>
+#include <linux/at803x_phy.h>
 
 #define AT803X_INTR_ENABLE			0x12
 #define AT803X_INTR_ENABLE_AUTONEG_ERR		BIT(15)
@@ -93,8 +94,8 @@  static int at803x_debug_reg_read(struct phy_device *phydev, u16 reg)
 	return phy_read(phydev, AT803X_DEBUG_DATA);
 }
 
-static int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
-				 u16 clear, u16 set)
+int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
+			  u16 clear, u16 set)
 {
 	u16 val;
 	int ret;
diff --git a/include/linux/at803x_phy.h b/include/linux/at803x_phy.h
new file mode 100644
index 000000000000..2460c17d56ec
--- /dev/null
+++ b/include/linux/at803x_phy.h
@@ -0,0 +1,16 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _AT803X_PHY_H
+#define _PHY_AT803X_PHY
+
+#define ATH8030_PHY_ID		0x004dd076
+#define ATH8031_PHY_ID		0x004dd074
+#define ATH8035_PHY_ID		0x004dd072
+#define AT803X_PHY_ID_MASK	0xffffffef
+
+#define AT8031_HIBERNATE	0x0B
+#define AT8031_PS_HIB_EN	0x8000 /* Hibernate enable */
+
+int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
+			  u16 clear, u16 set);
+
+#endif /* _AT803X_PHY_H */