Re: [PATCH v4 3/4] gpio: pl061: add Ambarella CV75 register layout variant

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Global replacement of strictly ordered MMIO accessors with relaxed variants strips required memory barriers, risking device synchronization and IRQ storms.
- [High] Unconditional clearing of the Alternate Function Select (`afsel`) register in probe forces all pins to GPIO mode, breaking bootloader-configured peripherals.
--

--- Patch [3]: [PATCH v4 3/4] gpio: pl061: add Ambarella CV75 register layout variant ---
commit 2ccca896232c8172aca5451ae64d6ee831bac07f
Author: Long Zhao <[email protected]>

gpio: pl061: add Ambarella CV75 register layout variant

This commit extends the gpio-pl061 driver with a per-variant register layout
to support the Ambarella CV75 GPIO banks. It introduces wrapper functions for
read/write operations and variant-specific data structures.

> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index 919cf86fd590f..d9e5a9f25681c 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c
[ ... ]
> @@ -26,41 +26,94 @@
[ ... ]
> +static u32 pl061_read(struct pl061 *pl061, u32 offset)
> +{
> +	if (pl061->variant->access_32bit)
> +		return readl_relaxed(pl061->base + offset);
> +
> +	return readb_relaxed(pl061->base + offset);
> +}
> +
> +static void pl061_write(struct pl061 *pl061, u32 value, u32 offset)
> +{
> +	if (pl061->variant->access_32bit)
> +		writel_relaxed(value, pl061->base + offset);
> +	else
> +		writeb_relaxed(value, pl061->base + offset);
> +}

[Severity: High]
Is it safe to replace the strictly ordered MMIO accessors with relaxed
variants globally in these wrappers?

By dropping the implicit memory barriers previously provided by readb() and
writeb(), could this lead to out-of-order MMIO operations and system lockups?

For example, in pl061_irq_ack(), clearing the interrupt uses this new relaxed
wrapper. Because writeb_relaxed() lacks the wmb() barrier provided by writeb(),
could the write clearing the peripheral interrupt remain in the CPU store buffer
when the IRQ handler completes and unmasks the parent IRQ? This might cause
the CPU to instantly re-take the uncleared interrupt, resulting in an IRQ
storm.

Similarly, could bit-banging operations using GPIO be reordered with respect to
DMA memory stores, breaking device data transfers?

[ ... ]
> @@ -335,7 +402,14 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
>  	/*
>  	 * irq_chip support
>  	 */
> -	writeb(0, pl061->base + GPIOIE); /* disable irqs */
> +	pl061_write(pl061, 0, pl061->variant->ie); /* disable irqs */
> +	if (pl061->variant->enable != PL061_REG_NONE)
> +		pl061_write(pl061, GENMASK(pl061->gc.ngpio - 1, 0),
> +			    pl061->variant->enable);
> +	if (pl061->variant->afsel != PL061_REG_NONE)
> +		pl061_write(pl061, 0, pl061->variant->afsel);

[Severity: High]
Does writing 0 to the entire afsel (Alternate Function Select) register
unconditionally during pl061_probe() reset the multiplexing for all pins
on the bank back to GPIO mode?

Could this forcibly disconnect any active peripherals (like UART, SPI, I2C)
that were already configured by the bootloader? If the system console UART
shares this GPIO bank, it seems like the system would immediately lose serial
output on boot.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3