Message-ID: <CAMuHMdVptwisoHJq9683r92XS-sgO8Uk52zxnEQUn6DTd3DeEw@mail.gmail.com>
Date: Wed, 19 Feb 2025 14:53:45 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Thierry Bultel <thierry.bultel.yh@...renesas.com>
Cc: thierry.bultel@...atsea.fr, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jiri Slaby <jirislaby@...nel.org>, 
	linux-renesas-soc@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-serial@...r.kernel.org
Subject: Re: [PATCH v2 09/13] serial: sh-sci: Introduced sci_of_data

Hi Thierry,

On Mon, 17 Feb 2025 at 12:04, Thierry Bultel
<thierry.bultel.yh@...renesas.com> wrote:
> The aim here is to provide an easier support to more different SCI
> controllers, like the RZ/T2H one.
>
> The existing .data field of_sci_match is changed to a structure containing
> all what that can be statically initialized, and avoid a call to
> 'sci_probe_regmap', in both 'sci_init_single', and 'early_console_setup'.
>
> 'sci_probe_regmap' is now assumed to be called in the only case where the
> device description is from a board file instead of a dts.
>
> In this way, there is no need to patch 'sci_probe_regmap' for adding new
> SCI type, and also, the specific sci_port_params for a new SCI type can be
> provided by an external file.
>
> Signed-off-by: Thierry Bultel <thierry.bultel.yh@...renesas.com>

Thanks for your patch!

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -3009,7 +3010,6 @@ static int sci_init_single(struct platform_device *dev,
>                 for (i = 1; i < ARRAY_SIZE(sci_port->irqs); i++)
>                         sci_port->irqs[i] = sci_port->irqs[0];
>
> -       sci_port->params = sci_probe_regmap(p);
>         if (unlikely(sci_port->params == NULL))
>                 return -EINVAL;

Ideally, this check can be removed here... (see below)

>
> @@ -3264,9 +3264,14 @@ static void sci_remove(struct platform_device *dev)
>                 device_remove_file(&dev->dev, &dev_attr_rx_fifo_timeout);
>  }
>
> -#define SCI_OF_DATA(type, regtype)     (void *)((type) << 16 | (regtype))
> -#define SCI_OF_TYPE(data)              ((unsigned long)(data) >> 16)
> -#define SCI_OF_REGTYPE(data)           ((unsigned long)(data) & 0xffff)
> +#define SCI_OF_DATA(_type, _regtype) (\
> +&(struct sci_of_data) {\
> +       .type = (_type), \
> +       .regtype = (_regtype),\
> +       .ops = &sci_port_ops,\
> +       .uart_ops = &sci_uart_ops,\
> +       .params = &sci_port_params[_regtype],\
> +})

Doing it this way means each and every entry in of_sci_match[] has its
own copy of struct sci_of_data, even if it is identical to one of the
others. Unfortunately s/struct sci_of_data/const struct sci_of_data/
doesn't help, so I'm afraid you have to deduplicate them explicitly.

>
>  static const struct of_device_id of_sci_match[] __maybe_unused = {
>         /* SoC-specific types */
> @@ -3334,7 +3339,7 @@ static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
>         struct reset_control *rstc;
>         struct plat_sci_port *p;
>         struct sci_port *sp;
> -       const void *data;
> +       const struct sci_of_data *data;
>         int id, ret;
>
>         if (!IS_ENABLED(CONFIG_OF) || !np)
> @@ -3380,8 +3385,12 @@ static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
>         sp = &sci_ports[id];
>         *dev_id = id;
>
> -       p->type = SCI_OF_TYPE(data);
> -       p->regtype = SCI_OF_REGTYPE(data);
> +       p->type = data->type;
> +       p->regtype = data->regtype;
> +
> +       sp->ops = data->ops;
> +       sp->port.ops = data->uart_ops;
> +       sp->params = data->params;
>
>         sp->has_rtscts = of_property_read_bool(np, "uart-has-rtscts");
>
> @@ -3471,6 +3480,7 @@ static int sci_probe(struct platform_device *dev)
>                 }
>
>                 dev_id = dev->id;
> +               sp->params = sci_probe_regmap(p, &sci_ports[dev_id]);

sp is still uninitialized here, so it crashes on SuperH.

In adition

if (!sp->params)
        return -ENODEV;

>         }
>
>         sp = &sci_ports[dev_id];
> @@ -3560,19 +3570,23 @@ sh_early_platform_init_buffer("earlyprintk", &sci_driver,
>  static struct plat_sci_port port_cfg __initdata;
>
>  int __init early_console_setup(struct earlycon_device *device,
> -                                     int type)
> +                              const struct sci_of_data *data)
>  {
>         const struct sci_common_regs *regs;
>
>         if (!device->port.membase)
>                 return -ENODEV;
>
> -       device->port.type = type;
> +       device->port.type = data->type;
>         memcpy(&sci_ports[0].port, &device->port, sizeof(struct uart_port));
> -       port_cfg.type = type;
> +
> +       port_cfg.type = data->type;
> +       port_cfg.regtype = data->regtype;
> +
>         sci_ports[0].cfg = &port_cfg;
> -       sci_ports[0].ops = &sci_port_ops;
> -       sci_ports[0].params = sci_probe_regmap(&port_cfg);
> +       sci_ports[0].params = data->params;
> +       sci_ports[0].ops = data->ops;
> +       sci_ports[0].port.ops = data->uart_ops;
>         regs = sci_ports[0].params->common_regs;
>
>         port_cfg.scscr = sci_ports[0].ops->read_reg(&sci_ports[0].port, regs->control);

I think you have to do some extra setup in sci_probe_earlyprintk(), too.
That function contains the second caller of sci_init_single(), and
thus relied on sci_init_single() calling sci_probe_regmap() before.
I haven't tested that case on actual hardware yet.

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

Powered by blists - more mailing lists