Message-Id: <20210412152210.929733-1-dqfext@gmail.com>
Date:   Mon, 12 Apr 2021 23:22:10 +0800
From:   DENG Qingfang <dqfext@...il.com>
To:     Marc Zyngier <maz@...nel.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Landen Chao <Landen.Chao@...iatek.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Sean Wang <sean.wang@...iatek.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Sergio Paracuellos <sergio.paracuellos@...il.com>,
        linux-kernel@...r.kernel.org, linux-mediatek@...ts.infradead.org,
        linux-staging@...ts.linux.dev, devicetree@...r.kernel.org,
        netdev@...r.kernel.org, Weijie Gao <weijie.gao@...iatek.com>,
        Chuanhong Guo <gch981213@...il.com>,
        René van Dorst <opensource@...rst.com>,
        Frank Wunderlich <frank-w@...lic-files.de>,
        Thomas Gleixner <tglx@...utronix.de>,
        Greg Ungerer <gerg@...nel.org>
Subject: 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 <dqfext@...il.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@...il.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.

Powered by blists - more mailing lists