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

[Conor.Dooley]

On 19/08/2022 15:22, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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.

Not sure if you just passed over it, but I agree:
>> Truncated base address I suppose would be a meaningful thing
>> to fall back to afterwards.

But if the names aren't an ABI, then either there's not much point in
having the regex at all for clock-output-names or failing the check for
it does not matter. I'll have a think over the weekend about what
exactly to do, but I think the driver side of this is clear to me now &
what not to do in the binding is too.

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

If Alestorm made a board with our FPGA, I could see that..
I'd buy the t-shirt too!