Re: [PATCH v2 2/2] dmaengine: dw-edma: Do not enable watermark interrupts for HDMA

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

 



On Mon, Aug 05, 2024 at 07:34:46PM +0530, Mrinmay Sarkar wrote:
> 
> On 8/2/2024 6:04 AM, Serge Semin wrote:
> > On Tue, Jul 23, 2024 at 06:49:32PM +0530, Mrinmay Sarkar wrote:
> > > DW_HDMA_V0_LIE and DW_HDMA_V0_RIE are initialized as BIT(3) and BIT(4)
> > > respectively in dw_hdma_control enum. But as per HDMA register these
> > > bits are corresponds to LWIE and RWIE bit i.e local watermark interrupt
> > > enable and remote watermarek interrupt enable. In linked list mode LWIE
> > > and RWIE bits only enable the local and remote watermark interrupt.
> > > 
> > > Since the watermark interrupts are not used but enabled, this leads to
> > > spurious interrupts getting generated. So remove the code that enables
> > > them to avoid generating spurious watermark interrupts.
> > > 
> > > And also rename DW_HDMA_V0_LIE to DW_HDMA_V0_LWIE and DW_HDMA_V0_RIE to
> > > DW_HDMA_V0_RWIE as there is no LIE and RIE bits in HDMA and those bits
> > > are corresponds to LWIE and RWIE bits.
> > > 
> > > Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
> > > cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Mrinmay Sarkar <quic_msarkar@xxxxxxxxxxx>
> > > ---
> > >   drivers/dma/dw-edma/dw-hdma-v0-core.c | 17 +++--------------
> > >   1 file changed, 3 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> > > index fa89b3a..9ad2e28 100644
> > > --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> > > +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> > > @@ -17,8 +17,8 @@ enum dw_hdma_control {
> > >   	DW_HDMA_V0_CB					= BIT(0),
> > >   	DW_HDMA_V0_TCB					= BIT(1),
> > >   	DW_HDMA_V0_LLP					= BIT(2),
> > > -	DW_HDMA_V0_LIE					= BIT(3),
> > > -	DW_HDMA_V0_RIE					= BIT(4),
> > > +	DW_HDMA_V0_LWIE					= BIT(3),
> > > +	DW_HDMA_V0_RWIE					= BIT(4),
> > >   	DW_HDMA_V0_CCS					= BIT(8),
> > >   	DW_HDMA_V0_LLE					= BIT(9),
> > >   };
> > > @@ -195,25 +195,14 @@ static void dw_hdma_v0_write_ll_link(struct dw_edma_chunk *chunk,
> > >   static void dw_hdma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
> > >   {
> > >   	struct dw_edma_burst *child;
> > > -	struct dw_edma_chan *chan = chunk->chan;
> > >   	u32 control = 0, i = 0;
> > > -	int j;
> > >   	if (chunk->cb)
> > >   		control = DW_HDMA_V0_CB;
> > > -	j = chunk->bursts_alloc;
> > > -	list_for_each_entry(child, &chunk->burst->list, list) {
> > > -		j--;
> > > -		if (!j) {
> > > -			control |= DW_HDMA_V0_LIE;
> > > -			if (!(chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
> > > -				control |= DW_HDMA_V0_RIE;
> > > -		}
> > > -
> > > +	list_for_each_entry(child, &chunk->burst->list, list)
> > >   		dw_hdma_v0_write_ll_data(chunk, i++, control, child->sz,
> > >   					 child->sar, child->dar);
> > > -	}
> > Hm, in case of DW EDMA the LIE/RIE flags of the LL entries gets to be
> > moved to the LIE/RIE flags of the channel context register by the
> > DMA-engine. In its turn the context register LIE/RIE flags determine
> > whether the Local and Remote Done/Abort IRQs being raised. So without
> > the LIE/RIE flags being set in the LL-entries the IRQs won't be raised
> > and the whole procedure won't work. I have doubts it works differently
> > in case of HDMA because changing the semantics would cause
> > implementing additional logic in the DW HDMA RTL-model. Seeing the DW
> > HDMA IP-core supports the eDMA compatibility mode it would needlessly
> > expand the controller size. What are the rest of the CONTROL1 register
> > fields? There must be LIE/RIE flags someplace there for the non-LL
> > transfers and to preserve the values retrieved from the LL-entries.
> > 
> > Moreover the DW eDMA HW manual has a dedicated chapter called
> > "Interrupts and Error Handling" with a very demonstrative figures
> > describing the way the flags work. Does the DW HDMA databook have
> > something like that?
> > 
> > Please also note, the DW _EDMA_ LIE and RIE flags can be also utilized
> > for the intermediate IRQ raising, to implement the runtime LL-entries
> > recycling pattern. The IRQ in that case is called as "watermark" IRQ
> > in the DW EDMA HW databook, but the flags are still called as just
> > LIE/RIE.
> > 
> > -Serge(y)
> Yes, you are right LIE/RIE flags need to be set without that the IRQs
> won't be raised in case of DW EDMA.
> But in DW HDMA case there in no such LIE/RIE flags and these particular
> bits has been mapped to LWIE and RWIE flags and these are used to enable
> watermark interrupt only.
> There is no LIE/RIE fields in HDMA_CONTROL1_OFF_WRCH register fields
> the same is present in EDMA CONTROL1 register.
> 
> DW HDMA has INT_SETUP register and it has LSIE/RSIE, LAIE/RAIE fields
> those are enabling Local and Remote Stop/Abort IRQs in LL mode.
> 
> yes DW HDMA data book also have figures in "Interrupts and Error Handling"
> section and there is no LIE/RIE flags and it is replaced with LWIE/RWIE
> flags
> as I mentioned above.

Got it. Thanks for clarification. No more notes in this regard.
Reviewed-by: Serge Semin <fancer.lancer@xxxxxxxxx>

-Serge(y)

> 
> Thanks,
> Mrinmay
> 
> > >   	control = DW_HDMA_V0_LLP | DW_HDMA_V0_TCB;
> > >   	if (!chunk->cb)
> > > -- 
> > > 2.7.4
> > > 




[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux PCI]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux