[v1] net: phy: microchip_t1: Interrupt support for lan887x

[PATCH net-next] net: phy: microchip_t1: Interrupt support for lan887x

Posted by Divya Koppera 1 month, 4 weeks ago

Add support for link up and link down interrupts in lan887x.

Signed-off-by: Divya Koppera <divya.koppera@microchip.com>
---
 drivers/net/phy/microchip_t1.c | 63 ++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/net/phy/microchip_t1.c b/drivers/net/phy/microchip_t1.c
index a5ef8fe50704..383050a5b0ed 100644
--- a/drivers/net/phy/microchip_t1.c
+++ b/drivers/net/phy/microchip_t1.c
@@ -226,6 +226,18 @@
 #define MICROCHIP_CABLE_MAX_TIME_DIFF	\
 	(MICROCHIP_CABLE_MIN_TIME_DIFF + MICROCHIP_CABLE_TIME_MARGIN)
 
+#define LAN887X_INT_STS				0xf000
+#define LAN887X_INT_MSK				0xf001
+#define LAN887X_INT_MSK_T1_PHY_INT_MSK		BIT(2)
+#define LAN887X_INT_MSK_LINK_UP_MSK		BIT(1)
+#define LAN887X_INT_MSK_LINK_DOWN_MSK		BIT(0)
+
+#define LAN887X_MX_CHIP_TOP_LINK_MSK	(LAN887X_INT_MSK_LINK_UP_MSK |\
+					 LAN887X_INT_MSK_LINK_DOWN_MSK)
+
+#define LAN887X_MX_CHIP_TOP_ALL_MSK	(LAN887X_INT_MSK_T1_PHY_INT_MSK |\
+					 LAN887X_MX_CHIP_TOP_LINK_MSK)
+
 #define DRIVER_AUTHOR	"Nisar Sayed <nisar.sayed@microchip.com>"
 #define DRIVER_DESC	"Microchip LAN87XX/LAN937x/LAN887x T1 PHY driver"
 
@@ -1474,6 +1486,7 @@ static void lan887x_get_strings(struct phy_device *phydev, u8 *data)
 		ethtool_puts(&data, lan887x_hw_stats[i].string);
 }
 
+static int lan887x_config_intr(struct phy_device *phydev);
 static int lan887x_cd_reset(struct phy_device *phydev,
 			    enum cable_diag_state cd_done)
 {
@@ -1504,6 +1517,10 @@ static int lan887x_cd_reset(struct phy_device *phydev,
 		if (rc < 0)
 			return rc;
 
+		rc = lan887x_config_intr(phydev);
+		if (rc < 0)
+			return rc;
+
 		rc = lan887x_phy_reconfig(phydev);
 		if (rc < 0)
 			return rc;
@@ -1830,6 +1847,50 @@ static int lan887x_cable_test_get_status(struct phy_device *phydev,
 	return lan887x_cable_test_report(phydev);
 }
 
+static int lan887x_config_intr(struct phy_device *phydev)
+{
+	int ret;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		/* Clear the interrupt status before enabling interrupts */
+		ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, LAN887X_INT_STS);
+		if (ret < 0)
+			return ret;
+
+		/* Unmask for enabling interrupt */
+		ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, LAN887X_INT_MSK,
+				    (u16)~LAN887X_MX_CHIP_TOP_ALL_MSK);
+	} else {
+		ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, LAN887X_INT_MSK,
+				    GENMASK(15, 0));
+		if (ret < 0)
+			return ret;
+
+		ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, LAN887X_INT_STS);
+	}
+
+	return ret < 0 ? ret : 0;
+}
+
+static irqreturn_t lan887x_handle_interrupt(struct phy_device *phydev)
+{
+	int ret = IRQ_NONE;
+	int irq_status;
+
+	irq_status = phy_read_mmd(phydev, MDIO_MMD_VEND1, LAN887X_INT_STS);
+	if (irq_status < 0) {
+		phy_error(phydev);
+		return IRQ_NONE;
+	}
+
+	if (irq_status & LAN887X_MX_CHIP_TOP_LINK_MSK) {
+		phy_trigger_machine(phydev);
+		ret = IRQ_HANDLED;
+	}
+
+	return ret;
+}
+
 static struct phy_driver microchip_t1_phy_driver[] = {
 	{
 		PHY_ID_MATCH_MODEL(PHY_ID_LAN87XX),
@@ -1881,6 +1942,8 @@ static struct phy_driver microchip_t1_phy_driver[] = {
 		.read_status	= genphy_c45_read_status,
 		.cable_test_start = lan887x_cable_test_start,
 		.cable_test_get_status = lan887x_cable_test_get_status,
+		.config_intr    = lan887x_config_intr,
+		.handle_interrupt = lan887x_handle_interrupt,
 	}
 };
 
-- 
2.17.1

Re: [PATCH net-next] net: phy: microchip_t1: Interrupt support for lan887x

Posted by Parthiban.Veerasooran@microchip.com 1 month, 4 weeks ago

Hi,

On 30/09/24 9:04 pm, Divya Koppera wrote:
> Add support for link up and link down interrupts in lan887x.
> 
> Signed-off-by: Divya Koppera <divya.koppera@microchip.com>
> ---
>   drivers/net/phy/microchip_t1.c | 63 ++++++++++++++++++++++++++++++++++
>   1 file changed, 63 insertions(+)
> 
> diff --git a/drivers/net/phy/microchip_t1.c b/drivers/net/phy/microchip_t1.c
> index a5ef8fe50704..383050a5b0ed 100644
> --- a/drivers/net/phy/microchip_t1.c
> +++ b/drivers/net/phy/microchip_t1.c
> @@ -226,6 +226,18 @@
>   #define MICROCHIP_CABLE_MAX_TIME_DIFF	\
>   	(MICROCHIP_CABLE_MIN_TIME_DIFF + MICROCHIP_CABLE_TIME_MARGIN)
>   
> +#define LAN887X_INT_STS				0xf000
> +#define LAN887X_INT_MSK				0xf001
> +#define LAN887X_INT_MSK_T1_PHY_INT_MSK		BIT(2)
> +#define LAN887X_INT_MSK_LINK_UP_MSK		BIT(1)
> +#define LAN887X_INT_MSK_LINK_DOWN_MSK		BIT(0)
> +
> +#define LAN887X_MX_CHIP_TOP_LINK_MSK	(LAN887X_INT_MSK_LINK_UP_MSK |\
> +					 LAN887X_INT_MSK_LINK_DOWN_MSK)
> +
> +#define LAN887X_MX_CHIP_TOP_ALL_MSK	(LAN887X_INT_MSK_T1_PHY_INT_MSK |\
> +					 LAN887X_MX_CHIP_TOP_LINK_MSK)
> +
>   #define DRIVER_AUTHOR	"Nisar Sayed <nisar.sayed@microchip.com>"
>   #define DRIVER_DESC	"Microchip LAN87XX/LAN937x/LAN887x T1 PHY driver"
>   
> @@ -1474,6 +1486,7 @@ static void lan887x_get_strings(struct phy_device *phydev, u8 *data)
>   		ethtool_puts(&data, lan887x_hw_stats[i].string);
>   }
>   
> +static int lan887x_config_intr(struct phy_device *phydev);
>   static int lan887x_cd_reset(struct phy_device *phydev,
>   			    enum cable_diag_state cd_done)
>   {
> @@ -1504,6 +1517,10 @@ static int lan887x_cd_reset(struct phy_device *phydev,
>   		if (rc < 0)
>   			return rc;
>   
> +		rc = lan887x_config_intr(phydev);
> +		if (rc < 0)
> +			return rc;
> +
>   		rc = lan887x_phy_reconfig(phydev);
>   		if (rc < 0)
>   			return rc;
> @@ -1830,6 +1847,50 @@ static int lan887x_cable_test_get_status(struct phy_device *phydev,
>   	return lan887x_cable_test_report(phydev);
>   }
>   
> +static int lan887x_config_intr(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> +		/* Clear the interrupt status before enabling interrupts */
> +		ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, LAN887X_INT_STS);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Unmask for enabling interrupt */
> +		ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, LAN887X_INT_MSK,
> +				    (u16)~LAN887X_MX_CHIP_TOP_ALL_MSK);
> +	} else {
> +		ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, LAN887X_INT_MSK,
> +				    GENMASK(15, 0));
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, LAN887X_INT_STS);
> +	}
> +
> +	return ret < 0 ? ret : 0;
> +}
> +
> +static irqreturn_t lan887x_handle_interrupt(struct phy_device *phydev)
> +{
> +	int ret = IRQ_NONE;
I think you can remove 'ret' by simply returning IRQ_HANDLED and 
IRQ_NONE directly to save one line.

Best regards,
Parthiban V
> +	int irq_status;
> +
> +	irq_status = phy_read_mmd(phydev, MDIO_MMD_VEND1, LAN887X_INT_STS);
> +	if (irq_status < 0) {
> +		phy_error(phydev);
> +		return IRQ_NONE;
> +	}
> +
> +	if (irq_status & LAN887X_MX_CHIP_TOP_LINK_MSK) {
> +		phy_trigger_machine(phydev);
> +		ret = IRQ_HANDLED;
> +	}
> +
> +	return ret;
> +}
> +
>   static struct phy_driver microchip_t1_phy_driver[] = {
>   	{
>   		PHY_ID_MATCH_MODEL(PHY_ID_LAN87XX),
> @@ -1881,6 +1942,8 @@ static struct phy_driver microchip_t1_phy_driver[] = {
>   		.read_status	= genphy_c45_read_status,
>   		.cable_test_start = lan887x_cable_test_start,
>   		.cable_test_get_status = lan887x_cable_test_get_status,
> +		.config_intr    = lan887x_config_intr,
> +		.handle_interrupt = lan887x_handle_interrupt,
>   	}
>   };
>

Re: [PATCH net-next] net: phy: microchip_t1: Interrupt support for lan887x

Posted by Andrew Lunn 1 month, 4 weeks ago

> +static int lan887x_config_intr(struct phy_device *phydev)
> +{
> +	int ret;

This driver consistently uses rc, not ret. It would be good to keep
with the existing convention.

    Andrew

---
pw-bot: cr

Re: [PATCH net-next] net: phy: microchip_t1: Interrupt support for lan887x

Posted by Kalesh Anakkur Purayil 1 month, 4 weeks ago

On Mon, Sep 30, 2024 at 9:02 PM Divya Koppera
<divya.koppera@microchip.com> wrote:
>
> Add support for link up and link down interrupts in lan887x.
>
> Signed-off-by: Divya Koppera <divya.koppera@microchip.com>
> ---
>  drivers/net/phy/microchip_t1.c | 63 ++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>
> diff --git a/drivers/net/phy/microchip_t1.c b/drivers/net/phy/microchip_t1.c
> index a5ef8fe50704..383050a5b0ed 100644
> --- a/drivers/net/phy/microchip_t1.c
> +++ b/drivers/net/phy/microchip_t1.c
> @@ -226,6 +226,18 @@
>  #define MICROCHIP_CABLE_MAX_TIME_DIFF  \
>         (MICROCHIP_CABLE_MIN_TIME_DIFF + MICROCHIP_CABLE_TIME_MARGIN)
>
> +#define LAN887X_INT_STS                                0xf000
> +#define LAN887X_INT_MSK                                0xf001
> +#define LAN887X_INT_MSK_T1_PHY_INT_MSK         BIT(2)
> +#define LAN887X_INT_MSK_LINK_UP_MSK            BIT(1)
> +#define LAN887X_INT_MSK_LINK_DOWN_MSK          BIT(0)
> +
> +#define LAN887X_MX_CHIP_TOP_LINK_MSK   (LAN887X_INT_MSK_LINK_UP_MSK |\
> +                                        LAN887X_INT_MSK_LINK_DOWN_MSK)
> +
> +#define LAN887X_MX_CHIP_TOP_ALL_MSK    (LAN887X_INT_MSK_T1_PHY_INT_MSK |\
> +                                        LAN887X_MX_CHIP_TOP_LINK_MSK)
> +
>  #define DRIVER_AUTHOR  "Nisar Sayed <nisar.sayed@microchip.com>"
>  #define DRIVER_DESC    "Microchip LAN87XX/LAN937x/LAN887x T1 PHY driver"
>
> @@ -1474,6 +1486,7 @@ static void lan887x_get_strings(struct phy_device *phydev, u8 *data)
>                 ethtool_puts(&data, lan887x_hw_stats[i].string);
>  }
>
> +static int lan887x_config_intr(struct phy_device *phydev);
[Kalesh] I would suggest you to avoid this forward declaration by
moving the function definition here.

>  static int lan887x_cd_reset(struct phy_device *phydev,
>                             enum cable_diag_state cd_done)
>  {
> @@ -1504,6 +1517,10 @@ static int lan887x_cd_reset(struct phy_device *phydev,
>                 if (rc < 0)
>                         return rc;
>
> +               rc = lan887x_config_intr(phydev);
> +               if (rc < 0)
> +                       return rc;
> +
>                 rc = lan887x_phy_reconfig(phydev);
>                 if (rc < 0)
>                         return rc;
> @@ -1830,6 +1847,50 @@ static int lan887x_cable_test_get_status(struct phy_device *phydev,
>         return lan887x_cable_test_report(phydev);
>  }
>
> +static int lan887x_config_intr(struct phy_device *phydev)
> +{
> +       int ret;
> +
> +       if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> +               /* Clear the interrupt status before enabling interrupts */
> +               ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, LAN887X_INT_STS);
> +               if (ret < 0)
> +                       return ret;
> +
> +               /* Unmask for enabling interrupt */
> +               ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, LAN887X_INT_MSK,
> +                                   (u16)~LAN887X_MX_CHIP_TOP_ALL_MSK);
> +       } else {
> +               ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, LAN887X_INT_MSK,
> +                                   GENMASK(15, 0));
> +               if (ret < 0)
> +                       return ret;
> +
> +               ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, LAN887X_INT_STS);
> +       }
> +
> +       return ret < 0 ? ret : 0;
> +}
> +
> +static irqreturn_t lan887x_handle_interrupt(struct phy_device *phydev)
> +{
> +       int ret = IRQ_NONE;
> +       int irq_status;
> +
> +       irq_status = phy_read_mmd(phydev, MDIO_MMD_VEND1, LAN887X_INT_STS);
> +       if (irq_status < 0) {
> +               phy_error(phydev);
> +               return IRQ_NONE;
> +       }
> +
> +       if (irq_status & LAN887X_MX_CHIP_TOP_LINK_MSK) {
> +               phy_trigger_machine(phydev);
> +               ret = IRQ_HANDLED;
> +       }
> +
> +       return ret;
> +}
> +
>  static struct phy_driver microchip_t1_phy_driver[] = {
>         {
>                 PHY_ID_MATCH_MODEL(PHY_ID_LAN87XX),
> @@ -1881,6 +1942,8 @@ static struct phy_driver microchip_t1_phy_driver[] = {
>                 .read_status    = genphy_c45_read_status,
>                 .cable_test_start = lan887x_cable_test_start,
>                 .cable_test_get_status = lan887x_cable_test_get_status,
> +               .config_intr    = lan887x_config_intr,
> +               .handle_interrupt = lan887x_handle_interrupt,
>         }
>  };
>
> --
> 2.17.1
>
>


-- 
Regards,
Kalesh A P