Re: [PATCH v4 4/7] riscv: dts: eswin: add hsp bus node

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The syscon node is nested under a simple-pm-bus, which relies on child PM runtime to enable its clock, but syscon bypasses PM runtime, leading to unclocked MMIO accesses.
- [Low] The compatible string 'eswin,eic7700-syscfg' is introduced without corresponding device tree binding documentation.
--

--- Patch [4]: [PATCH v4 4/7] riscv: dts: eswin: add hsp bus node ---
commit 478468d2187b14120b83e2c525f0193b36412f11
Author: Pinkesh Vaghela <[email protected]>

riscv: dts: eswin: add hsp bus node

This commit adds an HSP bus node to manage the HSP CFG clock used by HSP
peripherals. This clock is required for accessing both the HSP CSR registers
and the register spaces of HSP peripheral devices.

> diff --git a/arch/riscv/boot/dts/eswin/eic7700.dtsi b/arch/riscv/boot/dts/eswin/eic7700.dtsi
> index b16e3e28632c2..771a88645d835 100644
> --- 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: bus@50400000 {
> +			compatible = "simple-pm-bus";
> +			ranges = <0x0 0x50400000 0x0 0x50400000 0x0 0xa0000>;
> +			clocks = <&clk EIC7700_CLK_GATE_HSP_CFG_CLK>;
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +
> +			syscon: syscon@50440000 {

[Severity: High]
Will placing the syscon node under a simple-pm-bus lead to unclocked MMIO
accesses and system hangs?

The simple-pm-bus relies on child PM runtime to enable its clock
(EIC7700_CLK_GATE_HSP_CFG_CLK). However, syscon does not instantiate a standard
platform driver. When syscon_node_to_regmap() is called by any dependent
driver, it initializes a regmap with a NULL device and performs raw MMIO
accesses without calling pm_runtime_get_sync().

Because the parent bus is never resumed by the child, the clock remains gated,
which can cause any read/write to the syscon registers to hang the system.

> +				compatible = "eswin,eic7700-syscfg", "syscon";

[Severity: Low]
Is the "eswin,eic7700-syscfg" compatible string missing its device tree
binding documentation?

Without corresponding schema documentation in the bindings tree, this will
cause schema validation warnings during dtbs_check.

> +				reg = <0x0 0x50440000 0x0 0x2000>;
> +			};
> +		};
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4