Re: [PATCH net] net: airoha: Wait for NPU PPE configuration to complete in airoha_ppe_offload_setup()

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2536 bytes --]

> From: 'Simon Horman' <horms@kernel.org>
> 
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
>     net: airoha: Wait for NPU PPE configuration to complete in airoha_ppe_offload_setup()
> 
> This commit adds a poll loop in airoha_ppe_offload_setup() to wait for the NPU
> PPE configuration to complete before running airoha_ppe_hw_init().
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_ppe.c b/drivers/net/ethernet/airoha/airoha_ppe.c
> > index c2c32b6833df9..52199f6b39f9b 100644
> > --- a/drivers/net/ethernet/airoha/airoha_ppe.c
> > +++ b/drivers/net/ethernet/airoha/airoha_ppe.c
> [ ... ]
> > @@ -1340,6 +1341,13 @@ static int airoha_ppe_offload_setup(struct airoha_eth *eth)
> >  	if (err)
> >  		goto error_npu_put;
> >  
> > +	/* Wait for NPU PPE configuration to complete */
> > +	err = read_poll_timeout(airoha_fe_rr, val, val, USEC_PER_MSEC,
> > +				100 * USEC_PER_MSEC, false, eth,
> > +				REG_PPE_PPE_FLOW_CFG(0));
> 
> If the SoC has multiple PPE instances, is it sufficient to only wait for
> REG_PPE_PPE_FLOW_CFG(0)?
> 
> Later in airoha_ppe_hw_init(), the code iterates up to eth->soc->num_ppe,
> initializing all available instances:
> 
> drivers/net/ethernet/airoha/airoha_ppe.c:airoha_ppe_hw_init() {
>     ...
> 	for (i = 0; i < eth->soc->num_ppe; i++) {
> 		...
> 		airoha_fe_wr(eth, REG_PPE_TB_BASE(i),
> 			     ppe->foe_dma + sram_tb_size);
>     ...
> }

ack, let me look into it, I will post a v2.

> 
> If we only poll PPE 0, could the driver race with the NPU firmware's
> initialization of subsequent PPEs?
> 
> Additionally, does this poll condition properly ensure the hardware is ready?
> Since the break condition is simply `val`, it exits immediately if the register
> contains any non-zero value.
> 
> If the register retains a non-zero value from a previous session (like across
> driver reloads), or if an unclocked domain returns 0xFFFFFFFF, this poll might
> succeed immediately and bypass the wait.
> 
> Could we clear the register before polling, or wait for a specific bit instead?

I guess check val != 0 is enough since we reset the FE block and defualt
register value is 0 for both REG_PPE_PPE_FLOW_CFG(0) and
REG_PPE_PPE_FLOW_CFG(1).

Regards,
Lorenzo

> 
> > +	if (err)
> > +		goto error_npu_put;
> > +
> >  	ppe_num_stats_entries = airoha_ppe_get_total_num_stats_entries(ppe);

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]