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

[Guo Ren]

On Wed, Jun 14, 2023 at 3:36 AM Conor Dooley <conor at kernel.org> wrote:
>
> 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.
We need to put clone-csrs' execution entry into the reset entry (the
first reg address).

        /* Custom reset method for secondary harts */
        val = fdt_getprop(fdt, nodeoff, "entry-reg", &len);
        if (len > 0 && val) {
          p = (char *)(ulong)fdt64_to_cpu(*val);

                val_w = fdt_getprop(fdt, nodeoff, "entry-cnt", &len);
                if (len > 0 && val_w) {
                        tmp = fdt32_to_cpu(*val_w);

                        for (i = 0; i < tmp; i++) {
                                t = (ulong)&__thead_pre_start_warm;
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                writel(t, p + (8 * i));
                                t = (u64)(ulong)&__thead_pre_start_warm >> 32;
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                writel(t, p + (8 * i) + 4);
                                ^^^^^^^^^^^^^^^^^^^^^
                        }
                }

                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);
                        }
                }
        }

I don't think we need to separate it, which made maintenance more complex.

>
>
> > > > 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);
> > }
> > }
>


-- 
Best Regards
 Guo Ren