Re: [PATCH v4 2/4] dt-bindings: soc: spacemit: Add spacemit,k1-syscon

[Haylen Chu <heylenay@4d2.org>]

On Sat, Jan 04, 2025 at 11:07:58AM +0100, Krzysztof Kozlowski wrote:
> On Fri, Jan 03, 2025 at 09:56:35PM +0000, Haylen Chu wrote:
> > Add documentation to describe Spacemit K1 system controller registers.
> > 
> > Signed-off-by: Haylen Chu <heylenay@4d2.org>
> > ---
> >  .../soc/spacemit/spacemit,k1-syscon.yaml      | 52 +++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
> > new file mode 100644
> > index 000000000000..79c4a74ff30e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml
> > @@ -0,0 +1,52 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/soc/spacemit/spacemit,k1-syscon.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Spacemit K1 SoC System Controller
> > +
> > +maintainers:
> > +  - Haylen Chu <heylenay@4d2.org>
> > +
> > +description:
> > +  The Spacemit K1 SoC system controller provides access to shared register files
> > +  for related SoC modules, such as clock controller and reset controller.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - spacemit,k1-apbc-syscon
> > +          - spacemit,k1-apbs-syscon
> > +          - spacemit,k1-apmu-syscon
> > +          - spacemit,k1-mpmu-syscon
> > +      - const: syscon
> > +      - const: simple-mfd
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clock-controller:
> > +    $ref: /schemas/clock/spacemit,k1-ccu.yaml#
> > +    type: object
> 
> So now we see the full picture and it leads to questions.
> 
> 1. Why spacemit,k1-apbc-syscon with spacemit,k1-ccu-apmu child is a
> correct combination?
> 
> 2. Why having this split in the first place? Please confirm that clock
> controller is really, really a separate device and its child in
> datasheet. IOW, fake child for your Linux is a no-go. Fake child while
> devices are independent is another no-go.

These syscons are introduced because the clock controllers share
registers with reset controllers. Folding them into the parents results
in devicetree nodes act as both reset and clock controllers, like what
has been done for Rockchip SoCs. Such folding isn't practical for the
MPMU region either, since watchdog and other misc bits (e.g. PLL lock
status) locates in it.

If you're more comfortable with reset and clock controllers folded
together and eliminating most of these syscons, I'm willing to make the
change.

> Actual answer for 1+2 above would be to fold the child into parent,
> assuming clock controller split is fake in terms of datasheet.
> 
> If it is real device, then allOf:if:then: narrowing the compatibles of
> child might not be worth the complexity.
> 
> 3. Why using different naming, look:
> 
> spacemit,k1-XXXX-syscon
> spacemit,k1-ccu-XXXX

I didn't consider about consistency when naming them. Talked to Yixun,
I'll unify them as spacemit,k1-syscon-* and spacemit,k1-ccu-*, keeping
synchronized with other K1 peripherals.

> 
> Best regards,
> Krzysztof
> 

Thanks,
Haylen Chu