Re: 回复: 回复: [PATCH v3 1/3] dt-bindings: reset: add sky1 reset controller

On Mon, Dec 01, 2025 at 03:13:44AM +0000, Gary Yang wrote:

Hi Conor:

Thanks for your comments.

quoted

-----邮件原件-----
发件人: Conor Dooley [off-list ref]
发送时间: 2025年11月27日 3:15
收件人: Gary Yang [off-list ref]
抄送: p.zabel@pengutronix.de; robh@kernel.org; krzk+dt@kernel.org;
conor+dt@kernel.org; devicetree@vger.kernel.org;
linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
cix-kernel-upstream [off-list ref]
主题: Re: 回复: [PATCH v3 1/3] dt-bindings: reset: add sky1 reset controller

On Tue, Nov 25, 2025 at 02:12:23PM +0000, Gary Yang wrote:

quoted

Hi Conor:

Thanks for your comments

quoted

-----邮件原件-----
发件人: Conor Dooley [off-list ref]
发送时间: 2025年11月25日 3:54
收件人: Gary Yang [off-list ref]
抄送: p.zabel@pengutronix.de; robh@kernel.org; krzk+dt@kernel.org;
conor+dt@kernel.org; devicetree@vger.kernel.org;
linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
cix-kernel-upstream [off-list ref]
主题: Re: [PATCH v3 1/3] dt-bindings: reset: add sky1 reset controller

On Mon, Nov 24, 2025 at 02:32:33PM +0800, Gary Yang wrote:

quoted

There are two reset controllers on Cix sky1 Soc.
One is located in S0 domain, and the other is located in S5 domain.

Signed-off-by: Gary Yang <gary.yang@cixtech.com>
---
 .../bindings/reset/cix,sky1-rst.yaml          |  50 ++++++
 include/dt-bindings/reset/cix,sky1-rst-fch.h  |  42 +++++
 include/dt-bindings/reset/cix,sky1-rst.h      | 164

++++++++++++++++++

quoted

quoted

quoted

 3 files changed, 256 insertions(+)  create mode 100644
Documentation/devicetree/bindings/reset/cix,sky1-rst.yaml
 create mode 100644 include/dt-bindings/reset/cix,sky1-rst-fch.h
 create mode 100644 include/dt-bindings/reset/cix,sky1-rst.h

diff --git
a/Documentation/devicetree/bindings/reset/cix,sky1-rst.yaml
b/Documentation/devicetree/bindings/reset/cix,sky1-rst.yaml
new file mode 100644
index 000000000000..a28f938a283d

--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/cix,sky1-rst.yaml

@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML
+1.2
+---
+$id: http://devicetree.org/schemas/reset/cix,sky1-rst.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: CIX Sky1 Reset Controller
+
+maintainers:
+  - Gary Yang <gary.yang@cixtech.com>
+
+description: |
+  CIX Sky1 reset controller can be used to reset various set of

peripherals.

quoted

quoted

quoted

+  There are two reset controllers, one is located in S0 domain,
+the other
+  is located in S5 domain.
+
+  See also:
+  - include/dt-bindings/reset/cix,sky1-rst.h
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - cix,sky1-rst
+          - cix,sky1-rst-fch

You've not addressed my v2 commentary:
https://lore.kernel.org/all/20251114-problem-overbook-383f8e45cd0b@s
pud
/
I asked what else the device does, but you didn't answer me.
Dropping the syscon doesn't make sense if the device genuinely has other

functions.

quoted

quoted

First I'm sorry for not responding your questions earlier. We agree the fact

the register space of reset should not depends on other modules.

quoted

We found that while the reset register spaces on the sky1 platform are

non-contiguous, a specific register space among them is exclusively used by
reset.

quoted

So we can remove syscon property and split serval register spaces. All right?

No, not all right, sorry.
It's perfectly okay for some region to do multiple things, most SoCs have
multiple regions exactly like this.
The normal thing to do is to treat these regions as a syscon like your earlier
version did. The problem with your v1 was that you called the whole thing a
reset, when it isn't just that.
There's plenty of examples using mfd for how these kinds of devices are
handled in the kernel. There's some using the simple-mfd compatible, which is
for when there are subdevices with their own nodes and other defining
mfd_cells and calling mfd_add_devices() when the subdevices do not have
enough complexity for a node (like your reset controller that has one property
and is unlikely to be reusable on another platform).

quoted

quoted

quoted

+  reg:
+    minItems: 1
+    maxItems: 3
+
+  '#reset-cells':
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - '#reset-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/reset/cix,sky1-rst.h>
+    reset-controller@16000304 {
+      compatible = "cix,sky1-rst";

quoted

+      reg = <0x16000304 0xc>,
+            <0x16000400 0x10>,
+            <0x16000800 0x8>;

This is also highly suspect, and I believe what you had before was
probably much more realistic.
Do things properly and fully *now*, rather than pay the price of
unravelling it all later. I just did this for one of my own
platforms, and putting in the effort to completely describe stuff up
front is actually worth it rather than having to refactor years down the line.

quoted

Yes, I agree your view.
This scheme is discussed in our team. It is our decision, not only mine.
There are some modules here that haven't been pushed upstream yet.
If we take them as our internal names, maybe make you confuse. For
example,

If the naming is going to be confusing, then explain things in the description
property.

quoted

The register space based 0x16000000 belongs to PMCTRL_S5. It is a system

power control module, not SCP.

quoted

It not only includes reset controller, but also some usb control,
wakeup sources, clk gates, sleep states settings, generic registers for software,

and so on. But In kernel, we mainly focus on reset controller and usb control.

quoted

They are controlled by the different registers. So we decide to adopt this

scheme.

This is all very normal stuff that syscons are used for on other platforms.
Describe the register region based on what it contains, not based on what you
currently thing that linux is going to use. Maybe later you'll need the other
functions either in linux, or in other projects (like u-boot) that import our
devicetrees.

Yes，First we agree syscon scheme. It is very common in kernel. We want to verify the next actions with you.
One, we add a node syscon@16000000 as pmctrl_s5 an example.
Two, the reset node and usb node both contain a phandle used to point to syscon node

Not quite. The rest node would be empty, other than the phandle, and is
therefore pointless. The #reset-cells property should go in the syscon
node itself. For usb, because it sounds like the syscon is only
providing some control registers and is not the usb controller itself, a
phandle is okay. It's a case by case thing, but if the syscon node
provides an entire feature then a phandle is not suitable and instead
either a child node of the syscon (which would make it a simple-mfd too)
should be used or the properties should go directly into the syscon
node. That depends on how complex the feature is and how likely it is to
be reused between devices. reset is very unlikely to be suitable for
reuse, for example.

Three, In corresponding driver files, we can get the regmap pointers via syscon API.
All right? By the way, How should we describe syscon in yaml file? Are there some files used to refer?

microchip,mpfs-mss-top-sysreg.yaml is one I wrote recently. Going to the
corresponding driver you will be able to see how the reset controller
driver is probed. You should be able to find it in linux-next.
sophgo,sg2044-top-syscon.yaml is another that works in the same way.

Please show us your suggestions. Thanks

Best regards
Gary

quoted

quoted

If you have any questions, please let us know. If make any mistakes, please

remind me kindly.