netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Registering IRQ for MT7530 internal PHYs
@ 2020-12-30  4:22 DENG Qingfang
  2020-12-30  7:39 ` Heiner Kallweit
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: DENG Qingfang @ 2020-12-30  4:22 UTC (permalink / raw)
  To: David S. Miller, Andrew Lunn, Florian Fainelli, Heiner Kallweit,
	Jakub Kicinski, Landen Chao, Marc Zyngier, Matthias Brugger,
	Philipp Zabel, Russell King, Sean Wang, Thomas Gleixner,
	Vivien Didelot, Vladimir Oltean, linux-kernel, netdev
  Cc: Weijie Gao, Chuanhong Guo, Linus Walleij, René van Dorst

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: Registering IRQ for MT7530 internal PHYs
  2020-12-30  4:22 Registering IRQ for MT7530 internal PHYs DENG Qingfang
@ 2020-12-30  7:39 ` Heiner Kallweit
  2020-12-30  9:07   ` DENG Qingfang
  2020-12-30  9:42 ` Marc Zyngier
  2020-12-30 15:16 ` Andrew Lunn
  2 siblings, 1 reply; 13+ messages in thread
From: Heiner Kallweit @ 2020-12-30  7:39 UTC (permalink / raw)
  To: DENG Qingfang, David S. Miller, Andrew Lunn, Florian Fainelli,
	Jakub Kicinski, Landen Chao, Marc Zyngier, Matthias Brugger,
	Philipp Zabel, Russell King, Sean Wang, Thomas Gleixner,
	Vivien Didelot, Vladimir Oltean, linux-kernel, netdev
  Cc: Weijie Gao, Chuanhong Guo, Linus Walleij, René van Dorst

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Registering IRQ for MT7530 internal PHYs
  2020-12-30  7:39 ` Heiner Kallweit
@ 2020-12-30  9:07   ` DENG Qingfang
  2020-12-30  9:12     ` Heiner Kallweit
  0 siblings, 1 reply; 13+ messages in thread
From: DENG Qingfang @ 2020-12-30  9:07 UTC (permalink / raw)
  To: Heiner Kallweit
  Cc: David S. Miller, Andrew Lunn, Florian Fainelli, Jakub Kicinski,
	Landen Chao, Marc Zyngier, Matthias Brugger, Philipp Zabel,
	Russell King, Sean Wang, Thomas Gleixner, Vivien Didelot,
	Vladimir Oltean, linux-kernel, netdev, Weijie Gao, Chuanhong Guo,
	Linus Walleij, René van Dorst

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Registering IRQ for MT7530 internal PHYs
  2020-12-30  9:07   ` DENG Qingfang
@ 2020-12-30  9:12     ` Heiner Kallweit
  2020-12-30 16:15       ` Florian Fainelli
  0 siblings, 1 reply; 13+ messages in thread
From: Heiner Kallweit @ 2020-12-30  9:12 UTC (permalink / raw)
  To: DENG Qingfang
  Cc: David S. Miller, Andrew Lunn, Florian Fainelli, Jakub Kicinski,
	Landen Chao, Marc Zyngier, Matthias Brugger, Philipp Zabel,
	Russell King, Sean Wang, Thomas Gleixner, Vivien Didelot,
	Vladimir Oltean, linux-kernel, netdev, Weijie Gao, Chuanhong Guo,
	Linus Walleij, René van Dorst

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.
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Registering IRQ for MT7530 internal PHYs
  2020-12-30  4:22 Registering IRQ for MT7530 internal PHYs DENG Qingfang
  2020-12-30  7:39 ` Heiner Kallweit
@ 2020-12-30  9:42 ` Marc Zyngier
  2020-12-30 15:11   ` Andrew Lunn
  2020-12-30 15:23   ` Andrew Lunn
  2020-12-30 15:16 ` Andrew Lunn
  2 siblings, 2 replies; 13+ messages in thread
From: Marc Zyngier @ 2020-12-30  9:42 UTC (permalink / raw)
  To: DENG Qingfang
  Cc: David S. Miller, Andrew Lunn, Florian Fainelli, Heiner Kallweit,
	Jakub Kicinski, Landen Chao, Matthias Brugger, Philipp Zabel,
	Russell King, Sean Wang, Thomas Gleixner, Vivien Didelot,
	Vladimir Oltean, linux-kernel, netdev, Weijie Gao, Chuanhong Guo,
	Linus Walleij, René van Dorst

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...

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Registering IRQ for MT7530 internal PHYs
  2020-12-30  9:42 ` Marc Zyngier
@ 2020-12-30 15:11   ` Andrew Lunn
  2020-12-30 15:23   ` Andrew Lunn
  1 sibling, 0 replies; 13+ messages in thread
From: Andrew Lunn @ 2020-12-30 15:11 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: DENG Qingfang, David S. Miller, Florian Fainelli,
	Heiner Kallweit, Jakub Kicinski, Landen Chao, Matthias Brugger,
	Philipp Zabel, Russell King, Sean Wang, Thomas Gleixner,
	Vivien Didelot, Vladimir Oltean, linux-kernel, netdev,
	Weijie Gao, Chuanhong Guo, Linus Walleij, René van Dorst

> > +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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Registering IRQ for MT7530 internal PHYs
  2020-12-30  4:22 Registering IRQ for MT7530 internal PHYs DENG Qingfang
  2020-12-30  7:39 ` Heiner Kallweit
  2020-12-30  9:42 ` Marc Zyngier
@ 2020-12-30 15:16 ` Andrew Lunn
  2020-12-30 16:17   ` Florian Fainelli
  2 siblings, 1 reply; 13+ messages in thread
From: Andrew Lunn @ 2020-12-30 15:16 UTC (permalink / raw)
  To: DENG Qingfang
  Cc: David S. Miller, Florian Fainelli, Heiner Kallweit,
	Jakub Kicinski, Landen Chao, Marc Zyngier, Matthias Brugger,
	Philipp Zabel, Russell King, Sean Wang, Thomas Gleixner,
	Vivien Didelot, Vladimir Oltean, linux-kernel, netdev,
	Weijie Gao, Chuanhong Guo, Linus Walleij, René van Dorst

> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Registering IRQ for MT7530 internal PHYs
  2020-12-30  9:42 ` Marc Zyngier
  2020-12-30 15:11   ` Andrew Lunn
@ 2020-12-30 15:23   ` Andrew Lunn
  2021-01-06  8:54     ` DENG Qingfang
  1 sibling, 1 reply; 13+ messages in thread
From: Andrew Lunn @ 2020-12-30 15:23 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: DENG Qingfang, David S. Miller, Florian Fainelli,
	Heiner Kallweit, Jakub Kicinski, Landen Chao, Matthias Brugger,
	Philipp Zabel, Russell King, Sean Wang, Thomas Gleixner,
	Vivien Didelot, Vladimir Oltean, linux-kernel, netdev,
	Weijie Gao, Chuanhong Guo, Linus Walleij, René van Dorst

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Registering IRQ for MT7530 internal PHYs
  2020-12-30  9:12     ` Heiner Kallweit
@ 2020-12-30 16:15       ` Florian Fainelli
  2020-12-30 16:53         ` Heiner Kallweit
  0 siblings, 1 reply; 13+ messages in thread
From: Florian Fainelli @ 2020-12-30 16:15 UTC (permalink / raw)
  To: Heiner Kallweit, DENG Qingfang
  Cc: David S. Miller, Andrew Lunn, Jakub Kicinski, Landen Chao,
	Marc Zyngier, Matthias Brugger, Philipp Zabel, Russell King,
	Sean Wang, Thomas Gleixner, Vivien Didelot, Vladimir Oltean,
	linux-kernel, netdev, Weijie Gao, Chuanhong Guo, Linus Walleij,
	René van Dorst



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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Registering IRQ for MT7530 internal PHYs
  2020-12-30 15:16 ` Andrew Lunn
@ 2020-12-30 16:17   ` Florian Fainelli
  0 siblings, 0 replies; 13+ messages in thread
From: Florian Fainelli @ 2020-12-30 16:17 UTC (permalink / raw)
  To: Andrew Lunn, DENG Qingfang
  Cc: David S. Miller, Heiner Kallweit, Jakub Kicinski, Landen Chao,
	Marc Zyngier, Matthias Brugger, Philipp Zabel, Russell King,
	Sean Wang, Thomas Gleixner, Vivien Didelot, Vladimir Oltean,
	linux-kernel, netdev, Weijie Gao, Chuanhong Guo, Linus Walleij,
	René van Dorst



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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Registering IRQ for MT7530 internal PHYs
  2020-12-30 16:15       ` Florian Fainelli
@ 2020-12-30 16:53         ` Heiner Kallweit
  0 siblings, 0 replies; 13+ messages in thread
From: Heiner Kallweit @ 2020-12-30 16:53 UTC (permalink / raw)
  To: Florian Fainelli, DENG Qingfang
  Cc: David S. Miller, Andrew Lunn, Jakub Kicinski, Landen Chao,
	Marc Zyngier, Matthias Brugger, Philipp Zabel, Russell King,
	Sean Wang, Thomas Gleixner, Vivien Didelot, Vladimir Oltean,
	linux-kernel, netdev, Weijie Gao, Chuanhong Guo, Linus Walleij,
	René van Dorst

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.
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Registering IRQ for MT7530 internal PHYs
  2020-12-30 15:23   ` Andrew Lunn
@ 2021-01-06  8:54     ` DENG Qingfang
  2021-01-18 13:27       ` Landen Chao
  0 siblings, 1 reply; 13+ messages in thread
From: DENG Qingfang @ 2021-01-06  8:54 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Marc Zyngier, David S. Miller, Florian Fainelli, Heiner Kallweit,
	Jakub Kicinski, Landen Chao, Matthias Brugger, Philipp Zabel,
	Russell King, Sean Wang, Thomas Gleixner, Vivien Didelot,
	Vladimir Oltean, linux-kernel, netdev, Weijie Gao, Chuanhong Guo,
	Linus Walleij, René van Dorst

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Registering IRQ for MT7530 internal PHYs
  2021-01-06  8:54     ` DENG Qingfang
@ 2021-01-18 13:27       ` Landen Chao
  0 siblings, 0 replies; 13+ messages in thread
From: Landen Chao @ 2021-01-18 13:27 UTC (permalink / raw)
  To: DENG Qingfang
  Cc: Andrew Lunn, Marc Zyngier, David S. Miller, Florian Fainelli,
	Heiner Kallweit, Jakub Kicinski, Matthias Brugger, Philipp Zabel,
	Russell King, Sean Wang, Thomas Gleixner, Vivien Didelot,
	Vladimir Oltean, linux-kernel, netdev,
	Weijie Gao (高惟杰),
	Chuanhong Guo, Linus Walleij, René van Dorst

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-01-18 13:56 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-30  4:22 Registering IRQ for MT7530 internal PHYs DENG Qingfang
2020-12-30  7:39 ` Heiner Kallweit
2020-12-30  9:07   ` DENG Qingfang
2020-12-30  9:12     ` Heiner Kallweit
2020-12-30 16:15       ` Florian Fainelli
2020-12-30 16:53         ` Heiner Kallweit
2020-12-30  9:42 ` Marc Zyngier
2020-12-30 15:11   ` Andrew Lunn
2020-12-30 15:23   ` Andrew Lunn
2021-01-06  8:54     ` DENG Qingfang
2021-01-18 13:27       ` Landen Chao
2020-12-30 15:16 ` Andrew Lunn
2020-12-30 16:17   ` Florian Fainelli

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).