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

[Verma, Devendra]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Frank Li <Frank.li@xxxxxxx>
> Sent: Monday, March 9, 2026 9:10 PM
> 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 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.
>

Hi Frank
I respectfully disagree with your assessment of the conditional statement.
Your suggestion is not acceptable for the following reasons:
 - Unnecessarily expands the logic to 'else' condition
- end-result is unpredictable
- as a side effect increases the number of lines of code, if this to be done for every logic
   Including multiple statements when the "if()" clause can accept multiple boolean statements.

Nevethless, I am going to simplify the logic further; the above statement can be written as following:
If (cfg_non_ll || non_ll)
   chan->non_ll = true;

This way it is short and concise statement to follow, without any 'else' clause.
As I said, there could be multiple ways to write a logic.
The above logic is acceptable and represents the both the logics, proposed, and floated one.
And, it is straight forward too!

I will float the next version of the patch series, please provide your approval. Thanks!

> 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-581
> > 0/
> >
> > > >