[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <736f67be-a21a-40ba-9eeb-d94040d56dc2@lunn.ch>
Date: Wed, 14 Aug 2024 15:55:52 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Tristram.Ha@...rochip.com
Cc: Woojung.Huh@...rochip.com, UNGLinuxDriver@...rochip.com,
devicetree@...r.kernel.org, f.fainelli@...il.com, olteanv@...il.com,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, marex@...x.de, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 3/4] net: dsa: microchip: handle most interrupts
in KSZ9477/KSZ9893 switch families
On Tue, Aug 13, 2024 at 10:24:52PM +0000, Tristram.Ha@...rochip.com 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
Powered by blists - more mailing lists