Re: [PATCH v2 3/3] pinctrl: add pinctrl/GPIO driver for Apple SoCs

[Hector Martin <marcan@marcan.st>]

On 02/10/2021 04.12, Joey Gouly wrote:
> +#define REG_GPIO(x)          (4 * (x))
> +#define REG_GPIOx_DATA       BIT(0)
> +#define REG_GPIOx_MODE_MASK  GENMASK(3, 1)
> +#define REG_GPIOx_OUT        1
> +#define REG_GPIOx_IN_IRQ_HI  2
> +#define REG_GPIOx_IN_IRQ_LO  3
> +#define REG_GPIOx_IN_IRQ_UP  4
> +#define REG_GPIOx_IN_IRQ_DN  5
> +#define REG_GPIOx_IN_IRQ_ANY 6
> +#define REG_GPIOx_IN_IRQ_OFF 7
> +#define REG_GPIOx_PERIPH     BIT(5)
> +#define REG_GPIOx_CFG_DONE   BIT(9)
> +#define REG_GPIOx_GRP_MASK   GENMASK(18, 16)
> +#define REG_IRQ(g, x)        (0x800 + 0x40 * (g) + 4 * ((x) >> 5))

Can we update these defines with the correct definitions and names we 
figured out the other day and add the missing ones? We now know a bunch 
of these are wrong (e.g. CFG_DONE is INPUT_ENABLE, PERIPH should be two 
bits, we're missing pull-up control, drive strength, schmitt trigger and 
lock bits). Even if we don't implement all the features in the driver 
yet, we should have all the register bit defines for documentation 
purposes at least.

> +	if (!(prev & REG_GPIOx_CFG_DONE))
> +		writel_relaxed(cfg & ~REG_GPIOx_CFG_DONE, ppin);
> +	writel_relaxed(cfg, ppin);

We already determined this dance doesn't make any sense; if we want to 
change the pin config before enabling the input buffer (whether this 
serves any purpose at all is an open question) then that should be 
handled in the upper code responsible for enabling/disabling the input 
buffer, not in the core register wrappers.

> +	if (func)
> +		apple_gpio_set_reg(pctl, group, 0,
> +				   REG_GPIOx_PERIPH | REG_GPIOx_CFG_DONE);
> +	else
> +		apple_gpio_set_reg(pctl, group, REG_GPIOx_PERIPH,
> +				   REG_GPIOx_CFG_DONE);

Func is two bits (4 functions) :)

> +static void apple_gpio_gpio_set(struct gpio_chip *chip, unsigned int offset,
> +				int value)
> +{
> +	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
> +
> +	apple_gpio_set_reg(pctl, offset, REG_GPIOx_DATA,
> +			   REG_GPIOx_CFG_DONE | (value & REG_GPIOx_DATA));
> +}

`value ? REG_GPIOx_DATA : 0` please, otherwise this makes assumptions 
about value always being 1 and REG_GPIOx_DATA being the LSB.

Also as we now know, REG_GPIOx_CFG_DONE is nonsense and doesn't belong 
here. Let's drop the cargo cult and drive the hardware based on how it 
works, not how macOS or Corellium decided to do things.

> +static int apple_gpio_gpio_direction_input(struct gpio_chip *chip,
> +					   unsigned int offset)
> +{
> +	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
> +
> +	apple_gpio_set_reg(pctl, offset, REG_GPIOx_MODE_MASK | REG_GPIOx_DATA,
> +			   FIELD_PREP(REG_GPIOx_MODE_MASK,
> +				      REG_GPIOx_IN_IRQ_OFF) |
> +				   REG_GPIOx_CFG_DONE);

Is hardcoding IRQ_OFF correct here? Shouldn't this be getting the 
intended IRQ state from somewhere, or is it always guaranteed that that 
gets set later?

> +static int apple_gpio_gpio_direction_output(struct gpio_chip *chip,
> +					    unsigned int offset, int value)
> +{
> +	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
> +
> +	apple_gpio_set_reg(pctl, offset, REG_GPIOx_PERIPH | REG_GPIOx_DATA,
> +			   FIELD_PREP(REG_GPIOx_MODE_MASK, REG_GPIOx_OUT) |
> +				   (value & REG_GPIOx_DATA) |
> +				   REG_GPIOx_CFG_DONE);

I actually wonder if we should even bother turning on the input buffer 
for output pins, given we're shadowing the value anyway. Seems 
unnecessary and might save a few nanowatts.

Also, why is this clearing the peripheral (yet direction_input isn't)?

> +static void apple_gpio_gpio_irq_mask(struct irq_data *data)
> +{
> +	struct apple_gpio_pinctrl *pctl =
> +		gpiochip_get_data(irq_data_get_irq_chip_data(data));
> +	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_MODE_MASK,
> +			   FIELD_PREP(REG_GPIOx_MODE_MASK,
> +				      REG_GPIOx_IN_IRQ_OFF) |
> +				   REG_GPIOx_CFG_DONE);
> +}

-REG_GPIOx_CFG_DONE please

> +
> +static void apple_gpio_gpio_irq_unmask(struct irq_data *data)
> +{
> +	struct apple_gpio_pinctrl *pctl =
> +		gpiochip_get_data(irq_data_get_irq_chip_data(data));
> +	u32 irqtype = apple_gpio_irq_type(irqd_get_trigger_type(data));
> +
> +	if (WARN_ON(irqtype < 0))
> +		return;
> +	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_MODE_MASK,
> +			   FIELD_PREP(REG_GPIOx_MODE_MASK, irqtype) |
> +				   REG_GPIOx_CFG_DONE);

Ditto

> +static unsigned int apple_gpio_gpio_irq_startup(struct irq_data *data)
> +{
> +	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
> +	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
> +
> +	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_GRP_MASK,
> +			   FIELD_PREP(REG_GPIOx_GRP_MASK, 0));

I guess we're only using a single IRQ group right now?

The driver structure looks good (though see the regmap suggestion from 
Linus). Let's just get the actual hardware part right. I didn't spend a 
couple hours poking register bits with a multimeter, a scope, and a 
breadboard for nothing ;)

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub