[PATCH 0/3] serial: sh-sci: Clock handling improvements

[PATCH 0/3] serial: sh-sci: Clock handling improvements

[Geert Uytterhoeven]

	Hi Greg, Jiri,

This patch series contains a legacy cleanup and two small improvements
for the Renesas (H)SCI(F) serial driver.

This been tested on a various of Renesas (SuperH and ARM) SoCs.

Thanks!

Geert Uytterhoeven (3):
  serial: sh-sci: Drop support for "sci_ick" clock
  serial: sh-sci: Use dev_err_probe()
  serial: sh-sci: Use devm_clk_get_optional()

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

-- 
2.25.1

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 1/3] serial: sh-sci: Drop support for "sci_ick" clock

[Geert Uytterhoeven]

Since commit 1b463bd51042927e ("ARM: dts: r8a7794: Rename the serial
port clock to fck") in v4.6, all upstream DTS files call the SCIF
functional clock "fck".

Hence the time is ripe to drop backward-compatibility with old DTBs that
use the old "sci_ick" name.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Note that such old DTBs have stopped working anyway since commit
58256143cff7c2e0 ("clk: renesas: Remove R-Car Gen2 legacy DT clock
support") in v5.5.
---
 drivers/tty/serial/sh-sci.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 88005d2fc2a00b0b..686ca1777222b1d4 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -2784,17 +2784,6 @@ static int sci_init_clocks(struct sci_port *sci_port, struct device *dev)
 			return -EPROBE_DEFER;
 
 		if (IS_ERR(clk) && i == SCI_FCK) {
-			/*
-			 * "fck" used to be called "sci_ick", and we need to
-			 * maintain DT backward compatibility.
-			 */
-			clk = devm_clk_get(dev, "sci_ick");
-			if (PTR_ERR(clk) == -EPROBE_DEFER)
-				return -EPROBE_DEFER;
-
-			if (!IS_ERR(clk))
-				goto found;
-
 			/*
 			 * Not all SH platforms declare a clock lookup entry
 			 * for SCI devices, in which case we need to get the
-- 
2.25.1

Re: [PATCH 1/3] serial: sh-sci: Drop support for "sci_ick" clock

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Thu, Dec 16, 2021 at 03:17:32PM +0100, Geert Uytterhoeven wrote:
> Since commit 1b463bd51042927e ("ARM: dts: r8a7794: Rename the serial
> port clock to fck") in v4.6, all upstream DTS files call the SCIF
> functional clock "fck".
> 
> Hence the time is ripe to drop backward-compatibility with old DTBs that
> use the old "sci_ick" name.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> Note that such old DTBs have stopped working anyway since commit
> 58256143cff7c2e0 ("clk: renesas: Remove R-Car Gen2 legacy DT clock
> support") in v5.5.
> ---
>  drivers/tty/serial/sh-sci.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 88005d2fc2a00b0b..686ca1777222b1d4 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -2784,17 +2784,6 @@ static int sci_init_clocks(struct sci_port *sci_port, struct device *dev)
>  			return -EPROBE_DEFER;
>  
>  		if (IS_ERR(clk) && i == SCI_FCK) {
> -			/*
> -			 * "fck" used to be called "sci_ick", and we need to
> -			 * maintain DT backward compatibility.
> -			 */
> -			clk = devm_clk_get(dev, "sci_ick");
> -			if (PTR_ERR(clk) == -EPROBE_DEFER)
> -				return -EPROBE_DEFER;
> -
> -			if (!IS_ERR(clk))
> -				goto found;
> -
>  			/*
>  			 * Not all SH platforms declare a clock lookup entry
>  			 * for SCI devices, in which case we need to get the

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] serial: sh-sci: Drop support for "sci_ick" clock

[Wolfram Sang]

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

On Thu, Dec 16, 2021 at 03:17:32PM +0100, Geert Uytterhoeven wrote:
> Since commit 1b463bd51042927e ("ARM: dts: r8a7794: Rename the serial
> port clock to fck") in v4.6, all upstream DTS files call the SCIF
> functional clock "fck".
> 
> Hence the time is ripe to drop backward-compatibility with old DTBs that
> use the old "sci_ick" name.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Note that such old DTBs have stopped working anyway since commit
> 58256143cff7c2e0 ("clk: renesas: Remove R-Car Gen2 legacy DT clock
> support") in v5.5.

Ah, with that paragraph in mind:

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


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

[PATCH 2/3] serial: sh-sci: Use dev_err_probe()

[Geert Uytterhoeven]

Use the dev_err_probe() helper to streamline error handling.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/sh-sci.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 686ca1777222b1d4..5f6d85b8e3dd4173 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -2790,15 +2790,12 @@ static int sci_init_clocks(struct sci_port *sci_port, struct device *dev)
 			 * global "peripheral_clk" clock.
 			 */
 			clk = devm_clk_get(dev, "peripheral_clk");
-			if (!IS_ERR(clk))
-				goto found;
-
-			dev_err(dev, "failed to get %s (%ld)\n", clk_names[i],
-				PTR_ERR(clk));
-			return PTR_ERR(clk);
+			if (IS_ERR(clk))
+				return dev_err_probe(dev, PTR_ERR(clk),
+						     "failed to get %s\n",
+						     clk_names[i]);
 		}
 
-found:
 		if (IS_ERR(clk))
 			dev_dbg(dev, "failed to get %s (%ld)\n", clk_names[i],
 				PTR_ERR(clk));
-- 
2.25.1

Re: [PATCH 2/3] serial: sh-sci: Use dev_err_probe()

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Thu, Dec 16, 2021 at 03:17:33PM +0100, Geert Uytterhoeven wrote:
> Use the dev_err_probe() helper to streamline error handling.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/tty/serial/sh-sci.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 686ca1777222b1d4..5f6d85b8e3dd4173 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -2790,15 +2790,12 @@ static int sci_init_clocks(struct sci_port *sci_port, struct device *dev)
>  			 * global "peripheral_clk" clock.
>  			 */
>  			clk = devm_clk_get(dev, "peripheral_clk");
> -			if (!IS_ERR(clk))
> -				goto found;
> -
> -			dev_err(dev, "failed to get %s (%ld)\n", clk_names[i],
> -				PTR_ERR(clk));
> -			return PTR_ERR(clk);
> +			if (IS_ERR(clk))
> +				return dev_err_probe(dev, PTR_ERR(clk),
> +						     "failed to get %s\n",
> +						     clk_names[i]);
>  		}
>  
> -found:
>  		if (IS_ERR(clk))
>  			dev_dbg(dev, "failed to get %s (%ld)\n", clk_names[i],
>  				PTR_ERR(clk));

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] serial: sh-sci: Use dev_err_probe()

[Wolfram Sang]

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

On Thu, Dec 16, 2021 at 03:17:33PM +0100, Geert Uytterhoeven wrote:
> Use the dev_err_probe() helper to streamline error handling.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

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


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

[PATCH 3/3] serial: sh-sci: Use devm_clk_get_optional()

[Geert Uytterhoeven]

The sh-sci driver supports up to four input clocks, of which only the
first one is mandatory.

Replace devm_clk_get() and custom error checking by
devm_clk_get_optional(), to simplify the code and to catch all real
errors.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/sh-sci.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 5f6d85b8e3dd4173..bb3adf0a109324ca 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -2779,11 +2779,11 @@ static int sci_init_clocks(struct sci_port *sci_port, struct device *dev)
 		clk_names[SCI_SCK] = "hsck";
 
 	for (i = 0; i < SCI_NUM_CLKS; i++) {
-		clk = devm_clk_get(dev, clk_names[i]);
-		if (PTR_ERR(clk) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
+		clk = devm_clk_get_optional(dev, clk_names[i]);
+		if (IS_ERR(clk))
+			return PTR_ERR(clk);
 
-		if (IS_ERR(clk) && i == SCI_FCK) {
+		if (!clk && i == SCI_FCK) {
 			/*
 			 * Not all SH platforms declare a clock lookup entry
 			 * for SCI devices, in which case we need to get the
@@ -2796,13 +2796,12 @@ static int sci_init_clocks(struct sci_port *sci_port, struct device *dev)
 						     clk_names[i]);
 		}
 
-		if (IS_ERR(clk))
-			dev_dbg(dev, "failed to get %s (%ld)\n", clk_names[i],
-				PTR_ERR(clk));
+		if (!clk)
+			dev_dbg(dev, "failed to get %s\n", clk_names[i]);
 		else
 			dev_dbg(dev, "clk %s is %pC rate %lu\n", clk_names[i],
 				clk, clk_get_rate(clk));
-		sci_port->clks[i] = IS_ERR(clk) ? NULL : clk;
+		sci_port->clks[i] = clk;
 	}
 	return 0;
 }
-- 
2.25.1

Re: [PATCH 3/3] serial: sh-sci: Use devm_clk_get_optional()

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Thu, Dec 16, 2021 at 03:17:34PM +0100, Geert Uytterhoeven wrote:
> The sh-sci driver supports up to four input clocks, of which only the
> first one is mandatory.
> 
> Replace devm_clk_get() and custom error checking by
> devm_clk_get_optional(), to simplify the code and to catch all real
> errors.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/tty/serial/sh-sci.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 5f6d85b8e3dd4173..bb3adf0a109324ca 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -2779,11 +2779,11 @@ static int sci_init_clocks(struct sci_port *sci_port, struct device *dev)
>  		clk_names[SCI_SCK] = "hsck";
>  
>  	for (i = 0; i < SCI_NUM_CLKS; i++) {
> -		clk = devm_clk_get(dev, clk_names[i]);
> -		if (PTR_ERR(clk) == -EPROBE_DEFER)
> -			return -EPROBE_DEFER;
> +		clk = devm_clk_get_optional(dev, clk_names[i]);
> +		if (IS_ERR(clk))
> +			return PTR_ERR(clk);
>  
> -		if (IS_ERR(clk) && i == SCI_FCK) {
> +		if (!clk && i == SCI_FCK) {
>  			/*
>  			 * Not all SH platforms declare a clock lookup entry
>  			 * for SCI devices, in which case we need to get the
> @@ -2796,13 +2796,12 @@ static int sci_init_clocks(struct sci_port *sci_port, struct device *dev)
>  						     clk_names[i]);
>  		}
>  
> -		if (IS_ERR(clk))
> -			dev_dbg(dev, "failed to get %s (%ld)\n", clk_names[i],
> -				PTR_ERR(clk));
> +		if (!clk)
> +			dev_dbg(dev, "failed to get %s\n", clk_names[i]);
>  		else
>  			dev_dbg(dev, "clk %s is %pC rate %lu\n", clk_names[i],
>  				clk, clk_get_rate(clk));
> -		sci_port->clks[i] = IS_ERR(clk) ? NULL : clk;
> +		sci_port->clks[i] = clk;
>  	}
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] serial: sh-sci: Use devm_clk_get_optional()

[Wolfram Sang]

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

On Thu, Dec 16, 2021 at 03:17:34PM +0100, Geert Uytterhoeven wrote:
> The sh-sci driver supports up to four input clocks, of which only the
> first one is mandatory.
> 
> Replace devm_clk_get() and custom error checking by
> devm_clk_get_optional(), to simplify the code and to catch all real
> errors.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Yeah, much better. Great cleanup!

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


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

Re: [PATCH 0/3] serial: sh-sci: Clock handling improvements

[Rob Landley]

On 12/16/21 8:17 AM, Geert Uytterhoeven wrote:
> 	Hi Greg, Jiri,
> 
> This patch series contains a legacy cleanup and two small improvements
> for the Renesas (H)SCI(F) serial driver.

Tested-by: Rob Landley <rob@landley.net>

By the way, did you ever figure out how to get the first serial port to work on
qemu so qemu-system-sh4 doesn't have to "-serial null -serial mon:stdio" to get
a serial console?

Rob

Re: [PATCH 0/3] serial: sh-sci: Clock handling improvements

[Geert Uytterhoeven]

Hi Rob,

On Sun, Dec 19, 2021 at 11:34 AM Rob Landley <rob@landley.net> wrote:
> On 12/16/21 8:17 AM, Geert Uytterhoeven wrote:
> >       Hi Greg, Jiri,
> >
> > This patch series contains a legacy cleanup and two small improvements
> > for the Renesas (H)SCI(F) serial driver.
>
> Tested-by: Rob Landley <rob@landley.net>

Thanks!

> By the way, did you ever figure out how to get the first serial port to work on
> qemu so qemu-system-sh4 doesn't have to "-serial null -serial mon:stdio" to get
> a serial console?

Nope, same as last time you asked ;-)

However, upon a fresh look at linux/arch/sh/boards/mach-r2d/setup.c,
I noticed something I missed before: the SCI port is not available
as a UART, as it is used to talk SPI to an RTC.  So that means qemu
has the hardware description wrong?

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 0/3] serial: sh-sci: Clock handling improvements

[Rob Landley]

On 12/19/21 4:53 AM, Geert Uytterhoeven wrote:
>> By the way, did you ever figure out how to get the first serial port to work on
>> qemu so qemu-system-sh4 doesn't have to "-serial null -serial mon:stdio" to get
>> a serial console?
> 
> Nope, same as last time you asked ;-)

Well it had been a couple years, you never know...

> However, upon a fresh look at linux/arch/sh/boards/mach-r2d/setup.c,
> I noticed something I missed before: the SCI port is not available
> as a UART, as it is used to talk SPI to an RTC.

/* Single Epson RTC-9701JE attached on CS0 */

Huh. That does explain the CONFIG_RTC_DRV_R9701=y in rts7751r2d*_defconfig.
(And also SPI SPI_SH_SCI MFD_SM501 RTC_CLASS and RTC_DRV_SH RTC_HCTOSYS which
I've never gotten connected to QEMU.)

> So that means qemu
> has the hardware description wrong?

It would be nice if qemu-system-sh4 grew an RTC. Looks like Sato-san and Magnus
Damm are the qemu-system-sh4 maintainers? (cc'd)

Thanks,

Rob

Re: [PATCH 0/3] serial: sh-sci: Clock handling improvements

[Yoshinori Sato]

On Mon, 20 Dec 2021 19:29:24 +0900,
Rob Landley wrote:
> 
> On 12/19/21 4:53 AM, Geert Uytterhoeven wrote:
> >> By the way, did you ever figure out how to get the first serial port to work on
> >> qemu so qemu-system-sh4 doesn't have to "-serial null -serial mon:stdio" to get
> >> a serial console?
> > 
> > Nope, same as last time you asked ;-)
> 
> Well it had been a couple years, you never know...
> 
> > However, upon a fresh look at linux/arch/sh/boards/mach-r2d/setup.c,
> > I noticed something I missed before: the SCI port is not available
> > as a UART, as it is used to talk SPI to an RTC.

Yes.
R2D plus serial port connected SuperI/O UART. It not cpu on-chip SCI.
It is not possible to check SCI with the standard version of qemu-system-sh4.
There is SCI emulation, so you can change it to connect to the qemu serial port.

> /* Single Epson RTC-9701JE attached on CS0 */
> 
> Huh. That does explain the CONFIG_RTC_DRV_R9701=y in rts7751r2d*_defconfig.
> (And also SPI SPI_SH_SCI MFD_SM501 RTC_CLASS and RTC_DRV_SH RTC_HCTOSYS which
> I've never gotten connected to QEMU.)

SPI_SH_SCI using SCI pin like GPIO.
qemu-system-sh4 is not support this.

> > So that means qemu
> > has the hardware description wrong?
> 
> It would be nice if qemu-system-sh4 grew an RTC. Looks like Sato-san and Magnus
> Damm are the qemu-system-sh4 maintainers? (cc'd)
> 
> Thanks,
> 
> Rob

-- 
Yosinori Sato