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
