linux-kernel - Re: [PATCH v9 07/10] serial: sh-sci: Add support for RZ/T2H SCI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAMuHMdWhq+o18hgBO7Kg_Rsq47WsEwV7-DWYcyJCM0h1wsMshg@mail.gmail.com>
Date: Fri, 23 May 2025 11:57:37 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Thierry Bultel <thierry.bultel.yh@...renesas.com>
Cc: thierry.bultel@...atsea.fr, linux-renesas-soc@...r.kernel.org, 
	paul.barker.ct@...renesas.com, 
	Wolfram Sang <wsa+renesas@...g-engineering.com>, linux-kernel@...r.kernel.org, 
	linux-serial@...r.kernel.org
Subject: Re: [PATCH v9 07/10] serial: sh-sci: Add support for RZ/T2H SCI

Hi Thierry,

On Thu, 15 May 2025 at 16:19, Thierry Bultel
<thierry.bultel.yh@...renesas.com> wrote:
> Define a new RSCI port type, and the RSCI 32 bits registers set.
> The RZ/T2H SCI has a a fifo, and a quite different set of registers
> from the original SH SCI ones.
> DMA is not supported yet.
>
> Reviewed-by: Wolfram Sang <wsa+renesas@...g-engineering.com>
> Signed-off-by: Thierry Bultel <thierry.bultel.yh@...renesas.com>
> ---
> Changes v8->v9:
>   - Fixed some code formatting
>   - Renamed rzt2_sci_uart_ops to rsci_uart_ops
>   - Renamed of_sci_r9a09g077_data to of_sci_rsci_data
>   - Added EXPORT_SYMBOL for public functions
>   - Added MODULE_LICENSE & MODULE_DESCRIPTION
>   - Fixed RSCI clock names
>   - Fixed SCI_PORT_RSCI using BIT(7)

Thanks for the update!

> --- /dev/null
> +++ b/drivers/tty/serial/rsci.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __RSCI_H__
> +#define __RSCI_H__
> +
> +#include "sh-sci-common.h"
> +
> +#ifdef CONFIG_SERIAL_RSCI
> +extern struct sci_of_data of_sci_rsci_data;
> +#endif

The #ifdef isn't really needed.

> +
> +#endif /* __RSCI_H__ */

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c

> @@ -2977,14 +2987,27 @@ static int sci_init_clocks(struct sci_port *sci_port, struct device *dev)
>         struct clk *clk;
>         unsigned int i;
>
> -       if (sci_port->type == PORT_HSCIF)
> +       if (sci_port->type == PORT_HSCIF) {
>                 clk_names[SCI_SCK] = "hsck";
> +       } else if (sci_port->type == SCI_PORT_RSCI) {
> +               clk_names[SCI_FCK] = "operation";
> +               clk_names[SCI_BRG_INT] = "bus";
> +       }
>
>         for (i = 0; i < SCI_NUM_CLKS; i++) {
> -               clk = devm_clk_get_optional(dev, clk_names[i]);
> +               const char *name = clk_names[i];
> +
> +               clk = devm_clk_get_optional(dev, name);
>                 if (IS_ERR(clk))
>                         return PTR_ERR(clk);
>
> +               if (!clk && sci_port->type == SCI_PORT_RSCI &&
> +                   (i == SCI_FCK || i == SCI_BRG_INT)) {
> +                       return dev_err_probe(dev, -ENODEV,
> +                                            "failed to get '%s' clock\n",

I would make the error message identical to the other cases below,
so the format string can be shared by the compiler.

> +                                            name);
> +               }
> +
>                 if (!clk && i == SCI_FCK) {
>                         /*
>                          * Not all SH platforms declare a clock lookup entry
> @@ -2995,13 +3018,13 @@ static int sci_init_clocks(struct sci_port *sci_port, struct device *dev)
>                         if (IS_ERR(clk))
>                                 return dev_err_probe(dev, PTR_ERR(clk),
>                                                      "failed to get %s\n",
> -                                                    clk_names[i]);
> +                                                    name);
>                 }
>
>                 if (!clk)
> -                       dev_dbg(dev, "failed to get %s\n", clk_names[i]);
> +                       dev_dbg(dev, "failed to get %s\n", name);
>                 else
> -                       dev_dbg(dev, "clk %s is %pC rate %lu\n", clk_names[i],
> +                       dev_dbg(dev, "clk %s is %pC rate %lu\n", name,
>                                 clk, clk_get_rate(clk));
>                 sci_port->clks[i] = clk;
>         }

The rest of the (generic; I didn't look at the RSCI low-level details)
changes LGTM.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-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