diff mbox series

[10/19] usbnet: smsc95xx: Replace smsc95xx_mdio_read() with phy_read()

Message ID 20190103011040.25974-11-marex@denx.de (mailing list archive)
State New, archived
Headers show
Series usbnet: smsc95xx: Convert to phydev | expand

Commit Message

Marek Vasut Jan. 3, 2019, 1:10 a.m. UTC 
Just replace smsc95xx_mdio_read() with generic phy_read() to reduce
usage of the ad-hoc accessors.

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>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: linux-usb@vger.kernel.org
To: netdev@vger.kernel.org
---
 drivers/net/usb/smsc95xx.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

Comments

Andrew Lunn Jan. 3, 2019, 1:39 p.m. UTC | #1 
>  static int get_mdix_status(struct net_device *net)
>  {
>  	struct usbnet *dev = netdev_priv(net);
> +	struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
>  	u32 val;
>  	int buf;
>  
> -	buf = smsc95xx_mdio_read(dev->net, dev->mii.phy_id, SPECIAL_CTRL_STS);
> +	buf = phy_read(pdata->phydev, SPECIAL_CTRL_STS);
>  	if (buf & SPECIAL_CTRL_STS_OVRRD_AMDIX_) {
>  		if (buf & SPECIAL_CTRL_STS_AMDIX_ENABLE_)
>  			return ETH_TP_MDI_AUTO;
> @@ -793,7 +794,7 @@ static void set_mdix_status(struct net_device *net, __u8 mdix_ctrl)
>  	    (pdata->chip_id == ID_REV_CHIP_ID_89530_) ||
>  	    (pdata->chip_id == ID_REV_CHIP_ID_9730_)) {
>  		/* Extend Manual AutoMDIX timer for 9500A/9500Ai */
> -		buf = smsc95xx_mdio_read(dev->net, dev->mii.phy_id,
> +		buf = phy_read(pdata->phydev,
>  					 PHY_EDPD_CONFIG);
>  		buf |= PHY_EDPD_CONFIG_EXT_CROSSOVER_;
>  		smsc95xx_mdio_write(dev->net, dev->mii.phy_id,
> @@ -801,7 +802,7 @@ static void set_mdix_status(struct net_device *net, __u8 mdix_ctrl)
>  	}
>  
>  	if (mdix_ctrl == ETH_TP_MDI) {
> -		buf = smsc95xx_mdio_read(dev->net, dev->mii.phy_id,
> +		buf = phy_read(pdata->phydev,
>  					 SPECIAL_CTRL_STS);
>  		buf |= SPECIAL_CTRL_STS_OVRRD_AMDIX_;
>  		buf &= ~(SPECIAL_CTRL_STS_AMDIX_ENABLE_ |
> @@ -809,7 +810,7 @@ static void set_mdix_status(struct net_device *net, __u8 mdix_ctrl)
>  		smsc95xx_mdio_write(dev->net, dev->mii.phy_id,
>  				    SPECIAL_CTRL_STS, buf);
>  	} else if (mdix_ctrl == ETH_TP_MDI_X) {
> -		buf = smsc95xx_mdio_read(dev->net, dev->mii.phy_id,
> +		buf = phy_read(pdata->phydev,
>  					 SPECIAL_CTRL_STS);
>  		buf |= SPECIAL_CTRL_STS_OVRRD_AMDIX_;
>  		buf &= ~(SPECIAL_CTRL_STS_AMDIX_ENABLE_ |
> @@ -818,7 +819,7 @@ static void set_mdix_status(struct net_device *net, __u8 mdix_ctrl)
>  		smsc95xx_mdio_write(dev->net, dev->mii.phy_id,
>  				    SPECIAL_CTRL_STS, buf);
>  	} else if (mdix_ctrl == ETH_TP_MDI_AUTO) {
> -		buf = smsc95xx_mdio_read(dev->net, dev->mii.phy_id,
> +		buf = phy_read(pdata->phydev,
>  					 SPECIAL_CTRL_STS);
>  		buf &= ~SPECIAL_CTRL_STS_OVRRD_AMDIX_;
>  		buf &= ~(SPECIAL_CTRL_STS_AMDIX_ENABLE_ |
> @@ -968,6 +969,7 @@ static void smsc95xx_adjust_link(struct net_device *netdev)
 
All this crossover code should be moved into the PHY driver.

    Andrew
Marek Vasut Jan. 4, 2019, 2:19 a.m. UTC | #2 
On 1/3/19 2:39 PM, Andrew Lunn wrote:
>>  static int get_mdix_status(struct net_device *net)
>>  {
>>  	struct usbnet *dev = netdev_priv(net);
>> +	struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
>>  	u32 val;
>>  	int buf;
>>  
>> -	buf = smsc95xx_mdio_read(dev->net, dev->mii.phy_id, SPECIAL_CTRL_STS);
>> +	buf = phy_read(pdata->phydev, SPECIAL_CTRL_STS);
>>  	if (buf & SPECIAL_CTRL_STS_OVRRD_AMDIX_) {
>>  		if (buf & SPECIAL_CTRL_STS_AMDIX_ENABLE_)
>>  			return ETH_TP_MDI_AUTO;
>> @@ -793,7 +794,7 @@ static void set_mdix_status(struct net_device *net, __u8 mdix_ctrl)
>>  	    (pdata->chip_id == ID_REV_CHIP_ID_89530_) ||
>>  	    (pdata->chip_id == ID_REV_CHIP_ID_9730_)) {
>>  		/* Extend Manual AutoMDIX timer for 9500A/9500Ai */
>> -		buf = smsc95xx_mdio_read(dev->net, dev->mii.phy_id,
>> +		buf = phy_read(pdata->phydev,
>>  					 PHY_EDPD_CONFIG);
>>  		buf |= PHY_EDPD_CONFIG_EXT_CROSSOVER_;
>>  		smsc95xx_mdio_write(dev->net, dev->mii.phy_id,
>> @@ -801,7 +802,7 @@ static void set_mdix_status(struct net_device *net, __u8 mdix_ctrl)
>>  	}
>>  
>>  	if (mdix_ctrl == ETH_TP_MDI) {
>> -		buf = smsc95xx_mdio_read(dev->net, dev->mii.phy_id,
>> +		buf = phy_read(pdata->phydev,
>>  					 SPECIAL_CTRL_STS);
>>  		buf |= SPECIAL_CTRL_STS_OVRRD_AMDIX_;
>>  		buf &= ~(SPECIAL_CTRL_STS_AMDIX_ENABLE_ |
>> @@ -809,7 +810,7 @@ static void set_mdix_status(struct net_device *net, __u8 mdix_ctrl)
>>  		smsc95xx_mdio_write(dev->net, dev->mii.phy_id,
>>  				    SPECIAL_CTRL_STS, buf);
>>  	} else if (mdix_ctrl == ETH_TP_MDI_X) {
>> -		buf = smsc95xx_mdio_read(dev->net, dev->mii.phy_id,
>> +		buf = phy_read(pdata->phydev,
>>  					 SPECIAL_CTRL_STS);
>>  		buf |= SPECIAL_CTRL_STS_OVRRD_AMDIX_;
>>  		buf &= ~(SPECIAL_CTRL_STS_AMDIX_ENABLE_ |
>> @@ -818,7 +819,7 @@ static void set_mdix_status(struct net_device *net, __u8 mdix_ctrl)
>>  		smsc95xx_mdio_write(dev->net, dev->mii.phy_id,
>>  				    SPECIAL_CTRL_STS, buf);
>>  	} else if (mdix_ctrl == ETH_TP_MDI_AUTO) {
>> -		buf = smsc95xx_mdio_read(dev->net, dev->mii.phy_id,
>> +		buf = phy_read(pdata->phydev,
>>  					 SPECIAL_CTRL_STS);
>>  		buf &= ~SPECIAL_CTRL_STS_OVRRD_AMDIX_;
>>  		buf &= ~(SPECIAL_CTRL_STS_AMDIX_ENABLE_ |
>> @@ -968,6 +969,7 @@ static void smsc95xx_adjust_link(struct net_device *netdev)
>  
> All this crossover code should be moved into the PHY driver.

Fine, this can be done in a subsequent patch though, right ? I'd like to
keep the changes small, so if something breaks, it could be bisected easily.
Andrew Lunn Jan. 4, 2019, 1:24 p.m. UTC | #3 
> > All this crossover code should be moved into the PHY driver.
> 
> Fine, this can be done in a subsequent patch though, right ? I'd like to
> keep the changes small, so if something breaks, it could be bisected easily.

Hi Marek

The problem is, do you have a regression because of the changes you
made here, or because you have two things controlling the PHY, the MAC
driver and the PHY driver, which are not coordinating?

I don't know if you can do a piecemeal conversation like this. I don't
remember anybody else doing anything like this before.

What you can do piecemeal is add new functionality to the existing PHY
driver. If you break the PHY driver we can then bisect it to find out
which change broke it. Once the PHY driver has everything you need,
then swap the MAC driver to use phylib.

     Thanks
	Andrew
Marek Vasut Jan. 4, 2019, 2:58 p.m. UTC | #4 
On 1/4/19 2:24 PM, Andrew Lunn wrote:
>>> All this crossover code should be moved into the PHY driver.
>>
>> Fine, this can be done in a subsequent patch though, right ? I'd like to
>> keep the changes small, so if something breaks, it could be bisected easily.
> 
> Hi Marek

Hi,

> The problem is, do you have a regression because of the changes you
> made here, or because you have two things controlling the PHY, the MAC
> driver and the PHY driver, which are not coordinating?
> 
> I don't know if you can do a piecemeal conversation like this. I don't
> remember anybody else doing anything like this before.

I'd prefer that.

> What you can do piecemeal is add new functionality to the existing PHY
> driver. If you break the PHY driver we can then bisect it to find out
> which change broke it. Once the PHY driver has everything you need,
> then swap the MAC driver to use phylib.

I wonder, if I use the phylib functions instead of the ad-hoc ones in
the MAC driver, is there still a problem with synchronization ?
Andrew Lunn Jan. 4, 2019, 3:57 p.m. UTC | #5 
> I wonder, if I use the phylib functions instead of the ad-hoc ones in
> the MAC driver, is there still a problem with synchronization ?

You would need to look deep into phylib. When does it reset the PHY?
Configure auto-neg, setup interrupts, etc? It looks like both are
going to do this, so i expect they are going to mess each other up.

      Andrew
Heiner Kallweit Jan. 4, 2019, 6:02 p.m. UTC | #6 
On 04.01.2019 16:57, Andrew Lunn wrote:
>> I wonder, if I use the phylib functions instead of the ad-hoc ones in
>> the MAC driver, is there still a problem with synchronization ?
> 
> You would need to look deep into phylib. When does it reset the PHY?
> Configure auto-neg, setup interrupts, etc? It looks like both are
> going to do this, so i expect they are going to mess each other up.
> 
Marek, I recently went through this exercise when switching r8169
driver to phylib. As Andrew explained:
First add needed functionality to the respective PHY driver(s),
then you can switch the network driver.
You can look at f1e911d5d0df ("r8169: add basic phylib support")
plus related changes, and to what was added to the Realtek PHY
driver module.
diff mbox series

Patch

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index ce61be8ee32b..53f817e699df 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -547,7 +547,7 @@  static int smsc95xx_link_reset(struct usbnet *dev)
 	int ret;
 
 	/* clear interrupt status */
-	ret = smsc95xx_mdio_read(dev->net, mii->phy_id, PHY_INT_SRC);
+	ret = phy_read(pdata->phydev, PHY_INT_SRC);
 	if (ret < 0)
 		return ret;
 
@@ -557,8 +557,8 @@  static int smsc95xx_link_reset(struct usbnet *dev)
 
 	mii_check_media(mii, 1, 1);
 	mii_ethtool_gset(&dev->mii, &ecmd);
-	lcladv = smsc95xx_mdio_read(dev->net, mii->phy_id, MII_ADVERTISE);
-	rmtadv = smsc95xx_mdio_read(dev->net, mii->phy_id, MII_LPA);
+	lcladv = phy_read(pdata->phydev, MII_ADVERTISE);
+	rmtadv = phy_read(pdata->phydev, MII_LPA);
 
 	netif_dbg(dev, link, dev->net,
 		  "speed: %u duplex: %d lcladv: %04x rmtadv: %04x\n",
@@ -630,7 +630,7 @@  static void check_carrier(struct work_struct *work)
 	if (pdata->suspend_flags != 0)
 		return;
 
-	ret = smsc95xx_mdio_read(dev->net, dev->mii.phy_id, MII_BMSR);
+	ret = phy_read(pdata->phydev, MII_BMSR);
 	if (ret < 0) {
 		netdev_warn(dev->net, "Failed to read MII_BMSR\n");
 		return;
@@ -764,10 +764,11 @@  static int smsc95xx_ethtool_set_wol(struct net_device *net,
 static int get_mdix_status(struct net_device *net)
 {
 	struct usbnet *dev = netdev_priv(net);
+	struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
 	u32 val;
 	int buf;
 
-	buf = smsc95xx_mdio_read(dev->net, dev->mii.phy_id, SPECIAL_CTRL_STS);
+	buf = phy_read(pdata->phydev, SPECIAL_CTRL_STS);
 	if (buf & SPECIAL_CTRL_STS_OVRRD_AMDIX_) {
 		if (buf & SPECIAL_CTRL_STS_AMDIX_ENABLE_)
 			return ETH_TP_MDI_AUTO;
@@ -793,7 +794,7 @@  static void set_mdix_status(struct net_device *net, __u8 mdix_ctrl)
 	    (pdata->chip_id == ID_REV_CHIP_ID_89530_) ||
 	    (pdata->chip_id == ID_REV_CHIP_ID_9730_)) {
 		/* Extend Manual AutoMDIX timer for 9500A/9500Ai */
-		buf = smsc95xx_mdio_read(dev->net, dev->mii.phy_id,
+		buf = phy_read(pdata->phydev,
 					 PHY_EDPD_CONFIG);
 		buf |= PHY_EDPD_CONFIG_EXT_CROSSOVER_;
 		smsc95xx_mdio_write(dev->net, dev->mii.phy_id,
@@ -801,7 +802,7 @@  static void set_mdix_status(struct net_device *net, __u8 mdix_ctrl)
 	}
 
 	if (mdix_ctrl == ETH_TP_MDI) {
-		buf = smsc95xx_mdio_read(dev->net, dev->mii.phy_id,
+		buf = phy_read(pdata->phydev,
 					 SPECIAL_CTRL_STS);
 		buf |= SPECIAL_CTRL_STS_OVRRD_AMDIX_;
 		buf &= ~(SPECIAL_CTRL_STS_AMDIX_ENABLE_ |
@@ -809,7 +810,7 @@  static void set_mdix_status(struct net_device *net, __u8 mdix_ctrl)
 		smsc95xx_mdio_write(dev->net, dev->mii.phy_id,
 				    SPECIAL_CTRL_STS, buf);
 	} else if (mdix_ctrl == ETH_TP_MDI_X) {
-		buf = smsc95xx_mdio_read(dev->net, dev->mii.phy_id,
+		buf = phy_read(pdata->phydev,
 					 SPECIAL_CTRL_STS);
 		buf |= SPECIAL_CTRL_STS_OVRRD_AMDIX_;
 		buf &= ~(SPECIAL_CTRL_STS_AMDIX_ENABLE_ |
@@ -818,7 +819,7 @@  static void set_mdix_status(struct net_device *net, __u8 mdix_ctrl)
 		smsc95xx_mdio_write(dev->net, dev->mii.phy_id,
 				    SPECIAL_CTRL_STS, buf);
 	} else if (mdix_ctrl == ETH_TP_MDI_AUTO) {
-		buf = smsc95xx_mdio_read(dev->net, dev->mii.phy_id,
+		buf = phy_read(pdata->phydev,
 					 SPECIAL_CTRL_STS);
 		buf &= ~SPECIAL_CTRL_STS_OVRRD_AMDIX_;
 		buf &= ~(SPECIAL_CTRL_STS_AMDIX_ENABLE_ |
@@ -968,6 +969,7 @@  static void smsc95xx_adjust_link(struct net_device *netdev)
 
 static int smsc95xx_phy_initialize(struct usbnet *dev)
 {
+	struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
 	int bmcr, ret, timeout = 0;
 
 	/* reset phy and wait for reset to complete */
@@ -975,7 +977,7 @@  static int smsc95xx_phy_initialize(struct usbnet *dev)
 
 	do {
 		msleep(10);
-		bmcr = smsc95xx_mdio_read(dev->net, dev->mii.phy_id, MII_BMCR);
+		bmcr = phy_read(pdata->phydev, MII_BMCR);
 		timeout++;
 	} while ((bmcr & BMCR_RESET) && (timeout < 100));
 
@@ -989,7 +991,7 @@  static int smsc95xx_phy_initialize(struct usbnet *dev)
 		ADVERTISE_PAUSE_ASYM);
 
 	/* read to clear */
-	ret = smsc95xx_mdio_read(dev->net, dev->mii.phy_id, PHY_INT_SRC);
+	ret = phy_read(pdata->phydev, PHY_INT_SRC);
 	if (ret < 0) {
 		netdev_warn(dev->net, "Failed to read PHY_INT_SRC during init\n");
 		return ret;
@@ -1407,18 +1409,19 @@  static u32 smsc_crc(const u8 *buffer, size_t len, int filter)
 
 static int smsc95xx_enable_phy_wakeup_interrupts(struct usbnet *dev, u16 mask)
 {
+	struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
 	struct mii_if_info *mii = &dev->mii;
 	int ret;
 
 	netdev_dbg(dev->net, "enabling PHY wakeup interrupts\n");
 
 	/* read to clear */
-	ret = smsc95xx_mdio_read(dev->net, mii->phy_id, PHY_INT_SRC);
+	ret = phy_read(pdata->phydev, PHY_INT_SRC);
 	if (ret < 0)
 		return ret;
 
 	/* enable interrupt source */
-	ret = smsc95xx_mdio_read(dev->net, mii->phy_id, PHY_INT_MASK);
+	ret = phy_read(pdata->phydev, PHY_INT_MASK);
 	if (ret < 0)
 		return ret;
 
@@ -1431,15 +1434,15 @@  static int smsc95xx_enable_phy_wakeup_interrupts(struct usbnet *dev, u16 mask)
 
 static int smsc95xx_link_ok_nopm(struct usbnet *dev)
 {
-	struct mii_if_info *mii = &dev->mii;
+	struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
 	int ret;
 
 	/* first, a dummy read, needed to latch some MII phys */
-	ret = smsc95xx_mdio_read(dev->net, mii->phy_id, MII_BMSR);
+	ret = phy_read(pdata->phydev, MII_BMSR);
 	if (ret < 0)
 		return ret;
 
-	ret = smsc95xx_mdio_read(dev->net, mii->phy_id, MII_BMSR);
+	ret = phy_read(pdata->phydev, MII_BMSR);
 	if (ret < 0)
 		return ret;
 
@@ -1500,7 +1503,7 @@  static int smsc95xx_enter_suspend1(struct usbnet *dev)
 				    PHY_EDPD_CONFIG_DEFAULT);
 
 	/* enable energy detect power-down mode */
-	ret = smsc95xx_mdio_read(dev->net, mii->phy_id, PHY_MODE_CTRL_STS);
+	ret = phy_read(pdata->phydev, PHY_MODE_CTRL_STS);
 	if (ret < 0)
 		return ret;