Re: [PATCH V3 1/3] dt-bindings: clk: sprd: Add bindings for ums512 clock controller

[Cixi Geng]

Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> 于2022年4月19日周二 14:38写道：
>
> On 19/04/2022 03:53, Cixi Geng wrote:
> > Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> 于2022年4月19日周二 00:28写道：
> >>
> >> On 18/04/2022 14:56, Cixi Geng wrote:
> >>> From: Cixi Geng <cixi.geng1@xxxxxxxxxx>
> >>>
> >>> Add a new bindings to describe ums512 clock compatible string.
> >>>
> >>> Signed-off-by: Cixi Geng <cixi.geng1@xxxxxxxxxx>
> >>> ---
> >>>  .../bindings/clock/sprd,ums512-clk.yaml       | 112 ++++++++++++++++++
> >>>  1 file changed, 112 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml b/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
> >>> new file mode 100644
> >>> index 000000000000..89824d7c6be4
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
> >>> @@ -0,0 +1,112 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +# Copyright 2022 Unisoc Inc.
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: "http://devicetree.org/schemas/clock/sprd,ums512-clk.yaml#";
> >>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#";
> >>> +
> >>> +title: UMS512 Clock Control Unit Device Tree Bindings
> >>
> >> Remove "Device Tree Bindings". You could do the same also in the
> >> subject, because you repeat the prefix ("dt-bindings: clk: sprd: Add
> >> ums512 clock controller").
> >>
> >>> +
> >>> +maintainers:
> >>> +  - Orson Zhai <orsonzhai@xxxxxxxxx>
> >>> +  - Baolin Wang <baolin.wang7@xxxxxxxxx>
> >>> +  - Chunyan Zhang <zhang.lyra@xxxxxxxxx>
> >>> +
> >>> +properties:
> >>> +  "#clock-cells":
> >>> +    const: 1
> >>> +
> >>> +  compatible:
> >>
> >> Put the compatible first, by convention and makes finding matching
> >> bindings easier.
> >>
> >>> +    oneOf:
> >>> +      - items:
> >>> +          - const: sprd,ums512-glbregs
> >>> +          - const: syscon
> >>> +          - const: simple-mfd
> >>
> >> Why do you need simple-mfd for these? This looks like a regular syscon,
> >> so usually does not come with children. What is more, why this "usual
> >> syscon" is a separate clock controller in these bindings?
> > there is a warning log before add these const.  and the reason we need
> > the simply-mfd
> > is some clock is a child of syscon node,which should set these compatible.
> > failed to match any schema with compatible: ['sprd,ums512-glbregs',
> > 'syscon', 'simple-mfd']
>
> Neither here nor later you did not answer the question - why do you need
> such complex construction, instead of adding syscon to the clock controller?
>
> Let me paste again my concerns:
>
>   You have nodes with reg but without unit address ("rpll"). These nodes
>   are modeled as children but they are not children - it's a workaround
>   for exposing syscon, isn't it? The sc9863a looks like broken design,
>   so please do not duplicate it here.
>
> IOW, sc9863a uses similar pattern as here and the DTS is made wrong.
> Because of this you need to create complex ways to get the regmap for
> the clock controller... Why not making it simple? Clock controller with
> syscon?

I find the history discuss about the sp9863 clock[1] and last
ums512-clk dt-bindings patch[2] which from chunyan.
please refer to the reasons below.

These clocks are at the same register range with global registers.
the registers shared with more than one devices  which  basically
are multimedia devices. You may noticed that these are all gate
clocks which are in the global registers  ranges and are used to
controll the enable status of some devices or some part of devices.

[1] https://lore.kernel.org/all/CAAfSe-s0gcehu0ZDj=FTe5S7CzAHC5mahXBH2fJm7mXS7Xys1Q@xxxxxxxxxxxxxx/#r
[2] https://lore.kernel.org/all/163425295208.1688384.11023187625793114662@xxxxxxxxxxxxxxxxxxxxxxxxxx/#r

>
> >>
> >>> +      - items:
> >>> +          - enum:
> >>> +              - sprd,ums512-apahb-gate
> >>> +              - sprd,ums512-ap-clk
> >>> +              - sprd,ums512-aonapb-clk
> >>> +              - sprd,ums512-pmu-gate
> >>> +              - sprd,ums512-g0-pll
> >>> +              - sprd,ums512-g2-pll
> >>> +              - sprd,ums512-g3-pll
> >>> +              - sprd,ums512-gc-pll
> >>> +              - sprd,ums512-aon-gate
> >>> +              - sprd,ums512-audcpapb-gate
> >>> +              - sprd,ums512-audcpahb-gate
> >>> +              - sprd,ums512-gpu-clk
> >>> +              - sprd,ums512-mm-clk
> >>> +              - sprd,ums512-mm-gate-clk
> >>> +              - sprd,ums512-apapb-gate
> >>> +
> >>> +  clocks:
> >>> +    minItems: 1
> >>
> >> maxItems needed
> > the previous version did has the maxitems, but makes error when run
> > 'make DT_CHECKER_FLAGS=-m dt_binding_check O=./dt-out  \
> > DT_SCHEMA_FILES=Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml'
> >   CHKDT   Documentation/devicetree/bindings/processed-schema.json
> > /path-to-linux/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml:
> > properties:clock-names: {'minItems': 1, 'maxItems': 4, 'items':
> > [{'const': 'ext-26m'}, {'const': 'ext-32k'}, {'const': 'ext-4m'},
> > {'const': 'rco-100m'}]} should not be valid under {'required':
> > ['maxItems']}
> > hint: "maxItems" is not needed with an "items" list
>
> Warning is about clock-names, not about clocks. Fix the bindings.
>
> >
> >>
> >>> +    description: |
> >>> +      The input parent clock(s) phandle for this clock, only list fixed
> >>> +      clocks which are declared in devicetree.
> >>
> >> The description does not make sense. These are bindings for a clock
> >> controller, but you say here "for this clock", so what does "this" mean
> >> here?
> >>
> >>> +
> >>> +  clock-names:
> >>> +    minItems: 1
> >>> +    items:
> >>> +      - const: ext-26m
> >>> +      - const: ext-32k
> >>> +      - const: ext-4m
> >>> +      - const: rco-100m
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +  - '#clock-cells'
> >>
> >> Isn't reg also required? Always? Do you have examples where it is not
> >> required? How do you configure the clocks without "reg"? I see you
> >> copied a lot from sprd,sc9863a-clk.yaml but that file does not look correct.
> >>
> >> You have nodes with reg but without unit address ("rpll"). These nodes
> >> are modeled as children but they are not children - it's a workaround
> >> for exposing syscon, isn't it? The sc9863a looks like broken design, so
> >> please do not duplicate it here.
> >>
> >>> +
> >>> +if:
> >>> +  properties:
> >>> +    compatible:
> >>> +      enum:
> >>> +        - sprd,ums512-ap-clk
> >>> +        - sprd,ums512-aonapb-clk
> >>> +        - sprd,ums512-mm-clk
> >>> +then:
> >>> +  required:
> >>> +    - reg
> >>> +
> >>> +else:
> >>> +  description: |
> >>> +    Other UMS512 clock nodes should be the child of a syscon node in
> >>> +    which compatible string should be:
> >>> +            "sprd,ums512-glbregs", "syscon", "simple-mfd"
> >>> +
> >>> +    The 'reg' property for the clock node is also required if there is a sub
> >>> +    range of registers for the clocks.
> >>> +
> >>> +additionalProperties: true
> >>
> >> false
> > the "false" makes error log:
> > /path-to-linux/Documentation/devicetree/bindings/clock/sprd,ums512-clk.example.dtb:
> > syscon@71000000: '#address-cells', '#size-cells',
> > 'clock-controller@0', 'ranges' do not match any of the regexes:
> > 'pinctrl-[0-9]+'
> > and I reference the patch
> > [https://www.spinics.net/lists/linux-leds/msg17032.html]
>
> Which needs fixing. The "false" is a strict requirement for such end
> (non-extendable) bindings.
>
> >
> >>
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +    ap_clk: clock-controller@20200000 {
> >>> +      compatible = "sprd,ums512-ap-clk";
> >>> +      reg = <0x20200000 0x1000>;
> >>> +      clocks = <&ext_26m>;
> >>> +      clock-names = "ext-26m";
> >>> +      #clock-cells = <1>;
> >>> +    };
> >>> +
> >>> +  - |
> >>> +    ap_apb_regs: syscon@71000000 {
> >>> +      compatible = "sprd,ums512-glbregs", "syscon", "simple-mfd";
> >>
> >> So here is the answer why you added MFD, but I still don't get why do
> >> you need it for one child? It is quite a dance here and in your drivers,
> >> instead of adding "syscon" to your clock controller.
> >
> > [1]in the if/else describtion,  th other UMS512 clock nodes should be
> > the child of a syscon node in
> > which compatible string should be:   "sprd,ums512-glbregs", "syscon",
> > "simple-mfd"
>
> No, it should not. Fix the drivers, fix the DTS and the bindings. Then
> the "should" disappears.
> >
> >>
> >> This also pollutes the actual bindings because you did not add
> >> properties required for clock controllers, because of describing here
> >> the syscon part.
>
>
>
> Best regards,
> Krzysztof