[PATCH] arm64: dts: renesas: ulcb-kf: add HSCIF1 node

[PATCH] arm64: dts: renesas: ulcb-kf: add HSCIF1 node

[Wolfram Sang]

Exposed on CN4. Tested by connecting it to a Renesas Ebisu board. Also,
remove flow control for SCIF1. The schematics are misleading, the flow
control is for HSCIF1. SCIF1 (for GPS) does not use flow control.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

I extracted the removal of SCIF1 flow control from the GPS patches
because I think that actually belongs here.

 arch/arm64/boot/dts/renesas/ulcb-kf.dtsi | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi b/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi
index ff3a9ab6e6b0..e62f5359f64b 100644
--- a/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi
+++ b/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi
@@ -10,6 +10,7 @@ / {
 	aliases {
 		serial1 = &hscif0;
 		serial2 = &scif1;
+		serial3 = &hscif1;
 		mmc2 = &sdhi3;
 	};
 
@@ -132,6 +133,14 @@ &hscif0 {
 	status = "okay";
 };
 
+&hscif1 {
+	pinctrl-0 = <&hscif1_pins>;
+	pinctrl-names = "default";
+	uart-has-rtscts;
+
+	status = "okay";
+};
+
 &hsusb {
 	dr_mode = "otg";
 	status = "okay";
@@ -387,8 +396,13 @@ hscif0_pins: hscif0 {
 		function = "hscif0";
 	};
 
+	hscif1_pins: hscif1 {
+		groups = "hscif1_data_a", "hscif1_ctrl_a";
+		function = "hscif1";
+	};
+
 	scif1_pins: scif1 {
-		groups = "scif1_data_b", "scif1_ctrl";
+		groups = "scif1_data_b";
 		function = "scif1";
 	};
 
@@ -418,7 +432,6 @@ &sound_clk_pins
 &scif1 {
 	pinctrl-0 = <&scif1_pins>;
 	pinctrl-names = "default";
-	uart-has-rtscts;
 
 	status = "okay";
 };
-- 
2.30.2

Re: [PATCH] arm64: dts: renesas: ulcb-kf: add HSCIF1 node

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, May 24, 2023 at 10:37 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Exposed on CN4. Tested by connecting it to a Renesas Ebisu board. Also,
> remove flow control for SCIF1. The schematics are misleading, the flow
> control is for HSCIF1. SCIF1 (for GPS) does not use flow control.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for your patch!

It's actually a bit more complicated ;-)
CN4 can be served by either SCIF1 or HSCIF1, including flow control
for both.  But the current pin control setting for SCIF1 is wrong for
that case, as it should use scif1_data_a instead of scif1_data_b.

However, as the only serial port that can be muxed to the GPS pins
is SCIF1, we have no choice but to use SCIF1 for the GPS, and HSCIF1
for CN4, like your patch does.

> ---
>
> I extracted the removal of SCIF1 flow control from the GPS patches
> because I think that actually belongs here.

Yes it does, thanks!

> --- a/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi
> +++ b/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi
> @@ -10,6 +10,7 @@ / {
>         aliases {
>                 serial1 = &hscif0;
>                 serial2 = &scif1;
> +               serial3 = &hscif1;
>                 mmc2 = &sdhi3;
>         };
>
> @@ -132,6 +133,14 @@ &hscif0 {
>         status = "okay";
>  };
>
> +&hscif1 {
> +       pinctrl-0 = <&hscif1_pins>;
> +       pinctrl-names = "default";
> +       uart-has-rtscts;
> +
> +       status = "okay";
> +};
> +
>  &hsusb {
>         dr_mode = "otg";
>         status = "okay";
> @@ -387,8 +396,13 @@ hscif0_pins: hscif0 {
>                 function = "hscif0";
>         };
>
> +       hscif1_pins: hscif1 {
> +               groups = "hscif1_data_a", "hscif1_ctrl_a";
> +               function = "hscif1";
> +       };
> +

The above LGTM, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

>         scif1_pins: scif1 {
> -               groups = "scif1_data_b", "scif1_ctrl";
> +               groups = "scif1_data_b";
>                 function = "scif1";
>         };
>
> @@ -418,7 +432,6 @@ &sound_clk_pins
>  &scif1 {
>         pinctrl-0 = <&scif1_pins>;
>         pinctrl-names = "default";
> -       uart-has-rtscts;
>
>         status = "okay";
>  };

This part also LGTM, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

However, I think the scif1 changes should be a separate patch, with
Fixes: c6c816e22bc89ea4 ("arm64: dts: ulcb-kf: enable SCIF1")
so please split it off.
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] arm64: dts: renesas: ulcb-kf: add HSCIF1 node

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 280 bytes --]


> However, as the only serial port that can be muxed to the GPS pins
> is SCIF1, we have no choice but to use SCIF1 for the GPS, and HSCIF1
> for CN4, like your patch does.

Ack. IMHO we should have the intended configuration upstream. Users can
pinmux from there as they want.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] arm64: dts: renesas: ulcb-kf: add HSCIF1 node

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 176 bytes --]


> However, I think the scif1 changes should be a separate patch, with
> Fixes: c6c816e22bc89ea4 ("arm64: dts: ulcb-kf: enable SCIF1")
> so please split it off.

OK, will do.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]