Message-ID:
 <PUZPR06MB58873EBA3818208D061FF866EFDBA@PUZPR06MB5887.apcprd06.prod.outlook.com>
Date: Mon, 1 Dec 2025 03:13:44 +0000
From: Gary Yang <gary.yang@...tech.com>
To: Conor Dooley <conor@...nel.org>
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:
 回复: 回复: [PATCH v3 1/3] dt-bindings: reset: add sky1 reset controller

Hi Conor:

Thanks for your comments.

> -----邮件原件-----
> 发件人: Conor Dooley <conor@...nel.org>
> 发送时间: 2025年11月27日 3:15
> 收件人: 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 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@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.
> > >
> >
> > 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.
> 

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@...00000 as pmctrl_s5 an example.
Two, the reset node and usb node both contain a phandle used to point to syscon node
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?
Please show us your suggestions. Thanks

Best regards
Gary

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

Powered by blists - more mailing lists