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

[Cixi Geng]

Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> 于2022年4月24日周日 19:21写道：
>
> On 24/04/2022 12:47, Cixi Geng wrote:
> > 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
>
> Which looks like discussion about different bindings. You had there a
> clock controller and additional clock device using "sprd,syscon". Why
> the rpll is a subdevice and not a part of clock controller. The same as
> all other clocks coming from that clock-controller, right? What is so
> special about rpll that is is a separate device, not part of the clock
> controller? It's the same address space, isn't it?
The hardware spec design these clocks are not belonged to the syscon,
the phandle is only used to get virtual  map address for clocks which
have the same phsical address base with one syscon.(I don't know the
historical reason for this design) It also can wroten a clock sperated from
syscon by add the reg which same as syscon. but will lead to a duplicate
mapping--one is from the clock,and one is from syscon. which make difficulty
 in analyzing some panic problems.

>
> Best regards,
> Krzysztof