Re: [PATCH] pmdomain: bcm: bcm2835-power: Raise ASB poll timeout to 100us

[Stefan Wahren <wahrenst@gmx.net>]

Hi Maíra,

Am 30.05.26 um 22:46 schrieb Maíra Canal:
> Commit 18605b1b936b ("pmdomain: bcm: bcm2835-power: Increase ASB control
> timeout") raised the ASB handshake polling budget from 1us to 5us.
> Surveying the pmdomain subsystem, 5us is still one of the smallest polling
> budgets by a wide margin. Comparable handshakes in other drivers use:
>
>    - 100us : starfive jh71xx-pmu, apple pmgr-pwrstate
>    - 1ms   : renesas rcar-sysc, rmobile-sysc (power-on)
>    - 10ms  : renesas rcar-gen4-sysc, sunxi sun55i-pck600
>    - 1s    : mediatek mtk-pm-domains, mtk-scpsys
>
> Raise the BCM2835 timeout to 100us, matching analogous drivers. 100us is
> still negligible relative to a power-domain transition and gives the V3D
> master ASB substantially more headroom to drain under heavy workloads,
> assuring us that the timeout is enough for any scenario.
tbh I'm not convinced by this explanation. Starting with a timeout 
comparison across different pmdomain driver looks strange to me.

My expectation that the reason for such a patch is that there is some 
kind of scenario to trigger unexpected timeouts.
If this is the case, please provide more information about the scenario 
(specific platform, scenario, link to the bug report).

I'm not against the change in general, but please start your commit 
message with the problem and not with the solution.

A related question, does it make sense to add an error message here for 
the timeout case just like in the rest of the driver?

Best regards

>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>   drivers/pmdomain/bcm/bcm2835-power.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pmdomain/bcm/bcm2835-power.c b/drivers/pmdomain/bcm/bcm2835-power.c
> index b76d74e3849b..d507bb675e29 100644
> --- a/drivers/pmdomain/bcm/bcm2835-power.c
> +++ b/drivers/pmdomain/bcm/bcm2835-power.c
> @@ -175,7 +175,7 @@ static int bcm2835_asb_control(struct bcm2835_power *power, u32 reg, bool enable
>   	writel(PM_PASSWORD | val, base + reg);
>   
>   	if (readl_poll_timeout_atomic(base + reg, val,
> -				      !!(val & ASB_ACK) != enable, 0, 5))
> +				      !!(val & ASB_ACK) != enable, 0, 100))
>   		return -ETIMEDOUT;
>   
>   	return 0;