| 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: <CAMpxmJWcuV7goPWxOWv_Og9GwzGrioF62SfS1LCiHf9eDX=vdw@mail.gmail.com>
Date: Wed, 13 Nov 2019 14:10:30 +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 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs
wt., 12 lis 2019 o 13:12 Yash Shah <yash.shah@...ive.com> napisał(a):
>
> Adds the GPIO driver for SiFive RISC-V SoCs.
>
> 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 | 255 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 265 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..abdf839
> --- /dev/null
> +++ b/drivers/gpio/gpio-sifive.c
> @@ -0,0 +1,255 @@
> +// 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
> +
> +struct sifive_gpio {
> + raw_spinlock_t lock;
> + 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;
> +
> + raw_spin_lock_irqsave(&chip->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);
> + raw_spin_unlock_irqrestore(&chip->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);
> +
> + irq_chip_enable_parent(d);
> +
> + /* Switch to input */
> + gc->direction_input(gc, offset);
> +
> + /* Clear any sticky pending interrupts */
> + iowrite32(bit, chip->base + GPIO_RISE_IP);
> + iowrite32(bit, chip->base + GPIO_FALL_IP);
> + iowrite32(bit, chip->base + GPIO_HIGH_IP);
> + iowrite32(bit, chip->base + GPIO_LOW_IP);
> +
> + /* 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);
> +
> + /* Clear all pending interrupts */
> + iowrite32(bit, chip->base + GPIO_RISE_IP);
> + iowrite32(bit, chip->base + GPIO_FALL_IP);
> + iowrite32(bit, chip->base + GPIO_HIGH_IP);
> + iowrite32(bit, chip->base + GPIO_LOW_IP);
> +
> + 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)
> +{
> + /* All these interrupts are level high in the CPU */
> + *parent_type = IRQ_TYPE_LEVEL_HIGH;
> + *parent = child + 7;
> + return 0;
> +}
> +
> +static const struct regmap_config sifive_gpio_regmap_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .fast_io = 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;
> + int ret, ngpio;
> +
> + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> + if (!chip)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + chip->base = devm_ioremap_resource(dev, res);
Use devm_platform_ioremap_resource() and drop the res variable.
> + 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);
Why do you need this regmap here? You initialize a new regmap, then
use your own locking despite not having disabled the internal locking
in regmap, and then you initialize the mmio generic GPIO code which
will use yet another lock to operate on the same registers and in the
end you write to those registers without taking any lock anyway.
Doesn't make much sense to me.
> + 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;
> + }
> +
> + /* Disable all GPIO interrupts before enabling parent interrupts */
> + iowrite32(0, chip->base + GPIO_RISE_IE);
> + iowrite32(0, chip->base + GPIO_FALL_IE);
> + iowrite32(0, chip->base + GPIO_HIGH_IE);
> + iowrite32(0, chip->base + GPIO_LOW_IE);
> + chip->enabled = 0;
> +
> + raw_spin_lock_init(&chip->lock);
> + 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;
> +
> + ret = gpiochip_add_data(&chip->gc, chip);
> + if (ret)
> + return ret;
> +
> + platform_set_drvdata(pdev, chip);
> + dev_info(dev, "SiFive GPIO chip registered %d GPIOs\n", ngpio);
Core gpio library emits a very similar debug message from
gpiochip_setup_dev(), I think you can drop it and directly return
gpiochip_add_data().
Bartosz
> +
> + return 0;
> +}
> +
> +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
>
Powered by blists - more mailing lists