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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
 <PUZPR06MB5887CB84BC4D006EC594B654EFD1A@PUZPR06MB5887.apcprd06.prod.outlook.com>
Date: Tue, 25 Nov 2025 14:12:23 +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月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?
> > +
> > +  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.
> 
> Cheers,
> Conor.
> 

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

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

Best wishes
Gary

> pw-bot: changes-requested