Hi Biju,
On Tue, Mar 28, 2023 at 5:36 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > Subject: Re: [PATCH v2 3/3] tty: serial: sh-sci: Add RZ/G2L SCIF DMA rx
> > support
> > On Fri, Mar 24, 2023 at 11:02 AM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > wrote:
> > > Add SCIF DMA rx support for RZ/G2L alike SoCs.
> > >
> > > RZ/G2L alike SoCs use the same signal for both interrupt and DMA
> > > transfer requests, so we must disable line interrupt while transferring
> > DMA.
> > >
> > > We must set FIFO trigger to 1 so that SCIF will request DMA when there
> > > is a character as SCIF DMA request is based on FIFO data full RDF.
> > >
> > > Based on a patch in the BSP by Long Luu <long.luu.ur@xxxxxxxxxxx>
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/tty/serial/sh-sci.c
> > > +++ b/drivers/tty/serial/sh-sci.c
> > > @@ -1312,9 +1312,13 @@ static void sci_dma_rx_reenable_irq(struct
> > > sci_port *s)
> > >
> > > /* Direct new serial port interrupts back to CPU */
> > > scr = serial_port_in(port, SCSCR);
> > > - if (port->type == PORT_SCIFA || port->type == PORT_SCIFB) {
> > > - scr &= ~SCSCR_RDRQE;
> > > + if (port->type == PORT_SCIFA || port->type == PORT_SCIFB ||
> > > + is_rz_scif_port(port, s)) {
> > > enable_irq(s->irqs[SCIx_RXI_IRQ]);
> > > + if (port->type == PORT_SCIF)
> > > + scif_set_rtrg(port, s->rx_trigger);
> > > + else
> > > + scr &= ~SCSCR_RDRQE;
> > > }
> > > serial_port_out(port, SCSCR, scr | SCSCR_RIE); } @@ -1735,7
> > > +1739,15 @@ static irqreturn_t sci_rx_interrupt(int irq, void *ptr)
> > > if (sci_dma_rx_submit(s, false) < 0)
> > > goto handle_pio;
> > >
> > > - scr &= ~SCSCR_RIE;
> > > + if (is_rz_scif_port(port, s)) {
> > > + /* Switch irq from SCIF to DMA */
> > > +
> > > + disable_irq_nosync(s->irqs[SCIx_RXI_IRQ]);
> >
> > Perhaps this can be combined with the disable_irq_nosync() above?
>
> OK, will do, As per Table 22.19 SCIFA Interrupt Sources, DMAC activation
> Only possible with RXI interrupt.
>
> So we need to use "s->irqs[SCIx_RXI_IRQ]" instead of "irq" for RZ/G2L SCIFA.
>
>
> >
> > > + scif_set_rtrg(port, 1);
> > > + /* DMA need RIE enable */
> > > + scr |= SCSCR_RIE;
> > > + } else {
> > > + scr &= ~SCSCR_RIE;
> > > + }
> >
> > ... and this with some other SCIFA code path?
>
> OK.
>
> >
> > Looks like RZ/A2 and RZ/G2L "SCIFA" does have some resemblance with SCIFA,
> > contrary to earlier statements?
> > Perhaps the code can be simplified by treating it even more like a SCIFA?
>
> Yes, new patch looks like
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 367bdb913d4a..146409e44556 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1299,9 +1299,13 @@ static void sci_dma_rx_reenable_irq(struct sci_port *s)
>
> /* Direct new serial port interrupts back to CPU */
> scr = serial_port_in(port, SCSCR);
> - if (port->type == PORT_SCIFA || port->type == PORT_SCIFB) {
> - scr &= ~SCSCR_RDRQE;
> + if (port->type == PORT_SCIFA || port->type == PORT_SCIFB ||
> + s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE) {
> enable_irq(s->irqs[SCIx_RXI_IRQ]);
> + if (s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE)
> + scif_set_rtrg(port, s->rx_trigger);
> + else
> + scr &= ~SCSCR_RDRQE;
> }
> serial_port_out(port, SCSCR, scr | SCSCR_RIE);
> }
> @@ -1538,7 +1542,8 @@ static enum hrtimer_restart sci_dma_rx_timer_fn(struct hrtimer *t)
> tty_flip_buffer_push(&port->state->port);
> }
>
> - if (port->type == PORT_SCIFA || port->type == PORT_SCIFB)
> + if (port->type == PORT_SCIFA || port->type == PORT_SCIFB ||
> + s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE)
> sci_dma_rx_submit(s, true);
>
> sci_dma_rx_reenable_irq(s);
> @@ -1662,7 +1667,8 @@ static void sci_request_dma(struct uart_port *port)
>
> s->chan_rx_saved = s->chan_rx = chan;
>
> - if (port->type == PORT_SCIFA || port->type == PORT_SCIFB)
> + if (port->type == PORT_SCIFA || port->type == PORT_SCIFB ||
> + s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE)
> sci_dma_rx_submit(s, false);
> }
> }
> @@ -1715,9 +1721,16 @@ static irqreturn_t sci_rx_interrupt(int irq, void *ptr)
> u16 ssr = serial_port_in(port, SCxSR);
>
> /* Disable future Rx interrupts */
> - if (port->type == PORT_SCIFA || port->type == PORT_SCIFB) {
> - disable_irq_nosync(irq);
> - scr |= SCSCR_RDRQE;
> + if (port->type == PORT_SCIFA || port->type == PORT_SCIFB ||
> + s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE) {
> + if (s->cfg->regtype != SCIx_RZ_SCIFA_REGTYPE) {
Please reverse order, and use "==", to match the flow in
sci_dma_rx_reenable_irq() above.
> + disable_irq_nosync(irq);
I think that can be s->irqs[SCIx_RXI_IRQ] unconditionally, i.e. outside
the if/else?
> + scr |= SCSCR_RDRQE;
> + } else {
> + disable_irq_nosync(s->irqs[SCIx_RXI_IRQ]);
> + scif_set_rtrg(port, 1);
> + scr |= SCSCR_RIE;
> + }
> } else {
> if (sci_dma_rx_submit(s, false) < 0)
> goto handle_pio;
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
