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 |
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
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
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
> 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
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
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 --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 */