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

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

[Lorenzo Bianconi]

In order to properly enable flowtable hw offloading, poll
REG_PPE_PPE_FLOW_CFG register in airoha_ppe_offload_setup routine and
wait for NPU PPE configuration triggered by ppe_init callback to complete
before running airoha_ppe_hw_init().

Fixes: 00a7678310fe3 ("net: airoha: Introduce flowtable offload support")
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/net/ethernet/airoha/airoha_ppe.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/airoha/airoha_ppe.c b/drivers/net/ethernet/airoha/airoha_ppe.c
index c2c32b6833df..52199f6b39f9 100644
--- a/drivers/net/ethernet/airoha/airoha_ppe.c
+++ b/drivers/net/ethernet/airoha/airoha_ppe.c
@@ -1332,6 +1332,7 @@ static int airoha_ppe_offload_setup(struct airoha_eth *eth)
 	struct airoha_npu *npu = airoha_ppe_npu_get(eth);
 	struct airoha_ppe *ppe = eth->ppe;
 	int err, ppe_num_stats_entries;
+	u32 val;
 
 	if (IS_ERR(npu))
 		return PTR_ERR(npu);
@@ -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 (err)
+		goto error_npu_put;
+
 	ppe_num_stats_entries = airoha_ppe_get_total_num_stats_entries(ppe);
 	if (ppe_num_stats_entries > 0) {
 		err = npu->ops.ppe_init_stats(npu, ppe->foe_stats_dma,

---
base-commit: 02f72964395911e7a09bb2ea2fe6f79eda4ea2c2
change-id: 20260412-airoha-wait-for-npu-config-offload-setup-19d04522412d

Best regards,
-- 
Lorenzo Bianconi <lorenzo@kernel.org>

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

[Simon Horman]

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);
    ...
}

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?

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

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

[Lorenzo Bianconi]

[-- 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 --]