From: DENG Qingfang <dqfext@gmail.com>
To: Marc Zyngier <maz@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>,
"Andrew Lunn" <andrew@lunn.ch>,
"Florian Fainelli" <f.fainelli@gmail.com>,
"Heiner Kallweit" <hkallweit1@gmail.com>,
"Jakub Kicinski" <kuba@kernel.org>,
"Landen Chao" <Landen.Chao@mediatek.com>,
"Matthias Brugger" <matthias.bgg@gmail.com>,
"Russell King" <linux@armlinux.org.uk>,
"Sean Wang" <sean.wang@mediatek.com>,
"Vivien Didelot" <vivien.didelot@gmail.com>,
"Vladimir Oltean" <olteanv@gmail.com>,
"Rob Herring" <robh+dt@kernel.org>,
"Linus Walleij" <linus.walleij@linaro.org>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Sergio Paracuellos" <sergio.paracuellos@gmail.com>,
linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
linux-staging@lists.linux.dev, devicetree@vger.kernel.org,
netdev@vger.kernel.org, "Weijie Gao" <weijie.gao@mediatek.com>,
"Chuanhong Guo" <gch981213@gmail.com>,
"René van Dorst" <opensource@vdorst.com>,
"Frank Wunderlich" <frank-w@public-files.de>,
"Thomas Gleixner" <tglx@linutronix.de>,
"Greg Ungerer" <gerg@kernel.org>
Subject: Re: [RFC v4 net-next 2/4] net: dsa: mt7530: add interrupt support
Date: Mon, 12 Apr 2021 23:22:10 +0800 [thread overview]
Message-ID: <20210412152210.929733-1-dqfext@gmail.com> (raw)
In-Reply-To: <87fszvoqvb.wl-maz@kernel.org>
On Mon, Apr 12, 2021 at 09:21:12AM +0100, Marc Zyngier wrote:
> On Mon, 12 Apr 2021 04:42:35 +0100,
> DENG Qingfang <dqfext@gmail.com> 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 <dqfext@gmail.com>
> > ---
> > 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 <linux/mfd/syscon.h>
> > #include <linux/module.h>
> > #include <linux/netdevice.h>
> > +#include <linux/of_irq.h>
> > #include <linux/of_mdio.h>
> > #include <linux/of_net.h>
> > #include <linux/of_platform.h>
> > @@ -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(&priv->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(&priv->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(&priv->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(&priv->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 = mt7530_irq_bus_lock,
> > + .irq_bus_sync_unlock = mt7530_irq_bus_sync_unlock,
> > +};
> > +
> > +static int
> > +mt7530_irq_map(struct irq_domain *domain, unsigned int irq,
> > + irq_hw_number_t hwirq)
> > +{
> > + irq_set_chip_data(irq, domain->host_data);
> > + irq_set_chip_and_handler(irq, &mt7530_irq_chip, handle_simple_irq);
> > + irq_set_nested_thread(irq, true);
> > + irq_set_noprobe(irq);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct irq_domain_ops mt7530_irq_domain_ops = {
> > + .map = mt7530_irq_map,
> > + .xlate = irq_domain_xlate_onecell,
> > +};
> > +
> > +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.
Andrew, perhaps this can be done in DSA core?
>
> > + ds->slave_mii_bus->irq[p] = irq;
> > + }
> > + }
> > +}
> > +
> > +static int
> > +mt7530_setup_irq(struct mt7530_priv *priv)
> > +{
> > + struct device *dev = priv->dev;
> > + struct device_node *np = dev->of_node;
> > + int ret;
> > +
> > + if (!of_property_read_bool(np, "interrupt-controller")) {
> > + dev_info(dev, "no interrupt support\n");
> > + return 0;
> > + }
> > +
> > + priv->irq = of_irq_get(np, 0);
> > + if (priv->irq <= 0) {
> > + dev_err(dev, "failed to get parent IRQ: %d\n", priv->irq);
> > + return priv->irq ? : -EINVAL;
> > + }
> > +
> > + priv->irq_domain = irq_domain_add_linear(np, MT7530_NUM_PHYS,
> > + &mt7530_irq_domain_ops, priv);
> > + if (!priv->irq_domain) {
> > + dev_err(dev, "failed to create IRQ domain\n");
> > + return -ENOMEM;
> > + }
> > +
> > + /* This register must be set for MT7530 to properly fire interrupts */
> > + if (priv->id != ID_MT7531)
>
> Why not check for ID_MT7530 directly then?
There is also ID_MT7621, introduced by Greg Ungerer, but it is basically
MT7530 too.
>
> > + mt7530_set(priv, MT7530_TOP_SIG_CTRL, TOP_SIG_CTRL_NORMAL);
> > +
> > + ret = request_threaded_irq(priv->irq, NULL, mt7530_irq_thread_fn,
> > + IRQF_ONESHOT, KBUILD_MODNAME, priv);
> > + if (ret) {
> > + irq_domain_remove(priv->irq_domain);
> > + dev_err(dev, "failed to request IRQ: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void
> > +mt7530_free_mdio_irq(struct mt7530_priv *priv)
> > +{
> > + int p;
> > +
> > + for (p = 0; p < MT7530_NUM_PHYS; p++) {
> > + if (BIT(p) & priv->ds->phys_mii_mask) {
> > + unsigned int irq;
> > +
> > + irq = irq_find_mapping(priv->irq_domain, p);
> > + irq_dispose_mapping(irq);
> > + }
> > + }
> > +}
> > +
> > +
> > +static void
> > +mt7530_free_irq_common(struct mt7530_priv *priv)
> > +{
> > + free_irq(priv->irq, priv);
> > + irq_domain_remove(priv->irq_domain);
> > +}
> > +
> > +static void
> > +mt7530_free_irq(struct mt7530_priv *priv)
> > +{
> > + mt7530_free_mdio_irq(priv);
> > + mt7530_free_irq_common(priv);
> > +}
> > +
> > +static int
> > +mt7530_setup_mdio(struct mt7530_priv *priv)
> > +{
> > + struct dsa_switch *ds = priv->ds;
> > + struct device *dev = priv->dev;
> > + struct mii_bus *bus;
> > + static int idx;
> > + int ret;
> > +
> > + bus = devm_mdiobus_alloc(dev);
> > + if (!bus)
> > + return -ENOMEM;
> > +
> > + ds->slave_mii_bus = bus;
> > + bus->priv = priv;
> > + bus->name = KBUILD_MODNAME "-mii";
> > + snprintf(bus->id, MII_BUS_ID_SIZE, KBUILD_MODNAME "-%d", idx++);
> > + bus->read = mt753x_phy_read;
> > + bus->write = mt753x_phy_write;
> > + bus->parent = dev;
> > + bus->phy_mask = ~ds->phys_mii_mask;
> > +
> > + if (priv->irq)
> > + mt7530_setup_mdio_irq(priv);
> > +
> > + ret = mdiobus_register(bus);
> > + if (ret) {
> > + dev_err(dev, "failed to register MDIO bus: %d\n", ret);
> > + if (priv->irq)
> > + mt7530_free_mdio_irq(priv);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > static int
> > mt7530_setup(struct dsa_switch *ds)
> > {
> > @@ -2780,32 +2996,25 @@ static int
> > mt753x_setup(struct dsa_switch *ds)
> > {
> > struct mt7530_priv *priv = ds->priv;
> > + int ret = priv->info->sw_setup(ds);
> > + if (ret)
> > + return ret;
> >
> > - return priv->info->sw_setup(ds);
> > -}
> > -
> > -static int
> > -mt753x_phy_read(struct dsa_switch *ds, int port, int regnum)
> > -{
> > - struct mt7530_priv *priv = ds->priv;
> > -
> > - return priv->info->phy_read(ds, port, regnum);
> > -}
> > + ret = mt7530_setup_irq(priv);
> > + if (ret)
> > + return ret;
> >
> > -static int
> > -mt753x_phy_write(struct dsa_switch *ds, int port, int regnum, u16 val)
> > -{
> > - struct mt7530_priv *priv = ds->priv;
> > + ret = mt7530_setup_mdio(priv);
> > + if (ret && priv->irq)
> > + mt7530_free_irq_common(priv);
> >
> > - return priv->info->phy_write(ds, port, regnum, val);
> > + return ret;
> > }
> >
> > static const struct dsa_switch_ops mt7530_switch_ops = {
> > .get_tag_protocol = mtk_get_tag_protocol,
> > .setup = mt753x_setup,
> > .get_strings = mt7530_get_strings,
> > - .phy_read = mt753x_phy_read,
> > - .phy_write = mt753x_phy_write,
>
> I don't get why this change is part of the interrupt support. This
> should probably be a separate patch.
These 2 functions are for DSA slave MII bus. Since the driver now manages
the MII bus, mt753x_phy_{read,write} are assigned to bus->{read,write}
instead.
>
> > .get_ethtool_stats = mt7530_get_ethtool_stats,
> > .get_sset_count = mt7530_get_sset_count,
> > .set_ageing_time = mt7530_set_ageing_time,
> > @@ -2986,6 +3195,9 @@ mt7530_remove(struct mdio_device *mdiodev)
> > dev_err(priv->dev, "Failed to disable io pwr: %d\n",
> > ret);
> >
> > + if (priv->irq)
> > + mt7530_free_irq(priv);
> > +
> > dsa_unregister_switch(priv->ds);
> > mutex_destroy(&priv->reg_mutex);
> > }
> > diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> > index ec36ea5dfd57..62fcaabefba1 100644
> > --- a/drivers/net/dsa/mt7530.h
> > +++ b/drivers/net/dsa/mt7530.h
>
> nit: Another thing is that I don't see why this include file exists at
> all. The only user is mt7530.c, so the two could be merged and reduce
> the clutter.
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
WARNING: multiple messages have this Message-ID (diff)
From: DENG Qingfang <dqfext@gmail.com>
To: Marc Zyngier <maz@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>,
"Andrew Lunn" <andrew@lunn.ch>,
"Florian Fainelli" <f.fainelli@gmail.com>,
"Heiner Kallweit" <hkallweit1@gmail.com>,
"Jakub Kicinski" <kuba@kernel.org>,
"Landen Chao" <Landen.Chao@mediatek.com>,
"Matthias Brugger" <matthias.bgg@gmail.com>,
"Russell King" <linux@armlinux.org.uk>,
"Sean Wang" <sean.wang@mediatek.com>,
"Vivien Didelot" <vivien.didelot@gmail.com>,
"Vladimir Oltean" <olteanv@gmail.com>,
"Rob Herring" <robh+dt@kernel.org>,
"Linus Walleij" <linus.walleij@linaro.org>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Sergio Paracuellos" <sergio.paracuellos@gmail.com>,
linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
linux-staging@lists.linux.dev, devicetree@vger.kernel.org,
netdev@vger.kernel.org, "Weijie Gao" <weijie.gao@mediatek.com>,
"Chuanhong Guo" <gch981213@gmail.com>,
"René van Dorst" <opensource@vdorst.com>,
"Frank Wunderlich" <frank-w@public-files.de>,
"Thomas Gleixner" <tglx@linutronix.de>,
"Greg Ungerer" <gerg@kernel.org>
Subject: Re: [RFC v4 net-next 2/4] net: dsa: mt7530: add interrupt support
Date: Mon, 12 Apr 2021 23:22:10 +0800 [thread overview]
Message-ID: <20210412152210.929733-1-dqfext@gmail.com> (raw)
In-Reply-To: <87fszvoqvb.wl-maz@kernel.org>
On Mon, Apr 12, 2021 at 09:21:12AM +0100, Marc Zyngier wrote:
> On Mon, 12 Apr 2021 04:42:35 +0100,
> DENG Qingfang <dqfext@gmail.com> 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 <dqfext@gmail.com>
> > ---
> > 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 <linux/mfd/syscon.h>
> > #include <linux/module.h>
> > #include <linux/netdevice.h>
> > +#include <linux/of_irq.h>
> > #include <linux/of_mdio.h>
> > #include <linux/of_net.h>
> > #include <linux/of_platform.h>
> > @@ -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(&priv->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(&priv->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(&priv->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(&priv->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 = mt7530_irq_bus_lock,
> > + .irq_bus_sync_unlock = mt7530_irq_bus_sync_unlock,
> > +};
> > +
> > +static int
> > +mt7530_irq_map(struct irq_domain *domain, unsigned int irq,
> > + irq_hw_number_t hwirq)
> > +{
> > + irq_set_chip_data(irq, domain->host_data);
> > + irq_set_chip_and_handler(irq, &mt7530_irq_chip, handle_simple_irq);
> > + irq_set_nested_thread(irq, true);
> > + irq_set_noprobe(irq);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct irq_domain_ops mt7530_irq_domain_ops = {
> > + .map = mt7530_irq_map,
> > + .xlate = irq_domain_xlate_onecell,
> > +};
> > +
> > +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.
Andrew, perhaps this can be done in DSA core?
>
> > + ds->slave_mii_bus->irq[p] = irq;
> > + }
> > + }
> > +}
> > +
> > +static int
> > +mt7530_setup_irq(struct mt7530_priv *priv)
> > +{
> > + struct device *dev = priv->dev;
> > + struct device_node *np = dev->of_node;
> > + int ret;
> > +
> > + if (!of_property_read_bool(np, "interrupt-controller")) {
> > + dev_info(dev, "no interrupt support\n");
> > + return 0;
> > + }
> > +
> > + priv->irq = of_irq_get(np, 0);
> > + if (priv->irq <= 0) {
> > + dev_err(dev, "failed to get parent IRQ: %d\n", priv->irq);
> > + return priv->irq ? : -EINVAL;
> > + }
> > +
> > + priv->irq_domain = irq_domain_add_linear(np, MT7530_NUM_PHYS,
> > + &mt7530_irq_domain_ops, priv);
> > + if (!priv->irq_domain) {
> > + dev_err(dev, "failed to create IRQ domain\n");
> > + return -ENOMEM;
> > + }
> > +
> > + /* This register must be set for MT7530 to properly fire interrupts */
> > + if (priv->id != ID_MT7531)
>
> Why not check for ID_MT7530 directly then?
There is also ID_MT7621, introduced by Greg Ungerer, but it is basically
MT7530 too.
>
> > + mt7530_set(priv, MT7530_TOP_SIG_CTRL, TOP_SIG_CTRL_NORMAL);
> > +
> > + ret = request_threaded_irq(priv->irq, NULL, mt7530_irq_thread_fn,
> > + IRQF_ONESHOT, KBUILD_MODNAME, priv);
> > + if (ret) {
> > + irq_domain_remove(priv->irq_domain);
> > + dev_err(dev, "failed to request IRQ: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void
> > +mt7530_free_mdio_irq(struct mt7530_priv *priv)
> > +{
> > + int p;
> > +
> > + for (p = 0; p < MT7530_NUM_PHYS; p++) {
> > + if (BIT(p) & priv->ds->phys_mii_mask) {
> > + unsigned int irq;
> > +
> > + irq = irq_find_mapping(priv->irq_domain, p);
> > + irq_dispose_mapping(irq);
> > + }
> > + }
> > +}
> > +
> > +
> > +static void
> > +mt7530_free_irq_common(struct mt7530_priv *priv)
> > +{
> > + free_irq(priv->irq, priv);
> > + irq_domain_remove(priv->irq_domain);
> > +}
> > +
> > +static void
> > +mt7530_free_irq(struct mt7530_priv *priv)
> > +{
> > + mt7530_free_mdio_irq(priv);
> > + mt7530_free_irq_common(priv);
> > +}
> > +
> > +static int
> > +mt7530_setup_mdio(struct mt7530_priv *priv)
> > +{
> > + struct dsa_switch *ds = priv->ds;
> > + struct device *dev = priv->dev;
> > + struct mii_bus *bus;
> > + static int idx;
> > + int ret;
> > +
> > + bus = devm_mdiobus_alloc(dev);
> > + if (!bus)
> > + return -ENOMEM;
> > +
> > + ds->slave_mii_bus = bus;
> > + bus->priv = priv;
> > + bus->name = KBUILD_MODNAME "-mii";
> > + snprintf(bus->id, MII_BUS_ID_SIZE, KBUILD_MODNAME "-%d", idx++);
> > + bus->read = mt753x_phy_read;
> > + bus->write = mt753x_phy_write;
> > + bus->parent = dev;
> > + bus->phy_mask = ~ds->phys_mii_mask;
> > +
> > + if (priv->irq)
> > + mt7530_setup_mdio_irq(priv);
> > +
> > + ret = mdiobus_register(bus);
> > + if (ret) {
> > + dev_err(dev, "failed to register MDIO bus: %d\n", ret);
> > + if (priv->irq)
> > + mt7530_free_mdio_irq(priv);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > static int
> > mt7530_setup(struct dsa_switch *ds)
> > {
> > @@ -2780,32 +2996,25 @@ static int
> > mt753x_setup(struct dsa_switch *ds)
> > {
> > struct mt7530_priv *priv = ds->priv;
> > + int ret = priv->info->sw_setup(ds);
> > + if (ret)
> > + return ret;
> >
> > - return priv->info->sw_setup(ds);
> > -}
> > -
> > -static int
> > -mt753x_phy_read(struct dsa_switch *ds, int port, int regnum)
> > -{
> > - struct mt7530_priv *priv = ds->priv;
> > -
> > - return priv->info->phy_read(ds, port, regnum);
> > -}
> > + ret = mt7530_setup_irq(priv);
> > + if (ret)
> > + return ret;
> >
> > -static int
> > -mt753x_phy_write(struct dsa_switch *ds, int port, int regnum, u16 val)
> > -{
> > - struct mt7530_priv *priv = ds->priv;
> > + ret = mt7530_setup_mdio(priv);
> > + if (ret && priv->irq)
> > + mt7530_free_irq_common(priv);
> >
> > - return priv->info->phy_write(ds, port, regnum, val);
> > + return ret;
> > }
> >
> > static const struct dsa_switch_ops mt7530_switch_ops = {
> > .get_tag_protocol = mtk_get_tag_protocol,
> > .setup = mt753x_setup,
> > .get_strings = mt7530_get_strings,
> > - .phy_read = mt753x_phy_read,
> > - .phy_write = mt753x_phy_write,
>
> I don't get why this change is part of the interrupt support. This
> should probably be a separate patch.
These 2 functions are for DSA slave MII bus. Since the driver now manages
the MII bus, mt753x_phy_{read,write} are assigned to bus->{read,write}
instead.
>
> > .get_ethtool_stats = mt7530_get_ethtool_stats,
> > .get_sset_count = mt7530_get_sset_count,
> > .set_ageing_time = mt7530_set_ageing_time,
> > @@ -2986,6 +3195,9 @@ mt7530_remove(struct mdio_device *mdiodev)
> > dev_err(priv->dev, "Failed to disable io pwr: %d\n",
> > ret);
> >
> > + if (priv->irq)
> > + mt7530_free_irq(priv);
> > +
> > dsa_unregister_switch(priv->ds);
> > mutex_destroy(&priv->reg_mutex);
> > }
> > diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> > index ec36ea5dfd57..62fcaabefba1 100644
> > --- a/drivers/net/dsa/mt7530.h
> > +++ b/drivers/net/dsa/mt7530.h
>
> nit: Another thing is that I don't see why this include file exists at
> all. The only user is mt7530.c, so the two could be merged and reduce
> the clutter.
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
next prev parent reply other threads:[~2021-04-12 15:22 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-12 3:42 [RFC v4 net-next 0/4] MT7530 interrupt support DENG Qingfang
2021-04-12 3:42 ` DENG Qingfang
2021-04-12 3:42 ` [RFC v4 net-next 1/4] net: phy: add MediaTek PHY driver DENG Qingfang
2021-04-12 3:42 ` DENG Qingfang
2021-04-12 7:04 ` René van Dorst
2021-04-12 7:04 ` René van Dorst
2021-04-12 15:08 ` DENG Qingfang
2021-04-12 15:08 ` DENG Qingfang
2021-04-13 3:59 ` DENG Qingfang
2021-04-13 3:59 ` DENG Qingfang
2021-04-13 9:55 ` René van Dorst
2021-04-13 9:55 ` René van Dorst
2021-04-13 13:12 ` Russell King - ARM Linux admin
2021-04-13 13:12 ` Russell King - ARM Linux admin
2021-04-15 9:49 ` DENG Qingfang
2021-04-15 9:49 ` DENG Qingfang
2021-04-12 3:42 ` [RFC v4 net-next 2/4] net: dsa: mt7530: add interrupt support DENG Qingfang
2021-04-12 3:42 ` DENG Qingfang
2021-04-12 8:21 ` Marc Zyngier
2021-04-12 8:21 ` Marc Zyngier
2021-04-12 15:22 ` DENG Qingfang [this message]
2021-04-12 15:22 ` DENG Qingfang
2021-04-13 0:07 ` Andrew Lunn
2021-04-13 0:07 ` Andrew Lunn
2021-04-13 8:06 ` Marc Zyngier
2021-04-13 8:06 ` Marc Zyngier
2021-04-13 12:52 ` Andrew Lunn
2021-04-13 12:52 ` Andrew Lunn
2021-04-13 15:29 ` DENG Qingfang
2021-04-13 15:29 ` DENG Qingfang
2021-04-12 3:42 ` [RFC v4 net-next 3/4] dt-bindings: net: dsa: add MT7530 interrupt controller binding DENG Qingfang
2021-04-12 3:42 ` DENG Qingfang
2021-04-12 10:33 ` Kurt Kanzenbach
2021-04-12 10:33 ` Kurt Kanzenbach
2021-04-12 3:42 ` [RFC v4 net-next 4/4] staging: mt7621-dts: enable MT7530 interrupt controller DENG Qingfang
2021-04-12 3:42 ` DENG Qingfang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210412152210.929733-1-dqfext@gmail.com \
--to=dqfext@gmail.com \
--cc=Landen.Chao@mediatek.com \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=f.fainelli@gmail.com \
--cc=frank-w@public-files.de \
--cc=gch981213@gmail.com \
--cc=gerg@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=hkallweit1@gmail.com \
--cc=kuba@kernel.org \
--cc=linus.walleij@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-staging@lists.linux.dev \
--cc=linux@armlinux.org.uk \
--cc=matthias.bgg@gmail.com \
--cc=maz@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=olteanv@gmail.com \
--cc=opensource@vdorst.com \
--cc=robh+dt@kernel.org \
--cc=sean.wang@mediatek.com \
--cc=sergio.paracuellos@gmail.com \
--cc=tglx@linutronix.de \
--cc=vivien.didelot@gmail.com \
--cc=weijie.gao@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.