| Message ID | 20180911101223.4217-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | smsc95xx: Add support for automated PHY address detection | expand |
On 9/11/18 12:12 PM, Marek Vasut wrote: > The SMSC95xx chip can use either the internal PHY or an external one. > Currently, the driver hard-codes support for the internal PHY only. > > This patch reads out the HW_CFG register to determine whether external > PHY is attached or not. If an external PHY is not attached, the driver > falls back to internal PHY with fixed PHY address 0x1. > > If an external PHY is attached, the driver scans the entire address > range of the MDIO bus to determine whether there is a valid external > PHY attached. The scanning happens from the end of the address range, > since some PHYs also respond to address 0, which is considered a MDIO > broadcast address by them. We want to obtain their real MDIO address > instead. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: David S. Miller <davem@davemloft.net> > Cc: Nisar Sayed <Nisar.Sayed@microchip.com> > Cc: Woojung Huh <Woojung.Huh@microchip.com> Expanding the CC, since I received no feedback. Still applies cleanly on next/master. > --- > drivers/net/usb/smsc95xx.c | 42 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 41 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c > index 06b4d290784d..014bb71ce8a8 100644 > --- a/drivers/net/usb/smsc95xx.c > +++ b/drivers/net/usb/smsc95xx.c > @@ -983,6 +983,40 @@ static int smsc95xx_start_rx_path(struct usbnet *dev, int in_pm) > return __smsc95xx_write_reg(dev, MAC_CR, pdata->mac_cr, in_pm); > } > > +static int smsc95xx_phy_address(struct usbnet *dev) > +{ > + u32 read_buf; > + int ret, id1, id2, phyad; > + > + ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf); > + if (ret < 0) > + return ret; > + > + /* Check if using external PHY, if not, use internal PHY address */ > + if (!(read_buf & HW_CFG_PSEL_)) > + return SMSC95XX_INTERNAL_PHY_ID; > + > + /* > + * Detect external PHY address. Here we probe the MDIO bus from > + * the highest address, since some PHYs respond also on address > + * zero, which they consider MDIO broadcast address. We really > + * want to get their proper address instead though, so we scan > + * address zero last. > + */ > + for (phyad = 0x1f; phyad >= 0; phyad--) { > + id1 = smsc95xx_mdio_read(dev->net, phyad, MII_PHYSID1); > + id2 = smsc95xx_mdio_read(dev->net, phyad, MII_PHYSID2); > + /* Check for valid response from the PHY */ > + if (id1 > 0 && id2 > 0 && id1 != 0x7fff && id2 != 0xffff) > + return phyad; > + } > + > + /* No PHY found. */ > + netdev_warn(dev->net, "cannot detect any PHY"); > + > + return -ENODEV; > +} > + > static int smsc95xx_phy_initialize(struct usbnet *dev) > { > int bmcr, ret, timeout = 0; > @@ -993,7 +1027,13 @@ static int smsc95xx_phy_initialize(struct usbnet *dev) > dev->mii.mdio_write = smsc95xx_mdio_write; > dev->mii.phy_id_mask = 0x1f; > dev->mii.reg_num_mask = 0x1f; > - dev->mii.phy_id = SMSC95XX_INTERNAL_PHY_ID; > + > + /* Determine PHY address */ > + ret = smsc95xx_phy_address(dev); > + if (ret) > + return ret; > + > + dev->mii.phy_id = ret; > > /* reset phy and wait for reset to complete */ > smsc95xx_mdio_write(dev->net, dev->mii.phy_id, MII_BMCR, BMCR_RESET); >
> > +static int smsc95xx_phy_address(struct usbnet *dev) > > +{ > > + u32 read_buf; > > + int ret, id1, id2, phyad; > > + > > + ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf); > > + if (ret < 0) > > + return ret; > > + > > + /* Check if using external PHY, if not, use internal PHY address */ > > + if (!(read_buf & HW_CFG_PSEL_)) > > + return SMSC95XX_INTERNAL_PHY_ID; > > + > > + /* > > + * Detect external PHY address. Here we probe the MDIO bus from > > + * the highest address, since some PHYs respond also on address > > + * zero, which they consider MDIO broadcast address. We really > > + * want to get their proper address instead though, so we scan > > + * address zero last. > > + */ > > + for (phyad = 0x1f; phyad >= 0; phyad--) { > > + id1 = smsc95xx_mdio_read(dev->net, phyad, MII_PHYSID1); > > + id2 = smsc95xx_mdio_read(dev->net, phyad, MII_PHYSID2); > > + /* Check for valid response from the PHY */ > > + if (id1 > 0 && id2 > 0 && id1 != 0x7fff && id2 != 0xffff) > > + return phyad; > > + } This would be so much easier if the driver used the core mdio/phy code. Just set mdio->phy_mask to ~BIT(0) and then use phy_find_first(). Anyway, net is closed at the moment, so please repost in three weeks time. Andrew
On 12/23/18 11:23 AM, Andrew Lunn wrote: >>> +static int smsc95xx_phy_address(struct usbnet *dev) >>> +{ >>> + u32 read_buf; >>> + int ret, id1, id2, phyad; >>> + >>> + ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf); >>> + if (ret < 0) >>> + return ret; >>> + >>> + /* Check if using external PHY, if not, use internal PHY address */ >>> + if (!(read_buf & HW_CFG_PSEL_)) >>> + return SMSC95XX_INTERNAL_PHY_ID; >>> + >>> + /* >>> + * Detect external PHY address. Here we probe the MDIO bus from >>> + * the highest address, since some PHYs respond also on address >>> + * zero, which they consider MDIO broadcast address. We really >>> + * want to get their proper address instead though, so we scan >>> + * address zero last. >>> + */ >>> + for (phyad = 0x1f; phyad >= 0; phyad--) { >>> + id1 = smsc95xx_mdio_read(dev->net, phyad, MII_PHYSID1); >>> + id2 = smsc95xx_mdio_read(dev->net, phyad, MII_PHYSID2); >>> + /* Check for valid response from the PHY */ >>> + if (id1 > 0 && id2 > 0 && id1 != 0x7fff && id2 != 0xffff) >>> + return phyad; >>> + } > > This would be so much easier if the driver used the core mdio/phy > code. Just set mdio->phy_mask to ~BIT(0) and then use > phy_find_first(). That's in the pipeline, along with PM cleanups, but low prio. > Anyway, net is closed at the moment, so please repost in three weeks > time. OK
On Sun, Dec 23, 2018 at 11:43:05AM +0100, Marek Vasut wrote: > On 12/23/18 11:23 AM, Andrew Lunn wrote: > >>> +static int smsc95xx_phy_address(struct usbnet *dev) > >>> +{ > >>> + u32 read_buf; > >>> + int ret, id1, id2, phyad; > >>> + > >>> + ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf); > >>> + if (ret < 0) > >>> + return ret; > >>> + > >>> + /* Check if using external PHY, if not, use internal PHY address */ > >>> + if (!(read_buf & HW_CFG_PSEL_)) > >>> + return SMSC95XX_INTERNAL_PHY_ID; > >>> + > >>> + /* > >>> + * Detect external PHY address. Here we probe the MDIO bus from > >>> + * the highest address, since some PHYs respond also on address > >>> + * zero, which they consider MDIO broadcast address. We really > >>> + * want to get their proper address instead though, so we scan > >>> + * address zero last. > >>> + */ > >>> + for (phyad = 0x1f; phyad >= 0; phyad--) { > >>> + id1 = smsc95xx_mdio_read(dev->net, phyad, MII_PHYSID1); > >>> + id2 = smsc95xx_mdio_read(dev->net, phyad, MII_PHYSID2); > >>> + /* Check for valid response from the PHY */ > >>> + if (id1 > 0 && id2 > 0 && id1 != 0x7fff && id2 != 0xffff) > >>> + return phyad; > >>> + } > > > > This would be so much easier if the driver used the core mdio/phy > > code. Just set mdio->phy_mask to ~BIT(0) and then use > > phy_find_first(). > > That's in the pipeline, along with PM cleanups, but low prio. Great. Does using the broadcast address actually cause a problem? If not, i would say lets drop this part of the patch until you do the cleanup. Andrew
On 12/23/18 11:56 AM, Andrew Lunn wrote: > On Sun, Dec 23, 2018 at 11:43:05AM +0100, Marek Vasut wrote: >> On 12/23/18 11:23 AM, Andrew Lunn wrote: >>>>> +static int smsc95xx_phy_address(struct usbnet *dev) >>>>> +{ >>>>> + u32 read_buf; >>>>> + int ret, id1, id2, phyad; >>>>> + >>>>> + ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + /* Check if using external PHY, if not, use internal PHY address */ >>>>> + if (!(read_buf & HW_CFG_PSEL_)) >>>>> + return SMSC95XX_INTERNAL_PHY_ID; >>>>> + >>>>> + /* >>>>> + * Detect external PHY address. Here we probe the MDIO bus from >>>>> + * the highest address, since some PHYs respond also on address >>>>> + * zero, which they consider MDIO broadcast address. We really >>>>> + * want to get their proper address instead though, so we scan >>>>> + * address zero last. >>>>> + */ >>>>> + for (phyad = 0x1f; phyad >= 0; phyad--) { >>>>> + id1 = smsc95xx_mdio_read(dev->net, phyad, MII_PHYSID1); >>>>> + id2 = smsc95xx_mdio_read(dev->net, phyad, MII_PHYSID2); >>>>> + /* Check for valid response from the PHY */ >>>>> + if (id1 > 0 && id2 > 0 && id1 != 0x7fff && id2 != 0xffff) >>>>> + return phyad; >>>>> + } >>> >>> This would be so much easier if the driver used the core mdio/phy >>> code. Just set mdio->phy_mask to ~BIT(0) and then use >>> phy_find_first(). >> >> That's in the pipeline, along with PM cleanups, but low prio. > > Great. Does using the broadcast address actually cause a problem? If > not, i would say lets drop this part of the patch until you do the > cleanup. Yeah, at least the TJA11xx doesn't respond on 0x0, so I wouldn't depend on the broadcast address. In my case, the PHY just sits at 0x4.
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c index 06b4d290784d..014bb71ce8a8 100644 --- a/drivers/net/usb/smsc95xx.c +++ b/drivers/net/usb/smsc95xx.c @@ -983,6 +983,40 @@ static int smsc95xx_start_rx_path(struct usbnet *dev, int in_pm) return __smsc95xx_write_reg(dev, MAC_CR, pdata->mac_cr, in_pm); } +static int smsc95xx_phy_address(struct usbnet *dev) +{ + u32 read_buf; + int ret, id1, id2, phyad; + + ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf); + if (ret < 0) + return ret; + + /* Check if using external PHY, if not, use internal PHY address */ + if (!(read_buf & HW_CFG_PSEL_)) + return SMSC95XX_INTERNAL_PHY_ID; + + /* + * Detect external PHY address. Here we probe the MDIO bus from + * the highest address, since some PHYs respond also on address + * zero, which they consider MDIO broadcast address. We really + * want to get their proper address instead though, so we scan + * address zero last. + */ + for (phyad = 0x1f; phyad >= 0; phyad--) { + id1 = smsc95xx_mdio_read(dev->net, phyad, MII_PHYSID1); + id2 = smsc95xx_mdio_read(dev->net, phyad, MII_PHYSID2); + /* Check for valid response from the PHY */ + if (id1 > 0 && id2 > 0 && id1 != 0x7fff && id2 != 0xffff) + return phyad; + } + + /* No PHY found. */ + netdev_warn(dev->net, "cannot detect any PHY"); + + return -ENODEV; +} + static int smsc95xx_phy_initialize(struct usbnet *dev) { int bmcr, ret, timeout = 0; @@ -993,7 +1027,13 @@ static int smsc95xx_phy_initialize(struct usbnet *dev) dev->mii.mdio_write = smsc95xx_mdio_write; dev->mii.phy_id_mask = 0x1f; dev->mii.reg_num_mask = 0x1f; - dev->mii.phy_id = SMSC95XX_INTERNAL_PHY_ID; + + /* Determine PHY address */ + ret = smsc95xx_phy_address(dev); + if (ret) + return ret; + + dev->mii.phy_id = ret; /* reset phy and wait for reset to complete */ smsc95xx_mdio_write(dev->net, dev->mii.phy_id, MII_BMCR, BMCR_RESET);
The SMSC95xx chip can use either the internal PHY or an external one. Currently, the driver hard-codes support for the internal PHY only. This patch reads out the HW_CFG register to determine whether external PHY is attached or not. If an external PHY is not attached, the driver falls back to internal PHY with fixed PHY address 0x1. If an external PHY is attached, the driver scans the entire address range of the MDIO bus to determine whether there is a valid external PHY attached. The scanning happens from the end of the address range, since some PHYs also respond to address 0, which is considered a MDIO broadcast address by them. We want to obtain their real MDIO address instead. Signed-off-by: Marek Vasut <marex@denx.de> Cc: David S. Miller <davem@davemloft.net> Cc: Nisar Sayed <Nisar.Sayed@microchip.com> Cc: Woojung Huh <Woojung.Huh@microchip.com> --- drivers/net/usb/smsc95xx.c | 42 +++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-)