Re: [PATCH 1/3] dt: adin: document clk-out property

[Josua Mayer <josua@solid-run.com>]

\o/

Am 10.04.22 um 22:01 schrieb Krzysztof Kozlowski:
> On 10/04/2022 20:41, Josua Mayer wrote:
>>> Adjust subject prefix to the subsystem (dt-bindings, not dt, missing net).
>> Ack. So something like
>> dt-bindings: net: adin: document clk-out property
> Yes.
Great, I will have it changed in a future revision!
>>>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/net/adi,adin.yaml | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
>>>> index 1129f2b58e98..4e421bf5193d 100644
>>>> --- a/Documentation/devicetree/bindings/net/adi,adin.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
>>>> @@ -36,6 +36,11 @@ properties:
>>>>        enum: [ 4, 8, 12, 16, 20, 24 ]
>>>>        default: 8
>>>>    
>>>> +  adi,clk-out-frequency:
>>> Use types defined by the dtschema, so "adi,clk-out-hz". Then no need for
>>> type/ref.
>> That sounds useful, I was not aware. The only inspiration I used was the
>> at803x driver ...
>> It seemed natural to share the property name as it serves the same
>> purpose here.
> Indeed ar803x uses such property. In general reusing properties is a
> good idea, but not all properties are good enough to copy. I don't know
> why adi,clk-out-frequency got accepted because we really stick to common
> units when possible.
>
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
>
>>>> +    description: Clock output frequency in Hertz.
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    enum: [125000000]
>>> If only one value, then "const: 125000000", but why do you need such
>>> value in DT if it is always the same?
>> Yes yes yes.
>>   From my understanding of the adin1300 data-sheet, it can provide either
>> a 25MHz clock or a 125MHz clock on its GP_CLK pin. So for the context of
>> this feature we would have two options. However because we found the
>> documentation very confusing we skipped the 25MHz option.
>>
>> Actually my statement above omits some of the options.
>> - There are actually two 125MHz clocks, the first called "recovered" and
>> the second "free running".
>> - One can let the phy choose the rate based on its internal state.
>> This is indicated on page 73 of the datasheet
>> (https://www.analog.com/media/en/technical-documentation/data-sheets/adin1300.pdf)
>>
>> Because of this confusion we wanted to for now only allow selecting the
>> free-running 125MHz clock.
> Hm, so you do not insist on actual frequency but rather type of the
> clock (freerunning instead of recovered and 25 MHz). Then the frequency
> does not look enough because it does not offer you the choice of clock
> (freerunning or recovered) and instead you could have enum like:
>    adi,phy-output-clock:
>    $ref: /schemas/types.yaml#/definitions/string
>    enum: [125mhz-freerunning, 125mhz-recovered, 25mhz-freeruning....]
>
> Judging by page 24 you have 5 or more options... This could be also
> numeric ID (enum [0, 1, 2, 3 ...]) with some explanation, but strings
> seem easier to understand.

I agree that strings are more meaningful here, especially considering 
how each entry carries at least two pieces of information.
If we are not to reuse the qca,clk-out-frequency name, then an enum 
seems the easiest way to describe the available settings from the clock 
config register!

> The binding should describe the hardware, not implementation in Linux,
> therefore you should actually list all possible choices. The driver then
> can just return EINVAL on unsupported choices (or map them back to only
> one supported).

I have prepared a draft for the entries that should exist, it covers 
five of the 6 available bits. Maybe you can comment if this is 
understandable?

   adi,phy-output-clock:
     description: Select clock output on GP_CLK pin. Three clocks are 
available:
       A 25MHz reference, a free-running 125MHz and a recovered 125MHz.
       The phy can also automatically switch between the reference and the
       respective 125MHz clocks based on its internal state.
     $ref: /schemas/types.yaml#/definitions/string
     enum:
     - 25mhz-reference
     - 125mhz-free-running
     - 125mhz-recovered
     - adaptive-free-running
     - adaptive-recovered

Bit no. 3 (GE_REF_CLK_EN) is special in that it can be enabled 
independently from the 5 choices above,
and it controls a different pin. Therefore it deserves its own property, 
perhaps a flag or boolean adi,phy-output-ref-clock.
Any opinion if this should be added, or we can omit it completely?

sincerely
Josua Mayer