[PATCH v3 1/5] dt-bindings: soc: cix,sky1-system-control: add audss system control

[Joakim Zhang]

Hi,


> -----Original Message-----
> From: Krzysztof Kozlowski <krzk at kernel.org>
> Sent: Thursday, June 11, 2026 3:40 PM
> To: Joakim Zhang <joakim.zhang at cixtech.com>
> Cc: mturquette at baylibre.com; sboyd at kernel.org; bmasney at redhat.com;
> robh at kernel.org; krzk+dt at kernel.org; conor+dt at kernel.org;
> p.zabel at pengutronix.de; Gary Yang <gary.yang at cixtech.com>; cix-kernel-
> upstream <cix-kernel-upstream at cixtech.com>; linux-clk at vger.kernel.org;
> devicetree at vger.kernel.org; linux-kernel at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: [PATCH v3 1/5] dt-bindings: soc: cix,sky1-system-control: add audss
> system control
> 
> EXTERNAL EMAIL
> 
> On Wed, Jun 10, 2026 at 03:56:41PM +0800, joakim.zhang at cixtech.com wrote:
> > From: Joakim Zhang <joakim.zhang at cixtech.com>
> >
> > The Cix Sky1 Audio Subsystem (AUDSS) groups audio-related clock, reset
> > and control registers in a dedicated CRU block. Software reset lines
> > are exposed on the syscon parent via #reset-cells, following the same
> > model as the existing Sky1 FCH and S5 system control bindings.
> >
> > Add the cix,sky1-audss-system-control compatible to
> > cix,sky1-system-control.yaml for the MFD/syscon parent node, and
> > define AUDSS software reset indices in
> > include/dt-bindings/reset/cix,sky1-audss-system-control.h for I2S,
> > HDA, DMAC, mailbox, watchdog and timer blocks.
> 
> All this is pretty pointless - you explained the binding, which answers nothing
> why you did it that way. Instead you must explain the hardware design.
> 
> >
> > Signed-off-by: Joakim Zhang <joakim.zhang at cixtech.com>
> > ---
> >  .../soc/cix/cix,sky1-system-control.yaml      | 52 +++++++++++++++++--
> >  .../reset/cix,sky1-audss-system-control.h     | 25 +++++++++
> >  2 files changed, 72 insertions(+), 5 deletions(-)  create mode 100644
> > include/dt-bindings/reset/cix,sky1-audss-system-control.h
> >
> > diff --git
> > a/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-control.ya
> > ml
> > b/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-control.ya
> > ml index a01a515222c6..61d26a69fd44 100644
> > ---
> > a/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-control.ya
> > ml
> > +++ b/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-contro
> > +++ l.yaml
> > @@ -15,11 +15,16 @@ description:
> >
> >  properties:
> >    compatible:
> > -    items:
> > -      - enum:
> > -          - cix,sky1-system-control
> > -          - cix,sky1-s5-system-control
> > -      - const: syscon
> > +    oneOf:
> > +      - items:
> > +          - enum:
> > +              - cix,sky1-system-control
> > +              - cix,sky1-s5-system-control
> > +          - const: syscon
> > +      - items:
> > +          - const: cix,sky1-audss-system-control
> > +          - const: simple-mfd
> 
> Just so you are aware - this means children do not depend on the parent for
> operation. You will not be able to fix it later, if it turns out that children do
> depend...

Understood. simple-mfd is intentional: the clock child only accesses the parent MMIO via syscon and external resets/clocks via phandles. No parent driver coordination is needed today. We attached all resources audss needed from child node now. 


> > +          - const: syscon
> >
> >    reg:
> >      maxItems: 1
> > @@ -27,6 +32,28 @@ properties:
> >    '#reset-cells':
> >      const: 1
> >
> > +  clock-controller:
> > +    type: object
> > +    properties:
> > +      compatible:
> > +        const: cix,sky1-audss-clock
> > +    required:
> > +      - compatible
> > +    additionalProperties: true
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: cix,sky1-audss-system-control
> > +    then:
> > +      required:
> > +        - clock-controller
> > +    else:
> > +      properties:
> > +        clock-controller: false
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -40,3 +67,18 @@ examples:
> >        reg = <0x4160000 0x100>;
> >        #reset-cells = <1>;
> >      };
> > +  - |
> > +    audss_syscon: system-controller at 7110000 {
> > +        compatible = "cix,sky1-audss-system-control", "simple-mfd", "syscon";
> > +        reg = <0x7110000 0x10000>;
> > +        #reset-cells = <1>;
> > +
> > +        clock-controller {
> > +            compatible = "cix,sky1-audss-clock";
> > +            power-domains = <&smc_devpd 0>;
> 
> My questions from v2 from the other patch are still valid - why audss system
> clock controller is outside of the power domain? Why the audss reset is outside,
> but audss clock not?
> 
> This does not feel like correct hardware representation.

Yes, I agree with your point. This does not really reflect the hardware well. Both noc reset and power-domain takes effect on audss, should move to parent node.
 
Thanks,
Joakim