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

[Chen Wang <unicorn_wang@outlook.com>]

On 2023/11/30 16:01, Krzysztof Kozlowski wrote:
> 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@outlook.com>
>>>>>>
>>>>>> Add a driver for the SOPHGO SG2042 clock generator.
>>>>>>
>>>>>> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
>>>>> ...
>>>>>
>>>>> +}
>>>>> +
>>>>> +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.
Yes, syscon is inappropriate. As per suggestion from Conor in anther 
email, I will define a sg2042 specific sysctl to handle this.
>
>> 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
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv