On Tue, Aug 13, 2024 at 10:24:52PM +0000, Tristram.Ha@xxxxxxxxxxxxx wrote:
> > > +static irqreturn_t ksz9477_handle_port_irq(struct ksz_device *dev, u8 port,
> > > + u8 *data)
> > > +{
> > > + struct dsa_switch *ds = dev->ds;
> > > + struct phy_device *phydev;
> > > + int cnt = 0;
> > > +
> > > + phydev = mdiobus_get_phy(ds->user_mii_bus, port);
> > > + if (*data & PORT_PHY_INT) {
> > > + /* Handle the interrupt if there is no PHY device or its
> > > + * interrupt is not registered yet.
> > > + */
> > > + if (!phydev || phydev->interrupts != PHY_INTERRUPT_ENABLED) {
> > > + u8 phy_status;
> > > +
> > > + ksz_pread8(dev, port, REG_PORT_PHY_INT_STATUS,
> > > + &phy_status);
> > > + if (phydev)
> > > + phy_trigger_machine(phydev);
> > > + ++cnt;
> > > + *data &= ~PORT_PHY_INT;
> > > + }
> > > + }
> >
> > This looks like a layering violation. Why is this needed? An interrupt
> > controller generally has no idea what the individual interrupt is
> > about. It just calls into the interrupt core to get the handler
> > called, and then clears the interrupt. Why does that not work here?
> >
> > What other DSA drivers do if they need to handle some of the
> > interrupts is just request the interrupt like any other driver:
> >
> > https://elixir.bootlin.com/linux/v6.10.3/source/drivers/net/dsa/mv88e6xxx/pcs-
> > 639x.c#L95
>
> The PHY and ACL interrupt handling can be removed, but the SGMII
> interrupt handling cannot as the SGMII port is simulated as having an
> internal PHY but the regular PHY interrupt processing will not clear the
> interrupt.
>
> Furthermore, there will be a situation where the SGMII interrupt is
> triggered before the PHY interrupt handling function is registered.
This is one of the reasons i suggested a PCS driver. Look at
drivers/net/dsa/mv88e6xxx/pcs-6185.c as an example, how it handles
interrupts from the PCS. And it is a similar setup, the switch has an
interrupt controller, and the PCS driver requests the interrupt. PCS
drivers do not need to be complex. pcs-6185.c has an empty AN restart
callback, pcs_config does nothing, etc.
Andrew
