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.
next prev parent reply other threads:[~2021-04-12 15:22 UTC|newest]
Thread overview: 18+ 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 ` [RFC v4 net-next 1/4] net: phy: add MediaTek PHY driver DENG Qingfang
2021-04-12 7:04 ` René van Dorst
2021-04-12 15:08 ` DENG Qingfang
2021-04-13 3:59 ` DENG Qingfang
2021-04-13 9:55 ` René van Dorst
2021-04-13 13:12 ` Russell King - ARM Linux admin
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 8:21 ` Marc Zyngier
2021-04-12 15:22 ` DENG Qingfang [this message]
2021-04-13 0:07 ` Andrew Lunn
2021-04-13 8:06 ` Marc Zyngier
2021-04-13 12:52 ` Andrew Lunn
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 10:33 ` Kurt Kanzenbach
2021-04-12 3:42 ` [RFC v4 net-next 4/4] staging: mt7621-dts: enable MT7530 interrupt controller 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).