| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMpxmJX0A+_6LmrWPe=Ne5KGhG9r_HySpstDhV5ZTnpz2QXF7Q@mail.gmail.com>
Date: Wed, 20 Nov 2019 11:01:11 +0100
From: Bartosz Golaszewski <bgolaszewski@...libre.com>
To: Yash Shah <yash.shah@...ive.com>
Cc: "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"mark.rutland@....com" <mark.rutland@....com>,
"palmer@...belt.com" <palmer@...belt.com>,
"Paul Walmsley ( Sifive)" <paul.walmsley@...ive.com>,
"aou@...s.berkeley.edu" <aou@...s.berkeley.edu>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"jason@...edaemon.net" <jason@...edaemon.net>,
"maz@...nel.org" <maz@...nel.org>,
"bmeng.cn@...il.com" <bmeng.cn@...il.com>,
"atish.patra@....com" <atish.patra@....com>,
Sagar Kadam <sagar.kadam@...ive.com>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Sachin Ghadi <sachin.ghadi@...ive.com>
Subject: Re: [PATCH v2 4/5] gpio: sifive: Add GPIO driver for SiFive SoCs
śr., 20 lis 2019 o 07:59 Yash Shah <yash.shah@...ive.com> 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 <wesley@...ive.com>
> [Atish: Various fixes and code cleanup]
> Signed-off-by: Atish Patra <atish.patra@....com>
> Signed-off-by: Yash Shah <yash.shah@...ive.com>
> ---
> 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
Powered by blists - more mailing lists