From: Conor Dooley Date: Tue, 13 Jun 2023 20:36:24 +0100 Subject: [RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts In-Reply-To: References: <20230524-claw-obligate-e4815c3e232b@spud> <0115F3EA-6071-4876-A50B-CCCB6496FBB6@jrtc27.com> <20230524-climate-citrus-4959e052bf56@spud> <20230612-graveness-uncloak-5f3634a45339@spud> Message-ID: <20230613-define-constrict-7608d966e0e8@spud> List-Id: To: opensbi@lists.infradead.org MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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 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: