[PATCH v3 0/2] gpio: mlxbf2: Introduce proper interrupt handling

[PATCH v3 0/2] gpio: mlxbf2: Introduce proper interrupt handling

[Asmaa Mnebhi]

This is a follow up on a discussion regarding
proper handling of GPIO interrupts within the
gpio-mlxbf2.c driver.

Link to discussion:
https://lore.kernel.org/netdev/20210816115953.72533-7-andriy.shevchenko@linux.intel.com/T/

Patch 1 adds support to a GPIO IRQ handler in gpio-mlxbf2.c.
Patch 2 is a follow up removal of custom GPIO IRQ handling
from the mlxbf_gige driver and replacing it with a simple
IRQ request. The ACPI table for the mlxbf_gige driver is
responsible for instantiating the PHY GPIO interrupt via
GpioInt.

Andy Shevchenko, could you please review this patch series.

v3 vs. v2 patch:
- Add IRQ_TYPE_LEVEL* back to mlxbf2_gpio_irq_set_type.
  YU_GPIO_CAUSE_FALL_EN and YU_GPIO_CAUSE_RISE_EN
  are configured in Both level and edge interrupts cases.

Asmaa Mnebhi (2):
  gpio: mlxbf2: Introduce IRQ support
  net: mellanox: mlxbf_gige: Replace non-standard interrupt handling

 drivers/gpio/gpio-mlxbf2.c                    | 150 ++++++++++++-
 .../net/ethernet/mellanox/mlxbf_gige/Makefile |   1 -
 .../ethernet/mellanox/mlxbf_gige/mlxbf_gige.h |  12 -
 .../mellanox/mlxbf_gige/mlxbf_gige_gpio.c     | 212 ------------------
 .../mellanox/mlxbf_gige/mlxbf_gige_main.c     |  22 +-
 5 files changed, 157 insertions(+), 240 deletions(-)
 delete mode 100644 drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_gpio.c

-- 
2.30.1

[PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support

[Asmaa Mnebhi]

Introduce standard IRQ handling in the gpio-mlxbf2.c
driver.

Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
---
 drivers/gpio/gpio-mlxbf2.c | 150 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 148 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-mlxbf2.c b/drivers/gpio/gpio-mlxbf2.c
index 177d03ef4529..21c53d18ecd3 100644
--- a/drivers/gpio/gpio-mlxbf2.c
+++ b/drivers/gpio/gpio-mlxbf2.c
@@ -1,9 +1,14 @@
 // SPDX-License-Identifier: GPL-2.0
 
+/*
+ * Copyright (C) 2020-2021 NVIDIA CORPORATION & AFFILIATES
+ */
+
 #include <linux/bitfield.h>
 #include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/kernel.h>
@@ -43,9 +48,14 @@
 #define YU_GPIO_MODE0			0x0c
 #define YU_GPIO_DATASET			0x14
 #define YU_GPIO_DATACLEAR		0x18
+#define YU_GPIO_CAUSE_RISE_EN		0x44
+#define YU_GPIO_CAUSE_FALL_EN		0x48
 #define YU_GPIO_MODE1_CLEAR		0x50
 #define YU_GPIO_MODE0_SET		0x54
 #define YU_GPIO_MODE0_CLEAR		0x58
+#define YU_GPIO_CAUSE_OR_CAUSE_EVTEN0	0x80
+#define YU_GPIO_CAUSE_OR_EVTEN0		0x94
+#define YU_GPIO_CAUSE_OR_CLRCAUSE	0x98
 
 struct mlxbf2_gpio_context_save_regs {
 	u32 gpio_mode0;
@@ -55,6 +65,7 @@ struct mlxbf2_gpio_context_save_regs {
 /* BlueField-2 gpio block context structure. */
 struct mlxbf2_gpio_context {
 	struct gpio_chip gc;
+	struct irq_chip irq_chip;
 
 	/* YU GPIO blocks address */
 	void __iomem *gpio_io;
@@ -218,15 +229,117 @@ static int mlxbf2_gpio_direction_output(struct gpio_chip *chip,
 	return ret;
 }
 
+static void mlxbf2_gpio_irq_enable(struct irq_data *irqd)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct mlxbf2_gpio_context *gs = gpiochip_get_data(gc);
+	int offset = irqd_to_hwirq(irqd);
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
+	val = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_CLRCAUSE);
+	val |= BIT(offset);
+	writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_CLRCAUSE);
+
+	val = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0);
+	val |= BIT(offset);
+	writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0);
+	spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
+}
+
+static void mlxbf2_gpio_irq_disable(struct irq_data *irqd)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct mlxbf2_gpio_context *gs = gpiochip_get_data(gc);
+	int offset = irqd_to_hwirq(irqd);
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
+	val = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0);
+	val &= ~BIT(offset);
+	writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0);
+	spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
+}
+
+static irqreturn_t mlxbf2_gpio_irq_handler(int irq, void *ptr)
+{
+	struct mlxbf2_gpio_context *gs = ptr;
+	struct gpio_chip *gc = &gs->gc;
+	unsigned long pending;
+	u32 level;
+
+	pending = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_CAUSE_EVTEN0);
+	writel(pending, gs->gpio_io + YU_GPIO_CAUSE_OR_CLRCAUSE);
+
+	for_each_set_bit(level, &pending, gc->ngpio) {
+		int gpio_irq = irq_find_mapping(gc->irq.domain, level);
+		generic_handle_irq(gpio_irq);
+	}
+
+	return IRQ_RETVAL(pending);
+}
+
+static int
+mlxbf2_gpio_irq_set_type(struct irq_data *irqd, unsigned int type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct mlxbf2_gpio_context *gs = gpiochip_get_data(gc);
+	int offset = irqd_to_hwirq(irqd);
+	unsigned long flags;
+	bool fall = false;
+	bool rise = false;
+	u32 val;
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_BOTH:
+	case IRQ_TYPE_LEVEL_MASK:
+		fall = true;
+		rise = true;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+	case IRQ_TYPE_LEVEL_HIGH:
+		rise = true;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+	case IRQ_TYPE_LEVEL_LOW:
+		fall = true;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
+	if (fall) {
+		val = readl(gs->gpio_io + YU_GPIO_CAUSE_FALL_EN);
+		val |= BIT(offset);
+		writel(val, gs->gpio_io + YU_GPIO_CAUSE_FALL_EN);
+	}
+
+	if (rise) {
+		val = readl(gs->gpio_io + YU_GPIO_CAUSE_RISE_EN);
+		val |= BIT(offset);
+		writel(val, gs->gpio_io + YU_GPIO_CAUSE_RISE_EN);
+	}
+	spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
+
+	return 0;
+}
+
 /* BlueField-2 GPIO driver initialization routine. */
 static int
 mlxbf2_gpio_probe(struct platform_device *pdev)
 {
 	struct mlxbf2_gpio_context *gs;
 	struct device *dev = &pdev->dev;
+	struct gpio_irq_chip *girq;
 	struct gpio_chip *gc;
 	unsigned int npins;
-	int ret;
+	const char *name;
+	int ret, irq;
+
+	name = dev_name(dev);
 
 	gs = devm_kzalloc(dev, sizeof(*gs), GFP_KERNEL);
 	if (!gs)
@@ -256,11 +369,44 @@ mlxbf2_gpio_probe(struct platform_device *pdev)
 			NULL,
 			0);
 
+	if (ret) {
+		dev_err(dev, "bgpio_init failed\n");
+		return ret;
+	}
+
 	gc->direction_input = mlxbf2_gpio_direction_input;
 	gc->direction_output = mlxbf2_gpio_direction_output;
 	gc->ngpio = npins;
 	gc->owner = THIS_MODULE;
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq >= 0) {
+		gs->irq_chip.name = name;
+		gs->irq_chip.irq_set_type = mlxbf2_gpio_irq_set_type;
+		gs->irq_chip.irq_enable = mlxbf2_gpio_irq_enable;
+		gs->irq_chip.irq_disable = mlxbf2_gpio_irq_disable;
+
+		girq = &gs->gc.irq;
+		girq->chip = &gs->irq_chip;
+		girq->handler = handle_simple_irq;
+		girq->default_type = IRQ_TYPE_NONE;
+		/* This will let us handle the parent IRQ in the driver */
+		girq->num_parents = 0;
+		girq->parents = NULL;
+		girq->parent_handler = NULL;
+
+		/*
+		 * Directly request the irq here instead of passing
+		 * a flow-handler because the irq is shared.
+		 */
+		ret = devm_request_irq(dev, irq, mlxbf2_gpio_irq_handler,
+				       IRQF_SHARED, name, gs);
+		if (ret) {
+			dev_err(dev, "failed to request IRQ");
+			return ret;
+		}
+	}
+
 	platform_set_drvdata(pdev, gs);
 
 	ret = devm_gpiochip_add_data(dev, &gs->gc, gs);
@@ -315,5 +461,5 @@ static struct platform_driver mlxbf2_gpio_driver = {
 module_platform_driver(mlxbf2_gpio_driver);
 
 MODULE_DESCRIPTION("Mellanox BlueField-2 GPIO Driver");
-MODULE_AUTHOR("Mellanox Technologies");
+MODULE_AUTHOR("Asmaa Mnebhi <asmaa@nvidia.com>");
 MODULE_LICENSE("GPL v2");
-- 
2.30.1

[PATCH v2 2/2] net: mellanox: mlxbf_gige: Replace non-standard interrupt handling

[Asmaa Mnebhi]

Since the GPIO driver (gpio-mlxbf2.c) supports interrupt
handling, replace the custom routine with simple IRQ
request.

Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
---
 .../net/ethernet/mellanox/mlxbf_gige/Makefile |   1 -
 .../ethernet/mellanox/mlxbf_gige/mlxbf_gige.h |  12 -
 .../mellanox/mlxbf_gige/mlxbf_gige_gpio.c     | 212 ------------------
 .../mellanox/mlxbf_gige/mlxbf_gige_main.c     |  22 +-
 4 files changed, 9 insertions(+), 238 deletions(-)
 delete mode 100644 drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_gpio.c

diff --git a/drivers/net/ethernet/mellanox/mlxbf_gige/Makefile b/drivers/net/ethernet/mellanox/mlxbf_gige/Makefile
index e57c1375f236..a97c2bef846b 100644
--- a/drivers/net/ethernet/mellanox/mlxbf_gige/Makefile
+++ b/drivers/net/ethernet/mellanox/mlxbf_gige/Makefile
@@ -3,7 +3,6 @@
 obj-$(CONFIG_MLXBF_GIGE) += mlxbf_gige.o
 
 mlxbf_gige-y := mlxbf_gige_ethtool.o \
-		mlxbf_gige_gpio.o \
 		mlxbf_gige_intr.o \
 		mlxbf_gige_main.o \
 		mlxbf_gige_mdio.o \
diff --git a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige.h b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige.h
index e3509e69ed1c..86826a70f9dd 100644
--- a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige.h
+++ b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige.h
@@ -51,11 +51,6 @@
 #define MLXBF_GIGE_ERROR_INTR_IDX       0
 #define MLXBF_GIGE_RECEIVE_PKT_INTR_IDX 1
 #define MLXBF_GIGE_LLU_PLU_INTR_IDX     2
-#define MLXBF_GIGE_PHY_INT_N            3
-
-#define MLXBF_GIGE_MDIO_DEFAULT_PHY_ADDR 0x3
-
-#define MLXBF_GIGE_DEFAULT_PHY_INT_GPIO 12
 
 struct mlxbf_gige_stats {
 	u64 hw_access_errors;
@@ -81,11 +76,7 @@ struct mlxbf_gige {
 	struct platform_device *pdev;
 	void __iomem *mdio_io;
 	struct mii_bus *mdiobus;
-	void __iomem *gpio_io;
-	struct irq_domain *irqdomain;
-	u32 phy_int_gpio_mask;
 	spinlock_t lock;      /* for packet processing indices */
-	spinlock_t gpio_lock; /* for GPIO bus access */
 	u16 rx_q_entries;
 	u16 tx_q_entries;
 	u64 *tx_wqe_base;
@@ -184,7 +175,4 @@ int mlxbf_gige_poll(struct napi_struct *napi, int budget);
 extern const struct ethtool_ops mlxbf_gige_ethtool_ops;
 void mlxbf_gige_update_tx_wqe_next(struct mlxbf_gige *priv);
 
-int mlxbf_gige_gpio_init(struct platform_device *pdev, struct mlxbf_gige *priv);
-void mlxbf_gige_gpio_free(struct mlxbf_gige *priv);
-
 #endif /* !defined(__MLXBF_GIGE_H__) */
diff --git a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_gpio.c b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_gpio.c
deleted file mode 100644
index a8d966db5715..000000000000
--- a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_gpio.c
+++ /dev/null
@@ -1,212 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only OR BSD-3-Clause
-
-/* Initialize and handle GPIO interrupt triggered by INT_N PHY signal.
- * This GPIO interrupt triggers the PHY state machine to bring the link
- * up/down.
- *
- * Copyright (C) 2021 NVIDIA CORPORATION & AFFILIATES
- */
-
-#include <linux/acpi.h>
-#include <linux/bitfield.h>
-#include <linux/device.h>
-#include <linux/err.h>
-#include <linux/gpio/driver.h>
-#include <linux/interrupt.h>
-#include <linux/io.h>
-#include <linux/irq.h>
-#include <linux/irqdomain.h>
-#include <linux/irqreturn.h>
-#include <linux/platform_device.h>
-#include <linux/property.h>
-
-#include "mlxbf_gige.h"
-#include "mlxbf_gige_regs.h"
-
-#define MLXBF_GIGE_GPIO_CAUSE_FALL_EN		0x48
-#define MLXBF_GIGE_GPIO_CAUSE_OR_CAUSE_EVTEN0	0x80
-#define MLXBF_GIGE_GPIO_CAUSE_OR_EVTEN0		0x94
-#define MLXBF_GIGE_GPIO_CAUSE_OR_CLRCAUSE	0x98
-
-static void mlxbf_gige_gpio_enable(struct mlxbf_gige *priv)
-{
-	unsigned long flags;
-	u32 val;
-
-	spin_lock_irqsave(&priv->gpio_lock, flags);
-	val = readl(priv->gpio_io + MLXBF_GIGE_GPIO_CAUSE_OR_CLRCAUSE);
-	val |= priv->phy_int_gpio_mask;
-	writel(val, priv->gpio_io + MLXBF_GIGE_GPIO_CAUSE_OR_CLRCAUSE);
-
-	/* The INT_N interrupt level is active low.
-	 * So enable cause fall bit to detect when GPIO
-	 * state goes low.
-	 */
-	val = readl(priv->gpio_io + MLXBF_GIGE_GPIO_CAUSE_FALL_EN);
-	val |= priv->phy_int_gpio_mask;
-	writel(val, priv->gpio_io + MLXBF_GIGE_GPIO_CAUSE_FALL_EN);
-
-	/* Enable PHY interrupt by setting the priority level */
-	val = readl(priv->gpio_io + MLXBF_GIGE_GPIO_CAUSE_OR_EVTEN0);
-	val |= priv->phy_int_gpio_mask;
-	writel(val, priv->gpio_io + MLXBF_GIGE_GPIO_CAUSE_OR_EVTEN0);
-	spin_unlock_irqrestore(&priv->gpio_lock, flags);
-}
-
-static void mlxbf_gige_gpio_disable(struct mlxbf_gige *priv)
-{
-	unsigned long flags;
-	u32 val;
-
-	spin_lock_irqsave(&priv->gpio_lock, flags);
-	val = readl(priv->gpio_io + MLXBF_GIGE_GPIO_CAUSE_OR_EVTEN0);
-	val &= ~priv->phy_int_gpio_mask;
-	writel(val, priv->gpio_io + MLXBF_GIGE_GPIO_CAUSE_OR_EVTEN0);
-	spin_unlock_irqrestore(&priv->gpio_lock, flags);
-}
-
-static irqreturn_t mlxbf_gige_gpio_handler(int irq, void *ptr)
-{
-	struct mlxbf_gige *priv;
-	u32 val;
-
-	priv = ptr;
-
-	/* Check if this interrupt is from PHY device.
-	 * Return if it is not.
-	 */
-	val = readl(priv->gpio_io + MLXBF_GIGE_GPIO_CAUSE_OR_CAUSE_EVTEN0);
-	if (!(val & priv->phy_int_gpio_mask))
-		return IRQ_NONE;
-
-	/* Clear interrupt when done, otherwise, no further interrupt
-	 * will be triggered.
-	 */
-	val = readl(priv->gpio_io + MLXBF_GIGE_GPIO_CAUSE_OR_CLRCAUSE);
-	val |= priv->phy_int_gpio_mask;
-	writel(val, priv->gpio_io + MLXBF_GIGE_GPIO_CAUSE_OR_CLRCAUSE);
-
-	generic_handle_irq(priv->phy_irq);
-
-	return IRQ_HANDLED;
-}
-
-static void mlxbf_gige_gpio_mask(struct irq_data *irqd)
-{
-	struct mlxbf_gige *priv = irq_data_get_irq_chip_data(irqd);
-
-	mlxbf_gige_gpio_disable(priv);
-}
-
-static void mlxbf_gige_gpio_unmask(struct irq_data *irqd)
-{
-	struct mlxbf_gige *priv = irq_data_get_irq_chip_data(irqd);
-
-	mlxbf_gige_gpio_enable(priv);
-}
-
-static struct irq_chip mlxbf_gige_gpio_chip = {
-	.name			= "mlxbf_gige_phy",
-	.irq_mask		= mlxbf_gige_gpio_mask,
-	.irq_unmask		= mlxbf_gige_gpio_unmask,
-};
-
-static int mlxbf_gige_gpio_domain_map(struct irq_domain *d,
-				      unsigned int irq,
-				      irq_hw_number_t hwirq)
-{
-	irq_set_chip_data(irq, d->host_data);
-	irq_set_chip_and_handler(irq, &mlxbf_gige_gpio_chip, handle_simple_irq);
-	irq_set_noprobe(irq);
-
-	return 0;
-}
-
-static const struct irq_domain_ops mlxbf_gige_gpio_domain_ops = {
-	.map    = mlxbf_gige_gpio_domain_map,
-	.xlate	= irq_domain_xlate_twocell,
-};
-
-#ifdef CONFIG_ACPI
-static int mlxbf_gige_gpio_resources(struct acpi_resource *ares,
-				     void *data)
-{
-	struct acpi_resource_gpio *gpio;
-	u32 *phy_int_gpio = data;
-
-	if (ares->type == ACPI_RESOURCE_TYPE_GPIO) {
-		gpio = &ares->data.gpio;
-		*phy_int_gpio = gpio->pin_table[0];
-	}
-
-	return 1;
-}
-#endif
-
-void mlxbf_gige_gpio_free(struct mlxbf_gige *priv)
-{
-	irq_dispose_mapping(priv->phy_irq);
-	irq_domain_remove(priv->irqdomain);
-}
-
-int mlxbf_gige_gpio_init(struct platform_device *pdev,
-			 struct mlxbf_gige *priv)
-{
-	struct device *dev = &pdev->dev;
-	struct resource *res;
-	u32 phy_int_gpio = 0;
-	int ret;
-
-	LIST_HEAD(resources);
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, MLXBF_GIGE_RES_GPIO0);
-	if (!res)
-		return -ENODEV;
-
-	priv->gpio_io = devm_ioremap(dev, res->start, resource_size(res));
-	if (!priv->gpio_io)
-		return -ENOMEM;
-
-#ifdef CONFIG_ACPI
-	ret = acpi_dev_get_resources(ACPI_COMPANION(dev),
-				     &resources, mlxbf_gige_gpio_resources,
-				     &phy_int_gpio);
-	acpi_dev_free_resource_list(&resources);
-	if (ret < 0 || !phy_int_gpio) {
-		dev_err(dev, "Error retrieving the gpio phy pin");
-		return -EINVAL;
-	}
-#endif
-
-	priv->phy_int_gpio_mask = BIT(phy_int_gpio);
-
-	mlxbf_gige_gpio_disable(priv);
-
-	priv->hw_phy_irq = platform_get_irq(pdev, MLXBF_GIGE_PHY_INT_N);
-
-	priv->irqdomain = irq_domain_add_simple(NULL, 1, 0,
-						&mlxbf_gige_gpio_domain_ops,
-						priv);
-	if (!priv->irqdomain) {
-		dev_err(dev, "Failed to add IRQ domain\n");
-		return -ENOMEM;
-	}
-
-	priv->phy_irq = irq_create_mapping(priv->irqdomain, 0);
-	if (!priv->phy_irq) {
-		irq_domain_remove(priv->irqdomain);
-		priv->irqdomain = NULL;
-		dev_err(dev, "Error mapping PHY IRQ\n");
-		return -EINVAL;
-	}
-
-	ret = devm_request_irq(dev, priv->hw_phy_irq, mlxbf_gige_gpio_handler,
-			       IRQF_ONESHOT | IRQF_SHARED, "mlxbf_gige_phy", priv);
-	if (ret) {
-		dev_err(dev, "Failed to request PHY IRQ");
-		mlxbf_gige_gpio_free(priv);
-		return ret;
-	}
-
-	return ret;
-}
diff --git a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
index 3e85b17f5857..4382ec8f7d64 100644
--- a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
+++ b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
@@ -273,8 +273,8 @@ static int mlxbf_gige_probe(struct platform_device *pdev)
 	void __iomem *llu_base;
 	void __iomem *plu_base;
 	void __iomem *base;
+	int addr, phy_irq;
 	u64 control;
-	int addr;
 	int err;
 
 	base = devm_platform_ioremap_resource(pdev, MLXBF_GIGE_RES_MAC);
@@ -309,20 +309,12 @@ static int mlxbf_gige_probe(struct platform_device *pdev)
 	priv->pdev = pdev;
 
 	spin_lock_init(&priv->lock);
-	spin_lock_init(&priv->gpio_lock);
 
 	/* Attach MDIO device */
 	err = mlxbf_gige_mdio_probe(pdev, priv);
 	if (err)
 		return err;
 
-	err = mlxbf_gige_gpio_init(pdev, priv);
-	if (err) {
-		dev_err(&pdev->dev, "PHY IRQ initialization failed\n");
-		mlxbf_gige_mdio_remove(priv);
-		return -ENODEV;
-	}
-
 	priv->base = base;
 	priv->llu_base = llu_base;
 	priv->plu_base = plu_base;
@@ -343,6 +335,12 @@ static int mlxbf_gige_probe(struct platform_device *pdev)
 	priv->rx_irq = platform_get_irq(pdev, MLXBF_GIGE_RECEIVE_PKT_INTR_IDX);
 	priv->llu_plu_irq = platform_get_irq(pdev, MLXBF_GIGE_LLU_PLU_INTR_IDX);
 
+	phy_irq = acpi_dev_gpio_irq_get_by(ACPI_COMPANION(&pdev->dev), "phy-gpios", 0);
+	if (phy_irq < 0) {
+		dev_err(&pdev->dev, "Error getting PHY irq. Use polling instead");
+		phy_irq = PHY_POLL;
+	}
+
 	phydev = phy_find_first(priv->mdiobus);
 	if (!phydev) {
 		err = -ENODEV;
@@ -350,8 +348,8 @@ static int mlxbf_gige_probe(struct platform_device *pdev)
 	}
 
 	addr = phydev->mdio.addr;
-	priv->mdiobus->irq[addr] = priv->phy_irq;
-	phydev->irq = priv->phy_irq;
+	priv->mdiobus->irq[addr] = phy_irq;
+	phydev->irq = phy_irq;
 
 	err = phy_connect_direct(netdev, phydev,
 				 mlxbf_gige_adjust_link,
@@ -387,7 +385,6 @@ static int mlxbf_gige_probe(struct platform_device *pdev)
 	return 0;
 
 out:
-	mlxbf_gige_gpio_free(priv);
 	mlxbf_gige_mdio_remove(priv);
 	return err;
 }
@@ -398,7 +395,6 @@ static int mlxbf_gige_remove(struct platform_device *pdev)
 
 	unregister_netdev(priv->netdev);
 	phy_disconnect(priv->netdev->phydev);
-	mlxbf_gige_gpio_free(priv);
 	mlxbf_gige_mdio_remove(priv);
 	platform_set_drvdata(pdev, NULL);
 
-- 
2.30.1

[PATCH v3 0/2] gpio: mlxbf2: Introduce proper interrupt handling

[Asmaa Mnebhi]

This is a follow up on a discussion regarding
proper handling of GPIO interrupts within the
gpio-mlxbf2.c driver.

Link to discussion:
https://lore.kernel.org/netdev/20210816115953.72533-7-andriy.shevchenko@linux.intel.com/T/

Patch 1 adds support to a GPIO IRQ handler in gpio-mlxbf2.c.
Patch 2 is a follow up removal of custom GPIO IRQ handling
from the mlxbf_gige driver and replacing it with a simple
IRQ request. The ACPI table for the mlxbf_gige driver is
responsible for instantiating the PHY GPIO interrupt via
GpioInt.

Andy Shevchenko, could you please review this patch series.

v3 vs. v2 patch:
- Add IRQ_TYPE_LEVEL* back to mlxbf2_gpio_irq_set_type.
  YU_GPIO_CAUSE_FALL_EN and YU_GPIO_CAUSE_RISE_EN
  are configured in Both level and edge interrupts cases.

Asmaa Mnebhi (2):
  gpio: mlxbf2: Introduce IRQ support
  net: mellanox: mlxbf_gige: Replace non-standard interrupt handling

 drivers/gpio/gpio-mlxbf2.c                    | 150 ++++++++++++-
 .../net/ethernet/mellanox/mlxbf_gige/Makefile |   1 -
 .../ethernet/mellanox/mlxbf_gige/mlxbf_gige.h |  12 -
 .../mellanox/mlxbf_gige/mlxbf_gige_gpio.c     | 212 ------------------
 .../mellanox/mlxbf_gige/mlxbf_gige_main.c     |  22 +-
 5 files changed, 157 insertions(+), 240 deletions(-)
 delete mode 100644 drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_gpio.c

-- 
2.30.1

[PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support

[Asmaa Mnebhi]

Introduce standard IRQ handling in the gpio-mlxbf2.c
driver.

Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
---
 drivers/gpio/gpio-mlxbf2.c | 150 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 148 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-mlxbf2.c b/drivers/gpio/gpio-mlxbf2.c
index 177d03ef4529..21c53d18ecd3 100644
--- a/drivers/gpio/gpio-mlxbf2.c
+++ b/drivers/gpio/gpio-mlxbf2.c
@@ -1,9 +1,14 @@
 // SPDX-License-Identifier: GPL-2.0
 
+/*
+ * Copyright (C) 2020-2021 NVIDIA CORPORATION & AFFILIATES
+ */
+
 #include <linux/bitfield.h>
 #include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/kernel.h>
@@ -43,9 +48,14 @@
 #define YU_GPIO_MODE0			0x0c
 #define YU_GPIO_DATASET			0x14
 #define YU_GPIO_DATACLEAR		0x18
+#define YU_GPIO_CAUSE_RISE_EN		0x44
+#define YU_GPIO_CAUSE_FALL_EN		0x48
 #define YU_GPIO_MODE1_CLEAR		0x50
 #define YU_GPIO_MODE0_SET		0x54
 #define YU_GPIO_MODE0_CLEAR		0x58
+#define YU_GPIO_CAUSE_OR_CAUSE_EVTEN0	0x80
+#define YU_GPIO_CAUSE_OR_EVTEN0		0x94
+#define YU_GPIO_CAUSE_OR_CLRCAUSE	0x98
 
 struct mlxbf2_gpio_context_save_regs {
 	u32 gpio_mode0;
@@ -55,6 +65,7 @@ struct mlxbf2_gpio_context_save_regs {
 /* BlueField-2 gpio block context structure. */
 struct mlxbf2_gpio_context {
 	struct gpio_chip gc;
+	struct irq_chip irq_chip;
 
 	/* YU GPIO blocks address */
 	void __iomem *gpio_io;
@@ -218,15 +229,117 @@ static int mlxbf2_gpio_direction_output(struct gpio_chip *chip,
 	return ret;
 }
 
+static void mlxbf2_gpio_irq_enable(struct irq_data *irqd)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct mlxbf2_gpio_context *gs = gpiochip_get_data(gc);
+	int offset = irqd_to_hwirq(irqd);
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
+	val = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_CLRCAUSE);
+	val |= BIT(offset);
+	writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_CLRCAUSE);
+
+	val = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0);
+	val |= BIT(offset);
+	writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0);
+	spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
+}
+
+static void mlxbf2_gpio_irq_disable(struct irq_data *irqd)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct mlxbf2_gpio_context *gs = gpiochip_get_data(gc);
+	int offset = irqd_to_hwirq(irqd);
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
+	val = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0);
+	val &= ~BIT(offset);
+	writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0);
+	spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
+}
+
+static irqreturn_t mlxbf2_gpio_irq_handler(int irq, void *ptr)
+{
+	struct mlxbf2_gpio_context *gs = ptr;
+	struct gpio_chip *gc = &gs->gc;
+	unsigned long pending;
+	u32 level;
+
+	pending = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_CAUSE_EVTEN0);
+	writel(pending, gs->gpio_io + YU_GPIO_CAUSE_OR_CLRCAUSE);
+
+	for_each_set_bit(level, &pending, gc->ngpio) {
+		int gpio_irq = irq_find_mapping(gc->irq.domain, level);
+		generic_handle_irq(gpio_irq);
+	}
+
+	return IRQ_RETVAL(pending);
+}
+
+static int
+mlxbf2_gpio_irq_set_type(struct irq_data *irqd, unsigned int type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct mlxbf2_gpio_context *gs = gpiochip_get_data(gc);
+	int offset = irqd_to_hwirq(irqd);
+	unsigned long flags;
+	bool fall = false;
+	bool rise = false;
+	u32 val;
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_BOTH:
+	case IRQ_TYPE_LEVEL_MASK:
+		fall = true;
+		rise = true;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+	case IRQ_TYPE_LEVEL_HIGH:
+		rise = true;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+	case IRQ_TYPE_LEVEL_LOW:
+		fall = true;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
+	if (fall) {
+		val = readl(gs->gpio_io + YU_GPIO_CAUSE_FALL_EN);
+		val |= BIT(offset);
+		writel(val, gs->gpio_io + YU_GPIO_CAUSE_FALL_EN);
+	}
+
+	if (rise) {
+		val = readl(gs->gpio_io + YU_GPIO_CAUSE_RISE_EN);
+		val |= BIT(offset);
+		writel(val, gs->gpio_io + YU_GPIO_CAUSE_RISE_EN);
+	}
+	spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
+
+	return 0;
+}
+
 /* BlueField-2 GPIO driver initialization routine. */
 static int
 mlxbf2_gpio_probe(struct platform_device *pdev)
 {
 	struct mlxbf2_gpio_context *gs;
 	struct device *dev = &pdev->dev;
+	struct gpio_irq_chip *girq;
 	struct gpio_chip *gc;
 	unsigned int npins;
-	int ret;
+	const char *name;
+	int ret, irq;
+
+	name = dev_name(dev);
 
 	gs = devm_kzalloc(dev, sizeof(*gs), GFP_KERNEL);
 	if (!gs)
@@ -256,11 +369,44 @@ mlxbf2_gpio_probe(struct platform_device *pdev)
 			NULL,
 			0);
 
+	if (ret) {
+		dev_err(dev, "bgpio_init failed\n");
+		return ret;
+	}
+
 	gc->direction_input = mlxbf2_gpio_direction_input;
 	gc->direction_output = mlxbf2_gpio_direction_output;
 	gc->ngpio = npins;
 	gc->owner = THIS_MODULE;
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq >= 0) {
+		gs->irq_chip.name = name;
+		gs->irq_chip.irq_set_type = mlxbf2_gpio_irq_set_type;
+		gs->irq_chip.irq_enable = mlxbf2_gpio_irq_enable;
+		gs->irq_chip.irq_disable = mlxbf2_gpio_irq_disable;
+
+		girq = &gs->gc.irq;
+		girq->chip = &gs->irq_chip;
+		girq->handler = handle_simple_irq;
+		girq->default_type = IRQ_TYPE_NONE;
+		/* This will let us handle the parent IRQ in the driver */
+		girq->num_parents = 0;
+		girq->parents = NULL;
+		girq->parent_handler = NULL;
+
+		/*
+		 * Directly request the irq here instead of passing
+		 * a flow-handler because the irq is shared.
+		 */
+		ret = devm_request_irq(dev, irq, mlxbf2_gpio_irq_handler,
+				       IRQF_SHARED, name, gs);
+		if (ret) {
+			dev_err(dev, "failed to request IRQ");
+			return ret;
+		}
+	}
+
 	platform_set_drvdata(pdev, gs);
 
 	ret = devm_gpiochip_add_data(dev, &gs->gc, gs);
@@ -315,5 +461,5 @@ static struct platform_driver mlxbf2_gpio_driver = {
 module_platform_driver(mlxbf2_gpio_driver);
 
 MODULE_DESCRIPTION("Mellanox BlueField-2 GPIO Driver");
-MODULE_AUTHOR("Mellanox Technologies");
+MODULE_AUTHOR("Asmaa Mnebhi <asmaa@nvidia.com>");
 MODULE_LICENSE("GPL v2");
-- 
2.30.1

Re: [PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support

[Andrew Lunn]

> +static int
> +mlxbf2_gpio_irq_set_type(struct irq_data *irqd, unsigned int type)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> +	struct mlxbf2_gpio_context *gs = gpiochip_get_data(gc);
> +	int offset = irqd_to_hwirq(irqd);
> +	unsigned long flags;
> +	bool fall = false;
> +	bool rise = false;
> +	u32 val;
> +
> +	switch (type & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_EDGE_BOTH:
> +	case IRQ_TYPE_LEVEL_MASK:
> +		fall = true;
> +		rise = true;
> +		break;
> +	case IRQ_TYPE_EDGE_RISING:
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		rise = true;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +	case IRQ_TYPE_LEVEL_LOW:
> +		fall = true;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

I'm still not convinced this is correct. Rising edge is different to
high. Rising edge only ever interrupts once, level keeps interrupting
until the source is cleared. You cannot store the four different
options in two bits.

Linus, have you seen anything like this before?

       Andrew

Re: [PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support

[Linus Walleij]

On Fri, Sep 24, 2021 at 1:46 PM Andrew Lunn <andrew@lunn.ch> wrote:

> > +static int
> > +mlxbf2_gpio_irq_set_type(struct irq_data *irqd, unsigned int type)
> > +{
> > +     struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> > +     struct mlxbf2_gpio_context *gs = gpiochip_get_data(gc);
> > +     int offset = irqd_to_hwirq(irqd);
> > +     unsigned long flags;
> > +     bool fall = false;
> > +     bool rise = false;
> > +     u32 val;
> > +
> > +     switch (type & IRQ_TYPE_SENSE_MASK) {
> > +     case IRQ_TYPE_EDGE_BOTH:
> > +     case IRQ_TYPE_LEVEL_MASK:
> > +             fall = true;
> > +             rise = true;
> > +             break;
> > +     case IRQ_TYPE_EDGE_RISING:
> > +     case IRQ_TYPE_LEVEL_HIGH:
> > +             rise = true;
> > +             break;
> > +     case IRQ_TYPE_EDGE_FALLING:
> > +     case IRQ_TYPE_LEVEL_LOW:
> > +             fall = true;
> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
>
> I'm still not convinced this is correct. Rising edge is different to
> high. Rising edge only ever interrupts once, level keeps interrupting
> until the source is cleared. You cannot store the four different
> options in two bits.
>
> Linus, have you seen anything like this before?

No, and I agree it looks weird.

There must be some explanation, what does the datasheet say?

Yours,
Linus Walleij

RE: [PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support

[Asmaa Mnebhi]

> > +static int
> > +mlxbf2_gpio_irq_set_type(struct irq_data *irqd, unsigned int type) 
> > +{
> > +     struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> > +     struct mlxbf2_gpio_context *gs = gpiochip_get_data(gc);
> > +     int offset = irqd_to_hwirq(irqd);
> > +     unsigned long flags;
> > +     bool fall = false;
> > +     bool rise = false;
> > +     u32 val;
> > +
> > +     switch (type & IRQ_TYPE_SENSE_MASK) {
> > +     case IRQ_TYPE_EDGE_BOTH:
> > +     case IRQ_TYPE_LEVEL_MASK:
> > +             fall = true;
> > +             rise = true;
> > +             break;
> > +     case IRQ_TYPE_EDGE_RISING:
> > +     case IRQ_TYPE_LEVEL_HIGH:
> > +             rise = true;
> > +             break;
> > +     case IRQ_TYPE_EDGE_FALLING:
> > +     case IRQ_TYPE_LEVEL_LOW:
> > +             fall = true;
> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
>
> I'm still not convinced this is correct. Rising edge is different to 
> high. Rising edge only ever interrupts once, level keeps interrupting 
> until the source is cleared. You cannot store the four different 
> options in two bits.
>
> Linus, have you seen anything like this before?

> No, and I agree it looks weird.

> There must be some explanation, what does the datasheet say?

I have consulted the HW folks about this, and they confirmed that
Our internal GPIO HW detects the falling edge.
INT_N signal (KSZ9031) ---connected to---- > BlueField GPIO9 (or GPIO12)
Even if INT_N is an active level interrupt, the GPIO HW always detects
the falling edge. This is why the original signal doesn’t really matter.
The BlueField GPIO HW only support Edge interrupts.

Re: [PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support

[Andrew Lunn]

> The BlueField GPIO HW only support Edge interrupts.

O.K. So please remove all level support from this driver, and return
-EINVAL if requested to do level.

This also means, you cannot use interrupts with the Ethernet PHY. The
PHY is using level interrupts.

    Andrew

RE: [PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support

[Asmaa Mnebhi]

> The BlueField GPIO HW only support Edge interrupts.

O.K. So please remove all level support from this driver,
and return -EINVAL if requested to do level.
This also means, you cannot use interrupts with the
Ethernet PHY. The PHY is using level interrupts.

Why not? The HW folks said it is alright because they
Do some internal conversion of PHY signal and we have tested
This extensively.

RE: [PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support

[Asmaa Mnebhi]

> The BlueField GPIO HW only support Edge interrupts.

O.K. So please remove all level support from this driver,
and return -EINVAL if requested to do level.
This also means, you cannot use interrupts with the
Ethernet PHY. The PHY is using level interrupts.

Why not? The HW folks said it is alright because they
Do some internal conversion of PHY signal and we have
tested This extensively.

Oh sorry I misunderstood what you meant.
In software we don't use the GPIO pin value itself to
Register the interrupt. We use a HW interrupt common
To all GPIO pins. So we should be ok. We only set this
EDGE register because it is required from a HW
Perspective to detect the INT_N signal.

Re: [PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support

[Andrew Lunn]

On Mon, Sep 27, 2021 at 02:19:45PM +0000, Asmaa Mnebhi wrote:
> 
> > The BlueField GPIO HW only support Edge interrupts.
> 
> O.K. So please remove all level support from this driver,
> and return -EINVAL if requested to do level.
> This also means, you cannot use interrupts with the
> Ethernet PHY. The PHY is using level interrupts.
> 
> Why not? The HW folks said it is alright because they
> Do some internal conversion of PHY signal and we have tested
> This extensively.

So the PHY is level based. The PHY is combing multiple interrupt
sources into one external interrupt. If any of those internal
interrupt sources are active, the external interrupt is active. If
there are multiple active sources at once, the interrupt stays low,
until they are all cleared. This means there is not an edge per
interrupt. There is one edge when the first internal source occurs,
and no more edges, even if there are more internal interrupts.

The general flow in the PHY interrupt handler is to read the interrupt
status register, which tells you which internal interrupts have
fired. You then address these internal interrupts one by one. This can
take some time, MDIO is a slow bus etc. While handling these interrupt
sources, it could be another internal interrupt source triggers. This
new internal interrupt source keeps the external interrupt active. But
there has not been an edge, since the interrupt handler is still
clearing the sources which caused the first interrupt. With level
interrupts, this is not an issue. When the interrupt handler exits,
the interrupt is re-enabled. Since it is still active, due to the
unhandled internal interrupt sources, the level interrupt immediately
fires again. the handler then sees this new interrupt and handles
it. At that point the level interrupt goes inactive.

Now think about what happens if you are using an edge interrupt
controller with a level interrupt. You get the first edge, and call
the interrupt handler. And then there are no more edges, despite there
being more interrupts. You not only loose the new interrupt, you never
see any more interrupts. You PHY link can go up and down, it can try
to report being over temperature, that it has detected power from the
peer, cable tests have passed, etc. But since there is no edge, there
is never an interrupt.

So you say it has been extensively tested. Has it been extensively
tested with multiple internal interrupt sources at the same time? And
with slight timing variations, so that you trigger this race
condition? It is not going to happen very often, but when it does, it
is going to be very bad.

	Andrew

RE: [PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support

[Asmaa Mnebhi]

On Mon, Sep 27, 2021 at 02:19:45PM +0000, Asmaa Mnebhi wrote:
> 
> > The BlueField GPIO HW only support Edge interrupts.
> 
> O.K. So please remove all level support from this driver, and return 
> -EINVAL if requested to do level.
> This also means, you cannot use interrupts with the Ethernet PHY. The 
> PHY is using level interrupts.
> 
> Why not? The HW folks said it is alright because they Do some internal 
> conversion of PHY signal and we have tested This extensively.

So the PHY is level based. The PHY is combing multiple interrupt sources 
into one external interrupt. If any of those internal interrupt sources are active,
the external interrupt is active. If there are multiple active sources at once, the
interrupt stays low, until they are all cleared. This means there is not an edge
per interrupt. There is one edge when the first internal source occurs, and no
more edges, even if there are more internal interrupts.

The general flow in the PHY interrupt handler is to read the interrupt status
register, which tells you which internal interrupts have fired.
You then address these internal interrupts one by one. This can take some
time, MDIO is a slow bus etc. While handling these interrupt sources,
it could be another internal interrupt source triggers. This new internal
interrupt source keeps the external interrupt active. But there has not
been an edge, since the interrupt handler is still clearing the sources
which caused the first interrupt. With level interrupts, this is not an
issue. When the interrupt handler exits, the interrupt is re-enabled. Since
it is still active, due to the unhandled internal interrupt sources,
the level interrupt immediately fires again. the handler then sees this
new interrupt and handles it. At that point the level interrupt goes inactive.

Now think about what happens if you are using an edge interrupt
controller with a level interrupt. You get the first edge, and call the
interrupt handler. And then there are no more edges, despite there
being more interrupts. You not only loose the new interrupt, you
never see any more interrupts. You PHY link can go up and down,
it can try to report being over temperature, that it has detected
power from the peer, cable tests have passed, etc. But since there
is no edge, there is never an interrupt.

So you say it has been extensively tested. Has it been extensively
tested with multiple internal interrupt sources at the same time?
And with slight timing variations, so that you trigger this race
condition? It is not going to happen very often, but when it does,
it is going to be very bad.

Asmaa>> Thank you very much for the detailed and clear explanation!
we only enable/support link up/down interrupts. QA has tested
bringing up/down the network interface +200 times in a loop.
I agree with you that the INT_N should be connected to a GPIO
Pin which also supports level interrupt. From a software perspective,
that HW interrupt flow is not visible/accessible to software.
I was instructed by HW designers to enable the interrupt and set it as falling.
The software interrupt and handler is not registered
based on the GPIO interrupt but rather a HW interrupt which is
common to all GPIO pins (irrelevant here, but this is edge triggered):
ret = devm_request_irq(dev, irq, mlxbf2_gpio_irq_handler,
                                        IRQF_SHARED, name, gs);

Re: [PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support

[Andrew Lunn]

> Asmaa>> Thank you very much for the detailed and clear explanation!
> we only enable/support link up/down interrupts. QA has tested
> bringing up/down the network interface +200 times in a loop.

The micrel driver currently only uses two interrupts of the available
8. So it will be hard to trigger the problem with the current
driver. Your best way to trigger it is going to bring the link down as
soon as it goes up. So you get first a link up, and then a link down
very shortly afterwards.

There is however nothing stopping developers making use of the other
interrupts. That will then increase the likelihood of problems.

What does help you is that the interrupt register is clear on read. So
the race condition window is small.

> The software interrupt and handler is not registered
> based on the GPIO interrupt but rather a HW interrupt which is
> common to all GPIO pins (irrelevant here, but this is edge triggered):
> ret = devm_request_irq(dev, irq, mlxbf2_gpio_irq_handler,
>                                         IRQF_SHARED, name, gs);

IRQF_SHARED implied level. You cannot have a shared interrupt which is
using edges.

      Andrew

RE: [PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support

[Asmaa Mnebhi]

> Asmaa>> Thank you very much for the detailed and clear explanation!
> we only enable/support link up/down interrupts. QA has tested bringing 
> up/down the network interface +200 times in a loop.

The micrel driver currently only uses two interrupts of the available 8. 
So it will be hard to trigger the problem with the current driver. Your 

best way to trigger it is going to bring the link down as soon as it goes up.

 So you get first a link up, and then a link down very shortly afterwards.

There is however nothing stopping developers making use of the other interrupts. 

That will then increase the likelihood of problems.

What does help you is that the interrupt register is clear on read. So the race condition 
window is small.

Asmaa>> Hi Andrew,

I had a meeting today with the HW folks to explain the problem at stake.
The flow for this issue is like this:
1) PHY issues INT_N signal (active low level interrupt)
2) falling edge detected on the GPIO and transmitted to software
3) the first thing mlxbf2_gpio_irq_handler does is to clear the GPIO interrupt.
However even if we clear the GPIO interrupt, the GPIO value itself
will be low as long as the INT_N signal is low. The GPIO HW triggers
the interrupt by detecting the falling edge of the GPIO pin.
4) mlxbf2_gpio_irq_handler triggers phy_interrupt which
calls drv->handler_interrupt.
handle_interrupt in our case = kszphy_handle_interrupt, which reads
MII_KSZPHY_INTCS regs and hence clears all interrupts at once. 

- if no other interrupt happens within this time frame, INT_N goes
back to 1 and the next interrupt will trigger another GPIO falling edge

- if the interrupt happens after the MDIO read, then it is not a problem. The
read would have already cleared the register and INT_N would go back to 1.
So the new interrupt will trigger a new GPIO falling edge interrupt.

Problem:
- however, if there is a second interrupt right before or during the MDIO read of
MII_KSZPHY_INTCS, it might not be detected by our GPIO HW.

Anyways, the HW folks agreed that this is a problem since indeed they do not
support LEVEL interrupts on the GPIOs at the moment.
They suggested to read the GPIO pin value to check if it has returned to high
in mlxbf2_gpio_irq_handler, then trigger the phy_interrupt handler.
But I don't think it is a good workaround because there could be a chain
of interrupts which hold the  LEVEL low for a long time, and we don't want to
be waiting too long in an interrupt handler routine.
I would greatly appreciate some more feedback on what is the best way to deal
With this in the upstreamed version of the driver.
HW folks said they will fix this in future BlueField generations.


> The software interrupt and handler is not registered based on the GPIO 
> interrupt but rather a HW interrupt which is common to all GPIO pins 
> (irrelevant here, but this is edge triggered):
> ret = devm_request_irq(dev, irq, mlxbf2_gpio_irq_handler,
>                                         IRQF_SHARED, name, gs);

IRQF_SHARED implied level. You cannot have a shared interrupt which is using edges.

      Andrew

RE: [PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support

[Asmaa Mnebhi]

> So the PHY is level based. The PHY is combing multiple interrupt sources 
> into one external interrupt. If any of those internal interrupt sources are
> active, the external interrupt is active. If there are > multiple active sources
> at once, the interrupt stays low, until they are all cleared. This means
> there is not an edge per interrupt. There is one edge when the first internal
> source occurs, and no more edges, > even if there are more internal interrupts.

> The general flow in the PHY interrupt handler is to read the interrupt status
> register, which tells you which internal interrupts have fired. You then
> address these internal interrupts one by one.

In KSZ9031, Register MII_KSZPHY_INTCS=0x1B reports all interrupt events and
clear on read. So if there are 4 different interrupts, once it is read once, all 4 clear at once.
The micrel.c driver has defined ack_interrupt to read the above reg and is called every time the
interrupt handler phy_interrupt is called. So in this case, we should be good.
The code flow in our case would look like this:
- 2 interrupt sources (for example, link down followed by link up) set in MII_KSZPHY_INTCS
- interrupt handler (phy_interrupt) reads MII_KSZPHY_INT which automatically clears both
interrupts
- another internal source triggers and sets the register.
- The second edge will be caught accordingly by the GPIO.

> This can take some time, MDIO is a slow bus etc. While handling these interrupt sources,
> it could be another internal interrupt source triggers. This new internal interrupt source
> keeps the external interrupt active. But there has not been an edge, since the interrupt 
> handler is still clearing the sources which caused the first interrupt. With level interrupts,
> this is not an issue. When the interrupt handler exits, the interrupt is re-enabled. Since it
> is still active, due to the unhandled internal interrupt sources, the level interrupt
> immediately fires again. the handler then sees this new interrupt and handles it.
> At that point the level interrupt goes inactive.

Re: [PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support

[Andrew Lunn]

> In KSZ9031, Register MII_KSZPHY_INTCS=0x1B reports all interrupt events and
> clear on read. So if there are 4 different interrupts, once it is read once, all 4 clear at once.
> The micrel.c driver has defined ack_interrupt to read the above reg and is called every time the
> interrupt handler phy_interrupt is called. So in this case, we should be good.
> The code flow in our case would look like this:
> - 2 interrupt sources (for example, link down followed by link up) set in MII_KSZPHY_INTCS
> - interrupt handler (phy_interrupt) reads MII_KSZPHY_INT which automatically clears both
> interrupts
> - another internal source triggers and sets the register.
> - The second edge will be caught accordingly by the GPIO.

I still think there is a small race window. You product manager needs
to decide if that is acceptable, or if you should poll the PHY.

Anyway, it is clear the hardware only does level interrupts, so the
GPIO driver should only accept level interrupts. -EINVAL otherwise.

I also assume you have a ACPI blob which indicates what sort of
interrupts that should be used, level low, falling edge etc. Since
that is outside of the kernel, i will never know what you decide to
put there. Ideally, until the hardware is fixed, you should not list
any interrupt and fallback to polling.

    Andrew

RE: [PATCH v3 1/2] gpio: mlxbf2: Introduce IRQ support

[Asmaa Mnebhi]

> In KSZ9031, Register MII_KSZPHY_INTCS=0x1B reports all interrupt 
> events and clear on read. So if there are 4 different interrupts, once it is read once, all 4 clear at once.
> The micrel.c driver has defined ack_interrupt to read the above reg 
> and is called every time the interrupt handler phy_interrupt is called. So in this case, we should be good.
> The code flow in our case would look like this:
> - 2 interrupt sources (for example, link down followed by link up) set 
> in MII_KSZPHY_INTCS
> - interrupt handler (phy_interrupt) reads MII_KSZPHY_INT which 
> automatically clears both interrupts
> - another internal source triggers and sets the register.
> - The second edge will be caught accordingly by the GPIO.

> I still think there is a small race window. You product manager needs to decide if that is acceptable, or if you should poll the PHY.

I talked to both our managers and the HW team and they said it is ok to use the interrupt for our product.

> Anyway, it is clear the hardware only does level interrupts, so the GPIO driver should only accept level interrupts. -EINVAL otherwise.

There is an on going conversation with HW folks to address this for future BlueField generations.

Thank you.
Asmaa