| 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: <CAHp75Vc6aaDhVN7LzvLNQjuOPguz+nbfmfpZ7TZHK=fNjCRz8w@mail.gmail.com>
Date: Wed, 17 Mar 2021 15:08:00 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Sander Vanheule <sander@...nheule.net>
Cc: "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
devicetree <devicetree@...r.kernel.org>,
Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <bgolaszewski@...libre.com>,
Rob Herring <robh+dt@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Marc Zyngier <maz@...nel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Bert Vermeulen <bert@...t.com>
Subject: Re: [PATCH v2 2/2] gpio: Add Realtek Otto GPIO support
On Mon, Mar 15, 2021 at 11:11 PM Sander Vanheule <sander@...nheule.net> wrote:
>
> Realtek MIPS SoCs (platform name Otto) have GPIO controllers with up to
> 64 GPIOs, divided over two banks. Each bank has a set of registers for
> 32 GPIOs, with support for edge-triggered interrupts.
>
> Each GPIO bank consists of four 8-bit GPIO ports (ABCD and EFGH). Most
> registers pack one bit per GPIO, except for the IMR register, which
> packs two bits per GPIO (AB-CD).
>
> Although the byte order is currently assumed to have port A..D at offset
> 0x0..0x3, this has been observed to be reversed on other, Lexra-based,
> SoCs (e.g. RTL8196E/97D/97F).
>
> Interrupt support is disabled for the fallback devicetree-compatible
> 'realtek,otto-gpio'. This allows for quick support of GPIO banks in
> which the byte order would be unknown. In this case, the port ordering
> in the IMR registers may not match the reversed order in the other
> registers (DCBA, and BA-DC or DC-BA).
I have few comments.
> Signed-off-by: Sander Vanheule <sander@...nheule.net>
> Reviewed-by: Linus Walleij <linus.walleij@...aro.org>
> ---
> drivers/gpio/Kconfig | 12 ++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-realtek-otto.c | 331 +++++++++++++++++++++++++++++++
> 3 files changed, 344 insertions(+)
> create mode 100644 drivers/gpio/gpio-realtek-otto.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index e3607ec4c2e8..fedf1e49469e 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -502,6 +502,18 @@ config GPIO_RDA
> help
> Say Y here to support RDA Micro GPIO controller.
>
> +config GPIO_REALTEK_OTTO
> + bool "Realtek Otto GPIO support"
> + depends on MACH_REALTEK_RTL
> + depends on OF_GPIO
Don't see how it's used.
> + default MACH_REALTEK_RTL
> + select GPIO_GENERIC
> + select GPIOLIB_IRQCHIP
> + help
> + The GPIO controller on the Otto MIPS platform supports up to two
> + banks of 32 GPIOs, with edge triggered interrupts. The 32 GPIOs
> + are grouped in four 8-bit wide ports.
> +
> config GPIO_REG
> bool
> help
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index c58a90a3c3b1..8ace5934e3c3 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -124,6 +124,7 @@ obj-$(CONFIG_GPIO_RC5T583) += gpio-rc5t583.o
> obj-$(CONFIG_GPIO_RCAR) += gpio-rcar.o
> obj-$(CONFIG_GPIO_RDA) += gpio-rda.o
> obj-$(CONFIG_GPIO_RDC321X) += gpio-rdc321x.o
> +obj-$(CONFIG_GPIO_REALTEK_OTTO) += gpio-realtek-otto.o
> obj-$(CONFIG_GPIO_REG) += gpio-reg.o
> obj-$(CONFIG_ARCH_SA1100) += gpio-sa1100.o
> obj-$(CONFIG_GPIO_SAMA5D2_PIOBU) += gpio-sama5d2-piobu.o
> diff --git a/drivers/gpio/gpio-realtek-otto.c b/drivers/gpio/gpio-realtek-otto.c
> new file mode 100644
> index 000000000000..818412687346
> --- /dev/null
> +++ b/drivers/gpio/gpio-realtek-otto.c
> @@ -0,0 +1,331 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/gpio/driver.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>
> +#include <linux/of_irq.h>
Why?
Perhaps what you need is property.h and mod_devicetable.h. See below.
> +#include <linux/platform_device.h>
> +#include <linux/swab.h>
Not sure why you need this? See below.
> +/*
> + * Total register block size is 0x1C for four ports.
> + * On the RTL8380/RLT8390 platforms port A, B, and C are implemented.
> + * RTL8389 and RTL8328 implement a second bank with ports E, F, G, and H.
> + *
> + * Port information is stored with the first port at offset 0, followed by the
> + * second, etc. Most registers store one bit per GPIO and should be read out in
> + * reversed endian order. The two interrupt mask registers store two bits per
> + * GPIO, and should be manipulated with swahw32, if required.
> + */
> +
> +/*
> + * Pin select: (0) "normal", (1) "dedicate peripheral"
> + * Not used on RTL8380/RTL8390, peripheral selection is managed by control bits
> + * in the peripheral registers.
> + */
> +#define REALTEK_GPIO_REG_CNR 0x00
> +/* Clear bit (0) for input, set bit (1) for output */
> +#define REALTEK_GPIO_REG_DIR 0x08
> +#define REALTEK_GPIO_REG_DATA 0x0C
> +/* Read bit for IRQ status, write 1 to clear IRQ */
> +#define REALTEK_GPIO_REG_ISR 0x10
> +/* Two bits per GPIO */
> +#define REALTEK_GPIO_REG_IMR_AB 0x14
> +#define REALTEK_GPIO_REG_IMR_CD 0x18
> +#define REALTEK_GPIO_IMR_LINE_MASK 3
GENMASK() ?
> +#define REALTEK_GPIO_IRQ_EDGE_FALLING 1
> +#define REALTEK_GPIO_IRQ_EDGE_RISING 2
> +#define REALTEK_GPIO_IRQ_EDGE_BOTH 3
> +
> +#define REALTEK_GPIO_MAX 32
> +
> +/*
> + * Realtek GPIO driver data
> + * Because the interrupt mask register (IMR) combines the function of
> + * IRQ type selection and masking, two extra values are stored.
> + * intr_mask is used to mask/unmask the interrupts for certain GPIO,
> + * and intr_type is used to store the selected interrupt types. The
> + * logical AND of these values is written to IMR on changes.
> + *
> + * @dev Parent device
> + * @gc Associated gpio_chip instance
> + * @base Base address of the register block
> + * @lock Lock for accessing the IRQ registers and values
> + * @intr_mask Mask for GPIO interrupts
> + * @intr_type GPIO interrupt type selection
> + */
> +struct realtek_gpio_ctrl {
> + struct device *dev;
> + struct gpio_chip gc;
> + void __iomem *base;
> + raw_spinlock_t lock;
> + u32 intr_mask[2];
> + u32 intr_type[2];
> +};
> +
> +enum realtek_gpio_flags {
> + GPIO_INTERRUPTS = BIT(0),
> +};
> +
> +static struct realtek_gpio_ctrl *irq_data_to_ctrl(struct irq_data *data)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> +
> + return container_of(gc, struct realtek_gpio_ctrl, gc);
> +}
> +
> +static inline u32 realtek_gpio_isr_read(struct realtek_gpio_ctrl *ctrl)
> +{
> + return swab32(readl(ctrl->base + REALTEK_GPIO_REG_ISR));
Why swab?! How is this supposed to work on BE CPUs?
Ditto for all swabXX() usage.
> +}
> +
> +static inline void realtek_gpio_isr_clear(struct realtek_gpio_ctrl *ctrl,
> + unsigned int pin_mask)
> +{
> + writel(swab32(pin_mask), ctrl->base + REALTEK_GPIO_REG_ISR);
> +}
> +
> +static inline void realtek_gpio_update_imr(struct realtek_gpio_ctrl *ctrl,
> + unsigned int imr_offset, u32 type, u32 mask)
> +{
> + unsigned int reg;
> +
> + if (imr_offset == 0)
> + reg = REALTEK_GPIO_REG_IMR_AB;
> + else
> + reg = REALTEK_GPIO_REG_IMR_CD;
> + writel(swahw32(type & mask), ctrl->base + reg);
> +}
> +
> +/*
> + * Since the IMRs contain two bits per GPIO, only 16 GPIO lines fit in a 32-bit
> + * register.
> + * Use realtek_gpio_pin_to_imr_offset() to select the correct IMR offset, and
> + * realtek_gpio_imr_bits() to put the GPIO line's new value in the right place.
> + */
> +static inline u32 realtek_gpio_pin_to_imr_offset(unsigned int pin)
> +{
> + return pin/16;
Too few spaces.
> +}
> +
> +static inline u32 realtek_gpio_imr_bits(unsigned int pin, u32 value)
> +{
> + return ((value & REALTEK_GPIO_IMR_LINE_MASK) << 2*(pin % 16));
Too many parentheses.
Too few spaces.
> +}
> +
> +static void realtek_gpio_irq_ack(struct irq_data *data)
> +{
> + struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
> + u32 pin = irqd_to_hwirq(data);
irq_hwnum_t (or how is it called? check the return type of
irqd_to_hwirq() function).
> + realtek_gpio_isr_clear(ctrl, BIT(pin));
> +}
> +
> +static void realtek_gpio_irq_unmask(struct irq_data *data)
> +{
> + struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
> + unsigned int pin = irqd_to_hwirq(data);
> + unsigned int imr_offset = realtek_gpio_pin_to_imr_offset(pin);
> + unsigned long flags;
> + u32 m;
> +
> + raw_spin_lock_irqsave(&ctrl->lock, flags);
> + m = ctrl->intr_mask[imr_offset];
> + m |= realtek_gpio_imr_bits(pin, REALTEK_GPIO_IMR_LINE_MASK);
> + ctrl->intr_mask[imr_offset] = m;
> + realtek_gpio_update_imr(ctrl, imr_offset, ctrl->intr_type[imr_offset], m);
> + raw_spin_unlock_irqrestore(&ctrl->lock, flags);
> +}
> +
> +static void realtek_gpio_irq_mask(struct irq_data *data)
> +{
> + struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
> + unsigned int pin = irqd_to_hwirq(data);
> + unsigned int imr_offset = realtek_gpio_pin_to_imr_offset(pin);
> + unsigned long flags;
> + u32 m;
> +
> + raw_spin_lock_irqsave(&ctrl->lock, flags);
> + m = ctrl->intr_mask[imr_offset];
> + m &= ~realtek_gpio_imr_bits(pin, REALTEK_GPIO_IMR_LINE_MASK);
> + ctrl->intr_mask[imr_offset] = m;
> + realtek_gpio_update_imr(ctrl, imr_offset, ctrl->intr_type[imr_offset], m);
> + raw_spin_unlock_irqrestore(&ctrl->lock, flags);
> +}
> +
> +static int realtek_gpio_irq_set_type(struct irq_data *data,
> + unsigned int flow_type)
> +{
> + struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data);
> + irq_flow_handler_t handler;
> + unsigned int pin = irqd_to_hwirq(data);
> + unsigned int imr_offset = realtek_gpio_pin_to_imr_offset(pin);
> + unsigned long flags;
> + u32 type, t;
> +
> + switch (flow_type & IRQ_TYPE_SENSE_MASK) {
> + case IRQ_TYPE_NONE:
> + type = 0;
> + handler = handle_bad_irq;
> + break;
Why is it here? Make it default like many other GPIO drivers do.
> + case IRQ_TYPE_EDGE_FALLING:
> + type = REALTEK_GPIO_IRQ_EDGE_FALLING;
> + handler = handle_edge_irq;
> + break;
> + case IRQ_TYPE_EDGE_RISING:
> + type = REALTEK_GPIO_IRQ_EDGE_RISING;
> + handler = handle_edge_irq;
> + break;
> + case IRQ_TYPE_EDGE_BOTH:
> + type = REALTEK_GPIO_IRQ_EDGE_BOTH;
> + handler = handle_edge_irq;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + irq_set_handler_locked(data, handler);
handler is always the same. Use it directly here.
> + raw_spin_lock_irqsave(&ctrl->lock, flags);
> + t = ctrl->intr_type[imr_offset];
> + t &= ~realtek_gpio_imr_bits(pin, REALTEK_GPIO_IMR_LINE_MASK);
> + t |= realtek_gpio_imr_bits(pin, type);
> + ctrl->intr_type[imr_offset] = t;
> + realtek_gpio_update_imr(ctrl, imr_offset, t, ctrl->intr_mask[imr_offset]);
> + raw_spin_unlock_irqrestore(&ctrl->lock, flags);
> +
> + return 0;
> +}
> +
> +static void realtek_gpio_irq_handler(struct irq_desc *desc)
> +{
> + struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> + struct realtek_gpio_ctrl *ctrl = gpiochip_get_data(gc);
> + struct irq_chip *irq_chip = irq_desc_get_chip(desc);
> + int offset;
> + unsigned long status;
> +
> + chained_irq_enter(irq_chip, desc);
> +
> + status = realtek_gpio_isr_read(ctrl);
> + for_each_set_bit(offset, &status, gc->ngpio) {
> + dev_dbg(ctrl->dev, "gpio irq %d\n", offset);
Why? Drop this development-phase line.
> + generic_handle_irq(irq_find_mapping(gc->irq.domain, offset));
> + realtek_gpio_isr_clear(ctrl, BIT(offset));
> + }
> +
> + chained_irq_exit(irq_chip, desc);
> +}
> +
> +static struct irq_chip realtek_gpio_irq_chip = {
> + .name = "realtek-otto-gpio",
> + .irq_ack = realtek_gpio_irq_ack,
> + .irq_mask = realtek_gpio_irq_mask,
> + .irq_unmask = realtek_gpio_irq_unmask,
> + .irq_set_type = realtek_gpio_irq_set_type
+ comma
> +};
> +
> +static const struct of_device_id realtek_gpio_of_match[] = {
> + { .compatible = "realtek,otto-gpio" },
> + {
> + .compatible = "realtek,rtl8380-gpio",
> + .data = (void *)GPIO_INTERRUPTS
> + },
> + {
> + .compatible = "realtek,rtl8390-gpio",
> + .data = (void *)GPIO_INTERRUPTS
> + },
> + {},
Drop comma. They are not needed for terminator lines.
> +};
> +
> +MODULE_DEVICE_TABLE(of, realtek_gpio_of_match);
> +
> +static int realtek_gpio_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct gpio_irq_chip *girq;
> + struct realtek_gpio_ctrl *ctrl;
> + const struct of_device_id *match;
> + unsigned int match_flags;
> + u32 ngpios;
> + int err, irq;
> +
> + ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> + if (!ctrl)
> + return -ENOMEM;
> +
> + ctrl->dev = dev;
> +
> + if (!np) {
> + dev_err(&pdev->dev, "no DT node found\n");
> + return -EINVAL;
> + }
> +
> + match = of_match_node(realtek_gpio_of_match, np);
> + match_flags = (unsigned int) match->data;
device_get_match_data();
> + if (of_property_read_u32(np, "ngpios", &ngpios) != 0)
' != 0' can be replaced by ''
device_property_read_u32()
> + ngpios = REALTEK_GPIO_MAX;
> +
> + if (ngpios > REALTEK_GPIO_MAX) {
> + dev_err(&pdev->dev, "invalid ngpios (max. %d)\n",
> + REALTEK_GPIO_MAX);
> + return -EINVAL;
> + }
> +
> + ctrl->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(ctrl->base))
> + return PTR_ERR(ctrl->base);
> +
> + raw_spin_lock_init(&ctrl->lock);
> +
> + err = bgpio_init(&ctrl->gc, dev, 4,
> + ctrl->base + REALTEK_GPIO_REG_DATA, NULL, NULL,
> + ctrl->base + REALTEK_GPIO_REG_DIR, NULL,
> + BGPIOF_BIG_ENDIAN_BYTE_ORDER);
> + if (err) {
> + dev_err(dev, "unable to init generic GPIO");
> + return err;
> + }
> +
> + ctrl->gc.ngpio = ngpios;
> + ctrl->gc.owner = THIS_MODULE;
> +
> + irq = of_irq_get(np, 0);
Why not platform_get_irq_optional() ?
> + if ((match_flags & GPIO_INTERRUPTS) && irq > 0) {
> + girq = &ctrl->gc.irq;
> + girq->chip = &realtek_gpio_irq_chip;
> + girq->parent_handler = realtek_gpio_irq_handler;
> + girq->num_parents = 1;
> + girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
1 -> girq->num_parent
> + GFP_KERNEL);
> + if (!girq->parents)
> + return -ENOMEM;
> + girq->default_type = IRQ_TYPE_NONE;
> + girq->handler = handle_bad_irq;
> + girq->parents[0] = irq;
> +
> + /* Disable and clear all interrupts */
> + realtek_gpio_update_imr(ctrl, 0, 0, 0);
> + realtek_gpio_update_imr(ctrl, 1, 0, 0);
> + realtek_gpio_isr_clear(ctrl, BIT(ngpios)-1);
> + }
> + err = gpiochip_add_data(&ctrl->gc, ctrl);
> + return err;
Fold it.
> +}
> +
> +static struct platform_driver realtek_gpio_driver = {
> + .driver = {
> + .name = "realtek-otto-gpio",
> + .of_match_table = realtek_gpio_of_match,
> + },
> + .probe = realtek_gpio_probe,
> +};
> +
> +builtin_platform_driver(realtek_gpio_driver);
> +
> +MODULE_DESCRIPTION("Realtek Otto GPIO support");
> +MODULE_AUTHOR("Sander Vanheule <sander@...nheule.net>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.30.2
>
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists