Re: [PATCH v2 5/8] riscv: dts: eswin: add hsp bus node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `hsp_sp_csr` syscon node lacks a `clocks` property, causing unclocked MMIO accesses that can crash the system.
- [Low] Non-generic node name for syscon device.
--

commit 2f5171ba39f634bc5960f8f8754a5b822bc8c76b
Author: Pinkesh Vaghela <pinkesh.vaghela@einfochips.com>

riscv: dts: eswin: add hsp bus node

Add 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
> --- 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>;
> +
> +			hsp_sp_csr: hsp-sp-top-csr@50440000 {

[Severity: Low]
Does this node name violate standard Device Tree naming conventions?

According to the Devicetree Specification, node names should be generic and
reflect the function of the device rather than its precise programming model.

Should this be named syscon@50440000 instead of hsp-sp-top-csr@50440000?

> +				compatible = "eswin,eic7700-syscfg", "syscon";
> +				reg = <0x0 0x50440000 0x0 0x2000>;
> +			};

[Severity: High]
Will accessing this syscon node cause a synchronous external abort?

Since this syscon node is placed under a simple-pm-bus, and syscon nodes
do not bind to standard platform drivers, no pm_runtime_get_sync() is ever
called on behalf of the syscon. 

This means the parent bus remains runtime-suspended, leaving the
EIC7700_CLK_GATE_HSP_CFG_CLK clock disabled.

The syscon regmap implementation can manage its own clock explicitly during
IO accesses via regmap_mmio_attach_clk(), but it requires the clocks property
to be present in the syscon node itself.

Because this is missing the clocks property, neither the bus nor the regmap
will enable the clock prior to the MMIO access, which could crash the system
if accessed via debugfs or a consumer driver.

Should the clocks property be added directly to this node as well?

> +		};
> +

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