Re: [PATCH 6/6] riscv: dts: microchip: add the mpfs' fabric clock control

[Krzysztof Kozlowski]

On 19/08/2022 17:14, Conor.Dooley@xxxxxxxxxxxxx wrote:
> On 19/08/2022 15:06, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 19/08/2022 16:48, Conor.Dooley@xxxxxxxxxxxxx wrote:
>>> On 19/08/2022 14:28, Krzysztof Kozlowski wrote:
>>>>> Maybe that is me exploiting the "should", but I was not sure how to
>>>>> include the location in the devicetree.
>>>>
>>>> Neither node names nor clock names are considered an ABI, but some
>>>> pieces like to rely on them. Now you created such dependency so imagine
>>>> someone prepares a DTSI/DTS with "clock-controller" names for all four
>>>> blocks. How you driver would behave?
>>>
>>> -EEXIST, registration fails in the core.
>>>
>>>> The DTS would be perfectly valid but driver would not accept it
>>>> (conflicting names) or behave incorrect.
>>>>
>>>> I think what you need is the clock-output-names property. The core
>>>> schema dtschema/schemas/clock/clock.yaml recommends unified
>>>> interpretation of it - list of names for all the clocks - but accepts
>>>> other uses, e.g. as a prefix.
>>>
>>> So could I do `clock-output-names = "ccc_nw";`. That would work for me,
>>> with one question:
>>> How would I enforce the unique-ness of this property, since it would be
>>> a per CCC/clock-controller property? Maybe I missed something, but I
>>> gave it a shot with two different CCC nodes having "ccc_nw" & dtbs_check
>>> did not complain. Up to me to explain the restriction in the dt-bindings
>>> description?
>>
>> Uniqueness among entire DTS? I don't think you can, except of course
>> mentioning it in description. Your driver should handle such DTS -
>> minimally by gracefully failing but better behaving in some default way.
> 
> It fails not-too-gracefully at the moment, but that could easily be
> changed. Truncated base address I suppose would be a meaningful thing
> to fall back to afterwards.
> 
>>
>>>
>>> FWIW I would then have:
>>> ccc_sw: clock-controller@38400000 {
>>>        compatible = "microchip,mpfs-ccc";
>>>        reg = <0x0 0x38400000 0x0 0x1000>, <0x0 0x38800000 0x0 0x1000>,
>>>              <0x0 0x39400000 0x0 0x1000>, <0x0 0x39800000 0x0 0x1000>;
>>>        #clock-cells = <1>;
>>>        clock-output-names = "ccc_sw";
>>>        status = "disabled";
>>> };
>>>
>>> & in the binding:
>>>     clock-output-names:
>>>       pattern: ^ccc_[ns][ew]$
>>
>> Yes, although this won't enforce uniqueness.
> 
> I know :( I'll respin next week I guess, thanks again.


The issue with this is that we are getting close to tying bindings with
your Linux implementation. What if other implementation does not use
these names as prefix and instead creates some other clock names (as
clock names are not considered ABI)? Your binding would still enforce
such property with a specific pattern.

The clock names should not really matter, so if you have conflict of
names among multiple controllers, I think driver should embed unit
address in the name (as fallback of clock-output-name) and the binding
should not enforce specific pattern.

I can easily imagine a real hardware board design with
"sexy_duck_ccc_pll1_out3" clock names. :)

Best regards,
Krzysztof