From: Alex Elder <elder@riscstar.com>
To: Yixun Lan <dlan@gentoo.org>, Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzk+dt@kernel.org>,
Conor Dooley <conor+dt@kernel.org>,
Paul Walmsley <paul.walmsley@sifive.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
Albert Ou <aou@eecs.berkeley.edu>,
Alexandre Ghiti <alex@ghiti.fr>
Cc: Haylen Chu <heylenay@4d2.org>,
devicetree@vger.kernel.org, linux-riscv@lists.infradead.org,
spacemit@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] riscv: dts: spacemit: Acquire clocks for UART
Date: Mon, 21 Apr 2025 12:40:17 -0500 [thread overview]
Message-ID: <2d086fee-47d5-4be2-8b86-bf1cac6af803@riscstar.com> (raw)
In-Reply-To: <20250419-05-dts-clock-v1-2-1cce5d59aba2@gentoo.org>
On 4/18/25 10:32 PM, Yixun Lan wrote:
> The K1 SoC features two clocks for UART controller,
> Acquire them explicitly in the driver.
>
> Signed-off-by: Yixun Lan <dlan@gentoo.org>
I had an almost identical patch queued up to do this.
I think I'd mention explicitly in this description that you
are removing the clock-frequency property from all these nodes
(it is required to do this, otherwise the clock properties are
ignored by of_platform_serial_setup() in "8250_of.c").
Two more requests below. Otherwise this looks good.
If you address all three of my comments, feel free to add:
Reviewed-by: Alex Elder <elder@riscstar.com>
> ---
> arch/riscv/boot/dts/spacemit/k1.dtsi | 36 +++++++++++++++++++++++++++---------
> 1 file changed, 27 insertions(+), 9 deletions(-)
>
> diff --git a/arch/riscv/boot/dts/spacemit/k1.dtsi b/arch/riscv/boot/dts/spacemit/k1.dtsi
> index 153fd1160182b42fe1a2f7f042c9c1da90f63b0c..415e1c3e1c78db987cbb65759adc26e98aaa24d3 100644
> --- a/arch/riscv/boot/dts/spacemit/k1.dtsi
> +++ b/arch/riscv/boot/dts/spacemit/k1.dtsi
> @@ -360,8 +360,10 @@ syscon_apbc: system-control@d4015000 {
> uart0: serial@d4017000 {
> compatible = "spacemit,k1-uart", "intel,xscale-uart";
> reg = <0x0 0xd4017000 0x0 0x100>;
> + clocks = <&syscon_apbc CLK_UART0>,
> + <&syscon_apbc CLK_UART0_BUS>;
> + clock-names = "core", "bus";
> interrupts = <42>;
> - clock-frequency = <14857000>;
> reg-shift = <2>;
> reg-io-width = <4>;
> status = "disabled";
> @@ -370,8 +372,10 @@ uart0: serial@d4017000 {
> uart2: serial@d4017100 {
> compatible = "spacemit,k1-uart", "intel,xscale-uart";
> reg = <0x0 0xd4017100 0x0 0x100>;
> + clocks = <&syscon_apbc CLK_UART2>,
> + <&syscon_apbc CLK_UART2_BUS>;
> + clock-names = "core", "bus";
> interrupts = <44>;
> - clock-frequency = <14857000>;
> reg-shift = <2>;
> reg-io-width = <4>;
> status = "disabled";
> @@ -380,8 +384,10 @@ uart2: serial@d4017100 {
> uart3: serial@d4017200 {
> compatible = "spacemit,k1-uart", "intel,xscale-uart";
> reg = <0x0 0xd4017200 0x0 0x100>;
> + clocks = <&syscon_apbc CLK_UART3>,
> + <&syscon_apbc CLK_UART3_BUS>;
> + clock-names = "core", "bus";
> interrupts = <45>;
> - clock-frequency = <14857000>;
> reg-shift = <2>;
> reg-io-width = <4>;
> status = "disabled";
> @@ -390,8 +396,10 @@ uart3: serial@d4017200 {
> uart4: serial@d4017300 {
> compatible = "spacemit,k1-uart", "intel,xscale-uart";
> reg = <0x0 0xd4017300 0x0 0x100>;
> + clocks = <&syscon_apbc CLK_UART4>,
> + <&syscon_apbc CLK_UART4_BUS>;
> + clock-names = "core", "bus";
> interrupts = <46>;
> - clock-frequency = <14857000>;
> reg-shift = <2>;
> reg-io-width = <4>;
> status = "disabled";
> @@ -400,8 +408,10 @@ uart4: serial@d4017300 {
> uart5: serial@d4017400 {
> compatible = "spacemit,k1-uart", "intel,xscale-uart";
> reg = <0x0 0xd4017400 0x0 0x100>;
> + clocks = <&syscon_apbc CLK_UART5>,
> + <&syscon_apbc CLK_UART5_BUS>;
> + clock-names = "core", "bus";
> interrupts = <47>;
> - clock-frequency = <14857000>;
> reg-shift = <2>;
> reg-io-width = <4>;
> status = "disabled";
> @@ -410,8 +420,10 @@ uart5: serial@d4017400 {
> uart6: serial@d4017500 {
> compatible = "spacemit,k1-uart", "intel,xscale-uart";
> reg = <0x0 0xd4017500 0x0 0x100>;
> + clocks = <&syscon_apbc CLK_UART6>,
> + <&syscon_apbc CLK_UART6_BUS>;
> + clock-names = "core", "bus";
> interrupts = <48>;
> - clock-frequency = <14857000>;
> reg-shift = <2>;
> reg-io-width = <4>;
> status = "disabled";
> @@ -420,8 +432,10 @@ uart6: serial@d4017500 {
> uart7: serial@d4017600 {
> compatible = "spacemit,k1-uart", "intel,xscale-uart";
> reg = <0x0 0xd4017600 0x0 0x100>;
> + clocks = <&syscon_apbc CLK_UART7>,
> + <&syscon_apbc CLK_UART7_BUS>;
> + clock-names = "core", "bus";
> interrupts = <49>;
> - clock-frequency = <14857000>;
> reg-shift = <2>;
> reg-io-width = <4>;
> status = "disabled";
> @@ -431,7 +445,9 @@ uart8: serial@d4017700 {
> compatible = "spacemit,k1-uart", "intel,xscale-uart";
> reg = <0x0 0xd4017700 0x0 0x100>;
> interrupts = <50>;
> - clock-frequency = <14857000>;
> + clocks = <&syscon_apbc CLK_UART8>,
> + <&syscon_apbc CLK_UART8_BUS>;
> + clock-names = "core", "bus";
Please insert the clocks and clock-names properties *above* the
interrupts property, as you did for all of the above.
> reg-shift = <2>;
> reg-io-width = <4>;
> status = "disabled";
> @@ -441,7 +457,9 @@ uart9: serial@d4017800 {
> compatible = "spacemit,k1-uart", "intel,xscale-uart";
> reg = <0x0 0xd4017800 0x0 0x100>;
> interrupts = <51>;
> - clock-frequency = <14857000>;
> + clocks = <&syscon_apbc CLK_UART9>,
> + <&syscon_apbc CLK_UART9_BUS>;
> + clock-names = "core", "bus";
Same comment here.
-Alex
> reg-shift = <2>;
> reg-io-width = <4>;
> status = "disabled";
>
next prev parent reply other threads:[~2025-04-21 17:40 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-04-19 3:32 [PATCH 0/2] riscv: dts: spacemit: Add clocks to pinctrl and UART Yixun Lan
2025-04-19 3:32 ` [PATCH 1/2] riscv: dts: spacemit: Acquire clocks for pinctrl Yixun Lan
2025-04-21 17:40 ` Alex Elder
2025-04-19 3:32 ` [PATCH 2/2] riscv: dts: spacemit: Acquire clocks for UART Yixun Lan
2025-04-21 17:40 ` Alex Elder [this message]
2025-04-19 9:42 ` [PATCH 0/2] riscv: dts: spacemit: Add clocks to pinctrl and UART Haylen Chu
2025-04-19 10:08 ` Yixun Lan
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=2d086fee-47d5-4be2-8b86-bf1cac6af803@riscstar.com \
--to=elder@riscstar.com \
--cc=alex@ghiti.fr \
--cc=aou@eecs.berkeley.edu \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dlan@gentoo.org \
--cc=heylenay@4d2.org \
--cc=krzk+dt@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
--cc=robh@kernel.org \
--cc=spacemit@lists.linux.dev \
/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
all inboxes | Powered by JetHome®