Re: [RFC v4 net-next 2/4] net: dsa: mt7530: add interrupt support
On Tue, Apr 13, 2021 at 02:52:59PM +0200, Andrew Lunn wrote: > > I guess this is depends whether the most usual case is to have all > > these interrupts being actively in use or not. Most interrupts only > > use a limited portion of their interrupt space at any given time. > > Allocating all interrupts and creating mappings upfront is a waste of > > memory. > > > > If the use case here is that all these interrupts will be wired and > > used in most cases, then upfront allocation is probably not a problem. > > Hi Marc > > The interrupts are generally used. Since this is an Ethernet switch, > generally the port is administratively up, even if there is no cable > plugged in. Once/if a cable is plugged in and there is a link peer, > the PHY will interrupt to indicate this. > > The only real case i can think of when the interrupts are not used is > when the switch has more ports than connected to the front panel. This > can happen in industrial settings, but not SOHO. Those ports which > don't go anywhere are never configured up and so the interrupt is > never used. Hi Andrew This is what the extra check (BIT(p) & ds->phys_mii_mask) avoids. Currently the mv88e6xxx driver does not have this check, and creates 15 PHY IRQ mappings on my 88E6176 unconditionally, leaving a gap in /proc/interrupts: ... 57: 0 0 mv88e6xxx-g1 3 Edge mv88e6xxx-f1072004.mdio-mii:00-g1-atu-prob 59: 0 0 mv88e6xxx-g1 5 Edge mv88e6xxx-f1072004.mdio-mii:00-g1-vtu-prob 61: 8 5 mv88e6xxx-g1 7 Edge mv88e6xxx-f1072004.mdio-mii:00-g2 63: 8 4 mv88e6xxx-g2 0 Edge mv88e6xxx-1:00 64: 0 0 mv88e6xxx-g2 1 Edge mv88e6xxx-1:01 65: 0 0 mv88e6xxx-g2 2 Edge mv88e6xxx-1:02 66: 0 0 mv88e6xxx-g2 3 Edge mv88e6xxx-1:03 67: 0 2 mv88e6xxx-g2 4 Edge mv88e6xxx-1:04 // IRQ 68~77 are created but not used 78: 0 0 mv88e6xxx-g2 15 Edge mv88e6xxx-f1072004.mdio-mii:00-watchdog ... You may as well add irq_set_nested_thread(irq, true) to irq_domain_map so all IRQs share a single thread. > > Andrew
Re: [RFC v4 net-next 2/4] net: dsa: mt7530: add interrupt support
> I guess this is depends whether the most usual case is to have all > these interrupts being actively in use or not. Most interrupts only > use a limited portion of their interrupt space at any given time. > Allocating all interrupts and creating mappings upfront is a waste of > memory. > > If the use case here is that all these interrupts will be wired and > used in most cases, then upfront allocation is probably not a problem. Hi Marc The interrupts are generally used. Since this is an Ethernet switch, generally the port is administratively up, even if there is no cable plugged in. Once/if a cable is plugged in and there is a link peer, the PHY will interrupt to indicate this. The only real case i can think of when the interrupts are not used is when the switch has more ports than connected to the front panel. This can happen in industrial settings, but not SOHO. Those ports which don't go anywhere are never configured up and so the interrupt is never used. Andrew
Re: [RFC v4 net-next 2/4] net: dsa: mt7530: add interrupt support
On Tue, 13 Apr 2021 01:07:23 +0100, Andrew Lunn wrote: > > > > > +static void > > > > +mt7530_setup_mdio_irq(struct mt7530_priv *priv) > > > > +{ > > > > + struct dsa_switch *ds = priv->ds; > > > > + int p; > > > > + > > > > + for (p = 0; p < MT7530_NUM_PHYS; p++) { > > > > + if (BIT(p) & ds->phys_mii_mask) { > > > > + unsigned int irq; > > > > + > > > > + irq = irq_create_mapping(priv->irq_domain, p); > > > > > > This seems odd. Why aren't the MDIO IRQs allocated on demand as > > > endpoint attached to this interrupt controller are being probed > > > individually? In general, doing this allocation upfront is an > > > indication that there is some missing information in the DT to perform > > > the discovery. > > > > This is what Andrew's mv88e6xxx does, actually. In addition, I also check > > the phys_mii_mask to avoid creating mappings for unused ports. > > It can be done via DT, using the standard interrupt property, so long > as you use of_mdiobus_register(np). > > But when you have an 7 port switch, and a nice simple mapping, port 0 > PHY using interrupt 0, you can save a lot of device tree boilerplate > by doing it in code. And when you have 4 of these switches, it gets > very boring adding all the DT to just wire up the interrupts 28 > interrupts. I guess this is depends whether the most usual case is to have all these interrupts being actively in use or not. Most interrupts only use a limited portion of their interrupt space at any given time. Allocating all interrupts and creating mappings upfront is a waste of memory. If the use case here is that all these interrupts will be wired and used in most cases, then upfront allocation is probably not a problem. M. -- Without deviation from the norm, progress is not possible.
Re: [RFC v4 net-next 2/4] net: dsa: mt7530: add interrupt support
> > > +static void > > > +mt7530_setup_mdio_irq(struct mt7530_priv *priv) > > > +{ > > > + struct dsa_switch *ds = priv->ds; > > > + int p; > > > + > > > + for (p = 0; p < MT7530_NUM_PHYS; p++) { > > > + if (BIT(p) & ds->phys_mii_mask) { > > > + unsigned int irq; > > > + > > > + irq = irq_create_mapping(priv->irq_domain, p); > > > > This seems odd. Why aren't the MDIO IRQs allocated on demand as > > endpoint attached to this interrupt controller are being probed > > individually? In general, doing this allocation upfront is an > > indication that there is some missing information in the DT to perform > > the discovery. > > This is what Andrew's mv88e6xxx does, actually. In addition, I also check > the phys_mii_mask to avoid creating mappings for unused ports. It can be done via DT, using the standard interrupt property, so long as you use of_mdiobus_register(np). But when you have an 7 port switch, and a nice simple mapping, port 0 PHY using interrupt 0, you can save a lot of device tree boilerplate by doing it in code. And when you have 4 of these switches, it gets very boring adding all the DT to just wire up the interrupts 28 interrupts. > Andrew, perhaps this can be done in DSA core? Not easily. It is not always a simple mapping like this. Two of the switches supported by mv88exxx offset the PHYs by 0x10. You really need the switch driver involved, with its detailed knowledge of the hardware. Andrew
Re: [RFC v4 net-next 2/4] net: dsa: mt7530: add interrupt support
On Mon, Apr 12, 2021 at 09:21:12AM +0100, Marc Zyngier wrote: > On Mon, 12 Apr 2021 04:42:35 +0100, > DENG Qingfang wrote: > > > > Add support for MT7530 interrupt controller to handle internal PHYs. > > In order to assign an IRQ number to each PHY, the registration of MDIO bus > > is also done in this driver. > > > > Signed-off-by: DENG Qingfang > > --- > > RFC v3 -> RFC v4: > > - No changes. > > > > drivers/net/dsa/Kconfig | 1 + > > drivers/net/dsa/mt7530.c | 266 +++ > > drivers/net/dsa/mt7530.h | 20 ++- > > 3 files changed, 258 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig > > index a5f1aa911fe2..264384449f09 100644 > > --- a/drivers/net/dsa/Kconfig > > +++ b/drivers/net/dsa/Kconfig > > @@ -36,6 +36,7 @@ config NET_DSA_LANTIQ_GSWIP > > config NET_DSA_MT7530 > > tristate "MediaTek MT753x and MT7621 Ethernet switch support" > > select NET_DSA_TAG_MTK > > + select MEDIATEK_PHY > > help > > This enables support for the MediaTek MT7530, MT7531, and MT7621 > > Ethernet switch chips. > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > > index 2bd1bab71497..da033004a74d 100644 > > --- a/drivers/net/dsa/mt7530.c > > +++ b/drivers/net/dsa/mt7530.c > > @@ -10,6 +10,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -596,18 +597,14 @@ mt7530_mib_reset(struct dsa_switch *ds) > > mt7530_write(priv, MT7530_MIB_CCR, CCR_MIB_ACTIVATE); > > } > > > > -static int mt7530_phy_read(struct dsa_switch *ds, int port, int regnum) > > +static int mt7530_phy_read(struct mt7530_priv *priv, int port, int regnum) > > { > > - struct mt7530_priv *priv = ds->priv; > > - > > return mdiobus_read_nested(priv->bus, port, regnum); > > } > > > > -static int mt7530_phy_write(struct dsa_switch *ds, int port, int regnum, > > +static int mt7530_phy_write(struct mt7530_priv *priv, int port, int regnum, > > u16 val) > > { > > - struct mt7530_priv *priv = ds->priv; > > - > > return mdiobus_write_nested(priv->bus, port, regnum, val); > > } > > > > @@ -785,9 +782,8 @@ mt7531_ind_c22_phy_write(struct mt7530_priv *priv, int > > port, int regnum, > > } > > > > static int > > -mt7531_ind_phy_read(struct dsa_switch *ds, int port, int regnum) > > +mt7531_ind_phy_read(struct mt7530_priv *priv, int port, int regnum) > > { > > - struct mt7530_priv *priv = ds->priv; > > int devad; > > int ret; > > > > @@ -803,10 +799,9 @@ mt7531_ind_phy_read(struct dsa_switch *ds, int port, > > int regnum) > > } > > > > static int > > -mt7531_ind_phy_write(struct dsa_switch *ds, int port, int regnum, > > +mt7531_ind_phy_write(struct mt7530_priv *priv, int port, int regnum, > > u16 data) > > { > > - struct mt7530_priv *priv = ds->priv; > > int devad; > > int ret; > > > > @@ -822,6 +817,22 @@ mt7531_ind_phy_write(struct dsa_switch *ds, int port, > > int regnum, > > return ret; > > } > > > > +static int > > +mt753x_phy_read(struct mii_bus *bus, int port, int regnum) > > +{ > > + struct mt7530_priv *priv = bus->priv; > > + > > + return priv->info->phy_read(priv, port, regnum); > > +} > > + > > +static int > > +mt753x_phy_write(struct mii_bus *bus, int port, int regnum, u16 val) > > +{ > > + struct mt7530_priv *priv = bus->priv; > > + > > + return priv->info->phy_write(priv, port, regnum, val); > > +} > > + > > static void > > mt7530_get_strings(struct dsa_switch *ds, int port, u32 stringset, > >uint8_t *data) > > @@ -1828,6 +1839,211 @@ mt7530_setup_gpio(struct mt7530_priv *priv) > > } > > #endif /* CONFIG_GPIOLIB */ > > > > +static irqreturn_t > > +mt7530_irq_thread_fn(int irq, void *dev_id) > > +{ > > + struct mt7530_priv *priv = dev_id; > > + bool handled = false; > > + u32 val; > > + int p; > > + > > + mutex_lock_nested(>bus->mdio_lock, MDIO_MUTEX_NESTED); > > + val = mt7530_mii_read(priv, MT7530_SYS_INT_STS); > > + mt7530_mii_write(priv, MT7530_SYS_INT_STS, val); > > + mutex_unlock(>bus->mdio_lock); > > + > > + for (p = 0; p < MT7530_NUM_PHYS; p++) { > > + if (BIT(p) & val) { > > + unsigned int irq; > > + > > + irq = irq_find_mapping(priv->irq_domain, p); > > + handle_nested_irq(irq); > > + handled = true; > > + } > > + } > > + > > + return IRQ_RETVAL(handled); > > +} > > + > > +static void > > +mt7530_irq_mask(struct irq_data *d) > > +{ > > + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); > > + > > + priv->irq_enable &= ~BIT(d->hwirq); > > +} > > + > > +static void > > +mt7530_irq_unmask(struct irq_data *d) > > +{ > > + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); > > + > > + priv->irq_enable |= BIT(d->hwirq); > > +} > > + > > +static void > > +mt7530_irq_bus_lock(struct
Re: [RFC v4 net-next 2/4] net: dsa: mt7530: add interrupt support
On Mon, 12 Apr 2021 04:42:35 +0100, DENG Qingfang wrote: > > Add support for MT7530 interrupt controller to handle internal PHYs. > In order to assign an IRQ number to each PHY, the registration of MDIO bus > is also done in this driver. > > Signed-off-by: DENG Qingfang > --- > RFC v3 -> RFC v4: > - No changes. > > drivers/net/dsa/Kconfig | 1 + > drivers/net/dsa/mt7530.c | 266 +++ > drivers/net/dsa/mt7530.h | 20 ++- > 3 files changed, 258 insertions(+), 29 deletions(-) > > diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig > index a5f1aa911fe2..264384449f09 100644 > --- a/drivers/net/dsa/Kconfig > +++ b/drivers/net/dsa/Kconfig > @@ -36,6 +36,7 @@ config NET_DSA_LANTIQ_GSWIP > config NET_DSA_MT7530 > tristate "MediaTek MT753x and MT7621 Ethernet switch support" > select NET_DSA_TAG_MTK > + select MEDIATEK_PHY > help > This enables support for the MediaTek MT7530, MT7531, and MT7621 > Ethernet switch chips. > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > index 2bd1bab71497..da033004a74d 100644 > --- a/drivers/net/dsa/mt7530.c > +++ b/drivers/net/dsa/mt7530.c > @@ -10,6 +10,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -596,18 +597,14 @@ mt7530_mib_reset(struct dsa_switch *ds) > mt7530_write(priv, MT7530_MIB_CCR, CCR_MIB_ACTIVATE); > } > > -static int mt7530_phy_read(struct dsa_switch *ds, int port, int regnum) > +static int mt7530_phy_read(struct mt7530_priv *priv, int port, int regnum) > { > - struct mt7530_priv *priv = ds->priv; > - > return mdiobus_read_nested(priv->bus, port, regnum); > } > > -static int mt7530_phy_write(struct dsa_switch *ds, int port, int regnum, > +static int mt7530_phy_write(struct mt7530_priv *priv, int port, int regnum, > u16 val) > { > - struct mt7530_priv *priv = ds->priv; > - > return mdiobus_write_nested(priv->bus, port, regnum, val); > } > > @@ -785,9 +782,8 @@ mt7531_ind_c22_phy_write(struct mt7530_priv *priv, int > port, int regnum, > } > > static int > -mt7531_ind_phy_read(struct dsa_switch *ds, int port, int regnum) > +mt7531_ind_phy_read(struct mt7530_priv *priv, int port, int regnum) > { > - struct mt7530_priv *priv = ds->priv; > int devad; > int ret; > > @@ -803,10 +799,9 @@ mt7531_ind_phy_read(struct dsa_switch *ds, int port, int > regnum) > } > > static int > -mt7531_ind_phy_write(struct dsa_switch *ds, int port, int regnum, > +mt7531_ind_phy_write(struct mt7530_priv *priv, int port, int regnum, >u16 data) > { > - struct mt7530_priv *priv = ds->priv; > int devad; > int ret; > > @@ -822,6 +817,22 @@ mt7531_ind_phy_write(struct dsa_switch *ds, int port, > int regnum, > return ret; > } > > +static int > +mt753x_phy_read(struct mii_bus *bus, int port, int regnum) > +{ > + struct mt7530_priv *priv = bus->priv; > + > + return priv->info->phy_read(priv, port, regnum); > +} > + > +static int > +mt753x_phy_write(struct mii_bus *bus, int port, int regnum, u16 val) > +{ > + struct mt7530_priv *priv = bus->priv; > + > + return priv->info->phy_write(priv, port, regnum, val); > +} > + > static void > mt7530_get_strings(struct dsa_switch *ds, int port, u32 stringset, > uint8_t *data) > @@ -1828,6 +1839,211 @@ mt7530_setup_gpio(struct mt7530_priv *priv) > } > #endif /* CONFIG_GPIOLIB */ > > +static irqreturn_t > +mt7530_irq_thread_fn(int irq, void *dev_id) > +{ > + struct mt7530_priv *priv = dev_id; > + bool handled = false; > + u32 val; > + int p; > + > + mutex_lock_nested(>bus->mdio_lock, MDIO_MUTEX_NESTED); > + val = mt7530_mii_read(priv, MT7530_SYS_INT_STS); > + mt7530_mii_write(priv, MT7530_SYS_INT_STS, val); > + mutex_unlock(>bus->mdio_lock); > + > + for (p = 0; p < MT7530_NUM_PHYS; p++) { > + if (BIT(p) & val) { > + unsigned int irq; > + > + irq = irq_find_mapping(priv->irq_domain, p); > + handle_nested_irq(irq); > + handled = true; > + } > + } > + > + return IRQ_RETVAL(handled); > +} > + > +static void > +mt7530_irq_mask(struct irq_data *d) > +{ > + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); > + > + priv->irq_enable &= ~BIT(d->hwirq); > +} > + > +static void > +mt7530_irq_unmask(struct irq_data *d) > +{ > + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); > + > + priv->irq_enable |= BIT(d->hwirq); > +} > + > +static void > +mt7530_irq_bus_lock(struct irq_data *d) > +{ > + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); > + > + mutex_lock_nested(>bus->mdio_lock, MDIO_MUTEX_NESTED); > +} > + > +static void > +mt7530_irq_bus_sync_unlock(struct irq_data *d) > +{ > + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d);
[RFC v4 net-next 2/4] net: dsa: mt7530: add interrupt support
Add support for MT7530 interrupt controller to handle internal PHYs. In order to assign an IRQ number to each PHY, the registration of MDIO bus is also done in this driver. Signed-off-by: DENG Qingfang --- RFC v3 -> RFC v4: - No changes. drivers/net/dsa/Kconfig | 1 + drivers/net/dsa/mt7530.c | 266 +++ drivers/net/dsa/mt7530.h | 20 ++- 3 files changed, 258 insertions(+), 29 deletions(-) diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig index a5f1aa911fe2..264384449f09 100644 --- a/drivers/net/dsa/Kconfig +++ b/drivers/net/dsa/Kconfig @@ -36,6 +36,7 @@ config NET_DSA_LANTIQ_GSWIP config NET_DSA_MT7530 tristate "MediaTek MT753x and MT7621 Ethernet switch support" select NET_DSA_TAG_MTK + select MEDIATEK_PHY help This enables support for the MediaTek MT7530, MT7531, and MT7621 Ethernet switch chips. diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index 2bd1bab71497..da033004a74d 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -596,18 +597,14 @@ mt7530_mib_reset(struct dsa_switch *ds) mt7530_write(priv, MT7530_MIB_CCR, CCR_MIB_ACTIVATE); } -static int mt7530_phy_read(struct dsa_switch *ds, int port, int regnum) +static int mt7530_phy_read(struct mt7530_priv *priv, int port, int regnum) { - struct mt7530_priv *priv = ds->priv; - return mdiobus_read_nested(priv->bus, port, regnum); } -static int mt7530_phy_write(struct dsa_switch *ds, int port, int regnum, +static int mt7530_phy_write(struct mt7530_priv *priv, int port, int regnum, u16 val) { - struct mt7530_priv *priv = ds->priv; - return mdiobus_write_nested(priv->bus, port, regnum, val); } @@ -785,9 +782,8 @@ mt7531_ind_c22_phy_write(struct mt7530_priv *priv, int port, int regnum, } static int -mt7531_ind_phy_read(struct dsa_switch *ds, int port, int regnum) +mt7531_ind_phy_read(struct mt7530_priv *priv, int port, int regnum) { - struct mt7530_priv *priv = ds->priv; int devad; int ret; @@ -803,10 +799,9 @@ mt7531_ind_phy_read(struct dsa_switch *ds, int port, int regnum) } static int -mt7531_ind_phy_write(struct dsa_switch *ds, int port, int regnum, +mt7531_ind_phy_write(struct mt7530_priv *priv, int port, int regnum, u16 data) { - struct mt7530_priv *priv = ds->priv; int devad; int ret; @@ -822,6 +817,22 @@ mt7531_ind_phy_write(struct dsa_switch *ds, int port, int regnum, return ret; } +static int +mt753x_phy_read(struct mii_bus *bus, int port, int regnum) +{ + struct mt7530_priv *priv = bus->priv; + + return priv->info->phy_read(priv, port, regnum); +} + +static int +mt753x_phy_write(struct mii_bus *bus, int port, int regnum, u16 val) +{ + struct mt7530_priv *priv = bus->priv; + + return priv->info->phy_write(priv, port, regnum, val); +} + static void mt7530_get_strings(struct dsa_switch *ds, int port, u32 stringset, uint8_t *data) @@ -1828,6 +1839,211 @@ mt7530_setup_gpio(struct mt7530_priv *priv) } #endif /* CONFIG_GPIOLIB */ +static irqreturn_t +mt7530_irq_thread_fn(int irq, void *dev_id) +{ + struct mt7530_priv *priv = dev_id; + bool handled = false; + u32 val; + int p; + + mutex_lock_nested(>bus->mdio_lock, MDIO_MUTEX_NESTED); + val = mt7530_mii_read(priv, MT7530_SYS_INT_STS); + mt7530_mii_write(priv, MT7530_SYS_INT_STS, val); + mutex_unlock(>bus->mdio_lock); + + for (p = 0; p < MT7530_NUM_PHYS; p++) { + if (BIT(p) & val) { + unsigned int irq; + + irq = irq_find_mapping(priv->irq_domain, p); + handle_nested_irq(irq); + handled = true; + } + } + + return IRQ_RETVAL(handled); +} + +static void +mt7530_irq_mask(struct irq_data *d) +{ + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); + + priv->irq_enable &= ~BIT(d->hwirq); +} + +static void +mt7530_irq_unmask(struct irq_data *d) +{ + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); + + priv->irq_enable |= BIT(d->hwirq); +} + +static void +mt7530_irq_bus_lock(struct irq_data *d) +{ + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); + + mutex_lock_nested(>bus->mdio_lock, MDIO_MUTEX_NESTED); +} + +static void +mt7530_irq_bus_sync_unlock(struct irq_data *d) +{ + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); + + mt7530_mii_write(priv, MT7530_SYS_INT_EN, priv->irq_enable); + mutex_unlock(>bus->mdio_lock); +} + +static struct irq_chip mt7530_irq_chip = { + .name = KBUILD_MODNAME, + .irq_mask = mt7530_irq_mask, + .irq_unmask = mt7530_irq_unmask, + .irq_bus_lock =