This patch series adds GPIO drivers, DT documentation and DT nodes for
HiFive Unleashed board. The gpio patches are mostly based on Wesley's patch.
The patchset also adds hierarchy irq domain support as it is required by this
gpio driver.
This patchset is based on Linux 5.4-rc6 and tested on HiFive Unleashed board
Changes:
v2 vs v1:
- Add patch to introduce irq_domain_translate_onecell() and use it in
the sifive PLIC driver
- Drop the usage of own locks, instead use internal bgpio_locks
- Consistently use regmap for register access throughout the gpio code
- Convert the GPIO DT documentation into a json schema
- Other minor changes based upon feedback received on v1
v1 vs RFC:
Incorporated below changes as suggested by Linus Walleij on RFC version of this
patchset[0]
- Dropped PWM patches as they are already merged.
- Include "GPIO_GENERIC" and "REGMAP_MMIO" in Kconfig select option
- Remove unwanted inclusion of header files
- Use regmap MMIO instead of customised sifive_assign_bit()
- Use GPIOLIB_GENERIC and bgpio_init() to set up the accessors
- Use hierarchical irqdomain
[0] https://lore.kernel.org/linux-riscv/20181010123519.RVexDppaPFpIWl7QU_hpP8tc5qqWPJgeuLYn0FaGbeQ@z/
Yash Shah (5):
genirq: introduce irq_domain_translate_onecell
irqchip: sifive: Support hierarchy irq domain
gpio: sifive: Add DT documentation for SiFive GPIO
gpio: sifive: Add GPIO driver for SiFive SoCs
riscv: dts: Add DT support for SiFive FU540 GPIO driver
.../devicetree/bindings/gpio/gpio-sifive.yaml | 69 ++++++
arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 14 +-
.../riscv/boot/dts/sifive/hifive-unleashed-a00.dts | 4 +
drivers/gpio/Kconfig | 9 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-sifive.c | 256 +++++++++++++++++++++
drivers/irqchip/Kconfig | 1 +
drivers/irqchip/irq-sifive-plic.c | 30 ++-
include/linux/irqdomain.h | 5 +
kernel/irq/irqdomain.c | 20 ++
10 files changed, 404 insertions(+), 5 deletions(-)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-sifive.yaml
create mode 100644 drivers/gpio/gpio-sifive.c
--
2.7.4
Add a new function irq_domain_translate_onecell() that is to be used as
the translate function in struct irq_domain_ops for the v2 IRQ API.
Signed-off-by: Yash Shah <[email protected]>
---
include/linux/irqdomain.h | 5 +++++
kernel/irq/irqdomain.c | 20 ++++++++++++++++++++
2 files changed, 25 insertions(+)
diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 583e7ab..cad9eb8 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -426,6 +426,11 @@ int irq_domain_translate_twocell(struct irq_domain *d,
unsigned long *out_hwirq,
unsigned int *out_type);
+int irq_domain_translate_onecell(struct irq_domain *d,
+ struct irq_fwspec *fwspec,
+ unsigned long *out_hwirq,
+ unsigned int *out_type);
+
/* IPI functions */
int irq_reserve_ipi(struct irq_domain *domain, const struct cpumask *dest);
int irq_destroy_ipi(unsigned int irq, const struct cpumask *dest);
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 132672b..6972a48 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -987,6 +987,26 @@ const struct irq_domain_ops irq_domain_simple_ops = {
EXPORT_SYMBOL_GPL(irq_domain_simple_ops);
/**
+ * irq_domain_translate_onecell() - Generic translate for direct one cell
+ * bindings
+ *
+ * Device Tree IRQ specifier translation function which works with one cell
+ * bindings where the cell values map directly to the hwirq number.
+ */
+int irq_domain_translate_onecell(struct irq_domain *d,
+ struct irq_fwspec *fwspec,
+ unsigned long *out_hwirq,
+ unsigned int *out_type)
+{
+ if (WARN_ON(fwspec->param_count < 1))
+ return -EINVAL;
+ *out_hwirq = fwspec->param[0];
+ *out_type = IRQ_TYPE_NONE;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(irq_domain_translate_onecell);
+
+/**
* irq_domain_translate_twocell() - Generic translate for direct two cell
* bindings
*
--
2.7.4
DT json-schema for GPIO controller added.
Signed-off-by: Wesley W. Terpstra <[email protected]>
[Atish: Compatible string update]
Signed-off-by: Atish Patra <[email protected]>
Signed-off-by: Yash Shah <[email protected]>
---
.../devicetree/bindings/gpio/gpio-sifive.yaml | 69 ++++++++++++++++++++++
1 file changed, 69 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-sifive.yaml
diff --git a/Documentation/devicetree/bindings/gpio/gpio-sifive.yaml b/Documentation/devicetree/bindings/gpio/gpio-sifive.yaml
new file mode 100644
index 0000000..49214bb
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-sifive.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/gpio-sifive.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SiFive GPIO controller
+
+maintainers:
+ - Yash Shah <[email protected]>
+ - Paul Walmsley <[email protected]>
+
+properties:
+ compatible:
+ items:
+ - const: sifive,fu540-c000-gpio
+ - const: sifive,gpio0
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ description:
+ interrupt mapping one per GPIO. Maximum 16 GPIOs.
+ maxItems: 1
+
+ interrupt-controller: true
+
+ "#interrupt-cells":
+ const: 2
+
+ clocks:
+ maxItems: 1
+
+ clock-names: true
+
+ "#gpio-cells":
+ const: 2
+
+ gpio-controller: true
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - interrupt-controller
+ - "#interrupt-cells"
+ - clocks
+ - "#gpio-cells"
+ - gpio-controller
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/sifive-fu540-prci.h>
+ gpio@10060000 {
+ compatible = "sifive,fu540-c000-gpio", "sifive,gpio0";
+ interrupt-parent = <&plic>;
+ interrupts = <7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22>;
+ reg = <0x0 0x10060000 0x0 0x1000>;
+ clocks = <&tlclk PRCI_CLK_TLCLK>;
+ gpio-controller;
+ #gpio-cells = <2>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ };
+
+...
--
2.7.4
Adds the GPIO driver for SiFive RISC-V SoCs.
Signed-off-by: Wesley W. Terpstra <[email protected]>
[Atish: Various fixes and code cleanup]
Signed-off-by: Atish Patra <[email protected]>
Signed-off-by: Yash Shah <[email protected]>
---
drivers/gpio/Kconfig | 9 ++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-sifive.c | 256 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 266 insertions(+)
create mode 100644 drivers/gpio/gpio-sifive.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 38e096e..05e8a41 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -453,6 +453,15 @@ config GPIO_SAMA5D2_PIOBU
The difference from regular GPIOs is that they
maintain their value during backup/self-refresh.
+config GPIO_SIFIVE
+ bool "SiFive GPIO support"
+ depends on OF_GPIO
+ select GPIO_GENERIC
+ select GPIOLIB_IRQCHIP
+ select REGMAP_MMIO
+ help
+ Say yes here to support the GPIO device on SiFive SoCs.
+
config GPIO_SIOX
tristate "SIOX GPIO support"
depends on SIOX
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index d2fd19c..bf7984e 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -121,6 +121,7 @@ obj-$(CONFIG_ARCH_SA1100) += gpio-sa1100.o
obj-$(CONFIG_GPIO_SAMA5D2_PIOBU) += gpio-sama5d2-piobu.o
obj-$(CONFIG_GPIO_SCH311X) += gpio-sch311x.o
obj-$(CONFIG_GPIO_SCH) += gpio-sch.o
+obj-$(CONFIG_GPIO_SIFIVE) += gpio-sifive.o
obj-$(CONFIG_GPIO_SIOX) += gpio-siox.o
obj-$(CONFIG_GPIO_SODAVILLE) += gpio-sodaville.o
obj-$(CONFIG_GPIO_SPEAR_SPICS) += gpio-spear-spics.o
diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c
new file mode 100644
index 0000000..02666ae
--- /dev/null
+++ b/drivers/gpio/gpio-sifive.c
@@ -0,0 +1,256 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 SiFive
+ */
+#include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/of_irq.h>
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/regmap.h>
+
+#define GPIO_INPUT_VAL 0x00
+#define GPIO_INPUT_EN 0x04
+#define GPIO_OUTPUT_EN 0x08
+#define GPIO_OUTPUT_VAL 0x0C
+#define GPIO_RISE_IE 0x18
+#define GPIO_RISE_IP 0x1C
+#define GPIO_FALL_IE 0x20
+#define GPIO_FALL_IP 0x24
+#define GPIO_HIGH_IE 0x28
+#define GPIO_HIGH_IP 0x2C
+#define GPIO_LOW_IE 0x30
+#define GPIO_LOW_IP 0x34
+#define GPIO_OUTPUT_XOR 0x40
+
+#define MAX_GPIO 32
+#define SIFIVE_GPIO_IRQ_OFFSET 7
+
+struct sifive_gpio {
+ void __iomem *base;
+ struct gpio_chip gc;
+ struct regmap *regs;
+ u32 enabled;
+ unsigned int trigger[MAX_GPIO];
+ unsigned int irq_parent[MAX_GPIO];
+};
+
+static void sifive_set_ie(struct sifive_gpio *chip, unsigned int offset)
+{
+ unsigned long flags;
+ unsigned int trigger;
+
+ spin_lock_irqsave(&chip->gc.bgpio_lock, flags);
+ trigger = (chip->enabled & BIT(offset)) ? chip->trigger[offset] : 0;
+ regmap_update_bits(chip->regs, GPIO_RISE_IE, BIT(offset),
+ (trigger & IRQ_TYPE_EDGE_RISING) ? BIT(offset) : 0);
+ regmap_update_bits(chip->regs, GPIO_FALL_IE, BIT(offset),
+ (trigger & IRQ_TYPE_EDGE_FALLING) ? BIT(offset) : 0);
+ regmap_update_bits(chip->regs, GPIO_HIGH_IE, BIT(offset),
+ (trigger & IRQ_TYPE_LEVEL_HIGH) ? BIT(offset) : 0);
+ regmap_update_bits(chip->regs, GPIO_LOW_IE, BIT(offset),
+ (trigger & IRQ_TYPE_LEVEL_LOW) ? BIT(offset) : 0);
+ spin_unlock_irqrestore(&chip->gc.bgpio_lock, flags);
+}
+
+static int sifive_irq_set_type(struct irq_data *d, unsigned int trigger)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct sifive_gpio *chip = gpiochip_get_data(gc);
+ int offset = irqd_to_hwirq(d);
+
+ if (offset < 0 || offset >= gc->ngpio)
+ return -EINVAL;
+
+ chip->trigger[offset] = trigger;
+ sifive_set_ie(chip, offset);
+ return 0;
+}
+
+static void sifive_irq_enable(struct irq_data *d)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct sifive_gpio *chip = gpiochip_get_data(gc);
+ int offset = irqd_to_hwirq(d) % MAX_GPIO;
+ u32 bit = BIT(offset);
+ unsigned long flags;
+
+ irq_chip_enable_parent(d);
+
+ /* Switch to input */
+ gc->direction_input(gc, offset);
+
+ spin_lock_irqsave(&gc->bgpio_lock, flags);
+ /* Clear any sticky pending interrupts */
+ regmap_write(chip->regs, GPIO_RISE_IP, bit);
+ regmap_write(chip->regs, GPIO_FALL_IP, bit);
+ regmap_write(chip->regs, GPIO_HIGH_IP, bit);
+ regmap_write(chip->regs, GPIO_LOW_IP, bit);
+ spin_unlock_irqrestore(&gc->bgpio_lock, flags);
+
+ /* Enable interrupts */
+ assign_bit(offset, (unsigned long *)&chip->enabled, 1);
+ sifive_set_ie(chip, offset);
+}
+
+static void sifive_irq_disable(struct irq_data *d)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct sifive_gpio *chip = gpiochip_get_data(gc);
+ int offset = irqd_to_hwirq(d) % MAX_GPIO;
+
+ assign_bit(offset, (unsigned long *)&chip->enabled, 0);
+ sifive_set_ie(chip, offset);
+ irq_chip_disable_parent(d);
+}
+
+static void sifive_irq_eoi(struct irq_data *d)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct sifive_gpio *chip = gpiochip_get_data(gc);
+ int offset = irqd_to_hwirq(d) % MAX_GPIO;
+ u32 bit = BIT(offset);
+ unsigned long flags;
+
+ spin_lock_irqsave(&gc->bgpio_lock, flags);
+ /* Clear all pending interrupts */
+ regmap_write(chip->regs, GPIO_RISE_IP, bit);
+ regmap_write(chip->regs, GPIO_FALL_IP, bit);
+ regmap_write(chip->regs, GPIO_HIGH_IP, bit);
+ regmap_write(chip->regs, GPIO_LOW_IP, bit);
+ spin_unlock_irqrestore(&gc->bgpio_lock, flags);
+
+ irq_chip_eoi_parent(d);
+}
+
+static struct irq_chip sifive_irqchip = {
+ .name = "sifive-gpio",
+ .irq_set_type = sifive_irq_set_type,
+ .irq_mask = irq_chip_mask_parent,
+ .irq_unmask = irq_chip_unmask_parent,
+ .irq_enable = sifive_irq_enable,
+ .irq_disable = sifive_irq_disable,
+ .irq_eoi = sifive_irq_eoi,
+};
+
+static int sifive_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
+ unsigned int child,
+ unsigned int child_type,
+ unsigned int *parent,
+ unsigned int *parent_type)
+{
+ *parent_type = IRQ_TYPE_NONE;
+ *parent = child + SIFIVE_GPIO_IRQ_OFFSET;
+ return 0;
+}
+
+static const struct regmap_config sifive_gpio_regmap_config = {
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+ .fast_io = true,
+ .disable_locking = true,
+};
+
+static int sifive_gpio_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *node = pdev->dev.of_node;
+ struct device_node *irq_parent;
+ struct irq_domain *parent;
+ struct gpio_irq_chip *girq;
+ struct sifive_gpio *chip;
+ struct resource *res;
+ unsigned long flags;
+ int ret, ngpio;
+
+ chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+ if (!chip)
+ return -ENOMEM;
+
+ chip->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(chip->base)) {
+ dev_err(dev, "failed to allocate device memory\n");
+ return PTR_ERR(chip->base);
+ }
+
+ chip->regs = devm_regmap_init_mmio(dev, chip->base,
+ &sifive_gpio_regmap_config);
+ if (IS_ERR(chip->regs))
+ return PTR_ERR(chip->regs);
+
+ ngpio = of_irq_count(node);
+ if (ngpio >= MAX_GPIO) {
+ dev_err(dev, "Too many GPIO interrupts (max=%d)\n", MAX_GPIO);
+ return -ENXIO;
+ }
+
+ irq_parent = of_irq_find_parent(node);
+ if (!irq_parent) {
+ dev_err(dev, "no IRQ parent node\n");
+ return -ENODEV;
+ }
+ parent = irq_find_host(irq_parent);
+ if (!parent) {
+ dev_err(dev, "no IRQ parent domain\n");
+ return -ENODEV;
+ }
+
+ ret = bgpio_init(&chip->gc, dev, 4,
+ chip->base + GPIO_INPUT_VAL,
+ chip->base + GPIO_OUTPUT_VAL,
+ NULL,
+ chip->base + GPIO_OUTPUT_EN,
+ chip->base + GPIO_INPUT_EN,
+ 0);
+ if (ret) {
+ dev_err(dev, "unable to init generic GPIO\n");
+ return ret;
+ }
+
+ spin_lock_irqsave(&chip->gc.bgpio_lock, flags);
+ /* Disable all GPIO interrupts before enabling parent interrupts */
+ regmap_write(chip->regs, GPIO_RISE_IE, 0);
+ regmap_write(chip->regs, GPIO_FALL_IE, 0);
+ regmap_write(chip->regs, GPIO_HIGH_IE, 0);
+ regmap_write(chip->regs, GPIO_LOW_IE, 0);
+ spin_unlock_irqrestore(&chip->gc.bgpio_lock, flags);
+ chip->enabled = 0;
+
+ chip->gc.base = -1;
+ chip->gc.ngpio = ngpio;
+ chip->gc.label = dev_name(dev);
+ chip->gc.parent = dev;
+ chip->gc.owner = THIS_MODULE;
+ girq = &chip->gc.irq;
+ girq->chip = &sifive_irqchip;
+ girq->fwnode = of_node_to_fwnode(node);
+ girq->parent_domain = parent;
+ girq->child_to_parent_hwirq = sifive_gpio_child_to_parent_hwirq;
+ girq->handler = handle_bad_irq;
+ girq->default_type = IRQ_TYPE_NONE;
+
+ platform_set_drvdata(pdev, chip);
+ return gpiochip_add_data(&chip->gc, chip);
+}
+
+static const struct of_device_id sifive_gpio_match[] = {
+ { .compatible = "sifive,gpio0" },
+ { .compatible = "sifive,fu540-c000-gpio" },
+ { },
+};
+
+static struct platform_driver sifive_gpio_driver = {
+ .probe = sifive_gpio_probe,
+ .driver = {
+ .name = "sifive_gpio",
+ .of_match_table = of_match_ptr(sifive_gpio_match),
+ },
+};
+builtin_platform_driver(sifive_gpio_driver)
--
2.7.4
Add the gpio DT node in SiFive FU540 soc-specific DT file.
Enable the gpio node in HiFive Unleashed board-specific DT file.
Signed-off-by: Yash Shah <[email protected]>
---
arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 14 +++++++++++++-
arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts | 4 ++++
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
index afa43c7..2d7c284 100644
--- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
+++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
@@ -246,6 +246,18 @@
#pwm-cells = <3>;
status = "disabled";
};
-
+ gpio: gpio@10060000 {
+ compatible = "sifive,fu540-c000-gpio", "sifive,gpio0";
+ interrupt-parent = <&plic0>;
+ interrupts = <7 8 9 10 11 12 13 14 15
+ 16 17 18 19 20 21 22>;
+ reg = <0x0 0x10060000 0x0 0x1000>;
+ gpio-controller;
+ #gpio-cells = <2>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ clocks = <&prci PRCI_CLK_TLCLK>;
+ status = "disabled";
+ };
};
};
diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
index 88cfcb9..609198c 100644
--- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
+++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
@@ -94,3 +94,7 @@
&pwm1 {
status = "okay";
};
+
+&gpio {
+ status = "okay";
+};
--
2.7.4
Add support for hierarchy irq domains. This is needed as pre-requisite for
gpio-sifive driver.
Signed-off-by: Yash Shah <[email protected]>
---
drivers/irqchip/Kconfig | 1 +
drivers/irqchip/irq-sifive-plic.c | 30 ++++++++++++++++++++++++++----
2 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index ccbb897..a398552 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -488,6 +488,7 @@ endmenu
config SIFIVE_PLIC
bool "SiFive Platform-Level Interrupt Controller"
depends on RISCV
+ select IRQ_DOMAIN_HIERARCHY
help
This enables support for the PLIC chip found in SiFive (and
potentially other) RISC-V systems. The PLIC controls devices
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 7d0a12f..750e366 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -154,15 +154,37 @@ static struct irq_chip plic_chip = {
static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
irq_hw_number_t hwirq)
{
- irq_set_chip_and_handler(irq, &plic_chip, handle_fasteoi_irq);
- irq_set_chip_data(irq, NULL);
+ irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
+ handle_fasteoi_irq, NULL, NULL);
irq_set_noprobe(irq);
return 0;
}
+static int plic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *arg)
+{
+ int i, ret;
+ irq_hw_number_t hwirq;
+ unsigned int type = IRQ_TYPE_NONE;
+ struct irq_fwspec *fwspec = arg;
+
+ ret = irq_domain_translate_onecell(domain, fwspec, &hwirq, &type);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < nr_irqs; i++) {
+ ret = plic_irqdomain_map(domain, virq + i, hwirq + i);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
static const struct irq_domain_ops plic_irqdomain_ops = {
- .map = plic_irqdomain_map,
- .xlate = irq_domain_xlate_onecell,
+ .translate = irq_domain_translate_onecell,
+ .alloc = plic_irq_domain_alloc,
+ .free = irq_domain_free_irqs_top,
};
static struct irq_domain *plic_irqdomain;
--
2.7.4
On Nov 20 2019, Yash Shah wrote:
> diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> index 88cfcb9..609198c 100644
> --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> @@ -94,3 +94,7 @@
> &pwm1 {
> status = "okay";
> };
> +
> +&gpio {
> + status = "okay";
> +};
How about adding a gpio-restart?
Andreas.
--
Andreas Schwab, SUSE Labs, [email protected]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
On 2019-11-20 09:34, Thomas Gleixner wrote:
> On Wed, 20 Nov 2019, Yash Shah wrote:
>
>> Add a new function irq_domain_translate_onecell() that is to be used
>> as
>> the translate function in struct irq_domain_ops for the v2 IRQ API.
>
> What is the V2 IRQ API?
I believe that's a reference to a rather misleading comment in
irqdomain.h:
#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
/* extended V2 interfaces to support hierarchy irq_domains */
which we could drop, as everything refers to hierarchical domain
support.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 2019-11-20 06:59, Yash Shah wrote:
> Add a new function irq_domain_translate_onecell() that is to be used
> as
> the translate function in struct irq_domain_ops for the v2 IRQ API.
>
> Signed-off-by: Yash Shah <[email protected]>
> ---
> include/linux/irqdomain.h | 5 +++++
> kernel/irq/irqdomain.c | 20 ++++++++++++++++++++
> 2 files changed, 25 insertions(+)
>
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index 583e7ab..cad9eb8 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -426,6 +426,11 @@ int irq_domain_translate_twocell(struct
> irq_domain *d,
> unsigned long *out_hwirq,
> unsigned int *out_type);
>
> +int irq_domain_translate_onecell(struct irq_domain *d,
> + struct irq_fwspec *fwspec,
> + unsigned long *out_hwirq,
> + unsigned int *out_type);
> +
> /* IPI functions */
> int irq_reserve_ipi(struct irq_domain *domain, const struct cpumask
> *dest);
> int irq_destroy_ipi(unsigned int irq, const struct cpumask *dest);
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 132672b..6972a48 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -987,6 +987,26 @@ const struct irq_domain_ops
> irq_domain_simple_ops = {
> EXPORT_SYMBOL_GPL(irq_domain_simple_ops);
>
> /**
> + * irq_domain_translate_onecell() - Generic translate for direct one
> cell
> + * bindings
> + *
> + * Device Tree IRQ specifier translation function which works with
> one cell
nit: the whole point of the 'new' translate function is that they are
firmware-agnostic. Just drop the DT reference here.
> + * bindings where the cell values map directly to the hwirq number.
> + */
> +int irq_domain_translate_onecell(struct irq_domain *d,
> + struct irq_fwspec *fwspec,
> + unsigned long *out_hwirq,
> + unsigned int *out_type)
> +{
> + if (WARN_ON(fwspec->param_count < 1))
> + return -EINVAL;
> + *out_hwirq = fwspec->param[0];
> + *out_type = IRQ_TYPE_NONE;
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(irq_domain_translate_onecell);
> +
> +/**
> * irq_domain_translate_twocell() - Generic translate for direct two
> cell
> * bindings
> *
Can you please also update (potentially in a separate patch) the
potential
users of this? I mentioned the nvic driver last time...
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On Wed, 20 Nov 2019, Yash Shah wrote:
> Add a new function irq_domain_translate_onecell() that is to be used as
> the translate function in struct irq_domain_ops for the v2 IRQ API.
What is the V2 IRQ API?
Thanks,
tglx
śr., 20 lis 2019 o 07:59 Yash Shah <[email protected]> napisał(a):
>
> Adds the GPIO driver for SiFive RISC-V SoCs.
>
This looks much better just a couple nits.
> Signed-off-by: Wesley W. Terpstra <[email protected]>
> [Atish: Various fixes and code cleanup]
> Signed-off-by: Atish Patra <[email protected]>
> Signed-off-by: Yash Shah <[email protected]>
> ---
> drivers/gpio/Kconfig | 9 ++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-sifive.c | 256 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 266 insertions(+)
> create mode 100644 drivers/gpio/gpio-sifive.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 38e096e..05e8a41 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -453,6 +453,15 @@ config GPIO_SAMA5D2_PIOBU
> The difference from regular GPIOs is that they
> maintain their value during backup/self-refresh.
>
> +config GPIO_SIFIVE
> + bool "SiFive GPIO support"
> + depends on OF_GPIO
> + select GPIO_GENERIC
> + select GPIOLIB_IRQCHIP
> + select REGMAP_MMIO
> + help
> + Say yes here to support the GPIO device on SiFive SoCs.
> +
> config GPIO_SIOX
> tristate "SIOX GPIO support"
> depends on SIOX
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index d2fd19c..bf7984e 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -121,6 +121,7 @@ obj-$(CONFIG_ARCH_SA1100) += gpio-sa1100.o
> obj-$(CONFIG_GPIO_SAMA5D2_PIOBU) += gpio-sama5d2-piobu.o
> obj-$(CONFIG_GPIO_SCH311X) += gpio-sch311x.o
> obj-$(CONFIG_GPIO_SCH) += gpio-sch.o
> +obj-$(CONFIG_GPIO_SIFIVE) += gpio-sifive.o
> obj-$(CONFIG_GPIO_SIOX) += gpio-siox.o
> obj-$(CONFIG_GPIO_SODAVILLE) += gpio-sodaville.o
> obj-$(CONFIG_GPIO_SPEAR_SPICS) += gpio-spear-spics.o
> diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c
> new file mode 100644
> index 0000000..02666ae
> --- /dev/null
> +++ b/drivers/gpio/gpio-sifive.c
> @@ -0,0 +1,256 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 SiFive
> + */
I prefer to have a newline between the copyright notice and the headers.
> +#include <linux/bitops.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/of_irq.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/init.h>
> +#include <linux/of.h>
Is this really needed? I only see functions defined in of_irq.h.
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
Same here - I don't see any functions from this header being called.
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/regmap.h>
> +
> +#define GPIO_INPUT_VAL 0x00
> +#define GPIO_INPUT_EN 0x04
> +#define GPIO_OUTPUT_EN 0x08
> +#define GPIO_OUTPUT_VAL 0x0C
> +#define GPIO_RISE_IE 0x18
> +#define GPIO_RISE_IP 0x1C
> +#define GPIO_FALL_IE 0x20
> +#define GPIO_FALL_IP 0x24
> +#define GPIO_HIGH_IE 0x28
> +#define GPIO_HIGH_IP 0x2C
> +#define GPIO_LOW_IE 0x30
> +#define GPIO_LOW_IP 0x34
> +#define GPIO_OUTPUT_XOR 0x40
> +
> +#define MAX_GPIO 32
> +#define SIFIVE_GPIO_IRQ_OFFSET 7
Please use a single prefix for all symbols in this driver. Let's stick
to sifive_gpio and SIFIVE_GPIO for defines.
> +
> +struct sifive_gpio {
> + void __iomem *base;
> + struct gpio_chip gc;
> + struct regmap *regs;
> + u32 enabled;
The name of this field is a bit confusing - do you mind renaming it to
something like irq_state? Maybe something else, but simply using
'enabled' makes me think this has more to do with the chip being
enabled.
> + unsigned int trigger[MAX_GPIO];
> + unsigned int irq_parent[MAX_GPIO];
> +};
> +
> +static void sifive_set_ie(struct sifive_gpio *chip, unsigned int offset)
> +{
> + unsigned long flags;
> + unsigned int trigger;
> +
> + spin_lock_irqsave(&chip->gc.bgpio_lock, flags);
> + trigger = (chip->enabled & BIT(offset)) ? chip->trigger[offset] : 0;
> + regmap_update_bits(chip->regs, GPIO_RISE_IE, BIT(offset),
> + (trigger & IRQ_TYPE_EDGE_RISING) ? BIT(offset) : 0);
> + regmap_update_bits(chip->regs, GPIO_FALL_IE, BIT(offset),
> + (trigger & IRQ_TYPE_EDGE_FALLING) ? BIT(offset) : 0);
> + regmap_update_bits(chip->regs, GPIO_HIGH_IE, BIT(offset),
> + (trigger & IRQ_TYPE_LEVEL_HIGH) ? BIT(offset) : 0);
> + regmap_update_bits(chip->regs, GPIO_LOW_IE, BIT(offset),
> + (trigger & IRQ_TYPE_LEVEL_LOW) ? BIT(offset) : 0);
> + spin_unlock_irqrestore(&chip->gc.bgpio_lock, flags);
> +}
> +
> +static int sifive_irq_set_type(struct irq_data *d, unsigned int trigger)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct sifive_gpio *chip = gpiochip_get_data(gc);
> + int offset = irqd_to_hwirq(d);
> +
> + if (offset < 0 || offset >= gc->ngpio)
> + return -EINVAL;
> +
> + chip->trigger[offset] = trigger;
> + sifive_set_ie(chip, offset);
> + return 0;
> +}
> +
> +static void sifive_irq_enable(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct sifive_gpio *chip = gpiochip_get_data(gc);
> + int offset = irqd_to_hwirq(d) % MAX_GPIO;
> + u32 bit = BIT(offset);
> + unsigned long flags;
> +
> + irq_chip_enable_parent(d);
> +
> + /* Switch to input */
> + gc->direction_input(gc, offset);
> +
> + spin_lock_irqsave(&gc->bgpio_lock, flags);
> + /* Clear any sticky pending interrupts */
> + regmap_write(chip->regs, GPIO_RISE_IP, bit);
> + regmap_write(chip->regs, GPIO_FALL_IP, bit);
> + regmap_write(chip->regs, GPIO_HIGH_IP, bit);
> + regmap_write(chip->regs, GPIO_LOW_IP, bit);
> + spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> +
> + /* Enable interrupts */
> + assign_bit(offset, (unsigned long *)&chip->enabled, 1);
> + sifive_set_ie(chip, offset);
> +}
> +
> +static void sifive_irq_disable(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct sifive_gpio *chip = gpiochip_get_data(gc);
> + int offset = irqd_to_hwirq(d) % MAX_GPIO;
> +
> + assign_bit(offset, (unsigned long *)&chip->enabled, 0);
> + sifive_set_ie(chip, offset);
> + irq_chip_disable_parent(d);
> +}
> +
> +static void sifive_irq_eoi(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct sifive_gpio *chip = gpiochip_get_data(gc);
> + int offset = irqd_to_hwirq(d) % MAX_GPIO;
> + u32 bit = BIT(offset);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&gc->bgpio_lock, flags);
> + /* Clear all pending interrupts */
> + regmap_write(chip->regs, GPIO_RISE_IP, bit);
> + regmap_write(chip->regs, GPIO_FALL_IP, bit);
> + regmap_write(chip->regs, GPIO_HIGH_IP, bit);
> + regmap_write(chip->regs, GPIO_LOW_IP, bit);
> + spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> +
> + irq_chip_eoi_parent(d);
> +}
> +
> +static struct irq_chip sifive_irqchip = {
> + .name = "sifive-gpio",
> + .irq_set_type = sifive_irq_set_type,
> + .irq_mask = irq_chip_mask_parent,
> + .irq_unmask = irq_chip_unmask_parent,
> + .irq_enable = sifive_irq_enable,
> + .irq_disable = sifive_irq_disable,
> + .irq_eoi = sifive_irq_eoi,
> +};
> +
> +static int sifive_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
> + unsigned int child,
> + unsigned int child_type,
> + unsigned int *parent,
> + unsigned int *parent_type)
> +{
> + *parent_type = IRQ_TYPE_NONE;
> + *parent = child + SIFIVE_GPIO_IRQ_OFFSET;
> + return 0;
> +}
> +
> +static const struct regmap_config sifive_gpio_regmap_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .fast_io = true,
> + .disable_locking = true,
> +};
> +
> +static int sifive_gpio_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *node = pdev->dev.of_node;
> + struct device_node *irq_parent;
> + struct irq_domain *parent;
> + struct gpio_irq_chip *girq;
> + struct sifive_gpio *chip;
> + struct resource *res;
> + unsigned long flags;
> + int ret, ngpio;
> +
> + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> + if (!chip)
> + return -ENOMEM;
> +
> + chip->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(chip->base)) {
> + dev_err(dev, "failed to allocate device memory\n");
> + return PTR_ERR(chip->base);
> + }
> +
> + chip->regs = devm_regmap_init_mmio(dev, chip->base,
> + &sifive_gpio_regmap_config);
> + if (IS_ERR(chip->regs))
> + return PTR_ERR(chip->regs);
> +
> + ngpio = of_irq_count(node);
> + if (ngpio >= MAX_GPIO) {
> + dev_err(dev, "Too many GPIO interrupts (max=%d)\n", MAX_GPIO);
> + return -ENXIO;
> + }
> +
> + irq_parent = of_irq_find_parent(node);
> + if (!irq_parent) {
> + dev_err(dev, "no IRQ parent node\n");
> + return -ENODEV;
> + }
> + parent = irq_find_host(irq_parent);
> + if (!parent) {
> + dev_err(dev, "no IRQ parent domain\n");
> + return -ENODEV;
> + }
> +
> + ret = bgpio_init(&chip->gc, dev, 4,
> + chip->base + GPIO_INPUT_VAL,
> + chip->base + GPIO_OUTPUT_VAL,
> + NULL,
> + chip->base + GPIO_OUTPUT_EN,
> + chip->base + GPIO_INPUT_EN,
> + 0);
> + if (ret) {
> + dev_err(dev, "unable to init generic GPIO\n");
> + return ret;
> + }
> +
> + spin_lock_irqsave(&chip->gc.bgpio_lock, flags);
> + /* Disable all GPIO interrupts before enabling parent interrupts */
> + regmap_write(chip->regs, GPIO_RISE_IE, 0);
> + regmap_write(chip->regs, GPIO_FALL_IE, 0);
> + regmap_write(chip->regs, GPIO_HIGH_IE, 0);
> + regmap_write(chip->regs, GPIO_LOW_IE, 0);
> + spin_unlock_irqrestore(&chip->gc.bgpio_lock, flags);
No need for locking in probe().
> + chip->enabled = 0;
> +
> + chip->gc.base = -1;
> + chip->gc.ngpio = ngpio;
> + chip->gc.label = dev_name(dev);
> + chip->gc.parent = dev;
> + chip->gc.owner = THIS_MODULE;
> + girq = &chip->gc.irq;
> + girq->chip = &sifive_irqchip;
> + girq->fwnode = of_node_to_fwnode(node);
> + girq->parent_domain = parent;
> + girq->child_to_parent_hwirq = sifive_gpio_child_to_parent_hwirq;
> + girq->handler = handle_bad_irq;
> + girq->default_type = IRQ_TYPE_NONE;
> +
> + platform_set_drvdata(pdev, chip);
> + return gpiochip_add_data(&chip->gc, chip);
> +}
> +
> +static const struct of_device_id sifive_gpio_match[] = {
> + { .compatible = "sifive,gpio0" },
> + { .compatible = "sifive,fu540-c000-gpio" },
> + { },
> +};
> +
> +static struct platform_driver sifive_gpio_driver = {
> + .probe = sifive_gpio_probe,
> + .driver = {
> + .name = "sifive_gpio",
> + .of_match_table = of_match_ptr(sifive_gpio_match),
> + },
> +};
> +builtin_platform_driver(sifive_gpio_driver)
> --
> 2.7.4
>
Bartosz
On Wed, 20 Nov 2019, Marc Zyngier wrote:
> On 2019-11-20 09:34, Thomas Gleixner wrote:
> > On Wed, 20 Nov 2019, Yash Shah wrote:
> >
> > > Add a new function irq_domain_translate_onecell() that is to be used as
> > > the translate function in struct irq_domain_ops for the v2 IRQ API.
> >
> > What is the V2 IRQ API?
>
> I believe that's a reference to a rather misleading comment in irqdomain.h:
>
> #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> /* extended V2 interfaces to support hierarchy irq_domains */
>
> which we could drop, as everything refers to hierarchical domain
> support.
Yes, which reminds me that we also need to update the horribly stale
documentation of all this mess.
Thanks,
tglx
> -----Original Message-----
> From: Andreas Schwab <[email protected]>
> Sent: 20 November 2019 14:44
> To: Yash Shah <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Paul
> Walmsley ( Sifive) <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]; Sagar Kadam
> <[email protected]>; [email protected]; [email protected];
> [email protected]; Sachin Ghadi <[email protected]>
> Subject: Re: [PATCH v2 5/5] riscv: dts: Add DT support for SiFive FU540 GPIO
> driver
>
> On Nov 20 2019, Yash Shah wrote:
>
> > diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > index 88cfcb9..609198c 100644
> > --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > @@ -94,3 +94,7 @@
> > &pwm1 {
> > status = "okay";
> > };
> > +
> > +&gpio {
> > + status = "okay";
> > +};
>
> How about adding a gpio-restart?
I am planning to add it in a separate patch.
- Yash
> -----Original Message-----
> From: Bartosz Golaszewski <[email protected]>
> Sent: 20 November 2019 15:31
> To: Yash Shah <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; Paul Walmsley ( Sifive) <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Sagar Kadam
> <[email protected]>; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; Sachin Ghadi <[email protected]>
> Subject: Re: [PATCH v2 4/5] gpio: sifive: Add GPIO driver for SiFive SoCs
>
> śr., 20 lis 2019 o 07:59 Yash Shah <[email protected]> napisał(a):
> >
> > Adds the GPIO driver for SiFive RISC-V SoCs.
> >
>
> This looks much better just a couple nits.
>
> > Signed-off-by: Wesley W. Terpstra <[email protected]>
> > [Atish: Various fixes and code cleanup]
> > Signed-off-by: Atish Patra <[email protected]>
> > Signed-off-by: Yash Shah <[email protected]>
[...]
> > diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c
> > new file mode 100644 index 0000000..02666ae
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-sifive.c
> > @@ -0,0 +1,256 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2019 SiFive
> > + */
>
> I prefer to have a newline between the copyright notice and the headers.
Done
>
> > +#include <linux/bitops.h>
> > +#include <linux/device.h>
> > +#include <linux/errno.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/init.h>
> > +#include <linux/of.h>
>
> Is this really needed? I only see functions defined in of_irq.h.
>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm.h>
>
> Same here - I don't see any functions from this header being called.
>
Yes, you are right. Both the above inclusion of header file is unnecessary. Will remove them.
> > +#include <linux/slab.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/regmap.h>
> > +
> > +#define GPIO_INPUT_VAL 0x00
> > +#define GPIO_INPUT_EN 0x04
> > +#define GPIO_OUTPUT_EN 0x08
> > +#define GPIO_OUTPUT_VAL 0x0C
> > +#define GPIO_RISE_IE 0x18
> > +#define GPIO_RISE_IP 0x1C
> > +#define GPIO_FALL_IE 0x20
> > +#define GPIO_FALL_IP 0x24
> > +#define GPIO_HIGH_IE 0x28
> > +#define GPIO_HIGH_IP 0x2C
> > +#define GPIO_LOW_IE 0x30
> > +#define GPIO_LOW_IP 0x34
> > +#define GPIO_OUTPUT_XOR 0x40
> > +
> > +#define MAX_GPIO 32
> > +#define SIFIVE_GPIO_IRQ_OFFSET 7
>
> Please use a single prefix for all symbols in this driver. Let's stick to sifive_gpio
> and SIFIVE_GPIO for defines.
Sure.
>
> > +
> > +struct sifive_gpio {
> > + void __iomem *base;
> > + struct gpio_chip gc;
> > + struct regmap *regs;
> > + u32 enabled;
>
> The name of this field is a bit confusing - do you mind renaming it to
> something like irq_state? Maybe something else, but simply using 'enabled'
> makes me think this has more to do with the chip being enabled.
>
Sure, will rename it to irq_state.
[...]
> > + spin_lock_irqsave(&chip->gc.bgpio_lock, flags);
> > + /* Disable all GPIO interrupts before enabling parent interrupts */
> > + regmap_write(chip->regs, GPIO_RISE_IE, 0);
> > + regmap_write(chip->regs, GPIO_FALL_IE, 0);
> > + regmap_write(chip->regs, GPIO_HIGH_IE, 0);
> > + regmap_write(chip->regs, GPIO_LOW_IE, 0);
> > + spin_unlock_irqrestore(&chip->gc.bgpio_lock, flags);
>
> No need for locking in probe().
Ok.
Thanks for your comments!
- Yash
> -----Original Message-----
> From: Marc Zyngier <[email protected]>
> Sent: 20 November 2019 16:09
> To: Yash Shah <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Paul
> Walmsley ( Sifive) <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Sagar Kadam <[email protected]>; linux-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; Sachin Ghadi
> <[email protected]>
> Subject: Re: [PATCH v2 1/5] genirq: introduce irq_domain_translate_onecell
>
> On 2019-11-20 06:59, Yash Shah wrote:
> > Add a new function irq_domain_translate_onecell() that is to be used
> > as the translate function in struct irq_domain_ops for the v2 IRQ API.
> >
> > Signed-off-by: Yash Shah <[email protected]>
[...]
> >
> > /**
> > + * irq_domain_translate_onecell() - Generic translate for direct one
> > cell
> > + * bindings
> > + *
> > + * Device Tree IRQ specifier translation function which works with
> > one cell
>
> nit: the whole point of the 'new' translate function is that they are
> firmware-agnostic. Just drop the DT reference here.
Ok sure.
>
> > + * bindings where the cell values map directly to the hwirq number.
> > + */
> > +int irq_domain_translate_onecell(struct irq_domain *d,
> > + struct irq_fwspec *fwspec,
> > + unsigned long *out_hwirq,
> > + unsigned int *out_type)
> > +{
> > + if (WARN_ON(fwspec->param_count < 1))
> > + return -EINVAL;
> > + *out_hwirq = fwspec->param[0];
> > + *out_type = IRQ_TYPE_NONE;
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(irq_domain_translate_onecell);
> > +
> > +/**
> > * irq_domain_translate_twocell() - Generic translate for direct two
> > cell
> > * bindings
> > *
>
> Can you please also update (potentially in a separate patch) the
> potential
> users of this? I mentioned the nvic driver last time...
>
Ok, I will separate out this patch from the patchset and send it individually along with potential users of it.
Thanks for your comments
- Yash
> -----Original Message-----
> From: linux-riscv <[email protected]> On Behalf Of
> Yash Shah
> Sent: 21 November 2019 14:06
> To: Marc Zyngier <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Sachin Ghadi <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; Sagar Kadam <[email protected]>; linux-
> [email protected]; Paul Walmsley ( Sifive) <[email protected]>;
> [email protected]; [email protected]; [email protected]
> Subject: RE: [PATCH v2 1/5] genirq: introduce irq_domain_translate_onecell
>
> > -----Original Message-----
> > From: Marc Zyngier <[email protected]>
> > Sent: 20 November 2019 16:09
> > To: Yash Shah <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected]; Paul
> > Walmsley ( Sifive) <[email protected]>; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; Sagar Kadam <[email protected]>; linux-
> > [email protected]; [email protected]; linux-
> > [email protected]; [email protected]; Sachin Ghadi
> > <[email protected]>
> > Subject: Re: [PATCH v2 1/5] genirq: introduce
> > irq_domain_translate_onecell
> >
> > On 2019-11-20 06:59, Yash Shah wrote:
> > > Add a new function irq_domain_translate_onecell() that is to be used
> > > as the translate function in struct irq_domain_ops for the v2 IRQ API.
> > >
> > > Signed-off-by: Yash Shah <[email protected]>
> [...]
> > >
> > > /**
> > > + * irq_domain_translate_onecell() - Generic translate for direct
> > > + one
> > > cell
> > > + * bindings
> > > + *
> > > + * Device Tree IRQ specifier translation function which works with
> > > one cell
> >
> > nit: the whole point of the 'new' translate function is that they are
> > firmware-agnostic. Just drop the DT reference here.
>
> Ok sure.
>
> >
> > > + * bindings where the cell values map directly to the hwirq number.
> > > + */
> > > +int irq_domain_translate_onecell(struct irq_domain *d,
> > > + struct irq_fwspec *fwspec,
> > > + unsigned long *out_hwirq,
> > > + unsigned int *out_type)
> > > +{
> > > + if (WARN_ON(fwspec->param_count < 1))
> > > + return -EINVAL;
> > > + *out_hwirq = fwspec->param[0];
> > > + *out_type = IRQ_TYPE_NONE;
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(irq_domain_translate_onecell);
> > > +
> > > +/**
> > > * irq_domain_translate_twocell() - Generic translate for direct
> > > two cell
> > > * bindings
> > > *
> >
> > Can you please also update (potentially in a separate patch) the
> > potential users of this? I mentioned the nvic driver last time...
> >
>
> Ok, I will separate out this patch from the patchset and send it individually
> along with potential users of it.
> Thanks for your comments
I am sorry, I think I misunderstood you.
You want me to send a new separate patch in which the potential users will be updated to this new function.
Hope I got it right?
- Yash
On 2019-11-21 08:55, Yash Shah wrote:
[...]
>> > > + * bindings where the cell values map directly to the hwirq
>> number.
>> > > + */
>> > > +int irq_domain_translate_onecell(struct irq_domain *d,
>> > > + struct irq_fwspec *fwspec,
>> > > + unsigned long *out_hwirq,
>> > > + unsigned int *out_type)
>> > > +{
>> > > + if (WARN_ON(fwspec->param_count < 1))
>> > > + return -EINVAL;
>> > > + *out_hwirq = fwspec->param[0];
>> > > + *out_type = IRQ_TYPE_NONE;
>> > > + return 0;
>> > > +}
>> > > +EXPORT_SYMBOL_GPL(irq_domain_translate_onecell);
>> > > +
>> > > +/**
>> > > * irq_domain_translate_twocell() - Generic translate for
>> direct
>> > > two cell
>> > > * bindings
>> > > *
>> >
>> > Can you please also update (potentially in a separate patch) the
>> > potential users of this? I mentioned the nvic driver last time...
>> >
>>
>> Ok, I will separate out this patch from the patchset and send it
>> individually
>> along with potential users of it.
>> Thanks for your comments
>
> I am sorry, I think I misunderstood you.
> You want me to send a new separate patch in which the potential users
> will be updated to this new function.
> Hope I got it right?
Just add, as part of this series, a patch that updates the one or two
drivers that could make use of this. It doesn't need to be in a
separate
patch set (which would cause dependency issues).
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 2019-11-20 06:59, Yash Shah wrote:
> Add support for hierarchy irq domains. This is needed as
> pre-requisite for
> gpio-sifive driver.
>
> Signed-off-by: Yash Shah <[email protected]>
> ---
> drivers/irqchip/Kconfig | 1 +
> drivers/irqchip/irq-sifive-plic.c | 30
> ++++++++++++++++++++++++++----
> 2 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index ccbb897..a398552 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -488,6 +488,7 @@ endmenu
> config SIFIVE_PLIC
> bool "SiFive Platform-Level Interrupt Controller"
> depends on RISCV
> + select IRQ_DOMAIN_HIERARCHY
> help
> This enables support for the PLIC chip found in SiFive (and
> potentially other) RISC-V systems. The PLIC controls devices
> diff --git a/drivers/irqchip/irq-sifive-plic.c
> b/drivers/irqchip/irq-sifive-plic.c
> index 7d0a12f..750e366 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -154,15 +154,37 @@ static struct irq_chip plic_chip = {
> static int plic_irqdomain_map(struct irq_domain *d, unsigned int
> irq,
> irq_hw_number_t hwirq)
> {
> - irq_set_chip_and_handler(irq, &plic_chip, handle_fasteoi_irq);
> - irq_set_chip_data(irq, NULL);
> + irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
> + handle_fasteoi_irq, NULL, NULL);
> irq_set_noprobe(irq);
> return 0;
> }
>
> +static int plic_irq_domain_alloc(struct irq_domain *domain, unsigned
> int virq,
> + unsigned int nr_irqs, void *arg)
> +{
> + int i, ret;
> + irq_hw_number_t hwirq;
> + unsigned int type = IRQ_TYPE_NONE;
You shouldn't need this init here. The whole point of
irq_domain_translate_onecell
is that it either gives you a valid value, or an error.
> + struct irq_fwspec *fwspec = arg;
> +
> + ret = irq_domain_translate_onecell(domain, fwspec, &hwirq, &type);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < nr_irqs; i++) {
> + ret = plic_irqdomain_map(domain, virq + i, hwirq + i);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static const struct irq_domain_ops plic_irqdomain_ops = {
> - .map = plic_irqdomain_map,
> - .xlate = irq_domain_xlate_onecell,
> + .translate = irq_domain_translate_onecell,
> + .alloc = plic_irq_domain_alloc,
> + .free = irq_domain_free_irqs_top,
> };
>
> static struct irq_domain *plic_irqdomain;
Otherwise looks OK.
M.
--
Jazz is not dead. It just smells funny...