Re: [PATCH v3 4/4] serial: sh-sci: Add support for RZ/V2H(P) SoC

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Tue, Mar 19, 2024 at 8:21 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Mon, Mar 18, 2024 at 6:22 PM Prabhakar <prabhakar.csengg@gmailcom> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > Add serial support for RZ/V2H(P) SoC with earlycon.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > v2 - > v3
> > - new patch
>
> Thanks for your patch!
>
> > --- a/drivers/tty/serial/sh-sci.c
> > +++ b/drivers/tty/serial/sh-sci.c
> > @@ -290,7 +290,7 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = {
> >         },
> >
> >         /*
> > -        * The "SCIFA" that is in RZ/A2, RZ/G2L and RZ/T.
> > +        * The "SCIFA" that is in RZ/A2, RZ/G2L, RZ/T and RZ/V2H.
> >          * It looks like a normal SCIF with FIFO data, but with a
> >          * compressed address space. Also, the break out of interrupts
> >          * are different: ERI/BRI, RXI, TXI, TEI, DRI.
>
> and RZ/V2H has more interrupts than RZ/A1, RZ/G2L and RZ/T...
>
> In addition, RZ/V2H does not support synchronous mode (does not matter
> for the driver) and modem control signals.
>
> Currently, sci_init_pins() does write ones to the SCPTR bits that are
> reserved and marked as "write zero" on RZ/V2H. I am not sure how bad
> that is.  You could avoid that by adding a check for .hasrtscts, but
> that may have impact on other SoCs/boards, where currently e.g. RTS#
> is always programmed for output and active low...
>
Oops I had totally missed this difference, thanks for catching that.

> So if you really need to avoid writing to these bits, the only safe
> way may be to add a new SCIF type.  But perhaps I'm over-cautious? ;-)
>
As we are adding a SoC specific compat string we can update this if we
see an issue, but I'm happy to do that change now too. Please let me
know what would you prefer.

Cheers,
Prabhakar
> > @@ -3224,6 +3224,10 @@ static const struct of_device_id of_sci_match[] __maybe_unused = {
> >                 .compatible = "renesas,scif-r9a07g044",
> >                 .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE),
> >         },
> > +       {
> > +               .compatible = "renesas,scif-r9a09g057",
> > +               .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE),
> > +       },
> >         /* Family-specific types */
> >         {
> >                 .compatible = "renesas,rcar-gen1-scif",
> > @@ -3554,6 +3558,7 @@ OF_EARLYCON_DECLARE(sci, "renesas,sci", sci_early_console_setup);
> >  OF_EARLYCON_DECLARE(scif, "renesas,scif", scif_early_console_setup);
> >  OF_EARLYCON_DECLARE(scif, "renesas,scif-r7s9210", rzscifa_early_console_setup);
> >  OF_EARLYCON_DECLARE(scif, "renesas,scif-r9a07g044", rzscifa_early_console_setup);
> > +OF_EARLYCON_DECLARE(scif, "renesas,scif-r9a09g057", rzscifa_early_console_setup);
> >  OF_EARLYCON_DECLARE(scifa, "renesas,scifa", scifa_early_console_setup);
> >  OF_EARLYCON_DECLARE(scifb, "renesas,scifb", scifb_early_console_setup);
> >  OF_EARLYCON_DECLARE(hscif, "renesas,hscif", hscif_early_console_setup);
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
>
> 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