Re: [PATCH] smsc95xx: Add support for automated PHY address detection

Re: [PATCH] smsc95xx: Add support for automated PHY address detection

[Marek Vasut]

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);
> 

Re: [PATCH] smsc95xx: Add support for automated PHY address detection

[Andrew Lunn]

> > +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

Re: [PATCH] smsc95xx: Add support for automated PHY address detection

[Marek Vasut]

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

-- 
Best regards,
Marek Vasut

Re: [PATCH] smsc95xx: Add support for automated PHY address detection

[Andrew Lunn]

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

Re: [PATCH] smsc95xx: Add support for automated PHY address detection

[Marek Vasut]

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.

-- 
Best regards,
Marek Vasut