| Message ID | 20240930153423.16893-1-divya.koppera@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: microchip_t1: Interrupt support for lan887x | expand |
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 > >
> +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
Hi Andrew, Thanks for the review comments, I will apply it in next revision. /Divya > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Monday, September 30, 2024 10:14 PM > To: Divya Koppera - I30481 <Divya.Koppera@microchip.com> > Cc: Arun Ramadoss - I17769 <Arun.Ramadoss@microchip.com>; > UNGLinuxDriver <UNGLinuxDriver@microchip.com>; hkallweit1@gmail.com; > linux@armlinux.org.uk; davem@davemloft.net; edumazet@google.com; > kuba@kernel.org; pabeni@redhat.com; netdev@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH net-next] net: phy: microchip_t1: Interrupt support for > lan887x > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > > +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
Hi @Kalesh Anakkur Purayil, Thanks for the review comments, I will apply it in next revision. /Divya > -----Original Message----- > From: Kalesh Anakkur Purayil <kalesh-anakkur.purayil@broadcom.com> > Sent: Monday, September 30, 2024 9:49 PM > To: Divya Koppera - I30481 <Divya.Koppera@microchip.com> > Cc: Arun Ramadoss - I17769 <Arun.Ramadoss@microchip.com>; > UNGLinuxDriver <UNGLinuxDriver@microchip.com>; andrew@lunn.ch; > hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH net-next] net: phy: microchip_t1: Interrupt support for > lan887x > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe
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, > } > }; >
Hi Parthiban, Thanks for the review comments. Please find my reply inline. > -----Original Message----- > From: Parthiban Veerasooran - I17164 > <Parthiban.Veerasooran@microchip.com> > Sent: Tuesday, October 1, 2024 5:28 PM > To: Divya Koppera - I30481 <Divya.Koppera@microchip.com> > Cc: Arun Ramadoss - I17769 <Arun.Ramadoss@microchip.com>; > UNGLinuxDriver <UNGLinuxDriver@microchip.com>; andrew@lunn.ch; > hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH net-next] net: phy: microchip_t1: Interrupt support for > lan887x > > 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 This is declared for future series purpose. As suggested, I'll remove this variable now and introduce in next series. /Divya > > + 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, > > } > > }; > >
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, } };
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(+)