| Message ID | 1477688219-3871-3-git-send-email-jon.mason@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 28, 2016 at 04:56:55PM -0400, Jon Mason wrote: > The BCM54810 PHY requires some semi-unique configuration, which results > in some additional configuration in addition to the standard config. > Also, some users of the BCM54810 require the PHY lanes to be swapped. > Since there is no way to detect this, add a device tree query to see if > it is applicable. > > Inspired-by: Vikas Soni <vsoni@broadcom.com> > Signed-off-by: Jon Mason <jon.mason@broadcom.com> > --- > drivers/net/phy/Kconfig | 2 +- > drivers/net/phy/broadcom.c | 58 +++++++++++++++++++++++++++++++++++++++++++++- > include/linux/brcmphy.h | 10 ++++++++ Hi Jon The binding documentation is missing. > + if (of_property_read_bool(np, "brcm,enet-phy-lane-swap")) { > + /* Lane Swap - Undocumented register...magic! */ > + ret = bcm_phy_write_exp(phydev, MII_BCM54XX_EXP_SEL_ER + 0x9, > + 0x11B); > + if (ret < 0) > + return ret; > + } > + I wounder if this property could be made generic? What exactly are you swapping? Rx and Tx lanes? Maybe we should add it to phy.txt? Andrew
On Sat, Oct 29, 2016 at 10:18:39AM +0200, Andrew Lunn wrote: > On Fri, Oct 28, 2016 at 04:56:55PM -0400, Jon Mason wrote: > > The BCM54810 PHY requires some semi-unique configuration, which results > > in some additional configuration in addition to the standard config. > > Also, some users of the BCM54810 require the PHY lanes to be swapped. > > Since there is no way to detect this, add a device tree query to see if > > it is applicable. > > > > Inspired-by: Vikas Soni <vsoni@broadcom.com> > > Signed-off-by: Jon Mason <jon.mason@broadcom.com> > > --- > > drivers/net/phy/Kconfig | 2 +- > > drivers/net/phy/broadcom.c | 58 +++++++++++++++++++++++++++++++++++++++++++++- > > include/linux/brcmphy.h | 10 ++++++++ > > Hi Jon > > The binding documentation is missing. > > > + if (of_property_read_bool(np, "brcm,enet-phy-lane-swap")) { > > + /* Lane Swap - Undocumented register...magic! */ > > + ret = bcm_phy_write_exp(phydev, MII_BCM54XX_EXP_SEL_ER + 0x9, > > + 0x11B); > > + if (ret < 0) > > + return ret; > > + } > > + > > I wounder if this property could be made generic? What exactly are you > swapping? Rx and Tx lanes? Maybe we should add it to phy.txt? Are you envisioning adding a DT check (similar to the of_property_read_bool above, only with a more generic string) in phy_device_create(), which will then set a PHY device flag? This flag would then be checked for in the PHY driver and the appropriate action taken (in this case the bcm_phy_write_exp above). If so, I cam completely fine doing this. I think the only caveat would be that this would be creating a generic interface for only 1 user. If you envision this being used by others, then disregard my concern. Thanks, Jon > > Andrew
> > > + if (of_property_read_bool(np, "brcm,enet-phy-lane-swap")) { > > > + /* Lane Swap - Undocumented register...magic! */ > > > + ret = bcm_phy_write_exp(phydev, MII_BCM54XX_EXP_SEL_ER + 0x9, > > > + 0x11B); > > > + if (ret < 0) > > > + return ret; > > > + } > > > + > > > > I wounder if this property could be made generic? What exactly are you > > swapping? Rx and Tx lanes? Maybe we should add it to phy.txt? > > Are you envisioning adding a DT check (similar to the > of_property_read_bool above, only with a more generic string) in > phy_device_create(), which will then set a PHY device flag? This flag > would then be checked for in the PHY driver and the appropriate action > taken (in this case the bcm_phy_write_exp above). I would keep the parsing of the property in the driver. But if we think other PHYs could also support this feature, it would be good to avoid having "brcm,enet-phy-lane-swap", "marvell,enet-phy-lane-swap", "davicom,enet-phy-lane-swap", etc. It would be better to have one well defined property documented in phy.txt which any PHY is free to implement. Andrew
On Tue, Nov 01, 2016 at 05:26:48PM +0100, Andrew Lunn wrote: > > > > + if (of_property_read_bool(np, "brcm,enet-phy-lane-swap")) { > > > > + /* Lane Swap - Undocumented register...magic! */ > > > > + ret = bcm_phy_write_exp(phydev, MII_BCM54XX_EXP_SEL_ER + 0x9, > > > > + 0x11B); > > > > + if (ret < 0) > > > > + return ret; > > > > + } > > > > + > > > > > > I wounder if this property could be made generic? What exactly are you > > > swapping? Rx and Tx lanes? Maybe we should add it to phy.txt? > > > > Are you envisioning adding a DT check (similar to the > > of_property_read_bool above, only with a more generic string) in > > phy_device_create(), which will then set a PHY device flag? This flag > > would then be checked for in the PHY driver and the appropriate action > > taken (in this case the bcm_phy_write_exp above). > > I would keep the parsing of the property in the driver. But if we > think other PHYs could also support this feature, it would be good to > avoid having "brcm,enet-phy-lane-swap", "marvell,enet-phy-lane-swap", > "davicom,enet-phy-lane-swap", etc. It would be better to have one well > defined property documented in phy.txt which any PHY is free to > implement. Okay, I understand what you are saying now. I will assume that if nothing exists today aside from this Broadcom errata, something in the future will happen. So, I agree that making it generic is a good idea. I'll make the generic string be "enet-phy-lane-swap" (without the previous "brcm"), and add the flag to phy.txt to document it. Thanks, Jon > > Andrew
> I'll make the generic string be "enet-phy-lane-swap" (without the > previous "brcm"), and add the flag to phy.txt to document it. Great. Please be quite verbose as to what it means. Thanks Andrew
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 45f68ea..31967ca 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -217,7 +217,7 @@ config BROADCOM_PHY select BCM_NET_PHYLIB ---help--- Currently supports the BCM5411, BCM5421, BCM5461, BCM54616S, BCM5464, - BCM5481 and BCM5482 PHYs. + BCM5481, BCM54810 and BCM5482 PHYs. config CICADA_PHY tristate "Cicada PHYs" diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c index 3a64b3d..777ea29 100644 --- a/drivers/net/phy/broadcom.c +++ b/drivers/net/phy/broadcom.c @@ -18,7 +18,7 @@ #include <linux/module.h> #include <linux/phy.h> #include <linux/brcmphy.h> - +#include <linux/of.h> #define BRCM_PHY_MODEL(phydev) \ ((phydev)->drv->phy_id & (phydev)->drv->phy_id_mask) @@ -45,6 +45,34 @@ static int bcm54xx_auxctl_write(struct phy_device *phydev, u16 regnum, u16 val) return phy_write(phydev, MII_BCM54XX_AUX_CTL, regnum | val); } +static int bcm54810_config(struct phy_device *phydev) +{ + int rc, val; + + val = bcm_phy_read_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL); + val &= ~BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN; + rc = bcm_phy_write_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL, + val); + if (rc < 0) + return rc; + + val = bcm54xx_auxctl_read(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC); + val &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN; + val |= MII_BCM54XX_AUXCTL_MISC_WREN; + rc = bcm54xx_auxctl_write(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC, + val); + if (rc < 0) + return rc; + + val = bcm_phy_read_shadow(phydev, BCM54810_SHD_CLK_CTL); + val &= ~BCM54810_SHD_CLK_CTL_GTXCLK_EN; + rc = bcm_phy_write_shadow(phydev, BCM54810_SHD_CLK_CTL, val); + if (rc < 0) + return rc; + + return 0; +} + /* Needs SMDSP clock enabled via bcm54xx_phydsp_config() */ static int bcm50610_a0_workaround(struct phy_device *phydev) { @@ -217,6 +245,12 @@ static int bcm54xx_config_init(struct phy_device *phydev) (phydev->dev_flags & PHY_BRCM_AUTO_PWRDWN_ENABLE)) bcm54xx_adjust_rxrefclk(phydev); + if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810) { + err = bcm54810_config(phydev); + if (err) + return err; + } + bcm54xx_phydsp_config(phydev); return 0; @@ -314,6 +348,7 @@ static int bcm5482_read_status(struct phy_device *phydev) static int bcm5481_config_aneg(struct phy_device *phydev) { + struct device_node *np = phydev->mdio.dev.of_node; int ret; /* Aneg firsly. */ @@ -344,6 +379,14 @@ static int bcm5481_config_aneg(struct phy_device *phydev) phy_write(phydev, 0x18, reg); } + if (of_property_read_bool(np, "brcm,enet-phy-lane-swap")) { + /* Lane Swap - Undocumented register...magic! */ + ret = bcm_phy_write_exp(phydev, MII_BCM54XX_EXP_SEL_ER + 0x9, + 0x11B); + if (ret < 0) + return ret; + } + return ret; } @@ -578,6 +621,18 @@ static struct phy_driver broadcom_drivers[] = { .ack_interrupt = bcm_phy_ack_intr, .config_intr = bcm_phy_config_intr, }, { + .phy_id = PHY_ID_BCM54810, + .phy_id_mask = 0xfffffff0, + .name = "Broadcom BCM54810", + .features = PHY_GBIT_FEATURES | + SUPPORTED_Pause | SUPPORTED_Asym_Pause, + .flags = PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT, + .config_init = bcm54xx_config_init, + .config_aneg = bcm5481_config_aneg, + .read_status = genphy_read_status, + .ack_interrupt = bcm_phy_ack_intr, + .config_intr = bcm_phy_config_intr, +}, { .phy_id = PHY_ID_BCM5482, .phy_id_mask = 0xfffffff0, .name = "Broadcom BCM5482", @@ -661,6 +716,7 @@ static struct mdio_device_id __maybe_unused broadcom_tbl[] = { { PHY_ID_BCM54616S, 0xfffffff0 }, { PHY_ID_BCM5464, 0xfffffff0 }, { PHY_ID_BCM5481, 0xfffffff0 }, + { PHY_ID_BCM54810, 0xfffffff0 }, { PHY_ID_BCM5482, 0xfffffff0 }, { PHY_ID_BCM50610, 0xfffffff0 }, { PHY_ID_BCM50610M, 0xfffffff0 }, diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h index 0ed6691..69c7a79 100644 --- a/include/linux/brcmphy.h +++ b/include/linux/brcmphy.h @@ -13,6 +13,7 @@ #define PHY_ID_BCM5241 0x0143bc30 #define PHY_ID_BCMAC131 0x0143bc70 #define PHY_ID_BCM5481 0x0143bca0 +#define PHY_ID_BCM54810 0x03625d00 #define PHY_ID_BCM5482 0x0143bcb0 #define PHY_ID_BCM5411 0x00206070 #define PHY_ID_BCM5421 0x002060e0 @@ -56,6 +57,8 @@ #define PHY_BRCM_EXT_IBND_TX_ENABLE 0x00002000 #define PHY_BRCM_CLEAR_RGMII_MODE 0x00004000 #define PHY_BRCM_DIS_TXCRXC_NOENRGY 0x00008000 +#define PHY_BRCM_EXP_LANE_SWAP 0x00010000 + /* Broadcom BCM7xxx specific workarounds */ #define PHY_BRCM_7XXX_REV(x) (((x) >> 8) & 0xff) #define PHY_BRCM_7XXX_PATCH(x) ((x) & 0xff) @@ -111,6 +114,7 @@ #define MII_BCM54XX_AUXCTL_MISC_RDSEL_MISC 0x7000 #define MII_BCM54XX_AUXCTL_SHDWSEL_MISC 0x0007 #define MII_BCM54XX_AUXCTL_SHDWSEL_READ_SHIFT 12 +#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN (1 << 8) #define MII_BCM54XX_AUXCTL_SHDWSEL_MASK 0x0007 @@ -192,6 +196,12 @@ #define BCM5482_SSD_SGMII_SLAVE_EN 0x0002 /* Slave mode enable */ #define BCM5482_SSD_SGMII_SLAVE_AD 0x0001 /* Slave auto-detection */ +/* BCM54810 Registers */ +#define BCM54810_EXP_BROADREACH_LRE_MISC_CTL (MII_BCM54XX_EXP_SEL_ER + 0x90) +#define BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN (1 << 0) +#define BCM54810_SHD_CLK_CTL 0x3 +#define BCM54810_SHD_CLK_CTL_GTXCLK_EN (1 << 9) + /*****************************************************************************/ /* Fast Ethernet Transceiver definitions. */
The BCM54810 PHY requires some semi-unique configuration, which results in some additional configuration in addition to the standard config. Also, some users of the BCM54810 require the PHY lanes to be swapped. Since there is no way to detect this, add a device tree query to see if it is applicable. Inspired-by: Vikas Soni <vsoni@broadcom.com> Signed-off-by: Jon Mason <jon.mason@broadcom.com> --- drivers/net/phy/Kconfig | 2 +- drivers/net/phy/broadcom.c | 58 +++++++++++++++++++++++++++++++++++++++++++++- include/linux/brcmphy.h | 10 ++++++++ 3 files changed, 68 insertions(+), 2 deletions(-)