Re: [PATCH v11 2/2] dmaengine: dw-edma: Add non-LL mode

[Frank Li]

On Mon, Mar 09, 2026 at 11:18:33AM +0000, Verma, Devendra wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> > -----Original Message-----
> > From: Frank Li <Frank.li@xxxxxxx>
> > Sent: Saturday, March 7, 2026 2:47 AM
> > To: Verma, Devendra <Devendra.Verma@xxxxxxx>
> > Cc: bhelgaas@xxxxxxxxxx; mani@xxxxxxxxxx; vkoul@xxxxxxxxxx;
> > dmaengine@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx; Simek, Michal <michal.simek@xxxxxxx>
> > Subject: Re: [PATCH v11 2/2] dmaengine: dw-edma: Add non-LL mode
> >
> > Caution: This message originated from an External Source. Use proper
> > caution when opening attachments, clicking links, or responding.
> >
> >
> > On Fri, Mar 06, 2026 at 05:22:28PM +0530, Devendra K Verma wrote:
> > ...
> > > +             /*
> > > +              * When there is no valid LLP base address available then the
> > > +              * default DMA ops will use the non-LL mode.
> > > +              *
> > > +              * Cases where LL mode is enabled and client wants to use the
> > > +              * non-LL mode then also client can do so via providing the
> > > +              * peripheral_config param.
> > > +              */
> > > +             cfg_non_ll = chan->dw->chip->cfg_non_ll;
> > > +             if (config->peripheral_config) {
> > > +                     non_ll = *(int *)config->peripheral_config;
> > > +
> > > +                     if (cfg_non_ll && !non_ll) {
> > > +                             dev_err(dchan->device->dev, "invalid configuration\n");
> > > +                             return -EINVAL;
> > > +                     }
> > > +             }
> > > +
> > > +             if (cfg_non_ll || (!cfg_non_ll && non_ll))
> > > +                     chan->non_ll = true;
> >
> > this logic have a little bit complex
> >
> > if (cfg_non_ll)
> >         chan->non_ll = true;
> > else
> >         chan->non_ll = non_ll;
> >
>
> Thank you for your suggestion.
> I think it is individual preference. I am not sure what seem to be complex in the
> logic floated for review as all the boolean operations are easy to comprehend
> in a single statement.
> I am sure there are multiple ways to write the same logic.
> To me, the logic you suggested looks bigger with the same outcome delivered.
> If after distinction in variable names and simple boolean ops still cause confusion
> then I am not sure till what point it can be simplified.
> If fewer lines of code can deliver the same result, then it should be ok to keep it.
> I would request to keep the change of the floated review.

Reader need thank more about "cfg_non_ll || (!cfg_non_ll && non_ll)" to
make sure it is correct and what it do, need create true table in brain.

It is not enough straight forwards.

Frank

> Thanks!
>
> >
> > > +     } else if (config->peripheral_config) {
> > > +             dev_err(dchan->device->dev,
> > > +                     "peripheral config param applicable only for HDMA\n");
> > > +             return -EINVAL;
> > > +     }
> > >
> > ...
> > >
> > >  struct dw_edma_irq {
> > > diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c
> > > b/drivers/dma/dw-edma/dw-edma-pcie.c
> > > index b8208186a250..f538d728609f 100644
> > > --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> > > +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
> > > @@ -295,6 +295,15 @@ static void
> > dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
> > >       pdata->devmem_phys_off = off;
> > >  }
> > >
> > > +static u64 dw_edma_get_phys_addr(struct pci_dev *pdev,
> > > +                              struct dw_edma_pcie_data *pdata,
> > > +                              enum pci_barno bar) {
> > > +     if (pdev->vendor == PCI_VENDOR_ID_XILINX)
> > > +             return pdata->devmem_phys_off;
> > > +     return pci_bus_address(pdev, bar); }
> > > +
> >
> > Reduce each patches's changes, make each patch is straightforward
> >
> > Create Prepare patch firstly, change pci_bus_address() to
> > dw_edma_get_phys_addr()
> >
> > just
> >
> > dw_edma_get_phys_addr() {
> > {
> >         return return pci_bus_address(pdev, bar); }
> >
> > So this patch just add
> > two lines here
> >
> > if (pdev->vendor == PCI_VENDOR_ID_XILINX)
> >         return pdata->devmem_phys_off;
> >
> >
> > others look good.
> >
> > Frank
> >
>
> Regarding this we already had discussion and it was concluded to let this piece of code
> to be as is. Please check the discussion at the following link:
> https://lore.kernel.org/all/aXe5ts7E6lUF7YRq@lizhi-Precision-Tower-5810/
>
> > >