Re: [PATCH v2 3/4] clk: sophgo: Add SG2042 clock generator driver

[Krzysztof Kozlowski]

On 30/11/2023 07:37, Chen Wang wrote:
> 
> On 2023/11/27 17:16, Krzysztof Kozlowski wrote:
>> On 27/11/2023 09:07, Chen Wang wrote:
>>> On 2023/11/27 15:12, Krzysztof Kozlowski wrote:
>>>> On 27/11/2023 02:15, Chen Wang wrote:
>>>>> From: Chen Wang <unicorn_wang@xxxxxxxxxxx>
>>>>>
>>>>> Add a driver for the SOPHGO SG2042 clock generator.
>>>>>
>>>>> Signed-off-by: Chen Wang <unicorn_wang@xxxxxxxxxxx>
>>>> ...
>>>>
>>>> +}
>>>> +
>>>> +CLK_OF_DECLARE(sg2042_clk, "sophgo,sg2042-clkgen", sg2042_clk_init);
>>>> No, this should be platform device. It's a child of another device, so
>>>> you cannot use other way of init ordering.
>>> hi, Krzysztof,
>>>
>>> Thanks for your review.
>>>
>>> I don't quite understand your opinion. Do you mean CLK_OF_DECLARE is
>>> only used for platform device so it can not be use here? But I think
>> No, I meant you mix init ordering: you depend now on syscon earlier
>> initcall than CLK_OF_DECLARE. Do you remember which one is first? If
>> anything changes here, your driver is broken. There is no dependency, no
>> probe deferral.
> 
> hi, Krzysztof,
> 
> I found that the initcall method cannot be used for the clock controller 
> of sg2042. We need to initialize the clock earlier because there are two 
> dw-apb-timers in sg2042 (Sorry, I have not added them in the current DTS 
> of sg2042, will be submitted later). The initialization of these timers 
> (timer_probe()) depends on the initialization of the clock controller. 
> If we use the initcall mechanism, it will be too late for the timer. So 
> it seems better to use CLK_OF_DECLARE provided by CCF.

Sure, that's fine, but don't use syscon in such case.

> 
> I have a question here that I would like to discuss. The design of 
> sg2042 is like this, according to the design of memorymap in its TRM:
> 
> 070:3001:0000 ~ 070:3001:0FFF SYS_CTRL 4K
> 070:3001:1000 ~ 070:3001:1FFF PINMUX 4K
> 070:3001:2000 ~ 070:3001:2FFF CLOCK 4K
> 070:3001:3000 ~ 070:3001:3FFF RESET 4K
> 
> But also as per hw design (I don't know why and I don't like it also :( 
> ), some of the PLL/GATE CLOCK control registers are defined in the scope 
> of SYS_CTRL, and others are defined in the scope of CLOCK. That's why in 
> the current code, I define the syscon node corresponding to SYS_CTRL. 
> The purpose is just to get the regmap of syscon for the clock controller 
> through the device tree (through device_node_to_regmap()), so that the 
> syscon defined in SYS_CTRL can be accessed through the regmap from 
> clock. The clock controller driver itself does not rely on other 
> operations of syscon.
> 
> So based on the above analysis, is it still necessary for us to define 
> the clock controller as a child node of syscon? In the version v1 of 
> this patch, I actually did not define the clock controller as a child 
> node of syscon, but only accessed syscon through the phandle method. [1]

I have impression you ask me if your solution is ok, but I already
pointed the problem. Address the problem - how do you enforce ordering
of syscon and CLK_OF_DECLARE? What initcalls are both?

> 
> After more read of the TRM, I believe this situation only exists for 
> clock. That is to say, there will be only one child node of clook under 
> syscon. From a hardware design perspective, CLOCK and SYS_CTRL are two 
> different blocks. So I think it is better to restore the original 
> method, that is, restore clock and syscon to nodes of the same level, 
> and let clock use phandle to access syscon.
> 
> What do you think or do you have any good suggestions?


Best regards,
Krzysztof