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

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Conor.Dooley@microchip.com, mturquette@baylibre.com,
	sboyd@kernel.org, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, palmer@dabbelt.com,
	Daire.McNamara@microchip.com
Cc: paul.walmsley@sifive.com, aou@eecs.berkeley.edu,
	linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org
Subject: Re: [PATCH 6/6] riscv: dts: microchip: add the mpfs' fabric clock control
Date: Fri, 19 Aug 2022 16:28:38 +0300	[thread overview]
Message-ID: <f3d8be5c-737b-8c71-9926-a4036c797769@linaro.org> (raw)
In-Reply-To: <3ffba600-bda9-8ffa-a435-9a6f94e072b8@microchip.com>

On 19/08/2022 16:15, Conor.Dooley@microchip.com wrote:
>>>
>>> +             ccc_se: cccseclk@38010000 {
>>
>> Although you call it "Clock Conditioning Circuitry", but the role of
>> this device is a clock-controller, isn't it? If so, node names should be
>> generic, so "clock-controller".
> 
> Thanks for the prompt reply Krzysztof!
> I suspected that this is what I was going to hear back. The reason I
> had used the non-generic node name is that I wanted to use it for the
> "name" of the clocks in the clock framework. As you can see, there are
> four instances of the same clock, and I am using the of_node's name to
> generate the unique names the clock framework requires, like so:
> 
> # cat clk_summary
>     clock
> -------------------------
>   cccrefclk
>      cccnwclk_pll1
>         cccnwclk_pll1_out3
>         cccnwclk_pll1_out2
>         cccnwclk_pll1_out1
>         cccnwclk_pll1_out0
>      cccnwclk_pll0
>         cccnwclk_pll0_out3
>         cccnwclk_pll0_out2
>         cccnwclk_pll0_out1
>         cccnwclk_pll0_out0
>      cccswclk_pll1
>         cccswclk_pll1_out3
>         cccswclk_pll1_out2
>         cccswclk_pll1_out1
>         cccswclk_pll1_out0
>      cccnsclk_pll0
>         cccswclk_pll0_out3
>         cccswclk_pll0_out2
>         cccswclk_pll0_out1
>         cccswclk_pll0_out0
> 
> 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?

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.

> 
> I had experimented with a "microchip,ordinal" or "microchip,location"
> string property to do the same thing but I thought you/Rob might not
> like that - is location/placement on the chip a relevant property of the
> hardware? I'd argue that for an FPGA, where the user is the one deciding
> what clocks what, it could be relevant to some degree.
> 
> Knowing if a CCC is the north-west one has some extra benefits as it
> is co-located with the PLLs for the processor & has a reduced input
> mux range.
> 
> Any suggestions would be appreciated, even if it is just a NAK to all of
> the above!

Best regards,
Krzysztof