[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Conor Dooley]

Hey Guo Ren,

On Tue, Jun 13, 2023 at 10:31:40AM +0800, Guo Ren wrote:
> On Mon, Jun 12, 2023 at 11:39 PM Conor Dooley <conor at kernel.org> wrote:
> > On Mon, Jun 12, 2023 at 08:57:36AM +0800, Guo Ren wrote:
> > > Jisheng Zhang would update the Linux yaml patch, here is the final dts
> > > format of the reset controller:
> > >
> > >                reset-controller at ffff019050 {
> > >                        compatible = "thead,cpu-reset";

Also, please just add the soc-specific compatibles even if the driver
will bind against the most common form of it.

> > >                        reg = <0xff 0xff019050 0x0 0x4>, <0xff
> > > 0xff015004 0x0 0x0>;
> > >                        reset-ctrl = <0x1c>;
> > >                        clone-csrs = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3
> > > 0x7c5 0x7cc 0x7ce>;

To be honest, I don't actually understand why "clone-csrs" is even part
of this "cpu-reset" node. It seems to serve a different purpose to
something that could take various parts of the SoC out of reset.
IMO, the "clone-csrs" stuff is something that should be done based on
the soc-level compatible, and not related to the "cpu-reset" node at
all, which (given the below) sounds more and more like a regular reset
controller.


> > > The reset-ctrl is used to control different parts of soc, generally, a
> > > bit indicates a reset signal (a core/a interconnect/a subsystem).
> >
> > So what is "reset-ctrl"? Values to write into a register? A register
> > address?
> 
> It's a values, and every bit indicate a reset signal.
> 
> > Should this stuff be represented as a proper reset controller, with a
> > "resets" property in each of the controlled nodes w/ a phandle + index?
> > It's hard to say with your explanation here :/ Is there some
> > documentation in English that I could look at? Unfortunately that's only
> > language I speak that anyone writes technical docs in.
> Sorry, there is no exact document about reset-ctrl, becasue every SoC
> customers has some little difference here.
> 
> For SoC intergration, every t-head CPU would give a reset signal, and
> SoC customer could intergrate them into their own reset control
> register, generally, every bit indicate a reset signal. Some of them
> would combine it with their SoC bus reset signal, so we provide a
> flexiable/undefined setting value here.

So TL;DR, highly per-soc specific value, that you cannot explain since
it could mean completely different things, with completely different
behaviour (what registers it even writes to, which buses are affected)

I'm sorry, but I don't see why this shouldn't be split away from the csr
stuff completely, and become a "real" reset controller - especially if
different SoCs using your cores are going to be doing wildly different
things.

Cheers,
Conor.

> This has been implemented in opensbi, and we have used it many years :)
> https://github.com/riscv-software-src/opensbi/blob/master/lib/utils/reset/fdt_reset_thead.c
> 
> val = fdt_getprop(fdt, nodeoff, "control-reg", &len);
> if (len > 0 && val) {
> p = (void *)(ulong)fdt64_to_cpu(*val);
> 
> val_w = fdt_getprop(fdt, nodeoff, "control-val", &len);
> if (len > 0 && val_w) {
> tmp = fdt32_to_cpu(*val_w);
> tmp |= readl(p);
> writel(tmp, p);
> }
> }

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/opensbi/attachments/20230613/aafd8ad0/attachment.sig>