Message-ID: <20251126-dreamily-shorty-cb543d993e7a@spud>
Date: Wed, 26 Nov 2025 19:15:10 +0000
From: Conor Dooley <conor@...nel.org>
To: Gary Yang <gary.yang@...tech.com>
Cc: "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
	"robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
	cix-kernel-upstream <cix-kernel-upstream@...tech.com>
Subject: 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:
> Hi Conor:
> 
> Thanks for your comments
> 
> > -----邮件原件-----
> > 发件人: Conor Dooley <conor@...nel.org>
> > 发送时间: 2025年11月25日 3:54
> > 收件人: Gary Yang <gary.yang@...tech.com>
> > 抄送: p.zabel@...gutronix.de; robh@...nel.org; krzk+dt@...nel.org;
> > conor+dt@...nel.org; devicetree@...r.kernel.org;
> > linux-kernel@...r.kernel.org; linux-arm-kernel@...ts.infradead.org;
> > cix-kernel-upstream <cix-kernel-upstream@...tech.com>
> > 主题: 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:
> > > 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@...tech.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 ++++++++++++++++++
> > >  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@...tech.com>
> > > +
> > > +description: |
> > > +  CIX Sky1 reset controller can be used to reset various set of peripherals.
> > > +  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@spud
> > /
> > 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.
> > 
> 
> 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.
> 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.
> 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).

> > > +  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@...00304 {
> > > +      compatible = "cix,sky1-rst";
> > 
> > > +      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.

> 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.

> The register space based 0x16000000 belongs to PMCTRL_S5. It is a system power control module, not SCP.
> 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.
> 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.

> If you have any questions, please let us know. If make any mistakes, please remind me kindly. 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists