From: Conor Dooley <conor@kernel.org>
To: Chen Wang <unicorn_wang@outlook.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
Chen Wang <unicornxw@gmail.com>,
aou@eecs.berkeley.edu, chao.wei@sophgo.com,
krzysztof.kozlowski+dt@linaro.org, mturquette@baylibre.com,
palmer@dabbelt.com, paul.walmsley@sifive.com,
richardcochran@gmail.com, robh+dt@kernel.org, sboyd@kernel.org,
devicetree@vger.kernel.org, linux-clk@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org,
haijiao.liu@sophgo.com, xiaoguang.xing@sophgo.com,
guoren@kernel.org, jszhang@kernel.org, inochiama@outlook.com,
samuel.holland@sifive.com
Subject: Re: [PATCH v2 3/4] clk: sophgo: Add SG2042 clock generator driver
Date: Thu, 30 Nov 2023 08:12:34 +0000 [thread overview]
Message-ID: <20231130-enlarging-decode-31dc66f4490b@spud> (raw)
In-Reply-To: <MA0P287MB03320824AB953465E00394FEFE82A@MA0P287MB0332.INDP287.PROD.OUTLOOK.COM>
[-- Attachment #1.1: Type: text/plain, Size: 5558 bytes --]
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@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.
>
> 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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 161 bytes --]
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2023-11-30 8:12 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-27 0:57 [PATCH v2 0/4] riscv: sophgo: add clock support for sg2042 Chen Wang
2023-11-27 0:58 ` [PATCH v2 1/4] dt-bindings: clock: sophgo: Add SG2042 bindings Chen Wang
2023-11-27 7:08 ` Krzysztof Kozlowski
2023-11-27 0:58 ` [PATCH v2 2/4] dt-bindings: soc: sophgo: Add Sophgo syscon module Chen Wang
2023-11-27 7:09 ` Krzysztof Kozlowski
2023-11-27 1:15 ` [PATCH v2 3/4] clk: sophgo: Add SG2042 clock generator driver Chen Wang
2023-11-27 7:12 ` Krzysztof Kozlowski
2023-11-27 8:07 ` Chen Wang
2023-11-27 9:16 ` Krzysztof Kozlowski
2023-11-30 6:37 ` Chen Wang
2023-11-30 8:01 ` Krzysztof Kozlowski
2023-11-30 11:42 ` Chen Wang
2023-11-30 8:12 ` Conor Dooley [this message]
2023-11-30 11:32 ` Chen Wang
2023-11-27 1:16 ` [PATCH v2 4/4] riscv: dts: add clock generator for Sophgo SG2042 SoC Chen Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231130-enlarging-decode-31dc66f4490b@spud \
--to=conor@kernel.org \
--cc=aou@eecs.berkeley.edu \
--cc=chao.wei@sophgo.com \
--cc=devicetree@vger.kernel.org \
--cc=guoren@kernel.org \
--cc=haijiao.liu@sophgo.com \
--cc=inochiama@outlook.com \
--cc=jszhang@kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=krzysztof.kozlowski@linaro.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=mturquette@baylibre.com \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
--cc=richardcochran@gmail.com \
--cc=robh+dt@kernel.org \
--cc=samuel.holland@sifive.com \
--cc=sboyd@kernel.org \
--cc=unicorn_wang@outlook.com \
--cc=unicornxw@gmail.com \
--cc=xiaoguang.xing@sophgo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).