Registering IRQ for MT7530 internal PHYs

Registering IRQ for MT7530 internal PHYs

[DENG Qingfang]

Hi,

I added MT7530 IRQ support and registered its internal PHYs to IRQ.
It works but my patch used two hacks.

1. Removed phy_drv_supports_irq check, because config_intr and
handle_interrupt are not set for Generic PHY.

2. Allocated ds->slave_mii_bus before calling ds->ops->setup, because
we cannot call dsa_slave_mii_bus_init which is private.

Any better ideas?

Regards,
Qingfang

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index a67cac15a724..d59a8c50ede3 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>
@@ -1639,6 +1640,125 @@ mtk_get_tag_protocol(struct dsa_switch *ds, int port,
 	}
 }
 
+static irqreturn_t
+mt7530_irq(int irq, void *data)
+{
+	struct mt7530_priv *priv = data;
+	bool handled = false;
+	int phy;
+	u32 val;
+
+	val = mt7530_read(priv, MT7530_SYS_INT_STS);
+	mt7530_write(priv, MT7530_SYS_INT_STS, val);
+
+	dev_info_ratelimited(priv->dev, "interrupt status: 0x%08x\n", val);
+	dev_info_ratelimited(priv->dev, "interrupt enable: 0x%08x\n", mt7530_read(priv, MT7530_SYS_INT_EN));
+
+	for (phy = 0; phy < MT7530_NUM_PHYS; phy++) {
+		if (val & BIT(phy)) {
+			unsigned int child_irq;
+
+			child_irq = irq_find_mapping(priv->irq_domain, phy);
+			handle_nested_irq(child_irq);
+			handled = true;
+		}
+	}
+
+	return handled ? IRQ_HANDLED : IRQ_NONE;
+}
+
+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(&priv->reg_mutex);
+}
+
+static void mt7530_irq_bus_sync_unlock(struct irq_data *d)
+{
+	struct mt7530_priv *priv = irq_data_get_irq_chip_data(d);
+
+	mt7530_write(priv, MT7530_SYS_INT_EN, priv->irq_enable);
+	mutex_unlock(&priv->reg_mutex);
+}
+
+static struct irq_chip mt7530_irq_chip = {
+	.name = MT7530_NAME,
+	.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_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_irq_init(struct mt7530_priv *priv)
+{
+	struct mii_bus *bus = priv->ds->slave_mii_bus;
+	struct device *dev = priv->dev;
+	struct device_node *np = dev->of_node;
+	int parent_irq;
+	int phy, ret;
+
+	parent_irq = of_irq_get(np, 0);
+	if (parent_irq <= 0) {
+		dev_err(dev, "failed to get parent IRQ: %d\n", parent_irq);
+		return;
+	}
+
+	mt7530_set(priv, MT7530_TOP_SIG_CTRL, TOP_SIG_CTRL_NORMAL);
+	ret = devm_request_threaded_irq(dev, parent_irq, NULL, mt7530_irq,
+					IRQF_ONESHOT, MT7530_NAME, priv);
+	if (ret) {
+		dev_err(dev, "failed to request IRQ: %d\n", ret);
+		return;
+	}
+
+	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;
+	}
+
+	/* IRQ for internal PHYs */
+	for (phy = 0; phy < MT7530_NUM_PHYS; phy++) {
+		unsigned int irq = irq_create_mapping(priv->irq_domain, phy);
+
+		irq_set_parent(irq, parent_irq);
+		bus->irq[phy] = irq;
+	}
+}
+
 static int
 mt7530_setup(struct dsa_switch *ds)
 {
@@ -2578,8 +2698,13 @@ static int
 mt753x_setup(struct dsa_switch *ds)
 {
 	struct mt7530_priv *priv = ds->priv;
+	int ret =  priv->info->sw_setup(ds);
 
-	return priv->info->sw_setup(ds);
+	/* Setup interrupt */
+	if (!ret)
+		mt7530_irq_init(priv);
+
+	return ret;
 }
 
 static int
@@ -2780,6 +2905,9 @@ mt7530_remove(struct mdio_device *mdiodev)
 		dev_err(priv->dev, "Failed to disable io pwr: %d\n",
 			ret);
 
+	if (priv->irq_domain)
+		irq_domain_remove(priv->irq_domain);
+
 	dsa_unregister_switch(priv->ds);
 	mutex_destroy(&priv->reg_mutex);
 }
@@ -2788,7 +2916,7 @@ static struct mdio_driver mt7530_mdio_driver = {
 	.probe  = mt7530_probe,
 	.remove = mt7530_remove,
 	.mdiodrv.driver = {
-		.name = "mt7530",
+		.name = MT7530_NAME,
 		.of_match_table = mt7530_of_match,
 	},
 };
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 32d8969b3ace..b1988d8085bb 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -6,7 +6,10 @@
 #ifndef __MT7530_H
 #define __MT7530_H
 
+#define MT7530_NAME			"mt7530"
+
 #define MT7530_NUM_PORTS		7
+#define MT7530_NUM_PHYS			5
 #define MT7530_CPU_PORT			6
 #define MT7530_NUM_FDB_RECORDS		2048
 #define MT7530_ALL_MEMBERS		0xff
@@ -380,6 +383,12 @@ enum mt7531_sgmii_force_duplex {
 #define  SYS_CTRL_SW_RST		BIT(1)
 #define  SYS_CTRL_REG_RST		BIT(0)
 
+/* Register for system interrupt */
+#define MT7530_SYS_INT_EN		0x7008
+
+/* Register for system interrupt status */
+#define MT7530_SYS_INT_STS		0x700c
+
 /* Register for PHY Indirect Access Control */
 #define MT7531_PHY_IAC			0x701C
 #define  MT7531_PHY_ACS_ST		BIT(31)
@@ -761,6 +770,8 @@ struct mt7530_priv {
 	struct mt7530_port	ports[MT7530_NUM_PORTS];
 	/* protect among processes for registers access*/
 	struct mutex reg_mutex;
+	struct irq_domain *irq_domain;
+	u32 irq_enable;
 };
 
 struct mt7530_hw_vlan_entry {
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 80c2e646c093..cdddc27e2df7 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2846,12 +2846,6 @@ static int phy_probe(struct device *dev)
 
 	phydev->drv = phydrv;
 
-	/* Disable the interrupt if the PHY doesn't support it
-	 * but the interrupt is still a valid one
-	 */
-	 if (!phy_drv_supports_irq(phydrv) && phy_interrupt_is_valid(phydev))
-		phydev->irq = PHY_POLL;
-
 	if (phydrv->flags & PHY_IS_INTERNAL)
 		phydev->is_internal = true;
 
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 183003e45762..ef5db1106581 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -448,19 +448,19 @@ static int dsa_switch_setup(struct dsa_switch *ds)
 	if (err)
 		goto unregister_devlink_ports;
 
+	ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
+	if (!ds->slave_mii_bus) {
+		err = -ENOMEM;
+		goto unregister_notifier;
+	}
+
 	err = ds->ops->setup(ds);
 	if (err < 0)
 		goto unregister_notifier;
 
 	devlink_params_publish(ds->devlink);
 
-	if (!ds->slave_mii_bus && ds->ops->phy_read) {
-		ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
-		if (!ds->slave_mii_bus) {
-			err = -ENOMEM;
-			goto unregister_notifier;
-		}
-
+	if (ds->ops->phy_read) {
 		dsa_slave_mii_bus_init(ds);
 
 		err = mdiobus_register(ds->slave_mii_bus);
-- 
2.25.1

Re: Registering IRQ for MT7530 internal PHYs

[Heiner Kallweit]

On 30.12.2020 05:22, DENG Qingfang wrote:
> Hi,
> 
> I added MT7530 IRQ support and registered its internal PHYs to IRQ.
> It works but my patch used two hacks.
> 
> 1. Removed phy_drv_supports_irq check, because config_intr and
> handle_interrupt are not set for Generic PHY.
> 
I don't think that's the best option. First, did you check which
consequences this has? The check is there for a reason.
You may want to add a PHY driver for your chip. Supposedly it
supports at least PHY suspend/resume. You can use the RTL8366RB
PHY driver as template.

> 2. Allocated ds->slave_mii_bus before calling ds->ops->setup, because
> we cannot call dsa_slave_mii_bus_init which is private.
> 
> Any better ideas?
> 
> Regards,
> Qingfang
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index a67cac15a724..d59a8c50ede3 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>
> @@ -1639,6 +1640,125 @@ mtk_get_tag_protocol(struct dsa_switch *ds, int port,
>  	}
>  }
>  
> +static irqreturn_t
> +mt7530_irq(int irq, void *data)
> +{
> +	struct mt7530_priv *priv = data;
> +	bool handled = false;
> +	int phy;
> +	u32 val;
> +
> +	val = mt7530_read(priv, MT7530_SYS_INT_STS);
> +	mt7530_write(priv, MT7530_SYS_INT_STS, val);
> +
> +	dev_info_ratelimited(priv->dev, "interrupt status: 0x%08x\n", val);
> +	dev_info_ratelimited(priv->dev, "interrupt enable: 0x%08x\n", mt7530_read(priv, MT7530_SYS_INT_EN));
> +
This is debug code to be removed in the final version?

> +	for (phy = 0; phy < MT7530_NUM_PHYS; phy++) {
> +		if (val & BIT(phy)) {
> +			unsigned int child_irq;
> +
> +			child_irq = irq_find_mapping(priv->irq_domain, phy);
> +			handle_nested_irq(child_irq);
> +			handled = true;
> +		}
> +	}
> +
> +	return handled ? IRQ_HANDLED : IRQ_NONE;

IRQ_RETVAL() could be used here.

> +}
> +
> +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);

Here you don't actually do something. HW doesn't support masking
interrupt generation for a port?

> +}
> +
> +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(&priv->reg_mutex);
> +}
> +
> +static void mt7530_irq_bus_sync_unlock(struct irq_data *d)
> +{
> +	struct mt7530_priv *priv = irq_data_get_irq_chip_data(d);
> +
> +	mt7530_write(priv, MT7530_SYS_INT_EN, priv->irq_enable);
> +	mutex_unlock(&priv->reg_mutex);
> +}
> +
> +static struct irq_chip mt7530_irq_chip = {
> +	.name = MT7530_NAME,
> +	.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_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_irq_init(struct mt7530_priv *priv)
> +{
> +	struct mii_bus *bus = priv->ds->slave_mii_bus;
> +	struct device *dev = priv->dev;
> +	struct device_node *np = dev->of_node;
> +	int parent_irq;
> +	int phy, ret;
> +
> +	parent_irq = of_irq_get(np, 0);
> +	if (parent_irq <= 0) {
> +		dev_err(dev, "failed to get parent IRQ: %d\n", parent_irq);
> +		return;
> +	}
> +
> +	mt7530_set(priv, MT7530_TOP_SIG_CTRL, TOP_SIG_CTRL_NORMAL);
> +	ret = devm_request_threaded_irq(dev, parent_irq, NULL, mt7530_irq,
> +					IRQF_ONESHOT, MT7530_NAME, priv);
> +	if (ret) {
> +		dev_err(dev, "failed to request IRQ: %d\n", ret);
> +		return;
> +	}
> +
> +	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;
> +	}
> +
> +	/* IRQ for internal PHYs */
> +	for (phy = 0; phy < MT7530_NUM_PHYS; phy++) {
> +		unsigned int irq = irq_create_mapping(priv->irq_domain, phy);
> +
> +		irq_set_parent(irq, parent_irq);
> +		bus->irq[phy] = irq;
> +	}
> +}
> +
>  static int
>  mt7530_setup(struct dsa_switch *ds)
>  {
> @@ -2578,8 +2698,13 @@ static int
>  mt753x_setup(struct dsa_switch *ds)
>  {
>  	struct mt7530_priv *priv = ds->priv;
> +	int ret =  priv->info->sw_setup(ds);
>  
> -	return priv->info->sw_setup(ds);
> +	/* Setup interrupt */
> +	if (!ret)
> +		mt7530_irq_init(priv);
> +
> +	return ret;
>  }
>  
>  static int
> @@ -2780,6 +2905,9 @@ mt7530_remove(struct mdio_device *mdiodev)
>  		dev_err(priv->dev, "Failed to disable io pwr: %d\n",
>  			ret);
>  
> +	if (priv->irq_domain)
> +		irq_domain_remove(priv->irq_domain);
> +
>  	dsa_unregister_switch(priv->ds);
>  	mutex_destroy(&priv->reg_mutex);
>  }
> @@ -2788,7 +2916,7 @@ static struct mdio_driver mt7530_mdio_driver = {
>  	.probe  = mt7530_probe,
>  	.remove = mt7530_remove,
>  	.mdiodrv.driver = {
> -		.name = "mt7530",
> +		.name = MT7530_NAME,
>  		.of_match_table = mt7530_of_match,
>  	},
>  };
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index 32d8969b3ace..b1988d8085bb 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -6,7 +6,10 @@
>  #ifndef __MT7530_H
>  #define __MT7530_H
>  
> +#define MT7530_NAME			"mt7530"
> +
>  #define MT7530_NUM_PORTS		7
> +#define MT7530_NUM_PHYS			5
>  #define MT7530_CPU_PORT			6
>  #define MT7530_NUM_FDB_RECORDS		2048
>  #define MT7530_ALL_MEMBERS		0xff
> @@ -380,6 +383,12 @@ enum mt7531_sgmii_force_duplex {
>  #define  SYS_CTRL_SW_RST		BIT(1)
>  #define  SYS_CTRL_REG_RST		BIT(0)
>  
> +/* Register for system interrupt */
> +#define MT7530_SYS_INT_EN		0x7008
> +
> +/* Register for system interrupt status */
> +#define MT7530_SYS_INT_STS		0x700c
> +
>  /* Register for PHY Indirect Access Control */
>  #define MT7531_PHY_IAC			0x701C
>  #define  MT7531_PHY_ACS_ST		BIT(31)
> @@ -761,6 +770,8 @@ struct mt7530_priv {
>  	struct mt7530_port	ports[MT7530_NUM_PORTS];
>  	/* protect among processes for registers access*/
>  	struct mutex reg_mutex;
> +	struct irq_domain *irq_domain;
> +	u32 irq_enable;
>  };
>  
>  struct mt7530_hw_vlan_entry {
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 80c2e646c093..cdddc27e2df7 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -2846,12 +2846,6 @@ static int phy_probe(struct device *dev)
>  
>  	phydev->drv = phydrv;
>  
> -	/* Disable the interrupt if the PHY doesn't support it
> -	 * but the interrupt is still a valid one
> -	 */
> -	 if (!phy_drv_supports_irq(phydrv) && phy_interrupt_is_valid(phydev))
> -		phydev->irq = PHY_POLL;
> -
>  	if (phydrv->flags & PHY_IS_INTERNAL)
>  		phydev->is_internal = true;
>  
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index 183003e45762..ef5db1106581 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -448,19 +448,19 @@ static int dsa_switch_setup(struct dsa_switch *ds)
>  	if (err)
>  		goto unregister_devlink_ports;
>  
> +	ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
> +	if (!ds->slave_mii_bus) {
> +		err = -ENOMEM;
> +		goto unregister_notifier;
> +	}
> +
>  	err = ds->ops->setup(ds);
>  	if (err < 0)
>  		goto unregister_notifier;
>  
>  	devlink_params_publish(ds->devlink);
>  
> -	if (!ds->slave_mii_bus && ds->ops->phy_read) {
> -		ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
> -		if (!ds->slave_mii_bus) {
> -			err = -ENOMEM;
> -			goto unregister_notifier;
> -		}
> -
> +	if (ds->ops->phy_read) {
>  		dsa_slave_mii_bus_init(ds);
>  
>  		err = mdiobus_register(ds->slave_mii_bus);
> 

Re: Registering IRQ for MT7530 internal PHYs

[DENG Qingfang]

Hi Heiner,
Thanks for your reply.

On Wed, Dec 30, 2020 at 3:39 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
> I don't think that's the best option.

I'm well aware of that.

> You may want to add a PHY driver for your chip. Supposedly it
> supports at least PHY suspend/resume. You can use the RTL8366RB
> PHY driver as template.

There's no MediaTek PHY driver yet. Do we really need a new one just
for the interrupts?

> > +     dev_info_ratelimited(priv->dev, "interrupt status: 0x%08x\n", val);
> > +     dev_info_ratelimited(priv->dev, "interrupt enable: 0x%08x\n", mt7530_read(priv, MT7530_SYS_INT_EN));
> > +
> This is debug code to be removed in the final version?

Yes.

> > +     for (phy = 0; phy < MT7530_NUM_PHYS; phy++) {
> > +             if (val & BIT(phy)) {
> > +                     unsigned int child_irq;
> > +
> > +                     child_irq = irq_find_mapping(priv->irq_domain, phy);
> > +                     handle_nested_irq(child_irq);
> > +                     handled = true;
> > +             }
> > +     }
> > +
> > +     return handled ? IRQ_HANDLED : IRQ_NONE;
>
> IRQ_RETVAL() could be used here.

Good to know :)

>
> > +}
> > +
> > +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);
>
> Here you don't actually do something. HW doesn't support masking
> interrupt generation for a port?

priv->irq_enable will be written to MT7530_SYS_INT_EN in
mt7530_irq_bus_sync_unlock. You can think of it as an inverted mask.

Re: Registering IRQ for MT7530 internal PHYs

[Heiner Kallweit]

On 30.12.2020 10:07, DENG Qingfang wrote:
> Hi Heiner,
> Thanks for your reply.
> 
> On Wed, Dec 30, 2020 at 3:39 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>> I don't think that's the best option.
> 
> I'm well aware of that.
> 
>> You may want to add a PHY driver for your chip. Supposedly it
>> supports at least PHY suspend/resume. You can use the RTL8366RB
>> PHY driver as template.
> 
> There's no MediaTek PHY driver yet. Do we really need a new one just
> for the interrupts?
> 
Not only for the interrupts. The genphy driver e.g. doesn't support
PHY suspend/resume. And the PHY driver needs basically no code,
just set the proper callbacks.

>>> +     dev_info_ratelimited(priv->dev, "interrupt status: 0x%08x\n", val);
>>> +     dev_info_ratelimited(priv->dev, "interrupt enable: 0x%08x\n", mt7530_read(priv, MT7530_SYS_INT_EN));
>>> +
>> This is debug code to be removed in the final version?
> 
> Yes.
> 
>>> +     for (phy = 0; phy < MT7530_NUM_PHYS; phy++) {
>>> +             if (val & BIT(phy)) {
>>> +                     unsigned int child_irq;
>>> +
>>> +                     child_irq = irq_find_mapping(priv->irq_domain, phy);
>>> +                     handle_nested_irq(child_irq);
>>> +                     handled = true;
>>> +             }
>>> +     }
>>> +
>>> +     return handled ? IRQ_HANDLED : IRQ_NONE;
>>
>> IRQ_RETVAL() could be used here.
> 
> Good to know :)
> 
>>
>>> +}
>>> +
>>> +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);
>>
>> Here you don't actually do something. HW doesn't support masking
>> interrupt generation for a port?
> 
> priv->irq_enable will be written to MT7530_SYS_INT_EN in
> mt7530_irq_bus_sync_unlock. You can think of it as an inverted mask.
> 

Re: Registering IRQ for MT7530 internal PHYs

[Florian Fainelli]

On 12/30/2020 1:12 AM, Heiner Kallweit wrote:
> On 30.12.2020 10:07, DENG Qingfang wrote:
>> Hi Heiner,
>> Thanks for your reply.
>>
>> On Wed, Dec 30, 2020 at 3:39 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>> I don't think that's the best option.
>>
>> I'm well aware of that.
>>
>>> You may want to add a PHY driver for your chip. Supposedly it
>>> supports at least PHY suspend/resume. You can use the RTL8366RB
>>> PHY driver as template.
>>
>> There's no MediaTek PHY driver yet. Do we really need a new one just
>> for the interrupts?
>>
> Not only for the interrupts. The genphy driver e.g. doesn't support
> PHY suspend/resume. And the PHY driver needs basically no code,
> just set the proper callbacks.

That statement about not supporting suspend/resume is not exactly true,
the generic "1g" PHY driver only implements suspend/resume through the
use of the standard BMCR power down bit, but not anything more
complicated than that.

Interrupt handling within the PHY itself is not defined by the existing
standard registers and will typically not reside in a standard register
space either, so just for that reason you do need a custom PHY driver.
There are other advantages if you need to expose additional PHY features
down the road like PHY counters, energy detection, automatic power down etc.

I don't believe we will see discrete/standalone Mediatek PHY chips, but
if that happens, then you would already have a framework for supporting
them.
-- 
Florian

Re: Registering IRQ for MT7530 internal PHYs

[Heiner Kallweit]

On 30.12.2020 17:15, Florian Fainelli wrote:
> 
> 
> On 12/30/2020 1:12 AM, Heiner Kallweit wrote:
>> On 30.12.2020 10:07, DENG Qingfang wrote:
>>> Hi Heiner,
>>> Thanks for your reply.
>>>
>>> On Wed, Dec 30, 2020 at 3:39 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>>> I don't think that's the best option.
>>>
>>> I'm well aware of that.
>>>
>>>> You may want to add a PHY driver for your chip. Supposedly it
>>>> supports at least PHY suspend/resume. You can use the RTL8366RB
>>>> PHY driver as template.
>>>
>>> There's no MediaTek PHY driver yet. Do we really need a new one just
>>> for the interrupts?
>>>
>> Not only for the interrupts. The genphy driver e.g. doesn't support
>> PHY suspend/resume. And the PHY driver needs basically no code,
>> just set the proper callbacks.
> 
> That statement about not supporting suspend/resume is not exactly true,
> the generic "1g" PHY driver only implements suspend/resume through the
> use of the standard BMCR power down bit, but not anything more
> complicated than that.
> 
Oh, right. Somehow I had in the back of my mind that the genphy driver
has no suspend/resume callbacks set.

> Interrupt handling within the PHY itself is not defined by the existing
> standard registers and will typically not reside in a standard register
> space either, so just for that reason you do need a custom PHY driver.
> There are other advantages if you need to expose additional PHY features
> down the road like PHY counters, energy detection, automatic power down etc.
> 
> I don't believe we will see discrete/standalone Mediatek PHY chips, but
> if that happens, then you would already have a framework for supporting
> them.
> 

Re: Registering IRQ for MT7530 internal PHYs

[Marc Zyngier]

Hi Qingfang,

On 2020-12-30 04:22, DENG Qingfang wrote:
> Hi,
> 
> I added MT7530 IRQ support and registered its internal PHYs to IRQ.
> It works but my patch used two hacks.
> 
> 1. Removed phy_drv_supports_irq check, because config_intr and
> handle_interrupt are not set for Generic PHY.
> 
> 2. Allocated ds->slave_mii_bus before calling ds->ops->setup, because
> we cannot call dsa_slave_mii_bus_init which is private.
> 
> Any better ideas?
> 
> Regards,
> Qingfang
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index a67cac15a724..d59a8c50ede3 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>
> @@ -1639,6 +1640,125 @@ mtk_get_tag_protocol(struct dsa_switch *ds, int 
> port,
>  	}
>  }
> 
> +static irqreturn_t
> +mt7530_irq(int irq, void *data)
> +{
> +	struct mt7530_priv *priv = data;
> +	bool handled = false;
> +	int phy;
> +	u32 val;
> +
> +	val = mt7530_read(priv, MT7530_SYS_INT_STS);
> +	mt7530_write(priv, MT7530_SYS_INT_STS, val);

If that is an ack operation, it should be dealt with as such in
an irqchip callback instead of being open-coded here.

> +
> +	dev_info_ratelimited(priv->dev, "interrupt status: 0x%08x\n", val);
> +	dev_info_ratelimited(priv->dev, "interrupt enable: 0x%08x\n",
> mt7530_read(priv, MT7530_SYS_INT_EN));
> +

I don't think printing these from an interrupt handler is a good idea.
Use the debug primitives if you really want these messages.

> +	for (phy = 0; phy < MT7530_NUM_PHYS; phy++) {
> +		if (val & BIT(phy)) {
> +			unsigned int child_irq;
> +
> +			child_irq = irq_find_mapping(priv->irq_domain, phy);
> +			handle_nested_irq(child_irq);
> +			handled = true;
> +		}
> +	}
> +
> +	return handled ? IRQ_HANDLED : IRQ_NONE;
> +}

What is the reason for not implementing this as a chained interrupt 
flow?

> +
> +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(&priv->reg_mutex);

Are you always guaranteed to be in a thread context? I guess that
is the case, given that you request a threaded interrupt, but
it would be worth documenting.

> +}
> +
> +static void mt7530_irq_bus_sync_unlock(struct irq_data *d)
> +{
> +	struct mt7530_priv *priv = irq_data_get_irq_chip_data(d);
> +
> +	mt7530_write(priv, MT7530_SYS_INT_EN, priv->irq_enable);
> +	mutex_unlock(&priv->reg_mutex);
> +}
> +
> +static struct irq_chip mt7530_irq_chip = {
> +	.name = MT7530_NAME,
> +	.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_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_irq_init(struct mt7530_priv *priv)
> +{
> +	struct mii_bus *bus = priv->ds->slave_mii_bus;
> +	struct device *dev = priv->dev;
> +	struct device_node *np = dev->of_node;
> +	int parent_irq;
> +	int phy, ret;
> +
> +	parent_irq = of_irq_get(np, 0);
> +	if (parent_irq <= 0) {
> +		dev_err(dev, "failed to get parent IRQ: %d\n", parent_irq);
> +		return;

It seems odd not to propagate the error, since I assume the device will
not be functional.

> +	}
> +
> +	mt7530_set(priv, MT7530_TOP_SIG_CTRL, TOP_SIG_CTRL_NORMAL);
> +	ret = devm_request_threaded_irq(dev, parent_irq, NULL, mt7530_irq,
> +					IRQF_ONESHOT, MT7530_NAME, priv);
> +	if (ret) {
> +		dev_err(dev, "failed to request IRQ: %d\n", ret);
> +		return;
> +	}
> +
> +	priv->irq_domain = irq_domain_add_linear(np, MT7530_NUM_PHYS,
> +						&mt7530_irq_domain_ops, priv);

The creation order is... interesting. You have a handler ready to fire,
nothing seems to initialise the HW state, and the priv data structure
is not fully populated. If any interrupt is pending at this stage,
you have a panic in your hands.

> +	if (!priv->irq_domain) {
> +		dev_err(dev, "failed to create IRQ domain\n");
> +		return;
> +	}
> +
> +	/* IRQ for internal PHYs */
> +	for (phy = 0; phy < MT7530_NUM_PHYS; phy++) {
> +		unsigned int irq = irq_create_mapping(priv->irq_domain, phy);

Why are you eagerly creating all the interrupt mappings? They should be
created on demand as the endpoint drivers come up.

> +
> +		irq_set_parent(irq, parent_irq);
> +		bus->irq[phy] = irq;
> +	}
> +}
> +
>  static int
>  mt7530_setup(struct dsa_switch *ds)
>  {
> @@ -2578,8 +2698,13 @@ static int
>  mt753x_setup(struct dsa_switch *ds)
>  {
>  	struct mt7530_priv *priv = ds->priv;
> +	int ret =  priv->info->sw_setup(ds);
> 
> -	return priv->info->sw_setup(ds);
> +	/* Setup interrupt */
> +	if (!ret)
> +		mt7530_irq_init(priv);
> +
> +	return ret;
>  }
> 
>  static int
> @@ -2780,6 +2905,9 @@ mt7530_remove(struct mdio_device *mdiodev)
>  		dev_err(priv->dev, "Failed to disable io pwr: %d\n",
>  			ret);
> 
> +	if (priv->irq_domain)
> +		irq_domain_remove(priv->irq_domain);

See the comment in front of irq_domain_remove() about the need
to discard the mappings prior to removing a domain.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: Registering IRQ for MT7530 internal PHYs

[Andrew Lunn]

> > +static void mt7530_irq_bus_lock(struct irq_data *d)
> > +{
> > +	struct mt7530_priv *priv = irq_data_get_irq_chip_data(d);
> > +
> > +	mutex_lock(&priv->reg_mutex);
> 
> Are you always guaranteed to be in a thread context? I guess that
> is the case, given that you request a threaded interrupt, but
> it would be worth documenting.

Hi Marc

These Ethernet switches are often connected by MDIO, SPI or I2C busses
to the SoC. So in order to access switch registers over these busses,
sleeping is required. So yes, threaded interrupts are required.

	 Andrew

Re: Registering IRQ for MT7530 internal PHYs

[Andrew Lunn]

On Wed, Dec 30, 2020 at 09:42:09AM +0000, Marc Zyngier wrote:
> > +static irqreturn_t
> > +mt7530_irq(int irq, void *data)
> > +{
> > +	struct mt7530_priv *priv = data;
> > +	bool handled = false;
> > +	int phy;
> > +	u32 val;
> > +
> > +	val = mt7530_read(priv, MT7530_SYS_INT_STS);
> > +	mt7530_write(priv, MT7530_SYS_INT_STS, val);
> 
> If that is an ack operation, it should be dealt with as such in
> an irqchip callback instead of being open-coded here.

Hi Qingfang

Does the PHY itself have interrupt control and status registers?

My experience with the Marvell Switch and its embedded PHYs is that
the PHYs are just the same as the discrete PHYs. There are bits to
enable different interrupts, and there are status bits indicating what
event caused the interrupt. Clearing the interrupt in the PHY clears
the interrupt in the switch interrupt controller. So in the mv88e6xxx
interrupt code, you see i do a read of the switch interrupt controller
status register, but i don't write to it as you have done.

       Andrew

Re: Registering IRQ for MT7530 internal PHYs

[DENG Qingfang]

Hi Andrew,

On Wed, Dec 30, 2020 at 11:23 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Wed, Dec 30, 2020 at 09:42:09AM +0000, Marc Zyngier wrote:
> > > +static irqreturn_t
> > > +mt7530_irq(int irq, void *data)
> > > +{
> > > +   struct mt7530_priv *priv = data;
> > > +   bool handled = false;
> > > +   int phy;
> > > +   u32 val;
> > > +
> > > +   val = mt7530_read(priv, MT7530_SYS_INT_STS);
> > > +   mt7530_write(priv, MT7530_SYS_INT_STS, val);
> >
> > If that is an ack operation, it should be dealt with as such in
> > an irqchip callback instead of being open-coded here.
>
> Hi Qingfang
>
> Does the PHY itself have interrupt control and status registers?

MT7531's internal PHY has an interrupt status register, but I don't
know if the same applies to MT7530.

>
> My experience with the Marvell Switch and its embedded PHYs is that
> the PHYs are just the same as the discrete PHYs. There are bits to
> enable different interrupts, and there are status bits indicating what
> event caused the interrupt. Clearing the interrupt in the PHY clears
> the interrupt in the switch interrupt controller. So in the mv88e6xxx
> interrupt code, you see i do a read of the switch interrupt controller
> status register, but i don't write to it as you have done.
>
>        Andrew

Re: Registering IRQ for MT7530 internal PHYs

[Landen Chao]

Hi Qingfang,
On Wed, 2021-01-06 at 16:54 +0800, DENG Qingfang wrote:
> Hi Andrew,
> 
> On Wed, Dec 30, 2020 at 11:23 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Wed, Dec 30, 2020 at 09:42:09AM +0000, Marc Zyngier wrote:
> > > > +static irqreturn_t
> > > > +mt7530_irq(int irq, void *data)
> > > > +{
> > > > +   struct mt7530_priv *priv = data;
> > > > +   bool handled = false;
> > > > +   int phy;
> > > > +   u32 val;
> > > > +
> > > > +   val = mt7530_read(priv, MT7530_SYS_INT_STS);
> > > > +   mt7530_write(priv, MT7530_SYS_INT_STS, val);
> > >
> > > If that is an ack operation, it should be dealt with as such in
> > > an irqchip callback instead of being open-coded here.
> >
> > Hi Qingfang
> >
> > Does the PHY itself have interrupt control and status registers?
> 
> MT7531's internal PHY has an interrupt status register, but I don't
> know if the same applies to MT7530.
Interrupt status/mask registers of MT7530 internal PHY is the same as
MT7531. The switch interrupt status register MT7530_SYS_INT_STS[14:8]
reflects internal PHY interrupt status. MT7530_SYS_INT_STS[6:0] we used
before does not related to internal PHY "interrupt".

However, base on MT753x hardware behavior, after read-clear interrupt
status of internal phy, we still need to write-clear
MT7530_SYS_INT_STS[14:8] to clear switch interrupt.

Landen
> 
> >
> > My experience with the Marvell Switch and its embedded PHYs is that
> > the PHYs are just the same as the discrete PHYs. There are bits to
> > enable different interrupts, and there are status bits indicating what
> > event caused the interrupt. Clearing the interrupt in the PHY clears
> > the interrupt in the switch interrupt controller. So in the mv88e6xxx
> > interrupt code, you see i do a read of the switch interrupt controller
> > status register, but i don't write to it as you have done.
> >
> >        Andrew

Re: Registering IRQ for MT7530 internal PHYs

[Andrew Lunn]

> 2. Allocated ds->slave_mii_bus before calling ds->ops->setup, because
> we cannot call dsa_slave_mii_bus_init which is private.
> 
> Any better ideas?

Do what mv88e6xxx does, allocate the MDIO bus inside the switch
driver.

	Andrew

Re: Registering IRQ for MT7530 internal PHYs

[Florian Fainelli]

On 12/30/2020 7:16 AM, Andrew Lunn wrote:
>> 2. Allocated ds->slave_mii_bus before calling ds->ops->setup, because
>> we cannot call dsa_slave_mii_bus_init which is private.
>>
>> Any better ideas?
> 
> Do what mv88e6xxx does, allocate the MDIO bus inside the switch
> driver.

Yes, exactly, or you could add additional hooks to allow intercepting
the initialization and de-initialization of the ds->slave_mii_bus,
something like this:

https://github.com/ffainelli/linux/commit/758da087a819cd1a284de074ea7d8eae9f875f0b

which was part of a larger series adding threaded IRQ support to the b53
driver:

https://github.com/ffainelli/linux/commits/b53-irq
-- 
Florian