| Message ID | 20240809233840.59953-4-Tristram.Ha@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: microchip: add SGMII port support to KSZ9477 switch | expand |
> -----Original Message----- > From: Tristram.Ha@microchip.com <Tristram.Ha@microchip.com> > Sent: Saturday, August 10, 2024 5:09 AM > To: Woojung Huh <woojung.huh@microchip.com>; > UNGLinuxDriver@microchip.com; devicetree@vger.kernel.org; Andrew Lunn > <andrew@lunn.ch>; Florian Fainelli <f.fainelli@gmail.com>; Vladimir Oltean > <olteanv@gmail.com>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski > <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org> > Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni > <pabeni@redhat.com>; Marek Vasut <marex@denx.de>; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Tristram Ha > <tristram.ha@microchip.com> > Subject: [PATCH net-next 3/4] net: dsa: microchip: handle most > interrupts in KSZ9477/KSZ9893 switch families > > From: Tristram Ha <tristram. ha@ microchip. com> The KSZ9477 switch driver > can handle most interrupts. It enables address learning fail interrupt as SQA > would like to see such notification during testing. Input timestamp interrupt is > not > From: Tristram Ha <tristram.ha@microchip.com> > > The KSZ9477 switch driver can handle most interrupts. It enables address > learning fail interrupt as SQA would like to see such notification during > testing. > > Input timestamp interrupt is not implemented yet as that interrupt is related > to PTP operation and so will be handled by the PTP driver. > > Signed-off-by: Tristram Ha <tristram.ha@microchip.com> > --- > drivers/net/dsa/microchip/ksz9477.c | 64 ++++++++++++++++++++++++- > drivers/net/dsa/microchip/ksz9477.h | 4 +- > drivers/net/dsa/microchip/ksz9477_reg.h | 5 +- > drivers/net/dsa/microchip/ksz_common.c | 2 + > 4 files changed, 71 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/dsa/microchip/ksz9477.c > b/drivers/net/dsa/microchip/ksz9477.c > index 425e20daf1e9..518ba4a1e34b 100644 > --- a/drivers/net/dsa/microchip/ksz9477.c > +++ b/drivers/net/dsa/microchip/ksz9477.c > @@ -2,7 +2,7 @@ > /* > * Microchip KSZ9477 switch driver main logic > * > - * Copyright (C) 2017-2019 Microchip Technology Inc. > + * Copyright (C) 2017-2024 Microchip Technology Inc. > */ > > #include <linux/kernel.h> > @@ -1487,6 +1487,68 @@ void ksz9477_switch_exit(struct ksz_device *dev) > ksz9477_reset_switch(dev); > } > > +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; > + } > + } > + if (*data & PORT_ACL_INT) { > + ksz_pwrite8(dev, port, REG_PORT_INT_STATUS, > PORT_ACL_INT); > + ++cnt; > + *data &= ~PORT_ACL_INT; > + } > + > + return (cnt > 0) ? IRQ_HANDLED : IRQ_NONE; } Until unless there is a need to service both PHY, ACL interrupts simultaneously, "cnt" increment operations can be avoided like this to reduce processing time. if (*data & PORT_PHY_INT) { // Handle PORT_PHY_INT return IRQ_HANDLED; } if (*data & PORT_ACL_INT) { // Handle PORT_ACL_INT return IRQ_HANDLED; } return IRQ_NONE; > + > +void ksz9477_enable_irq(struct ksz_device *dev) { > + regmap_update_bits(ksz_regmap_32(dev), REG_SW_INT_MASK__4, > LUE_INT, 0); > + ksz_write8(dev, REG_SW_LUE_INT_ENABLE, LEARN_FAIL_INT | > +WRITE_FAIL_INT); } > + > +irqreturn_t ksz9477_handle_irq(struct ksz_device *dev, u8 port, u8 > +*data) { > + irqreturn_t ret = IRQ_NONE; > + u32 data32; > + > + if (port > 0) > + return ksz9477_handle_port_irq(dev, port - 1, data); > + > + ksz_read32(dev, REG_SW_INT_STATUS__4, &data32); > + if (data32 & LUE_INT) { > + u8 lue; > + > + ksz_read8(dev, REG_SW_LUE_INT_STATUS, &lue); > + ksz_write8(dev, REG_SW_LUE_INT_STATUS, lue); > + if (lue & LEARN_FAIL_INT) > + dev_info_ratelimited(dev->dev, "lue learn fail\n"); > + if (lue & WRITE_FAIL_INT) > + dev_info_ratelimited(dev->dev, "lue write fail\n"); > + ret = IRQ_HANDLED; > + } > + > + return ret; > +} > + > MODULE_AUTHOR("Woojung Huh <Woojung.Huh@microchip.com>"); > MODULE_DESCRIPTION("Microchip KSZ9477 Series Switch DSA Driver"); > MODULE_LICENSE("GPL"); diff --git a/drivers/net/dsa/microchip/ksz9477.h > b/drivers/net/dsa/microchip/ksz9477.h > index 239a281da10b..51252d0d0774 100644 > --- a/drivers/net/dsa/microchip/ksz9477.h > +++ b/drivers/net/dsa/microchip/ksz9477.h > @@ -2,7 +2,7 @@ > /* > * Microchip KSZ9477 series Header file > * > - * Copyright (C) 2017-2022 Microchip Technology Inc. > + * Copyright (C) 2017-2024 Microchip Technology Inc. > */ > > #ifndef __KSZ9477_H > @@ -58,6 +58,8 @@ int ksz9477_reset_switch(struct ksz_device *dev); int > ksz9477_switch_init(struct ksz_device *dev); void ksz9477_switch_exit(struct > ksz_device *dev); void ksz9477_port_queue_split(struct ksz_device *dev, int > port); > +void ksz9477_enable_irq(struct ksz_device *dev); irqreturn_t > +ksz9477_handle_irq(struct ksz_device *dev, u8 port, u8 *data); > void ksz9477_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr); > void ksz9477_hsr_leave(struct dsa_switch *ds, int port, struct net_device > *hsr); void ksz9477_get_wol(struct ksz_device *dev, int port, diff --git > a/drivers/net/dsa/microchip/ksz9477_reg.h > b/drivers/net/dsa/microchip/ksz9477_reg.h > index d5354c600ea1..da4ef3eb97c7 100644 > --- a/drivers/net/dsa/microchip/ksz9477_reg.h > +++ b/drivers/net/dsa/microchip/ksz9477_reg.h > @@ -2,7 +2,7 @@ > /* > * Microchip KSZ9477 register definitions > * > - * Copyright (C) 2017-2018 Microchip Technology Inc. > + * Copyright (C) 2017-2024 Microchip Technology Inc. > */ > > #ifndef __KSZ9477_REGS_H > @@ -75,7 +75,8 @@ > #define TRIG_TS_INT BIT(30) > #define APB_TIMEOUT_INT BIT(29) > > -#define SWITCH_INT_MASK (TRIG_TS_INT | > APB_TIMEOUT_INT) > +#define SWITCH_INT_MASK \ > + (LUE_INT | TRIG_TS_INT | APB_TIMEOUT_INT) > > #define REG_SW_PORT_INT_STATUS__4 0x0018 > #define REG_SW_PORT_INT_MASK__4 0x001C > diff --git a/drivers/net/dsa/microchip/ksz_common.c > b/drivers/net/dsa/microchip/ksz_common.c > index f328c97f27d1..7db74e036c3f 100644 > --- a/drivers/net/dsa/microchip/ksz_common.c > +++ b/drivers/net/dsa/microchip/ksz_common.c > @@ -357,6 +357,8 @@ static const struct ksz_dev_ops ksz9477_dev_ops = { > .reset = ksz9477_reset_switch, > .init = ksz9477_switch_init, > .exit = ksz9477_switch_exit, > + .enable_irq = ksz9477_enable_irq, > + .handle_irq = ksz9477_handle_irq, > }; > > static const struct phylink_mac_ops lan937x_phylink_mac_ops = { > -- > 2.34.1 >
> +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 > +irqreturn_t ksz9477_handle_irq(struct ksz_device *dev, u8 port, u8 *data) > +{ > + irqreturn_t ret = IRQ_NONE; > + u32 data32; > + > + if (port > 0) > + return ksz9477_handle_port_irq(dev, port - 1, data); > + > + ksz_read32(dev, REG_SW_INT_STATUS__4, &data32); > + if (data32 & LUE_INT) { > + u8 lue; > + > + ksz_read8(dev, REG_SW_LUE_INT_STATUS, &lue); > + ksz_write8(dev, REG_SW_LUE_INT_STATUS, lue); > + if (lue & LEARN_FAIL_INT) > + dev_info_ratelimited(dev->dev, "lue learn fail\n"); > + if (lue & WRITE_FAIL_INT) > + dev_info_ratelimited(dev->dev, "lue write fail\n"); > + ret = IRQ_HANDLED; > + } https://elixir.bootlin.com/linux/v6.10.3/source/drivers/net/dsa/mv88e6xxx/global1_atu.c#L474 Andrew
> > +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.
On Tue, Aug 13, 2024 at 10:24:52PM +0000, Tristram.Ha@microchip.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
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c index 425e20daf1e9..518ba4a1e34b 100644 --- a/drivers/net/dsa/microchip/ksz9477.c +++ b/drivers/net/dsa/microchip/ksz9477.c @@ -2,7 +2,7 @@ /* * Microchip KSZ9477 switch driver main logic * - * Copyright (C) 2017-2019 Microchip Technology Inc. + * Copyright (C) 2017-2024 Microchip Technology Inc. */ #include <linux/kernel.h> @@ -1487,6 +1487,68 @@ void ksz9477_switch_exit(struct ksz_device *dev) ksz9477_reset_switch(dev); } +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; + } + } + if (*data & PORT_ACL_INT) { + ksz_pwrite8(dev, port, REG_PORT_INT_STATUS, PORT_ACL_INT); + ++cnt; + *data &= ~PORT_ACL_INT; + } + + return (cnt > 0) ? IRQ_HANDLED : IRQ_NONE; +} + +void ksz9477_enable_irq(struct ksz_device *dev) +{ + regmap_update_bits(ksz_regmap_32(dev), REG_SW_INT_MASK__4, LUE_INT, 0); + ksz_write8(dev, REG_SW_LUE_INT_ENABLE, LEARN_FAIL_INT | WRITE_FAIL_INT); +} + +irqreturn_t ksz9477_handle_irq(struct ksz_device *dev, u8 port, u8 *data) +{ + irqreturn_t ret = IRQ_NONE; + u32 data32; + + if (port > 0) + return ksz9477_handle_port_irq(dev, port - 1, data); + + ksz_read32(dev, REG_SW_INT_STATUS__4, &data32); + if (data32 & LUE_INT) { + u8 lue; + + ksz_read8(dev, REG_SW_LUE_INT_STATUS, &lue); + ksz_write8(dev, REG_SW_LUE_INT_STATUS, lue); + if (lue & LEARN_FAIL_INT) + dev_info_ratelimited(dev->dev, "lue learn fail\n"); + if (lue & WRITE_FAIL_INT) + dev_info_ratelimited(dev->dev, "lue write fail\n"); + ret = IRQ_HANDLED; + } + + return ret; +} + MODULE_AUTHOR("Woojung Huh <Woojung.Huh@microchip.com>"); MODULE_DESCRIPTION("Microchip KSZ9477 Series Switch DSA Driver"); MODULE_LICENSE("GPL"); diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h index 239a281da10b..51252d0d0774 100644 --- a/drivers/net/dsa/microchip/ksz9477.h +++ b/drivers/net/dsa/microchip/ksz9477.h @@ -2,7 +2,7 @@ /* * Microchip KSZ9477 series Header file * - * Copyright (C) 2017-2022 Microchip Technology Inc. + * Copyright (C) 2017-2024 Microchip Technology Inc. */ #ifndef __KSZ9477_H @@ -58,6 +58,8 @@ int ksz9477_reset_switch(struct ksz_device *dev); int ksz9477_switch_init(struct ksz_device *dev); void ksz9477_switch_exit(struct ksz_device *dev); void ksz9477_port_queue_split(struct ksz_device *dev, int port); +void ksz9477_enable_irq(struct ksz_device *dev); +irqreturn_t ksz9477_handle_irq(struct ksz_device *dev, u8 port, u8 *data); void ksz9477_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr); void ksz9477_hsr_leave(struct dsa_switch *ds, int port, struct net_device *hsr); void ksz9477_get_wol(struct ksz_device *dev, int port, diff --git a/drivers/net/dsa/microchip/ksz9477_reg.h b/drivers/net/dsa/microchip/ksz9477_reg.h index d5354c600ea1..da4ef3eb97c7 100644 --- a/drivers/net/dsa/microchip/ksz9477_reg.h +++ b/drivers/net/dsa/microchip/ksz9477_reg.h @@ -2,7 +2,7 @@ /* * Microchip KSZ9477 register definitions * - * Copyright (C) 2017-2018 Microchip Technology Inc. + * Copyright (C) 2017-2024 Microchip Technology Inc. */ #ifndef __KSZ9477_REGS_H @@ -75,7 +75,8 @@ #define TRIG_TS_INT BIT(30) #define APB_TIMEOUT_INT BIT(29) -#define SWITCH_INT_MASK (TRIG_TS_INT | APB_TIMEOUT_INT) +#define SWITCH_INT_MASK \ + (LUE_INT | TRIG_TS_INT | APB_TIMEOUT_INT) #define REG_SW_PORT_INT_STATUS__4 0x0018 #define REG_SW_PORT_INT_MASK__4 0x001C diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index f328c97f27d1..7db74e036c3f 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -357,6 +357,8 @@ static const struct ksz_dev_ops ksz9477_dev_ops = { .reset = ksz9477_reset_switch, .init = ksz9477_switch_init, .exit = ksz9477_switch_exit, + .enable_irq = ksz9477_enable_irq, + .handle_irq = ksz9477_handle_irq, }; static const struct phylink_mac_ops lan937x_phylink_mac_ops = {