| 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: <CAMuHMdU4=dRZQKbAx0=zLWCjd7sPRjLtE3KEJP_-8qjjakCBXw@mail.gmail.com>
Date: Thu, 24 Jun 2021 13:13:12 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Cc: Rob Herring <robh+dt@...nel.org>,
Linus Walleij <linus.walleij@...aro.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
Prabhakar <prabhakar.csengg@...il.com>,
Biju Das <biju.das.jz@...renesas.com>
Subject: Re: [PATCH 2/3] pinctrl: renesas: Add RZ/G2L pin and gpio controller
core wrapper
Hi Prabhakar,
On Wed, Jun 16, 2021 at 3:27 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@...renesas.com> wrote:
> Add core support for pin and gpio controller.
>
> Based on a patch in the BSP by Hien Huynh <hien.huynh.px@...esas.com>.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@...renesas.com>
> ---
> drivers/pinctrl/renesas/Kconfig | 11 +
> drivers/pinctrl/renesas/Makefile | 1 +
> drivers/pinctrl/renesas/pinctrl-rzg2l.c | 530 ++++++++++++++++++++++++
> drivers/pinctrl/renesas/pinctrl-rzg2l.h | 94 +++++
> 4 files changed, 636 insertions(+)
> create mode 100644 drivers/pinctrl/renesas/pinctrl-rzg2l.c
> create mode 100644 drivers/pinctrl/renesas/pinctrl-rzg2l.h
>
> diff --git a/drivers/pinctrl/renesas/Kconfig b/drivers/pinctrl/renesas/Kconfig
> index 4b84a744ae87..2b4ac226ce35 100644
> --- a/drivers/pinctrl/renesas/Kconfig
> +++ b/drivers/pinctrl/renesas/Kconfig
> @@ -176,6 +176,17 @@ config PINCTRL_RZA2
> help
> This selects GPIO and pinctrl driver for Renesas RZ/A2 platforms.
>
> +config PINCTRL_RZG2L
> + bool "pin control support for RZ/G2L family"
As this is selected by PINCTRL_PFC_R9A07G044, it should probably be
invisible, i.e. add "if COMPILE_TEST"?
> + depends on OF
> + depends on ARCH_R9A07G044 || COMPILE_TEST
Hence no need for this dependency.
> + select GPIOLIB
> + select GENERIC_PINCTRL_GROUPS
> + select GENERIC_PINMUX_FUNCTIONS
> + select GENERIC_PINCONF
> + help
> + This enables common pin control functionality for platforms based on RZ/G2L family.
> +
> config PINCTRL_PFC_R8A77470
> bool "pin control support for RZ/G1C" if COMPILE_TEST
> select PINCTRL_SH_PFC
> diff --git a/drivers/pinctrl/renesas/Makefile b/drivers/pinctrl/renesas/Makefile
> index 353563228dc2..7d9238a9ef57 100644
> --- a/drivers/pinctrl/renesas/Makefile
> +++ b/drivers/pinctrl/renesas/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_PINCTRL_PFC_SHX3) += pfc-shx3.o
>
> obj-$(CONFIG_PINCTRL_RZA1) += pinctrl-rza1.o
> obj-$(CONFIG_PINCTRL_RZA2) += pinctrl-rza2.o
> +obj-$(CONFIG_PINCTRL_RZG2L) += pinctrl-rzg2l.o
> obj-$(CONFIG_PINCTRL_RZN1) += pinctrl-rzn1.o
>
> ifeq ($(CONFIG_COMPILE_TEST),y)
> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> new file mode 100644
> index 000000000000..b9730b53fd85
> --- /dev/null
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -0,0 +1,530 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/G2L Pin Control and GPIO driver core
> + *
> + * Copyright (C) 2021 Renesas Electronics Corporation.
> + */
> +
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +
> +#include "pinctrl-rzg2l.h"
> +
> +#define DRV_NAME "pinctrl-rzg2l"
> +
> +#define P(n) (0x0000 + 0x10 + (n))
> +#define PM(n) (0x0100 + 0x20 + (n) * 2)
> +#define PMC(n) (0x0200 + 0x10 + (n))
> +#define PFC(n) (0x0400 + 0x40 + (n) * 4)
> +#define PIN(n) (0x0800 + 0x10 + (n))
> +#define PWPR (0x3014)
> +
> +#define PWPR_B0WI BIT(7) /* Bit Write Disable */
> +#define PWPR_PFCWE BIT(6) /* PFC Register Write Enable */
> +
> +#define PM_MASK 0x03
> +#define PFC_MASK 0x07
> +
> +#define PM_INPUT 0x1
> +#define PM_OUTPUT 0x2
> +#define PM_OUTPUT_INPUT 0x3
> +
> +#define GPIOF_OUTPUT 0
> +#define GPIOF_INPUT 1
> +#define GPIOF_BIDIRECTION 2
> +#define GPIOF_HI_Z 3
Please drop these, and use GPIO_LINE_DIRECTION_{IN,OUT} instead.
> +
> +#define RZG2L_PIN_ID_TO_PORT(id) ((id) / RZG2L_MAX_PINS_PER_PORT)
> +#define RZG2L_PIN_ID_TO_PIN(id) ((id) % RZG2L_MAX_PINS_PER_PORT)
> +
> +struct rzg2l_pinctrl {
> + struct pinctrl_dev *pctrl_dev;
> + struct pinctrl_desc pctrl_desc;
> +
> + void __iomem *base;
> + struct device *dev;
> + struct clk *clk;
> +
> + struct gpio_chip gpio_chip;
> + struct pinctrl_gpio_range gpio_range;
> +
> + const struct rzg2l_pin_soc *psoc;
> +
> + spinlock_t lock;
> + unsigned int nports;
> +};
> +
> +static void rzg2l_pinctrl_set_pfc_mode(struct rzg2l_pinctrl *pctrl,
> + int pins, unsigned long pfc_mode)
unsigned int pfc_mode?
> +{
> + u32 port = RZG2L_PIN_ID_TO_PORT(pins);
> + u8 bit = RZG2L_PIN_ID_TO_PIN(pins);
> + unsigned long flags;
> + u32 reg32, mask32;
> + u16 reg16, mask16;
> + u8 reg8;
> +
> + spin_lock_irqsave(&pctrl->lock, flags);
> +
> + /* Set pin to 'Non-use (Hi-Z input protection)' */
> + reg16 = readw(pctrl->base + PM(port));
> + mask16 = PM_MASK << (bit * 2);
> + reg16 = reg16 & ~mask16;
reg16 &= ...
Perhaps drop the mask16:
reg16 &= ~(PM_MASK << (bit * 2));
> + writew(reg16, pctrl->base + PM(port));
> +
> + /* Temporarily switch to GPIO mode with PMC register */
> + reg8 = readb(pctrl->base + PMC(port));
> + writeb(reg8 & ~BIT(bit), pctrl->base + PMC(port));
> +
> + /* Set the PWPR register to allow PFC register to write */
> + writel(0x00, pctrl->base + PWPR); /* B0WI=0, PFCWE=0 */
> + writel(PWPR_PFCWE, pctrl->base + PWPR); /* B0WI=0, PFCWE=1 */
> +
> + /* Select Pin function mode with PFC register */
> + reg32 = readl(pctrl->base + PFC(port));
> + mask32 = PFC_MASK << (bit * 4);
> + reg32 = reg32 & ~mask32;
reg32 &= ...
Perhaps drop the mask32, too?
> + pfc_mode = pfc_mode << (bit * 4);
pfc_mode <<= ...
> + writel(reg32 | pfc_mode, pctrl->base + PFC(port));
Or just drop the line before, and do:
writel(reg32 | (pfc_mode << (bit * 4)), pctrl->base + PFC(port));
> +
> + /* Set the PWPR register to be write-protected */
> + writel(0x00, pctrl->base + PWPR); /* B0WI=0, PFCWE=0 */
> + writel(PWPR_B0WI, pctrl->base + PWPR); /* B0WI=1, PFCWE=0 */
> +
> + /* Switch to Peripheral pin function with PMC register */
> + reg8 = readb(pctrl->base + PMC(port));
> + writeb(reg8 | BIT(bit), pctrl->base + PMC(port));
> +
> + spin_unlock_irqrestore(&pctrl->lock, flags);
> +};
> +
> +static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev,
> + unsigned int func_selector,
> + unsigned int group_selector)
> +{
> + struct rzg2l_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> + struct function_desc *func;
> + struct group_desc *group;
> + unsigned long data;
> + int *pins;
> + int i;
unsigned int i
> +
> + func = pinmux_generic_get_function(pctldev, func_selector);
> + if (!func)
> + return -EINVAL;
> + group = pinctrl_generic_get_group(pctldev, group_selector);
> + if (!group)
> + return -EINVAL;
> +
> + pins = group->pins;
> + data = (unsigned long)group->data;
> +
> + dev_dbg(pctldev->dev, "enable function %s group %s\n",
> + func->name, group->name);
> +
> + for (i = 0; i < group->num_pins; i++)
> + rzg2l_pinctrl_set_pfc_mode(pctrl, *(pins + i), data);
pins[i]
> +
> + return 0;
> +};
> +
> +static const struct pinctrl_ops rzg2l_pinctrl_pctlops = {
> + .get_groups_count = pinctrl_generic_get_group_count,
> + .get_group_name = pinctrl_generic_get_group_name,
> + .get_group_pins = pinctrl_generic_get_group_pins,
> + .dt_node_to_map = pinconf_generic_dt_node_to_map_all,
> + .dt_free_map = pinconf_generic_dt_free_map,
> +};
> +
> +static const struct pinmux_ops rzg2l_pinctrl_pmxops = {
> + .get_functions_count = pinmux_generic_get_function_count,
> + .get_function_name = pinmux_generic_get_function_name,
> + .get_function_groups = pinmux_generic_get_function_groups,
> + .set_mux = rzg2l_pinctrl_set_mux,
> + .strict = true,
> +};
> +
> +static int rzg2l_pinctrl_add_groups(struct rzg2l_pinctrl *pctrl)
> +{
> + int ret, i;
unsigned int i
> +
> + for (i = 0; i < pctrl->psoc->ngroups; i++) {
> + const struct group_desc *group = pctrl->psoc->groups + i;
> +
> + ret = pinctrl_generic_add_group(pctrl->pctrl_dev, group->name,
> + group->pins, group->num_pins,
> + group->data);
> + if (ret < 0) {
> + dev_err(pctrl->dev, "Failed to register group %s\n",
> + group->name);
This can really only fail in case of out-of-memory, or if group->name
is NULL, so I don't think there's a need to print an error message.
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int rzg2l_pinctrl_add_functions(struct rzg2l_pinctrl *pctrl)
> +{
> + int ret, i;
unsigned int i
> +
> + for (i = 0; i < pctrl->psoc->nfuncs; i++) {
> + const struct function_desc *func = pctrl->psoc->funcs + i;
> +
> + ret = pinmux_generic_add_function(pctrl->pctrl_dev, func->name,
> + func->group_names,
> + func->num_group_names,
> + func->data);
> + if (ret < 0) {
> + dev_err(pctrl->dev, "Failed to register function %s\n",
> + func->name);
This can really only fail in case of out-of-memory, or if func->name
is NULL, so I don't think there's a need to print an error message.
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +static void rzg2l_gpio_set_direction(struct rzg2l_pinctrl *pctrl, u32 port,
> + u8 bit, bool output)
> +{
> + unsigned long flags;
> + u16 reg16;
> +
> + spin_lock_irqsave(&pctrl->lock, flags);
> +
> + reg16 = readw(pctrl->base + PM(port));
> + reg16 = reg16 & ~(PM_MASK << (bit * 2));
reg16 &= ...
or just combine with the line before?
> +
> + if (output)
> + writew(reg16 | (PM_OUTPUT << (bit * 2)),
> + pctrl->base + PM(port));
> + else
> + writew(reg16 | (PM_INPUT << (bit * 2)),
> + pctrl->base + PM(port));
This can be simplified to
if (output)
reg16 |= PM_OUTPUT << (bit * 2);
else
reg16 |= PM_INPUT << (bit * 2);
writew(reg16, pctrl->base + PM(port));
or perhaps even shortened to:
reg16 |= (output ? PM_OUTPUT : PM_INPUT) << (bit * 2);
writew(reg16, pctrl->base + PM(port));
> +
> + spin_unlock_irqrestore(&pctrl->lock, flags);
> +}
> +
> +static int rzg2l_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct rzg2l_pinctrl *pctrl = gpiochip_get_data(chip);
> + u32 port = RZG2L_PIN_ID_TO_PORT(offset);
> + u8 bit = RZG2L_PIN_ID_TO_PIN(offset);
> +
> + if (!(readb(pctrl->base + PMC(port)) & BIT(bit))) {
> + u16 reg16;
> +
> + reg16 = readw(pctrl->base + PM(port));
> + reg16 = (reg16 >> (bit * 2)) & PM_MASK;
> + if (reg16 == PM_OUTPUT)
> + return GPIOF_OUTPUT;
> + else if (reg16 == PM_INPUT)
> + return GPIOF_INPUT;
> + else if (reg16 == PM_OUTPUT_INPUT)
> + return GPIOF_BIDIRECTION;
> + else
> + return GPIOF_HI_Z;
These should return either GPIO_LINE_DIRECTION_OUT or
GPIO_LINE_DIRECTION_IN. No other non-error values are defined for
the .get_direction() callback.
> + } else {
> + return -EINVAL;
> + }
> +}
> +static int rzg2l_gpio_direction_output(struct gpio_chip *chip,
> + unsigned int offset, int value)
> +{
> + struct rzg2l_pinctrl *pctrl = gpiochip_get_data(chip);
> + u32 port = RZG2L_PIN_ID_TO_PORT(offset);
> + u8 bit = RZG2L_PIN_ID_TO_PIN(offset);
> +
> + rzg2l_gpio_set_direction(pctrl, port, bit, true);
> + rzg2l_gpio_set(chip, offset, value);
Probably the order of these two operations should be reversed, to
avoid glitches.
> +
> + return 0;
> +}
> +
> +static int rzg2l_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct rzg2l_pinctrl *pctrl = gpiochip_get_data(chip);
> + u32 port = RZG2L_PIN_ID_TO_PORT(offset);
> + u8 bit = RZG2L_PIN_ID_TO_PIN(offset);
> + u16 reg16;
> +
> + reg16 = readw(pctrl->base + PM(port));
> + reg16 = (reg16 >> (bit * 2)) & PM_MASK;
> +
> + if (reg16 == PM_INPUT || reg16 == PM_OUTPUT_INPUT)
BTW, how do you configure a pin for PM_OUTPUT_INPUT?
> + return !!(readb(pctrl->base + PIN(port)) & BIT(bit));
> + else if (reg16 == PM_OUTPUT)
> + return !!(readb(pctrl->base + P(port)) & BIT(bit));
> + else
> + return -EINVAL;
> +}
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Powered by blists - more mailing lists