Re: [PATCH v3 3/4] riscv: dts: sophgo: add clock generator for Sophgo CV1800 series SoC

[Inochi Amaoto]

>
>On Fri, Dec 08, 2023 at 09:13:34AM +0800, Inochi Amaoto wrote:
>>> On Thu, Dec 07, 2023 at 01:52:16PM +0100, Krzysztof Kozlowski wrote:
>>>> On 07/12/2023 10:42, Inochi Amaoto wrote:
>>>>>>> +&clk {
>>>>>>> +	compatible = "sophgo,cv1810-clk";
>>>>>>> +};
>>>>>>> diff --git a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
>>>>>>> index 2d6f4a4b1e58..6ea1b2784db9 100644
>>>>>>> --- a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
>>>>>>> +++ b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
>>>>>>> @@ -53,6 +53,12 @@ soc {
>>>>>>>  		dma-noncoherent;
>>>>>>>  		ranges;
>>>>>>>
>>>>>>> +		clk: clock-controller@3002000 {
>>>>>>> +			reg = <0x03002000 0x1000>;
>>>>>>> +			clocks = <&osc>;
>>>>>>> +			#clock-cells = <1>;
>>>>>>
>>>>>> I don't find such layout readable and maintainable. I did some parts
>>>>>> like this long, long time ago for one of my SoCs (Exynos54xx), but I
>>>>>> find it over time unmaintainable approach. I strongly suggest to have
>>>>>> compatible and other properties in one place, so cv1800 and cv1812, even
>>>>>> if it duplicates the code.
>>>>>>
>>>>>
>>>>> Hi Krzysztof:
>>>>>
>>>>> Thanks for your advice, but I have a question about this: when I should
>>>>> use the DT override? The memory mapping of the CV1800 and CV1810 are
>>>>> almost the same (the CV1810 have more peripheral and the future SG200X
>>>>> have the same layout). IIRC, this is why conor suggested using DT override
>>>>> to make modification easier. But duplicating node seems to break thiS, so
>>>>> I's pretty confused.
>>>>
>>>> Go with whatever your subarchitecture and architecture maintainers
>>>> prefer, I just shared my opinion that I find such code difficult to read
>>>> and maintain.
>>>>
>>>> Extending node with supplies, pinctrl or even clocks would be readable.
>>>> But the compatible: no. The same applies when you need to delete
>>>> property or subnode: not readable/maintainable IMHO.
>>>
>>> There are apparently 3 or 4 of these SoCs that are basically identical,
>>> which is why the approach was taken. I do agree that it looks somewhat
>>> messy because I was looking for device-specific compatibles for these
>>> SoCs.
>>>
>>
>> I agree that this may be messy. But it might still be acceptable if we
>> limit the number of devices in this format.
>>
>> IIRC, only clint, plic, clk, maybe pinmux only needs different compatible.
>> For more complex device, such as tpu and codec, I agree with duplicating
>> nodes and make them SoC specific.
>
>Okay. We will see how it goes. We are not stuck doing it one way, we can
>revisit the decision later if things start to be confusing.
>
>>
>> As for this patch, I have already adjusted the order of clock to ensure
>> the compatible among different SoCs. This will make the clock assignment
>> easier.
>
>
>On Fri, Dec 08, 2023 at 09:13:34AM +0800, Inochi Amaoto wrote:
>>> On Thu, Dec 07, 2023 at 01:52:16PM +0100, Krzysztof Kozlowski wrote:
>>>> On 07/12/2023 10:42, Inochi Amaoto wrote:
>>>>>>> +&clk {
>>>>>>> +	compatible = "sophgo,cv1810-clk";
>>>>>>> +};
>>>>>>> diff --git a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
>>>>>>> index 2d6f4a4b1e58..6ea1b2784db9 100644
>>>>>>> --- a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
>>>>>>> +++ b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
>>>>>>> @@ -53,6 +53,12 @@ soc {
>>>>>>>  		dma-noncoherent;
>>>>>>>  		ranges;
>>>>>>>
>>>>>>> +		clk: clock-controller@3002000 {
>>>>>>> +			reg = <0x03002000 0x1000>;
>>>>>>> +			clocks = <&osc>;
>>>>>>> +			#clock-cells = <1>;
>>>>>>
>>>>>> I don't find such layout readable and maintainable. I did some parts
>>>>>> like this long, long time ago for one of my SoCs (Exynos54xx), but I
>>>>>> find it over time unmaintainable approach. I strongly suggest to have
>>>>>> compatible and other properties in one place, so cv1800 and cv1812, even
>>>>>> if it duplicates the code.
>>>>>>
>>>>>
>>>>> Hi Krzysztof:
>>>>>
>>>>> Thanks for your advice, but I have a question about this: when I should
>>>>> use the DT override? The memory mapping of the CV1800 and CV1810 are
>>>>> almost the same (the CV1810 have more peripheral and the future SG200X
>>>>> have the same layout). IIRC, this is why conor suggested using DT override
>>>>> to make modification easier. But duplicating node seems to break thiS, so
>>>>> I's pretty confused.
>>>>
>>>> Go with whatever your subarchitecture and architecture maintainers
>>>> prefer, I just shared my opinion that I find such code difficult to read
>>>> and maintain.
>>>>
>>>> Extending node with supplies, pinctrl or even clocks would be readable.
>>>> But the compatible: no. The same applies when you need to delete
>>>> property or subnode: not readable/maintainable IMHO.
>>>
>>> There are apparently 3 or 4 of these SoCs that are basically identical,
>>> which is why the approach was taken. I do agree that it looks somewhat
>>> messy because I was looking for device-specific compatibles for these
>>> SoCs.
>>>
>>
>> I agree that this may be messy. But it might still be acceptable if we
>> limit the number of devices in this format.
>>
>> IIRC, only clint, plic, clk, maybe pinmux only needs different compatible.
>> For more complex device, such as tpu and codec, I agree with duplicating
>> nodes and make them SoC specific.
>
>Okay. We will see how it goes. We are not stuck doing it one way, we can
>revisit the decision later if things start to be confusing.
>

Yes, now let's see what will happen and then improve it.

>>
>> As for this patch, I have already adjusted the order of clock to ensure
>> the compatible among different SoCs. This will make the clock assignment
>> easier.
>