Re: [PATCH 3/3] serial: sh-sci: Add reset support for RZ/G2L SoC

Hi Geert,

On Tue, Nov 9, 2021 at 7:50 AM Geert Uytterhoeven [off-list ref] wrote:

Hi Prabhakar,

On Tue, Nov 9, 2021 at 1:34 AM Lad, Prabhakar
[off-list ref] wrote:

quoted

On Mon, Nov 8, 2021 at 4:40 PM Geert Uytterhoeven [off-list ref] wrote:

quoted

On Wed, Nov 3, 2021 at 6:31 PM Lad Prabhakar
[off-list ref] wrote:

quoted

On RZ/G2L devices should be explicitly pulled out of reset for it
to work. This patch adds support to read the "resets" property and
performs deassert/assert when required.

Also, propagate the error to the caller of sci_parse_dt() instead of
returning NULL in case of failure.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

quoted

---
Hi Geert,
For handling the resets I was in dual mind whether to perform
reset based on compatible strings or soc-id, let me know your
thoughts. Currently no SoC's use "renesas,sci" so using the same
for performing the reset operation for SCI.

We do, on H8/300.

Missed that.

quoted

quoted

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

@@ -3203,23 +3204,58 @@ static const struct of_device_id of_sci_match[] = {
 };
 MODULE_DEVICE_TABLE(of, of_sci_match);

+static void sci_reset_control_assert(void *data)
+{
+       reset_control_assert(data);
+}
+
 static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
                                          unsigned int *dev_id)
 {
        struct device_node *np = pdev->dev.of_node;
+       const struct of_device_id *of_id;
        struct plat_sci_port *p;
        struct sci_port *sp;
        const void *data;
        int id;

        if (!IS_ENABLED(CONFIG_OF) || !np)
-               return NULL;
+               return ERR_PTR(-EINVAL);
+
+       of_id = of_match_device(of_sci_match, &pdev->dev);
+       if (!of_id)
+               return ERR_PTR(-EINVAL);

-       data = of_device_get_match_data(&pdev->dev);
+       if (!strcmp(of_id->compatible, "renesas,scif-r9a07g044") ||
+           !strcmp(of_id->compatible, "renesas,sci")) {

This will match on H8/300, too, which doesn't have resets.
Please match against "renesas,sci-r9a07g044" instead.

Please don't use explicit strcmp() calls here, but add a flag to
of_sci_match[].data.

Agreed, does the below hunk look good for handling the reset?

-#define SCI_OF_DATA(type, regtype)     (void *)((type) << 16 | (regtype))
+#define SCIx_RESET                             BIT(31)
+#define SCI_OF_DATA(type, regtype, reset)      (void *)(reset |
(type) << 16 | (regtype))
 #define SCI_OF_TYPE(data)              ((unsigned long)(data) >> 16)
 #define SCI_OF_REGTYPE(data)           ((unsigned long)(data) & 0xffff)

@@ -3169,7 +3170,11 @@ static const struct of_device_id of_sci_match[] = {
        },
        {
                .compatible = "renesas,scif-r9a07g044",
-               .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE),
+               .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE,

SCIx_RESET),
+       },
+       {
+               .compatible = "renesas,sci-r9a07g044",
+               .data = SCI_OF_DATA(PORT_SCI, SCIx_SCI_REGTYPE, SCIx_RESET),
        },

That's what I had in mind.

But upon second thought, it might be better to just drop the check,
and obtain an optional reset instead?

Agreed, will do that.

After all the reset requirement is not a feature of this specific
SCI(F) variant, but an SoC integration feature.  And deasserting
the reset on other SoCs that have a reset should be fine.

Indeed.

Cheers,
Prabhakar

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