Re: [PATCH v2 3/4] clk: sophgo: Add SG2042 clock generator driver
From: Chen Wang
Date: Thu Nov 30 2023 - 06:44:19 EST
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@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.
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