Re: [PATCH v5 1/2] dmaengine: 8250_mtk_dma: add Mediatek uart DMA support

[Long Cheng <long.cheng@mediatek.com>]

On Mon, 2018-12-17 at 02:07 -0800, Sean Wang wrote:
> On Mon, Dec 17, 2018 at 12:40 AM Long Cheng <long.cheng@mediatek.com> wrote:
> >
> > On Fri, 2018-12-14 at 12:09 -0800, Sean Wang wrote:
> 
> < ... >
> 
> > > > > > +
> > > > > > +               mtk_dma_chan_write(c, VFF_ADDR, cfg->src_addr);
> > > > > > +               mtk_dma_chan_write(c, VFF_LEN, rx_len);
> > > > > > +               mtk_dma_chan_write(c, VFF_THRE, VFF_RX_THRE(rx_len));
> > > > > > +               mtk_dma_chan_write(c,
> > > > > > +                                  VFF_INT_EN, VFF_RX_INT_EN0_B
> > > > > > +                                  | VFF_RX_INT_EN1_B);
> > > > > > +               mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > > > > > +               mtk_dma_chan_write(c, VFF_EN, VFF_EN_B);
> > > > >
> > > > > I'd prefer to move those channel interrupt enablement to
> > > > > .device_alloc_chan_resources
> > > > > and related disablement to .device_free_chan_resources
> > > > >
> > > >
> > > > i think that, first need set the config to HW, and the enable the DMA.
> > > >
> > >
> > > I've read through the client driver and the dmaengine, I probably know
> > > how interaction they work and find out there is something you seem
> > > completely make a wrong.
> > >
> > > You can't do enable DMA with enabling VFF here. That causes a big
> > > problem, DMA would self decide to move data and not managed and issued
> > > by the dmaengine framework. For instance in DMA Tx path, because you
> > > enable the DMA and DMA  buffer (VFF) and UART Tx ring uses the same
> > > memory area,  DMA would self start to move data once data from
> > > userland go in Tx ring even without being issued by dmaengine.
> > >
> > > Instead, you should ensure all descriptors are being batched by
> > > .device_prep_slave_sg and DMA does not start moving data until
> > > .device_issue_pending done and once descriptors are issued, DMA
> > > hardware or software have to do it as fast as possible.
> > >
> >
> > the VFF enable just enable the DMA function. DMA can't move data here.
> > Now, the code get length of the data in '.device_prep_slave_sg'.
> > And let DMA move data in '.device_issue_pending function'. in here, just
> > enable the function.
> >
> 
> My all curiosity are all from the descriptor programmed in
> .device_issue_pending in the other drivers at least includes
> information such data length, target address, and hardware control
> code, but in the driver only includes data length and even the target
> address is set up at device_config, that seems unusual.
> 
> And, It does too for DMA_DEV_TO_MEM?  What I see is there is almost no
> any code be programmed for kick off the hardware for the direction but
> DMA still can move. That is another point I got confused.
> 

8250_dma in tty, mapping xmit buffer to DMA buffer. and the tx just use
the only one buffer. it's ring buffer. 8250_dma set the begin address
and length of the buffer by means of '.deivce_config' function. 
when TX happen, tty_write will write data to xmit buffer. in 8250_dma,
will set the address and length of the data by means of
'.device_prep_slave_sg'. but the address is in the xmit buffer rang. the
WPT(write pointer), RPT(read pointer) registers are recored the DMA data
transfer status, include the current location of transmission. and the
apdma is only for UART. So don't need recored the the address of data ,
target address in '.device_prep_slave_sg'.
in '.device_issue_pending', just using the data length from descriptor,
WPT ,  we can figure out what's data need to move. then update the WPT
and flush TX.

RX too. RX use other buffer in instead of XMIT buffer. when RX interrupt
coming, will update the RPT, and 8250_dma will get the length from vchan
complete. then 8250_dma can get the data.

> > > > > > +
> > > > > > +               if (!c->requested) {
> > > > > > +                       c->requested = true;
> > > > > > +                       ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > > > > > +                                         mtk_dma_rx_interrupt,
> > > > > > +                                         IRQF_TRIGGER_NONE,
> > > > > > +                                         KBUILD_MODNAME, chan);
> > > > >
> > > > > ISR registration usually happens as the driver got probe, it can give
> > > > > the system more flexibility to manage such IRQ affinity on the fly.
> > > > >
> > > >
> > > > i will move the request irq to alloc channel.
> > > >
> > >
> > > why don't let it done in driver probe?
> > >
> > there are many uart ports, like UART[0-5]. in probe, just get the all
> > irq of ports. which port is using, who request irq. example, UART1 is
> > using. we just request irq of uart1. uart0, uart[2-5] don't need request
> > irq.
> >
> > > > > > +                       if (ret < 0) {
> > > > > > +                               dev_err(chan->device->dev, "Can't request rx dma IRQ\n");
> > > > > > +                               return -EINVAL;
> > > > > > +                       }
> > > > > > +               }
> > > > > > +       } else if (cfg->direction == DMA_MEM_TO_DEV)    {
> > > > > > +               unsigned int tx_len = cfg->dst_addr_width * 1024;
> > > > >
> > > > > Ditto as above, it seems you should use cfg->dst_port_window_size
> > > > >
> > > > > > +
> > > > > > +               mtk_dma_chan_write(c, VFF_ADDR, cfg->dst_addr);
> > > > > > +               mtk_dma_chan_write(c, VFF_LEN, tx_len);
> > > > > > +               mtk_dma_chan_write(c, VFF_THRE, VFF_TX_THRE(tx_len));
> > > > > > +               mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > > > > > +               mtk_dma_chan_write(c, VFF_EN, VFF_EN_B);
> > > > >
> > > > > ditto, I'd prefer to move those channel interrupt enablement to
> > > > > .device_alloc_chan_resources and related disablement to
> > > > > .device_free_chan_resources
> > > > >
> > > > > > +
> > > > > > +               if (!c->requested) {
> > > > > > +                       c->requested = true;
> > > > > > +                       ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > > > > > +                                         mtk_dma_tx_interrupt,
> > > > > > +                                         IRQF_TRIGGER_NONE,
> > > > > > +                                         KBUILD_MODNAME, chan);
> > > > >
> > > > > ditto, we can request ISR with devm_request_irq in the driver got
> > > > > probe and trim the c->request member
> > > > >
> > > > > > +                       if (ret < 0) {
> > > > > > +                               dev_err(chan->device->dev, "Can't request tx dma IRQ\n");
> > > > > > +                               return -EINVAL;
> > > > > > +                       }
> > > > > > +               }
> > > > > > +       }
> > > > > > +
> > > > > > +       if (mtkd->support_33bits)
> > > > > > +               mtk_dma_chan_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_B);
> > > > > > +
> > > > > > +       if (mtk_dma_chan_read(c, VFF_EN) != VFF_EN_B) {
> > > > > > +               dev_err(chan->device->dev,
> > > > > > +                       "config dma dir[%d] fail\n", cfg->direction);
> > > > > > +               return -EINVAL;
> > > > > > +       }
> > > > > > +
> > > > > > +       return 0;
> > > > > > +}
> > > > > > +
< ... >