[v1,1/1] Add driver for Mellanox BlueField-3 GPIO controller

This patch adds support for the GPIO controller used by Mellanox BlueField-3 SOCs.
By default (after a reset for example), only the hardware has control over the GPIO values.
The GPIOs need to be configured accordingly for the software to control them.
We don't allow the user to manipulate the GPIO pins unless it is allowed by
the ACPI table property "ctrl_gpio_mask".

Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
---
 drivers/gpio/Kconfig       |   7 +
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-mlxbf3.c | 337 +++++++++++++++++++++++++++++++++++++
 3 files changed, 345 insertions(+)
 create mode 100644 drivers/gpio/gpio-mlxbf3.c

Hi,

Could you please have a look at this patch?

Thank you,
Asmaa

-----Original Message-----
From: Asmaa Mnebhi <asmaa@nvidia.com> 
Sent: Monday, April 18, 2022 10:14 AM
To: andy.shevchenko@gmail.com; linus.walleij@linaro.org; bgolaszewski@baylibre.com; linux-gpio@vger.kernel.org; linux-acpi@vger.kernel.org
Cc: Asmaa Mnebhi <asmaa@nvidia.com>
Subject: [PATCH v1 1/1] Add driver for Mellanox BlueField-3 GPIO controller
Importance: High

This patch adds support for the GPIO controller used by Mellanox BlueField-3 SOCs.
By default (after a reset for example), only the hardware has control over the GPIO values.
The GPIOs need to be configured accordingly for the software to control them.
We don't allow the user to manipulate the GPIO pins unless it is allowed by the ACPI table property "ctrl_gpio_mask".

Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
---
 drivers/gpio/Kconfig       |   7 +
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-mlxbf3.c | 337 +++++++++++++++++++++++++++++++++++++
 3 files changed, 345 insertions(+)
 create mode 100644 drivers/gpio/gpio-mlxbf3.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 45764ec3b2eb..d470117e7254 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1530,6 +1530,13 @@ config GPIO_MLXBF2
 	help
 	  Say Y here if you want GPIO support on Mellanox BlueField 2 SoC.
 
+config GPIO_MLXBF3
+	tristate "Mellanox BlueField 3 SoC GPIO"
+	depends on (MELLANOX_PLATFORM && ARM64 && ACPI) || (64BIT && COMPILE_TEST)
+	select GPIO_GENERIC
+	help
+	  Say Y here if you want GPIO support on Mellanox BlueField 3 SoC.
+
 config GPIO_ML_IOH
 	tristate "OKI SEMICONDUCTOR ML7213 IOH GPIO support"
 	depends on X86 || COMPILE_TEST
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 14352f6dfe8e..eb24e8e4c4e1 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -97,6 +97,7 @@ obj-$(CONFIG_GPIO_MERRIFIELD)		+= gpio-merrifield.o
 obj-$(CONFIG_GPIO_ML_IOH)		+= gpio-ml-ioh.o
 obj-$(CONFIG_GPIO_MLXBF)		+= gpio-mlxbf.o
 obj-$(CONFIG_GPIO_MLXBF2)		+= gpio-mlxbf2.o
+obj-$(CONFIG_GPIO_MLXBF3)		+= gpio-mlxbf3.o
 obj-$(CONFIG_GPIO_MM_LANTIQ)		+= gpio-mm-lantiq.o
 obj-$(CONFIG_GPIO_MOCKUP)		+= gpio-mockup.o
 obj-$(CONFIG_GPIO_MOXTET)		+= gpio-moxtet.o
diff --git a/drivers/gpio/gpio-mlxbf3.c b/drivers/gpio/gpio-mlxbf3.c new file mode 100644 index 000000000000..1f576de43680
--- /dev/null
+++ b/drivers/gpio/gpio-mlxbf3.c
@@ -0,0 +1,337 @@
+// SPDX-License-Identifier: GPL-2.0-only or BSD-3-Clause
+
+/*
+ * Copyright (C) 2022 NVIDIA CORPORATION & AFFILIATES  */
+
+#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>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/resource.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
+/*
+ * There are 2 YU GPIO blocks:
+ * gpio[0]: HOST_GPIO0->HOST_GPIO31
+ * gpio[1]: HOST_GPIO32->HOST_GPIO55
+ */
+#define MLXBF3_GPIO_MAX_PINS_PER_BLOCK 32
+
+/*
+ * fw_gpio[x] block registers and their offset  */
+#define YU_GPIO_FW_CONTROL_SET		0x00
+#define YU_GPIO_FW_OUTPUT_ENABLE_SET	0x04
+#define YU_GPIO_FW_DATA_OUT_SET		0x08
+#define YU_GPIO_FW_CONTROL_CLEAR	0x14
+#define YU_GPIO_FW_OUTPUT_ENABLE_CLEAR	0x18
+#define YU_GPIO_FW_DATA_OUT_CLEAR	0x1c
+#define YU_GPIO_CAUSE_RISE_EN		0x28
+#define YU_GPIO_CAUSE_FALL_EN		0x2c
+#define YU_GPIO_READ_DATA_IN		0x30
+#define YU_GPIO_READ_OUTPUT_ENABLE	0x34
+#define YU_GPIO_READ_DATA_OUT		0x38
+#define YU_GPIO_READ_FW_CONTROL		0x44
+
+#define YU_GPIO_CAUSE_OR_CAUSE_EVTEN0	0x00
+#define YU_GPIO_CAUSE_OR_EVTEN0		0x14
+#define YU_GPIO_CAUSE_OR_CLRCAUSE	0x18
+
+/* BlueField-3 gpio block context structure. */ struct 
+mlxbf3_gpio_context {
+	struct gpio_chip gc;
+	struct irq_chip irq_chip;
+
+	/* YU GPIO blocks address */
+	void __iomem *gpio_io;
+
+	/* YU GPIO cause block address */
+	void __iomem *gpio_cause_io;
+
+	/* Represents GPIO pins that the user can manipulate */
+	uint32_t ctrl_gpio_mask;
+};
+
+static void mlxbf3_gpio_set(struct gpio_chip *chip, unsigned int 
+offset, int val) {
+	struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
+
+	/* Software can only control GPIO pins defined by ctrl_gpio_mask */
+	if (!(BIT(offset) & gs->ctrl_gpio_mask))
+		return;
+
+	if (val)
+		writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_DATA_OUT_SET);
+	else
+		writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_DATA_OUT_CLEAR);
+
+	/* Make sure all previous writes are done before changing YU_GPIO_FW_CONTROL_SET */
+	wmb();
+
+	/* This needs to be done last to avoid glitches */
+	writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_CONTROL_SET); }
+
+static int mlxbf3_gpio_direction_input(struct gpio_chip *chip,
+				       unsigned int offset)
+{
+	struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
+	unsigned long flags;
+
+	spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
+
+	writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_OUTPUT_ENABLE_CLEAR);
+	writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_CONTROL_CLEAR);
+
+	spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
+
+	return 0;
+}
+
+static int mlxbf3_gpio_direction_output(struct gpio_chip *chip,
+					unsigned int offset,
+					int value)
+{
+	struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
+	unsigned long flags;
+
+	spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
+
+	writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_OUTPUT_ENABLE_SET);
+
+	spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
+
+	return 0;
+}
+
+static void mlxbf3_gpio_irq_enable(struct irq_data *irqd) {
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct mlxbf3_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_cause_io + YU_GPIO_CAUSE_OR_CLRCAUSE);
+	val |= BIT(offset);
+	writel(val, gs->gpio_cause_io + YU_GPIO_CAUSE_OR_CLRCAUSE);
+
+	val = readl(gs->gpio_cause_io + YU_GPIO_CAUSE_OR_EVTEN0);
+	val |= BIT(offset);
+	writel(val, gs->gpio_cause_io + YU_GPIO_CAUSE_OR_EVTEN0);
+	spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags); }
+
+static void mlxbf3_gpio_irq_disable(struct irq_data *irqd) {
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct mlxbf3_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_cause_io + YU_GPIO_CAUSE_OR_EVTEN0);
+	val &= ~BIT(offset);
+	writel(val, gs->gpio_cause_io + YU_GPIO_CAUSE_OR_EVTEN0);
+	spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags); }
+
+static irqreturn_t mlxbf3_gpio_irq_handler(int irq, void *ptr) {
+	struct mlxbf3_gpio_context *gs = ptr;
+	struct gpio_chip *gc = &gs->gc;
+	unsigned long pending;
+	u32 level;
+
+	pending = readl(gs->gpio_cause_io + YU_GPIO_CAUSE_OR_CAUSE_EVTEN0);
+	writel(pending, gs->gpio_cause_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
+mlxbf3_gpio_irq_set_type(struct irq_data *irqd, unsigned int type) {
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct mlxbf3_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:
+		fall = true;
+		rise = true;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		rise = true;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		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-3 GPIO driver initialization routine. */ static int 
+mlxbf3_gpio_probe(struct platform_device *pdev) {
+	struct mlxbf3_gpio_context *gs;
+	struct device *dev = &pdev->dev;
+	struct gpio_irq_chip *girq;
+	struct gpio_chip *gc;
+	unsigned int npins;
+	const char *name;
+	int ret, irq;
+
+	name = dev_name(dev);
+
+	gs = devm_kzalloc(dev, sizeof(*gs), GFP_KERNEL);
+	if (!gs)
+		return -ENOMEM;
+
+	/* YU GPIO block address */
+	gs->gpio_io = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(gs->gpio_io))
+		return PTR_ERR(gs->gpio_io);
+
+	/* YU GPIO block address */
+	gs->gpio_cause_io = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(gs->gpio_cause_io))
+		return PTR_ERR(gs->gpio_cause_io);
+
+	if (device_property_read_u32(dev, "npins", &npins))
+		npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
+
+	if (device_property_read_u32(dev, "ctrl_gpio_mask", &gs->ctrl_gpio_mask))
+		gs->ctrl_gpio_mask = 0x0;
+
+	gc = &gs->gc;
+
+	/* To set the direction to input, just give control to HW by setting
+	 * YU_GPIO_FW_CONTROL_CLEAR.
+	 * If the GPIO is controlled by HW, read its value via read_data_in register.
+	 *
+	 * When the direction = output, the GPIO is controlled by SW and
+	 * datain=dataout. If software modifies the value of the GPIO pin,
+	 * the value can be read from read_data_in without changing the direction.
+	 */
+	ret = bgpio_init(gc, dev, 4,
+			gs->gpio_io + YU_GPIO_READ_DATA_IN,
+			NULL,
+			NULL,
+			NULL,
+			NULL,
+			0);
+
+	gc->set = mlxbf3_gpio_set;
+	gc->direction_input = mlxbf3_gpio_direction_input;
+	gc->direction_output = mlxbf3_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 = mlxbf3_gpio_irq_set_type;
+		gs->irq_chip.irq_enable = mlxbf3_gpio_irq_enable;
+		gs->irq_chip.irq_disable = mlxbf3_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, mlxbf3_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);
+	if (ret) {
+		dev_err(dev, "Failed adding memory mapped gpiochip\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int mlxbf3_gpio_remove(struct platform_device *pdev) {
+	struct mlxbf3_gpio_context *gs = platform_get_drvdata(pdev);
+
+	/* Set the GPIO control back to HW */
+	writel(gs->ctrl_gpio_mask, gs->gpio_io + YU_GPIO_FW_CONTROL_CLEAR);
+
+	return 0;
+}
+
+static const struct acpi_device_id __maybe_unused mlxbf3_gpio_acpi_match[] = {
+	{ "MLNXBF33", 0 },
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, mlxbf3_gpio_acpi_match);
+
+static struct platform_driver mlxbf3_gpio_driver = {
+	.driver = {
+		.name = "mlxbf3_gpio",
+		.acpi_match_table = mlxbf3_gpio_acpi_match,
+	},
+	.probe    = mlxbf3_gpio_probe,
+	.remove   = mlxbf3_gpio_remove,
+};
+
+module_platform_driver(mlxbf3_gpio_driver);
+
+MODULE_DESCRIPTION("Mellanox BlueField-3 GPIO Driver"); 
+MODULE_AUTHOR("Asmaa Mnebhi <asmaa@nvidia.com>"); MODULE_LICENSE("Dual 
+BSD/GPL");
--
2.30.1

> Could you please have a look at this patch?

Sure!

> +static void mlxbf3_gpio_set(struct gpio_chip *chip, unsigned int
> +offset, int val) {

Put opening bracket on new line.
Run your code through scripts/checkpatch.pl before submitting.

> +       struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
> +
> +       /* Software can only control GPIO pins defined by ctrl_gpio_mask */
> +       if (!(BIT(offset) & gs->ctrl_gpio_mask))
> +               return;
> +
> +       if (val)
> +               writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_DATA_OUT_SET);
> +       else
> +               writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_DATA_OUT_CLEAR);
> +
> +       /* Make sure all previous writes are done before changing YU_GPIO_FW_CONTROL_SET */
> +       wmb();
> +
> +       /* This needs to be done last to avoid glitches */
> +       writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_CONTROL_SET); }

Bracket on new line. This coding style is very odd.

The hardware is a bit odd too but I see why you can't use GPIO_GENERIC
properly with this FW_CONTROL_SET business :/

> +static int mlxbf3_gpio_direction_input(struct gpio_chip *chip,
> +                                      unsigned int offset)
> +{
> +       struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> +
> +       writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_OUTPUT_ENABLE_CLEAR);
> +       writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_CONTROL_CLEAR);
> +
> +       spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
> +
> +       return 0;
> +}
> +
> +static int mlxbf3_gpio_direction_output(struct gpio_chip *chip,
> +                                       unsigned int offset,
> +                                       int value)
> +{
> +       struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> +
> +       writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_OUTPUT_ENABLE_SET);
> +
> +       spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
> +
> +       return 0;
> +}

Why isn't FW_CONTROL_CLEAR/SET used when making  a line
into an output? Seems unsymmetric? At least put a comment in the
code why this is different from making a line into input.

> +       /* To set the direction to input, just give control to HW by setting
> +        * YU_GPIO_FW_CONTROL_CLEAR.
> +        * If the GPIO is controlled by HW, read its value via read_data_in register.
> +        *
> +        * When the direction = output, the GPIO is controlled by SW and
> +        * datain=dataout. If software modifies the value of the GPIO pin,
> +        * the value can be read from read_data_in without changing the direction.
> +        */
> +       ret = bgpio_init(gc, dev, 4,
> +                       gs->gpio_io + YU_GPIO_READ_DATA_IN,
> +                       NULL,
> +                       NULL,
> +                       NULL,
> +                       NULL,
> +                       0);

Hm. Is it still worth it?

MVH
Linus Walleij

-----Original Message-----
From: Linus Walleij <linus.walleij@linaro.org> 
Sent: Thursday, May 5, 2022 11:12 AM
To: Asmaa Mnebhi <asmaa@nvidia.com>
Cc: andy.shevchenko@gmail.com; bgolaszewski@baylibre.com; linux-gpio@vger.kernel.org; linux-acpi@vger.kernel.org
Subject: Re: [PATCH v1 1/1] Add driver for Mellanox BlueField-3 GPIO controller
Importance: High

> Could you please have a look at this patch?

Sure!

> +static void mlxbf3_gpio_set(struct gpio_chip *chip, unsigned int 
> +offset, int val) {

Put opening bracket on new line.
Run your code through scripts/checkpatch.pl before submitting.

It is odd that it got sent this way in the email. In my patch and my local file, the bracket is aligned correctly as follows:
 +static void mlxbf3_gpio_set(struct gpio_chip *chip, unsigned int offset, int val)
 +{

I did run checkpatch.pl before sending this. I will resend the email , hopefully it will be sent properly this time.

> +       struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
> +
> +       /* Software can only control GPIO pins defined by ctrl_gpio_mask */
> +       if (!(BIT(offset) & gs->ctrl_gpio_mask))
> +               return;
> +
> +       if (val)
> +               writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_DATA_OUT_SET);
> +       else
> +               writel(BIT(offset), gs->gpio_io + 
> + YU_GPIO_FW_DATA_OUT_CLEAR);
> +
> +       /* Make sure all previous writes are done before changing YU_GPIO_FW_CONTROL_SET */
> +       wmb();
> +
> +       /* This needs to be done last to avoid glitches */
> +       writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_CONTROL_SET); }

Bracket on new line. This coding style is very odd.

Same comment as above. Not sure why the email prints it this way.

The hardware is a bit odd too but I see why you can't use GPIO_GENERIC properly with this FW_CONTROL_SET business :/

> +static int mlxbf3_gpio_direction_input(struct gpio_chip *chip,
> +                                      unsigned int offset) {
> +       struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> +
> +       writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_OUTPUT_ENABLE_CLEAR);
> +       writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_CONTROL_CLEAR);
> +
> +       spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
> +
> +       return 0;
> +}
> +
> +static int mlxbf3_gpio_direction_output(struct gpio_chip *chip,
> +                                       unsigned int offset,
> +                                       int value) {
> +       struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> +
> +       writel(BIT(offset), gs->gpio_io + 
> + YU_GPIO_FW_OUTPUT_ENABLE_SET);
> +
> +       spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
> +
> +       return 0;
> +}

Why isn't FW_CONTROL_CLEAR/SET used when making  a line into an output? Seems unsymmetric? At least put a comment in the code why this is different from making a line into input.

Ok I will add a comment.

All GPIOs are always controlled by HW by default.
FW_CONTROL_SET is used when we want to release control of the GPIO by HW and give the control to software instead. We want to give control to software only after changing the value of a GPIO pin to avoid any glitches in the HW, so this is the only case where YU_GPIO_FW_CONTROL_SET is set. This is why we don’t do it in mlxbf3_gpio_direction_output. Anyways, the user cannot set a gpio pin to a certain value from linux, if they haven't changed the direction of the gpio to out in the first place. 
It is a bit an odd implementation but basically even if we set this bit in mlxbf3_gpio_direction_output:
writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_OUTPUT_ENABLE_SET);
It doesn’t actually take effect until we also set the corresponding bit in YU_GPIO_FW_CONTROL_SET. But it was advised by our HW team to set YU_GPIO_FW_CONTROL_SET only after setting the desired GPIO value to avoid HW glitches.
 

> +       /* To set the direction to input, just give control to HW by setting
> +        * YU_GPIO_FW_CONTROL_CLEAR.
> +        * If the GPIO is controlled by HW, read its value via read_data_in register.
> +        *
> +        * When the direction = output, the GPIO is controlled by SW and
> +        * datain=dataout. If software modifies the value of the GPIO pin,
> +        * the value can be read from read_data_in without changing the direction.
> +        */
> +       ret = bgpio_init(gc, dev, 4,
> +                       gs->gpio_io + YU_GPIO_READ_DATA_IN,
> +                       NULL,
> +                       NULL,
> +                       NULL,
> +                       NULL,
> +                       0);

Hm. Is it still worth it?

MVH
Linus Walleij

On Thu, May 5, 2022 at 7:01 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:
>

Your email is broken.  Please, fix the client or other parts that make
your email unreadable.

On Thu, May 5, 2022 at 7:01 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:

>> Why isn't FW_CONTROL_CLEAR/SET used when making  a line into an output?
>> Seems unsymmetric? At least put a comment in the code why this is different from
>> making a line into input.
>
> Ok I will add a comment.
>
> All GPIOs are always controlled by HW by default.
> FW_CONTROL_SET is used when we want to release
> control of the GPIO by HW and give the control to
> software instead.

What does that mean in practice? What way does hardware
control a GPIO?

It's not some way of remuxing the pins is it, so you what you
are calling "controlled by hardware" is actually that it is
controlled by e.g. I2C and other stuff that may be muxed in?

Yours,
Linus Walleij

>
> All GPIOs are always controlled by HW by default.
> FW_CONTROL_SET is used when we want to release control of the GPIO by 
> HW and give the control to software instead.

What does that mean in practice? What way does hardware control a GPIO?

It's not some way of remuxing the pins is it, so you what you are calling "controlled by hardware" is actually that it is controlled by e.g. I2C and other stuff that may be muxed in?

Yes exactly. These GPIO pins are already assigned a specific HW functionality like I2C SDA/SCL, MDIO etc... but we would like to support the option to control them from software if desired.

On Thu, May 12, 2022 at 10:45 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:

>> > All GPIOs are always controlled by HW by default.
>> > FW_CONTROL_SET is used when we want to release control of the GPIO by
>> > HW and give the control to software instead.
>>
>> What does that mean in practice? What way does hardware control a GPIO?
>>
>> It's not some way of remuxing the pins is it, so you what you are calling
>> "controlled by hardware" is actually that it is controlled by e.g. I2C and
>> other stuff that may be muxed in?
>
> Yes exactly. These GPIO pins are already assigned a specific HW
> functionality like I2C SDA/SCL, MDIO etc... but we would like to
> support the option to control them from software if desired.

But how does that play with your pin controller?

I mean: is the multiplexing of different other devices also software
controlled, so this should have a proper pin control driver rather
than just a GPIO driver?

Yours,
Linus Walleij

>> > All GPIOs are always controlled by HW by default.
>> > FW_CONTROL_SET is used when we want to release control of the GPIO 
>> > by HW and give the control to software instead.
>>
>> What does that mean in practice? What way does hardware control a GPIO?
>>
>> It's not some way of remuxing the pins is it, so you what you are 
>> calling "controlled by hardware" is actually that it is controlled by 
>> e.g. I2C and other stuff that may be muxed in?
>
> Yes exactly. These GPIO pins are already assigned a specific HW 
> functionality like I2C SDA/SCL, MDIO etc... but we would like to 
> support the option to control them from software if desired.

But how does that play with your pin controller?

I mean: is the multiplexing of different other devices also software controlled, so this should have a proper pin control driver rather than just a GPIO driver?

So these GPIO pins are assigned one specific HW functionality on the boards and software should never change them. 
By default, for security purposes, I think we shouldn't let the user have the option to control the GPIO pins since they have a specific HW functionality.
But for bringup/debug purposes, we would like to support the option of software being able to change these pin values. We also might have customers that choose to change the default HW connection of a certain GPIO pin and connect it to control their LEDs for instance.  


Thanks.
Asmaa

On Mon, May 16, 2022 at 3:00 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:

> So these GPIO pins are assigned one specific HW functionality on the boards
>  and software should never change them.
>
> By default, for security purposes, I think we shouldn't let the user have the option
> to control the GPIO pins since they have a specific HW functionality.
>
> But for bringup/debug purposes, we would like to support the option of software
> being able to change these pin values. We also might have customers that choose
> to change the default HW connection of a certain GPIO pin and connect it to
> control their LEDs for instance.

The fact that the usecase is bringup/debug does not mean we cut
corners and do "quick fixes". The proper APIs have to be implemented,
the alternative is to not submit the driver at all.

What I hear is that these pins have two modes:

1. Used for a device (I2C etc)
2. Used as GPIO by setting a bit in YU_GPIO_FW_CONTROL_SET

This is two pin control multiplexing states already.

So this should have a simple pin control driver as back-end with
the GPIO as front end. A shortcut to enabling pins into GPIO
mode can be provided using .gpio_request_enable() from
struct pinmux_ops.

Please refer to
https://docs.kernel.org/driver-api/pin-control.html

I know this means more work and is kind of complex. But drivers/pinctrl
has a lot of examples you can follow, for example
drivers/pinctrl/pinctrl-sx150x.c and other simple multipurpose
chips.

Yours,
Linus Walleij

Thank you for the feedback Linus! I will address your comments in my next patch.

Best,
Asmaa

-----Original Message-----
From: Linus Walleij <linus.walleij@linaro.org> 
Sent: Wednesday, May 18, 2022 9:51 AM
To: Asmaa Mnebhi <asmaa@nvidia.com>
Cc: andy.shevchenko@gmail.com; bgolaszewski@baylibre.com; linux-gpio@vger.kernel.org; linux-acpi@vger.kernel.org
Subject: Re: [PATCH v1 1/1] Add driver for Mellanox BlueField-3 GPIO controller
Importance: High

On Mon, May 16, 2022 at 3:00 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:

> So these GPIO pins are assigned one specific HW functionality on the 
> boards  and software should never change them.
>
> By default, for security purposes, I think we shouldn't let the user 
> have the option to control the GPIO pins since they have a specific HW functionality.
>
> But for bringup/debug purposes, we would like to support the option of 
> software being able to change these pin values. We also might have 
> customers that choose to change the default HW connection of a certain 
> GPIO pin and connect it to control their LEDs for instance.

The fact that the usecase is bringup/debug does not mean we cut corners and do "quick fixes". The proper APIs have to be implemented, the alternative is to not submit the driver at all.

What I hear is that these pins have two modes:

1. Used for a device (I2C etc)
2. Used as GPIO by setting a bit in YU_GPIO_FW_CONTROL_SET

This is two pin control multiplexing states already.

So this should have a simple pin control driver as back-end with the GPIO as front end. A shortcut to enabling pins into GPIO mode can be provided using .gpio_request_enable() from struct pinmux_ops.

Please refer to
https://docs.kernel.org/driver-api/pin-control.html

I know this means more work and is kind of complex. But drivers/pinctrl has a lot of examples you can follow, for example drivers/pinctrl/pinctrl-sx150x.c and other simple multipurpose chips.

Yours,
Linus Walleij

Hi Linus,

Sorry for the long delay. I did work on adding the pinctrl driver besides the gpio driver and I am working on testing it.
I added the following to the gpio driver:
gc->set = mlxbf3_gpio_set;
gc->direction_input = mlxbf3_gpio_direction_input;
gc->direction_output = mlxbf3_gpio_direction_output;
gc->request = gpiochip_generic_request;
gc->free = gpiochip_generic_free;

In the pinctrl driver, I defined the following:
static const struct pinmux_ops mlxbf_pmx_ops = {
.get_functions_count = mlxbf_pmx_get_funcs_count,
.get_function_name = mlxbf_pmx_get_func_name,
.get_function_groups = mlxbf_pmx_get_groups,
.set_mux = mlxbf_pmx_set,
.gpio_request_enable = mlxbf_gpio_request_enable,
.gpio_disable_free = mlxbf_gpio_disable_free,
};

During testing, I use the sysfs to change the gpio value as follows:
Cd /sys/class/gpio
echo 480 > export
When I do the export, I see that gpiochip_generic_request is being called which calls .gpio_request_enable = mlxbf_gpio_request_enable.

Is this how it also works in other driver? Or am I missing something? 
I wanted to disallow muxing from user space. I would like that to be controlled by the ACPI table only. For example, use devm_gpio_request from some other driver if needed.

Thanks.
Asmaa
-----Original Message-----
From: Linus Walleij <linus.walleij@linaro.org> 
Sent: Wednesday, May 18, 2022 9:51 AM
To: Asmaa Mnebhi <asmaa@nvidia.com>
Cc: andy.shevchenko@gmail.com; bgolaszewski@baylibre.com; linux-gpio@vger.kernel.org; linux-acpi@vger.kernel.org
Subject: Re: [PATCH v1 1/1] Add driver for Mellanox BlueField-3 GPIO controller
Importance: High

On Mon, May 16, 2022 at 3:00 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:

> So these GPIO pins are assigned one specific HW functionality on the 
> boards  and software should never change them.
>
> By default, for security purposes, I think we shouldn't let the user 
> have the option to control the GPIO pins since they have a specific HW functionality.
>
> But for bringup/debug purposes, we would like to support the option of 
> software being able to change these pin values. We also might have 
> customers that choose to change the default HW connection of a certain 
> GPIO pin and connect it to control their LEDs for instance.

The fact that the usecase is bringup/debug does not mean we cut corners and do "quick fixes". The proper APIs have to be implemented, the alternative is to not submit the driver at all.

What I hear is that these pins have two modes:

1. Used for a device (I2C etc)
2. Used as GPIO by setting a bit in YU_GPIO_FW_CONTROL_SET

This is two pin control multiplexing states already.

So this should have a simple pin control driver as back-end with the GPIO as front end. A shortcut to enabling pins into GPIO mode can be provided using .gpio_request_enable() from struct pinmux_ops.

Please refer to
https://docs.kernel.org/driver-api/pin-control.html

I know this means more work and is kind of complex. But drivers/pinctrl has a lot of examples you can follow, for example drivers/pinctrl/pinctrl-sx150x.c and other simple multipurpose chips.

Yours,
Linus Walleij

On Fri, Sep 2, 2022 at 5:55 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:

> During testing, I use the sysfs to change the gpio value as follows:
> Cd /sys/class/gpio
> echo 480 > export
> When I do the export, I see that gpiochip_generic_request is being called which calls .gpio_request_enable = mlxbf_gpio_request_enable.

Yes but don't use the deprecated sysfs to test GPIO, use libgpiod
https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/

> Is this how it also works in other driver? Or am I missing something?

The gpio_chip usually works as a front end for pin control.

> I wanted to disallow muxing from user space.

If you ask for a GPIO then it will be muxed in if you implement
.gpio_request_enable().

If you want to make it impossible to use certain gpios alter
.valid_mask.

If you don't want people to use the sysfs ABI (which by the
way requires you to first select the "CONFIG_EXPERT"
option) then do not compile it into the kernel. It is a big
risk to use it in any case, so just don't.

If you use the character device (which is enabled by default),
you can set permissions on /dev/gpiochipN such that only
privileged users can access it, just like you protect any
other block/character device.

> I would like that to be controlled by the ACPI table only.

I don't know if it is possible to restrict GPIOs to just be
used from ACPI.

> For example, use devm_gpio_request from some other driver if needed.

If you only want other kernel consumers to use GPIOs,
the disable the sysfs ABI, and also disable the character
device, then only the kernel can use GPIOs.

Yours,
Linus Walleij

Thank you for your response! All clear! 

-----Original Message-----
From: Linus Walleij <linus.walleij@linaro.org> 
Sent: Friday, September 2, 2022 5:55 PM
To: Asmaa Mnebhi <asmaa@nvidia.com>
Cc: andy.shevchenko@gmail.com; bgolaszewski@baylibre.com; linux-gpio@vger.kernel.org; linux-acpi@vger.kernel.org
Subject: Re: [PATCH v1 1/1] Add driver for Mellanox BlueField-3 GPIO controller
Importance: High

On Fri, Sep 2, 2022 at 5:55 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:

> During testing, I use the sysfs to change the gpio value as follows:
> Cd /sys/class/gpio
> echo 480 > export
> When I do the export, I see that gpiochip_generic_request is being called which calls .gpio_request_enable = mlxbf_gpio_request_enable.

Yes but don't use the deprecated sysfs to test GPIO, use libgpiod https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/

> Is this how it also works in other driver? Or am I missing something?

The gpio_chip usually works as a front end for pin control.

> I wanted to disallow muxing from user space.

If you ask for a GPIO then it will be muxed in if you implement .gpio_request_enable().

If you want to make it impossible to use certain gpios alter .valid_mask.

If you don't want people to use the sysfs ABI (which by the way requires you to first select the "CONFIG_EXPERT"
option) then do not compile it into the kernel. It is a big risk to use it in any case, so just don't.

If you use the character device (which is enabled by default), you can set permissions on /dev/gpiochipN such that only privileged users can access it, just like you protect any other block/character device.

> I would like that to be controlled by the ACPI table only.

I don't know if it is possible to restrict GPIOs to just be used from ACPI.

> For example, use devm_gpio_request from some other driver if needed.

If you only want other kernel consumers to use GPIOs, the disable the sysfs ABI, and also disable the character device, then only the kernel can use GPIOs.

Yours,
Linus Walleij

On Tue, Sep 6, 2022 at 5:08 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:
> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: Friday, September 2, 2022 5:55 PM
> On Fri, Sep 2, 2022 at 5:55 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:

...

> > I would like that to be controlled by the ACPI table only.
>
> I don't know if it is possible to restrict GPIOs to just be used from ACPI.

On Intel platforms that's done with HW assistance, i.e. firmware
configures special registers inside pin control / GPIO address space
that reflect ownership. The driver rejects any attempts to request
such GPIOs. As a workaround one may simply disable all user space
interfaces (as you already discussed in this thread).