Date:   Wed, 18 Aug 2021 18:05:11 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Thierry Reding <thierry.reding@...il.com>
Cc:     Jonathan Hunter <jonathanh@...dia.com>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Viresh Kumar <vireshk@...nel.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Peter De Schrijver <pdeschrijver@...dia.com>,
        Mikko Perttunen <mperttunen@...dia.com>,
        Peter Chen <peter.chen@...nel.org>,
        Mark Brown <broonie@...nel.org>,
        Lee Jones <lee.jones@...aro.org>,
        Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
        Nishanth Menon <nm@...com>,
        Vignesh Raghavendra <vigneshr@...com>,
        Richard Weinberger <richard@....at>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Lucas Stach <dev@...xeye.de>, Stefan Agner <stefan@...er.ch>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-pm@...r.kernel.org, linux-usb@...r.kernel.org,
        linux-staging@...ts.linux.dev, linux-spi@...r.kernel.org,
        linux-pwm@...r.kernel.org, linux-mtd@...ts.infradead.org,
        linux-mmc@...r.kernel.org, linux-media@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
        linux-clk@...r.kernel.org
Subject: Re: [PATCH v8 06/34] dt-bindings: clock: tegra-car: Document new
 tegra-clocks sub-node

18.08.2021 16:59, Thierry Reding пишет:
> On Tue, Aug 17, 2021 at 04:27:26AM +0300, Dmitry Osipenko wrote:
>> Document tegra-clocks sub-node which describes Tegra SoC clocks that
>> require a higher voltage of the core power domain in order to operate
>> properly on a higher clock rates.  Each node contains a phandle to OPP
>> table and power domain.
>>
>> The root PLLs and system clocks don't have any specific device dedicated
>> to them, clock controller is in charge of managing power for them.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
>> ---
>>  .../bindings/clock/nvidia,tegra20-car.yaml    | 51 +++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml
>> index 459d2a525393..7f5cd27e4ce0 100644
>> --- a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml
>> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml
>> @@ -42,6 +42,48 @@ properties:
>>    "#reset-cells":
>>      const: 1
>>  
>> +  tegra-clocks:
>> +    description: child nodes are the output clocks from the CAR
>> +    type: object
>> +
>> +    patternProperties:
>> +      "^[a-z]+[0-9]+$":
>> +        type: object
>> +        properties:
>> +          compatible:
>> +            allOf:
>> +              - items:
>> +                  - enum:
>> +                      - nvidia,tegra20-sclk
>> +                      - nvidia,tegra30-sclk
>> +                      - nvidia,tegra30-pllc
>> +                      - nvidia,tegra30-plle
>> +                      - nvidia,tegra30-pllm
>> +              - const: nvidia,tegra-clock
>> +
>> +          operating-points-v2:
>> +            $ref: /schemas/types.yaml#/definitions/phandle
>> +            description:
>> +              Phandle to OPP table that contains frequencies, voltages and
>> +              opp-supported-hw property, which is a bitfield indicating
>> +              SoC process or speedo ID mask.
>> +
>> +          clocks:
>> +            items:
>> +              - description: node's clock
>> +
>> +          power-domains:
>> +            maxItems: 1
>> +            description: phandle to the core SoC power domain
>> +
>> +        required:
>> +          - compatible
>> +          - operating-points-v2
>> +          - clocks
>> +          - power-domains
>> +
>> +        additionalProperties: false
>> +
>>  required:
>>    - compatible
>>    - reg
>> @@ -59,6 +101,15 @@ examples:
>>          reg = <0x60006000 0x1000>;
>>          #clock-cells = <1>;
>>          #reset-cells = <1>;
>> +
>> +        tegra-clocks {
>> +            sclk {
>> +                compatible = "nvidia,tegra20-sclk", "nvidia,tegra-clock";
>> +                operating-points-v2 = <&opp_table>;
>> +                clocks = <&tegra_car TEGRA20_CLK_SCLK>;
>> +                power-domains = <&domain>;
>> +            };
>> +        };
> 
> I wonder if it'd be better to match on the name of the node rather than
> add an artificial compatible string. We usually use the compatible
> string to match a device, but here you're really trying to add
> information about a resource provided by the CAR controller.
> 
> We do similar things for example in PMIC bindings where the individual
> regulators are represented in the device tree via nodes named after the
> regulator.
> 
> You could then also leave out the clocks property, which is weird as it
> is because it's basically a self-reference. But you don't really need
> the reference here in the first place because the CAR is already the
> parent of SCLK.

We don't have a platform device for CaR. I don't see how it's going to
work. We need to create a platform device for each RPM-capable clock
because that's how RPM works. The compatible string is required for
instantiating OF-devices from a node, otherwise we will have to
re-invent the OF core.

> Also, I don't think the tegra- prefix is necessary here. The parent node
> is already identified as Tegra via the compatible string.
> 
> In the case of CAR, I'd imagine something like:
> 
> 	clocks {
> 		sclk {
> 			operating-points-v2 = <&opp_table>;
> 			power-domains = <&domain>;
> 		};
> 	};
> 
> Now you've only got the bare minimum in here that you actually add. All
> the other data that you used to have is simply derived from the parent.

'clocks' is already a generic keyword in DT. It's probably not okay to
redefine it.

Powered by blists - more mailing lists