Hi Andrew,
> > +static int mlxbf_gige_phy_enable_interrupt(struct phy_device *phydev)
> > +{
> > + int err = 0;
> > +
> > + if (phydev->drv->ack_interrupt)
> > + err = phydev->drv->ack_interrupt(phydev);
> > + if (err < 0)
> > + return err;
> > +
> > + phydev->interrupts = PHY_INTERRUPT_ENABLED;
> > + if (phydev->drv->config_intr)
> > + err = phydev->drv->config_intr(phydev);
> > +
> > + return err;
> > +}
We actually just need to make sure here, the PHY INT register is still
configured properly. mlxbf_gige_phy_enable_interrupt is called right after
phy_start because the PHY INT controller/status register gets cleared out after
phy_start due to the call to __phy_resume. Because it gets cleared, the PHY no
longer reports any interrupts.
> > +
> > +static int mlxbf_gige_phy_disable_interrupt(struct phy_device
> > +*phydev) {
> > + int err = 0;
> > +
> > + if (phydev->drv->ack_interrupt)
> > + err = phydev->drv->ack_interrupt(phydev);
> > + if (err < 0)
> > + return err;
> > +
> > + phydev->interrupts = PHY_INTERRUPT_DISABLED;
> > + if (phydev->drv->config_intr)
> > + err = phydev->drv->config_intr(phydev);
> > +
> > + return err;
> > +}
>
We need to call mlxbf_gige_phy_disable_interrupt from the stop routine because
if we leave it enabled, and decide to call the ndo_open routine after, the
interrupt coming from the PHY will be discarded until the PHY state machine is
setup properly (via phy_start).
> This is, erm, interesting.
>
> > +irqreturn_t mlxbf_gige_mdio_handle_phy_interrupt(int irq, void
> > +*dev_id) {
> > + struct phy_device *phydev;
> > + struct mlxbf_gige *priv;
> > + u32 val;
> > +
> > + priv = dev_id;
> > + phydev = priv->netdev->phydev;
> > +
> > + /* Check if this interrupt is from PHY device.
> > + * Return if it is not.
> > + */
> > + val = readl(priv->gpio_io +
> > + MLXBF_GIGE_GPIO_CAUSE_OR_CAUSE_EVTEN0);
> > + if (!(val & MLXBF_GIGE_CAUSE_OR_CAUSE_EVTEN0_MASK))
> > + return IRQ_NONE;
> > +
> > + phy_mac_interrupt(phydev);
> > +
> > + /* Clear interrupt when done, otherwise, no further interrupt
> > + * will be triggered.
> > + */
> > + val = readl(priv->gpio_io +
> > + MLXBF_GIGE_GPIO_CAUSE_OR_CLRCAUSE);
> > + val |= MLXBF_GIGE_CAUSE_OR_CLRCAUSE_MASK;
> > + writel(val, priv->gpio_io +
> > + MLXBF_GIGE_GPIO_CAUSE_OR_CLRCAUSE);
> > +
> > + /* Make sure to clear the PHY device interrupt */
> > + if (phydev->drv->ack_interrupt)
> > + phydev->drv->ack_interrupt(phydev);
> > +
> > + phydev->interrupts = PHY_INTERRUPT_ENABLED;
> > + if (phydev->drv->config_intr)
> > + phydev->drv->config_intr(phydev);
>
Yes mlxbf_gige_mdio_handle_phy_interrupt is used to check whether the interrupt
is coming from GPIO12 (which is set in HW as the PHY INT_N pin). There is one
HW interrupt line (here defined as MLXBF_GIGE_PHY_INT_N) shared among all the
GPIOs and other components (like I2C).
Now you mentioned that we should not be copying core PHY code. Ack_interrupt
needs to be called here to make sure we clear the interrupt after clearing the
Mellanox HW interrupt MLXBF_GIGE_PHY_INT_N. and config_intr needs to be called
because phy_mac_interrupt causes the PHY device to clear the PHY INT enable
bits.
> And more interesting code.
>
> We have to find a better way to do this, you should not by copying core PHY
> code into a MAC driver.
>
> So it seems to me, the PHY interrupt you request is not actually a PHY
> interrupt. It looks more like an interrupt controller. The EVTEN0 suggests
> that
> there could be multiple interrupts here, of which one if the PHY? This is more
> a generic GPIO block which can do interrupts when the pins are configured as
> inputs? Or is it an interrupt controller with multiple interrupts?
>
> Once you model this correctly in Linux, you can probably remove all the
> interesting code.
>
> Andrew
