[PATCH] phy: nxp-c45-tja11xx: add interrupt support

[PATCH] phy: nxp-c45-tja11xx: add interrupt support

[Radu Pirea (NXP OSS)]

Added .config_intr and .handle_interrupt callbacks.

Link event interrupt will trigger an interrupt every time when the link
goes up or down.

Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
---
 drivers/net/phy/nxp-c45-tja11xx.c | 33 +++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c
index 95307097ebff..54eb74e46cbe 100644
--- a/drivers/net/phy/nxp-c45-tja11xx.c
+++ b/drivers/net/phy/nxp-c45-tja11xx.c
@@ -28,6 +28,11 @@
 #define DEVICE_CONTROL_CONFIG_GLOBAL_EN	BIT(14)
 #define DEVICE_CONTROL_CONFIG_ALL_EN	BIT(13)
 
+#define VEND1_PHY_IRQ_ACK		0x80A0
+#define VEND1_PHY_IRQ_EN		0x80A1
+#define VEND1_PHY_IRQ_STATUS		0x80A2
+#define PHY_IRQ_LINK_EVENT		BIT(1)
+
 #define VEND1_PHY_CONTROL		0x8100
 #define PHY_CONFIG_EN			BIT(14)
 #define PHY_START_OP			BIT(0)
@@ -188,6 +193,32 @@ static int nxp_c45_start_op(struct phy_device *phydev)
 				PHY_START_OP);
 }
 
+static int nxp_c45_config_intr(struct phy_device *phydev)
+{
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+		return phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
+					VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT);
+	else
+		return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1,
+					  VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT);
+}
+
+static irqreturn_t nxp_c45_handle_interrupt(struct phy_device *phydev)
+{
+	irqreturn_t ret = IRQ_NONE;
+	int irq;
+
+	irq = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_PHY_IRQ_STATUS);
+	if (irq & PHY_IRQ_LINK_EVENT) {
+		phy_trigger_machine(phydev);
+		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_PHY_IRQ_ACK,
+			      PHY_IRQ_LINK_EVENT);
+		ret = IRQ_HANDLED;
+	}
+
+	return ret;
+}
+
 static int nxp_c45_soft_reset(struct phy_device *phydev)
 {
 	int ret;
@@ -560,6 +591,8 @@ static struct phy_driver nxp_c45_driver[] = {
 		.soft_reset		= nxp_c45_soft_reset,
 		.config_aneg		= nxp_c45_config_aneg,
 		.config_init		= nxp_c45_config_init,
+		.config_intr		= nxp_c45_config_intr,
+		.handle_interrupt	= nxp_c45_handle_interrupt,
 		.read_status		= nxp_c45_read_status,
 		.suspend		= genphy_c45_pma_suspend,
 		.resume			= genphy_c45_pma_resume,

base-commit: cad4162a90aeff737a16c0286987f51e927f003a
-- 
2.31.1

Re: [PATCH] phy: nxp-c45-tja11xx: add interrupt support

[Andrew Lunn]

> +static irqreturn_t nxp_c45_handle_interrupt(struct phy_device *phydev)
> +{
> +	irqreturn_t ret = IRQ_NONE;
> +	int irq;
> +
> +	irq = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_PHY_IRQ_STATUS);
> +	if (irq & PHY_IRQ_LINK_EVENT) {
> +		phy_trigger_machine(phydev);
> +		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_PHY_IRQ_ACK,
> +			      PHY_IRQ_LINK_EVENT);

The ordering here is interesting. Could phy_trigger_machine() cause a
second interrupt? Which you then clear without acting upon before
exiting the interrupt handler? I think you should ACK the interrupt
before calling phy_trigger_machine().

       Andrew

Re: [PATCH] phy: nxp-c45-tja11xx: add interrupt support

[Vladimir Oltean]

On Fri, Apr 23, 2021 at 03:07:51PM +0200, Andrew Lunn wrote:
> > +static irqreturn_t nxp_c45_handle_interrupt(struct phy_device *phydev)
> > +{
> > +	irqreturn_t ret = IRQ_NONE;
> > +	int irq;
> > +
> > +	irq = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_PHY_IRQ_STATUS);
> > +	if (irq & PHY_IRQ_LINK_EVENT) {
> > +		phy_trigger_machine(phydev);
> > +		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_PHY_IRQ_ACK,
> > +			      PHY_IRQ_LINK_EVENT);
> 
> The ordering here is interesting. Could phy_trigger_machine() cause a
> second interrupt? Which you then clear without acting upon before
> exiting the interrupt handler? I think you should ACK the interrupt
> before calling phy_trigger_machine().

I thought that the irqchip driver keeps the interrupt line disabled
until the handler finishes, and that recursive interrupts aren't a thing
in Linux? Even with threaded interrupts, I thought this is what
IRQF_ONESHOT deals with.

Re: [PATCH] phy: nxp-c45-tja11xx: add interrupt support

[Vladimir Oltean]

On Fri, Apr 23, 2021 at 04:42:29PM +0300, Vladimir Oltean wrote:
> On Fri, Apr 23, 2021 at 03:07:51PM +0200, Andrew Lunn wrote:
> > > +static irqreturn_t nxp_c45_handle_interrupt(struct phy_device *phydev)
> > > +{
> > > +	irqreturn_t ret = IRQ_NONE;
> > > +	int irq;
> > > +
> > > +	irq = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_PHY_IRQ_STATUS);
> > > +	if (irq & PHY_IRQ_LINK_EVENT) {
> > > +		phy_trigger_machine(phydev);
> > > +		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_PHY_IRQ_ACK,
> > > +			      PHY_IRQ_LINK_EVENT);
> > 
> > The ordering here is interesting. Could phy_trigger_machine() cause a
> > second interrupt? Which you then clear without acting upon before
> > exiting the interrupt handler? I think you should ACK the interrupt
> > before calling phy_trigger_machine().
> 
> I thought that the irqchip driver keeps the interrupt line disabled
> until the handler finishes, and that recursive interrupts aren't a thing
> in Linux? Even with threaded interrupts, I thought this is what
> IRQF_ONESHOT deals with.

Ah, you mean the driver could be ACKing an event which was not the event
that triggered this IRQ. In that case, the ordering should be reversed,
sorry for the noise.

Re: [PATCH] phy: nxp-c45-tja11xx: add interrupt support

[Andrew Lunn]

> Ah, you mean the driver could be ACKing an event which was not the event
> that triggered this IRQ. In that case, the ordering should be reversed,
> sorry for the noise.

Yes, that is what i was meaning.

     Andrew