RE: [PATCH v2 3/3] tty: serial: sh-sci: Add RZ/G2L SCIF DMA rx support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v2 3/3] tty: serial: sh-sci: Add RZ/G2L SCIF DMA rx
> support
> 
> 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.

OK.

> 
> > +                               disable_irq_nosync(irq);
> 
> I think that can be s->irqs[SCIx_RXI_IRQ] unconditionally, i.e. outside the
> if/else?

Ok. Will do.

Thanks and regards,
Biju

> 
> > +                               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@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




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux