[PATCH v1 0/6] gpio: mlxbf2: Introduce proper interrupt handling

[PATCH v1 0/6] gpio: mlxbf2: Introduce proper interrupt handling

[Andy Shevchenko]

This is just a WIP / TODO series based on the discussion [1].
I hope nVidia will finish it and fix the initial problem sooner than later.

Bart, Linus, First 4 patches may be directly applied to the tree (they are
at least compile-tested, but I believe they won't change any functionality.

Patch 5 is some stubs that should have been done in the driver.
Patch 6 is follow up removal of custom GPIO IRQ handling from
Mellanox GBE driver. Both of them are quite far from finishing,
but it's a start for nVidia to develop and test proper solution.

In any case, I will probably sent end this week the ACPI IRQ abuse
part from the GBE driver (I won't touch OF path).

ARs for nVidia:
0) review this series;
1) properly develop GPIO driver;
2) replace custom code with correct one;
3) send the work for review to GPIO and ACPI maintainers (basically list
   of this series).

On my side I will help you if you have any questions regarding to GPIO
and ACPI.

Andy Shevchenko (6):
  gpio: mlxbf2: Convert to device PM ops
  gpio: mlxbf2: Drop wrong use of ACPI_PTR()
  gpio: mlxbf2: Use devm_platform_ioremap_resource()
  gpio: mlxbf2: Use DEFINE_RES_MEM_NAMED() helper macro
  TODO: gpio: mlxbf2: Introduce IRQ support
  TODO: net: mellanox: mlxbf_gige: Replace non-standard interrupt
    handling

 drivers/gpio/gpio-mlxbf2.c                    | 151 ++++++++++---
 .../mellanox/mlxbf_gige/mlxbf_gige_gpio.c     | 212 ------------------
 2 files changed, 120 insertions(+), 243 deletions(-)
 delete mode 100644 drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_gpio.c

-- 
2.30.2

[PATCH v1 1/6] gpio: mlxbf2: Convert to device PM ops

[Andy Shevchenko]

Convert driver to use modern device PM ops interface.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-mlxbf2.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpio-mlxbf2.c b/drivers/gpio/gpio-mlxbf2.c
index befa5e109943..68c471c10fa4 100644
--- a/drivers/gpio/gpio-mlxbf2.c
+++ b/drivers/gpio/gpio-mlxbf2.c
@@ -47,12 +47,10 @@
 #define YU_GPIO_MODE0_SET		0x54
 #define YU_GPIO_MODE0_CLEAR		0x58
 
-#ifdef CONFIG_PM
 struct mlxbf2_gpio_context_save_regs {
 	u32 gpio_mode0;
 	u32 gpio_mode1;
 };
-#endif
 
 /* BlueField-2 gpio block context structure. */
 struct mlxbf2_gpio_context {
@@ -61,9 +59,7 @@ struct mlxbf2_gpio_context {
 	/* YU GPIO blocks address */
 	void __iomem *gpio_io;
 
-#ifdef CONFIG_PM
 	struct mlxbf2_gpio_context_save_regs *csave_regs;
-#endif
 };
 
 /* BlueField-2 gpio shared structure. */
@@ -284,11 +280,9 @@ mlxbf2_gpio_probe(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM
-static int mlxbf2_gpio_suspend(struct platform_device *pdev,
-				pm_message_t state)
+static int __maybe_unused mlxbf2_gpio_suspend(struct device *dev)
 {
-	struct mlxbf2_gpio_context *gs = platform_get_drvdata(pdev);
+	struct mlxbf2_gpio_context *gs = dev_get_drvdata(dev);
 
 	gs->csave_regs->gpio_mode0 = readl(gs->gpio_io +
 		YU_GPIO_MODE0);
@@ -298,9 +292,9 @@ static int mlxbf2_gpio_suspend(struct platform_device *pdev,
 	return 0;
 }
 
-static int mlxbf2_gpio_resume(struct platform_device *pdev)
+static int __maybe_unused mlxbf2_gpio_resume(struct device *dev)
 {
-	struct mlxbf2_gpio_context *gs = platform_get_drvdata(pdev);
+	struct mlxbf2_gpio_context *gs = dev_get_drvdata(dev);
 
 	writel(gs->csave_regs->gpio_mode0, gs->gpio_io +
 		YU_GPIO_MODE0);
@@ -309,7 +303,7 @@ static int mlxbf2_gpio_resume(struct platform_device *pdev)
 
 	return 0;
 }
-#endif
+static SIMPLE_DEV_PM_OPS(mlxbf2_pm_ops, mlxbf2_gpio_suspend, mlxbf2_gpio_resume);
 
 static const struct acpi_device_id __maybe_unused mlxbf2_gpio_acpi_match[] = {
 	{ "MLNXBF22", 0 },
@@ -321,12 +315,9 @@ static struct platform_driver mlxbf2_gpio_driver = {
 	.driver = {
 		.name = "mlxbf2_gpio",
 		.acpi_match_table = ACPI_PTR(mlxbf2_gpio_acpi_match),
+		.pm = &mlxbf2_pm_ops,
 	},
 	.probe    = mlxbf2_gpio_probe,
-#ifdef CONFIG_PM
-	.suspend  = mlxbf2_gpio_suspend,
-	.resume   = mlxbf2_gpio_resume,
-#endif
 };
 
 module_platform_driver(mlxbf2_gpio_driver);
-- 
2.30.2

[PATCH v1 2/6] gpio: mlxbf2: Drop wrong use of ACPI_PTR()

[Andy Shevchenko]

ACPI_PTR() is more harmful than helpful. For example, in this case
if CONFIG_ACPI=n, the ID table left unused which is not what we want.

Instead of adding ifdeffery here and there, drop ACPI_PTR() and
replace acpi.h with mod_devicetable.h.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-mlxbf2.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-mlxbf2.c b/drivers/gpio/gpio-mlxbf2.c
index 68c471c10fa4..c0aa622fef76 100644
--- a/drivers/gpio/gpio-mlxbf2.c
+++ b/drivers/gpio/gpio-mlxbf2.c
@@ -1,6 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
 
-#include <linux/acpi.h>
 #include <linux/bitfield.h>
 #include <linux/bitops.h>
 #include <linux/device.h>
@@ -8,6 +7,7 @@
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
@@ -307,14 +307,14 @@ static SIMPLE_DEV_PM_OPS(mlxbf2_pm_ops, mlxbf2_gpio_suspend, mlxbf2_gpio_resume)
 
 static const struct acpi_device_id __maybe_unused mlxbf2_gpio_acpi_match[] = {
 	{ "MLNXBF22", 0 },
-	{},
+	{}
 };
 MODULE_DEVICE_TABLE(acpi, mlxbf2_gpio_acpi_match);
 
 static struct platform_driver mlxbf2_gpio_driver = {
 	.driver = {
 		.name = "mlxbf2_gpio",
-		.acpi_match_table = ACPI_PTR(mlxbf2_gpio_acpi_match),
+		.acpi_match_table = mlxbf2_gpio_acpi_match,
 		.pm = &mlxbf2_pm_ops,
 	},
 	.probe    = mlxbf2_gpio_probe,
-- 
2.30.2

[PATCH v1 3/6] gpio: mlxbf2: Use devm_platform_ioremap_resource()

[Andy Shevchenko]

Simplify the platform_get_resource() and devm_ioremap_resource()
calls with devm_platform_ioremap_resource().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-mlxbf2.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-mlxbf2.c b/drivers/gpio/gpio-mlxbf2.c
index c0aa622fef76..c193d1a9a5dd 100644
--- a/drivers/gpio/gpio-mlxbf2.c
+++ b/drivers/gpio/gpio-mlxbf2.c
@@ -228,7 +228,6 @@ mlxbf2_gpio_probe(struct platform_device *pdev)
 	struct mlxbf2_gpio_context *gs;
 	struct device *dev = &pdev->dev;
 	struct gpio_chip *gc;
-	struct resource *res;
 	unsigned int npins;
 	int ret;
 
@@ -237,13 +236,9 @@ mlxbf2_gpio_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	/* YU GPIO block address */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
-		return -ENODEV;
-
-	gs->gpio_io = devm_ioremap(dev, res->start, resource_size(res));
-	if (!gs->gpio_io)
-		return -ENOMEM;
+	gs->gpio_io = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(gs->gpio_io))
+		return PTR_ERR(gs->gpio_io);
 
 	ret = mlxbf2_gpio_get_lock_res(pdev);
 	if (ret) {
-- 
2.30.2

[PATCH v1 4/6] gpio: mlxbf2: Use DEFINE_RES_MEM_NAMED() helper macro

[Andy Shevchenko]

Use DEFINE_RES_MEM_NAMED() to save a couple of lines of code, which makes
the code a bit shorter and easier to read.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-mlxbf2.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-mlxbf2.c b/drivers/gpio/gpio-mlxbf2.c
index c193d1a9a5dd..3ed95e958c17 100644
--- a/drivers/gpio/gpio-mlxbf2.c
+++ b/drivers/gpio/gpio-mlxbf2.c
@@ -69,11 +69,8 @@ struct mlxbf2_gpio_param {
 	struct mutex *lock;
 };
 
-static struct resource yu_arm_gpio_lock_res = {
-	.start = YU_ARM_GPIO_LOCK_ADDR,
-	.end   = YU_ARM_GPIO_LOCK_ADDR + YU_ARM_GPIO_LOCK_SIZE - 1,
-	.name  = "YU_ARM_GPIO_LOCK",
-};
+static struct resource yu_arm_gpio_lock_res =
+	DEFINE_RES_MEM_NAMED(YU_ARM_GPIO_LOCK_ADDR, YU_ARM_GPIO_LOCK_SIZE, "YU_ARM_GPIO_LOCK");
 
 static DEFINE_MUTEX(yu_arm_gpio_lock_mutex);
 
-- 
2.30.2

[PATCH v1 5/6] TODO: gpio: mlxbf2: Introduce IRQ support

[Andy Shevchenko]

TBD

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-mlxbf2.c | 106 +++++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)

diff --git a/drivers/gpio/gpio-mlxbf2.c b/drivers/gpio/gpio-mlxbf2.c
index 3ed95e958c17..bd4c29120b62 100644
--- a/drivers/gpio/gpio-mlxbf2.c
+++ b/drivers/gpio/gpio-mlxbf2.c
@@ -43,9 +43,13 @@
 #define YU_GPIO_MODE0			0x0c
 #define YU_GPIO_DATASET			0x14
 #define YU_GPIO_DATACLEAR		0x18
+#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;
@@ -218,6 +222,108 @@ static int mlxbf2_gpio_direction_output(struct gpio_chip *chip,
 	return ret;
 }
 
+static void mlxbf2_gpio_irq_enable(struct mlxbf2_gpio_context *gs, int offset)
+{
+	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);
+
+	/* The INT_N interrupt level is active low.
+	 * So enable cause fall bit to detect when GPIO
+	 * state goes low.
+	 */
+	val = readl(gs->gpio_io + YU_GPIO_CAUSE_FALL_EN);
+	val |= BIT(offset);
+	writel(val, gs->gpio_io + YU_GPIO_CAUSE_FALL_EN);
+
+	/* Enable PHY interrupt by setting the priority level */
+	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 mlxbf2_gpio_context *gs, int offset)
+{
+	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 void mlxbf2_gpio_irq_ack(struct mlxbf2_gpio_context *gs, int offset)
+{
+	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);
+	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);
+	for_each_set_bit(level, &pending, gc->ngpio) {
+		int nested_irq = irq_find_mapping(gc->irq.domain, level);
+
+		handle_nested_irq(nested_irq);
+	}
+
+	return IRQ_RETVAL(pending);
+}
+
+static void mlxbf2_gpio_irq_mask(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) % MLXBF2_GPIO_MAX_PINS_PER_BLOCK;
+
+	mlxbf2_gpio_irq_disable(gs, offset);
+}
+
+static void mlxbf2_gpio_irq_unmask(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) % MLXBF2_GPIO_MAX_PINS_PER_BLOCK;
+
+	mlxbf2_gpio_irq_enable(gs, offset);
+}
+
+static void mlxbf2_gpio_irq_bus_lock(struct irq_data *irqd)
+{
+	mutex_lock(yu_arm_gpio_lock_param.lock);
+}
+
+static void mlxbf2_gpio_irq_bus_sync_unlock(struct irq_data *irqd)
+{
+	mutex_unlock(yu_arm_gpio_lock_param.lock);
+}
+
+static struct irq_chip mlxbf2_gpio_irq_chip = {
+	.name			= "mlxbf2_gpio",
+	.irq_mask		= mlxbf2_gpio_irq_mask,
+	.irq_unmask		= mlxbf2_gpio_irq_unmask,
+	.irq_bus_lock		= mlxbf2_gpio_irq_bus_lock,
+	.irq_bus_sync_unlock	= mlxbf2_gpio_irq_bus_sync_unlock,
+};
+
 /* BlueField-2 GPIO driver initialization routine. */
 static int
 mlxbf2_gpio_probe(struct platform_device *pdev)
-- 
2.30.2

[PATCH v1 6/6] TODO: net: mellanox: mlxbf_gige: Replace non-standard interrupt handling

[Andy Shevchenko]

Since GPIO driver supports interrupt handling, replace custom routine with
simple IRQ request.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 .../mellanox/mlxbf_gige/mlxbf_gige_gpio.c     | 212 ------------------
 1 file changed, 212 deletions(-)
 delete mode 100644 drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_gpio.c

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;
-}
-- 
2.30.2

Re: [PATCH v1 0/6] gpio: mlxbf2: Introduce proper interrupt handling

[Andy Shevchenko]

On Mon, Aug 16, 2021 at 3:01 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> This is just a WIP / TODO series based on the discussion [1].
> I hope nVidia will finish it and fix the initial problem sooner than later.
>
> Bart, Linus, First 4 patches may be directly applied to the tree (they are
> at least compile-tested, but I believe they won't change any functionality.
>
> Patch 5 is some stubs that should have been done in the driver.
> Patch 6 is follow up removal of custom GPIO IRQ handling from
> Mellanox GBE driver. Both of them are quite far from finishing,
> but it's a start for nVidia to develop and test proper solution.
>
> In any case, I will probably sent end this week the ACPI IRQ abuse
> part from the GBE driver (I won't touch OF path).
>
> ARs for nVidia:
> 0) review this series;
> 1) properly develop GPIO driver;
> 2) replace custom code with correct one;
> 3) send the work for review to GPIO and ACPI maintainers (basically list
>    of this series).
>
> On my side I will help you if you have any questions regarding to GPIO
> and ACPI.

Missed link

[1]: https://x-lore.kernel.org/linux-acpi/YRUskkALrPLa2cSf@smile.fi.intel.com/T/#u

> Andy Shevchenko (6):
>   gpio: mlxbf2: Convert to device PM ops
>   gpio: mlxbf2: Drop wrong use of ACPI_PTR()
>   gpio: mlxbf2: Use devm_platform_ioremap_resource()
>   gpio: mlxbf2: Use DEFINE_RES_MEM_NAMED() helper macro
>   TODO: gpio: mlxbf2: Introduce IRQ support
>   TODO: net: mellanox: mlxbf_gige: Replace non-standard interrupt
>     handling
>
>  drivers/gpio/gpio-mlxbf2.c                    | 151 ++++++++++---
>  .../mellanox/mlxbf_gige/mlxbf_gige_gpio.c     | 212 ------------------
>  2 files changed, 120 insertions(+), 243 deletions(-)
>  delete mode 100644 drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_gpio.c
>
> --
> 2.30.2
>


-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v1 1/6] gpio: mlxbf2: Convert to device PM ops

[Asmaa Mnebhi]

Acked-by: Asmaa Mnebhi <asmaa@nvidia.com>

-----Original Message-----
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 
Sent: Monday, August 16, 2021 8:00 AM
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; David Thompson <davthompson@nvidia.com>; linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; netdev@vger.kernel.org; linux-acpi@vger.kernel.org
Cc: Linus Walleij <linus.walleij@linaro.org>; Bartosz Golaszewski <bgolaszewski@baylibre.com>; David S. Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>; Asmaa Mnebhi <asmaa@nvidia.com>; Liming Sun <limings@nvidia.com>
Subject: [PATCH v1 1/6] gpio: mlxbf2: Convert to device PM ops
Importance: High

Convert driver to use modern device PM ops interface.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-mlxbf2.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpio-mlxbf2.c b/drivers/gpio/gpio-mlxbf2.c index befa5e109943..68c471c10fa4 100644
--- a/drivers/gpio/gpio-mlxbf2.c
+++ b/drivers/gpio/gpio-mlxbf2.c
@@ -47,12 +47,10 @@
 #define YU_GPIO_MODE0_SET		0x54
 #define YU_GPIO_MODE0_CLEAR		0x58
 
-#ifdef CONFIG_PM
 struct mlxbf2_gpio_context_save_regs {
 	u32 gpio_mode0;
 	u32 gpio_mode1;
 };
-#endif
 
 /* BlueField-2 gpio block context structure. */  struct mlxbf2_gpio_context { @@ -61,9 +59,7 @@ struct mlxbf2_gpio_context {
 	/* YU GPIO blocks address */
 	void __iomem *gpio_io;
 
-#ifdef CONFIG_PM
 	struct mlxbf2_gpio_context_save_regs *csave_regs; -#endif  };
 
 /* BlueField-2 gpio shared structure. */ @@ -284,11 +280,9 @@ mlxbf2_gpio_probe(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM
-static int mlxbf2_gpio_suspend(struct platform_device *pdev,
-				pm_message_t state)
+static int __maybe_unused mlxbf2_gpio_suspend(struct device *dev)
 {
-	struct mlxbf2_gpio_context *gs = platform_get_drvdata(pdev);
+	struct mlxbf2_gpio_context *gs = dev_get_drvdata(dev);
 
 	gs->csave_regs->gpio_mode0 = readl(gs->gpio_io +
 		YU_GPIO_MODE0);
@@ -298,9 +292,9 @@ static int mlxbf2_gpio_suspend(struct platform_device *pdev,
 	return 0;
 }
 
-static int mlxbf2_gpio_resume(struct platform_device *pdev)
+static int __maybe_unused mlxbf2_gpio_resume(struct device *dev)
 {
-	struct mlxbf2_gpio_context *gs = platform_get_drvdata(pdev);
+	struct mlxbf2_gpio_context *gs = dev_get_drvdata(dev);
 
 	writel(gs->csave_regs->gpio_mode0, gs->gpio_io +
 		YU_GPIO_MODE0);
@@ -309,7 +303,7 @@ static int mlxbf2_gpio_resume(struct platform_device *pdev)
 
 	return 0;
 }
-#endif
+static SIMPLE_DEV_PM_OPS(mlxbf2_pm_ops, mlxbf2_gpio_suspend, 
+mlxbf2_gpio_resume);
 
 static const struct acpi_device_id __maybe_unused mlxbf2_gpio_acpi_match[] = {
 	{ "MLNXBF22", 0 },
@@ -321,12 +315,9 @@ static struct platform_driver mlxbf2_gpio_driver = {
 	.driver = {
 		.name = "mlxbf2_gpio",
 		.acpi_match_table = ACPI_PTR(mlxbf2_gpio_acpi_match),
+		.pm = &mlxbf2_pm_ops,
 	},
 	.probe    = mlxbf2_gpio_probe,
-#ifdef CONFIG_PM
-	.suspend  = mlxbf2_gpio_suspend,
-	.resume   = mlxbf2_gpio_resume,
-#endif
 };
 
 module_platform_driver(mlxbf2_gpio_driver);
--
2.30.2

RE: [PATCH v1 2/6] gpio: mlxbf2: Drop wrong use of ACPI_PTR()

[Asmaa Mnebhi]

Acked-by: Asmaa Mnehi <asmaa@nvidia.com>

-----Original Message-----
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 
Sent: Monday, August 16, 2021 8:00 AM
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; David Thompson <davthompson@nvidia.com>; linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; netdev@vger.kernel.org; linux-acpi@vger.kernel.org
Cc: Linus Walleij <linus.walleij@linaro.org>; Bartosz Golaszewski <bgolaszewski@baylibre.com>; David S. Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>; Asmaa Mnebhi <asmaa@nvidia.com>; Liming Sun <limings@nvidia.com>
Subject: [PATCH v1 2/6] gpio: mlxbf2: Drop wrong use of ACPI_PTR()
Importance: High

ACPI_PTR() is more harmful than helpful. For example, in this case if CONFIG_ACPI=n, the ID table left unused which is not what we want.

Instead of adding ifdeffery here and there, drop ACPI_PTR() and replace acpi.h with mod_devicetable.h.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-mlxbf2.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-mlxbf2.c b/drivers/gpio/gpio-mlxbf2.c index 68c471c10fa4..c0aa622fef76 100644
--- a/drivers/gpio/gpio-mlxbf2.c
+++ b/drivers/gpio/gpio-mlxbf2.c
@@ -1,6 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
 
-#include <linux/acpi.h>
 #include <linux/bitfield.h>
 #include <linux/bitops.h>
 #include <linux/device.h>
@@ -8,6 +7,7 @@
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
@@ -307,14 +307,14 @@ static SIMPLE_DEV_PM_OPS(mlxbf2_pm_ops, mlxbf2_gpio_suspend, mlxbf2_gpio_resume)
 
 static const struct acpi_device_id __maybe_unused mlxbf2_gpio_acpi_match[] = {
 	{ "MLNXBF22", 0 },
-	{},
+	{}
 };
 MODULE_DEVICE_TABLE(acpi, mlxbf2_gpio_acpi_match);
 
 static struct platform_driver mlxbf2_gpio_driver = {
 	.driver = {
 		.name = "mlxbf2_gpio",
-		.acpi_match_table = ACPI_PTR(mlxbf2_gpio_acpi_match),
+		.acpi_match_table = mlxbf2_gpio_acpi_match,
 		.pm = &mlxbf2_pm_ops,
 	},
 	.probe    = mlxbf2_gpio_probe,
--
2.30.2

RE: [PATCH v1 3/6] gpio: mlxbf2: Use devm_platform_ioremap_resource()

[Asmaa Mnebhi]

Acked-by: Asmaa Mnebhi <asmaa@nvidia.com>

-----Original Message-----
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 
Sent: Monday, August 16, 2021 8:00 AM
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; David Thompson <davthompson@nvidia.com>; linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; netdev@vger.kernel.org; linux-acpi@vger.kernel.org
Cc: Linus Walleij <linus.walleij@linaro.org>; Bartosz Golaszewski <bgolaszewski@baylibre.com>; David S. Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>; Asmaa Mnebhi <asmaa@nvidia.com>; Liming Sun <limings@nvidia.com>
Subject: [PATCH v1 3/6] gpio: mlxbf2: Use devm_platform_ioremap_resource()
Importance: High

Simplify the platform_get_resource() and devm_ioremap_resource() calls with devm_platform_ioremap_resource().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-mlxbf2.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-mlxbf2.c b/drivers/gpio/gpio-mlxbf2.c index c0aa622fef76..c193d1a9a5dd 100644
--- a/drivers/gpio/gpio-mlxbf2.c
+++ b/drivers/gpio/gpio-mlxbf2.c
@@ -228,7 +228,6 @@ mlxbf2_gpio_probe(struct platform_device *pdev)
 	struct mlxbf2_gpio_context *gs;
 	struct device *dev = &pdev->dev;
 	struct gpio_chip *gc;
-	struct resource *res;
 	unsigned int npins;
 	int ret;
 
@@ -237,13 +236,9 @@ mlxbf2_gpio_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	/* YU GPIO block address */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
-		return -ENODEV;
-
-	gs->gpio_io = devm_ioremap(dev, res->start, resource_size(res));
-	if (!gs->gpio_io)
-		return -ENOMEM;
+	gs->gpio_io = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(gs->gpio_io))
+		return PTR_ERR(gs->gpio_io);
 
 	ret = mlxbf2_gpio_get_lock_res(pdev);
 	if (ret) {
--
2.30.2

RE: [PATCH v1 4/6] gpio: mlxbf2: Use DEFINE_RES_MEM_NAMED() helper macro

[Asmaa Mnebhi]

Acked-by: Asmaa Mnebhi <asmaa@nvidia.com>

-----Original Message-----
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 
Sent: Monday, August 16, 2021 8:00 AM
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; David Thompson <davthompson@nvidia.com>; linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; netdev@vger.kernel.org; linux-acpi@vger.kernel.org
Cc: Linus Walleij <linus.walleij@linaro.org>; Bartosz Golaszewski <bgolaszewski@baylibre.com>; David S. Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>; Asmaa Mnebhi <asmaa@nvidia.com>; Liming Sun <limings@nvidia.com>
Subject: [PATCH v1 4/6] gpio: mlxbf2: Use DEFINE_RES_MEM_NAMED() helper macro
Importance: High

Use DEFINE_RES_MEM_NAMED() to save a couple of lines of code, which makes the code a bit shorter and easier to read.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-mlxbf2.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-mlxbf2.c b/drivers/gpio/gpio-mlxbf2.c index c193d1a9a5dd..3ed95e958c17 100644
--- a/drivers/gpio/gpio-mlxbf2.c
+++ b/drivers/gpio/gpio-mlxbf2.c
@@ -69,11 +69,8 @@ struct mlxbf2_gpio_param {
 	struct mutex *lock;
 };
 
-static struct resource yu_arm_gpio_lock_res = {
-	.start = YU_ARM_GPIO_LOCK_ADDR,
-	.end   = YU_ARM_GPIO_LOCK_ADDR + YU_ARM_GPIO_LOCK_SIZE - 1,
-	.name  = "YU_ARM_GPIO_LOCK",
-};
+static struct resource yu_arm_gpio_lock_res =
+	DEFINE_RES_MEM_NAMED(YU_ARM_GPIO_LOCK_ADDR, YU_ARM_GPIO_LOCK_SIZE, 
+"YU_ARM_GPIO_LOCK");
 
 static DEFINE_MUTEX(yu_arm_gpio_lock_mutex);
 
--
2.30.2

Re: [PATCH v1 2/6] gpio: mlxbf2: Drop wrong use of ACPI_PTR()

[Bartosz Golaszewski]

On Mon, Aug 16, 2021 at 2:00 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> ACPI_PTR() is more harmful than helpful. For example, in this case
> if CONFIG_ACPI=n, the ID table left unused which is not what we want.
>
> Instead of adding ifdeffery here and there, drop ACPI_PTR() and
> replace acpi.h with mod_devicetable.h.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpio-mlxbf2.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mlxbf2.c b/drivers/gpio/gpio-mlxbf2.c
> index 68c471c10fa4..c0aa622fef76 100644
> --- a/drivers/gpio/gpio-mlxbf2.c
> +++ b/drivers/gpio/gpio-mlxbf2.c
> @@ -1,6 +1,5 @@
>  // SPDX-License-Identifier: GPL-2.0
>
> -#include <linux/acpi.h>
>  #include <linux/bitfield.h>
>  #include <linux/bitops.h>
>  #include <linux/device.h>
> @@ -8,6 +7,7 @@
>  #include <linux/io.h>
>  #include <linux/ioport.h>
>  #include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
> @@ -307,14 +307,14 @@ static SIMPLE_DEV_PM_OPS(mlxbf2_pm_ops, mlxbf2_gpio_suspend, mlxbf2_gpio_resume)
>
>  static const struct acpi_device_id __maybe_unused mlxbf2_gpio_acpi_match[] = {
>         { "MLNXBF22", 0 },
> -       {},
> +       {}

Ninja change :) I removed it - send a separate patch for this if you want to.

Bart

>  };
>  MODULE_DEVICE_TABLE(acpi, mlxbf2_gpio_acpi_match);
>
>  static struct platform_driver mlxbf2_gpio_driver = {
>         .driver = {
>                 .name = "mlxbf2_gpio",
> -               .acpi_match_table = ACPI_PTR(mlxbf2_gpio_acpi_match),
> +               .acpi_match_table = mlxbf2_gpio_acpi_match,
>                 .pm = &mlxbf2_pm_ops,
>         },
>         .probe    = mlxbf2_gpio_probe,
> --
> 2.30.2
>

Re: [PATCH v1 0/6] gpio: mlxbf2: Introduce proper interrupt handling

[Bartosz Golaszewski]

On Mon, Aug 16, 2021 at 2:00 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> This is just a WIP / TODO series based on the discussion [1].
> I hope nVidia will finish it and fix the initial problem sooner than later.
>
> Bart, Linus, First 4 patches may be directly applied to the tree (they are
> at least compile-tested, but I believe they won't change any functionality.
>
> Patch 5 is some stubs that should have been done in the driver.
> Patch 6 is follow up removal of custom GPIO IRQ handling from
> Mellanox GBE driver. Both of them are quite far from finishing,
> but it's a start for nVidia to develop and test proper solution.
>
> In any case, I will probably sent end this week the ACPI IRQ abuse
> part from the GBE driver (I won't touch OF path).
>
> ARs for nVidia:
> 0) review this series;
> 1) properly develop GPIO driver;
> 2) replace custom code with correct one;
> 3) send the work for review to GPIO and ACPI maintainers (basically list
>    of this series).
>
> On my side I will help you if you have any questions regarding to GPIO
> and ACPI.
>
> Andy Shevchenko (6):
>   gpio: mlxbf2: Convert to device PM ops
>   gpio: mlxbf2: Drop wrong use of ACPI_PTR()
>   gpio: mlxbf2: Use devm_platform_ioremap_resource()
>   gpio: mlxbf2: Use DEFINE_RES_MEM_NAMED() helper macro
>   TODO: gpio: mlxbf2: Introduce IRQ support
>   TODO: net: mellanox: mlxbf_gige: Replace non-standard interrupt
>     handling
>
>  drivers/gpio/gpio-mlxbf2.c                    | 151 ++++++++++---
>  .../mellanox/mlxbf_gige/mlxbf_gige_gpio.c     | 212 ------------------
>  2 files changed, 120 insertions(+), 243 deletions(-)
>  delete mode 100644 drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_gpio.c
>
> --
> 2.30.2
>

Applied first four patches.

Bart

Re: [PATCH v1 2/6] gpio: mlxbf2: Drop wrong use of ACPI_PTR()

[Andy Shevchenko]

On Mon, Aug 16, 2021 at 10:21 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> On Mon, Aug 16, 2021 at 2:00 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> >  static const struct acpi_device_id __maybe_unused mlxbf2_gpio_acpi_match[] = {
> >         { "MLNXBF22", 0 },
> > -       {},
> > +       {}
>
> Ninja change :) I removed it -

Thanks!

> send a separate patch for this if you want to.

I don't think it's needed per se right now.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v1 5/6] TODO: gpio: mlxbf2: Introduce IRQ support

[Asmaa Mnebhi]

Hi Andy,

Thanks for your help!
Please see my comments/questions below.

-----Original Message-----
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 
Sent: Monday, August 16, 2021 8:00 AM
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; David Thompson <davthompson@nvidia.com>; linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; netdev@vger.kernel.org; linux-acpi@vger.kernel.org
Cc: Linus Walleij <linus.walleij@linaro.org>; Bartosz Golaszewski <bgolaszewski@baylibre.com>; David S. Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>; Asmaa Mnebhi <asmaa@nvidia.com>; Liming Sun <limings@nvidia.com>
Subject: [PATCH v1 5/6] TODO: gpio: mlxbf2: Introduce IRQ support
Importance: High

TBD

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-mlxbf2.c | 106 +++++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)

diff --git a/drivers/gpio/gpio-mlxbf2.c b/drivers/gpio/gpio-mlxbf2.c index 3ed95e958c17..bd4c29120b62 100644
--- a/drivers/gpio/gpio-mlxbf2.c
+++ b/drivers/gpio/gpio-mlxbf2.c
@@ -43,9 +43,13 @@
 #define YU_GPIO_MODE0			0x0c
 #define YU_GPIO_DATASET			0x14
 #define YU_GPIO_DATACLEAR		0x18
+#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;
@@ -218,6 +222,108 @@ static int mlxbf2_gpio_direction_output(struct gpio_chip *chip,
 	return ret;
 }
 
+static void mlxbf2_gpio_irq_enable(struct mlxbf2_gpio_context *gs, int 
+offset) {
+	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);
+
+	/* The INT_N interrupt level is active low.
+	 * So enable cause fall bit to detect when GPIO
+	 * state goes low.
+	 */
+	val = readl(gs->gpio_io + YU_GPIO_CAUSE_FALL_EN);
+	val |= BIT(offset);
+	writel(val, gs->gpio_io + YU_GPIO_CAUSE_FALL_EN);
+
+	/* Enable PHY interrupt by setting the priority level */
+	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 mlxbf2_gpio_context *gs, int 
+offset) {
+	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 void mlxbf2_gpio_irq_ack(struct mlxbf2_gpio_context *gs, int 
+offset) {
+	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);
+	spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags); }
+
+static irqreturn_t mlxbf2_gpio_irq_handler(int irq, void *ptr) {

So how do you suggest registering this handler?

1) should I still use BF_RSH0_DEVICE_YU_INT shared interrupt signal?

2) or does Linux kernel know (based on parsing GpioInt) how trigger the handler based on the GPIO datain changing (active low/high)? In this case, the kernel will call this handler whenever the GPIO pin (9 or 12) value changes. I need to check whether GPIO is active low/high but lets assume for now it is open drain active low. We will use acpi_dev_gpio_irq_get to translate GpioInt to a Linux IRQ number:
irq = acpi_dev_gpio_irq_get_by(ACPI_COMPANION(dev), " phy-gpios ", 0);
ret = devm_request_irq(dev, irq, mlxbf2_gpio_irq_handler, IRQF_ONESHOT | IRQF_SHARED, dev_name(dev), gs);

And I will need to add GpioInt to the GPI0 ACPI table as follows:

// GPIO Controller
      Device(GPI0) {
       Name(_HID, "MLNXBF22")
        Name(_UID, Zero)
        Name(_CCA, 1)
        Name(_CRS, ResourceTemplate() {
          // for gpio[0] yu block
         Memory32Fixed(ReadWrite, 0x0280c000, 0x00000100)
         GpioInt (Level, ActiveLow, Exclusive, PullDefault, , " \\_SB.GPI0") {9}
        })
        Name(_DSD, Package() {
          ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
          Package() {
            Package () { "phy-gpios", Package() {^GPI0, 0, 0, 0 }},
            Package () { "rst-pin", 32 }, // GPIO pin triggering soft reset on BlueSphere and PRIS
          }
        })
      }


+	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);
+	for_each_set_bit(level, &pending, gc->ngpio) {
+		int nested_irq = irq_find_mapping(gc->irq.domain, level);
+
+		handle_nested_irq(nested_irq);

Now how can the mlxbf_gige_main.c driver also retrieve this nested_irq to register its interrupt handler as well? This irq.domain is only visible to the gpio-mlxbf2.c driver isn't it?
phydev->irq (below) should be populated with nested_irq at init time because it is used to register the phy interrupt in this generic function:

void phy_request_interrupt(struct phy_device *phydev)
{
	int err;

	err = request_threaded_irq(phydev->irq, NULL, phy_interrupt,
				   IRQF_ONESHOT | IRQF_SHARED,
				   phydev_name(phydev), phydev);
	if (err) {
		phydev_warn(phydev, "Error %d requesting IRQ %d, falling back to polling\n",
			    err, phydev->irq);
		phydev->irq = PHY_POLL;
	} else {
		if (phy_enable_interrupts(phydev)) {
			phydev_warn(phydev, "Can't enable interrupt, falling back to polling\n");
			phy_free_interrupt(phydev);
			phydev->irq = PHY_POLL;
		}
	}
}
EXPORT_SYMBOL(phy_request_interrupt);


+	}
+
+	return IRQ_RETVAL(pending);
+}
+
+static void mlxbf2_gpio_irq_mask(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) % MLXBF2_GPIO_MAX_PINS_PER_BLOCK;
Why is the modulo needed? Isn't the hwirq returned a number between 0 and MLXBF2_GPIO_MAX_PINS_PER_BLOCK-1 ?

+
+	mlxbf2_gpio_irq_disable(gs, offset);
+}
+
+static void mlxbf2_gpio_irq_unmask(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) % MLXBF2_GPIO_MAX_PINS_PER_BLOCK;
+
+	mlxbf2_gpio_irq_enable(gs, offset);
+}
+
+static void mlxbf2_gpio_irq_bus_lock(struct irq_data *irqd) {
+	mutex_lock(yu_arm_gpio_lock_param.lock);
+}
+
+static void mlxbf2_gpio_irq_bus_sync_unlock(struct irq_data *irqd) {
+	mutex_unlock(yu_arm_gpio_lock_param.lock);
+}
+
+static struct irq_chip mlxbf2_gpio_irq_chip = {
+	.name			= "mlxbf2_gpio",
+	.irq_mask		= mlxbf2_gpio_irq_mask,
+	.irq_unmask		= mlxbf2_gpio_irq_unmask,
+	.irq_bus_lock		= mlxbf2_gpio_irq_bus_lock,
+	.irq_bus_sync_unlock	= mlxbf2_gpio_irq_bus_sync_unlock,
+};
+

We also need to make sure that the gpio driver is loaded before the mlxbf-gige driver. Otherwise, the mlxbf-gige 1G interface fails to come up. I have implemented this dependency on the gpio driver before, something like this at the end of the mlxbf-gige driver:
MODULE_SOFTDEP("pre: gpio_mlxbf2");

 /* BlueField-2 GPIO driver initialization routine. */  static int  mlxbf2_gpio_probe(struct platform_device *pdev)
--
2.30.2

Re: [PATCH v1 6/6] TODO: net: mellanox: mlxbf_gige: Replace non-standard interrupt handling

[Linus Walleij]

On Mon, Aug 16, 2021 at 2:00 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Since GPIO driver supports interrupt handling, replace custom routine with
> simple IRQ request.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  .../mellanox/mlxbf_gige/mlxbf_gige_gpio.c     | 212 ------------------

Don't you also need to remove this file from Makefile?

Yours,
Linus Walleij

Re: [PATCH v1 6/6] TODO: net: mellanox: mlxbf_gige: Replace non-standard interrupt handling

[Andy Shevchenko]

On Tue, Aug 17, 2021 at 01:06:06AM +0200, Linus Walleij wrote:
> On Mon, Aug 16, 2021 at 2:00 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> 
> > Since GPIO driver supports interrupt handling, replace custom routine with
> > simple IRQ request.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  .../mellanox/mlxbf_gige/mlxbf_gige_gpio.c     | 212 ------------------
> 
> Don't you also need to remove this file from Makefile?

Of course, this is simply to show the intention. It's just a hint, I have no
time to write the code for Mellanox, really :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/6] TODO: gpio: mlxbf2: Introduce IRQ support

[Andy Shevchenko]

On Mon, Aug 16, 2021 at 09:34:50PM +0000, Asmaa Mnebhi wrote:
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 
> Sent: Monday, August 16, 2021 8:00 AM

...

> +static irqreturn_t mlxbf2_gpio_irq_handler(int irq, void *ptr) {
> 
> So how do you suggest registering this handler?

As usual. This handler should be probably registered via standard mechanisms.
Perhaps it's hierarchical IRQ, then use that facility of GPIO library.
(see gpio-dwapb.c for the example).

> 1) should I still use BF_RSH0_DEVICE_YU_INT shared interrupt signal?

I don't know your hardware connection between GPIO and GIC. You have to look
into TRM and see how they are connected and what should be programmed for the
mode you want to run this in.

> 2) or does Linux kernel know (based on parsing GpioInt) how trigger the
> handler based on the GPIO datain changing (active low/high)? In this case,
> the kernel will call this handler whenever the GPIO pin (9 or 12) value
> changes.

After driver in place kernel will know how to map, register and handle the GPIO
interrupt. But the GIC part is out of the picture here. It may be you will need
additional stuff there, like disabling (or else) the interrupts, or providing a
bypass. I can't answer to this.

> I need to check whether GPIO is active low/high but lets assume for
> now it is open drain active low. We will use acpi_dev_gpio_irq_get to
> translate GpioInt to a Linux IRQ number:

> irq = acpi_dev_gpio_irq_get_by(ACPI_COMPANION(dev), "phy-gpios", 0);
> ret = devm_request_irq(dev, irq, mlxbf2_gpio_irq_handler, IRQF_ONESHOT | IRQF_SHARED, dev_name(dev), gs);

Yes.
(I dunno about one short and shared flags, but you should know it better than me)

> And I will need to add GpioInt to the GPI0 ACPI table as follows:

But you told me that it's already on the market, how are you suppose to change
existing tables?

> // GPIO Controller
>       Device(GPI0) {
>        Name(_HID, "MLNXBF22")
>         Name(_UID, Zero)
>         Name(_CCA, 1)
>         Name(_CRS, ResourceTemplate() {
>           // for gpio[0] yu block
>          Memory32Fixed(ReadWrite, 0x0280c000, 0x00000100)
>          GpioInt (Level, ActiveLow, Exclusive, PullDefault, , " \\_SB.GPI0") {9}
>         })
>         Name(_DSD, Package() {
>           ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>           Package() {
>             Package () { "phy-gpios", Package() {^GPI0, 0, 0, 0 }},
>             Package () { "rst-pin", 32 }, // GPIO pin triggering soft reset on BlueSphere and PRIS
>           }
>         })
>       }

No, it's completely wrong. The resources are provided by GPIO controller and
consumed by devices. You showed me the table for the consumer, which is good
(of course if you wish to use Edge triggered interrupts there).

...

> +		handle_nested_irq(nested_irq);

> Now how can the mlxbf_gige_main.c driver also retrieve this nested_irq to
> register its interrupt handler as well? This irq.domain is only visible to
> the gpio-mlxbf2.c driver isn't it?  phydev->irq (below) should be populated
> with nested_irq at init time because it is used to register the phy interrupt
> in this generic function:

nested here is an example, you have to check which one to use.

Moreover the code misses ->irq_set_type() callback.

So, yes, domain will be GPIOs but IRQ core will handle it properly.

> void phy_request_interrupt(struct phy_device *phydev)
> {
> 	int err;
> 
> 	err = request_threaded_irq(phydev->irq, NULL, phy_interrupt,
> 				   IRQF_ONESHOT | IRQF_SHARED,
> 				   phydev_name(phydev), phydev);

You have several IRQ resources (Interrupt() and GpioInt() ones) in the consumer
device node. I don't know how your hardware is designed, but if you want to use
GPIO, then this phydev->irq should be a Linux vIRQ returned from above
mentioned acpi_dev_gpio_irq_get_by() call. Everything else is magically happens.

...

> +	int offset = irqd_to_hwirq(irqd) % MLXBF2_GPIO_MAX_PINS_PER_BLOCK;

> Why is the modulo needed? Isn't the hwirq returned a number between 0 and
> MLXBF2_GPIO_MAX_PINS_PER_BLOCK-1 ?

It's copy'n'paste from somewhere, since you have device per bank you don't
need it.

...

> We also need to make sure that the gpio driver is loaded before the
> mlxbf-gige driver. Otherwise, the mlxbf-gige 1G interface fails to come up.
> I have implemented this dependency on the gpio driver before, something like
> this at the end of the mlxbf-gige driver:

> MODULE_SOFTDEP("pre: gpio_mlxbf2");

No, when you have GPIO device is listed in the tables the IRQ mapping will
return you deferred probe. It doesn't matter when device will appear, but it
will be functional only when all resource requirements are satisfied.

Above soft dependency doesn't guarantee this, deferred probe does.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/6] TODO: gpio: mlxbf2: Introduce IRQ support

[Andrew Lunn]

Hi Asmaa

> > And I will need to add GpioInt to the GPI0 ACPI table as follows:
> 
> But you told me that it's already on the market, how are you suppose to change
> existing tables?

BIOSes have as many bugs a the kernel. So your product should be
designed so you can upgrade the kernel and upgrade the BIOS.

phylib itself does not care if there is an interrupt or not. It will
fall back to polling. So if your driver finds itself running with old
tables, it does not matter. Just print a warning to the kernel logs
suggesting the user upgrades their BIOS firmware.

> > // GPIO Controller
> >       Device(GPI0) {
> >        Name(_HID, "MLNXBF22")
> >         Name(_UID, Zero)
> >         Name(_CCA, 1)
> >         Name(_CRS, ResourceTemplate() {
> >           // for gpio[0] yu block
> >          Memory32Fixed(ReadWrite, 0x0280c000, 0x00000100)
> >          GpioInt (Level, ActiveLow, Exclusive, PullDefault, , " \\_SB.GPI0") {9}
> >         })
> >         Name(_DSD, Package() {
> >           ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >           Package() {
> >             Package () { "phy-gpios", Package() {^GPI0, 0, 0, 0 }},
> >             Package () { "rst-pin", 32 }, // GPIO pin triggering soft reset on BlueSphere and PRIS
> >           }
> >         })
> >       }
> 
> No, it's completely wrong. The resources are provided by GPIO controller and
> consumed by devices.

In the device tree world, you list the interrupt in the PHY node.
Documentation/devicetree/bindings/net/ethernet-phy.yaml gives an
example:

    ethernet {
        #address-cells = <1>;
        #size-cells = <0>;

        ethernet-phy@0 {
            compatible = "ethernet-phy-id0141.0e90", "ethernet-phy-ieee802.3-c45";
            interrupt-parent = <&PIC>;
            interrupts = <35 1>;
            reg = <0>;

            resets = <&rst 8>;
            reset-names = "phy";
            reset-gpios = <&gpio1 4 1>;
            reset-assert-us = <1000>;
            reset-deassert-us = <2000>;
        };
    };

You need to do something similar in the ACPI world. There was a very
long discussion in this area recently, and some patches merged. You
probably need to build on that. See:

firmware-guide/acpi/dsd/phy.rst

	Andrew

RE: [PATCH v1 5/6] TODO: gpio: mlxbf2: Introduce IRQ support

[Asmaa Mnebhi]

Thank you Andrew and Andy! I will prepare 2 patches (one for gpio-mlxbf2.c and one for mlxbf-gige) and send it your way.

-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch> 
Sent: Wednesday, August 18, 2021 6:40 PM
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Asmaa Mnebhi <asmaa@nvidia.com>; David Thompson <davthompson@nvidia.com>; linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; netdev@vger.kernel.org; linux-acpi@vger.kernel.org; Linus Walleij <linus.walleij@linaro.org>; Bartosz Golaszewski <bgolaszewski@baylibre.com>; David S. Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>; Liming Sun <limings@nvidia.com>
Subject: Re: [PATCH v1 5/6] TODO: gpio: mlxbf2: Introduce IRQ support
Importance: High

Hi Asmaa

> > And I will need to add GpioInt to the GPI0 ACPI table as follows:
> 
> But you told me that it's already on the market, how are you suppose 
> to change existing tables?

BIOSes have as many bugs a the kernel. So your product should be designed so you can upgrade the kernel and upgrade the BIOS.

phylib itself does not care if there is an interrupt or not. It will fall back to polling. So if your driver finds itself running with old tables, it does not matter. Just print a warning to the kernel logs suggesting the user upgrades their BIOS firmware.

> > // GPIO Controller
> >       Device(GPI0) {
> >        Name(_HID, "MLNXBF22")
> >         Name(_UID, Zero)
> >         Name(_CCA, 1)
> >         Name(_CRS, ResourceTemplate() {
> >           // for gpio[0] yu block
> >          Memory32Fixed(ReadWrite, 0x0280c000, 0x00000100)
> >          GpioInt (Level, ActiveLow, Exclusive, PullDefault, , " \\_SB.GPI0") {9}
> >         })
> >         Name(_DSD, Package() {
> >           ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >           Package() {
> >             Package () { "phy-gpios", Package() {^GPI0, 0, 0, 0 }},
> >             Package () { "rst-pin", 32 }, // GPIO pin triggering soft reset on BlueSphere and PRIS
> >           }
> >         })
> >       }
> 
> No, it's completely wrong. The resources are provided by GPIO 
> controller and consumed by devices.

In the device tree world, you list the interrupt in the PHY node.
Documentation/devicetree/bindings/net/ethernet-phy.yaml gives an
example:

    ethernet {
        #address-cells = <1>;
        #size-cells = <0>;

        ethernet-phy@0 {
            compatible = "ethernet-phy-id0141.0e90", "ethernet-phy-ieee802.3-c45";
            interrupt-parent = <&PIC>;
            interrupts = <35 1>;
            reg = <0>;

            resets = <&rst 8>;
            reset-names = "phy";
            reset-gpios = <&gpio1 4 1>;
            reset-assert-us = <1000>;
            reset-deassert-us = <2000>;
        };
    };

You need to do something similar in the ACPI world. There was a very long discussion in this area recently, and some patches merged. You probably need to build on that. See:

firmware-guide/acpi/dsd/phy.rst

	Andrew

RE: [PATCH v1 5/6] TODO: gpio: mlxbf2: Introduce IRQ support

[Asmaa Mnebhi]

Hi Andy, Hi Andrew,

I have a question regarding patch submission. I am going to mimic what Andy has done for v5/6 and v6/6 and send 2 patches in a bundle as follows:
/* for the cover letter */ : Subject: [PATCH v1 0/2] gpio: mlxbf2: Introduce proper interrupt handling
Subject: [PATCH v1 1/2] gpio: mlxbf2: Introduce IRQ support
Subject: [PATCH v1 2/2] net: mellanox: mlxbf_gige: Replace non-standard interrupt handling

Questions:
1) do the subject lines look ok? i.e. sending patches that target "net" as opposed to "net-next"
2) would you like me to add a "Fixes" tag to each patch as follows? I am not sure if you consider this a bug?
Fixes: f92e1869d74e ("Add Mellanox BlueField Gigabit Ethernet driver")

Thank you.
Asmaa

-----Original Message-----
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 
Sent: Wednesday, August 18, 2021 10:08 AM
To: Asmaa Mnebhi <asmaa@nvidia.com>
Cc: David Thompson <davthompson@nvidia.com>; linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; netdev@vger.kernel.org; linux-acpi@vger.kernel.org; Linus Walleij <linus.walleij@linaro.org>; Bartosz Golaszewski <bgolaszewski@baylibre.com>; David S. Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>; Liming Sun <limings@nvidia.com>
Subject: Re: [PATCH v1 5/6] TODO: gpio: mlxbf2: Introduce IRQ support
Importance: High

On Mon, Aug 16, 2021 at 09:34:50PM +0000, Asmaa Mnebhi wrote:
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Monday, August 16, 2021 8:00 AM

...

> +static irqreturn_t mlxbf2_gpio_irq_handler(int irq, void *ptr) {
> 
> So how do you suggest registering this handler?

As usual. This handler should be probably registered via standard mechanisms.
Perhaps it's hierarchical IRQ, then use that facility of GPIO library.
(see gpio-dwapb.c for the example).

> 1) should I still use BF_RSH0_DEVICE_YU_INT shared interrupt signal?

I don't know your hardware connection between GPIO and GIC. You have to look into TRM and see how they are connected and what should be programmed for the mode you want to run this in.

> 2) or does Linux kernel know (based on parsing GpioInt) how trigger 
> the handler based on the GPIO datain changing (active low/high)? In 
> this case, the kernel will call this handler whenever the GPIO pin (9 
> or 12) value changes.

After driver in place kernel will know how to map, register and handle the GPIO interrupt. But the GIC part is out of the picture here. It may be you will need additional stuff there, like disabling (or else) the interrupts, or providing a bypass. I can't answer to this.

> I need to check whether GPIO is active low/high but lets assume for 
> now it is open drain active low. We will use acpi_dev_gpio_irq_get to 
> translate GpioInt to a Linux IRQ number:

> irq = acpi_dev_gpio_irq_get_by(ACPI_COMPANION(dev), "phy-gpios", 0); 
> ret = devm_request_irq(dev, irq, mlxbf2_gpio_irq_handler, IRQF_ONESHOT 
> | IRQF_SHARED, dev_name(dev), gs);

Yes.
(I dunno about one short and shared flags, but you should know it better than me)

> And I will need to add GpioInt to the GPI0 ACPI table as follows:

But you told me that it's already on the market, how are you suppose to change existing tables?

> // GPIO Controller
>       Device(GPI0) {
>        Name(_HID, "MLNXBF22")
>         Name(_UID, Zero)
>         Name(_CCA, 1)
>         Name(_CRS, ResourceTemplate() {
>           // for gpio[0] yu block
>          Memory32Fixed(ReadWrite, 0x0280c000, 0x00000100)
>          GpioInt (Level, ActiveLow, Exclusive, PullDefault, , " \\_SB.GPI0") {9}
>         })
>         Name(_DSD, Package() {
>           ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>           Package() {
>             Package () { "phy-gpios", Package() {^GPI0, 0, 0, 0 }},
>             Package () { "rst-pin", 32 }, // GPIO pin triggering soft reset on BlueSphere and PRIS
>           }
>         })
>       }

No, it's completely wrong. The resources are provided by GPIO controller and consumed by devices. You showed me the table for the consumer, which is good (of course if you wish to use Edge triggered interrupts there).

...

> +		handle_nested_irq(nested_irq);

> Now how can the mlxbf_gige_main.c driver also retrieve this nested_irq 
> to register its interrupt handler as well? This irq.domain is only 
> visible to the gpio-mlxbf2.c driver isn't it?  phydev->irq (below) 
> should be populated with nested_irq at init time because it is used to 
> register the phy interrupt in this generic function:

nested here is an example, you have to check which one to use.

Moreover the code misses ->irq_set_type() callback.

So, yes, domain will be GPIOs but IRQ core will handle it properly.

> void phy_request_interrupt(struct phy_device *phydev) {
> 	int err;
> 
> 	err = request_threaded_irq(phydev->irq, NULL, phy_interrupt,
> 				   IRQF_ONESHOT | IRQF_SHARED,
> 				   phydev_name(phydev), phydev);

You have several IRQ resources (Interrupt() and GpioInt() ones) in the consumer device node. I don't know how your hardware is designed, but if you want to use GPIO, then this phydev->irq should be a Linux vIRQ returned from above mentioned acpi_dev_gpio_irq_get_by() call. Everything else is magically happens.

...

> +	int offset = irqd_to_hwirq(irqd) % MLXBF2_GPIO_MAX_PINS_PER_BLOCK;

> Why is the modulo needed? Isn't the hwirq returned a number between 0 
> and
> MLXBF2_GPIO_MAX_PINS_PER_BLOCK-1 ?

It's copy'n'paste from somewhere, since you have device per bank you don't need it.

...

> We also need to make sure that the gpio driver is loaded before the 
> mlxbf-gige driver. Otherwise, the mlxbf-gige 1G interface fails to come up.
> I have implemented this dependency on the gpio driver before, 
> something like this at the end of the mlxbf-gige driver:

> MODULE_SOFTDEP("pre: gpio_mlxbf2");

No, when you have GPIO device is listed in the tables the IRQ mapping will return you deferred probe. It doesn't matter when device will appear, but it will be functional only when all resource requirements are satisfied.

Above soft dependency doesn't guarantee this, deferred probe does.

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/6] TODO: gpio: mlxbf2: Introduce IRQ support

[Andrew Lunn]

On Wed, Sep 15, 2021 at 07:27:51PM +0000, Asmaa Mnebhi wrote:
> Hi Andy, Hi Andrew,
> 
> I have a question regarding patch submission. I am going to mimic what Andy has done for v5/6 and v6/6 and send 2 patches in a bundle as follows:
> /* for the cover letter */ : Subject: [PATCH v1 0/2] gpio: mlxbf2: Introduce proper interrupt handling
> Subject: [PATCH v1 1/2] gpio: mlxbf2: Introduce IRQ support
> Subject: [PATCH v1 2/2] net: mellanox: mlxbf_gige: Replace non-standard interrupt handling
> 
> Questions:
> 1) do the subject lines look ok? i.e. sending patches that target "net" as opposed to "net-next"
> 2) would you like me to add a "Fixes" tag to each patch as follows? I am not sure if you consider this a bug?
> Fixes: f92e1869d74e ("Add Mellanox BlueField Gigabit Ethernet driver")

You are posting patches which go into two different subsystems. So you
need to pay special care here. Pick a maintainer you want to merge
this, and make sure you Cc: the other maintainer. Make it clear in
patch 0/X which maintainer you would like to take the patch series,
and that the other should give an Acked-by if they are happy with the
patches.

I don't think this should be considered a bug, so no need for a Fixes:
tag.

The subject lines look O.K.

Also, please fix your mailer to wrap lines at about 75 characters.

      Andrew