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? Best regards, Krzysztof
