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

[Conor Dooley]

On Thu, Nov 30, 2023 at 02:37:53PM +0800, 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.
> 
> 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]

In that version of the code, clkgen, your DTS, looked like:
+	clkgen: clock-controller {
+		compatible = "sophgo,sg2042-clkgen";
+		#clock-cells = <1>;
+		system-ctrl = <&sys_ctrl>;
+		clocks = <&cgi>;
+		assigned-clocks = \

+		assigned-clock-rates = \

+	};

It had no register regions of its own, just what it got from the sys
ctrl block, which is why I said that. The syscon block looked like:

+		sys_ctrl: syscon@7030010000 {
+			compatible = "sophgo,sg2042-syscon", "syscon";
+			reg = <0x70 0x30010000 0x0 0x8000>;
+		};

which given the register map does not seem like an accurate reflection
of the size of this region. The "0x8000" should be "0x1000".
> 
> 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.

This sounds two me like there are two different devices. One the "CLOCK"
region at 070:3001:2000 that should be documented as being
"sophgo,sg2042-clkgen" or similar and the second being the "SYS_CTRL" at
070:3001:0000 that is called something like "sophgo,sg2042-sysctrl".
Having more than one clock controller is not a problem and sounds like a
more accurate description of the hardware.

> 
> What do you think or do you have any good suggestions?
> 
> Link: https://lore.kernel.org/linux-riscv/20231114-timid-habitat-a06e52e59c9c@squawk/#t
> [1]
> 
> Thanks
> 
> Chen
> 
> > 
> > > this driver is still for platform device though I move the clock
> > > controller node as a child of the system contoller node. System
> > > controller node is just a block of registers which are used to control
> > > some other platform devices ,such as clock controller, reset controller
> > > and pin controller for this SoC.
> > > 
> > > And I also see other similar code in kernel, for example:
> > > drivers/clk/clk-k210.c.
> > > 
> > > And I'm confused by your input "so you cannot use other way of init
> > > ordering." Do you mean "so you CAN use other way of init ordering"?
> > No, I meant you cannot. If you want to use syscon, then your driver
> > should be a proper driver. Therefore add a driver.
> > 
> > > What's the other way of init ordering do you mean?
> > The one coming not from initcalls but driver model.
> > 
> > Best regards,
> > Krzysztof
> >

Attachment: signature.asc
Description: PGP signature