Re: [PATCH 2/2] pinctrl: Add Intel Keem Bay pinctrl driver

From: Linus Walleij
Date: Wed May 26 2021 - 20:11:44 EST

Hi Lakshmi,

thanks for your patch!

On Mon, May 24, 2021 at 11:26 AM <lakshmi.sowjanya.d@xxxxxxxxx> wrote:

> From: "D, Lakshmi Sowjanya" <lakshmi.sowjanya.d@xxxxxxxxx>
>
> Add pinctrl driver to enable pin control support in the Intel Keem Bay SoC.

That's a very terse summary, please include some info on the SoC
and that it is not x86 (I guess not?)

What really lacks is a description of how the interrupts are routed and
grouped, there is some details about 4 GPIOs sharing one interrupt
but this really needs to be explained, the code is way to terse to
understand. Probably we also need comments in the code itself
to be able to read it and understand the interrupt handling, so add
some of that, illustrations would be good, anything that make it
crystal clear how the GPIO interrupts are grouped and work.

The pin mux / config on the other hand is very straight-forward,
not much to say about that.

> Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@xxxxxxxxx>
> Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@xxxxxxxxx>
> Signed-off-by: D, Lakshmi Sowjanya <lakshmi.sowjanya.d@xxxxxxxxx>
> Reviewed-by: Mark Gross <mgross@xxxxxxxxxxxxxxx>
> Tested-by: A, JyothiX <jyothix.a@xxxxxxxxx>

My first reaction is "how is this hardware different from pinctrl-equilibrium.c?
Also known as "Intel Lightning Mountain".
Can it share code with the former?

Can you do what post pin controller families do and abstract out a
generic pincontrol driver for this family with equilibrium and keembay
as plug-ins? The registers seem to differ so I am not sure if it
can be done.

> + select GPIO_GENERIC

Are you really using this? It would be great if you did.

> +/* GPIO data registers' offsets */
> +#define KEEMBAY_GPIO_DATA_OUT 0x000
> +#define KEEMBAY_GPIO_DATA_IN 0x020
> +#define KEEMBAY_GPIO_DATA_IN_RAW 0x040
> +#define KEEMBAY_GPIO_DATA_HIGH 0x060
> +#define KEEMBAY_GPIO_DATA_LOW 0x080
> +
> +/* GPIO Interrupt and mode registers' offsets */
> +#define KEEMBAY_GPIO_INT_CFG 0x000
> +#define KEEMBAY_GPIO_MODE 0x070

Yeah I haven't seen this before.
(Andy please make sure it doesn't look like some other Intel.)

I guess this hardware is all brand new.

> +/* GPIO mode register bit fields */
> +#define KEEMBAY_GPIO_MODE_PULLUP_MASK GENMASK(13, 12)
> +#define KEEMBAY_GPIO_MODE_DRIVE_MASK GENMASK(8, 7)
> +#define KEEMBAY_GPIO_MODE_INV_MASK GENMASK(5, 4)
> +#define KEEMBAY_GPIO_MODE_SELECT_MASK GENMASK(2, 0)
> +#define KEEMBAY_GPIO_MODE_DIR_OVR BIT(15)
> +#define KEEMBAY_GPIO_MODE_REN BIT(11)
> +#define KEEMBAY_GPIO_MODE_SCHMITT_EN BIT(10)
> +#define KEEMBAY_GPIO_MODE_SLEW_RATE BIT(9)
> +#define KEEMBAY_GPIO_IRQ_ENABLE BIT(7)
> +#define KEEMBAY_GPIO_MODE_DIR BIT(3)
> +#define KEEMBAY_GPIO_MODE_DEFAULT 0x7
> +#define KEEMBAY_GPIO_MODE_INV_VAL 0x3
> +
> +#define KEEMBAY_GPIO_DISABLE 0
> +#define KEEMBAY_GPIO_PULL_UP 1
> +#define KEEMBAY_GPIO_PULL_DOWN 2
> +#define KEEMBAY_GPIO_BUS_HOLD 3
> +#define KEEMBAY_GPIO_NUM_IRQ 8
> +#define KEEMBAY_GPIO_MAX_PER_IRQ 4
> +#define KEEMBAY_GPIO_MAX_PER_REG 32
> +#define KEEMBAY_GPIO_MIN_STRENGTH 2
> +#define KEEMBAY_GPIO_MAX_STRENGTH 12
> +#define KEEMBAY_GPIO_SENSE_LOW (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)

Lots of config features!

(...)
> + KEEMBAY_PIN_DESC(79, "GPIO79",
> + KEEMBAY_MUX(0x0, "PCIE_M0"),
> + KEEMBAY_MUX(0x1, "I2C2_M1"),
> + KEEMBAY_MUX(0x2, "SLVDS1_M2"),
> + KEEMBAY_MUX(0x3, "TPIU_M3"),
> + KEEMBAY_MUX(0x4, "I3C2_M4"),
> + KEEMBAY_MUX(0x5, "LCD_M5"),
> + KEEMBAY_MUX(0x6, "UART3_M6"),
> + KEEMBAY_MUX(0x7, "GPIO_M7")),

I see each pin gets muxed individually.

> +static inline u32 keembay_read_gpio_reg(void __iomem *base, unsigned int pin)
> +{
> + return keembay_read_reg(base, pin / KEEMBAY_GPIO_MAX_PER_REG);
> +}
> +
> +static inline u32 keembay_read_pin(void __iomem *base, unsigned int pin)
> +{
> + u32 val = keembay_read_gpio_reg(base, pin);
> +
> + return !!(val & BIT(pin % KEEMBAY_GPIO_MAX_PER_REG));
> +}

So this is clamping to 32 bits.

What about the old trick of registering one gpiochip per 32 bits and using
GENERIC_GPIO for each? No can do? It is pretty easy to tie it together
using the gpio-ranges see
Documentation/devicetree/bindings/gpio/gpio.txt

> +static void keembay_gpio_invert(struct keembay_pinctrl *kpc, unsigned int pin)
> +{
> + unsigned int val = keembay_read_reg(kpc->base1 + KEEMBAY_GPIO_MODE, pin);
> +
> + val |= FIELD_PREP(KEEMBAY_GPIO_MODE_INV_MASK, KEEMBAY_GPIO_MODE_INV_VAL);
> + keembay_write_reg(val, kpc->base1 + KEEMBAY_GPIO_MODE, pin);
> +}

Why would you want to invert? OK I guess I read and see..

> +static int keembay_request_gpio(struct pinctrl_dev *pctldev,
> + struct pinctrl_gpio_range *range, unsigned int pin)
> +{
> + struct keembay_pinctrl *kpc = pinctrl_dev_get_drvdata(pctldev);
> + unsigned int val;
> +
> + if (pin >= kpc->npins)
> + return -EINVAL;
> +
> + val = keembay_read_reg(kpc->base1 + KEEMBAY_GPIO_MODE, pin);
> + val = FIELD_GET(KEEMBAY_GPIO_MODE_SELECT_MASK, val);
> +
> + /* As per Pin Mux Map, Modes 0 to 6 are for peripherals */
> + if (val != KEEMBAY_GPIO_MODE_DEFAULT)
> + return -EBUSY;
> +
> + return 0;
> +}

> +static u32 keembay_pinconf_get_pull(struct keembay_pinctrl *kpc, unsigned int pin)

All of these pinconf accessors look pretty good.

> + val = u32_replace_bits(val, pull, KEEMBAY_GPIO_MODE_PULLUP_MASK);

Aha bitfield. Smart!

> +static const struct pinctrl_ops keembay_pctlops = {
> + .get_groups_count = pinctrl_generic_get_group_count,
> + .get_group_name = pinctrl_generic_get_group_name,
> + .get_group_pins = pinctrl_generic_get_group_pins,
> + .dt_node_to_map = pinconf_generic_dt_node_to_map_all,
> + .dt_free_map = pinconf_generic_dt_free_map,
> +};
> +
> +static const struct pinmux_ops keembay_pmxops = {
> + .get_functions_count = pinmux_generic_get_function_count,
> + .get_function_name = pinmux_generic_get_function_name,
> + .get_function_groups = pinmux_generic_get_function_groups,
> + .gpio_request_enable = keembay_request_gpio,
> + .set_mux = keembay_set_mux,
> +};

Nice reuse of the generic stuff, nice use of gpio_request_enable()!

> +static int keembay_gpio_get(struct gpio_chip *gc, unsigned int pin)
> +{
> + struct keembay_pinctrl *kpc = gpiochip_get_data(gc);
> + unsigned int val, offset;
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&kpc->lock, flags);
> + val = keembay_read_reg(kpc->base1 + KEEMBAY_GPIO_MODE, pin);
> + offset = (val & KEEMBAY_GPIO_MODE_DIR) ? KEEMBAY_GPIO_DATA_IN : KEEMBAY_GPIO_DATA_OUT;
> +
> + val = keembay_read_pin(kpc->base0 + offset, pin);
> + raw_spin_unlock_irqrestore(&kpc->lock, flags);
> +
> + return val;
> +}
> +
> +static void keembay_gpio_set(struct gpio_chip *gc, unsigned int pin, int val)
> +{
> + struct keembay_pinctrl *kpc = gpiochip_get_data(gc);
> + unsigned int reg_val;
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&kpc->lock, flags);
> + reg_val = keembay_read_gpio_reg(kpc->base0 + KEEMBAY_GPIO_DATA_OUT, pin);
> + if (val)
> + keembay_write_gpio_reg(reg_val | BIT(pin % KEEMBAY_GPIO_MAX_PER_REG),
> + kpc->base0 + KEEMBAY_GPIO_DATA_HIGH, pin);
> + else
> + keembay_write_gpio_reg(~reg_val | BIT(pin % KEEMBAY_GPIO_MAX_PER_REG),
> + kpc->base0 + KEEMBAY_GPIO_DATA_LOW, pin);
> +
> + raw_spin_unlock_irqrestore(&kpc->lock, flags);
> +}

So the spinlock protects against stuff that GPIO_GENERIC in
gpio-mmio.c is already implementing for single 8/16/32/64 bit
registers.

So if you could split this controller into one gpio_chip per
register, you could reuse all that.

> +static void keembay_gpio_irq_handler(struct irq_desc *desc)
> +{
> + struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> + unsigned int kmb_irq = irq_desc_get_irq(desc);
> + unsigned long reg, clump = 0, bit = 0;
> + unsigned int src, trig, pin, val;
> + struct irq_chip *parent_chip;
> + struct keembay_pinctrl *kpc;
> +
> + for (src = 0; src < KEEMBAY_GPIO_NUM_IRQ; src++) {
> + if (kmb_irq == gc->irq.parents[src])
> + break;
> + }
> +
> + if (src == KEEMBAY_GPIO_NUM_IRQ)
> + return;

So this gets a bit awkward to look up we need to understand the
way GPIOs are grouped into IRQs here.

> +
> + parent_chip = irq_desc_get_chip(desc);
> + kpc = gpiochip_get_data(gc);
> +
> + chained_irq_enter(parent_chip, desc);
> + reg = keembay_read_reg(kpc->base1 + KEEMBAY_GPIO_INT_CFG, src);
> + trig = kpc->irq[src].trigger;
> +
> + /*
> + * Each Interrupt line can be shared up to 4 GPIO pins. Enable bit and
> + * input values were checked to indentify the source of the Interrupt.

Indentify?

> + * The checked enable bit positions are 7, 15, 23 and 31.
> + */
> + for_each_set_clump8(bit, clump, &reg, BITS_PER_TYPE(typeof(reg))) {
> + pin = clump & ~KEEMBAY_GPIO_IRQ_ENABLE;
> + val = keembay_read_pin(kpc->base0 + KEEMBAY_GPIO_DATA_IN, pin);
> + kmb_irq = irq_linear_revmap(gc->irq.domain, pin);
> +
> + if (val && (trig & IRQ_TYPE_SENSE_MASK))
> + generic_handle_irq(kmb_irq);

Put in a comment why you have to check the trigger.

(...)

> +static int keembay_pinctrl_reg(struct keembay_pinctrl *kpc, struct device *dev)
> +{
> + int ret = of_property_read_u32(dev->of_node, "num-gpios", &kpc->npins);

ngpios is the standard property. Use that. Also change the bindings to
reflect this.

The GPIO chip does not implement .set_config though it should be
super simple: just use gpiochip_generic_config() like
drivers/pinctrl/intel/pinctrl-intel.c does.

I guess I will have more comments once I understand the hardware.

Yours,
Linus Walleij