[PATCH v3 0/8] Add support for the rest of Renesas RZ/G3S serial interfaces

[PATCH v3 0/8] Add support for the rest of Renesas RZ/G3S serial interfaces

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Hi,

The Renesas RZ/G3S SoC has 6 serial interfaces. One of them is used
as debug console (and it is already enabled in the current code base).
Series adds support for the remaining ones.

Patches:
-    01/08 - adds clock, reset and power domain support for the serial
             interfaces
-    02/08 - serial driver fix patch identified while adding RZ/G3S
             support
-    03/08 - extends suspend to RAM support on the serial driver for
             the RZ/G3S SoC
- 04-08/08 - add device tree support

Merge strategy, if any:
- patch 01/08 can go through Renesas tree
- patches 02-03/08 can go through serial tree
- patches 04-08/08 can go through Renesas tree

Thank you,
Claudiu Beznea

Changes in v3:
- in patch "serial: sh-sci: Check if TX data was written to device in
  .tx_empty()":
-- check the status of the DMA transaction in tx_empty()
-- changed the variable name that tracks if TX occurred

Changes in v2:
- drop patch "serial: sh-sci: Clean sci_ports[0] after at earlycon exit"
  from v1 as it was already applied
- used bool instead of atomic_t in patch
  "serial: sh-sci: Check if TX data was written to device in .tx_empty()"

Claudiu Beznea (8):
  clk: renesas: r9a08g045: Add clock, reset and power domain for the
    remaining SCIFs
  serial: sh-sci: Check if TX data was written to device in .tx_empty()
  serial: sh-sci: Update the suspend/resume support
  arm64: dts: renesas: r9a08g045: Add the remaining SCIF interfaces
  arm64: dts: renesas: rzg3s-smarc: Fix the debug serial alias
  arm64: dts: renesas: rzg3s-smarc-switches: Add a header to describe
    different switches
  arm64: dts: renesas: rzg3s-smarc: Enable SCIF3
  arm64: dts: renesas: r9a08g045s33-smarc-pmod: Add overlay for SCIF1

 arch/arm64/boot/dts/renesas/Makefile          |  3 +
 arch/arm64/boot/dts/renesas/r9a08g045.dtsi    | 90 +++++++++++++++++++
 .../dts/renesas/r9a08g045s33-smarc-pmod.dtso  | 48 ++++++++++
 .../boot/dts/renesas/rzg3s-smarc-som.dtsi     | 25 +-----
 .../boot/dts/renesas/rzg3s-smarc-switches.h   | 32 +++++++
 arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi  | 25 +++++-
 drivers/clk/renesas/r9a08g045-cpg.c           | 20 +++++
 drivers/tty/serial/sh-sci.c                   | 79 ++++++++++++++--
 8 files changed, 288 insertions(+), 34 deletions(-)
 create mode 100644 arch/arm64/boot/dts/renesas/r9a08g045s33-smarc-pmod.dtso
 create mode 100644 arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h

-- 
2.39.2

[PATCH v3 1/8] clk: renesas: r9a08g045: Add clock, reset and power domain for the remaining SCIFs

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The Renesas RZ/G3S SoC has 6 SCIF interfaces. SCIF0 is used as debug
console and is already enabled. Add the clock, reset and power domain
support for the remaining ones.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v3:
- none

Changes in v2:
- none

 drivers/clk/renesas/r9a08g045-cpg.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/clk/renesas/r9a08g045-cpg.c b/drivers/clk/renesas/r9a08g045-cpg.c
index b2ae8cdc4723..da6dfffa089a 100644
--- a/drivers/clk/renesas/r9a08g045-cpg.c
+++ b/drivers/clk/renesas/r9a08g045-cpg.c
@@ -224,6 +224,11 @@ static const struct rzg2l_mod_clk r9a08g045_mod_clks[] = {
 	DEF_MOD("i2c2_pclk",		R9A08G045_I2C2_PCLK, R9A08G045_CLK_P0, 0x580, 2),
 	DEF_MOD("i2c3_pclk",		R9A08G045_I2C3_PCLK, R9A08G045_CLK_P0, 0x580, 3),
 	DEF_MOD("scif0_clk_pck",	R9A08G045_SCIF0_CLK_PCK, R9A08G045_CLK_P0, 0x584, 0),
+	DEF_MOD("scif1_clk_pck",	R9A08G045_SCIF1_CLK_PCK, R9A08G045_CLK_P0, 0x584, 1),
+	DEF_MOD("scif2_clk_pck",	R9A08G045_SCIF2_CLK_PCK, R9A08G045_CLK_P0, 0x584, 2),
+	DEF_MOD("scif3_clk_pck",	R9A08G045_SCIF3_CLK_PCK, R9A08G045_CLK_P0, 0x584, 3),
+	DEF_MOD("scif4_clk_pck",	R9A08G045_SCIF4_CLK_PCK, R9A08G045_CLK_P0, 0x584, 4),
+	DEF_MOD("scif5_clk_pck",	R9A08G045_SCIF5_CLK_PCK, R9A08G045_CLK_P0, 0x584, 5),
 	DEF_MOD("gpio_hclk",		R9A08G045_GPIO_HCLK, R9A08G045_OSCCLK, 0x598, 0),
 	DEF_MOD("vbat_bclk",		R9A08G045_VBAT_BCLK, R9A08G045_OSCCLK, 0x614, 0),
 };
@@ -249,6 +254,11 @@ static const struct rzg2l_reset r9a08g045_resets[] = {
 	DEF_RST(R9A08G045_I2C2_MRST, 0x880, 2),
 	DEF_RST(R9A08G045_I2C3_MRST, 0x880, 3),
 	DEF_RST(R9A08G045_SCIF0_RST_SYSTEM_N, 0x884, 0),
+	DEF_RST(R9A08G045_SCIF1_RST_SYSTEM_N, 0x884, 1),
+	DEF_RST(R9A08G045_SCIF2_RST_SYSTEM_N, 0x884, 2),
+	DEF_RST(R9A08G045_SCIF3_RST_SYSTEM_N, 0x884, 3),
+	DEF_RST(R9A08G045_SCIF4_RST_SYSTEM_N, 0x884, 4),
+	DEF_RST(R9A08G045_SCIF5_RST_SYSTEM_N, 0x884, 5),
 	DEF_RST(R9A08G045_GPIO_RSTN, 0x898, 0),
 	DEF_RST(R9A08G045_GPIO_PORT_RESETN, 0x898, 1),
 	DEF_RST(R9A08G045_GPIO_SPARE_RESETN, 0x898, 2),
@@ -306,6 +316,16 @@ static const struct rzg2l_cpg_pm_domain_init_data r9a08g045_pm_domains[] = {
 				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(13)), 0),
 	DEF_PD("scif0",		R9A08G045_PD_SCIF0,
 				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(1)), 0),
+	DEF_PD("scif1",		R9A08G045_PD_SCIF1,
+				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(2)), 0),
+	DEF_PD("scif2",		R9A08G045_PD_SCIF2,
+				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(3)), 0),
+	DEF_PD("scif3",		R9A08G045_PD_SCIF3,
+				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(4)), 0),
+	DEF_PD("scif4",		R9A08G045_PD_SCIF4,
+				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(5)), 0),
+	DEF_PD("scif5",		R9A08G045_PD_SCIF5,
+				DEF_REG_CONF(CPG_BUS_MCPU3_MSTOP, BIT(4)), 0),
 	DEF_PD("vbat",		R9A08G045_PD_VBAT,
 				DEF_REG_CONF(CPG_BUS_MCPU3_MSTOP, BIT(8)),
 				GENPD_FLAG_ALWAYS_ON),
-- 
2.39.2

Re: [PATCH v3 1/8] clk: renesas: r9a08g045: Add clock, reset and power domain for the remaining SCIFs

[Geert Uytterhoeven]

On Fri, Nov 15, 2024 at 2:56 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The Renesas RZ/G3S SoC has 6 SCIF interfaces. SCIF0 is used as debug
> console and is already enabled. Add the clock, reset and power domain
> support for the remaining ones.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-clk for v6.14.

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

[PATCH v3 2/8] serial: sh-sci: Check if TX data was written to device in .tx_empty()

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

On the Renesas RZ/G3S, when doing suspend to RAM, the uart_suspend_port()
is called. The uart_suspend_port() calls 3 times the
struct uart_port::ops::tx_empty() before shutting down the port.

According to the documentation, the struct uart_port::ops::tx_empty()
API tests whether the transmitter FIFO and shifter for the port is
empty.

The Renesas RZ/G3S SCIFA IP reports the number of data units stored in the
transmit FIFO through the FDR (FIFO Data Count Register). The data units
in the FIFOs are written in the shift register and transmitted from there.
The TEND bit in the Serial Status Register reports if the data was
transmitted from the shift register.

In the previous code, in the tx_empty() API implemented by the sh-sci
driver, it is considered that the TX is empty if the hardware reports the
TEND bit set and the number of data units in the FIFO is zero.

According to the HW manual, the TEND bit has the following meaning:

0: Transmission is in the waiting state or in progress.
1: Transmission is completed.

It has been noticed that when opening the serial device w/o using it and
then switch to a power saving mode, the tx_empty() call in the
uart_port_suspend() function fails, leading to the "Unable to drain
transmitter" message being printed on the console. This is because the
TEND=0 if nothing has been transmitted and the FIFOs are empty. As the
TEND=0 has double meaning (waiting state, in progress) we can't
determined the scenario described above.

Add a software workaround for this. This sets a variable if any data has
been sent on the serial console (when using PIO) or if the DMA callback has
been called (meaning something has been transmitted). In the tx_empty()
API the status of the DMA transaction is also checked and if it is
completed or in progress the code falls back in checking the hardware
registers instead of relying on the software variable.

Fixes: 73a19e4c0301 ("serial: sh-sci: Add DMA support.")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v3:
- s/first_time_tx/tx_occurred/g
- checked the DMA status in sci_tx_empty() through sci_dma_check_tx_occurred()
  function; added this new function as the DMA support is conditioned by
  the CONFIG_SERIAL_SH_SCI_DMA flag
- dropped the tx_occurred initialization in sci_shutdown() as it is already
  initialized in sci_startup()
- adjusted the commit message to reflect latest changes

Changes in v2:
- use bool type instead of atomic_t

 drivers/tty/serial/sh-sci.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 136e0c257af1..ade151ff39d2 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -157,6 +157,7 @@ struct sci_port {
 
 	bool has_rtscts;
 	bool autorts;
+	bool tx_occurred;
 };
 
 #define SCI_NPORTS CONFIG_SERIAL_SH_SCI_NR_UARTS
@@ -850,6 +851,7 @@ static void sci_transmit_chars(struct uart_port *port)
 {
 	struct tty_port *tport = &port->state->port;
 	unsigned int stopped = uart_tx_stopped(port);
+	struct sci_port *s = to_sci_port(port);
 	unsigned short status;
 	unsigned short ctrl;
 	int count;
@@ -885,6 +887,7 @@ static void sci_transmit_chars(struct uart_port *port)
 		}
 
 		sci_serial_out(port, SCxTDR, c);
+		s->tx_occurred = true;
 
 		port->icount.tx++;
 	} while (--count > 0);
@@ -1241,6 +1244,8 @@ static void sci_dma_tx_complete(void *arg)
 	if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
 		uart_write_wakeup(port);
 
+	s->tx_occurred = true;
+
 	if (!kfifo_is_empty(&tport->xmit_fifo)) {
 		s->cookie_tx = 0;
 		schedule_work(&s->work_tx);
@@ -1731,6 +1736,16 @@ static void sci_flush_buffer(struct uart_port *port)
 		s->cookie_tx = -EINVAL;
 	}
 }
+
+static void sci_dma_check_tx_occurred(struct sci_port *s)
+{
+	struct dma_tx_state state;
+	enum dma_status status;
+
+	status = dmaengine_tx_status(s->chan_tx, s->cookie_tx, &state);
+	if (status == DMA_COMPLETE || status == DMA_IN_PROGRESS)
+		s->tx_occurred = true;
+}
 #else /* !CONFIG_SERIAL_SH_SCI_DMA */
 static inline void sci_request_dma(struct uart_port *port)
 {
@@ -1740,6 +1755,10 @@ static inline void sci_free_dma(struct uart_port *port)
 {
 }
 
+static void sci_dma_check_tx_occurred(struct sci_port *s)
+{
+}
+
 #define sci_flush_buffer	NULL
 #endif /* !CONFIG_SERIAL_SH_SCI_DMA */
 
@@ -2076,6 +2095,12 @@ static unsigned int sci_tx_empty(struct uart_port *port)
 {
 	unsigned short status = sci_serial_in(port, SCxSR);
 	unsigned short in_tx_fifo = sci_txfill(port);
+	struct sci_port *s = to_sci_port(port);
+
+	sci_dma_check_tx_occurred(s);
+
+	if (!s->tx_occurred)
+		return TIOCSER_TEMT;
 
 	return (status & SCxSR_TEND(port)) && !in_tx_fifo ? TIOCSER_TEMT : 0;
 }
@@ -2247,6 +2272,7 @@ static int sci_startup(struct uart_port *port)
 
 	dev_dbg(port->dev, "%s(%d)\n", __func__, port->line);
 
+	s->tx_occurred = false;
 	sci_request_dma(port);
 
 	ret = sci_request_irq(s);
-- 
2.39.2

Re: [PATCH v3 2/8] serial: sh-sci: Check if TX data was written to device in .tx_empty()

[Greg KH]

On Fri, Nov 15, 2024 at 03:43:55PM +0200, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> On the Renesas RZ/G3S, when doing suspend to RAM, the uart_suspend_port()
> is called. The uart_suspend_port() calls 3 times the
> struct uart_port::ops::tx_empty() before shutting down the port.
> 
> According to the documentation, the struct uart_port::ops::tx_empty()
> API tests whether the transmitter FIFO and shifter for the port is
> empty.
> 
> The Renesas RZ/G3S SCIFA IP reports the number of data units stored in the
> transmit FIFO through the FDR (FIFO Data Count Register). The data units
> in the FIFOs are written in the shift register and transmitted from there.
> The TEND bit in the Serial Status Register reports if the data was
> transmitted from the shift register.
> 
> In the previous code, in the tx_empty() API implemented by the sh-sci
> driver, it is considered that the TX is empty if the hardware reports the
> TEND bit set and the number of data units in the FIFO is zero.
> 
> According to the HW manual, the TEND bit has the following meaning:
> 
> 0: Transmission is in the waiting state or in progress.
> 1: Transmission is completed.
> 
> It has been noticed that when opening the serial device w/o using it and
> then switch to a power saving mode, the tx_empty() call in the
> uart_port_suspend() function fails, leading to the "Unable to drain
> transmitter" message being printed on the console. This is because the
> TEND=0 if nothing has been transmitted and the FIFOs are empty. As the
> TEND=0 has double meaning (waiting state, in progress) we can't
> determined the scenario described above.
> 
> Add a software workaround for this. This sets a variable if any data has
> been sent on the serial console (when using PIO) or if the DMA callback has
> been called (meaning something has been transmitted). In the tx_empty()
> API the status of the DMA transaction is also checked and if it is
> completed or in progress the code falls back in checking the hardware
> registers instead of relying on the software variable.
> 
> Fixes: 73a19e4c0301 ("serial: sh-sci: Add DMA support.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Why is this bug/regression fix burried in a long series?  It should be
sent individually so that it could be applied on its own as it is not
related to the other ones, right?

Or are you ok with waiting for this to show up in 6.14-rc1?

thanks,

greg k-h

Re: [PATCH v3 2/8] serial: sh-sci: Check if TX data was written to device in .tx_empty()

[Claudiu Beznea]

Hi, Greg,

On 21.11.2024 23:32, Greg KH wrote:
> On Fri, Nov 15, 2024 at 03:43:55PM +0200, Claudiu wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> On the Renesas RZ/G3S, when doing suspend to RAM, the uart_suspend_port()
>> is called. The uart_suspend_port() calls 3 times the
>> struct uart_port::ops::tx_empty() before shutting down the port.
>>
>> According to the documentation, the struct uart_port::ops::tx_empty()
>> API tests whether the transmitter FIFO and shifter for the port is
>> empty.
>>
>> The Renesas RZ/G3S SCIFA IP reports the number of data units stored in the
>> transmit FIFO through the FDR (FIFO Data Count Register). The data units
>> in the FIFOs are written in the shift register and transmitted from there.
>> The TEND bit in the Serial Status Register reports if the data was
>> transmitted from the shift register.
>>
>> In the previous code, in the tx_empty() API implemented by the sh-sci
>> driver, it is considered that the TX is empty if the hardware reports the
>> TEND bit set and the number of data units in the FIFO is zero.
>>
>> According to the HW manual, the TEND bit has the following meaning:
>>
>> 0: Transmission is in the waiting state or in progress.
>> 1: Transmission is completed.
>>
>> It has been noticed that when opening the serial device w/o using it and
>> then switch to a power saving mode, the tx_empty() call in the
>> uart_port_suspend() function fails, leading to the "Unable to drain
>> transmitter" message being printed on the console. This is because the
>> TEND=0 if nothing has been transmitted and the FIFOs are empty. As the
>> TEND=0 has double meaning (waiting state, in progress) we can't
>> determined the scenario described above.
>>
>> Add a software workaround for this. This sets a variable if any data has
>> been sent on the serial console (when using PIO) or if the DMA callback has
>> been called (meaning something has been transmitted). In the tx_empty()
>> API the status of the DMA transaction is also checked and if it is
>> completed or in progress the code falls back in checking the hardware
>> registers instead of relying on the software variable.
>>
>> Fixes: 73a19e4c0301 ("serial: sh-sci: Add DMA support.")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Why is this bug/regression fix burried in a long series?  It should be
> sent individually so that it could be applied on its own as it is not
> related to the other ones, right?

It is related to the suspend to RAM support added in this series.

> 
> Or are you ok with waiting for this to show up in 6.14-rc1?

I'll resend it individually.

Thank you,
Claudiu Beznea

> 
> thanks,
> 
> greg k-h

Re: [PATCH v3 2/8] serial: sh-sci: Check if TX data was written to device in .tx_empty()

[Geert Uytterhoeven]

Hi Claudiu,

On Fri, Nov 15, 2024 at 2:49 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> On the Renesas RZ/G3S, when doing suspend to RAM, the uart_suspend_port()
> is called. The uart_suspend_port() calls 3 times the
> struct uart_port::ops::tx_empty() before shutting down the port.
>
> According to the documentation, the struct uart_port::ops::tx_empty()
> API tests whether the transmitter FIFO and shifter for the port is
> empty.
>
> The Renesas RZ/G3S SCIFA IP reports the number of data units stored in the
> transmit FIFO through the FDR (FIFO Data Count Register). The data units
> in the FIFOs are written in the shift register and transmitted from there.
> The TEND bit in the Serial Status Register reports if the data was
> transmitted from the shift register.
>
> In the previous code, in the tx_empty() API implemented by the sh-sci
> driver, it is considered that the TX is empty if the hardware reports the
> TEND bit set and the number of data units in the FIFO is zero.
>
> According to the HW manual, the TEND bit has the following meaning:
>
> 0: Transmission is in the waiting state or in progress.
> 1: Transmission is completed.
>
> It has been noticed that when opening the serial device w/o using it and
> then switch to a power saving mode, the tx_empty() call in the
> uart_port_suspend() function fails, leading to the "Unable to drain
> transmitter" message being printed on the console. This is because the
> TEND=0 if nothing has been transmitted and the FIFOs are empty. As the
> TEND=0 has double meaning (waiting state, in progress) we can't
> determined the scenario described above.
>
> Add a software workaround for this. This sets a variable if any data has
> been sent on the serial console (when using PIO) or if the DMA callback has
> been called (meaning something has been transmitted). In the tx_empty()
> API the status of the DMA transaction is also checked and if it is
> completed or in progress the code falls back in checking the hardware
> registers instead of relying on the software variable.
>
> Fixes: 73a19e4c0301 ("serial: sh-sci: Add DMA support.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>
> Changes in v3:
> - s/first_time_tx/tx_occurred/g
> - checked the DMA status in sci_tx_empty() through sci_dma_check_tx_occurred()
>   function; added this new function as the DMA support is conditioned by
>   the CONFIG_SERIAL_SH_SCI_DMA flag
> - dropped the tx_occurred initialization in sci_shutdown() as it is already
>   initialized in sci_startup()
> - adjusted the commit message to reflect latest changes

Thanks for the update!

This causes a crash during boot on R-Car Gen2/3:

8<--- cut here ---
Unable to handle kernel NULL pointer dereference at virtual address
00000000 when read
[00000000] *pgd=43d6d003, *pmd=00000000
Internal error: Oops: 205 [#1] SMP ARM
Modules linked in:
CPU: 0 UID: 0 PID: 1 Comm: systemd Tainted: G        W        N
6.12.0-koelsch-10073-ge416b6f6bb75 #2051
Tainted: [W]=WARN, [N]=TEST
Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
PC is at sci_tx_empty+0x40/0xb8
LR is at sci_txfill+0x44/0x60
pc : [<c063cfd0>]    lr : [<c063cf74>]    psr: 60010013
sp : f0815e40  ip : 00000000  fp : ffbfff78
r10: 00000001  r9 : c21c0000  r8 : c1205d40
r7 : ffff91eb  r6 : 00000060  r5 : 00000000  r4 : c1da4390
r3 : f097d01c  r2 : f0815e44  r1 : 00000000  r0 : 00000000
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 30c5387d  Table: 422d8900  DAC: fffffffd
Register r0 information: NULL pointer
Register r1 information: NULL pointer
Register r2 information: 2-page vmalloc region starting at 0xf0814000
allocated at kernel_clone+0xa0/0x320
Register r3 information: 0-page vmalloc region starting at 0xf097d000
allocated at sci_remap_port+0x58/0x8c
Register r4 information: non-slab/vmalloc memory
Register r5 information: NULL pointer
Register r6 information: non-paged memory
Register r7 information: non-paged memory
Register r8 information: non-slab/vmalloc memory
Register r9 information: slab task_struct start c21c0000 pointer
offset 0 size 6208
Register r10 information: non-paged memory
Register r11 information: non-paged memory
Register r12 information: NULL pointer
Process systemd (pid: 1, stack limit = 0x(ptrval))
Stack: (0xf0815e40 to 0xf0816000)
5e40: 00000bb8 00000001 60010013 c02bca80 00000000 95203767 c1da4390 00000004
5e60: 00000001 c0637e88 c44bc000 c59f7c00 00000000 60010013 c44bc0b4 00000000
5e80: 00000001 c0625c5c c44bc000 c59f7c00 c44bc000 c59f7c00 c216b0d0 c388d800
5ea0: c2c002a8 00000000 b5403587 c0625e20 c59f7c00 00000000 c216b0d0 c061e3c0
5ec0: 0000019f c2c002a8 00000000 b5403587 f0815ef4 c388d800 000e0003 c216b0d0
5ee0: c28923c0 c2c002a8 00000000 b5403587 ffbfff78 c03ae2f0 c388d800 00000001
5f00: c21c0000 c03ae450 00000000 c21c0000 c0e6f112 c21c07a4 c13b1d18 c0244030
5f20: f0815fb0 c0200298 00040000 c21c0000 c0200298 c0209690 00000000 c21c0000
5f40: b5003500 c388d8b8 beca19c4 c0243e6c c388d800 c388d8b8 c2177500 c3b53c00
5f60: c388d800 c03ae134 00000000 c388d800 c2177500 c03a9568 00000002 c2177538
5f80: c2177500 95203767 ffffffff ffffffff 00000002 00000003 0000003f c0200298
5fa0: c21c0000 0000003f beca19c4 c0200088 00000002 00000002 00000000 00000000
5fc0: ffffffff 00000002 00000003 0000003f 00000004 ffffffff beca19c4 beca19c4
5fe0: b6e68264 beca19a4 b6d48857 b6bbbcc8 20010030 00000004 00000000 00000000
Call trace:
 sci_tx_empty from uart_wait_until_sent+0xcc/0x118
 uart_wait_until_sent from tty_port_close_start+0x118/0x190
 tty_port_close_start from tty_port_close+0x10/0x58
 tty_port_close from tty_release+0xf4/0x394
 tty_release from __fput+0x10c/0x218
 __fput from task_work_run+0x84/0xb4
 task_work_run from do_work_pending+0x3b8/0x3f0
 do_work_pending from slow_work_pending+0xc/0x24
Exception stack(0xf0815fb0 to 0xf0815ff8)
5fa0:                                     00000002 00000002 00000000 00000000
5fc0: ffffffff 00000002 00000003 0000003f 00000004 ffffffff beca19c4 beca19c4
5fe0: b6e68264 beca19a4 b6d48857 b6bbbcc8 20010030 00000004
Code: e1a05000 e594020c e28d2004 e594121c (e5903000)
---[ end trace 0000000000000000 ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
---[ end Kernel panic - not syncing: Attempted to kill init!
exitcode=0x0000000b ]---

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 v3 2/8] serial: sh-sci: Check if TX data was written to device in .tx_empty()

[Claudiu Beznea]

Hi, Geert,

On 27.11.2024 15:47, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Fri, Nov 15, 2024 at 2:49 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> On the Renesas RZ/G3S, when doing suspend to RAM, the uart_suspend_port()
>> is called. The uart_suspend_port() calls 3 times the
>> struct uart_port::ops::tx_empty() before shutting down the port.
>>
>> According to the documentation, the struct uart_port::ops::tx_empty()
>> API tests whether the transmitter FIFO and shifter for the port is
>> empty.
>>
>> The Renesas RZ/G3S SCIFA IP reports the number of data units stored in the
>> transmit FIFO through the FDR (FIFO Data Count Register). The data units
>> in the FIFOs are written in the shift register and transmitted from there.
>> The TEND bit in the Serial Status Register reports if the data was
>> transmitted from the shift register.
>>
>> In the previous code, in the tx_empty() API implemented by the sh-sci
>> driver, it is considered that the TX is empty if the hardware reports the
>> TEND bit set and the number of data units in the FIFO is zero.
>>
>> According to the HW manual, the TEND bit has the following meaning:
>>
>> 0: Transmission is in the waiting state or in progress.
>> 1: Transmission is completed.
>>
>> It has been noticed that when opening the serial device w/o using it and
>> then switch to a power saving mode, the tx_empty() call in the
>> uart_port_suspend() function fails, leading to the "Unable to drain
>> transmitter" message being printed on the console. This is because the
>> TEND=0 if nothing has been transmitted and the FIFOs are empty. As the
>> TEND=0 has double meaning (waiting state, in progress) we can't
>> determined the scenario described above.
>>
>> Add a software workaround for this. This sets a variable if any data has
>> been sent on the serial console (when using PIO) or if the DMA callback has
>> been called (meaning something has been transmitted). In the tx_empty()
>> API the status of the DMA transaction is also checked and if it is
>> completed or in progress the code falls back in checking the hardware
>> registers instead of relying on the software variable.
>>
>> Fixes: 73a19e4c0301 ("serial: sh-sci: Add DMA support.")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v3:
>> - s/first_time_tx/tx_occurred/g
>> - checked the DMA status in sci_tx_empty() through sci_dma_check_tx_occurred()
>>   function; added this new function as the DMA support is conditioned by
>>   the CONFIG_SERIAL_SH_SCI_DMA flag
>> - dropped the tx_occurred initialization in sci_shutdown() as it is already
>>   initialized in sci_startup()
>> - adjusted the commit message to reflect latest changes
> 
> Thanks for the update!
> 
> This causes a crash during boot on R-Car Gen2/3:
> 
> 8<--- cut here ---
> Unable to handle kernel NULL pointer dereference at virtual address
> 00000000 when read
> [00000000] *pgd=43d6d003, *pmd=00000000
> Internal error: Oops: 205 [#1] SMP ARM
> Modules linked in:
> CPU: 0 UID: 0 PID: 1 Comm: systemd Tainted: G        W        N
> 6.12.0-koelsch-10073-ge416b6f6bb75 #2051
> Tainted: [W]=WARN, [N]=TEST
> Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
> PC is at sci_tx_empty+0x40/0xb8
> LR is at sci_txfill+0x44/0x60
> pc : [<c063cfd0>]    lr : [<c063cf74>]    psr: 60010013
> sp : f0815e40  ip : 00000000  fp : ffbfff78
> r10: 00000001  r9 : c21c0000  r8 : c1205d40
> r7 : ffff91eb  r6 : 00000060  r5 : 00000000  r4 : c1da4390
> r3 : f097d01c  r2 : f0815e44  r1 : 00000000  r0 : 00000000
> Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
> Control: 30c5387d  Table: 422d8900  DAC: fffffffd
> Register r0 information: NULL pointer
> Register r1 information: NULL pointer
> Register r2 information: 2-page vmalloc region starting at 0xf0814000
> allocated at kernel_clone+0xa0/0x320
> Register r3 information: 0-page vmalloc region starting at 0xf097d000
> allocated at sci_remap_port+0x58/0x8c
> Register r4 information: non-slab/vmalloc memory
> Register r5 information: NULL pointer
> Register r6 information: non-paged memory
> Register r7 information: non-paged memory
> Register r8 information: non-slab/vmalloc memory
> Register r9 information: slab task_struct start c21c0000 pointer
> offset 0 size 6208
> Register r10 information: non-paged memory
> Register r11 information: non-paged memory
> Register r12 information: NULL pointer
> Process systemd (pid: 1, stack limit = 0x(ptrval))
> Stack: (0xf0815e40 to 0xf0816000)
> 5e40: 00000bb8 00000001 60010013 c02bca80 00000000 95203767 c1da4390 00000004
> 5e60: 00000001 c0637e88 c44bc000 c59f7c00 00000000 60010013 c44bc0b4 00000000
> 5e80: 00000001 c0625c5c c44bc000 c59f7c00 c44bc000 c59f7c00 c216b0d0 c388d800
> 5ea0: c2c002a8 00000000 b5403587 c0625e20 c59f7c00 00000000 c216b0d0 c061e3c0
> 5ec0: 0000019f c2c002a8 00000000 b5403587 f0815ef4 c388d800 000e0003 c216b0d0
> 5ee0: c28923c0 c2c002a8 00000000 b5403587 ffbfff78 c03ae2f0 c388d800 00000001
> 5f00: c21c0000 c03ae450 00000000 c21c0000 c0e6f112 c21c07a4 c13b1d18 c0244030
> 5f20: f0815fb0 c0200298 00040000 c21c0000 c0200298 c0209690 00000000 c21c0000
> 5f40: b5003500 c388d8b8 beca19c4 c0243e6c c388d800 c388d8b8 c2177500 c3b53c00
> 5f60: c388d800 c03ae134 00000000 c388d800 c2177500 c03a9568 00000002 c2177538
> 5f80: c2177500 95203767 ffffffff ffffffff 00000002 00000003 0000003f c0200298
> 5fa0: c21c0000 0000003f beca19c4 c0200088 00000002 00000002 00000000 00000000
> 5fc0: ffffffff 00000002 00000003 0000003f 00000004 ffffffff beca19c4 beca19c4
> 5fe0: b6e68264 beca19a4 b6d48857 b6bbbcc8 20010030 00000004 00000000 00000000
> Call trace:
>  sci_tx_empty from uart_wait_until_sent+0xcc/0x118
>  uart_wait_until_sent from tty_port_close_start+0x118/0x190
>  tty_port_close_start from tty_port_close+0x10/0x58
>  tty_port_close from tty_release+0xf4/0x394
>  tty_release from __fput+0x10c/0x218
>  __fput from task_work_run+0x84/0xb4
>  task_work_run from do_work_pending+0x3b8/0x3f0
>  do_work_pending from slow_work_pending+0xc/0x24
> Exception stack(0xf0815fb0 to 0xf0815ff8)
> 5fa0:                                     00000002 00000002 00000000 00000000
> 5fc0: ffffffff 00000002 00000003 0000003f 00000004 ffffffff beca19c4 beca19c4
> 5fe0: b6e68264 beca19a4 b6d48857 b6bbbcc8 20010030 00000004
> Code: e1a05000 e594020c e28d2004 e594121c (e5903000)
> ---[ end trace 0000000000000000 ]---
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> ---[ end Kernel panic - not syncing: Attempted to kill init!
> exitcode=0x0000000b ]---

Sorry for that. It should be fixed by the standalone version of this patch
[1] by the following check:

+static void sci_dma_check_tx_occurred(struct sci_port *s)
+{
+	struct dma_tx_state state;
+	enum dma_status status;
+
+	if (!s->chan_tx)
+		return;

[1]
https://lore.kernel.org/all/20241125115856.513642-1-claudiu.beznea.uj@bp.renesas.com/


Thank you,
Claudiu

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

[PATCH v3 3/8] serial: sh-sci: Update the suspend/resume support

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The Renesas RZ/G3S supports a power saving mode where power to most of the
SoC components is turned off. When returning from this power saving mode,
SoC components need to be re-configured.

The SCIFs on the Renesas RZ/G3S need to be re-configured as well when
returning from this power saving mode. The sh-sci code already configures
the SCIF clocks, power domain and registers by calling uart_resume_port()
in sci_resume(). On suspend path the SCIF UART ports are suspended
accordingly (by calling uart_suspend_port() in sci_suspend()). The only
missing setting is the reset signal. For this assert/de-assert the reset
signal on driver suspend/resume.

In case the no_console_suspend is specified by the user, the registers need
to be saved on suspend path and restore on resume path. To do this the
sci_console_setup() function was added. There is no need to cache/restore
the status or FIFO registers. Only the control registers. To differentiate
b/w these, the struct sci_port_params::regs was updated with a new member
that specifies if the register needs to be chached on suspend. Only the
RZ_SCIFA instances were updated with this new support as the hardware for
the rest of variants was missing for testing.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v3:
- none

Changes in v2:
- rebased on top of the update version of patch 2/8 from
  this series

 drivers/tty/serial/sh-sci.c | 53 ++++++++++++++++++++++++++++++-------
 1 file changed, 44 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index ade151ff39d2..e53496d2708e 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -101,7 +101,7 @@ enum SCI_CLKS {
 		if ((_port)->sampling_rate_mask & SCI_SR((_sr)))
 
 struct plat_sci_reg {
-	u8 offset, size;
+	u8 offset, size, suspend_cacheable;
 };
 
 struct sci_port_params {
@@ -134,6 +134,8 @@ struct sci_port {
 	struct dma_chan			*chan_tx;
 	struct dma_chan			*chan_rx;
 
+	struct reset_control		*rstc;
+
 #ifdef CONFIG_SERIAL_SH_SCI_DMA
 	struct dma_chan			*chan_tx_saved;
 	struct dma_chan			*chan_rx_saved;
@@ -153,6 +155,7 @@ struct sci_port {
 	int				rx_trigger;
 	struct timer_list		rx_fifo_timer;
 	int				rx_fifo_timeout;
+	unsigned int			console_cached_regs[SCIx_NR_REGS];
 	u16				hscif_tot;
 
 	bool has_rtscts;
@@ -298,17 +301,17 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = {
 	 */
 	[SCIx_RZ_SCIFA_REGTYPE] = {
 		.regs = {
-			[SCSMR]		= { 0x00, 16 },
-			[SCBRR]		= { 0x02,  8 },
-			[SCSCR]		= { 0x04, 16 },
+			[SCSMR]		= { 0x00, 16, 1 },
+			[SCBRR]		= { 0x02,  8, 1 },
+			[SCSCR]		= { 0x04, 16, 1 },
 			[SCxTDR]	= { 0x06,  8 },
 			[SCxSR]		= { 0x08, 16 },
 			[SCxRDR]	= { 0x0A,  8 },
-			[SCFCR]		= { 0x0C, 16 },
+			[SCFCR]		= { 0x0C, 16, 1 },
 			[SCFDR]		= { 0x0E, 16 },
-			[SCSPTR]	= { 0x10, 16 },
+			[SCSPTR]	= { 0x10, 16, 1 },
 			[SCLSR]		= { 0x12, 16 },
-			[SEMR]		= { 0x14, 8 },
+			[SEMR]		= { 0x14, 8, 1 },
 		},
 		.fifosize = 16,
 		.overrun_reg = SCLSR,
@@ -3380,6 +3383,7 @@ static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
 	}
 
 	sp = &sci_ports[id];
+	sp->rstc = rstc;
 	*dev_id = id;
 
 	p->type = SCI_OF_TYPE(data);
@@ -3507,13 +3511,34 @@ static int sci_probe(struct platform_device *dev)
 	return 0;
 }
 
+static void sci_console_setup(struct sci_port *s, bool save)
+{
+	for (u16 i = 0; i < SCIx_NR_REGS; i++) {
+		struct uart_port *port = &s->port;
+
+		if (!s->params->regs[i].suspend_cacheable)
+			continue;
+
+		if (save)
+			s->console_cached_regs[i] = sci_serial_in(port, i);
+		else
+			sci_serial_out(port, i, s->console_cached_regs[i]);
+	}
+}
+
 static __maybe_unused int sci_suspend(struct device *dev)
 {
 	struct sci_port *sport = dev_get_drvdata(dev);
 
-	if (sport)
+	if (sport) {
 		uart_suspend_port(&sci_uart_driver, &sport->port);
 
+		if (!console_suspend_enabled && uart_console(&sport->port))
+			sci_console_setup(sport, true);
+		else
+			return reset_control_assert(sport->rstc);
+	}
+
 	return 0;
 }
 
@@ -3521,8 +3546,18 @@ static __maybe_unused int sci_resume(struct device *dev)
 {
 	struct sci_port *sport = dev_get_drvdata(dev);
 
-	if (sport)
+	if (sport) {
+		if (!console_suspend_enabled && uart_console(&sport->port)) {
+			sci_console_setup(sport, false);
+		} else {
+			int ret = reset_control_deassert(sport->rstc);
+
+			if (ret)
+				return ret;
+		}
+
 		uart_resume_port(&sci_uart_driver, &sport->port);
+	}
 
 	return 0;
 }
-- 
2.39.2

Re: [PATCH v3 3/8] serial: sh-sci: Update the suspend/resume support

[Philipp Zabel]

On Fr, 2024-11-15 at 15:43 +0200, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The Renesas RZ/G3S supports a power saving mode where power to most of the
> SoC components is turned off. When returning from this power saving mode,
> SoC components need to be re-configured.
> 
> The SCIFs on the Renesas RZ/G3S need to be re-configured as well when
> returning from this power saving mode. The sh-sci code already configures
> the SCIF clocks, power domain and registers by calling uart_resume_port()
> in sci_resume(). On suspend path the SCIF UART ports are suspended
> accordingly (by calling uart_suspend_port() in sci_suspend()). The only
> missing setting is the reset signal. For this assert/de-assert the reset
> signal on driver suspend/resume.
> 
> In case the no_console_suspend is specified by the user, the registers need
> to be saved on suspend path and restore on resume path. To do this the
> sci_console_setup() function was added. There is no need to cache/restore
> the status or FIFO registers. Only the control registers. To differentiate
> b/w these, the struct sci_port_params::regs was updated with a new member
> that specifies if the register needs to be chached on suspend. Only the
> RZ_SCIFA instances were updated with this new support as the hardware for
> the rest of variants was missing for testing.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v3:
> - none
> 
> Changes in v2:
> - rebased on top of the update version of patch 2/8 from
>   this series
> 
>  drivers/tty/serial/sh-sci.c | 53 ++++++++++++++++++++++++++++++-------
>  1 file changed, 44 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index ade151ff39d2..e53496d2708e 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -101,7 +101,7 @@ enum SCI_CLKS {
>  		if ((_port)->sampling_rate_mask & SCI_SR((_sr)))
>  
>  struct plat_sci_reg {
> -	u8 offset, size;
> +	u8 offset, size, suspend_cacheable;
>  };
>  
>  struct sci_port_params {
> @@ -134,6 +134,8 @@ struct sci_port {
>  	struct dma_chan			*chan_tx;
>  	struct dma_chan			*chan_rx;
>  
> +	struct reset_control		*rstc;
> +
>  #ifdef CONFIG_SERIAL_SH_SCI_DMA
>  	struct dma_chan			*chan_tx_saved;
>  	struct dma_chan			*chan_rx_saved;
> @@ -153,6 +155,7 @@ struct sci_port {
>  	int				rx_trigger;
>  	struct timer_list		rx_fifo_timer;
>  	int				rx_fifo_timeout;
> +	unsigned int			console_cached_regs[SCIx_NR_REGS];
>  	u16				hscif_tot;
>  
>  	bool has_rtscts;
> @@ -298,17 +301,17 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = {
>  	 */
>  	[SCIx_RZ_SCIFA_REGTYPE] = {
>  		.regs = {
> -			[SCSMR]		= { 0x00, 16 },
> -			[SCBRR]		= { 0x02,  8 },
> -			[SCSCR]		= { 0x04, 16 },
> +			[SCSMR]		= { 0x00, 16, 1 },
> +			[SCBRR]		= { 0x02,  8, 1 },
> +			[SCSCR]		= { 0x04, 16, 1 },
>  			[SCxTDR]	= { 0x06,  8 },
>  			[SCxSR]		= { 0x08, 16 },
>  			[SCxRDR]	= { 0x0A,  8 },
> -			[SCFCR]		= { 0x0C, 16 },
> +			[SCFCR]		= { 0x0C, 16, 1 },
>  			[SCFDR]		= { 0x0E, 16 },
> -			[SCSPTR]	= { 0x10, 16 },
> +			[SCSPTR]	= { 0x10, 16, 1 },
>  			[SCLSR]		= { 0x12, 16 },
> -			[SEMR]		= { 0x14, 8 },
> +			[SEMR]		= { 0x14, 8, 1 },
>  		},
>  		.fifosize = 16,
>  		.overrun_reg = SCLSR,
> @@ -3380,6 +3383,7 @@ static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
>  	}
>  
>  	sp = &sci_ports[id];
> +	sp->rstc = rstc;
>  	*dev_id = id;
>  
>  	p->type = SCI_OF_TYPE(data);
> @@ -3507,13 +3511,34 @@ static int sci_probe(struct platform_device *dev)
>  	return 0;
>  }
>  
> +static void sci_console_setup(struct sci_port *s, bool save)
> +{
> +	for (u16 i = 0; i < SCIx_NR_REGS; i++) {
> +		struct uart_port *port = &s->port;
> +
> +		if (!s->params->regs[i].suspend_cacheable)
> +			continue;
> +
> +		if (save)
> +			s->console_cached_regs[i] = sci_serial_in(port, i);
> +		else
> +			sci_serial_out(port, i, s->console_cached_regs[i]);
> +	}
> +}
> +
>  static __maybe_unused int sci_suspend(struct device *dev)
>  {
>  	struct sci_port *sport = dev_get_drvdata(dev);
>  
> -	if (sport)
> +	if (sport) {
>  		uart_suspend_port(&sci_uart_driver, &sport->port);
>  
> +		if (!console_suspend_enabled && uart_console(&sport->port))
> +			sci_console_setup(sport, true);
> +		else
> +			return reset_control_assert(sport->rstc);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -3521,8 +3546,18 @@ static __maybe_unused int sci_resume(struct device *dev)
>  {
>  	struct sci_port *sport = dev_get_drvdata(dev);
>  
> -	if (sport)
> +	if (sport) {
> +		if (!console_suspend_enabled && uart_console(&sport->port)) {
> +			sci_console_setup(sport, false);
> +		} else {
> +			int ret = reset_control_deassert(sport->rstc);

With this, is the reset_control_deassert() in sci_parse_dt() still
needed?

Likewise, does the reset_control_assert() in sci_suspend() remove the
need for the sci_reset_control_assert() devm action?

regards
Philipp

Re: [PATCH v3 3/8] serial: sh-sci: Update the suspend/resume support

[Claudiu Beznea]

Hi, Philipp,

On 15.11.2024 17:40, Philipp Zabel wrote:
> On Fr, 2024-11-15 at 15:43 +0200, Claudiu wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The Renesas RZ/G3S supports a power saving mode where power to most of the
>> SoC components is turned off. When returning from this power saving mode,
>> SoC components need to be re-configured.
>>
>> The SCIFs on the Renesas RZ/G3S need to be re-configured as well when
>> returning from this power saving mode. The sh-sci code already configures
>> the SCIF clocks, power domain and registers by calling uart_resume_port()
>> in sci_resume(). On suspend path the SCIF UART ports are suspended
>> accordingly (by calling uart_suspend_port() in sci_suspend()). The only
>> missing setting is the reset signal. For this assert/de-assert the reset
>> signal on driver suspend/resume.
>>
>> In case the no_console_suspend is specified by the user, the registers need
>> to be saved on suspend path and restore on resume path. To do this the
>> sci_console_setup() function was added. There is no need to cache/restore
>> the status or FIFO registers. Only the control registers. To differentiate
>> b/w these, the struct sci_port_params::regs was updated with a new member
>> that specifies if the register needs to be chached on suspend. Only the
>> RZ_SCIFA instances were updated with this new support as the hardware for
>> the rest of variants was missing for testing.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v3:
>> - none
>>
>> Changes in v2:
>> - rebased on top of the update version of patch 2/8 from
>>   this series
>>
>>  drivers/tty/serial/sh-sci.c | 53 ++++++++++++++++++++++++++++++-------
>>  1 file changed, 44 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
>> index ade151ff39d2..e53496d2708e 100644
>> --- a/drivers/tty/serial/sh-sci.c
>> +++ b/drivers/tty/serial/sh-sci.c
>> @@ -101,7 +101,7 @@ enum SCI_CLKS {
>>  		if ((_port)->sampling_rate_mask & SCI_SR((_sr)))
>>  
>>  struct plat_sci_reg {
>> -	u8 offset, size;
>> +	u8 offset, size, suspend_cacheable;
>>  };
>>  
>>  struct sci_port_params {
>> @@ -134,6 +134,8 @@ struct sci_port {
>>  	struct dma_chan			*chan_tx;
>>  	struct dma_chan			*chan_rx;
>>  
>> +	struct reset_control		*rstc;
>> +
>>  #ifdef CONFIG_SERIAL_SH_SCI_DMA
>>  	struct dma_chan			*chan_tx_saved;
>>  	struct dma_chan			*chan_rx_saved;
>> @@ -153,6 +155,7 @@ struct sci_port {
>>  	int				rx_trigger;
>>  	struct timer_list		rx_fifo_timer;
>>  	int				rx_fifo_timeout;
>> +	unsigned int			console_cached_regs[SCIx_NR_REGS];
>>  	u16				hscif_tot;
>>  
>>  	bool has_rtscts;
>> @@ -298,17 +301,17 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = {
>>  	 */
>>  	[SCIx_RZ_SCIFA_REGTYPE] = {
>>  		.regs = {
>> -			[SCSMR]		= { 0x00, 16 },
>> -			[SCBRR]		= { 0x02,  8 },
>> -			[SCSCR]		= { 0x04, 16 },
>> +			[SCSMR]		= { 0x00, 16, 1 },
>> +			[SCBRR]		= { 0x02,  8, 1 },
>> +			[SCSCR]		= { 0x04, 16, 1 },
>>  			[SCxTDR]	= { 0x06,  8 },
>>  			[SCxSR]		= { 0x08, 16 },
>>  			[SCxRDR]	= { 0x0A,  8 },
>> -			[SCFCR]		= { 0x0C, 16 },
>> +			[SCFCR]		= { 0x0C, 16, 1 },
>>  			[SCFDR]		= { 0x0E, 16 },
>> -			[SCSPTR]	= { 0x10, 16 },
>> +			[SCSPTR]	= { 0x10, 16, 1 },
>>  			[SCLSR]		= { 0x12, 16 },
>> -			[SEMR]		= { 0x14, 8 },
>> +			[SEMR]		= { 0x14, 8, 1 },
>>  		},
>>  		.fifosize = 16,
>>  		.overrun_reg = SCLSR,
>> @@ -3380,6 +3383,7 @@ static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
>>  	}
>>  
>>  	sp = &sci_ports[id];
>> +	sp->rstc = rstc;
>>  	*dev_id = id;
>>  
>>  	p->type = SCI_OF_TYPE(data);
>> @@ -3507,13 +3511,34 @@ static int sci_probe(struct platform_device *dev)
>>  	return 0;
>>  }
>>  
>> +static void sci_console_setup(struct sci_port *s, bool save)
>> +{
>> +	for (u16 i = 0; i < SCIx_NR_REGS; i++) {
>> +		struct uart_port *port = &s->port;
>> +
>> +		if (!s->params->regs[i].suspend_cacheable)
>> +			continue;
>> +
>> +		if (save)
>> +			s->console_cached_regs[i] = sci_serial_in(port, i);
>> +		else
>> +			sci_serial_out(port, i, s->console_cached_regs[i]);
>> +	}
>> +}
>> +
>>  static __maybe_unused int sci_suspend(struct device *dev)
>>  {
>>  	struct sci_port *sport = dev_get_drvdata(dev);
>>  
>> -	if (sport)
>> +	if (sport) {
>>  		uart_suspend_port(&sci_uart_driver, &sport->port);
>>  
>> +		if (!console_suspend_enabled && uart_console(&sport->port))
>> +			sci_console_setup(sport, true);
>> +		else
>> +			return reset_control_assert(sport->rstc);
>> +	}
>> +
>>  	return 0;
>>  }
>>  
>> @@ -3521,8 +3546,18 @@ static __maybe_unused int sci_resume(struct device *dev)
>>  {
>>  	struct sci_port *sport = dev_get_drvdata(dev);
>>  
>> -	if (sport)
>> +	if (sport) {
>> +		if (!console_suspend_enabled && uart_console(&sport->port)) {
>> +			sci_console_setup(sport, false);
>> +		} else {
>> +			int ret = reset_control_deassert(sport->rstc);
> 
> With this, is the reset_control_deassert() in sci_parse_dt() still
> needed?

If I'm not wrongly understanding your question, yes, the
reset_control_deassert() is still needed in the sci_parse_dt() as the
sci_parse_dt() is called on probe path. After resume the sci_parse_dt() is
not called unless the driver is unbinded and then re-binded.

In case the reset_control_dessert() here fails (or not) and an
unbind/re-bind will be requested, the unbind will call
reset_control_assert() (though the devm action) and then the re-bind will
call reset_control_deassert() though sci_parse_dt(). That should be safe,
AFAICT.


> 
> Likewise, does the reset_control_assert() in sci_suspend() remove the
> need for the sci_reset_control_assert() devm action?

No, the sci_reset_control_assert() is still needed as explained above,
unless I missed your point.

Please let me know if missed your point and/or answered your question?


Thank you,
Claudiu Beznea

> 
> regards
> Philipp

Re: [PATCH v3 3/8] serial: sh-sci: Update the suspend/resume support

[Philipp Zabel]

On Mo, 2024-11-18 at 11:47 +0200, Claudiu Beznea wrote:
> Hi, Philipp,
> 
> On 15.11.2024 17:40, Philipp Zabel wrote:
> > On Fr, 2024-11-15 at 15:43 +0200, Claudiu wrote:
> > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > > 
> > > The Renesas RZ/G3S supports a power saving mode where power to most of the
> > > SoC components is turned off. When returning from this power saving mode,
> > > SoC components need to be re-configured.
> > > 
> > > The SCIFs on the Renesas RZ/G3S need to be re-configured as well when
> > > returning from this power saving mode. The sh-sci code already configures
> > > the SCIF clocks, power domain and registers by calling uart_resume_port()
> > > in sci_resume(). On suspend path the SCIF UART ports are suspended
> > > accordingly (by calling uart_suspend_port() in sci_suspend()). The only
> > > missing setting is the reset signal. For this assert/de-assert the reset
> > > signal on driver suspend/resume.
> > > 
> > > In case the no_console_suspend is specified by the user, the registers need
> > > to be saved on suspend path and restore on resume path. To do this the
> > > sci_console_setup() function was added. There is no need to cache/restore
> > > the status or FIFO registers. Only the control registers. To differentiate
> > > b/w these, the struct sci_port_params::regs was updated with a new member
> > > that specifies if the register needs to be chached on suspend. Only the
> > > RZ_SCIFA instances were updated with this new support as the hardware for
> > > the rest of variants was missing for testing.
> > > 
> > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > > ---
> > > 
> > > Changes in v3:
> > > - none
> > > 
> > > Changes in v2:
> > > - rebased on top of the update version of patch 2/8 from
> > >   this series
> > > 
> > >  drivers/tty/serial/sh-sci.c | 53 ++++++++++++++++++++++++++++++-------
> > >  1 file changed, 44 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> > > index ade151ff39d2..e53496d2708e 100644
> > > --- a/drivers/tty/serial/sh-sci.c
> > > +++ b/drivers/tty/serial/sh-sci.c
> > > @@ -101,7 +101,7 @@ enum SCI_CLKS {
> > >  		if ((_port)->sampling_rate_mask & SCI_SR((_sr)))
> > >  
> > >  struct plat_sci_reg {
> > > -	u8 offset, size;
> > > +	u8 offset, size, suspend_cacheable;
> > >  };
> > >  
> > >  struct sci_port_params {
> > > @@ -134,6 +134,8 @@ struct sci_port {
> > >  	struct dma_chan			*chan_tx;
> > >  	struct dma_chan			*chan_rx;
> > >  
> > > +	struct reset_control		*rstc;
> > > +
> > >  #ifdef CONFIG_SERIAL_SH_SCI_DMA
> > >  	struct dma_chan			*chan_tx_saved;
> > >  	struct dma_chan			*chan_rx_saved;
> > > @@ -153,6 +155,7 @@ struct sci_port {
> > >  	int				rx_trigger;
> > >  	struct timer_list		rx_fifo_timer;
> > >  	int				rx_fifo_timeout;
> > > +	unsigned int			console_cached_regs[SCIx_NR_REGS];
> > >  	u16				hscif_tot;
> > >  
> > >  	bool has_rtscts;
> > > @@ -298,17 +301,17 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = {
> > >  	 */
> > >  	[SCIx_RZ_SCIFA_REGTYPE] = {
> > >  		.regs = {
> > > -			[SCSMR]		= { 0x00, 16 },
> > > -			[SCBRR]		= { 0x02,  8 },
> > > -			[SCSCR]		= { 0x04, 16 },
> > > +			[SCSMR]		= { 0x00, 16, 1 },
> > > +			[SCBRR]		= { 0x02,  8, 1 },
> > > +			[SCSCR]		= { 0x04, 16, 1 },
> > >  			[SCxTDR]	= { 0x06,  8 },
> > >  			[SCxSR]		= { 0x08, 16 },
> > >  			[SCxRDR]	= { 0x0A,  8 },
> > > -			[SCFCR]		= { 0x0C, 16 },
> > > +			[SCFCR]		= { 0x0C, 16, 1 },
> > >  			[SCFDR]		= { 0x0E, 16 },
> > > -			[SCSPTR]	= { 0x10, 16 },
> > > +			[SCSPTR]	= { 0x10, 16, 1 },
> > >  			[SCLSR]		= { 0x12, 16 },
> > > -			[SEMR]		= { 0x14, 8 },
> > > +			[SEMR]		= { 0x14, 8, 1 },
> > >  		},
> > >  		.fifosize = 16,
> > >  		.overrun_reg = SCLSR,
> > > @@ -3380,6 +3383,7 @@ static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
> > >  	}
> > >  
> > >  	sp = &sci_ports[id];
> > > +	sp->rstc = rstc;
> > >  	*dev_id = id;
> > >  
> > >  	p->type = SCI_OF_TYPE(data);
> > > @@ -3507,13 +3511,34 @@ static int sci_probe(struct platform_device *dev)
> > >  	return 0;
> > >  }
> > >  
> > > +static void sci_console_setup(struct sci_port *s, bool save)
> > > +{
> > > +	for (u16 i = 0; i < SCIx_NR_REGS; i++) {
> > > +		struct uart_port *port = &s->port;
> > > +
> > > +		if (!s->params->regs[i].suspend_cacheable)
> > > +			continue;
> > > +
> > > +		if (save)
> > > +			s->console_cached_regs[i] = sci_serial_in(port, i);
> > > +		else
> > > +			sci_serial_out(port, i, s->console_cached_regs[i]);
> > > +	}
> > > +}
> > > +
> > >  static __maybe_unused int sci_suspend(struct device *dev)
> > >  {
> > >  	struct sci_port *sport = dev_get_drvdata(dev);
> > >  
> > > -	if (sport)
> > > +	if (sport) {
> > >  		uart_suspend_port(&sci_uart_driver, &sport->port);
> > >  
> > > +		if (!console_suspend_enabled && uart_console(&sport->port))
> > > +			sci_console_setup(sport, true);
> > > +		else
> > > +			return reset_control_assert(sport->rstc);
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > @@ -3521,8 +3546,18 @@ static __maybe_unused int sci_resume(struct device *dev)
> > >  {
> > >  	struct sci_port *sport = dev_get_drvdata(dev);
> > >  
> > > -	if (sport)
> > > +	if (sport) {
> > > +		if (!console_suspend_enabled && uart_console(&sport->port)) {
> > > +			sci_console_setup(sport, false);
> > > +		} else {
> > > +			int ret = reset_control_deassert(sport->rstc);
> > 
> > With this, is the reset_control_deassert() in sci_parse_dt() still
> > needed?
> 
> If I'm not wrongly understanding your question, yes, the
> reset_control_deassert() is still needed in the sci_parse_dt() as the
> sci_parse_dt() is called on probe path. After resume the sci_parse_dt() is
> not called unless the driver is unbinded and then re-binded.

Ah, I was thinking of runtime PM resume callbacks. Thank you for the
answer, this cleared my confusion.

regards
Philipp

[PATCH v3 4/8] arm64: dts: renesas: r9a08g045: Add the remaining SCIF interfaces

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The Renesas RZ/G3S SoC has 6 SCIF interfaces. SCIF0 is used as debug
console. Add the remaining ones.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v3:
- none

Changes in v2:
- none

 arch/arm64/boot/dts/renesas/r9a08g045.dtsi | 90 ++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
index be8a0a768c65..5b15ff2482ab 100644
--- a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
@@ -73,6 +73,96 @@ scif0: serial@1004b800 {
 			status = "disabled";
 		};
 
+		scif1: serial@1004bc00 {
+			compatible = "renesas,scif-r9a08g045", "renesas,scif-r9a07g044";
+			reg = <0 0x1004bc00 0 0x400>;
+			interrupts = <GIC_SPI 325 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 327 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 328 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 326 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 329 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 329 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "eri", "rxi", "txi",
+					  "bri", "dri", "tei";
+			clocks = <&cpg CPG_MOD R9A08G045_SCIF1_CLK_PCK>;
+			clock-names = "fck";
+			power-domains = <&cpg>;
+			resets = <&cpg R9A08G045_SCIF1_RST_SYSTEM_N>;
+			status = "disabled";
+		};
+
+		scif2: serial@1004c000 {
+			compatible = "renesas,scif-r9a08g045", "renesas,scif-r9a07g044";
+			reg = <0 0x1004c000 0 0x400>;
+			interrupts = <GIC_SPI 330 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 332 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 333 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 331 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 334 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 334 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "eri", "rxi", "txi",
+					  "bri", "dri", "tei";
+			clocks = <&cpg CPG_MOD R9A08G045_SCIF2_CLK_PCK>;
+			clock-names = "fck";
+			power-domains = <&cpg>;
+			resets = <&cpg R9A08G045_SCIF2_RST_SYSTEM_N>;
+			status = "disabled";
+		};
+
+		scif3: serial@1004c400 {
+			compatible = "renesas,scif-r9a08g045", "renesas,scif-r9a07g044";
+			reg = <0 0x1004c400 0 0x400>;
+			interrupts = <GIC_SPI 335 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 337 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 338 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 336 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 339 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 339 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "eri", "rxi", "txi",
+					  "bri", "dri", "tei";
+			clocks = <&cpg CPG_MOD R9A08G045_SCIF3_CLK_PCK>;
+			clock-names = "fck";
+			power-domains = <&cpg>;
+			resets = <&cpg R9A08G045_SCIF3_RST_SYSTEM_N>;
+			status = "disabled";
+		};
+
+		scif4: serial@1004c800 {
+			compatible = "renesas,scif-r9a08g045", "renesas,scif-r9a07g044";
+			reg = <0 0x1004c800 0 0x400>;
+			interrupts = <GIC_SPI 340 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 342 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 343 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 341 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 344 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 344 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "eri", "rxi", "txi",
+					  "bri", "dri", "tei";
+			clocks = <&cpg CPG_MOD R9A08G045_SCIF4_CLK_PCK>;
+			clock-names = "fck";
+			power-domains = <&cpg>;
+			resets = <&cpg R9A08G045_SCIF4_RST_SYSTEM_N>;
+			status = "disabled";
+		};
+
+		scif5: serial@1004e000 {
+			compatible = "renesas,scif-r9a08g045", "renesas,scif-r9a07g044";
+			reg = <0 0x1004e000 0 0x400>;
+			interrupts = <GIC_SPI 345 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 347 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 348 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 346 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 349 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 349 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "eri", "rxi", "txi",
+					  "bri", "dri", "tei";
+			clocks = <&cpg CPG_MOD R9A08G045_SCIF5_CLK_PCK>;
+			clock-names = "fck";
+			power-domains = <&cpg>;
+			resets = <&cpg R9A08G045_SCIF5_RST_SYSTEM_N>;
+			status = "disabled";
+		};
+
 		rtc: rtc@1004ec00 {
 			compatible = "renesas,r9a08g045-rtca3", "renesas,rz-rtca3";
 			reg = <0 0x1004ec00 0 0x400>;
-- 
2.39.2

Re: [PATCH v3 4/8] arm64: dts: renesas: r9a08g045: Add the remaining SCIF interfaces

[Geert Uytterhoeven]

On Fri, Nov 15, 2024 at 2:50 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The Renesas RZ/G3S SoC has 6 SCIF interfaces. SCIF0 is used as debug
> console. Add the remaining ones.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel for v6.14.

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

[PATCH v3 5/8] arm64: dts: renesas: rzg3s-smarc: Fix the debug serial alias

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The debug serial of the RZ/G3S is SCIF0 which is routed on the Renesas
RZ SMARC Carrier II board on the SER3_UART. Use serial3 alias for it for
better hardware description. Along with it, the chosen properties were
moved to the device tree corresponding to the RZ SMARC Carrier II board.

Fixes: adb4f0c5699c ("arm64: dts: renesas: Add initial support for RZ/G3S SMARC SoM")
Fixes: d1ae4200bb26 ("arm64: dts: renesas: Add initial device tree for RZ SMARC Carrier-II Board")
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v3:
- none

Changes in v2:
- none

 arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi | 5 -----
 arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi     | 7 ++++++-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
index 2ed01d391554..55c72c8a0735 100644
--- a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
@@ -43,11 +43,6 @@ aliases {
 #endif
 	};
 
-	chosen {
-		bootargs = "ignore_loglevel";
-		stdout-path = "serial0:115200n8";
-	};
-
 	memory@48000000 {
 		device_type = "memory";
 		/* First 128MB is reserved for secure area. */
diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi b/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
index 4509151344c4..33b9873b225a 100644
--- a/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
@@ -12,10 +12,15 @@
 / {
 	aliases {
 		i2c0 = &i2c0;
-		serial0 = &scif0;
+		serial3 = &scif0;
 		mmc1 = &sdhi1;
 	};
 
+	chosen {
+		bootargs = "ignore_loglevel";
+		stdout-path = "serial3:115200n8";
+	};
+
 	keys {
 		compatible = "gpio-keys";
 
-- 
2.39.2

Re: [PATCH v3 5/8] arm64: dts: renesas: rzg3s-smarc: Fix the debug serial alias

[Geert Uytterhoeven]

Hi Claudiu,

On Fri, Nov 15, 2024 at 2:50 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The debug serial of the RZ/G3S is SCIF0 which is routed on the Renesas
> RZ SMARC Carrier II board on the SER3_UART. Use serial3 alias for it for
> better hardware description. Along with it, the chosen properties were
> moved to the device tree corresponding to the RZ SMARC Carrier II board.
>
> Fixes: adb4f0c5699c ("arm64: dts: renesas: Add initial support for RZ/G3S SMARC SoM")
> Fixes: d1ae4200bb26 ("arm64: dts: renesas: Add initial device tree for RZ SMARC Carrier-II Board")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

> --- a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
> +++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
> @@ -43,11 +43,6 @@ aliases {
>  #endif
>         };
>
> -       chosen {
> -               bootargs = "ignore_loglevel";

I'd say please keep bootargs here, but as you don't support using the
SoM without the carrier board, I guess it's fine to keep it together
with stdout-path.

> -               stdout-path = "serial0:115200n8";
> -       };

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel for v6.14.

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

[PATCH v3 6/8] arm64: dts: renesas: rzg3s-smarc-switches: Add a header to describe different switches

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

There are different switches available on both the RZ/G3S SMARC Module and
RZ SMARC Carrier II boards. These switches are used to route different SoC
signals to different parts available on board.

These switches are described in device trees through macros. These macros
are set accordingly such that the resulted compiled dtb to describe the
on-board switches states.

Based on the SW_CONFIG3 switch state (populated on the module board), the
SCIF3 SoC interface is routed or not to an U(S)ART pin header available on
the carrier board. As the SCIF3 is accessible through the carrier board,
the device tree enables it in the carrier DTS. To be able to cope with
these type of configurations, add a header file where all the on-board
switches can be described and shared accordingly between module and carrier
board.

Commit prepares the code to enable SCIF3 on the RZ/G3S carrier device
tree.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v3:
- none

Changes in v2:
- none

 .../boot/dts/renesas/rzg3s-smarc-som.dtsi     | 20 +-----------
 .../boot/dts/renesas/rzg3s-smarc-switches.h   | 32 +++++++++++++++++++
 2 files changed, 33 insertions(+), 19 deletions(-)
 create mode 100644 arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h

diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
index 55c72c8a0735..5c88e130c89e 100644
--- a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
@@ -9,25 +9,7 @@
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/pinctrl/rzg2l-pinctrl.h>
 
-/*
- * On-board switches' states:
- * @SW_OFF: switch's state is OFF
- * @SW_ON:  switch's state is ON
- */
-#define SW_OFF		0
-#define SW_ON		1
-
-/*
- * SW_CONFIG[x] switches' states:
- * @SW_CONFIG2:
- *	SW_OFF - SD0 is connected to eMMC
- *	SW_ON  - SD0 is connected to uSD0 card
- * @SW_CONFIG3:
- *	SW_OFF - SD2 is connected to SoC
- *	SW_ON  - SCIF1, SSI0, IRQ0, IRQ1 connected to SoC
- */
-#define SW_CONFIG2	SW_OFF
-#define SW_CONFIG3	SW_ON
+#include "rzg3s-smarc-switches.h"
 
 / {
 	compatible = "renesas,rzg3s-smarcm", "renesas,r9a08g045s33", "renesas,r9a08g045";
diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h b/arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h
new file mode 100644
index 000000000000..e2d9b953f627
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * On-board switches for the Renesas RZ/G3S SMARC Module and RZ SMARC Carrier II
+ * boards.
+ *
+ * Copyright (C) 2024 Renesas Electronics Corp.
+ */
+
+#ifndef __RZG3S_SMARC_SWITCHES__
+#define __RZG3S_SMARC_SWITCHES__
+
+/*
+ * On-board switches' states:
+ * @SW_OFF: switch's state is OFF
+ * @SW_ON:  switch's state is ON
+ */
+#define SW_OFF		0
+#define SW_ON		1
+
+/*
+ * SW_CONFIG[x] switches' states:
+ * @SW_CONFIG2:
+ *	SW_OFF - SD0 is connected to eMMC
+ *	SW_ON  - SD0 is connected to uSD0 card
+ * @SW_CONFIG3:
+ *	SW_OFF - SD2 is connected to SoC
+ *	SW_ON  - SCIF3, SSI3, IRQ0, IRQ1 connected to SoC
+ */
+#define SW_CONFIG2	SW_OFF
+#define SW_CONFIG3	SW_ON
+
+#endif /* __RZG3S_SMARC_SWITCHES__ */
-- 
2.39.2

Re: [PATCH v3 6/8] arm64: dts: renesas: rzg3s-smarc-switches: Add a header to describe different switches

[Rob Herring]

On Fri, Nov 15, 2024 at 03:43:59PM +0200, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> There are different switches available on both the RZ/G3S SMARC Module and
> RZ SMARC Carrier II boards. These switches are used to route different SoC
> signals to different parts available on board.
> 
> These switches are described in device trees through macros. These macros
> are set accordingly such that the resulted compiled dtb to describe the
> on-board switches states.
> 
> Based on the SW_CONFIG3 switch state (populated on the module board), the
> SCIF3 SoC interface is routed or not to an U(S)ART pin header available on
> the carrier board. As the SCIF3 is accessible through the carrier board,
> the device tree enables it in the carrier DTS. To be able to cope with
> these type of configurations, add a header file where all the on-board
> switches can be described and shared accordingly between module and carrier
> board.
> 
> Commit prepares the code to enable SCIF3 on the RZ/G3S carrier device
> tree.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v3:
> - none
> 
> Changes in v2:
> - none
> 
>  .../boot/dts/renesas/rzg3s-smarc-som.dtsi     | 20 +-----------
>  .../boot/dts/renesas/rzg3s-smarc-switches.h   | 32 +++++++++++++++++++
>  2 files changed, 33 insertions(+), 19 deletions(-)
>  create mode 100644 arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h
> 
> diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
> index 55c72c8a0735..5c88e130c89e 100644
> --- a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
> +++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
> @@ -9,25 +9,7 @@
>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/pinctrl/rzg2l-pinctrl.h>
>  
> -/*
> - * On-board switches' states:
> - * @SW_OFF: switch's state is OFF
> - * @SW_ON:  switch's state is ON
> - */
> -#define SW_OFF		0
> -#define SW_ON		1
> -
> -/*
> - * SW_CONFIG[x] switches' states:
> - * @SW_CONFIG2:
> - *	SW_OFF - SD0 is connected to eMMC
> - *	SW_ON  - SD0 is connected to uSD0 card
> - * @SW_CONFIG3:
> - *	SW_OFF - SD2 is connected to SoC
> - *	SW_ON  - SCIF1, SSI0, IRQ0, IRQ1 connected to SoC
> - */
> -#define SW_CONFIG2	SW_OFF
> -#define SW_CONFIG3	SW_ON
> +#include "rzg3s-smarc-switches.h"
>  
>  / {
>  	compatible = "renesas,rzg3s-smarcm", "renesas,r9a08g045s33", "renesas,r9a08g045";
> diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h b/arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h
> new file mode 100644
> index 000000000000..e2d9b953f627
> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

Use the same license as the .dtsi file.

> +/*
> + * On-board switches for the Renesas RZ/G3S SMARC Module and RZ SMARC Carrier II
> + * boards.
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#ifndef __RZG3S_SMARC_SWITCHES__
> +#define __RZG3S_SMARC_SWITCHES__
> +
> +/*
> + * On-board switches' states:
> + * @SW_OFF: switch's state is OFF
> + * @SW_ON:  switch's state is ON
> + */
> +#define SW_OFF		0
> +#define SW_ON		1
> +
> +/*
> + * SW_CONFIG[x] switches' states:
> + * @SW_CONFIG2:
> + *	SW_OFF - SD0 is connected to eMMC
> + *	SW_ON  - SD0 is connected to uSD0 card
> + * @SW_CONFIG3:
> + *	SW_OFF - SD2 is connected to SoC
> + *	SW_ON  - SCIF3, SSI3, IRQ0, IRQ1 connected to SoC
> + */
> +#define SW_CONFIG2	SW_OFF
> +#define SW_CONFIG3	SW_ON
> +
> +#endif /* __RZG3S_SMARC_SWITCHES__ */
> -- 
> 2.39.2
> 

Re: [PATCH v3 6/8] arm64: dts: renesas: rzg3s-smarc-switches: Add a header to describe different switches

[Geert Uytterhoeven]

Hi Claudiu,

On Fri, Nov 15, 2024 at 2:50 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> There are different switches available on both the RZ/G3S SMARC Module and
> RZ SMARC Carrier II boards. These switches are used to route different SoC
> signals to different parts available on board.
>
> These switches are described in device trees through macros. These macros
> are set accordingly such that the resulted compiled dtb to describe the
> on-board switches states.
>
> Based on the SW_CONFIG3 switch state (populated on the module board), the
> SCIF3 SoC interface is routed or not to an U(S)ART pin header available on
> the carrier board. As the SCIF3 is accessible through the carrier board,
> the device tree enables it in the carrier DTS. To be able to cope with
> these type of configurations, add a header file where all the on-board
> switches can be described and shared accordingly between module and carrier
> board.
>
> Commit prepares the code to enable SCIF3 on the RZ/G3S carrier device
> tree.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

> --- a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
> +++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
> @@ -9,25 +9,7 @@
>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/pinctrl/rzg2l-pinctrl.h>
>
> -/*
> - * On-board switches' states:
> - * @SW_OFF: switch's state is OFF
> - * @SW_ON:  switch's state is ON
> - */
> -#define SW_OFF         0
> -#define SW_ON          1
> -
> -/*
> - * SW_CONFIG[x] switches' states:
> - * @SW_CONFIG2:
> - *     SW_OFF - SD0 is connected to eMMC
> - *     SW_ON  - SD0 is connected to uSD0 card
> - * @SW_CONFIG3:
> - *     SW_OFF - SD2 is connected to SoC
> - *     SW_ON  - SCIF1, SSI0, IRQ0, IRQ1 connected to SoC
> - */
> -#define SW_CONFIG2     SW_OFF
> -#define SW_CONFIG3     SW_ON
> +#include "rzg3s-smarc-switches.h"
>
>  / {
>         compatible = "renesas,rzg3s-smarcm", "renesas,r9a08g045s33", "renesas,r9a08g045";
> diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h b/arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h
> new file mode 100644
> index 000000000000..e2d9b953f627
> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

I agree with Rob about the license.

> +/*
> + * On-board switches for the Renesas RZ/G3S SMARC Module and RZ SMARC Carrier II
> + * boards.
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#ifndef __RZG3S_SMARC_SWITCHES__
> +#define __RZG3S_SMARC_SWITCHES__
> +
> +/*
> + * On-board switches' states:
> + * @SW_OFF: switch's state is OFF
> + * @SW_ON:  switch's state is ON
> + */
> +#define SW_OFF         0
> +#define SW_ON          1
> +
> +/*
> + * SW_CONFIG[x] switches' states:
> + * @SW_CONFIG2:
> + *     SW_OFF - SD0 is connected to eMMC
> + *     SW_ON  - SD0 is connected to uSD0 card
> + * @SW_CONFIG3:
> + *     SW_OFF - SD2 is connected to SoC
> + *     SW_ON  - SCIF3, SSI3, IRQ0, IRQ1 connected to SoC

Note that the original comment above says "SCIF1, SSI0", and looking
at the schematics (IC7 and IC8 controlled by SW_SD2_EN#), that is
actually correct?

> + */
> +#define SW_CONFIG2     SW_OFF
> +#define SW_CONFIG3     SW_ON
> +
> +#endif /* __RZG3S_SMARC_SWITCHES__ */

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 v3 6/8] arm64: dts: renesas: rzg3s-smarc-switches: Add a header to describe different switches

[Claudiu Beznea]

Hi, Geert,

On 09.12.2024 12:09, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Fri, Nov 15, 2024 at 2:50 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> There are different switches available on both the RZ/G3S SMARC Module and
>> RZ SMARC Carrier II boards. These switches are used to route different SoC
>> signals to different parts available on board.
>>
>> These switches are described in device trees through macros. These macros
>> are set accordingly such that the resulted compiled dtb to describe the
>> on-board switches states.
>>
>> Based on the SW_CONFIG3 switch state (populated on the module board), the
>> SCIF3 SoC interface is routed or not to an U(S)ART pin header available on
>> the carrier board. As the SCIF3 is accessible through the carrier board,
>> the device tree enables it in the carrier DTS. To be able to cope with
>> these type of configurations, add a header file where all the on-board
>> switches can be described and shared accordingly between module and carrier
>> board.
>>
>> Commit prepares the code to enable SCIF3 on the RZ/G3S carrier device
>> tree.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Thanks for your patch!
> 
>> --- a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
>> +++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
>> @@ -9,25 +9,7 @@
>>  #include <dt-bindings/gpio/gpio.h>
>>  #include <dt-bindings/pinctrl/rzg2l-pinctrl.h>
>>
>> -/*
>> - * On-board switches' states:
>> - * @SW_OFF: switch's state is OFF
>> - * @SW_ON:  switch's state is ON
>> - */
>> -#define SW_OFF         0
>> -#define SW_ON          1
>> -
>> -/*
>> - * SW_CONFIG[x] switches' states:
>> - * @SW_CONFIG2:
>> - *     SW_OFF - SD0 is connected to eMMC
>> - *     SW_ON  - SD0 is connected to uSD0 card
>> - * @SW_CONFIG3:
>> - *     SW_OFF - SD2 is connected to SoC
>> - *     SW_ON  - SCIF1, SSI0, IRQ0, IRQ1 connected to SoC
>> - */
>> -#define SW_CONFIG2     SW_OFF
>> -#define SW_CONFIG3     SW_ON
>> +#include "rzg3s-smarc-switches.h"
>>
>>  / {
>>         compatible = "renesas,rzg3s-smarcm", "renesas,r9a08g045s33", "renesas,r9a08g045";
>> diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h b/arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h
>> new file mode 100644
>> index 000000000000..e2d9b953f627
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
> 
> I agree with Rob about the license.
> 
>> +/*
>> + * On-board switches for the Renesas RZ/G3S SMARC Module and RZ SMARC Carrier II
>> + * boards.
>> + *
>> + * Copyright (C) 2024 Renesas Electronics Corp.
>> + */
>> +
>> +#ifndef __RZG3S_SMARC_SWITCHES__
>> +#define __RZG3S_SMARC_SWITCHES__
>> +
>> +/*
>> + * On-board switches' states:
>> + * @SW_OFF: switch's state is OFF
>> + * @SW_ON:  switch's state is ON
>> + */
>> +#define SW_OFF         0
>> +#define SW_ON          1
>> +
>> +/*
>> + * SW_CONFIG[x] switches' states:
>> + * @SW_CONFIG2:
>> + *     SW_OFF - SD0 is connected to eMMC
>> + *     SW_ON  - SD0 is connected to uSD0 card
>> + * @SW_CONFIG3:
>> + *     SW_OFF - SD2 is connected to SoC
>> + *     SW_ON  - SCIF3, SSI3, IRQ0, IRQ1 connected to SoC
> 
> Note that the original comment above says "SCIF1, SSI0", and looking
> at the schematics (IC7 and IC8 controlled by SW_SD2_EN#), that is
> actually correct?

You're right, I'm not sure why I've changed it. I'll fix it in the next
version.

Thank  you for your review,
Claudiu

> 
>> + */
>> +#define SW_CONFIG2     SW_OFF
>> +#define SW_CONFIG3     SW_ON
>> +
>> +#endif /* __RZG3S_SMARC_SWITCHES__ */
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

[PATCH v3 7/8] arm64: dts: renesas: rzg3s-smarc: Enable SCIF3

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Enable SCIF3. It is routed on the RZ SMARC Carrier II board on SER1_UART
interface.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v3:
- none

Changes in v2:
- none

 arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi b/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
index 33b9873b225a..1be21ece131e 100644
--- a/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
@@ -9,9 +9,14 @@
 #include <dt-bindings/input/input.h>
 #include <dt-bindings/pinctrl/rzg2l-pinctrl.h>
 
+#include "rzg3s-smarc-switches.h"
+
 / {
 	aliases {
 		i2c0 = &i2c0;
+#if SW_CONFIG3 == SW_ON
+		serial1 = &scif3;
+#endif
 		serial3 = &scif0;
 		mmc1 = &sdhi1;
 	};
@@ -102,6 +107,11 @@ scif0_pins: scif0 {
 			 <RZG2L_PORT_PINMUX(6, 4, 1)>; /* TXD */
 	};
 
+	scif3_pins: scif3 {
+		pinmux = <RZG2L_PORT_PINMUX(17, 2, 7)>, /* RXD */
+			 <RZG2L_PORT_PINMUX(17, 3, 7)>; /* TXD */
+	};
+
 	sdhi1_pins: sd1 {
 		data {
 			pins = "SD1_DATA0", "SD1_DATA1", "SD1_DATA2", "SD1_DATA3";
@@ -141,6 +151,14 @@ &scif0 {
 	status = "okay";
 };
 
+#if SW_CONFIG3 == SW_ON
+&scif3 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&scif3_pins>;
+	status = "okay";
+};
+#endif
+
 &sdhi1 {
 	pinctrl-0 = <&sdhi1_pins>;
 	pinctrl-1 = <&sdhi1_pins_uhs>;
-- 
2.39.2

Re: [PATCH v3 7/8] arm64: dts: renesas: rzg3s-smarc: Enable SCIF3

[Geert Uytterhoeven]

Hi Claudiu,

On Fri, Nov 15, 2024 at 2:50 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Enable SCIF3. It is routed on the RZ SMARC Carrier II board on SER1_UART
> interface.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

> --- a/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
> +++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
> @@ -9,9 +9,14 @@
>  #include <dt-bindings/input/input.h>
>  #include <dt-bindings/pinctrl/rzg2l-pinctrl.h>
>
> +#include "rzg3s-smarc-switches.h"
> +
>  / {
>         aliases {
>                 i2c0 = &i2c0;
> +#if SW_CONFIG3 == SW_ON
> +               serial1 = &scif3;
> +#endif

According to my schematics, SCIF3 is routed unconditionally to
SER1_UART.

>                 serial3 = &scif0;
>                 mmc1 = &sdhi1;
>         };

The rest LGTM.

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

[PATCH v3 8/8] arm64: dts: renesas: r9a08g045s33-smarc-pmod: Add overlay for SCIF1

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Add DT overlay for SCIF1 (of the Renesas RZ/G3S SoC) routed through the
PMOD1_3A interface available on the Renesas RZ SMARC Carrier II board.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v3:
- none

Changes in v2:
- none

 arch/arm64/boot/dts/renesas/Makefile          |  3 ++
 .../dts/renesas/r9a08g045s33-smarc-pmod.dtso  | 48 +++++++++++++++++++
 2 files changed, 51 insertions(+)
 create mode 100644 arch/arm64/boot/dts/renesas/r9a08g045s33-smarc-pmod.dtso

diff --git a/arch/arm64/boot/dts/renesas/Makefile b/arch/arm64/boot/dts/renesas/Makefile
index 97228a3cb99c..7ad52630d350 100644
--- a/arch/arm64/boot/dts/renesas/Makefile
+++ b/arch/arm64/boot/dts/renesas/Makefile
@@ -137,6 +137,9 @@ r9a07g054l2-smarc-cru-csi-ov5645-dtbs := r9a07g054l2-smarc.dtb r9a07g054l2-smarc
 dtb-$(CONFIG_ARCH_R9A07G054) += r9a07g054l2-smarc-cru-csi-ov5645.dtb
 
 dtb-$(CONFIG_ARCH_R9A08G045) += r9a08g045s33-smarc.dtb
+dtb-$(CONFIG_ARCH_R9A07G043) += r9a08g045s33-smarc-pmod.dtbo
+r9a08g045s33-smarc-pmod-dtbs := r9a08g045s33-smarc.dtb r9a08g045s33-smarc-pmod.dtbo
+dtb-$(CONFIG_ARCH_R9A07G043) += r9a08g045s33-smarc-pmod.dtb
 
 dtb-$(CONFIG_ARCH_R9A09G011) += r9a09g011-v2mevk2.dtb
 
diff --git a/arch/arm64/boot/dts/renesas/r9a08g045s33-smarc-pmod.dtso b/arch/arm64/boot/dts/renesas/r9a08g045s33-smarc-pmod.dtso
new file mode 100644
index 000000000000..7d637ab110e1
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/r9a08g045s33-smarc-pmod.dtso
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device Tree Source for the RZ/G3S SMARC Carrier II EVK PMOD parts
+ *
+ * Copyright (C) 2024 Renesas Electronics Corp.
+ *
+ *
+ * [Connection]
+ *
+ * SMARC Carrier II EVK
+ * +--------------------------------------------+
+ * |PMOD1_3A (PMOD1 PIN HEADER)			|
+ * |	SCIF1_CTS# (pin1)  (pin7)  PMOD1_GPIO10	|
+ * |	SCIF1_TXD  (pin2)  (pin8)  PMOD1_GPIO11	|
+ * |	SCIF1_RXD  (pin3)  (pin9)  PMOD1_GPIO12	|
+ * |	SCIF1_RTS# (pin4)  (pin10) PMOD1_GPIO13	|
+ * |	GND	   (pin5)  (pin11) GND		|
+ * |	PWR_PMOD1  (pin6)  (pin12) GND		|
+ * +--------------------------------------------+
+ *
+ */
+
+/dts-v1/;
+/plugin/;
+
+#include <dt-bindings/pinctrl/rzg2l-pinctrl.h>
+
+&{/} {
+	aliases {
+		serial0 = "/soc/serial@1004bc00";
+	};
+};
+
+&pinctrl {
+	scif1_pins: scif1-pins {
+		pinmux = <RZG2L_PORT_PINMUX(14, 0, 1)>, /* TXD */
+			 <RZG2L_PORT_PINMUX(14, 1, 1)>, /* RXD */
+			 <RZG2L_PORT_PINMUX(16, 0, 1)>, /* CTS */
+			 <RZG2L_PORT_PINMUX(16, 1, 1)>; /* RTS */
+	};
+};
+
+&scif1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&scif1_pins>;
+	uart-has-rtscts;
+	status = "okay";
+};
-- 
2.39.2

Re: [PATCH v3 8/8] arm64: dts: renesas: r9a08g045s33-smarc-pmod: Add overlay for SCIF1

[Geert Uytterhoeven]

Hi Claudiu,

On Fri, Nov 15, 2024 at 2:50 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Add DT overlay for SCIF1 (of the Renesas RZ/G3S SoC) routed through the
> PMOD1_3A interface available on the Renesas RZ SMARC Carrier II board.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/r9a08g045s33-smarc-pmod.dtso
> @@ -0,0 +1,48 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree Source for the RZ/G3S SMARC Carrier II EVK PMOD parts

So you plan to describe all 3 PMOD interfaces in a single overlay?
The board has:
  1. PMOD0 - Type-2A,
  2. PMOD1 - Tpye-3A,
  3. PMOD1 - Type 6A.
Wouldn't it be more convenient to have separate overlays for each port,
so you can more easily use them as e.g. Type-1(A) (GPIO only)?

BTW, naming both the second and third port "PMOD1" in the schematics,
and differentiating them by their type, was definitely a bad idea.
How can you distinguish between Type-1(A) on the second or third port?

> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + *
> + *
> + * [Connection]
> + *
> + * SMARC Carrier II EVK
> + * +--------------------------------------------+
> + * |PMOD1_3A (PMOD1 PIN HEADER)                        |
> + * |   SCIF1_CTS# (pin1)  (pin7)  PMOD1_GPIO10 |
> + * |   SCIF1_TXD  (pin2)  (pin8)  PMOD1_GPIO11 |
> + * |   SCIF1_RXD  (pin3)  (pin9)  PMOD1_GPIO12 |
> + * |   SCIF1_RTS# (pin4)  (pin10) PMOD1_GPIO13 |
> + * |   GND        (pin5)  (pin11) GND          |
> + * |   PWR_PMOD1  (pin6)  (pin12) GND          |
> + * +--------------------------------------------+

This depends not only on CONFIG_SW3 (for RXD only), but also on
SW_OPT_MUX4 (SW_SER0_PMOD=L gates all 4 SCIF1 signals).
While including "rzg3s-smarc-switches.h" for (out-of-tree) overlay
configfs is not really needed, please document the switches in the
comments.  As this is included in r9a08g045s33-smarc-pmod.dtb, you
may still want to include "rzg3s-smarc-switches.h".

> + *
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +
> +#include <dt-bindings/pinctrl/rzg2l-pinctrl.h>
> +
> +&{/} {
> +       aliases {
> +               serial0 = "/soc/serial@1004bc00";

Note that configuring aliases doesn't work in dynamic overlays
(but we don't care in upstream).
However, this is also wired on the Carrier board to the M2 slot when
SW_SER0_PMOD is low. so I think it makes sense to have the alias
unconditionally in rzg3s-smarc.dtsi instead.

> +       };

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 v3 8/8] arm64: dts: renesas: r9a08g045s33-smarc-pmod: Add overlay for SCIF1

[Claudiu Beznea]

Hi, Geert,

On 09.12.2024 12:44, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Fri, Nov 15, 2024 at 2:50 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Add DT overlay for SCIF1 (of the Renesas RZ/G3S SoC) routed through the
>> PMOD1_3A interface available on the Renesas RZ SMARC Carrier II board.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Thanks for your patch!
> 
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/renesas/r9a08g045s33-smarc-pmod.dtso
>> @@ -0,0 +1,48 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Device Tree Source for the RZ/G3S SMARC Carrier II EVK PMOD parts
> 
> So you plan to describe all 3 PMOD interfaces in a single overlay?
> The board has:
>   1. PMOD0 - Type-2A,
>   2. PMOD1 - Tpye-3A,
>   3. PMOD1 - Type 6A.
> Wouldn't it be more convenient to have separate overlays for each port,
> so you can more easily use them as e.g. Type-1(A) (GPIO only)?

That would be better, indeed.

> 
> BTW, naming both the second and third port "PMOD1" in the schematics,
> and differentiating them by their type, was definitely a bad idea.
> How can you distinguish between Type-1(A) on the second or third port?
> 
>> + *
>> + * Copyright (C) 2024 Renesas Electronics Corp.
>> + *
>> + *
>> + * [Connection]
>> + *
>> + * SMARC Carrier II EVK
>> + * +--------------------------------------------+
>> + * |PMOD1_3A (PMOD1 PIN HEADER)                        |
>> + * |   SCIF1_CTS# (pin1)  (pin7)  PMOD1_GPIO10 |
>> + * |   SCIF1_TXD  (pin2)  (pin8)  PMOD1_GPIO11 |
>> + * |   SCIF1_RXD  (pin3)  (pin9)  PMOD1_GPIO12 |
>> + * |   SCIF1_RTS# (pin4)  (pin10) PMOD1_GPIO13 |
>> + * |   GND        (pin5)  (pin11) GND          |
>> + * |   PWR_PMOD1  (pin6)  (pin12) GND          |
>> + * +--------------------------------------------+
> 
> This depends not only on CONFIG_SW3 (for RXD only), but also on
> SW_OPT_MUX4 (SW_SER0_PMOD=L gates all 4 SCIF1 signals).

You're right!

> While including "rzg3s-smarc-switches.h" for (out-of-tree) overlay
> configfs is not really needed, please document the switches in the
> comments.  

Sure!

> As this is included in r9a08g045s33-smarc-pmod.dtb, you
> may still want to include "rzg3s-smarc-switches.h".

OK, I'll handle it in the next version.


> 
>> + *
>> + */
>> +
>> +/dts-v1/;
>> +/plugin/;
>> +
>> +#include <dt-bindings/pinctrl/rzg2l-pinctrl.h>
>> +
>> +&{/} {
>> +       aliases {
>> +               serial0 = "/soc/serial@1004bc00";
> 
> Note that configuring aliases doesn't work in dynamic overlays
> (but we don't care in upstream).
> However, this is also wired on the Carrier board to the M2 slot when
> SW_SER0_PMOD is low. so I think it makes sense to have the alias
> unconditionally in rzg3s-smarc.dtsi instead.

OK, I'll move it there.

Thank you for your review,
Claudiu

> 
>> +       };
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>