Re: [PATCH] firmware: stratix10-svc: support up to 4 buffer claims and extend timeout

On Tue, Feb 10, 2026 at 03:16:42PM +0800, [email protected] wrote:
> From: Adrian Ng Ho Yin <[email protected]>
> 
> The service layer previously only returned up to three buffer addresses
> per transaction. Extend the logic in svc_thread_cmd_data_claim() to
> collect up to four buffer claims. A new field `kaddr4` is added to

Why previously use 3 buffers per transaction, and why now use 4, not
5, 6...  I.e., I saw some association from struct
arm_smccc_res::a1/a2/a3 so we used 3 buffers previouly (correct me if
wrong). But the 4th buffer seems pure SVC decision so please elaborate.

> struct stratix10_svc_cb_data, and the FPGA manager callback unlocks this
> fourth buffer accordingly.
> 
> Timeout values for reconfiguration and buffer transactions are also
> increased (from ~300–720ms to 5000ms), since real-world processing takes
> significantly longer (~600ms or more). The reconfiguration complete
> timeout is also replaced with S10_RECONFIG_TIMEOUT (>1s) to avoid
> premature aborts.
> 
> Additional changes:
> - Callbacks are updated to pass all claimed buffers to clients.
> - Debug logging is added to trace received status values.
> - Simplified buffer wait logic in s10_ops_write() by always using
>   wait_for_completion_timeout().

The patch includes too much independent changes which makes reviewers
hard to distinguish which line is for which purpose, please split your
patch into a series. This also gives chances for you to explain why each
change is necessary or nice to have.

> 
> These changes improve robustness of FPGA configuration on SoCFPGA
> platforms by handling larger transactions and accommodating realistic
> timing.
> 
...

> diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
> index 0a295ccf1644..4fea9458f92b 100644
> --- a/drivers/fpga/stratix10-soc.c
> +++ b/drivers/fpga/stratix10-soc.c
> @@ -147,6 +147,7 @@ static void s10_receive_callback(struct stratix10_svc_client *client,
>  	u32 status;
>  	int i;
>  
> +	pr_debug("%s data %x\n", __func__, data->status);

I'd prefer you delete it when you think the patches are ready to get merged.

...

> @@ -353,7 +348,10 @@ static int s10_ops_write_complete(struct fpga_manager *mgr,
>  	unsigned long timeout;
>  	int ret;
>  
> -	timeout = usecs_to_jiffies(info->config_complete_timeout_us);
> +	/* The time taken to process this is close to 600ms
> +	 * This MUST be increased over 1 second
> +	 */
> +	timeout = S10_RECONFIG_TIMEOUT;

This was a configurable option and you now hard code it, I don't see the
rationale.