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

Hi Sashiko,

> Thank you for your contribution! Sashiko AI review found 1 potential issu=
e(s)
> to consider:
> - [High] Generic `syscon` node under `simple-pm-bus` will be accessed wit=
h
> its clock disabled, causing a bus fault.
> --
>=20
> --- Patch [5]: [PATCH v3 5/8] riscv: dts: eswin: add hsp bus node --- com=
mit
> 4e57b37c3aadc988b1aed52b079b78ca92be234c
> Author: Pinkesh Vaghela <[email protected]>
>=20
> riscv: dts: eswin: add hsp bus node
>=20
> 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 r=
egister
> spaces of HSP peripheral devices.
>=20
> > --- a/arch/riscv/boot/dts/eswin/eic7700.dtsi
> > +++ b/arch/riscv/boot/dts/eswin/eic7700.dtsi
> > @@ -252,6 +252,19 @@ plic: interrupt-controller@c000000 {
> [ ... ]
> > +             hsp: bus@50400000 {
> > +                     compatible =3D "simple-pm-bus";
> > +                     ranges =3D <0x0 0x50400000 0x0 0x50400000 0x0 0xa=
0000>;
> > +                     clocks =3D <&clk EIC7700_CLK_GATE_HSP_CFG_CLK>;
> > +                     #address-cells =3D <2>;
> > +                     #size-cells =3D <2>;
> > +
> > +                     syscon: syscon@50440000 {
> > +                             compatible =3D "eswin,eic7700-syscfg", "s=
yscon";
> > +                             reg =3D <0x0 0x50440000 0x0 0x2000>;
>=20
> [Severity: High]
> Will accessing this syscon node cause a bus fault due to the clock being
> disabled?
>=20
> Because the child syscon@50440000 uses the generic syscon compatible
> string, it does not bind to a standard platform driver that would invoke
> pm_runtime_get_sync(). As a result, the parent simple-pm-bus will remain
> suspended and the clock EIC7700_CLK_GATE_HSP_CFG_CLK will stay disabled.
>=20
> Additionally, since the clocks property is on the parent bus rather than =
directly
> on the syscon node, of_syscon_register() cannot fetch the clock to attach=
 it to
> the underlying regmap_mmio.
>=20

In this series, we have not yet submitted the DTS patches for the HSP perip=
heral
devices. The intention is for all HSP peripherals to be placed under this
simple-pm-bus node so that the HSP CFG clock can be managed centrally.
A simplified example is shown below:
=20
hsp: bus@50400000 {
        compatible =3D "simple-pm-bus";
        ranges =3D <0x0 0x50400000 0x0 0x50400000 0x0 0xa0000>;
        clocks =3D <&clk EIC7700_CLK_GATE_HSP_CFG_CLK>;
        #address-cells =3D <2>;
        #size-cells =3D <2>;
=20
        syscon: syscon@50440000 {
                compatible =3D "eswin,eic7700-syscfg", "syscon";
                reg =3D <0x0 0x50440000 0x0 0x2000>;
        };
=20
        sdhci_emmc: mmc@50450000 {
                ...
        };
};

> Does this clock need to be placed directly on the syscon node so the gene=
ric
> regmap core can manage it during register accesses?

This clock belongs to HSP bus because it is required to access both HSP CSR
registers and register space of HSP peripheral devices.
So, we should keep this as it is.

Regards,
Pinkesh

>=20
> > +                     };
> > +             };
>=20
> --
> Sashiko AI review *
> https://nam04.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fsashi=
k
> o.dev%2F%23%2Fpatchset%2F20260804104431.1391839-1-
> pinkesh.vaghela%40einfochips.com%3Fpart%3D5&data=3D05%7C02%7Cpinkesh
> .vaghela%40einfochips.com%7Cf2c475cd29514377c91508def21941fb%7C0be
> b0c359cbb4feb99e5589e415c7944%7C1%7C0%7C639214387462287117%7CU
> nknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAw
> MCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C
> &sdata=3D0qgID0%2FlnkF1Lugz0W5MhGpiuV2CbrQ3BVxS2gQH2ZM%3D&reser
> ved=3D0