Re: [PATCH] dts: riscv: spacemit: k3: Fix I/O power settings

[Yixun Lan <dlan@kernel.org>]

Hi 

On 21:15 Mon 18 May     , sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The newly added `spacemit,apbc` DT property triggers a spurious boot warning and relies on a fragile probe deferral dependency, because the target `syscon_apbc` node lacks the `"syscon"` compatible string.
> --
> 
> commit 89bb8c89cecf7c32a2e825a2b8f604c5c45762bd
> Author: Yixun Lan <dlan@kernel.org>
> 
> dts: riscv: spacemit: k3: Fix I/O power settings
>     
> SpacemiT K3 SoC support dual-voltage I/O power domain, while initially
> configure to 3.3v, and need to access register from APBC space to switch
> to 1.8v domain.
>     
> Fix the GMAC0's I/O pins 1.8v switch failure that will result a broken
> ethernet driver.
> 
> > diff --git a/arch/riscv/boot/dts/spacemit/k3.dtsi b/arch/riscv/boot/dts/spacemit/k3.dtsi
> > index 815debd16409b..2b0bf9cd755bd 100644
> > --- a/arch/riscv/boot/dts/spacemit/k3.dtsi
> > +++ b/arch/riscv/boot/dts/spacemit/k3.dtsi
> > @@ -803,6 +803,7 @@ pinctrl: pinctrl@d401e000 {
> >  			clocks = <&syscon_apbc CLK_APBC_AIB>,
> >  				 <&syscon_apbc CLK_APBC_AIB_BUS>;
> >  			clock-names = "func", "bus";
> > +			spacemit,apbc = <&syscon_apbc>;
> 
> Does this newly added spacemit,apbc property trigger a spurious boot warning
> and rely on a fragile probe deferral dependency?
> 
> Looking at the referenced syscon_apbc node, it lacks the "syscon" compatible
> string.
> 
> This prevents syscon_node_to_regmap() from instantiating the regmap
> on-the-fly, causing it to return -EPROBE_DEFER if the pinctrl driver probes
> before the clock driver registers the regmap.
> 
This is possible, I think it's worth the effort to add some logic to
handle the deferral probe situation

> The pinctrl driver catches this with a blanket IS_ERR() check and prints a
> now-spurious "no syscon found" warning during boot while discarding the
> regmap pointer:
> 
> drivers/pinctrl/spacemit/pinctrl-k1.c:spacemit_pinctrl_probe() {
> 	...
> 	pctrl->regmap_apbc = syscon_regmap_lookup_by_phandle(np, "spacemit,apbc");
> 	if (IS_ERR(pctrl->regmap_apbc)) {
> 		dev_warn(dev, "no syscon found, disable power voltage switch functionality\n");
> 		pctrl->regmap_apbc = NULL;
> 	}
> 	...
> }
> 
> The voltage switch functionality only ultimately succeeds because the
> subsequent devm_clk_get_enabled() call against the same node also returns
> -EPROBE_DEFER.
> 
> This gives the clock driver time to probe and register the regmap before the
> pinctrl driver retries.
> 
make sense

> Would adding "syscon" to the syscon_apbc node's compatible string, or
> explicitly handling -EPROBE_DEFER in the pinctrl driver, resolve this issue
> and prevent the spurious warning?
> 
I prefer to do the latter way

-- 
Yixun Lan (dlan)