Re: [PATCH v2 2/6] remoteproc: amd_mbv_bram: Restore reset GPIO direction after PL reconfig

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] remoteproc: amd_mbv_bram: processor not held in reset during fw load after DFX
--

commit 1347fae87b8f238d03e271a968bc4d43270edfb5
Author: Ben Levinsky <ben.levinsky@amd.com>

remoteproc: amd_mbv_bram: Restore reset GPIO direction after PL reconfig

This commit replaces gpiod_set_value_cansleep() with gpiod_direction_output()
when asserting and deasserting the soft-core reset line, and adds a reset
pulse in amd_bram_rproc_start() so that the GPIO output direction is restored
after programmable logic partial reconfiguration.

> diff --git a/drivers/remoteproc/amd_mbv_bram_rproc.c b/drivers/remoteproc/amd_mbv_bram_rproc.c
> index 6cf33c7b7e641..da621f62f7df5 100644
> --- a/drivers/remoteproc/amd_mbv_bram_rproc.c
> +++ b/drivers/remoteproc/amd_mbv_bram_rproc.c
[ ... ]
> @@ -94,8 +95,24 @@ static int amd_bram_rproc_start(struct rproc *rproc)
>  		return ret;
>  	}
>  
> +	/*
> +	 * Partial reconfiguration restores the PL GPIO hardware defaults but
> +	 * leaves gpiolib's cached direction unchanged. Re-establish the output
> +	 * direction and generate a reset pulse after firmware has been loaded.
> +	 * gpiod_set_value_cansleep() is insufficient here because it changes the
> +	 * value without restoring an output direction lost during DFX.
> +	 */
> +	ret = gpiod_direction_output(priv->reset, 1);

[Severity: Medium]
If partial reconfiguration restores the PL GPIO to its hardware default input
direction while the remoteproc is offline, is restoring the output direction
and asserting reset in amd_bram_rproc_start() too late to keep the processor
in reset while firmware is loaded into BRAM?

In amd_bram_rproc_probe(), the driver notes that the processor must be held in
reset during firmware loading:

	/*
	 * Keep the processor in reset until remoteproc has finished loading
	 * firmware into the executable memory window described by reg and
	 * translated through the parent bus ranges property.
	 */
	priv->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);

However, during rproc_boot() -> rproc_fw_boot(), remoteproc calls
rproc_prepare_device() -> amd_bram_rproc_prepare() and then rproc_start(),
which runs rproc_load_segments() -> rproc_elf_load_segments() and copies the
loaded resource table into dual-port BRAM before invoking
amd_bram_rproc_start().

If the PL clock is already running (such as a shared PL clock required to
access the AXI BRAM controller during rproc_elf_load_segments()) and DFX left
the reset GPIO released in input mode, the soft-core processor can begin
executing from BRAM while rproc_load_segments() is writing the ELF image.
Even though amd_bram_rproc_start() subsequently pulses reset for 10-20 us,
any writable segments (.data, .bss, or the resource table) modified in BRAM
by that premature execution will not be reloaded before the processor starts
again.

Should gpiod_direction_output(priv->reset, 1) also be called in
amd_bram_rproc_prepare() so that the processor is guaranteed to be held in
reset before rproc_load_segments() writes to BRAM?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105051902.1646651-1-ben.levinsky@amd.com?part=2