Add support for the BlueField-3 SoC GPIO driver.
This driver configures and handles GPIO interrupts. It also enables a user
to manipulate certain GPIO pins via libgpiod tools or other kernel drivers.
The usables pins are defined via the gpio-reserved-ranges property.
Signed-off-by: Asmaa Mnebhi <[email protected]>
---
drivers/gpio/Kconfig | 12 ++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-mlxbf3.c | 230 +++++++++++++++++++++++++++++++++++++
3 files changed, 243 insertions(+)
create mode 100644 drivers/gpio/gpio-mlxbf3.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index ec7cfd4f52b1..5cddcb56353d 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1534,6 +1534,18 @@ 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 if you want GPIO support on Mellanox BlueField 3 SoC.
+ This GPIO controller supports interrupt handling and enables the
+ manipulation of certain GPIO pins.
+ This controller should be used in parallel with pinctrl-mlxbf to
+ control the desired gpios.
+ This driver can also be built as a module called mlxbf3-gpio.
+
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 010587025fc8..76545ca31457 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -101,6 +101,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..cdc93c8e77ff
--- /dev/null
+++ b/drivers/gpio/gpio-mlxbf3.c
@@ -0,0 +1,230 @@
+// SPDX-License-Identifier: GPL-2.0-only or BSD-3-Clause
+/* Copyright (C) 2022 NVIDIA CORPORATION & AFFILIATES */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/resource.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+#include <linux/version.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 MLXBF_GPIO_FW_OUTPUT_ENABLE_SET 0x04
+#define MLXBF_GPIO_FW_DATA_OUT_SET 0x08
+#define MLXBF_GPIO_FW_OUTPUT_ENABLE_CLEAR 0x18
+#define MLXBF_GPIO_FW_DATA_OUT_CLEAR 0x1c
+#define MLXBF_GPIO_CAUSE_RISE_EN 0x28
+#define MLXBF_GPIO_CAUSE_FALL_EN 0x2c
+#define MLXBF_GPIO_READ_DATA_IN 0x30
+
+#define MLXBF_GPIO_CAUSE_OR_CAUSE_EVTEN0 0x00
+#define MLXBF_GPIO_CAUSE_OR_EVTEN0 0x14
+#define MLXBF_GPIO_CAUSE_OR_CLRCAUSE 0x18
+
+struct mlxbf3_gpio_context {
+ struct gpio_chip gc;
+
+ /* YU GPIO block address */
+ void __iomem *gpio_io;
+
+ /* YU GPIO cause block address */
+ void __iomem *gpio_cause_io;
+};
+
+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;
+
+ raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
+ writel(BIT(offset), gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_CLRCAUSE);
+
+ val = readl(gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
+ val |= BIT(offset);
+ writel(val, gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
+ raw_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;
+
+ raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
+ val = readl(gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
+ val &= ~BIT(offset);
+ writel(val, gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
+ raw_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 + MLXBF_GPIO_CAUSE_OR_CAUSE_EVTEN0);
+ writel(pending, gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_CLRCAUSE);
+
+ for_each_set_bit(level, &pending, gc->ngpio)
+ generic_handle_domain_irq(gc->irq.domain, level);
+
+ 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;
+ u32 val;
+
+ raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
+
+ switch (type & IRQ_TYPE_SENSE_MASK) {
+ case IRQ_TYPE_EDGE_BOTH:
+ val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
+ val |= BIT(offset);
+ writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
+ val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
+ val |= BIT(offset);
+ writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
+ break;
+ case IRQ_TYPE_EDGE_RISING:
+ val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
+ val |= BIT(offset);
+ writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
+ val |= BIT(offset);
+ writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
+
+ irq_set_handler_locked(irqd, handle_simple_irq);
+
+ return 0;
+}
+
+static const struct irq_chip gpio_mlxbf3_irqchip = {
+ .name = "MLNXBF33",
+ .irq_set_type = mlxbf3_gpio_irq_set_type,
+ .irq_enable = mlxbf3_gpio_irq_enable,
+ .irq_disable = mlxbf3_gpio_irq_disable,
+};
+
+static int
+mlxbf3_gpio_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct mlxbf3_gpio_context *gs;
+ struct gpio_irq_chip *girq;
+ struct gpio_chip *gc;
+ unsigned int npins;
+ int ret, irq;
+
+ gs = devm_kzalloc(dev, sizeof(*gs), GFP_KERNEL);
+ if (!gs)
+ return -ENOMEM;
+
+ gs->gpio_io = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(gs->gpio_io))
+ return PTR_ERR(gs->gpio_io);
+
+ gs->gpio_cause_io = devm_platform_ioremap_resource(pdev, 1);
+ if (IS_ERR(gs->gpio_cause_io))
+ return PTR_ERR(gs->gpio_cause_io);
+
+ npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
+ device_property_read_u32(dev, "npins", &npins);
+
+ gc = &gs->gc;
+
+ ret = bgpio_init(gc, dev, 4,
+ gs->gpio_io + MLXBF_GPIO_READ_DATA_IN,
+ gs->gpio_io + MLXBF_GPIO_FW_DATA_OUT_SET,
+ gs->gpio_io + MLXBF_GPIO_FW_DATA_OUT_CLEAR,
+ gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_SET,
+ gs->gpio_io + MLXBF_GPIO_FW_OUTPUT_ENABLE_CLEAR, 0);
+
+ gc->request = gpiochip_generic_request;
+ gc->free = gpiochip_generic_free;
+
+ gc->ngpio = npins;
+ gc->owner = THIS_MODULE;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq >= 0) {
+ girq = &gs->gc.irq;
+ gpio_irq_chip_set_chip(girq, &gpio_mlxbf3_irqchip);
+ 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, dev_name(dev), gs);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to request IRQ");
+ }
+
+ platform_set_drvdata(pdev, gs);
+
+ ret = devm_gpiochip_add_data(dev, &gs->gc, gs);
+ if (ret)
+ dev_err_probe(dev, ret, "Failed adding memory mapped gpiochip\n");
+
+ 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,
+};
+module_platform_driver(mlxbf3_gpio_driver);
+
+MODULE_DESCRIPTION("NVIDIA BlueField-3 GPIO Driver");
+MODULE_AUTHOR("Asmaa Mnebhi <[email protected]>");
+MODULE_LICENSE("Dual BSD/GPL");
--
2.30.1
On Fri, Feb 17, 2023 at 11:27 PM Asmaa Mnebhi <[email protected]> wrote:
>
> Add support for the BlueField-3 SoC GPIO driver.
> This driver configures and handles GPIO interrupts. It also enables a user
> to manipulate certain GPIO pins via libgpiod tools or other kernel drivers.
> The usables pins are defined via the gpio-reserved-ranges property.
Probably
"gpio-reserved-ranges"
(in double quotes).
...
> + depends on (MELLANOX_PLATFORM && ARM64 && ACPI) || (64BIT && COMPILE_TEST)
But do you really need the ACPI dependency? I don't see a single bit
of it in the driver.
(Yes, I know about IDs.)
Also, how 64BIT is needed for testing?
...
> +#include <linux/version.h>
I believe it's included automatically. Please, double check the header
inclusions to make sure you don't have some spurious ones in the list.
> +struct mlxbf3_gpio_context {
> + struct gpio_chip gc;
> +
> + /* YU GPIO block address */
> + void __iomem *gpio_io;
> +
> + /* YU GPIO cause block address */
> + void __iomem *gpio_cause_io;
> +};
...
> + int offset = irqd_to_hwirq(irqd);
It returns irq_hw_number_t IIRC. It's an unsigned type. Otherwise you
may have interesting effects.
...
> + int offset = irqd_to_hwirq(irqd);
Ditto.
...
> + raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> +
> + switch (type & IRQ_TYPE_SENSE_MASK) {
> + case IRQ_TYPE_EDGE_BOTH:
> + val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
> + val |= BIT(offset);
> + writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
> + val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
> + val |= BIT(offset);
> + writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
> + break;
> + case IRQ_TYPE_EDGE_RISING:
> + val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
> + val |= BIT(offset);
> + writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
> + break;
> + case IRQ_TYPE_EDGE_FALLING:
> + val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
> + val |= BIT(offset);
> + writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
> + break;
> + default:
Dead lock starts here.
> + return -EINVAL;
> + }
> +
> + raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
...
> + irq_set_handler_locked(irqd, handle_simple_irq);
Why not using propert handler type (edge)?
...
> +static const struct irq_chip gpio_mlxbf3_irqchip = {
> + .name = "MLNXBF33",
> + .irq_set_type = mlxbf3_gpio_irq_set_type,
> + .irq_enable = mlxbf3_gpio_irq_enable,
> + .irq_disable = mlxbf3_gpio_irq_disable,
> +};
Seems missing two things (dunno if bgpio_init() helps with that):
- IMMUTABLE flag
- actual calls to enable and disable IRQs for internal GPIO library usage
See other drivers how it's done. There are even plenty of patches to
enable this thing separately.
...
> +static int
> +mlxbf3_gpio_probe(struct platform_device *pdev)
Doesn't it perfectly fit one line?
...
> + npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
> + device_property_read_u32(dev, "npins", &npins);
I don't see DT bindings for this property (being added in this
series). Is it already established one?
...
> + girq->default_type = IRQ_TYPE_NONE;
Assigning handle_bad_irq() is missing.
--
With Best Regards,
Andy Shevchenko
Hi Asmaa,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linusw-pinctrl/devel]
[also build test WARNING on linusw-pinctrl/for-next brgl/gpio/for-next linus/master v6.2-rc8 next-20230217]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Asmaa-Mnebhi/pinctrl-pinctrl-mlxbf-Add-pinctrl-driver-support/20230218-062744
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
patch link: https://lore.kernel.org/r/28f0d670407c127614b64d9c382b11c795f5077d.1676668853.git.asmaa%40nvidia.com
patch subject: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support
reproduce:
make versioncheck
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
versioncheck warnings: (new ones prefixed by >>)
INFO PATH=/opt/cross/clang/bin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
/usr/bin/timeout -k 100 3h /usr/bin/make W=1 --keep-going HOSTCC=gcc-11 CC=gcc-11 -j32 ARCH=x86_64 versioncheck
find ./* \( -name SCCS -o -name BitKeeper -o -name .svn -o -name CVS -o -name .pc -o -name .hg -o -name .git \) -prune -o \
-name '*.[hcS]' -type f -print | sort \
| xargs perl -w ./scripts/checkversion.pl
./drivers/accessibility/speakup/genmap.c: 13 linux/version.h not needed.
./drivers/accessibility/speakup/makemapdata.c: 13 linux/version.h not needed.
>> ./drivers/gpio/gpio-mlxbf3.c: 14 linux/version.h not needed.
./drivers/net/ethernet/qlogic/qede/qede.h: 10 linux/version.h not needed.
./drivers/net/ethernet/qlogic/qede/qede_ethtool.c: 7 linux/version.h not needed.
./drivers/scsi/cxgbi/libcxgbi.h: 27 linux/version.h not needed.
./drivers/scsi/mpi3mr/mpi3mr.h: 32 linux/version.h not needed.
./drivers/scsi/qedi/qedi_dbg.h: 14 linux/version.h not needed.
./drivers/soc/tegra/cbb/tegra-cbb.c: 19 linux/version.h not needed.
./drivers/soc/tegra/cbb/tegra194-cbb.c: 26 linux/version.h not needed.
./drivers/soc/tegra/cbb/tegra234-cbb.c: 27 linux/version.h not needed.
./drivers/staging/media/atomisp/include/linux/atomisp.h: 23 linux/version.h not needed.
./init/version-timestamp.c: 5 linux/version.h not needed.
./samples/trace_events/trace_custom_sched.c: 11 linux/version.h not needed.
./sound/soc/codecs/cs42l42.c: 14 linux/version.h not needed.
./tools/lib/bpf/bpf_helpers.h: 289: need linux/version.h
./tools/perf/tests/bpf-script-example.c: 60: need linux/version.h
./tools/perf/tests/bpf-script-test-kbuild.c: 21: need linux/version.h
./tools/perf/tests/bpf-script-test-prologue.c: 47: need linux/version.h
./tools/perf/tests/bpf-script-test-relocation.c: 51: need linux/version.h
./tools/testing/selftests/bpf/progs/dev_cgroup.c: 9 linux/version.h not needed.
./tools/testing/selftests/bpf/progs/netcnt_prog.c: 3 linux/version.h not needed.
./tools/testing/selftests/bpf/progs/test_map_lock.c: 4 linux/version.h not needed.
./tools/testing/selftests/bpf/progs/test_send_signal_kern.c: 4 linux/version.h not needed.
./tools/testing/selftests/bpf/progs/test_spin_lock.c: 4 linux/version.h not needed.
./tools/testing/selftests/bpf/progs/test_tcp_estats.c: 37 linux/version.h not needed.
./tools/testing/selftests/wireguard/qemu/init.c: 27 linux/version.h not needed.
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
> +static const struct irq_chip gpio_mlxbf3_irqchip = {
> + .name = "MLNXBF33",
> + .irq_set_type = mlxbf3_gpio_irq_set_type,
> + .irq_enable = mlxbf3_gpio_irq_enable,
> + .irq_disable = mlxbf3_gpio_irq_disable, };
Seems missing two things (dunno if bgpio_init() helps with that):
- IMMUTABLE flag
- actual calls to enable and disable IRQs for internal GPIO library usage
See other drivers how it's done. There are even plenty of patches to enable this thing separately.
I saw that in other drivers only irq_enable and irq_disable are defined. Example in gpio-davinci.c:
static struct irq_chip gpio_irqchip = {
.name = "GPIO",
.irq_enable = gpio_irq_enable,
.irq_disable = gpio_irq_disable,
.irq_set_type = gpio_irq_type,
.flags = IRQCHIP_SET_TYPE_MASKED,
};
Which I think is ok due to the following logic:
gpiochip_add_irqchip calls
gpiochip_set_irq_hooks which contains the following logic:
if (irqchip->irq_disable) {
gc->irq.irq_disable = irqchip->irq_disable;
irqchip->irq_disable = gpiochip_irq_disable;
} else {
gc->irq.irq_mask = irqchip->irq_mask;
irqchip->irq_mask = gpiochip_irq_mask;
}
if (irqchip->irq_enable) {
gc->irq.irq_enable = irqchip->irq_enable;
irqchip->irq_enable = gpiochip_irq_enable;
} else {
gc->irq.irq_unmask = irqchip->irq_unmask;
irqchip->irq_unmask = gpiochip_irq_unmask;
}
So it doesn’t seem like we need to define irq_mask/unmask if we have irq_enable/disable?
> + npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
> + device_property_read_u32(dev, "npins", &npins);
I don't see DT bindings for this property (being added in this series). Is it already established one?
Ah that’s my bad. The property should be called "ngpios" like in the DT documentation. Will fix.
--
With Best Regards,
Andy Shevchenko
On Wed, Feb 22, 2023 at 8:40 PM Asmaa Mnebhi <[email protected]> wrote:
First of all, please, please, fix your email client!
It's so hard to distinguish my own words from yours.
> > +static const struct irq_chip gpio_mlxbf3_irqchip = {
> > + .name = "MLNXBF33",
> > + .irq_set_type = mlxbf3_gpio_irq_set_type,
> > + .irq_enable = mlxbf3_gpio_irq_enable,
> > + .irq_disable = mlxbf3_gpio_irq_disable, };
>
> Seems missing two things (dunno if bgpio_init() helps with that):
> - IMMUTABLE flag
> - actual calls to enable and disable IRQs for internal GPIO library usage
> See other drivers how it's done. There are even plenty of patches to enable this thing separately.
>
> I saw that in other drivers only irq_enable and irq_disable are defined. Example in gpio-davinci.c:
> static struct irq_chip gpio_irqchip = {
> .name = "GPIO",
> .irq_enable = gpio_irq_enable,
> .irq_disable = gpio_irq_disable,
> .irq_set_type = gpio_irq_type,
> .flags = IRQCHIP_SET_TYPE_MASKED,
> };
>
> Which I think is ok due to the following logic:
>
> gpiochip_add_irqchip calls
> gpiochip_set_irq_hooks which contains the following logic:
>
> if (irqchip->irq_disable) {
> gc->irq.irq_disable = irqchip->irq_disable;
> irqchip->irq_disable = gpiochip_irq_disable;
> } else {
> gc->irq.irq_mask = irqchip->irq_mask;
> irqchip->irq_mask = gpiochip_irq_mask;
> }
> if (irqchip->irq_enable) {
> gc->irq.irq_enable = irqchip->irq_enable;
> irqchip->irq_enable = gpiochip_irq_enable;
> } else {
> gc->irq.irq_unmask = irqchip->irq_unmask;
> irqchip->irq_unmask = gpiochip_irq_unmask;
> }
The chips have another flag there:
.flags = IRQCHIP_IMMUTABLE,
GPIOCHIP_IRQ_RESOURCE_HELPERS,
> So it doesn’t seem like we need to define irq_mask/unmask if we have irq_enable/disable?
>
> > + npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
> > + device_property_read_u32(dev, "npins", &npins);
>
> I don't see DT bindings for this property (being added in this series). Is it already established one?
>
> Ah that’s my bad. The property should be called "ngpios" like in the DT documentation. Will fix.
And why do you need it? What's a corner case that the GPIO library
doesn't handle yet?
--
With Best Regards,
Andy Shevchenko
Hi Asamaa,
thanks for your patch!
getting a lot better all the time.
On Fri, Feb 17, 2023 at 10:27 PM Asmaa Mnebhi <[email protected]> wrote:
> Add support for the BlueField-3 SoC GPIO driver.
> This driver configures and handles GPIO interrupts. It also enables a user
> to manipulate certain GPIO pins via libgpiod tools or other kernel drivers.
> The usables pins are defined via the gpio-reserved-ranges property.
>
> Signed-off-by: Asmaa Mnebhi <[email protected]>
(...)
> +config GPIO_MLXBF3
> + tristate "Mellanox BlueField 3 SoC GPIO"
> + depends on (MELLANOX_PLATFORM && ARM64 && ACPI) || (64BIT && COMPILE_TEST)
> + select GPIO_GENERIC
select GPIOLIB_IRQCHIP
since you use it.
(...)
> +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;
> +
> + raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> + writel(BIT(offset), gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_CLRCAUSE);
> +
> + val = readl(gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
> + val |= BIT(offset);
> + writel(val, gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
> + raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
Here, at the end (*after* writing registers) call:
gpiochip_disable_irq(gc, offset);
> +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;
> +
Here, at the beginning (*before* writing registers) call:
gpiochip_disable_irq(gc, offset);
> + raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> + val = readl(gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
> + val &= ~BIT(offset);
> + writel(val, gs->gpio_cause_io + MLXBF_GPIO_CAUSE_OR_EVTEN0);
> + raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
> +}
(...)
> +static const struct irq_chip gpio_mlxbf3_irqchip = {
> + .name = "MLNXBF33",
> + .irq_set_type = mlxbf3_gpio_irq_set_type,
> + .irq_enable = mlxbf3_gpio_irq_enable,
> + .irq_disable = mlxbf3_gpio_irq_disable,
Like Andy said:
.flags = IRQCHIP_IMMUTABLE,
GPIOCHIP_IRQ_RESOURCE_HELPERS,
Apart from this it looks all right.
Yours,
Linus Walleij
> > > + npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
> > > + device_property_read_u32(dev, "npins", &npins);
> >
> > I don't see DT bindings for this property (being added in this series). Is it
> already established one?
> >
> > Ah that’s my bad. The property should be called "ngpios" like in the DT
> documentation. Will fix.
>
> And why do you need it? What's a corner case that the GPIO library doesn't
> handle yet?
We have 2 gpiochips, gpiochip 0 supports 32 gpio pins and gpiochip 1 supports only 24 pins.
If I remove the logic from gpio-mlxbf3.c, the gpiolib.c logic will correctly set the ngpios = 32 for gpiochip 0 but will wrongly set ngpios=32 for gpiogchip 1:
gpiochip_add_data_with_key {
[...]
ngpios = gc->ngpio;
if (ngpios == 0) {
ret = device_property_read_u32(&gdev->dev, "ngpios", &ngpios);
if (ret == -ENODATA)
/*
* -ENODATA means that there is no property found and
* we want to issue the error message to the user.
* Besides that, we want to return different error code
* to state that supplied value is not valid.
*/
ngpios = 0;
else if (ret)
goto err_free_dev_name;
gc->ngpio = ngpios;
}
[...]
}
bgpio_init {
gc->bgpio_bits = sz * 8;
gc->ngpio = gc->bgpio_bits;
}
On Thu, Feb 23, 2023 at 9:08 PM Asmaa Mnebhi <[email protected]> wrote:
>
> > > > + npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
> > > > + device_property_read_u32(dev, "npins", &npins);
> > >
> > > I don't see DT bindings for this property (being added in this series). Is it
> > already established one?
> > >
> > > Ah that’s my bad. The property should be called "ngpios" like in the DT
> > documentation. Will fix.
> >
> > And why do you need it? What's a corner case that the GPIO library doesn't
> > handle yet?
>
> We have 2 gpiochips, gpiochip 0 supports 32 gpio pins and gpiochip 1 supports only 24 pins.
> If I remove the logic from gpio-mlxbf3.c, the gpiolib.c logic will correctly set the ngpios = 32 for gpiochip 0 but will wrongly set ngpios=32 for gpiogchip 1:
So, either you need to have two entries in DT per chip or ngpios should be 56.
--
With Best Regards,
Andy Shevchenko
> >
> > > > > + npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
> > > > > + device_property_read_u32(dev, "npins", &npins);
> > > >
> > > > I don't see DT bindings for this property (being added in this
> > > > series). Is it
> > > already established one?
> > > >
> > > > Ah that’s my bad. The property should be called "ngpios" like in
> > > > the DT
> > > documentation. Will fix.
> > >
> > > And why do you need it? What's a corner case that the GPIO library
> > > doesn't handle yet?
> >
> > We have 2 gpiochips, gpiochip 0 supports 32 gpio pins and gpiochip 1
> supports only 24 pins.
> > If I remove the logic from gpio-mlxbf3.c, the gpiolib.c logic will correctly set
> the ngpios = 32 for gpiochip 0 but will wrongly set ngpios=32 for gpiogchip 1:
>
> So, either you need to have two entries in DT per chip or ngpios should be 56.
>
I already have 2 entries in my ACPI table, in the first entry, ngpios = 32 and in the second entry ngpios = 24.
Gpiochip_add_data_with_keys only reads the ngpios property if (ngpios == 0) which is not the case when
bgpio_init is called. bgpio_init uses "sz" argument to populate the ngpio in bgpio_init, which is not what we want.
On Fri, Feb 24, 2023 at 12:51 AM Asmaa Mnebhi <[email protected]> wrote:
...
> > > > > Ah that’s my bad. The property should be called "ngpios" like in
> > > > > the DT
> > > > documentation. Will fix.
> > > >
> > > > And why do you need it? What's a corner case that the GPIO library
> > > > doesn't handle yet?
> > >
> > > We have 2 gpiochips, gpiochip 0 supports 32 gpio pins and gpiochip 1
> > supports only 24 pins.
> > > If I remove the logic from gpio-mlxbf3.c, the gpiolib.c logic will correctly set
> > the ngpios = 32 for gpiochip 0 but will wrongly set ngpios=32 for gpiogchip 1:
> >
> > So, either you need to have two entries in DT per chip or ngpios should be 56.
> >
> I already have 2 entries in my ACPI table, in the first entry, ngpios = 32 and in the second entry ngpios = 24.
Can you show the DSDT excerpt of this device? (Also including the
pieces for pin control)
Is this a table of the device in the wild?
> Gpiochip_add_data_with_keys only reads the ngpios property if (ngpios == 0) which is not the case when
> bgpio_init is called. bgpio_init uses "sz" argument to populate the ngpio in bgpio_init, which is not what we want.
Maybe bgpio_init() is not a good API for your case?
--
With Best Regards,
Andy Shevchenko
> > > > > > Ah that’s my bad. The property should be called "ngpios" like
> > > > > > in the DT
> > > > > documentation. Will fix.
> > > > >
> > > > > And why do you need it? What's a corner case that the GPIO
> > > > > library doesn't handle yet?
> > > >
> > > > We have 2 gpiochips, gpiochip 0 supports 32 gpio pins and gpiochip
> > > > 1
> > > supports only 24 pins.
> > > > If I remove the logic from gpio-mlxbf3.c, the gpiolib.c logic will
> > > > correctly set
> > > the ngpios = 32 for gpiochip 0 but will wrongly set ngpios=32 for gpiogchip
> 1:
> > >
> > > So, either you need to have two entries in DT per chip or ngpios should be
> 56.
> > >
> > I already have 2 entries in my ACPI table, in the first entry, ngpios = 32 and
> in the second entry ngpios = 24.
>
> Can you show the DSDT excerpt of this device? (Also including the pieces for
> pin control)
Our ACPI tables are internal:
Device(GPI0) {
Name(_HID, "MLNXBF33")
Name(_UID, Zero)
Name(_CCA, 1)
Name(_CRS, ResourceTemplate() {
// for fw_gpio[0] yu block
Memory32Fixed(ReadWrite, 0x13401100, 0x00000080)
// for yu_gpio_causereg0 block
Memory32Fixed(ReadWrite, 0x13401c00, 0x00000020)
Interrupt(ResourceConsumer, Edge, ActiveHigh, Shared)
{ BF_RSH0_YU }
})
Name(_DSD, Package() {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package() {
Package () { "ngpios", 32 }, // Number of GPIO pins on gpio block 0
Package () { "gpio-reserved-ranges", Package () {5, 1, 7, 3, 11, 31}},
}
})
}
Device(GPI1) {
Name(_HID, "MLNXBF33")
Name(_UID, 1)
Name(_CCA, 1)
Name(_CRS, ResourceTemplate() {
// for fw_gpio[1] yu block
Memory32Fixed(ReadWrite, 0x13401180, 0x00000080)
// for yu_gpio_causereg1 block
Memory32Fixed(ReadWrite, 0x13401c20, 0x00000020)
})
Name(_DSD, Package() {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package() {
Package () { "ngpios", 24 }, // Number of GPIO pins on gpio block 1
Package () { "gpio-reserved-ranges", Package () {0, 21}},
}
})
}
Device(PIN0) {
Name(_HID, "MLNXBF34")
Name(_UID, Zero)
Name(_CCA, 1)
Name(_CRS, ResourceTemplate() {
// for fw_gpio[0] and fw_gpio[1] yu blocks
Memory32Fixed(ReadWrite, 0x13401100, 0x00000100)
})
}
>
> > Gpiochip_add_data_with_keys only reads the ngpios property if (ngpios
> > == 0) which is not the case when bgpio_init is called. bgpio_init uses "sz"
> argument to populate the ngpio in bgpio_init, which is not what we want.
>
> Maybe bgpio_init() is not a good API for your case?
At the moment, bgpio_init handles all the direction in/out get/set gpio value for us.
So I can either remove bgpio_init so that we get the correct ngpios property, but will have to define get_gpio, set_gpio, dir in and dir out.
Or I can keep bgpio_init and device_property_read_u32 in the gpio-mlxbf3.c driver.
Please let me know which approach you prefer.
On Fri, Feb 24, 2023 at 4:42 PM Asmaa Mnebhi <[email protected]> wrote:
>
> > > > > > > Ah that’s my bad. The property should be called "ngpios" like
> > > > > > > in the DT
> > > > > > documentation. Will fix.
> > > > > >
> > > > > > And why do you need it? What's a corner case that the GPIO
> > > > > > library doesn't handle yet?
> > > > >
> > > > > We have 2 gpiochips, gpiochip 0 supports 32 gpio pins and gpiochip
> > > > > 1
> > > > supports only 24 pins.
> > > > > If I remove the logic from gpio-mlxbf3.c, the gpiolib.c logic will
> > > > > correctly set
> > > > the ngpios = 32 for gpiochip 0 but will wrongly set ngpios=32 for gpiogchip
> > 1:
> > > >
> > > > So, either you need to have two entries in DT per chip or ngpios should be
> > 56.
> > > >
> > > I already have 2 entries in my ACPI table, in the first entry, ngpios = 32 and
> > in the second entry ngpios = 24.
> >
> > Can you show the DSDT excerpt of this device? (Also including the pieces for
> > pin control)
>
> Our ACPI tables are internal:
>
> Device(GPI0) {
> Name(_HID, "MLNXBF33")
> Name(_UID, Zero)
> Name(_CCA, 1)
> Name(_CRS, ResourceTemplate() {
> // for fw_gpio[0] yu block
> Memory32Fixed(ReadWrite, 0x13401100, 0x00000080)
> // for yu_gpio_causereg0 block
> Memory32Fixed(ReadWrite, 0x13401c00, 0x00000020)
> Interrupt(ResourceConsumer, Edge, ActiveHigh, Shared)
> { BF_RSH0_YU }
> })
> Name(_DSD, Package() {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package() {
> Package () { "ngpios", 32 }, // Number of GPIO pins on gpio block 0
> Package () { "gpio-reserved-ranges", Package () {5, 1, 7, 3, 11, 31}},
> }
> })
> }
> Device(GPI1) {
> Name(_HID, "MLNXBF33")
> Name(_UID, 1)
> Name(_CCA, 1)
> Name(_CRS, ResourceTemplate() {
> // for fw_gpio[1] yu block
> Memory32Fixed(ReadWrite, 0x13401180, 0x00000080)
> // for yu_gpio_causereg1 block
> Memory32Fixed(ReadWrite, 0x13401c20, 0x00000020)
> })
> Name(_DSD, Package() {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package() {
> Package () { "ngpios", 24 }, // Number of GPIO pins on gpio block 1
> Package () { "gpio-reserved-ranges", Package () {0, 21}},
> }
> })
> }
>
> Device(PIN0) {
> Name(_HID, "MLNXBF34")
> Name(_UID, Zero)
> Name(_CCA, 1)
> Name(_CRS, ResourceTemplate() {
> // for fw_gpio[0] and fw_gpio[1] yu blocks
> Memory32Fixed(ReadWrite, 0x13401100, 0x00000100)
> })
> }
So far I see no issues with the tables. Thanks for sharing!
> > > Gpiochip_add_data_with_keys only reads the ngpios property if (ngpios
> > > == 0) which is not the case when bgpio_init is called. bgpio_init uses "sz"
> > argument to populate the ngpio in bgpio_init, which is not what we want.
> >
> > Maybe bgpio_init() is not a good API for your case?
>
> At the moment, bgpio_init handles all the direction in/out get/set gpio value for us.
> So I can either remove bgpio_init so that we get the correct ngpios property, but will have to define get_gpio, set_gpio, dir in and dir out.
> Or I can keep bgpio_init and device_property_read_u32 in the gpio-mlxbf3.c driver.
So far it seems the issue is in bgpio_init() that doesn't handle
ngpios properly. Maybe we need to fix this first?
Btw, have you considered using the gpio-regmap approach?
--
With Best Regards,
Andy Shevchenko
On Fri, Feb 24, 2023 at 4:42 PM Asmaa Mnebhi <[email protected]> wrote:
> Package () { "gpio-reserved-ranges", Package () {5, 1, 7, 3, 11, 31}},
Side note, doesn't it the same to
Package () { "gpio-reserved-ranges", Package () {5, 1, 7, 34}},
?
--
With Best Regards,
Andy Shevchenko
On Fri, Feb 24, 2023 at 5:13 PM Andy Shevchenko
<[email protected]> wrote:
> On Fri, Feb 24, 2023 at 4:42 PM Asmaa Mnebhi <[email protected]> wrote:
>
> > Package () { "gpio-reserved-ranges", Package () {5, 1, 7, 3, 11, 31}},
>
> Side note, doesn't it the same to
>
> Package () { "gpio-reserved-ranges", Package () {5, 1, 7, 34}},
>
> ?
Ah, now I got it, the 10 is OK, while 7-9 and 11+ are not.
Sorry for the noise.
--
With Best Regards,
Andy Shevchenko
> > > > Gpiochip_add_data_with_keys only reads the ngpios property if
> > > > (ngpios == 0) which is not the case when bgpio_init is called. bgpio_init
> uses "sz"
> > > argument to populate the ngpio in bgpio_init, which is not what we want.
> > >
> > > Maybe bgpio_init() is not a good API for your case?
> >
> > At the moment, bgpio_init handles all the direction in/out get/set gpio value
> for us.
> > So I can either remove bgpio_init so that we get the correct ngpios
> property, but will have to define get_gpio, set_gpio, dir in and dir out.
> > Or I can keep bgpio_init and device_property_read_u32 in the gpio-
> mlxbf3.c driver.
>
> So far it seems the issue is in bgpio_init() that doesn't handle ngpios properly.
> Maybe we need to fix this first?
Ok I will send a sperate patch shortly addressing this.
> Btw, have you considered using the gpio-regmap approach?
Thanks! I see examples of drivers already using this (gpio-tn48m.c). Will take a look.