Re: [PATCH 5/7] riscv: dts: eswin: add hsp power domain

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The HSP CFG clock will never be enabled, causing a hardware hang during register access, because `simple-pm-bus` requires children to use `pm_runtime` but `syscon` bypasses it.
- [Medium] The node is labeled and described as a 'power domain' but is modeled as a simple bus without the ability to act as a power domain provider.
--

commit 0dad730760e600f73d118515b1907e14ac735dad
Author: Pinkesh Vaghela <pinkesh.vaghela@einfochips.com>

riscv: dts: eswin: add hsp power domain

This commit adds an HSP power domain node to manage the HSP CFG clock,
which is required to access the HSP CSR registers. The CSR is defined
as a syscon child node within a simple-pm-bus.

> diff --git a/arch/riscv/boot/dts/eswin/eic7700.dtsi b/arch/riscv/boot/dts/eswin/eic7700.dtsi
> --- a/arch/riscv/boot/dts/eswin/eic7700.dtsi
> +++ b/arch/riscv/boot/dts/eswin/eic7700.dtsi
> @@ -252,6 +252,19 @@ plic: interrupt-controller@c000000 {
>  			#interrupt-cells = <1>;
>  		};
>  
> +		hsp_power_domain: bus@50400000 {
> +			compatible = "simple-pm-bus";

[Severity: Medium]
The commit message and node label describe this as a power domain, but the
node uses simple-pm-bus and lacks a #power-domain-cells property. Can
other devices actually reference this as a power domain?

simple-pm-bus acts as a transparent bus for managing clocks or resets for
its direct children, rather than a genpd provider. If this is meant to act
as a power domain that other IP blocks depend on via power-domains = <...>,
does it need a dedicated driver and the #power-domain-cells property?

If it is strictly meant to manage the syscon clock, should the wrapper be
removed entirely?

> +			ranges = <0x0 0x50400000 0x0 0x50400000 0x0 0xa0000>;
> +			clocks = <&clk EIC7700_CLK_GATE_HSP_CFG_CLK>;

[Severity: High]
Since the child node uses the generic syscon compatible, will this clock
ever be enabled?

The syscon framework in drivers/mfd/syscon.c does not register a platform
driver, does not probe a device, and does not invoke pm_runtime_get_sync().
Because the child device is never probed or runtime-resumed, the parent
simple-pm-bus will remain suspended.

Would this cause a system hang when accessing the syscon registers since
the clock would remain disabled?

Additionally, of_syscon_register() natively expects the clocks property to
be directly in the syscon node. Should the clocks property be moved to the
hsp_sp_csr node and the simple-pm-bus wrapper removed?

> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +
> +			hsp_sp_csr: hsp-sp-top-csr@50440000 {
> +				compatible = "eswin,eic7700-syscfg", "syscon";
> +				reg = <0x0 0x50440000 0x0 0x2000>;
> +			};
> +		};
> +
>  		uart0: serial@50900000 {
>  			compatible = "snps,dw-apb-uart";
>  			reg = <0x0 0x50900000 0x0 0x10000>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615122016.1110206-1-pinkesh.vaghela@einfochips.com?part=5