| Message ID | 20210315190806.66762-3-sander@svanheule.net |
|---|---|
| State | New |
| Headers | show |
| Series | Add Realtek Otto GPIO support | expand |
On Mon, Mar 15, 2021 at 11:11 PM Sander Vanheule <sander@svanheule.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@svanheule.net> > Reviewed-by: Linus Walleij <linus.walleij@linaro.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@svanheule.net>"); > +MODULE_LICENSE("GPL v2"); > -- > 2.30.2 >
Hi Andy, Thanks for the review. I'll address the style comments in a v3. Some further comments and discussion below. On Wed, 2021-03-17 at 15:08 +0200, Andy Shevchenko wrote: > On Mon, Mar 15, 2021 at 11:11 PM Sander Vanheule < > sander@svanheule.net> wrote: > > + depends on OF_GPIO > > Don't see how it's used. It isn't, so I'll remove it. > > +#include <linux/of_irq.h> > > Why? > Perhaps what you need is property.h and mod_devicetable.h. See below. With you suggestions, I was able to drop most explicit OF references. Only of_device_id remains, for which I'll include mod_devicetable.h. > > +#include <linux/swab.h> > > Not sure why you need this? See below. [snip] > > > + > > +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. My use of swab32/swahw32 has little to do with the CPU being BE or LE, but more with the register packing in the GPIO peripheral. The supported SoCs have port layout A-B-C-D in the registers, where firmware built with Realtek's SDK always denotes A0 as the first GPIO line. So bit 24 in a register has the value for A0 (with the exception of the IMR register). I wrote these wrapper functions to be able to use the BIT() macro with the GPIO line number, similar to how gpio-mmio uses ioread32be() when the BGPIOF_BIG_ENDIAN_BYTE_ORDER flag is used. For the IMR register, port A again comes first, but is now 16 bits wide instead of 8, with A0 at bits 16:17. That's why swahw32 is used for this register. On the currently unsupported RTL9300-series, the port layout is reversed: D-C-B-A. GPIO line A0 is then at bit 0, so the swapping functions won't be required. When support for this alternate port layout is added, some code will need to be added to differentiate between the two cases. > > +} > > + > > +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); > > +} [snip] > > + 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. I'll drop the IRQ_TYPE_NONE case. Do I understand it correctly, that IRQ_TYPE_NONE should never be used as the new value, but only as the default initial value? Best, Sander
On Fri, Mar 19, 2021 at 5:51 PM Sander Vanheule <sander@svanheule.net> wrote: > On Wed, 2021-03-17 at 15:08 +0200, Andy Shevchenko wrote: > > On Mon, Mar 15, 2021 at 11:11 PM Sander Vanheule < > > sander@svanheule.net> wrote: ... > > > +#include <linux/swab.h> > > > > Not sure why you need this? See below. > > > + 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. > > My use of swab32/swahw32 has little to do with the CPU being BE or LE, > but more with the register packing in the GPIO peripheral. > > The supported SoCs have port layout A-B-C-D in the registers, where > firmware built with Realtek's SDK always denotes A0 as the first GPIO > line. So bit 24 in a register has the value for A0 (with the exception > of the IMR register). > > I wrote these wrapper functions to be able to use the BIT() macro with > the GPIO line number, similar to how gpio-mmio uses ioread32be() when > the BGPIOF_BIG_ENDIAN_BYTE_ORDER flag is used. > > For the IMR register, port A again comes first, but is now 16 bits wide > instead of 8, with A0 at bits 16:17. That's why swahw32 is used for > this register. > > On the currently unsupported RTL9300-series, the port layout is > reversed: D-C-B-A. GPIO line A0 is then at bit 0, so the swapping > functions won't be required. When support for this alternate port > layout is added, some code will need to be added to differentiate > between the two cases. Yes, you have different endianess on the hardware level, why not to use the proper accessors (with or without utilization of the above mentioned BGPIOF_BIG_ENDIAN_BYTE_ORDER)? ... > > > + case IRQ_TYPE_NONE: > > > + type = 0; > > > + handler = handle_bad_irq; > > > + break; > > > > Why is it here? Make it default like many other GPIO drivers do. > > > + irq_set_handler_locked(data, handler); > > > > handler is always the same. Use it directly here. > > I'll drop the IRQ_TYPE_NONE case. Do I understand it correctly, that > IRQ_TYPE_NONE should never be used as the new value, but only as the > default initial value? Initially you initialize the default handler to be "bad" (in order to easily catch up issues with IRQ configurations). When ->irq_set_type() is called, if everything is okay it will lock the handler to the proper one. -- With Best Regards, Andy Shevchenko
On Fri, 2021-03-19 at 19:57 +0200, Andy Shevchenko wrote: > On Fri, Mar 19, 2021 at 5:51 PM Sander Vanheule > <sander@svanheule.net> wrote: > > On Wed, 2021-03-17 at 15:08 +0200, Andy Shevchenko wrote: > > > On Mon, Mar 15, 2021 at 11:11 PM Sander Vanheule < > > > sander@svanheule.net> wrote: > > ... > > > > > +#include <linux/swab.h> > > > > > > Not sure why you need this? See below. > > > > > + 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. > > > > My use of swab32/swahw32 has little to do with the CPU being BE or > > LE, > > but more with the register packing in the GPIO peripheral. > > > > The supported SoCs have port layout A-B-C-D in the registers, where > > firmware built with Realtek's SDK always denotes A0 as the first > > GPIO > > line. So bit 24 in a register has the value for A0 (with the > > exception > > of the IMR register). > > > > I wrote these wrapper functions to be able to use the BIT() macro > > with > > the GPIO line number, similar to how gpio-mmio uses ioread32be() > > when > > the BGPIOF_BIG_ENDIAN_BYTE_ORDER flag is used. > > > > For the IMR register, port A again comes first, but is now 16 bits > > wide > > instead of 8, with A0 at bits 16:17. That's why swahw32 is used for > > this register. > > > > On the currently unsupported RTL9300-series, the port layout is > > reversed: D-C-B-A. GPIO line A0 is then at bit 0, so the swapping > > functions won't be required. When support for this alternate port > > layout is added, some code will need to be added to differentiate > > between the two cases. > > Yes, you have different endianess on the hardware level, why not to > use the proper accessors (with or without utilization of the above > mentioned BGPIOF_BIG_ENDIAN_BYTE_ORDER)? The point I was trying to make, is that it isn't an endianess issue. I shouldn't have used a register with single byte values to try to illustrate that. Consider instead the interrupt masking registers. To write the IMR bits for port A (GPIO 0-7), a 16-bit value must be written. This value (e.g. u16 port_a_imr) is always BE, independent of the packing order of the ports in the registers: // On RTL8380: port A is in the upper word writew(port_a_imr, base + OFFSET_IMR_AB); // On RTL9300: port A is in the lower word writew(port_a_imr, base + OFFSET_IMR_AB + 2); I want the low GPIO lines to be in the lower half-word, so I can manipulate GPIO lines 0-15 with simple mask and shift operations. It just so happens, that all registers needed by bgpio_init contain single-byte values. With BGPIO_BIG_ENDIAN_BYTE_ORDER the port order is reversed as required, but it's a bit of a misnomer here. Best, Sander
On Fri, 2021-03-19 at 23:24 +0200, Andy Shevchenko wrote: > On Fri, Mar 19, 2021 at 11:20 PM Sander Vanheule < > sander@svanheule.net> wrote: > > On Fri, 2021-03-19 at 19:57 +0200, Andy Shevchenko wrote: > > > On Fri, Mar 19, 2021 at 5:51 PM Sander Vanheule > > > <sander@svanheule.net> wrote: > > > > On Wed, 2021-03-17 at 15:08 +0200, Andy Shevchenko wrote: > > > > > On Mon, Mar 15, 2021 at 11:11 PM Sander Vanheule < > > > > > sander@svanheule.net> wrote: > > ... > > > > > > > + 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. > > > > > > > > My use of swab32/swahw32 has little to do with the CPU being BE > > > > or > > > > LE, > > > > but more with the register packing in the GPIO peripheral. > > > > > > > > The supported SoCs have port layout A-B-C-D in the registers, > > > > where > > > > firmware built with Realtek's SDK always denotes A0 as the first > > > > GPIO > > > > line. So bit 24 in a register has the value for A0 (with the > > > > exception > > > > of the IMR register). > > > > > > > > I wrote these wrapper functions to be able to use the BIT() macro > > > > with > > > > the GPIO line number, similar to how gpio-mmio uses ioread32be() > > > > when > > > > the BGPIOF_BIG_ENDIAN_BYTE_ORDER flag is used. > > > > > > > > For the IMR register, port A again comes first, but is now 16 > > > > bits > > > > wide > > > > instead of 8, with A0 at bits 16:17. That's why swahw32 is used > > > > for > > > > this register. > > > > > > > > On the currently unsupported RTL9300-series, the port layout is > > > > reversed: D-C-B-A. GPIO line A0 is then at bit 0, so the swapping > > > > functions won't be required. When support for this alternate port > > > > layout is added, some code will need to be added to differentiate > > > > between the two cases. > > > > > > Yes, you have different endianess on the hardware level, why not to > > > use the proper accessors (with or without utilization of the above > > > mentioned BGPIOF_BIG_ENDIAN_BYTE_ORDER)? > > > > The point I was trying to make, is that it isn't an endianess issue. > > I > > shouldn't have used a register with single byte values to try to > > illustrate that. > > > > Consider instead the interrupt masking registers. To write the IMR > > bits > > for port A (GPIO 0-7), a 16-bit value must be written. This value > > (e.g. > > u16 port_a_imr) is always BE, independent of the packing order of the > > ports in the registers: > > > > // On RTL8380: port A is in the upper word > > writew(port_a_imr, base + OFFSET_IMR_AB); > > > > // On RTL9300: port A is in the lower word > > writew(port_a_imr, base + OFFSET_IMR_AB + 2); > > > > I want the low GPIO lines to be in the lower half-word, so I can > > manipulate GPIO lines 0-15 with simple mask and shift operations. > > > > It just so happens, that all registers needed by bgpio_init contain > > single-byte values. With BGPIO_BIG_ENDIAN_BYTE_ORDER the port order > > is > > reversed as required, but it's a bit of a misnomer here. > > How many registers (per GPIO / port) do you have? > Can you list them and show endianess of the data for each of them and > for old and new hardware (something like a 3 column table)? Each GPIO bank, with 32 GPIO lines, consists of four 8-line ports. There are seven registers per port, but only five are used: | | Data | RTL8380 | RTL9300 Reg | Offset | type | byte order | byte order -------+--------+---------+------------+----------- DIR | 0x08 | 4 * u8 | A-B-C-D | D-C-B-A DATA | 0x0C | 4 * u8 | A-B-C-D | D-C-B-A ISR | 0x10 | 4 * u8 | A-B-C-D | D-C-B-A IMR_AB | 0x14 | 2 * u16 | A-A-B-B | B-B-A-A IMR_CD | 0x18 | 2 * u16 | C-C-D-D | D-D-C-C The unused other registers are all 4*u8. A-B-C-D means: (A << 24) | (B << 16) | (C << 8) | D A-A-B-B means: (A << 16) | B -- Best, Sander
On Fri, Mar 19, 2021 at 11:48 PM Sander Vanheule <sander@svanheule.net> wrote: > On Fri, 2021-03-19 at 23:24 +0200, Andy Shevchenko wrote: > > On Fri, Mar 19, 2021 at 11:20 PM Sander Vanheule < > > sander@svanheule.net> wrote: > > > On Fri, 2021-03-19 at 19:57 +0200, Andy Shevchenko wrote: ... > > > The point I was trying to make, is that it isn't an endianess issue. > > > I > > > shouldn't have used a register with single byte values to try to > > > illustrate that. > > > > > > Consider instead the interrupt masking registers. To write the IMR > > > bits > > > for port A (GPIO 0-7), a 16-bit value must be written. This value > > > (e.g. > > > u16 port_a_imr) is always BE, independent of the packing order of the > > > ports in the registers: > > > > > > // On RTL8380: port A is in the upper word > > > writew(port_a_imr, base + OFFSET_IMR_AB); > > > > > > // On RTL9300: port A is in the lower word > > > writew(port_a_imr, base + OFFSET_IMR_AB + 2); > > > > > > I want the low GPIO lines to be in the lower half-word, so I can > > > manipulate GPIO lines 0-15 with simple mask and shift operations. > > > > > > It just so happens, that all registers needed by bgpio_init contain > > > single-byte values. With BGPIO_BIG_ENDIAN_BYTE_ORDER the port order > > > is > > > reversed as required, but it's a bit of a misnomer here. > > > > How many registers (per GPIO / port) do you have? > > Can you list them and show endianess of the data for each of them and > > for old and new hardware (something like a 3 column table)? > > Each GPIO bank, with 32 GPIO lines, consists of four 8-line ports. > There are seven registers per port, but only five are used: > > | | Data | RTL8380 | RTL9300 > Reg | Offset | type | byte order | byte order > -------+--------+---------+------------+----------- > DIR | 0x08 | 4 * u8 | A-B-C-D | D-C-B-A > DATA | 0x0C | 4 * u8 | A-B-C-D | D-C-B-A > ISR | 0x10 | 4 * u8 | A-B-C-D | D-C-B-A > IMR_AB | 0x14 | 2 * u16 | A-A-B-B | B-B-A-A > IMR_CD | 0x18 | 2 * u16 | C-C-D-D | D-D-C-C > > The unused other registers are all 4*u8. You mean that they are following the same rules as DIR/DATA/ISR. right? > A-B-C-D means: (A << 24) | (B << 16) | (C << 8) | D > A-A-B-B means: (A << 16) | B If the above is true for unused registers, it's clearly hardware endianness. You need special treatment for IMR, but in general it follows the logic behind the others. So, you need some kind of I/O accessors like read_u8_reg() write_u8_reg() read_u16_reg() write_u16_reg() And depending on endianess of hardware to call proper set of them.
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 + 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> +#include <linux/platform_device.h> +#include <linux/swab.h> + +/* + * 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 +#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)); +} + +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; +} + +static inline u32 realtek_gpio_imr_bits(unsigned int pin, u32 value) +{ + return ((value & REALTEK_GPIO_IMR_LINE_MASK) << 2*(pin % 16)); +} + +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); + + 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; + 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); + + 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); + 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 +}; + +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 + }, + {}, +}; + +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; + + if (of_property_read_u32(np, "ngpios", &ngpios) != 0) + 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); + 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), + 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; +} + +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@svanheule.net>"); +MODULE_LICENSE("GPL v2");