diff mbox series

[v3,12/16] pinctrl: starfive: Add pinctrl driver for StarFive SoCs

Message ID 20211102161125.1144023-13-kernel@esmil.dk (mailing list archive)
State New, archived
Headers show
Series Basic StarFive JH7100 RISC-V SoC support | expand

Commit Message

Emil Renner Berthing Nov. 2, 2021, 4:11 p.m. UTC
Add a combined pinctrl and GPIO driver for the JH7100 RISC-V SoC by
StarFive Ltd. This is a test chip for their upcoming JH7110 SoC, which
is said to feature only minor changes to these pinctrl/GPIO parts.

For each "GPIO" there are two registers for configuring the output and
output enable signals which may come from other peripherals. Among these
are two special signals that are constant 0 and constant 1 respectively.
Controlling the GPIOs from software is done by choosing one of these
signals. In other words the same registers are used for both pin muxing
and controlling the GPIOs, which makes it easier to combine the pinctrl
and GPIO driver in one.

I wrote the pinconf and pinmux parts, but the GPIO part of the code is
based on the GPIO driver in the vendor tree written by Huan Feng with
cleanups and fixes by Drew and me.

Datasheet: https://github.com/starfive-tech/JH7100_Docs/blob/main/JH7100%20Data%20Sheet%20V01.01.04-EN%20(4-21-2021).pdf
Co-developed-by: Huan Feng <huan.feng@starfivetech.com>
Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
Co-developed-by: Drew Fustini <drew@beagleboard.org>
Signed-off-by: Drew Fustini <drew@beagleboard.org>
---
 MAINTAINERS                        |    8 +
 drivers/pinctrl/Kconfig            |   17 +
 drivers/pinctrl/Makefile           |    1 +
 drivers/pinctrl/pinctrl-starfive.c | 1353 ++++++++++++++++++++++++++++
 4 files changed, 1379 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-starfive.c

Comments

Andy Shevchenko Nov. 2, 2021, 8:02 p.m. UTC | #1
On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
>
> Add a combined pinctrl and GPIO driver for the JH7100 RISC-V SoC by
> StarFive Ltd. This is a test chip for their upcoming JH7110 SoC, which
> is said to feature only minor changes to these pinctrl/GPIO parts.
>
> For each "GPIO" there are two registers for configuring the output and
> output enable signals which may come from other peripherals. Among these
> are two special signals that are constant 0 and constant 1 respectively.
> Controlling the GPIOs from software is done by choosing one of these
> signals. In other words the same registers are used for both pin muxing
> and controlling the GPIOs, which makes it easier to combine the pinctrl
> and GPIO driver in one.
>
> I wrote the pinconf and pinmux parts, but the GPIO part of the code is
> based on the GPIO driver in the vendor tree written by Huan Feng with
> cleanups and fixes by Drew and me.

...

> +       depends on OF

So this descreases test coverage.
Linus, can we provide a necessary stub so we may drop this dependency?

...

> +static inline struct device *starfive_dev(const struct starfive_pinctrl *sfp)
> +{
> +       return sfp->gc.parent;
> +}
> +

This seems useless helper. You may do what it's doing just in place.
It will save 5 LOCs.

...

> +static void starfive_pin_dbg_show(struct pinctrl_dev *pctldev,
> +                                 struct seq_file *s,
> +                                 unsigned int pin)
> +{
> +       struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev);
> +       unsigned int gpio = starfive_pin_to_gpio(sfp, pin);
> +       void __iomem *reg;
> +       u32 dout, doen;

> +       if (gpio >= NR_GPIOS)
> +               return;

Dead code?

> +       reg = sfp->base + GPON_DOUT_CFG + 8 * gpio;
> +       dout = readl_relaxed(reg + 0x000);
> +       doen = readl_relaxed(reg + 0x004);
> +
> +       seq_printf(s, "dout=%lu%s doen=%lu%s",
> +                  dout & GENMASK(7, 0), (dout & BIT(31)) ? "r" : "",
> +                  doen & GENMASK(7, 0), (doen & BIT(31)) ? "r" : "");
> +}

...

> +       struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev);
> +       struct device *dev = starfive_dev(sfp);
> +       const char **pgnames;
> +       struct pinctrl_map *map;
> +       struct device_node *child;
> +       const char *grpname;
> +       int *pins;
> +       u32 *pinmux;

Reversed xmas tree order?

> +       int nmaps;
> +       int ngroups;
> +       int ret;

...

> +static int starfive_pinconf_group_set(struct pinctrl_dev *pctldev,
> +                                     unsigned int gsel,
> +                                     unsigned long *configs,
> +                                     unsigned int num_configs)
> +{
> +       struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev);
> +       const struct group_desc *group;
> +       u16 mask, value;
> +       int i;
> +
> +       group = pinctrl_generic_get_group(pctldev, gsel);
> +       if (!group)
> +               return -EINVAL;
> +
> +       mask = 0;
> +       value = 0;
> +       for (i = 0; i < num_configs; i++) {
> +               int param = pinconf_to_config_param(configs[i]);
> +               u32 arg = pinconf_to_config_argument(configs[i]);
> +
> +               switch (param) {
> +               case PIN_CONFIG_BIAS_DISABLE:
> +                       mask |= PAD_BIAS_MASK;
> +                       value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE;
> +                       break;
> +               case PIN_CONFIG_BIAS_PULL_DOWN:
> +                       if (arg == 0)
> +                               return -ENOTSUPP;
> +                       mask |= PAD_BIAS_MASK;
> +                       value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_PULL_DOWN;
> +                       break;
> +               case PIN_CONFIG_BIAS_PULL_UP:
> +                       if (arg == 0)
> +                               return -ENOTSUPP;
> +                       mask |= PAD_BIAS_MASK;
> +                       value = value & ~PAD_BIAS_MASK;
> +                       break;
> +               case PIN_CONFIG_DRIVE_STRENGTH:
> +                       mask |= PAD_DRIVE_STRENGTH_MASK;
> +                       value = (value & ~PAD_DRIVE_STRENGTH_MASK) |
> +                               starfive_drive_strength_from_max_mA(arg);
> +                       break;
> +               case PIN_CONFIG_INPUT_ENABLE:
> +                       mask |= PAD_INPUT_ENABLE;
> +                       if (arg)
> +                               value |= PAD_INPUT_ENABLE;
> +                       else
> +                               value &= ~PAD_INPUT_ENABLE;
> +                       break;
> +               case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> +                       mask |= PAD_INPUT_SCHMITT_ENABLE;
> +                       if (arg)
> +                               value |= PAD_INPUT_SCHMITT_ENABLE;
> +                       else
> +                               value &= ~PAD_INPUT_SCHMITT_ENABLE;
> +                       break;
> +               case PIN_CONFIG_SLEW_RATE:
> +                       mask |= PAD_SLEW_RATE_MASK;
> +                       value = (value & ~PAD_SLEW_RATE_MASK) |
> +                               ((arg << PAD_SLEW_RATE_POS) & PAD_SLEW_RATE_MASK);
> +                       break;
> +               case PIN_CONFIG_STARFIVE_STRONG_PULL_UP:
> +                       if (arg) {
> +                               mask |= PAD_BIAS_MASK;
> +                               value = (value & ~PAD_BIAS_MASK) |
> +                                       PAD_BIAS_STRONG_PULL_UP;
> +                       } else {
> +                               mask |= PAD_BIAS_STRONG_PULL_UP;
> +                               value = value & ~PAD_BIAS_STRONG_PULL_UP;
> +                       }
> +                       break;
> +               default:
> +                       return -ENOTSUPP;
> +               }
> +       }
> +
> +       for (i = 0; i < group->num_pins; i++)
> +               starfive_padctl_rmw(sfp, group->pins[i], mask, value);
> +
> +       return 0;
> +}

...

> +static int starfive_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
> +{
> +       struct starfive_pinctrl *sfp = container_of(gc, struct starfive_pinctrl, gc);
> +       void __iomem *doen = sfp->base + GPON_DOEN_CFG + 8 * gpio;
> +
> +       /* return GPIO_LINE_DIRECTION_OUT (0) only if doen == GPO_ENABLE (0) */
> +       return readl_relaxed(doen) != GPO_ENABLE;

I believe the idea was to return the predefined values for the direction.

> +}

...

> +static int starfive_irq_set_type(struct irq_data *d, unsigned int trigger)
> +{
> +       struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
> +       irq_hw_number_t gpio = irqd_to_hwirq(d);
> +       void __iomem *base = sfp->base + 4 * (gpio / 32);
> +       u32 mask = BIT(gpio % 32);
> +       u32 irq_type, edge_both, polarity;
> +       unsigned long flags;
> +
> +       if (trigger & IRQ_TYPE_EDGE_BOTH)
> +               irq_set_handler_locked(d, handle_edge_irq);
> +       else if (trigger & IRQ_TYPE_LEVEL_MASK)
> +               irq_set_handler_locked(d, handle_level_irq);

Usually we don't assign this twice, so it should be after the switch.

> +       switch (trigger) {
> +       case IRQ_TYPE_EDGE_RISING:
> +               irq_type  = mask; /* 1: edge triggered */
> +               edge_both = 0;    /* 0: single edge */
> +               polarity  = mask; /* 1: rising edge */
> +               break;
> +       case IRQ_TYPE_EDGE_FALLING:
> +               irq_type  = mask; /* 1: edge triggered */
> +               edge_both = 0;    /* 0: single edge */
> +               polarity  = 0;    /* 0: falling edge */
> +               break;
> +       case IRQ_TYPE_EDGE_BOTH:
> +               irq_type  = mask; /* 1: edge triggered */
> +               edge_both = mask; /* 1: both edges */
> +               polarity  = 0;    /* 0: ignored */
> +               break;
> +       case IRQ_TYPE_LEVEL_HIGH:
> +               irq_type  = 0;    /* 0: level triggered */
> +               edge_both = 0;    /* 0: ignored */
> +               polarity  = mask; /* 1: high level */
> +               break;
> +       case IRQ_TYPE_LEVEL_LOW:
> +               irq_type  = 0;    /* 0: level triggered */
> +               edge_both = 0;    /* 0: ignored */
> +               polarity  = 0;    /* 0: low level */
> +               break;
> +       default:

> +               irq_set_handler_locked(d, handle_bad_irq);

Why? You have it already in ->probe(), what's the point?

> +               return -EINVAL;
> +       }
> +
> +       raw_spin_lock_irqsave(&sfp->lock, flags);
> +       irq_type |= readl_relaxed(base + GPIOIS) & ~mask;
> +       writel_relaxed(irq_type, base + GPIOIS);
> +       edge_both |= readl_relaxed(base + GPIOIBE) & ~mask;
> +       writel_relaxed(edge_both, base + GPIOIBE);
> +       polarity |= readl_relaxed(base + GPIOIEV) & ~mask;
> +       writel_relaxed(polarity, base + GPIOIEV);
> +       raw_spin_unlock_irqrestore(&sfp->lock, flags);
> +       return 0;
> +}

...

> +       ret = reset_control_deassert(rst);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "could not deassert resetd\n");

> +       ret = devm_pinctrl_register_and_init(dev, &starfive_desc, sfp, &sfp->pctl);
> +       if (ret)

I don't see who will assert reset here.

> +               return dev_err_probe(dev, ret, "could not register pinctrl driver\n");

...

> +       switch (value) {
> +       case 0:
> +               sfp->gpios.pin_base = PAD_INVALID_GPIO;
> +               goto done;
> +       case 1:
> +               sfp->gpios.pin_base = PAD_GPIO(0);
> +               break;
> +       case 2:
> +               sfp->gpios.pin_base = PAD_FUNC_SHARE(72);
> +               break;
> +       case 3:
> +               sfp->gpios.pin_base = PAD_FUNC_SHARE(70);
> +               break;
> +       case 4: case 5: case 6:
> +               sfp->gpios.pin_base = PAD_FUNC_SHARE(0);
> +               break;
> +       default:

Ditto.

> +               return dev_err_probe(dev, -EINVAL, "invalid signal group %u\n", value);
> +       }

...

> +       ret = devm_gpiochip_add_data(dev, &sfp->gc, sfp);
> +       if (ret)

Ditto.

> +               return dev_err_probe(dev, ret, "could not register gpiochip\n");
> +
> +done:
> +       return pinctrl_enable(sfp->pctl);

Ditto.

And better to use label name like following
out_pinctrl_enable:
Andy Shevchenko Nov. 2, 2021, 8:07 p.m. UTC | #2
On Tue, Nov 2, 2021 at 10:02 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@esmil.dk> wrote:

...

> > +static int starfive_pinconf_group_set(struct pinctrl_dev *pctldev,
> > +                                     unsigned int gsel,
> > +                                     unsigned long *configs,
> > +                                     unsigned int num_configs)
> > +{
> > +       struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev);
> > +       const struct group_desc *group;
> > +       u16 mask, value;
> > +       int i;
> > +
> > +       group = pinctrl_generic_get_group(pctldev, gsel);
> > +       if (!group)
> > +               return -EINVAL;
> > +
> > +       mask = 0;
> > +       value = 0;
> > +       for (i = 0; i < num_configs; i++) {
> > +               int param = pinconf_to_config_param(configs[i]);
> > +               u32 arg = pinconf_to_config_argument(configs[i]);
> > +
> > +               switch (param) {
> > +               case PIN_CONFIG_BIAS_DISABLE:
> > +                       mask |= PAD_BIAS_MASK;
> > +                       value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE;
> > +                       break;
> > +               case PIN_CONFIG_BIAS_PULL_DOWN:
> > +                       if (arg == 0)
> > +                               return -ENOTSUPP;
> > +                       mask |= PAD_BIAS_MASK;
> > +                       value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_PULL_DOWN;
> > +                       break;
> > +               case PIN_CONFIG_BIAS_PULL_UP:
> > +                       if (arg == 0)
> > +                               return -ENOTSUPP;
> > +                       mask |= PAD_BIAS_MASK;
> > +                       value = value & ~PAD_BIAS_MASK;
> > +                       break;
> > +               case PIN_CONFIG_DRIVE_STRENGTH:
> > +                       mask |= PAD_DRIVE_STRENGTH_MASK;
> > +                       value = (value & ~PAD_DRIVE_STRENGTH_MASK) |
> > +                               starfive_drive_strength_from_max_mA(arg);
> > +                       break;
> > +               case PIN_CONFIG_INPUT_ENABLE:
> > +                       mask |= PAD_INPUT_ENABLE;
> > +                       if (arg)
> > +                               value |= PAD_INPUT_ENABLE;
> > +                       else
> > +                               value &= ~PAD_INPUT_ENABLE;
> > +                       break;
> > +               case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> > +                       mask |= PAD_INPUT_SCHMITT_ENABLE;
> > +                       if (arg)
> > +                               value |= PAD_INPUT_SCHMITT_ENABLE;
> > +                       else
> > +                               value &= ~PAD_INPUT_SCHMITT_ENABLE;
> > +                       break;
> > +               case PIN_CONFIG_SLEW_RATE:
> > +                       mask |= PAD_SLEW_RATE_MASK;
> > +                       value = (value & ~PAD_SLEW_RATE_MASK) |
> > +                               ((arg << PAD_SLEW_RATE_POS) & PAD_SLEW_RATE_MASK);
> > +                       break;
> > +               case PIN_CONFIG_STARFIVE_STRONG_PULL_UP:
> > +                       if (arg) {
> > +                               mask |= PAD_BIAS_MASK;
> > +                               value = (value & ~PAD_BIAS_MASK) |
> > +                                       PAD_BIAS_STRONG_PULL_UP;
> > +                       } else {
> > +                               mask |= PAD_BIAS_STRONG_PULL_UP;
> > +                               value = value & ~PAD_BIAS_STRONG_PULL_UP;
> > +                       }
> > +                       break;
> > +               default:
> > +                       return -ENOTSUPP;
> > +               }
> > +       }
> > +
> > +       for (i = 0; i < group->num_pins; i++)
> > +               starfive_padctl_rmw(sfp, group->pins[i], mask, value);
> > +
> > +       return 0;
> > +}

Linus any comments on this code (sorry if I missed your reply)? The
idea behind above is to skip all settings from the same category and
apply only the last one, e.g. if we have "bias set to X", ..., "bias
disable", ..., "bias set to Y", the hardware will see only the last
operation, i.e. "bias set to Y". I think it may not be the best
approach (theoretically?) since the hardware definitely may behave
differently on the other side in case of such series of the
configurations (yes, I have seen some interesting implementations of
the touchpad / touchscreen GPIOs that may be affected).
Emil Renner Berthing Nov. 2, 2021, 8:35 p.m. UTC | #3
On Tue, 2 Nov 2021 at 21:02, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
> > Add a combined pinctrl and GPIO driver for the JH7100 RISC-V SoC by
> > StarFive Ltd. This is a test chip for their upcoming JH7110 SoC, which
> > is said to feature only minor changes to these pinctrl/GPIO parts.
> >
> > For each "GPIO" there are two registers for configuring the output and
> > output enable signals which may come from other peripherals. Among these
> > are two special signals that are constant 0 and constant 1 respectively.
> > Controlling the GPIOs from software is done by choosing one of these
> > signals. In other words the same registers are used for both pin muxing
> > and controlling the GPIOs, which makes it easier to combine the pinctrl
> > and GPIO driver in one.
> >
> > I wrote the pinconf and pinmux parts, but the GPIO part of the code is
> > based on the GPIO driver in the vendor tree written by Huan Feng with
> > cleanups and fixes by Drew and me.
>
> ...
>
> > +       depends on OF
>
> So this descreases test coverage.
> Linus, can we provide a necessary stub so we may drop this dependency?
>
> ...
>
> > +static inline struct device *starfive_dev(const struct starfive_pinctrl *sfp)
> > +{
> > +       return sfp->gc.parent;
> > +}
> > +
>
> This seems useless helper. You may do what it's doing just in place.
> It will save 5 LOCs.

I don't mind removing it, I just think it's easier to read when we're
explicit that all we want is a dev pointer, and we don't suddenly need
to know the parent of the gpio chip in all the pinmux/pinconf
callbacks.

> > +static void starfive_pin_dbg_show(struct pinctrl_dev *pctldev,
> > +                                 struct seq_file *s,
> > +                                 unsigned int pin)
> > +{
> > +       struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev);
> > +       unsigned int gpio = starfive_pin_to_gpio(sfp, pin);
> > +       void __iomem *reg;
> > +       u32 dout, doen;
>
> > +       if (gpio >= NR_GPIOS)
> > +               return;
>
> Dead code?

No, this function is called for all 206 configurable pins, but only 64
of them are gpio pins. Which ones depend on the signal group.

> > +       reg = sfp->base + GPON_DOUT_CFG + 8 * gpio;
> > +       dout = readl_relaxed(reg + 0x000);
> > +       doen = readl_relaxed(reg + 0x004);
> > +
> > +       seq_printf(s, "dout=%lu%s doen=%lu%s",
> > +                  dout & GENMASK(7, 0), (dout & BIT(31)) ? "r" : "",
> > +                  doen & GENMASK(7, 0), (doen & BIT(31)) ? "r" : "");
> > +}
>
> ...
>
> > +       struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev);
> > +       struct device *dev = starfive_dev(sfp);
> > +       const char **pgnames;
> > +       struct pinctrl_map *map;
> > +       struct device_node *child;
> > +       const char *grpname;
> > +       int *pins;
> > +       u32 *pinmux;
>
> Reversed xmas tree order?
>
> > +static int starfive_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
> > +{
> > +       struct starfive_pinctrl *sfp = container_of(gc, struct starfive_pinctrl, gc);
> > +       void __iomem *doen = sfp->base + GPON_DOEN_CFG + 8 * gpio;
> > +
> > +       /* return GPIO_LINE_DIRECTION_OUT (0) only if doen == GPO_ENABLE (0) */
> > +       return readl_relaxed(doen) != GPO_ENABLE;
>
> I believe the idea was to return the predefined values for the direction.

You mean this?
  return readl_relaxed(doen) == GPO_ENABLE ? GPIO_LINE_DIRECTION_OUT :
GPIO_LINE_DIRECTION_IN;

> > +}
>
> ...
>
> > +static int starfive_irq_set_type(struct irq_data *d, unsigned int trigger)
> > +{
> > +       struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
> > +       irq_hw_number_t gpio = irqd_to_hwirq(d);
> > +       void __iomem *base = sfp->base + 4 * (gpio / 32);
> > +       u32 mask = BIT(gpio % 32);
> > +       u32 irq_type, edge_both, polarity;
> > +       unsigned long flags;
> > +
> > +       if (trigger & IRQ_TYPE_EDGE_BOTH)
> > +               irq_set_handler_locked(d, handle_edge_irq);
> > +       else if (trigger & IRQ_TYPE_LEVEL_MASK)
> > +               irq_set_handler_locked(d, handle_level_irq);
>
> Usually we don't assign this twice, so it should be after the switch.
>
> > +       switch (trigger) {
> > +       case IRQ_TYPE_EDGE_RISING:
> > +               irq_type  = mask; /* 1: edge triggered */
> > +               edge_both = 0;    /* 0: single edge */
> > +               polarity  = mask; /* 1: rising edge */
> > +               break;
> > +       case IRQ_TYPE_EDGE_FALLING:
> > +               irq_type  = mask; /* 1: edge triggered */
> > +               edge_both = 0;    /* 0: single edge */
> > +               polarity  = 0;    /* 0: falling edge */
> > +               break;
> > +       case IRQ_TYPE_EDGE_BOTH:
> > +               irq_type  = mask; /* 1: edge triggered */
> > +               edge_both = mask; /* 1: both edges */
> > +               polarity  = 0;    /* 0: ignored */
> > +               break;
> > +       case IRQ_TYPE_LEVEL_HIGH:
> > +               irq_type  = 0;    /* 0: level triggered */
> > +               edge_both = 0;    /* 0: ignored */
> > +               polarity  = mask; /* 1: high level */
> > +               break;
> > +       case IRQ_TYPE_LEVEL_LOW:
> > +               irq_type  = 0;    /* 0: level triggered */
> > +               edge_both = 0;    /* 0: ignored */
> > +               polarity  = 0;    /* 0: low level */
> > +               break;
> > +       default:
>
> > +               irq_set_handler_locked(d, handle_bad_irq);
>
> Why? You have it already in ->probe(), what's the point?

So last time you asked about this, I explained a situation where
userspace first grabs a GPIO, set the interrupt to edge triggered, and
then later loads a driver that requests an unsupported IRQ type. Then
I'd like to set the handler back to handle_bad_irq so we don't get
weird interrupts, but maybe now you know a reason why that doesn't
matter or can't happen?

> > +               return -EINVAL;
> > +       }
> > +
> > +       raw_spin_lock_irqsave(&sfp->lock, flags);
> > +       irq_type |= readl_relaxed(base + GPIOIS) & ~mask;
> > +       writel_relaxed(irq_type, base + GPIOIS);
> > +       edge_both |= readl_relaxed(base + GPIOIBE) & ~mask;
> > +       writel_relaxed(edge_both, base + GPIOIBE);
> > +       polarity |= readl_relaxed(base + GPIOIEV) & ~mask;
> > +       writel_relaxed(polarity, base + GPIOIEV);
> > +       raw_spin_unlock_irqrestore(&sfp->lock, flags);
> > +       return 0;
> > +}
>
> ...
>
> > +       ret = reset_control_deassert(rst);
> > +       if (ret)
> > +               return dev_err_probe(dev, ret, "could not deassert resetd\n");
>
> > +       ret = devm_pinctrl_register_and_init(dev, &starfive_desc, sfp, &sfp->pctl);
> > +       if (ret)
>
> I don't see who will assert reset here.

No, so originally this driver would first assert and then deassert
reset. I decided against that because in all likelyhood earlier boot
stages would have set pinmux up for a serial port, and we don't want
to interrupt the serial debug output. The only reason I make sure the
reset line is deasserted is in case someone makes a really minimal
bootloader that just does the absolute minimal to load a Linux kernel
and doesn't even log any anything.

By the same token we also don't want to assert reset on error in case
it resets pin muxing for the the serial line that was supposed to log
the error.

>
> > +               return dev_err_probe(dev, ret, "could not register pinctrl driver\n");
>
> ...
>
> > +       switch (value) {
> > +       case 0:
> > +               sfp->gpios.pin_base = PAD_INVALID_GPIO;
> > +               goto done;
> > +       case 1:
> > +               sfp->gpios.pin_base = PAD_GPIO(0);
> > +               break;
> > +       case 2:
> > +               sfp->gpios.pin_base = PAD_FUNC_SHARE(72);
> > +               break;
> > +       case 3:
> > +               sfp->gpios.pin_base = PAD_FUNC_SHARE(70);
> > +               break;
> > +       case 4: case 5: case 6:
> > +               sfp->gpios.pin_base = PAD_FUNC_SHARE(0);
> > +               break;
> > +       default:
>
> Ditto.
>
> > +               return dev_err_probe(dev, -EINVAL, "invalid signal group %u\n", value);
> > +       }
>
> ...
>
> > +       ret = devm_gpiochip_add_data(dev, &sfp->gc, sfp);
> > +       if (ret)
>
> Ditto.
>
> > +               return dev_err_probe(dev, ret, "could not register gpiochip\n");
> > +
> > +done:
> > +       return pinctrl_enable(sfp->pctl);
>
> Ditto.
>
> And better to use label name like following
> out_pinctrl_enable:

Good idea.
Andy Shevchenko Nov. 3, 2021, 9:12 a.m. UTC | #4
On Tue, Nov 2, 2021 at 10:35 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
>
> On Tue, 2 Nov 2021 at 21:02, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@esmil.dk> wrote:

...

> > > +static inline struct device *starfive_dev(const struct starfive_pinctrl *sfp)
> > > +{
> > > +       return sfp->gc.parent;
> > > +}
> > > +
> >
> > This seems useless helper. You may do what it's doing just in place.
> > It will save 5 LOCs.
>
> I don't mind removing it, I just think it's easier to read when we're
> explicit that all we want is a dev pointer, and we don't suddenly need
> to know the parent of the gpio chip in all the pinmux/pinconf
> callbacks.

I don't really see the gain of it.

...

> > > +static int starfive_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
> > > +{
> > > +       struct starfive_pinctrl *sfp = container_of(gc, struct starfive_pinctrl, gc);
> > > +       void __iomem *doen = sfp->base + GPON_DOEN_CFG + 8 * gpio;
> > > +
> > > +       /* return GPIO_LINE_DIRECTION_OUT (0) only if doen == GPO_ENABLE (0) */
> > > +       return readl_relaxed(doen) != GPO_ENABLE;
> >
> > I believe the idea was to return the predefined values for the direction.
>
> You mean this?
>   return readl_relaxed(doen) == GPO_ENABLE ? GPIO_LINE_DIRECTION_OUT :
> GPIO_LINE_DIRECTION_IN;

For example, or with if (...) return _OUT; return _IN;'

> > > +}

...

> > > +       if (trigger & IRQ_TYPE_EDGE_BOTH)
> > > +               irq_set_handler_locked(d, handle_edge_irq);
> > > +       else if (trigger & IRQ_TYPE_LEVEL_MASK)
> > > +               irq_set_handler_locked(d, handle_level_irq);
> >
> > Usually we don't assign this twice, so it should be after the switch.
> >
> > > +       switch (trigger) {

> > > +       default:
> >
> > > +               irq_set_handler_locked(d, handle_bad_irq);
> >
> > Why? You have it already in ->probe(), what's the point?
>
> So last time you asked about this, I explained a situation where
> userspace first grabs a GPIO, set the interrupt to edge triggered, and
> then later loads a driver that requests an unsupported IRQ type.

I didn't get this scenario. Is it real?

> Then
> I'd like to set the handler back to handle_bad_irq so we don't get
> weird interrupts, but maybe now you know a reason why that doesn't
> matter or can't happen?

In ->probe() you set _default_ handler to bad(), what do you mean by
'set the handler back to bad()'? How is it otherwise if you free an
interrupt?

So, please elaborate with call traces what the scenario / use case you
are talking about. If it's true what you are saying, we have a
situation (plenty of GPIO drivers don't do what you are suggesting
here).

> > > +               return -EINVAL;
> > > +       }

...

> > > +       ret = reset_control_deassert(rst);
> > > +       if (ret)
> > > +               return dev_err_probe(dev, ret, "could not deassert resetd\n");
> >
> > > +       ret = devm_pinctrl_register_and_init(dev, &starfive_desc, sfp, &sfp->pctl);
> > > +       if (ret)
> >
> > I don't see who will assert reset here.
>
> No, so originally this driver would first assert and then deassert
> reset. I decided against that because in all likelyhood earlier boot
> stages would have set pinmux up for a serial port, and we don't want
> to interrupt the serial debug output. The only reason I make sure the
> reset line is deasserted is in case someone makes a really minimal
> bootloader that just does the absolute minimal to load a Linux kernel
> and doesn't even log any anything.
>
> By the same token we also don't want to assert reset on error in case
> it resets pin muxing for the the serial line that was supposed to log
> the error.

Perhaps comment in the code explaining this?
Emil Renner Berthing Nov. 3, 2021, 12:35 p.m. UTC | #5
On Wed, 3 Nov 2021 at 10:13, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Tue, Nov 2, 2021 at 10:35 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
> > On Tue, 2 Nov 2021 at 21:02, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
>
> > > > +       switch (trigger) {
>
> > > > +       default:
> > >
> > > > +               irq_set_handler_locked(d, handle_bad_irq);
> > >
> > > Why? You have it already in ->probe(), what's the point?
> >
> > So last time you asked about this, I explained a situation where
> > userspace first grabs a GPIO, set the interrupt to edge triggered, and
> > then later loads a driver that requests an unsupported IRQ type.
>
> I didn't get this scenario. Is it real?

No, it's totally made up, but I mean we even have tools like fuzzing
to help us find bugs that would never happen in real use cases.

> > Then
> > I'd like to set the handler back to handle_bad_irq so we don't get
> > weird interrupts, but maybe now you know a reason why that doesn't
> > matter or can't happen?
>
> In ->probe() you set _default_ handler to bad(), what do you mean by
> 'set the handler back to bad()'? How is it otherwise if you free an
> interrupt?

It might not be, but when not sure I thought it better to error on the
safe side.

> So, please elaborate with call traces what the scenario / use case you
> are talking about. If it's true what you are saying, we have a
> situation (plenty of GPIO drivers don't do what you are suggesting
> here).
>
> > > > +               return -EINVAL;
> > > > +       }
>
> ...
>
> > > > +       ret = reset_control_deassert(rst);
> > > > +       if (ret)
> > > > +               return dev_err_probe(dev, ret, "could not deassert resetd\n");
> > >
> > > > +       ret = devm_pinctrl_register_and_init(dev, &starfive_desc, sfp, &sfp->pctl);
> > > > +       if (ret)
> > >
> > > I don't see who will assert reset here.
> >
> > No, so originally this driver would first assert and then deassert
> > reset. I decided against that because in all likelyhood earlier boot
> > stages would have set pinmux up for a serial port, and we don't want
> > to interrupt the serial debug output. The only reason I make sure the
> > reset line is deasserted is in case someone makes a really minimal
> > bootloader that just does the absolute minimal to load a Linux kernel
> > and doesn't even log any anything.
> >
> > By the same token we also don't want to assert reset on error in case
> > it resets pin muxing for the the serial line that was supposed to log
> > the error.
>
> Perhaps comment in the code explaining this?

Sure.
Andy Shevchenko Nov. 3, 2021, 2:13 p.m. UTC | #6
On Wed, Nov 03, 2021 at 01:35:23PM +0100, Emil Renner Berthing wrote:
> On Wed, 3 Nov 2021 at 10:13, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, Nov 2, 2021 at 10:35 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
> > > On Tue, 2 Nov 2021 at 21:02, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@esmil.dk> wrote:

...

> > > > > +               irq_set_handler_locked(d, handle_bad_irq);
> > > >
> > > > Why? You have it already in ->probe(), what's the point?
> > >
> > > So last time you asked about this, I explained a situation where
> > > userspace first grabs a GPIO, set the interrupt to edge triggered, and
> > > then later loads a driver that requests an unsupported IRQ type.
> >
> > I didn't get this scenario. Is it real?
> 
> No, it's totally made up, but I mean we even have tools like fuzzing
> to help us find bugs that would never happen in real use cases.
> 
> > > Then
> > > I'd like to set the handler back to handle_bad_irq so we don't get
> > > weird interrupts, but maybe now you know a reason why that doesn't
> > > matter or can't happen?
> >
> > In ->probe() you set _default_ handler to bad(), what do you mean by
> > 'set the handler back to bad()'? How is it otherwise if you free an
> > interrupt?
> 
> It might not be, but when not sure I thought it better to error on the
> safe side.

With a dead code?

I do not believe there is an issue since. like I said, there are plenty drivers
that don't do what you are suggesting here --> 99.99% you added a dead code.

> > So, please elaborate with call traces what the scenario / use case you
> > are talking about. If it's true what you are saying, we have a
> > situation (plenty of GPIO drivers don't do what you are suggesting
> > here).
Linus Walleij Nov. 9, 2021, 12:54 a.m. UTC | #7
On Tue, Nov 2, 2021 at 9:02 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> > +       depends on OF
>
> So this descreases test coverage.
> Linus, can we provide a necessary stub so we may drop this dependency?

Hm it further selects OF_GPIO which depends on OF
so I don't know how that would work.

But does it decrease compile coverage a lot, even x86 has
optional OF support so I imagine it appears in x86
allyesconfig I suppose? Or am I wrong?

Yours,
Linus Walleij
Linus Walleij Nov. 9, 2021, 1:01 a.m. UTC | #8
On Tue, Nov 2, 2021 at 9:08 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
(...)
> > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
>
> ...
>
> > > +static int starfive_pinconf_group_set(struct pinctrl_dev *pctldev,
> > > +                                     unsigned int gsel,
> > > +                                     unsigned long *configs,
> > > +                                     unsigned int num_configs)
> > > +{
> > > +       struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev);
> > > +       const struct group_desc *group;
> > > +       u16 mask, value;
> > > +       int i;
> > > +
> > > +       group = pinctrl_generic_get_group(pctldev, gsel);
> > > +       if (!group)
> > > +               return -EINVAL;
> > > +
> > > +       mask = 0;
> > > +       value = 0;
> > > +       for (i = 0; i < num_configs; i++) {
> > > +               int param = pinconf_to_config_param(configs[i]);
> > > +               u32 arg = pinconf_to_config_argument(configs[i]);
> > > +
> > > +               switch (param) {
> > > +               case PIN_CONFIG_BIAS_DISABLE:
> > > +                       mask |= PAD_BIAS_MASK;
> > > +                       value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE;
> > > +                       break;
> > > +               case PIN_CONFIG_BIAS_PULL_DOWN:
> > > +                       if (arg == 0)
> > > +                               return -ENOTSUPP;
> > > +                       mask |= PAD_BIAS_MASK;
> > > +                       value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_PULL_DOWN;
> > > +                       break;
> > > +               case PIN_CONFIG_BIAS_PULL_UP:
> > > +                       if (arg == 0)
> > > +                               return -ENOTSUPP;
> > > +                       mask |= PAD_BIAS_MASK;
> > > +                       value = value & ~PAD_BIAS_MASK;
> > > +                       break;
> > > +               case PIN_CONFIG_DRIVE_STRENGTH:
> > > +                       mask |= PAD_DRIVE_STRENGTH_MASK;
> > > +                       value = (value & ~PAD_DRIVE_STRENGTH_MASK) |
> > > +                               starfive_drive_strength_from_max_mA(arg);
> > > +                       break;
> > > +               case PIN_CONFIG_INPUT_ENABLE:
> > > +                       mask |= PAD_INPUT_ENABLE;
> > > +                       if (arg)
> > > +                               value |= PAD_INPUT_ENABLE;
> > > +                       else
> > > +                               value &= ~PAD_INPUT_ENABLE;
> > > +                       break;
> > > +               case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> > > +                       mask |= PAD_INPUT_SCHMITT_ENABLE;
> > > +                       if (arg)
> > > +                               value |= PAD_INPUT_SCHMITT_ENABLE;
> > > +                       else
> > > +                               value &= ~PAD_INPUT_SCHMITT_ENABLE;
> > > +                       break;
> > > +               case PIN_CONFIG_SLEW_RATE:
> > > +                       mask |= PAD_SLEW_RATE_MASK;
> > > +                       value = (value & ~PAD_SLEW_RATE_MASK) |
> > > +                               ((arg << PAD_SLEW_RATE_POS) & PAD_SLEW_RATE_MASK);
> > > +                       break;
> > > +               case PIN_CONFIG_STARFIVE_STRONG_PULL_UP:
> > > +                       if (arg) {
> > > +                               mask |= PAD_BIAS_MASK;
> > > +                               value = (value & ~PAD_BIAS_MASK) |
> > > +                                       PAD_BIAS_STRONG_PULL_UP;
> > > +                       } else {
> > > +                               mask |= PAD_BIAS_STRONG_PULL_UP;
> > > +                               value = value & ~PAD_BIAS_STRONG_PULL_UP;
> > > +                       }
> > > +                       break;
> > > +               default:
> > > +                       return -ENOTSUPP;
> > > +               }
> > > +       }
> > > +
> > > +       for (i = 0; i < group->num_pins; i++)
> > > +               starfive_padctl_rmw(sfp, group->pins[i], mask, value);
> > > +
> > > +       return 0;
> > > +}
>
> Linus any comments on this code (sorry if I missed your reply)? The
> idea behind above is to skip all settings from the same category and
> apply only the last one, e.g. if we have "bias set to X", ..., "bias
> disable", ..., "bias set to Y", the hardware will see only the last
> operation, i.e. "bias set to Y". I think it may not be the best
> approach (theoretically?) since the hardware definitely may behave
> differently on the other side in case of such series of the
> configurations (yes, I have seen some interesting implementations of
> the touchpad / touchscreen GPIOs that may be affected).

That sounds weird. I think we need to look at how other drivers
deal with this.

To me it seems more natural that
starfive_padctl_rmw(sfp, group->pins[i], mask, value);
would get called at the end of each iteration of the
for (i = 0; i < num_configs; i++) loop.

Yours,
Linus Walleij
Andy Shevchenko Nov. 9, 2021, 8:58 a.m. UTC | #9
On Tue, Nov 9, 2021 at 2:54 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Nov 2, 2021 at 9:02 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>
> > > +       depends on OF
> >
> > So this descreases test coverage.
> > Linus, can we provide a necessary stub so we may drop this dependency?
>
> Hm it further selects OF_GPIO which depends on OF
> so I don't know how that would work.
>
> But does it decrease compile coverage a lot, even x86 has
> optional OF support so I imagine it appears in x86
> allyesconfig I suppose? Or am I wrong?

I believe so. At least in my environment I have OF enabled (I haven't
looked into what was the change to the config, though).
Emil Renner Berthing Nov. 9, 2021, 9:21 a.m. UTC | #10
On Tue, 9 Nov 2021 at 02:01, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Nov 2, 2021 at 9:08 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> (...)
> > > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
> >
> > ...
> >
> > > > +static int starfive_pinconf_group_set(struct pinctrl_dev *pctldev,
> > > > +                                     unsigned int gsel,
> > > > +                                     unsigned long *configs,
> > > > +                                     unsigned int num_configs)
> > > > +{
> > > > +       struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev);
> > > > +       const struct group_desc *group;
> > > > +       u16 mask, value;
> > > > +       int i;
> > > > +
> > > > +       group = pinctrl_generic_get_group(pctldev, gsel);
> > > > +       if (!group)
> > > > +               return -EINVAL;
> > > > +
> > > > +       mask = 0;
> > > > +       value = 0;
> > > > +       for (i = 0; i < num_configs; i++) {
> > > > +               int param = pinconf_to_config_param(configs[i]);
> > > > +               u32 arg = pinconf_to_config_argument(configs[i]);
> > > > +
> > > > +               switch (param) {
> > > > +               case PIN_CONFIG_BIAS_DISABLE:
> > > > +                       mask |= PAD_BIAS_MASK;
> > > > +                       value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE;
> > > > +                       break;
> > > > +               case PIN_CONFIG_BIAS_PULL_DOWN:
> > > > +                       if (arg == 0)
> > > > +                               return -ENOTSUPP;
> > > > +                       mask |= PAD_BIAS_MASK;
> > > > +                       value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_PULL_DOWN;
> > > > +                       break;
> > > > +               case PIN_CONFIG_BIAS_PULL_UP:
> > > > +                       if (arg == 0)
> > > > +                               return -ENOTSUPP;
> > > > +                       mask |= PAD_BIAS_MASK;
> > > > +                       value = value & ~PAD_BIAS_MASK;
> > > > +                       break;
> > > > +               case PIN_CONFIG_DRIVE_STRENGTH:
> > > > +                       mask |= PAD_DRIVE_STRENGTH_MASK;
> > > > +                       value = (value & ~PAD_DRIVE_STRENGTH_MASK) |
> > > > +                               starfive_drive_strength_from_max_mA(arg);
> > > > +                       break;
> > > > +               case PIN_CONFIG_INPUT_ENABLE:
> > > > +                       mask |= PAD_INPUT_ENABLE;
> > > > +                       if (arg)
> > > > +                               value |= PAD_INPUT_ENABLE;
> > > > +                       else
> > > > +                               value &= ~PAD_INPUT_ENABLE;
> > > > +                       break;
> > > > +               case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> > > > +                       mask |= PAD_INPUT_SCHMITT_ENABLE;
> > > > +                       if (arg)
> > > > +                               value |= PAD_INPUT_SCHMITT_ENABLE;
> > > > +                       else
> > > > +                               value &= ~PAD_INPUT_SCHMITT_ENABLE;
> > > > +                       break;
> > > > +               case PIN_CONFIG_SLEW_RATE:
> > > > +                       mask |= PAD_SLEW_RATE_MASK;
> > > > +                       value = (value & ~PAD_SLEW_RATE_MASK) |
> > > > +                               ((arg << PAD_SLEW_RATE_POS) & PAD_SLEW_RATE_MASK);
> > > > +                       break;
> > > > +               case PIN_CONFIG_STARFIVE_STRONG_PULL_UP:
> > > > +                       if (arg) {
> > > > +                               mask |= PAD_BIAS_MASK;
> > > > +                               value = (value & ~PAD_BIAS_MASK) |
> > > > +                                       PAD_BIAS_STRONG_PULL_UP;
> > > > +                       } else {
> > > > +                               mask |= PAD_BIAS_STRONG_PULL_UP;
> > > > +                               value = value & ~PAD_BIAS_STRONG_PULL_UP;
> > > > +                       }
> > > > +                       break;
> > > > +               default:
> > > > +                       return -ENOTSUPP;
> > > > +               }
> > > > +       }
> > > > +
> > > > +       for (i = 0; i < group->num_pins; i++)
> > > > +               starfive_padctl_rmw(sfp, group->pins[i], mask, value);
> > > > +
> > > > +       return 0;
> > > > +}
> >
> > Linus any comments on this code (sorry if I missed your reply)? The
> > idea behind above is to skip all settings from the same category and
> > apply only the last one, e.g. if we have "bias set to X", ..., "bias
> > disable", ..., "bias set to Y", the hardware will see only the last
> > operation, i.e. "bias set to Y". I think it may not be the best
> > approach (theoretically?) since the hardware definitely may behave
> > differently on the other side in case of such series of the
> > configurations (yes, I have seen some interesting implementations of
> > the touchpad / touchscreen GPIOs that may be affected).
>
> That sounds weird. I think we need to look at how other drivers
> deal with this.
>
> To me it seems more natural that
> starfive_padctl_rmw(sfp, group->pins[i], mask, value);
> would get called at the end of each iteration of the
> for (i = 0; i < num_configs; i++) loop.

That would work, but when the loop is done the end result would be
exactly the same. The only difference is that the above would rapidly
"blink" the different states during the loop until it arrives at the
result. This would certainly be different, but it can never be the
intended behaviour and only a side-effect on how the pinctrl framework
works. The order the different states are blinked depends entirely on
how the pinctrl framework parses the device tree. I still think it
would be more natural to cleanly go to the end result without this
blinking.

/Emil
Andy Shevchenko Nov. 9, 2021, 9:33 a.m. UTC | #11
On Tue, Nov 9, 2021 at 11:21 AM Emil Renner Berthing <kernel@esmil.dk> wrote:
> On Tue, 9 Nov 2021 at 02:01, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Tue, Nov 2, 2021 at 9:08 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:

...

> > > Linus any comments on this code (sorry if I missed your reply)? The
> > > idea behind above is to skip all settings from the same category and
> > > apply only the last one, e.g. if we have "bias set to X", ..., "bias
> > > disable", ..., "bias set to Y", the hardware will see only the last
> > > operation, i.e. "bias set to Y". I think it may not be the best
> > > approach (theoretically?) since the hardware definitely may behave
> > > differently on the other side in case of such series of the
> > > configurations (yes, I have seen some interesting implementations of
> > > the touchpad / touchscreen GPIOs that may be affected).
> >
> > That sounds weird. I think we need to look at how other drivers
> > deal with this.
> >
> > To me it seems more natural that
> > starfive_padctl_rmw(sfp, group->pins[i], mask, value);
> > would get called at the end of each iteration of the
> > for (i = 0; i < num_configs; i++) loop.
>
> That would work, but when the loop is done the end result would be
> exactly the same.

It seems we interpret the term "result" differently. The result when
we talking about GPIOs is the series of pin state changes incl.
configuration. This is how it should be recognized when programming
hardware.

>  The only difference is that the above would rapidly
> "blink" the different states during the loop until it arrives at the
> result. This would certainly be different, but it can never be the
> intended behaviour and only a side-effect on how the pinctrl framework
> works.

Is it? That's what I'm trying to get an answer to. If you may
guarantee this (the keywords "intended behaviour" and "side effect"),
I wouldn't object.

> The order the different states are blinked depends entirely on
> how the pinctrl framework parses the device tree. I still think it
> would be more natural to cleanly go to the end result without this
> blinking.
Emil Renner Berthing Nov. 9, 2021, 9:40 a.m. UTC | #12
On Tue, 9 Nov 2021 at 10:34, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Tue, Nov 9, 2021 at 11:21 AM Emil Renner Berthing <kernel@esmil.dk> wrote:
> > On Tue, 9 Nov 2021 at 02:01, Linus Walleij <linus.walleij@linaro.org> wrote:
> > > On Tue, Nov 2, 2021 at 9:08 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
>
> ...
>
> > > > Linus any comments on this code (sorry if I missed your reply)? The
> > > > idea behind above is to skip all settings from the same category and
> > > > apply only the last one, e.g. if we have "bias set to X", ..., "bias
> > > > disable", ..., "bias set to Y", the hardware will see only the last
> > > > operation, i.e. "bias set to Y". I think it may not be the best
> > > > approach (theoretically?) since the hardware definitely may behave
> > > > differently on the other side in case of such series of the
> > > > configurations (yes, I have seen some interesting implementations of
> > > > the touchpad / touchscreen GPIOs that may be affected).
> > >
> > > That sounds weird. I think we need to look at how other drivers
> > > deal with this.
> > >
> > > To me it seems more natural that
> > > starfive_padctl_rmw(sfp, group->pins[i], mask, value);
> > > would get called at the end of each iteration of the
> > > for (i = 0; i < num_configs; i++) loop.
> >
> > That would work, but when the loop is done the end result would be
> > exactly the same.
>
> It seems we interpret the term "result" differently. The result when
> we talking about GPIOs is the series of pin state changes incl.
> configuration. This is how it should be recognized when programming
> hardware.
>
> >  The only difference is that the above would rapidly
> > "blink" the different states during the loop until it arrives at the
> > result. This would certainly be different, but it can never be the
> > intended behaviour and only a side-effect on how the pinctrl framework
> > works.
>
> Is it? That's what I'm trying to get an answer to. If you may
> guarantee this (the keywords "intended behaviour" and "side effect"),
> I wouldn't object.
>
> > The order the different states are blinked depends entirely on
> > how the pinctrl framework parses the device tree. I still think it
> > would be more natural to cleanly go to the end result without this
> > blinking.

Hmm.. but if going through the different states is what you want, then
wouldn't you need the device tree to have an ordered list of the
states rather than just a single node and also a way to tune how long
time the different states are blinked?
Linus Walleij Nov. 9, 2021, 8:29 p.m. UTC | #13
On Tue, Nov 9, 2021 at 10:40 AM Emil Renner Berthing <kernel@esmil.dk> wrote:
> On Tue, 9 Nov 2021 at 10:34, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> > > The order the different states are blinked depends entirely on
> > > how the pinctrl framework parses the device tree. I still think it
> > > would be more natural to cleanly go to the end result without this
> > > blinking.
>
> Hmm.. but if going through the different states is what you want, then
> wouldn't you need the device tree to have an ordered list of the
> states rather than just a single node and also a way to tune how long
> time the different states are blinked?

In a way you are correct that the DT is a functional language and it's
a bit lite a style sheet or prolog or something in that the end reduction
is what counts.

In this case, I would say something is weird if there are interim states,
the yaml validation should not allow you to set the same thing back
and forth in your DTS file.

Alas we are not perfect as in yaml validation isn't perfect either.
I can't see what the problem is really, just write proper DTS files
and there will not be any interim states, right? And if it is possible
to write DTS files that have states and sequence requirements,
these should be caught in validation. Should be.

Yours,
Linus Walleij
Emil Renner Berthing Nov. 9, 2021, 9:04 p.m. UTC | #14
On Tue, 9 Nov 2021 at 21:29, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Nov 9, 2021 at 10:40 AM Emil Renner Berthing <kernel@esmil.dk> wrote:
> > On Tue, 9 Nov 2021 at 10:34, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> > > > The order the different states are blinked depends entirely on
> > > > how the pinctrl framework parses the device tree. I still think it
> > > > would be more natural to cleanly go to the end result without this
> > > > blinking.
> >
> > Hmm.. but if going through the different states is what you want, then
> > wouldn't you need the device tree to have an ordered list of the
> > states rather than just a single node and also a way to tune how long
> > time the different states are blinked?
>
> In a way you are correct that the DT is a functional language and it's
> a bit lite a style sheet or prolog or something in that the end reduction
> is what counts.
>
> In this case, I would say something is weird if there are interim states,
> the yaml validation should not allow you to set the same thing back
> and forth in your DTS file.

Yes, exactly.

> Alas we are not perfect as in yaml validation isn't perfect either.
> I can't see what the problem is really, just write proper DTS files
> and there will not be any interim states, right?

No, I agree. I think it's only that Andy wasn't sure if these interim
states might be meaningful/useful.

> And if it is possible
> to write DTS files that have states and sequence requirements,
> these should be caught in validation. Should be.
>
> Yours,
> Linus Walleij
Andy Shevchenko Nov. 10, 2021, 8:04 a.m. UTC | #15
On Tue, Nov 09, 2021 at 10:04:24PM +0100, Emil Renner Berthing wrote:
> On Tue, 9 Nov 2021 at 21:29, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Tue, Nov 9, 2021 at 10:40 AM Emil Renner Berthing <kernel@esmil.dk> wrote:

...

> No, I agree. I think it's only that Andy wasn't sure if these interim
> states might be meaningful/useful.

Exactly. Because HW could behave differently.

> > And if it is possible
> > to write DTS files that have states and sequence requirements,
> > these should be caught in validation. Should be.
Emil Renner Berthing Nov. 10, 2021, 11:15 a.m. UTC | #16
On Wed, 10 Nov 2021 at 09:05, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Tue, Nov 09, 2021 at 10:04:24PM +0100, Emil Renner Berthing wrote:
> > On Tue, 9 Nov 2021 at 21:29, Linus Walleij <linus.walleij@linaro.org> wrote:
> > > On Tue, Nov 9, 2021 at 10:40 AM Emil Renner Berthing <kernel@esmil.dk> wrote:
>
> ...
>
> > No, I agree. I think it's only that Andy wasn't sure if these interim
> > states might be meaningful/useful.
>
> Exactly. Because HW could behave differently.

Right. But I think we've now established that what is described in the
device tree is the state the pins should be in after the function has
been called, eg. only the reduction matters, and any interim states
would just be a byproduct of storing the state in the configs list.

> > > And if it is possible
> > > to write DTS files that have states and sequence requirements,
> > > these should be caught in validation. Should be.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 8274fa4b8430..73c987480a44 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17866,6 +17866,14 @@  F:	Documentation/devicetree/bindings/clock/starfive,jh7100-clkgen.yaml
 F:	drivers/clk/starfive/clk-starfive-jh7100.c
 F:	include/dt-bindings/clock/starfive-jh7100.h
 
+STARFIVE JH7100 PINCTRL DRIVER
+M:	Emil Renner Berthing <kernel@esmil.dk>
+L:	linux-gpio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/pinctrl/starfive,jh7100-pinctrl.yaml
+F:	drivers/pinctrl/pinctrl-starfive.c
+F:	include/dt-bindings/pinctrl/pinctrl-starfive.h
+
 STARFIVE JH7100 RESET CONTROLLER DRIVER
 M:	Emil Renner Berthing <kernel@esmil.dk>
 S:	Maintained
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 31921108e456..b298cf32804a 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -265,6 +265,23 @@  config PINCTRL_ST
 	select PINCONF
 	select GPIOLIB_IRQCHIP
 
+config PINCTRL_STARFIVE
+	tristate "Pinctrl and GPIO driver for the StarFive JH7100 SoC"
+	depends on SOC_STARFIVE || COMPILE_TEST
+	depends on OF
+	default SOC_STARFIVE
+	select GENERIC_PINCTRL_GROUPS
+	select GENERIC_PINMUX_FUNCTIONS
+	select GENERIC_PINCONF
+	select GPIOLIB
+	select GPIOLIB_IRQCHIP
+	select OF_GPIO
+	help
+	  Say yes here to support pin control on the StarFive JH7100 SoC.
+	  This also provides an interface to the GPIO pins not used by other
+	  peripherals supporting inputs, outputs, configuring pull-up/pull-down
+	  and interrupts on input changes.
+
 config PINCTRL_STMFX
 	tristate "STMicroelectronics STMFX GPIO expander pinctrl driver"
 	depends on I2C
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 200073bcc2c1..9c258047f11c 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -38,6 +38,7 @@  obj-$(CONFIG_PINCTRL_LANTIQ)	+= pinctrl-lantiq.o
 obj-$(CONFIG_PINCTRL_LPC18XX)	+= pinctrl-lpc18xx.o
 obj-$(CONFIG_PINCTRL_TB10X)	+= pinctrl-tb10x.o
 obj-$(CONFIG_PINCTRL_ST) 	+= pinctrl-st.o
+obj-$(CONFIG_PINCTRL_STARFIVE)	+= pinctrl-starfive.o
 obj-$(CONFIG_PINCTRL_STMFX) 	+= pinctrl-stmfx.o
 obj-$(CONFIG_PINCTRL_ZYNQ)	+= pinctrl-zynq.o
 obj-$(CONFIG_PINCTRL_ZYNQMP)	+= pinctrl-zynqmp.o
diff --git a/drivers/pinctrl/pinctrl-starfive.c b/drivers/pinctrl/pinctrl-starfive.c
new file mode 100644
index 000000000000..15e5a672ffff
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-starfive.c
@@ -0,0 +1,1353 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Pinctrl / GPIO driver for StarFive JH7100 SoC
+ *
+ * Copyright (C) 2020 Shanghai StarFive Technology Co., Ltd.
+ * Copyright (C) 2021 Emil Renner Berthing <kernel@esmil.dk>
+ */
+
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/gpio/driver.h>
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/spinlock.h>
+
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include <dt-bindings/pinctrl/pinctrl-starfive.h>
+
+#include "core.h"
+#include "pinctrl-utils.h"
+#include "pinmux.h"
+#include "pinconf.h"
+
+#define DRIVER_NAME "pinctrl-starfive"
+
+/*
+ * Refer to Section 12. GPIO Registers in the JH7100 data sheet:
+ * https://github.com/starfive-tech/JH7100_Docs
+ */
+#define NR_GPIOS	64
+
+/*
+ * Global enable for GPIO interrupts. If bit 0 is set to 1 the GPIO interrupts
+ * are enabled. If set to 0 the GPIO interrupts are disabled.
+ */
+#define GPIOEN		0x000
+
+/*
+ * The following 32-bit registers come in pairs, but only the offset of the
+ * first register is defined. The first controls (interrupts for) GPIO 0-31 and
+ * the second GPIO 32-63.
+ */
+
+/*
+ * Interrupt Type. If set to 1 the interrupt is edge-triggered. If set to 0 the
+ * interrupt is level-triggered.
+ */
+#define GPIOIS		0x010
+
+/*
+ * Edge-Trigger Interrupt Type.  If set to 1 the interrupt gets triggered on
+ * both positive and negative edges. If set to 0 the interrupt is triggered by a
+ * single edge.
+ */
+#define GPIOIBE		0x018
+
+/*
+ * Interrupt Trigger Polarity. If set to 1 the interrupt is triggered on a
+ * rising edge (edge-triggered) or high level (level-triggered). If set to 0 the
+ * interrupt is triggered on a falling edge (edge-triggered) or low level
+ * (level-triggered).
+ */
+#define GPIOIEV		0x020
+
+/*
+ * Interrupt Mask. If set to 1 the interrupt is enabled (unmasked). If set to 0
+ * the interrupt is disabled (masked). Note that the current documentation is
+ * wrong and says the exct opposite of this.
+ */
+#define GPIOIE		0x028
+
+/*
+ * Clear Edge-Triggered Interrupts. Write a 1 to clear the edge-triggered
+ * interrupt.
+ */
+#define GPIOIC		0x030
+
+/*
+ * Edge-Triggered Interrupt Status. A 1 means the configured edge was detected.
+ */
+#define GPIORIS		0x038
+
+/*
+ * Interrupt Status after Masking. A 1 means the configured edge or level was
+ * detected and not masked.
+ */
+#define GPIOMIS		0x040
+
+/*
+ * Data Value. Dynamically reflects the value of the GPIO pin. If 1 the pin is
+ * a digital 1 and if 0 the pin is a digital 0.
+ */
+#define GPIODIN		0x048
+
+/*
+ * From the data sheet section 12.2, there are 64 32-bit output data registers
+ * and 64 output enable registers. Output data and output enable registers for
+ * a given GPIO are contiguous. Eg. GPO0_DOUT_CFG is 0x50 and GPO0_DOEN_CFG is
+ * 0x54 while GPO1_DOUT_CFG is 0x58 and GPO1_DOEN_CFG is 0x5c.  The stride
+ * between GPIO registers is effectively 8, thus: GPOn_DOUT_CFG is 0x50 + 8n
+ * and GPOn_DOEN_CFG is 0x54 + 8n.
+ */
+#define GPON_DOUT_CFG	0x050
+#define GPON_DOEN_CFG	0x054
+
+/*
+ * From Section 12.3, there are 75 input signal configuration registers which
+ * are 4 bytes wide starting with GPI_CPU_JTAG_TCK_CFG at 0x250 and ending with
+ * GPI_USB_OVER_CURRENT_CFG 0x378
+ */
+#define GPI_CFG_OFFSET	0x250
+
+/*
+ * Pad Control Bits. There are 16 pad control bits for each pin located in 103
+ * 32-bit registers controlling PAD_GPIO[0] to PAD_GPIO[63] followed by
+ * PAD_FUNC_SHARE[0] to PAD_FUNC_SHARE[141]. Odd numbered pins use the upper 16
+ * bit of each register.
+ */
+#define PAD_SLEW_RATE_MASK		GENMASK(11, 9)
+#define PAD_SLEW_RATE_POS		9
+#define PAD_BIAS_STRONG_PULL_UP		BIT(8)
+#define PAD_INPUT_ENABLE		BIT(7)
+#define PAD_INPUT_SCHMITT_ENABLE	BIT(6)
+#define PAD_BIAS_DISABLE		BIT(5)
+#define PAD_BIAS_PULL_DOWN		BIT(4)
+#define PAD_BIAS_MASK \
+	(PAD_BIAS_STRONG_PULL_UP | \
+	 PAD_BIAS_DISABLE | \
+	 PAD_BIAS_PULL_DOWN)
+#define PAD_DRIVE_STRENGTH_MASK		GENMASK(3, 0)
+#define PAD_DRIVE_STRENGTH_POS		0
+
+/*
+ * From Section 11, the IO_PADSHARE_SEL register can be programmed to select
+ * one of seven pre-defined multiplexed signal groups on PAD_FUNC_SHARE and
+ * PAD_GPIO pads. This is a global setting.
+ */
+#define IO_PADSHARE_SEL			0x1a0
+
+/*
+ * This just needs to be some number such that when
+ * sfp->gpio.pin_base = PAD_INVALID_GPIO then
+ * starfive_pin_to_gpio(sfp, validpin) is never a valid GPIO number.
+ * That is it should underflow and return something >= NR_GPIOS.
+ */
+#define PAD_INVALID_GPIO		0x10000
+
+/*
+ * The packed pinmux values from the device tree look like this:
+ *
+ *  | 31 - 24 | 23 - 16 | 15 - 8 |     7    |     6    |  5 - 0  |
+ *  |  dout   |  doen   |  din   | dout rev | doen rev | gpio nr |
+ *
+ * ..but the GPOn_DOUT_CFG and GPOn_DOEN_CFG registers look like this:
+ *
+ *  |      31       | 30 - 8 |   7 - 0   |
+ *  | dout/doen rev | unused | dout/doen |
+ */
+static unsigned int starfive_pinmux_to_gpio(u32 v)
+{
+	return v & (NR_GPIOS - 1);
+}
+
+static u32 starfive_pinmux_to_dout(u32 v)
+{
+	return ((v & BIT(7)) << (31 - 7)) | ((v >> 24) & GENMASK(7, 0));
+}
+
+static u32 starfive_pinmux_to_doen(u32 v)
+{
+	return ((v & BIT(6)) << (31 - 6)) | ((v >> 16) & GENMASK(7, 0));
+}
+
+static u32 starfive_pinmux_to_din(u32 v)
+{
+	return (v >> 8) & GENMASK(7, 0);
+}
+
+/*
+ * The maximum GPIO output current depends on the chosen drive strength:
+ *
+ *  DS:   0     1     2     3     4     5     6     7
+ *  mA:  14.2  21.2  28.2  35.2  42.2  49.1  56.0  62.8
+ *
+ * After rounding that is 7*DS + 14 mA
+ */
+static u32 starfive_drive_strength_to_max_mA(u16 ds)
+{
+	return 7 * ds + 14;
+}
+
+static u16 starfive_drive_strength_from_max_mA(u32 i)
+{
+	return (clamp(i, 14U, 63U) - 14) / 7;
+}
+
+struct starfive_pinctrl {
+	struct gpio_chip gc;
+	struct pinctrl_gpio_range gpios;
+	raw_spinlock_t lock;
+	void __iomem *base;
+	void __iomem *padctl;
+	struct pinctrl_dev *pctl;
+};
+
+static inline struct device *starfive_dev(const struct starfive_pinctrl *sfp)
+{
+	return sfp->gc.parent;
+}
+
+static inline unsigned int starfive_pin_to_gpio(const struct starfive_pinctrl *sfp,
+						unsigned int pin)
+{
+	return pin - sfp->gpios.pin_base;
+}
+
+static inline unsigned int starfive_gpio_to_pin(const struct starfive_pinctrl *sfp,
+						unsigned int gpio)
+{
+	return sfp->gpios.pin_base + gpio;
+}
+
+static struct starfive_pinctrl *starfive_from_irq_data(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+	return container_of(gc, struct starfive_pinctrl, gc);
+}
+
+static struct starfive_pinctrl *starfive_from_irq_desc(struct irq_desc *desc)
+{
+	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+
+	return container_of(gc, struct starfive_pinctrl, gc);
+}
+
+static const struct pinctrl_pin_desc starfive_pins[] = {
+	PINCTRL_PIN(PAD_GPIO(0), "GPIO[0]"),
+	PINCTRL_PIN(PAD_GPIO(1), "GPIO[1]"),
+	PINCTRL_PIN(PAD_GPIO(2), "GPIO[2]"),
+	PINCTRL_PIN(PAD_GPIO(3), "GPIO[3]"),
+	PINCTRL_PIN(PAD_GPIO(4), "GPIO[4]"),
+	PINCTRL_PIN(PAD_GPIO(5), "GPIO[5]"),
+	PINCTRL_PIN(PAD_GPIO(6), "GPIO[6]"),
+	PINCTRL_PIN(PAD_GPIO(7), "GPIO[7]"),
+	PINCTRL_PIN(PAD_GPIO(8), "GPIO[8]"),
+	PINCTRL_PIN(PAD_GPIO(9), "GPIO[9]"),
+	PINCTRL_PIN(PAD_GPIO(10), "GPIO[10]"),
+	PINCTRL_PIN(PAD_GPIO(11), "GPIO[11]"),
+	PINCTRL_PIN(PAD_GPIO(12), "GPIO[12]"),
+	PINCTRL_PIN(PAD_GPIO(13), "GPIO[13]"),
+	PINCTRL_PIN(PAD_GPIO(14), "GPIO[14]"),
+	PINCTRL_PIN(PAD_GPIO(15), "GPIO[15]"),
+	PINCTRL_PIN(PAD_GPIO(16), "GPIO[16]"),
+	PINCTRL_PIN(PAD_GPIO(17), "GPIO[17]"),
+	PINCTRL_PIN(PAD_GPIO(18), "GPIO[18]"),
+	PINCTRL_PIN(PAD_GPIO(19), "GPIO[19]"),
+	PINCTRL_PIN(PAD_GPIO(20), "GPIO[20]"),
+	PINCTRL_PIN(PAD_GPIO(21), "GPIO[21]"),
+	PINCTRL_PIN(PAD_GPIO(22), "GPIO[22]"),
+	PINCTRL_PIN(PAD_GPIO(23), "GPIO[23]"),
+	PINCTRL_PIN(PAD_GPIO(24), "GPIO[24]"),
+	PINCTRL_PIN(PAD_GPIO(25), "GPIO[25]"),
+	PINCTRL_PIN(PAD_GPIO(26), "GPIO[26]"),
+	PINCTRL_PIN(PAD_GPIO(27), "GPIO[27]"),
+	PINCTRL_PIN(PAD_GPIO(28), "GPIO[28]"),
+	PINCTRL_PIN(PAD_GPIO(29), "GPIO[29]"),
+	PINCTRL_PIN(PAD_GPIO(30), "GPIO[30]"),
+	PINCTRL_PIN(PAD_GPIO(31), "GPIO[31]"),
+	PINCTRL_PIN(PAD_GPIO(32), "GPIO[32]"),
+	PINCTRL_PIN(PAD_GPIO(33), "GPIO[33]"),
+	PINCTRL_PIN(PAD_GPIO(34), "GPIO[34]"),
+	PINCTRL_PIN(PAD_GPIO(35), "GPIO[35]"),
+	PINCTRL_PIN(PAD_GPIO(36), "GPIO[36]"),
+	PINCTRL_PIN(PAD_GPIO(37), "GPIO[37]"),
+	PINCTRL_PIN(PAD_GPIO(38), "GPIO[38]"),
+	PINCTRL_PIN(PAD_GPIO(39), "GPIO[39]"),
+	PINCTRL_PIN(PAD_GPIO(40), "GPIO[40]"),
+	PINCTRL_PIN(PAD_GPIO(41), "GPIO[41]"),
+	PINCTRL_PIN(PAD_GPIO(42), "GPIO[42]"),
+	PINCTRL_PIN(PAD_GPIO(43), "GPIO[43]"),
+	PINCTRL_PIN(PAD_GPIO(44), "GPIO[44]"),
+	PINCTRL_PIN(PAD_GPIO(45), "GPIO[45]"),
+	PINCTRL_PIN(PAD_GPIO(46), "GPIO[46]"),
+	PINCTRL_PIN(PAD_GPIO(47), "GPIO[47]"),
+	PINCTRL_PIN(PAD_GPIO(48), "GPIO[48]"),
+	PINCTRL_PIN(PAD_GPIO(49), "GPIO[49]"),
+	PINCTRL_PIN(PAD_GPIO(50), "GPIO[50]"),
+	PINCTRL_PIN(PAD_GPIO(51), "GPIO[51]"),
+	PINCTRL_PIN(PAD_GPIO(52), "GPIO[52]"),
+	PINCTRL_PIN(PAD_GPIO(53), "GPIO[53]"),
+	PINCTRL_PIN(PAD_GPIO(54), "GPIO[54]"),
+	PINCTRL_PIN(PAD_GPIO(55), "GPIO[55]"),
+	PINCTRL_PIN(PAD_GPIO(56), "GPIO[56]"),
+	PINCTRL_PIN(PAD_GPIO(57), "GPIO[57]"),
+	PINCTRL_PIN(PAD_GPIO(58), "GPIO[58]"),
+	PINCTRL_PIN(PAD_GPIO(59), "GPIO[59]"),
+	PINCTRL_PIN(PAD_GPIO(60), "GPIO[60]"),
+	PINCTRL_PIN(PAD_GPIO(61), "GPIO[61]"),
+	PINCTRL_PIN(PAD_GPIO(62), "GPIO[62]"),
+	PINCTRL_PIN(PAD_GPIO(63), "GPIO[63]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(0), "FUNC_SHARE[0]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(1), "FUNC_SHARE[1]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(2), "FUNC_SHARE[2]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(3), "FUNC_SHARE[3]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(4), "FUNC_SHARE[4]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(5), "FUNC_SHARE[5]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(6), "FUNC_SHARE[6]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(7), "FUNC_SHARE[7]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(8), "FUNC_SHARE[8]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(9), "FUNC_SHARE[9]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(10), "FUNC_SHARE[10]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(11), "FUNC_SHARE[11]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(12), "FUNC_SHARE[12]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(13), "FUNC_SHARE[13]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(14), "FUNC_SHARE[14]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(15), "FUNC_SHARE[15]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(16), "FUNC_SHARE[16]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(17), "FUNC_SHARE[17]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(18), "FUNC_SHARE[18]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(19), "FUNC_SHARE[19]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(20), "FUNC_SHARE[20]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(21), "FUNC_SHARE[21]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(22), "FUNC_SHARE[22]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(23), "FUNC_SHARE[23]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(24), "FUNC_SHARE[24]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(25), "FUNC_SHARE[25]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(26), "FUNC_SHARE[26]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(27), "FUNC_SHARE[27]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(28), "FUNC_SHARE[28]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(29), "FUNC_SHARE[29]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(30), "FUNC_SHARE[30]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(31), "FUNC_SHARE[31]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(32), "FUNC_SHARE[32]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(33), "FUNC_SHARE[33]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(34), "FUNC_SHARE[34]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(35), "FUNC_SHARE[35]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(36), "FUNC_SHARE[36]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(37), "FUNC_SHARE[37]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(38), "FUNC_SHARE[38]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(39), "FUNC_SHARE[39]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(40), "FUNC_SHARE[40]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(41), "FUNC_SHARE[41]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(42), "FUNC_SHARE[42]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(43), "FUNC_SHARE[43]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(44), "FUNC_SHARE[44]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(45), "FUNC_SHARE[45]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(46), "FUNC_SHARE[46]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(47), "FUNC_SHARE[47]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(48), "FUNC_SHARE[48]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(49), "FUNC_SHARE[49]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(50), "FUNC_SHARE[50]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(51), "FUNC_SHARE[51]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(52), "FUNC_SHARE[52]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(53), "FUNC_SHARE[53]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(54), "FUNC_SHARE[54]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(55), "FUNC_SHARE[55]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(56), "FUNC_SHARE[56]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(57), "FUNC_SHARE[57]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(58), "FUNC_SHARE[58]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(59), "FUNC_SHARE[59]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(60), "FUNC_SHARE[60]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(61), "FUNC_SHARE[61]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(62), "FUNC_SHARE[62]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(63), "FUNC_SHARE[63]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(64), "FUNC_SHARE[64]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(65), "FUNC_SHARE[65]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(66), "FUNC_SHARE[66]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(67), "FUNC_SHARE[67]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(68), "FUNC_SHARE[68]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(69), "FUNC_SHARE[69]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(70), "FUNC_SHARE[70]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(71), "FUNC_SHARE[71]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(72), "FUNC_SHARE[72]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(73), "FUNC_SHARE[73]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(74), "FUNC_SHARE[74]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(75), "FUNC_SHARE[75]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(76), "FUNC_SHARE[76]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(77), "FUNC_SHARE[77]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(78), "FUNC_SHARE[78]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(79), "FUNC_SHARE[79]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(80), "FUNC_SHARE[80]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(81), "FUNC_SHARE[81]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(82), "FUNC_SHARE[82]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(83), "FUNC_SHARE[83]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(84), "FUNC_SHARE[84]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(85), "FUNC_SHARE[85]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(86), "FUNC_SHARE[86]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(87), "FUNC_SHARE[87]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(88), "FUNC_SHARE[88]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(89), "FUNC_SHARE[89]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(90), "FUNC_SHARE[90]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(91), "FUNC_SHARE[91]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(92), "FUNC_SHARE[92]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(93), "FUNC_SHARE[93]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(94), "FUNC_SHARE[94]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(95), "FUNC_SHARE[95]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(96), "FUNC_SHARE[96]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(97), "FUNC_SHARE[97]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(98), "FUNC_SHARE[98]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(99), "FUNC_SHARE[99]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(100), "FUNC_SHARE[100]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(101), "FUNC_SHARE[101]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(102), "FUNC_SHARE[102]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(103), "FUNC_SHARE[103]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(104), "FUNC_SHARE[104]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(105), "FUNC_SHARE[105]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(106), "FUNC_SHARE[106]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(107), "FUNC_SHARE[107]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(108), "FUNC_SHARE[108]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(109), "FUNC_SHARE[109]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(110), "FUNC_SHARE[110]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(111), "FUNC_SHARE[111]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(112), "FUNC_SHARE[112]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(113), "FUNC_SHARE[113]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(114), "FUNC_SHARE[114]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(115), "FUNC_SHARE[115]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(116), "FUNC_SHARE[116]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(117), "FUNC_SHARE[117]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(118), "FUNC_SHARE[118]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(119), "FUNC_SHARE[119]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(120), "FUNC_SHARE[120]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(121), "FUNC_SHARE[121]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(122), "FUNC_SHARE[122]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(123), "FUNC_SHARE[123]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(124), "FUNC_SHARE[124]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(125), "FUNC_SHARE[125]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(126), "FUNC_SHARE[126]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(127), "FUNC_SHARE[127]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(128), "FUNC_SHARE[128]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(129), "FUNC_SHARE[129]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(130), "FUNC_SHARE[130]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(131), "FUNC_SHARE[131]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(132), "FUNC_SHARE[132]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(133), "FUNC_SHARE[133]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(134), "FUNC_SHARE[134]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(135), "FUNC_SHARE[135]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(136), "FUNC_SHARE[136]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(137), "FUNC_SHARE[137]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(138), "FUNC_SHARE[138]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(139), "FUNC_SHARE[139]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(140), "FUNC_SHARE[140]"),
+	PINCTRL_PIN(PAD_FUNC_SHARE(141), "FUNC_SHARE[141]"),
+};
+
+#ifdef CONFIG_DEBUG_FS
+static void starfive_pin_dbg_show(struct pinctrl_dev *pctldev,
+				  struct seq_file *s,
+				  unsigned int pin)
+{
+	struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev);
+	unsigned int gpio = starfive_pin_to_gpio(sfp, pin);
+	void __iomem *reg;
+	u32 dout, doen;
+
+	if (gpio >= NR_GPIOS)
+		return;
+
+	reg = sfp->base + GPON_DOUT_CFG + 8 * gpio;
+	dout = readl_relaxed(reg + 0x000);
+	doen = readl_relaxed(reg + 0x004);
+
+	seq_printf(s, "dout=%lu%s doen=%lu%s",
+		   dout & GENMASK(7, 0), (dout & BIT(31)) ? "r" : "",
+		   doen & GENMASK(7, 0), (doen & BIT(31)) ? "r" : "");
+}
+#else
+#define starfive_pin_dbg_show NULL
+#endif
+
+static int starfive_dt_node_to_map(struct pinctrl_dev *pctldev,
+				   struct device_node *np,
+				   struct pinctrl_map **maps,
+				   unsigned int *num_maps)
+{
+	struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev);
+	struct device *dev = starfive_dev(sfp);
+	const char **pgnames;
+	struct pinctrl_map *map;
+	struct device_node *child;
+	const char *grpname;
+	int *pins;
+	u32 *pinmux;
+	int nmaps;
+	int ngroups;
+	int ret;
+
+	nmaps = 0;
+	ngroups = 0;
+	for_each_child_of_node(np, child) {
+		int npinmux = of_property_count_u32_elems(child, "pinmux");
+		int npins   = of_property_count_u32_elems(child, "pins");
+
+		if (npinmux > 0 && npins > 0) {
+			dev_err(dev, "invalid pinctrl group %pOFn.%pOFn: both pinmux and pins set\n",
+				np, child);
+			of_node_put(child);
+			return -EINVAL;
+		}
+		if (npinmux == 0 && npins == 0) {
+			dev_err(dev, "invalid pinctrl group %pOFn.%pOFn: neither pinmux nor pins set\n",
+				np, child);
+			of_node_put(child);
+			return -EINVAL;
+		}
+
+		if (npinmux > 0)
+			nmaps += 2;
+		else
+			nmaps += 1;
+		ngroups += 1;
+	}
+
+	pgnames = devm_kcalloc(dev, ngroups, sizeof(*pgnames), GFP_KERNEL);
+	if (!pgnames)
+		return -ENOMEM;
+
+	map = kcalloc(nmaps, sizeof(*map), GFP_KERNEL);
+	if (!map)
+		return -ENOMEM;
+
+	nmaps = 0;
+	ngroups = 0;
+	for_each_child_of_node(np, child) {
+		int npins;
+		int i;
+
+		grpname = devm_kasprintf(dev, GFP_KERNEL, "%pOFn.%pOFn", np, child);
+		if (!grpname) {
+			ret = -ENOMEM;
+			goto put_child;
+		}
+
+		pgnames[ngroups++] = grpname;
+
+		if ((npins = of_property_count_u32_elems(child, "pinmux")) > 0) {
+			pins = devm_kcalloc(dev, npins, sizeof(*pins), GFP_KERNEL);
+			if (!pins) {
+				ret = -ENOMEM;
+				goto put_child;
+			}
+
+			pinmux = devm_kcalloc(dev, npins, sizeof(*pinmux), GFP_KERNEL);
+			if (!pinmux) {
+				ret = -ENOMEM;
+				goto put_child;
+			}
+
+			ret = of_property_read_u32_array(child, "pinmux", pinmux, npins);
+			if (ret)
+				goto put_child;
+
+			for (i = 0; i < npins; i++) {
+				unsigned int gpio = starfive_pinmux_to_gpio(pinmux[i]);
+
+				pins[i] = starfive_gpio_to_pin(sfp, gpio);
+			}
+
+			map[nmaps].type = PIN_MAP_TYPE_MUX_GROUP;
+			map[nmaps].data.mux.function = np->name;
+			map[nmaps].data.mux.group = grpname;
+			nmaps += 1;
+		} else if ((npins = of_property_count_u32_elems(child, "pins")) > 0) {
+			pins = devm_kcalloc(dev, npins, sizeof(*pins), GFP_KERNEL);
+			if (!pins) {
+				ret = -ENOMEM;
+				goto put_child;
+			}
+
+			pinmux = NULL;
+
+			for (i = 0; i < npins; i++) {
+				u32 v;
+
+				ret = of_property_read_u32_index(child, "pins", i, &v);
+				if (ret)
+					goto put_child;
+				pins[i] = v;
+			}
+		} else {
+			ret = -EINVAL;
+			goto put_child;
+		}
+
+		ret = pinctrl_generic_add_group(pctldev, grpname, pins, npins, pinmux);
+		if (ret < 0) {
+			dev_err(dev, "error adding group %s: %d\n", grpname, ret);
+			goto put_child;
+		}
+
+		ret = pinconf_generic_parse_dt_config(child, pctldev,
+						      &map[nmaps].data.configs.configs,
+						      &map[nmaps].data.configs.num_configs);
+		if (ret) {
+			dev_err(dev, "error parsing pin config of group %s: %d\n",
+				grpname, ret);
+			goto put_child;
+		}
+
+		/* don't create a map if there are no pinconf settings */
+		if (map[nmaps].data.configs.num_configs == 0)
+			continue;
+
+		map[nmaps].type = PIN_MAP_TYPE_CONFIGS_GROUP;
+		map[nmaps].data.configs.group_or_pin = grpname;
+		nmaps += 1;
+	}
+
+	ret = pinmux_generic_add_function(pctldev, np->name, pgnames, ngroups, NULL);
+	if (ret < 0) {
+		dev_err(dev, "error adding function %s: %d\n", np->name, ret);
+		goto free_map;
+	}
+
+	*maps = map;
+	*num_maps = nmaps;
+	return 0;
+
+put_child:
+	of_node_put(child);
+free_map:
+	pinctrl_utils_free_map(pctldev, map, nmaps);
+	return ret;
+}
+
+static const struct pinctrl_ops starfive_pinctrl_ops = {
+	.get_groups_count = pinctrl_generic_get_group_count,
+	.get_group_name = pinctrl_generic_get_group_name,
+	.get_group_pins = pinctrl_generic_get_group_pins,
+	.pin_dbg_show = starfive_pin_dbg_show,
+	.dt_node_to_map = starfive_dt_node_to_map,
+	.dt_free_map = pinctrl_utils_free_map,
+};
+
+static int starfive_set_mux(struct pinctrl_dev *pctldev,
+			    unsigned int fsel, unsigned int gsel)
+{
+	struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev);
+	struct device *dev = starfive_dev(sfp);
+	const struct group_desc *group;
+	const u32 *pinmux;
+	unsigned int i;
+
+	group = pinctrl_generic_get_group(pctldev, gsel);
+	if (!group)
+		return -EINVAL;
+
+	pinmux = group->data;
+	for (i = 0; i < group->num_pins; i++) {
+		u32 v = pinmux[i];
+		unsigned int gpio = starfive_pinmux_to_gpio(v);
+		u32 dout = starfive_pinmux_to_dout(v);
+		u32 doen = starfive_pinmux_to_doen(v);
+		u32 din = starfive_pinmux_to_din(v);
+		void __iomem *reg_dout;
+		void __iomem *reg_doen;
+		void __iomem *reg_din;
+		unsigned long flags;
+
+		dev_dbg(dev, "GPIO%u: dout=0x%x doen=0x%x din=0x%x\n",
+			gpio, dout, doen, din);
+
+		reg_dout = sfp->base + GPON_DOUT_CFG + 8 * gpio;
+		reg_doen = sfp->base + GPON_DOEN_CFG + 8 * gpio;
+		if (din != GPI_NONE)
+			reg_din = sfp->base + GPI_CFG_OFFSET + 4 * din;
+		else
+			reg_din = NULL;
+
+		raw_spin_lock_irqsave(&sfp->lock, flags);
+		writel_relaxed(dout, reg_dout);
+		writel_relaxed(doen, reg_doen);
+		if (reg_din)
+			writel_relaxed(gpio + 2, reg_din);
+		raw_spin_unlock_irqrestore(&sfp->lock, flags);
+	}
+
+	return 0;
+}
+
+static const struct pinmux_ops starfive_pinmux_ops = {
+	.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 = starfive_set_mux,
+	.strict = true,
+};
+
+static u16 starfive_padctl_get(struct starfive_pinctrl *sfp,
+			       unsigned int pin)
+{
+	void __iomem *reg = sfp->padctl + 4 * (pin / 2);
+	int shift = 16 * (pin % 2);
+
+	return readl_relaxed(reg) >> shift;
+}
+
+static void starfive_padctl_rmw(struct starfive_pinctrl *sfp,
+				unsigned int pin,
+				u16 _mask, u16 _value)
+{
+	void __iomem *reg = sfp->padctl + 4 * (pin / 2);
+	int shift = 16 * (pin % 2);
+	u32 mask = (u32)_mask << shift;
+	u32 value = (u32)_value << shift;
+	unsigned long flags;
+
+	dev_dbg(starfive_dev(sfp), "padctl_rmw(%u, 0x%03x, 0x%03x)\n", pin, _mask, _value);
+
+	raw_spin_lock_irqsave(&sfp->lock, flags);
+	value |= readl_relaxed(reg) & ~mask;
+	writel_relaxed(value, reg);
+	raw_spin_unlock_irqrestore(&sfp->lock, flags);
+}
+
+#define PIN_CONFIG_STARFIVE_STRONG_PULL_UP	(PIN_CONFIG_END + 1)
+
+static const struct pinconf_generic_params starfive_pinconf_custom_params[] = {
+	{ "starfive,strong-pull-up", PIN_CONFIG_STARFIVE_STRONG_PULL_UP, 1 },
+};
+
+#ifdef CONFIG_DEBUG_FS
+static const struct pin_config_item starfive_pinconf_custom_conf_items[] = {
+	PCONFDUMP(PIN_CONFIG_STARFIVE_STRONG_PULL_UP, "input bias strong pull-up", NULL, false),
+};
+
+static_assert(ARRAY_SIZE(starfive_pinconf_custom_conf_items) ==
+	      ARRAY_SIZE(starfive_pinconf_custom_params));
+#else
+#define starfive_pinconf_custom_conf_items NULL
+#endif
+
+static int starfive_pinconf_get(struct pinctrl_dev *pctldev,
+				unsigned int pin, unsigned long *config)
+{
+	struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev);
+	u16 value = starfive_padctl_get(sfp, pin);
+	int param = pinconf_to_config_param(*config);
+	u32 arg;
+	bool enabled;
+
+	switch (param) {
+	case PIN_CONFIG_BIAS_DISABLE:
+		enabled = value & PAD_BIAS_DISABLE;
+		arg = 0;
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		enabled = value & PAD_BIAS_PULL_DOWN;
+		arg = 1;
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		enabled = !(value & PAD_BIAS_MASK);
+		arg = 1;
+		break;
+	case PIN_CONFIG_DRIVE_STRENGTH:
+		enabled = value & PAD_DRIVE_STRENGTH_MASK;
+		arg = starfive_drive_strength_to_max_mA(value & PAD_DRIVE_STRENGTH_MASK);
+		break;
+	case PIN_CONFIG_INPUT_ENABLE:
+		enabled = value & PAD_INPUT_ENABLE;
+		arg = enabled;
+		break;
+	case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
+		enabled = value & PAD_INPUT_SCHMITT_ENABLE;
+		arg = enabled;
+		break;
+	case PIN_CONFIG_SLEW_RATE:
+		enabled = value & PAD_SLEW_RATE_MASK;
+		arg = (value & PAD_SLEW_RATE_MASK) >> PAD_SLEW_RATE_POS;
+		break;
+	case PIN_CONFIG_STARFIVE_STRONG_PULL_UP:
+		enabled = value & PAD_BIAS_STRONG_PULL_UP;
+		arg = enabled;
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	*config = pinconf_to_config_packed(param, arg);
+	return enabled ? 0 : -EINVAL;
+}
+
+static int starfive_pinconf_group_get(struct pinctrl_dev *pctldev,
+				      unsigned int gsel, unsigned long *config)
+{
+	const struct group_desc *group;
+
+	group = pinctrl_generic_get_group(pctldev, gsel);
+	if (!group)
+		return -EINVAL;
+
+	return starfive_pinconf_get(pctldev, group->pins[0], config);
+}
+
+static int starfive_pinconf_group_set(struct pinctrl_dev *pctldev,
+				      unsigned int gsel,
+				      unsigned long *configs,
+				      unsigned int num_configs)
+{
+	struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev);
+	const struct group_desc *group;
+	u16 mask, value;
+	int i;
+
+	group = pinctrl_generic_get_group(pctldev, gsel);
+	if (!group)
+		return -EINVAL;
+
+	mask = 0;
+	value = 0;
+	for (i = 0; i < num_configs; i++) {
+		int param = pinconf_to_config_param(configs[i]);
+		u32 arg = pinconf_to_config_argument(configs[i]);
+
+		switch (param) {
+		case PIN_CONFIG_BIAS_DISABLE:
+			mask |= PAD_BIAS_MASK;
+			value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE;
+			break;
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			if (arg == 0)
+				return -ENOTSUPP;
+			mask |= PAD_BIAS_MASK;
+			value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_PULL_DOWN;
+			break;
+		case PIN_CONFIG_BIAS_PULL_UP:
+			if (arg == 0)
+				return -ENOTSUPP;
+			mask |= PAD_BIAS_MASK;
+			value = value & ~PAD_BIAS_MASK;
+			break;
+		case PIN_CONFIG_DRIVE_STRENGTH:
+			mask |= PAD_DRIVE_STRENGTH_MASK;
+			value = (value & ~PAD_DRIVE_STRENGTH_MASK) |
+				starfive_drive_strength_from_max_mA(arg);
+			break;
+		case PIN_CONFIG_INPUT_ENABLE:
+			mask |= PAD_INPUT_ENABLE;
+			if (arg)
+				value |= PAD_INPUT_ENABLE;
+			else
+				value &= ~PAD_INPUT_ENABLE;
+			break;
+		case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
+			mask |= PAD_INPUT_SCHMITT_ENABLE;
+			if (arg)
+				value |= PAD_INPUT_SCHMITT_ENABLE;
+			else
+				value &= ~PAD_INPUT_SCHMITT_ENABLE;
+			break;
+		case PIN_CONFIG_SLEW_RATE:
+			mask |= PAD_SLEW_RATE_MASK;
+			value = (value & ~PAD_SLEW_RATE_MASK) |
+				((arg << PAD_SLEW_RATE_POS) & PAD_SLEW_RATE_MASK);
+			break;
+		case PIN_CONFIG_STARFIVE_STRONG_PULL_UP:
+			if (arg) {
+				mask |= PAD_BIAS_MASK;
+				value = (value & ~PAD_BIAS_MASK) |
+					PAD_BIAS_STRONG_PULL_UP;
+			} else {
+				mask |= PAD_BIAS_STRONG_PULL_UP;
+				value = value & ~PAD_BIAS_STRONG_PULL_UP;
+			}
+			break;
+		default:
+			return -ENOTSUPP;
+		}
+	}
+
+	for (i = 0; i < group->num_pins; i++)
+		starfive_padctl_rmw(sfp, group->pins[i], mask, value);
+
+	return 0;
+}
+
+#ifdef CONFIG_DEBUG_FS
+static void starfive_pinconf_dbg_show(struct pinctrl_dev *pctldev,
+				      struct seq_file *s, unsigned int pin)
+{
+	struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev);
+	u16 value = starfive_padctl_get(sfp, pin);
+
+	seq_printf(s, " (0x%03x)", value);
+}
+#else
+#define starfive_pinconf_dbg_show NULL
+#endif
+
+static const struct pinconf_ops starfive_pinconf_ops = {
+	.pin_config_get = starfive_pinconf_get,
+	.pin_config_group_get = starfive_pinconf_group_get,
+	.pin_config_group_set = starfive_pinconf_group_set,
+	.pin_config_dbg_show = starfive_pinconf_dbg_show,
+	.is_generic = true,
+};
+
+static struct pinctrl_desc starfive_desc = {
+	.name = DRIVER_NAME,
+	.pins = starfive_pins,
+	.npins = ARRAY_SIZE(starfive_pins),
+	.pctlops = &starfive_pinctrl_ops,
+	.pmxops = &starfive_pinmux_ops,
+	.confops = &starfive_pinconf_ops,
+	.owner = THIS_MODULE,
+	.num_custom_params = ARRAY_SIZE(starfive_pinconf_custom_params),
+	.custom_params = starfive_pinconf_custom_params,
+	.custom_conf_items = starfive_pinconf_custom_conf_items,
+};
+
+static int starfive_gpio_request(struct gpio_chip *gc, unsigned int gpio)
+{
+	return pinctrl_gpio_request(gc->base + gpio);
+}
+
+static void starfive_gpio_free(struct gpio_chip *gc, unsigned int gpio)
+{
+	pinctrl_gpio_free(gc->base + gpio);
+}
+
+static int starfive_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct starfive_pinctrl *sfp = container_of(gc, struct starfive_pinctrl, gc);
+	void __iomem *doen = sfp->base + GPON_DOEN_CFG + 8 * gpio;
+
+	/* return GPIO_LINE_DIRECTION_OUT (0) only if doen == GPO_ENABLE (0) */
+	return readl_relaxed(doen) != GPO_ENABLE;
+}
+
+static int starfive_gpio_direction_input(struct gpio_chip *gc,
+					 unsigned int gpio)
+{
+	struct starfive_pinctrl *sfp = container_of(gc, struct starfive_pinctrl, gc);
+	void __iomem *doen = sfp->base + GPON_DOEN_CFG + 8 * gpio;
+	unsigned long flags;
+
+	/* enable input and schmitt trigger */
+	starfive_padctl_rmw(sfp, starfive_gpio_to_pin(sfp, gpio),
+			    PAD_INPUT_ENABLE | PAD_INPUT_SCHMITT_ENABLE,
+			    PAD_INPUT_ENABLE | PAD_INPUT_SCHMITT_ENABLE);
+
+	raw_spin_lock_irqsave(&sfp->lock, flags);
+	writel_relaxed(GPO_DISABLE, doen);
+	raw_spin_unlock_irqrestore(&sfp->lock, flags);
+	return 0;
+}
+
+static int starfive_gpio_direction_output(struct gpio_chip *gc,
+					  unsigned int gpio, int value)
+{
+	struct starfive_pinctrl *sfp = container_of(gc, struct starfive_pinctrl, gc);
+	void __iomem *dout = sfp->base + GPON_DOUT_CFG + 8 * gpio;
+	void __iomem *doen = sfp->base + GPON_DOEN_CFG + 8 * gpio;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&sfp->lock, flags);
+	writel_relaxed(value, dout);
+	writel_relaxed(GPO_ENABLE, doen);
+	raw_spin_unlock_irqrestore(&sfp->lock, flags);
+
+	/* disable input, schmitt trigger and bias */
+	starfive_padctl_rmw(sfp, starfive_gpio_to_pin(sfp, gpio),
+			    PAD_BIAS_MASK | PAD_INPUT_ENABLE | PAD_INPUT_SCHMITT_ENABLE,
+			    PAD_BIAS_DISABLE);
+
+	return 0;
+}
+
+static int starfive_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct starfive_pinctrl *sfp = container_of(gc, struct starfive_pinctrl, gc);
+	void __iomem *din = sfp->base + GPIODIN + 4 * (gpio / 32);
+
+	return !!(readl_relaxed(din) & BIT(gpio % 32));
+}
+
+static void starfive_gpio_set(struct gpio_chip *gc, unsigned int gpio,
+			      int value)
+{
+	struct starfive_pinctrl *sfp = container_of(gc, struct starfive_pinctrl, gc);
+	void __iomem *dout = sfp->base + GPON_DOUT_CFG + 8 * gpio;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&sfp->lock, flags);
+	writel_relaxed(value, dout);
+	raw_spin_unlock_irqrestore(&sfp->lock, flags);
+}
+
+static int starfive_gpio_set_config(struct gpio_chip *gc, unsigned int gpio,
+				    unsigned long config)
+{
+	struct starfive_pinctrl *sfp = container_of(gc, struct starfive_pinctrl, gc);
+	u32 arg = pinconf_to_config_argument(config);
+	u16 mask;
+	u16 value;
+
+	switch (pinconf_to_config_param(config)) {
+	case PIN_CONFIG_BIAS_DISABLE:
+		mask  = PAD_BIAS_MASK;
+		value = PAD_BIAS_DISABLE;
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		if (arg == 0)
+			return -ENOTSUPP;
+		mask  = PAD_BIAS_MASK;
+		value = PAD_BIAS_PULL_DOWN;
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		if (arg == 0)
+			return -ENOTSUPP;
+		mask  = PAD_BIAS_MASK;
+		value = 0;
+		break;
+	case PIN_CONFIG_DRIVE_PUSH_PULL:
+		return 0;
+	case PIN_CONFIG_INPUT_ENABLE:
+		mask  = PAD_INPUT_ENABLE;
+		value = arg ? PAD_INPUT_ENABLE : 0;
+		break;
+	case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
+		mask  = PAD_INPUT_SCHMITT_ENABLE;
+		value = arg ? PAD_INPUT_SCHMITT_ENABLE : 0;
+		break;
+	default:
+		return -ENOTSUPP;
+	};
+
+	starfive_padctl_rmw(sfp, starfive_gpio_to_pin(sfp, gpio), mask, value);
+	return 0;
+}
+
+static int starfive_gpio_add_pin_ranges(struct gpio_chip *gc)
+{
+	struct starfive_pinctrl *sfp = container_of(gc, struct starfive_pinctrl, gc);
+
+	sfp->gpios.name = sfp->gc.label;
+	sfp->gpios.base = sfp->gc.base;
+	/*
+	 * sfp->gpios.pin_base depends on the chosen signal group
+	 * and is set in starfive_probe()
+	 */
+	sfp->gpios.npins = NR_GPIOS;
+	sfp->gpios.gc = &sfp->gc;
+	pinctrl_add_gpio_range(sfp->pctl, &sfp->gpios);
+	return 0;
+}
+
+static void starfive_irq_ack(struct irq_data *d)
+{
+	struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
+	irq_hw_number_t gpio = irqd_to_hwirq(d);
+	void __iomem *ic = sfp->base + GPIOIC + 4 * (gpio / 32);
+	u32 mask = BIT(gpio % 32);
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&sfp->lock, flags);
+	writel_relaxed(mask, ic);
+	raw_spin_unlock_irqrestore(&sfp->lock, flags);
+}
+
+static void starfive_irq_mask(struct irq_data *d)
+{
+	struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
+	irq_hw_number_t gpio = irqd_to_hwirq(d);
+	void __iomem *ie = sfp->base + GPIOIE + 4 * (gpio / 32);
+	u32 mask = BIT(gpio % 32);
+	unsigned long flags;
+	u32 value;
+
+	raw_spin_lock_irqsave(&sfp->lock, flags);
+	value = readl_relaxed(ie) & ~mask;
+	writel_relaxed(value, ie);
+	raw_spin_unlock_irqrestore(&sfp->lock, flags);
+}
+
+static void starfive_irq_mask_ack(struct irq_data *d)
+{
+	struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
+	irq_hw_number_t gpio = irqd_to_hwirq(d);
+	void __iomem *ie = sfp->base + GPIOIE + 4 * (gpio / 32);
+	void __iomem *ic = sfp->base + GPIOIC + 4 * (gpio / 32);
+	u32 mask = BIT(gpio % 32);
+	unsigned long flags;
+	u32 value;
+
+	raw_spin_lock_irqsave(&sfp->lock, flags);
+	value = readl_relaxed(ie) & ~mask;
+	writel_relaxed(value, ie);
+	writel_relaxed(mask, ic);
+	raw_spin_unlock_irqrestore(&sfp->lock, flags);
+}
+
+static void starfive_irq_unmask(struct irq_data *d)
+{
+	struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
+	irq_hw_number_t gpio = irqd_to_hwirq(d);
+	void __iomem *ie = sfp->base + GPIOIE + 4 * (gpio / 32);
+	u32 mask = BIT(gpio % 32);
+	unsigned long flags;
+	u32 value;
+
+	raw_spin_lock_irqsave(&sfp->lock, flags);
+	value = readl_relaxed(ie) | mask;
+	writel_relaxed(value, ie);
+	raw_spin_unlock_irqrestore(&sfp->lock, flags);
+}
+
+static int starfive_irq_set_type(struct irq_data *d, unsigned int trigger)
+{
+	struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
+	irq_hw_number_t gpio = irqd_to_hwirq(d);
+	void __iomem *base = sfp->base + 4 * (gpio / 32);
+	u32 mask = BIT(gpio % 32);
+	u32 irq_type, edge_both, polarity;
+	unsigned long flags;
+
+	if (trigger & IRQ_TYPE_EDGE_BOTH)
+		irq_set_handler_locked(d, handle_edge_irq);
+	else if (trigger & IRQ_TYPE_LEVEL_MASK)
+		irq_set_handler_locked(d, handle_level_irq);
+
+	switch (trigger) {
+	case IRQ_TYPE_EDGE_RISING:
+		irq_type  = mask; /* 1: edge triggered */
+		edge_both = 0;    /* 0: single edge */
+		polarity  = mask; /* 1: rising edge */
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		irq_type  = mask; /* 1: edge triggered */
+		edge_both = 0;    /* 0: single edge */
+		polarity  = 0;    /* 0: falling edge */
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		irq_type  = mask; /* 1: edge triggered */
+		edge_both = mask; /* 1: both edges */
+		polarity  = 0;    /* 0: ignored */
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		irq_type  = 0;    /* 0: level triggered */
+		edge_both = 0;    /* 0: ignored */
+		polarity  = mask; /* 1: high level */
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		irq_type  = 0;    /* 0: level triggered */
+		edge_both = 0;    /* 0: ignored */
+		polarity  = 0;    /* 0: low level */
+		break;
+	default:
+		irq_set_handler_locked(d, handle_bad_irq);
+		return -EINVAL;
+	}
+
+	raw_spin_lock_irqsave(&sfp->lock, flags);
+	irq_type |= readl_relaxed(base + GPIOIS) & ~mask;
+	writel_relaxed(irq_type, base + GPIOIS);
+	edge_both |= readl_relaxed(base + GPIOIBE) & ~mask;
+	writel_relaxed(edge_both, base + GPIOIBE);
+	polarity |= readl_relaxed(base + GPIOIEV) & ~mask;
+	writel_relaxed(polarity, base + GPIOIEV);
+	raw_spin_unlock_irqrestore(&sfp->lock, flags);
+	return 0;
+}
+
+static struct irq_chip starfive_irq_chip = {
+	.irq_ack = starfive_irq_ack,
+	.irq_mask = starfive_irq_mask,
+	.irq_mask_ack = starfive_irq_mask_ack,
+	.irq_unmask = starfive_irq_unmask,
+	.irq_set_type = starfive_irq_set_type,
+	.flags = IRQCHIP_SET_TYPE_MASKED,
+};
+
+static void starfive_gpio_irq_handler(struct irq_desc *desc)
+{
+	struct starfive_pinctrl *sfp = starfive_from_irq_desc(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned long mis;
+	unsigned int pin;
+
+	chained_irq_enter(chip, desc);
+
+	mis = readl_relaxed(sfp->base + GPIOMIS + 0);
+	for_each_set_bit(pin, &mis, 32)
+		generic_handle_domain_irq(sfp->gc.irq.domain, pin);
+
+	mis = readl_relaxed(sfp->base + GPIOMIS + 4);
+	for_each_set_bit(pin, &mis, 32)
+		generic_handle_domain_irq(sfp->gc.irq.domain, pin + 32);
+
+	chained_irq_exit(chip, desc);
+}
+
+static int starfive_gpio_init_hw(struct gpio_chip *gc)
+{
+	struct starfive_pinctrl *sfp = container_of(gc, struct starfive_pinctrl, gc);
+
+	/* mask all GPIO interrupts */
+	writel(0, sfp->base + GPIOIE + 0);
+	writel(0, sfp->base + GPIOIE + 4);
+	/* clear edge interrupt flags */
+	writel(~0U, sfp->base + GPIOIC + 0);
+	writel(~0U, sfp->base + GPIOIC + 4);
+	/* enable GPIO interrupts */
+	writel(1, sfp->base + GPIOEN);
+	return 0;
+}
+
+static void starfive_disable_clock(void *data)
+{
+	clk_disable_unprepare(data);
+}
+
+static int starfive_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct starfive_pinctrl *sfp;
+	struct clk *clk;
+	struct reset_control *rst;
+	u32 value;
+	int ret;
+
+	sfp = devm_kzalloc(dev, sizeof(*sfp), GFP_KERNEL);
+	if (!sfp)
+		return -ENOMEM;
+
+	sfp->base = devm_platform_ioremap_resource_byname(pdev, "gpio");
+	if (IS_ERR(sfp->base))
+		return PTR_ERR(sfp->base);
+
+	sfp->padctl = devm_platform_ioremap_resource_byname(pdev, "padctl");
+	if (IS_ERR(sfp->padctl))
+		return PTR_ERR(sfp->padctl);
+
+	clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk), "could not get clock\n");
+
+	rst = devm_reset_control_get_exclusive(dev, NULL);
+	if (IS_ERR(rst))
+		return dev_err_probe(dev, PTR_ERR(rst), "could not get reset\n");
+
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		return dev_err_probe(dev, ret, "could not enable clock\n");
+
+	ret = devm_add_action_or_reset(dev, starfive_disable_clock, clk);
+	if (ret)
+		return ret;
+
+	ret = reset_control_deassert(rst);
+	if (ret)
+		return dev_err_probe(dev, ret, "could not deassert resetd\n");
+
+	platform_set_drvdata(pdev, sfp);
+	sfp->gc.parent = dev;
+	raw_spin_lock_init(&sfp->lock);
+
+	ret = devm_pinctrl_register_and_init(dev, &starfive_desc, sfp, &sfp->pctl);
+	if (ret)
+		return dev_err_probe(dev, ret, "could not register pinctrl driver\n");
+
+	if (!of_property_read_u32(dev->of_node, "starfive,signal-group", &value)) {
+		if (value > 6)
+			return dev_err_probe(dev, -EINVAL, "invalid signal group %u\n", value);
+		writel(value, sfp->padctl + IO_PADSHARE_SEL);
+	}
+
+	value = readl(sfp->padctl + IO_PADSHARE_SEL);
+	switch (value) {
+	case 0:
+		sfp->gpios.pin_base = PAD_INVALID_GPIO;
+		goto done;
+	case 1:
+		sfp->gpios.pin_base = PAD_GPIO(0);
+		break;
+	case 2:
+		sfp->gpios.pin_base = PAD_FUNC_SHARE(72);
+		break;
+	case 3:
+		sfp->gpios.pin_base = PAD_FUNC_SHARE(70);
+		break;
+	case 4: case 5: case 6:
+		sfp->gpios.pin_base = PAD_FUNC_SHARE(0);
+		break;
+	default:
+		return dev_err_probe(dev, -EINVAL, "invalid signal group %u\n", value);
+	}
+
+	sfp->gc.label = dev_name(dev);
+	sfp->gc.owner = THIS_MODULE;
+	sfp->gc.request = starfive_gpio_request;
+	sfp->gc.free = starfive_gpio_free;
+	sfp->gc.get_direction = starfive_gpio_get_direction;
+	sfp->gc.direction_input = starfive_gpio_direction_input;
+	sfp->gc.direction_output = starfive_gpio_direction_output;
+	sfp->gc.get = starfive_gpio_get;
+	sfp->gc.set = starfive_gpio_set;
+	sfp->gc.set_config = starfive_gpio_set_config;
+	sfp->gc.add_pin_ranges = starfive_gpio_add_pin_ranges;
+	sfp->gc.base = -1;
+	sfp->gc.ngpio = NR_GPIOS;
+
+	starfive_irq_chip.parent_device = dev;
+	starfive_irq_chip.name = sfp->gc.label;
+
+	sfp->gc.irq.chip = &starfive_irq_chip;
+	sfp->gc.irq.parent_handler = starfive_gpio_irq_handler;
+	sfp->gc.irq.num_parents = 1;
+	sfp->gc.irq.parents = devm_kcalloc(dev, sfp->gc.irq.num_parents,
+					   sizeof(*sfp->gc.irq.parents), GFP_KERNEL);
+	if (!sfp->gc.irq.parents)
+		return -ENOMEM;
+	sfp->gc.irq.default_type = IRQ_TYPE_NONE;
+	sfp->gc.irq.handler = handle_bad_irq;
+	sfp->gc.irq.init_hw = starfive_gpio_init_hw;
+
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0)
+		return ret;
+	sfp->gc.irq.parents[0] = ret;
+
+	ret = devm_gpiochip_add_data(dev, &sfp->gc, sfp);
+	if (ret)
+		return dev_err_probe(dev, ret, "could not register gpiochip\n");
+
+done:
+	return pinctrl_enable(sfp->pctl);
+}
+
+static const struct of_device_id starfive_of_match[] = {
+	{ .compatible = "starfive,jh7100-pinctrl" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, starfive_of_match);
+
+static struct platform_driver starfive_pinctrl_driver = {
+	.probe = starfive_probe,
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = starfive_of_match,
+	},
+};
+module_platform_driver(starfive_pinctrl_driver);
+
+MODULE_DESCRIPTION("Pinctrl driver for StarFive SoCs");
+MODULE_AUTHOR("Emil Renner Berthing <kernel@esmil.dk>");
+MODULE_LICENSE("GPL v2");