Re: [PATCH v8 06/34] dt-bindings: clock: tegra-car: Document new tegra-clocks sub-node

[Dmitry Osipenko <digetx@xxxxxxxxx>]

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@xxxxxxxxx>
>> ---
>>  .../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.