On 08/07/2015 01:44 PM, Peter Ujfalusi wrote:
>>>> Cc: <stable@xxxxxxxxxxxxxxx>
>>>
>>> Why stable? This is not fixing any bugs since the PAUSE was not allowed for
>>> non cyclic transfers.
>>
>> Hmmm. The DRA7x was using pause before for UART. I just did not see it
>> coming that it was not allowed here. John made a similar change to the
>> edma driver and I assumed it went stable but now I see that it was just
>> cherry-picked into the ti tree.
>> If you are not comfortable it being stable material I can drop it.
>
> This change is needed for the UART DMA support if I'm not mistaken and this
> mode is not really supported by older kernels, so having this to implement
> something which is not going to be used in the stable kernels feels somehow wrong.
We have the DT pieces since v3.19-rc1. And if I remember correctly I
tested this on am335x-evm and dra7-evm by I the time I posted the
patches. I agree that dra7 support was not the best back then but I am
almost sure that I had vanilla running for testing.
But I don't insist on the stable tag. Consider it dropped.
>>>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
>>>> ---
>>>> drivers/dma/omap-dma.c | 54 ++++++++++++++++++++++++++++++++++++++------------
>>>> 1 file changed, 41 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c
>>>> index 249445c8a4c6..6b8497203caf 100644
>>>> --- a/drivers/dma/omap-dma.c
>>>> +++ b/drivers/dma/omap-dma.c
>>>> @@ -299,7 +299,7 @@ static void omap_dma_start(struct omap_chan *c, struct omap_desc *d)
>>>> omap_dma_chan_write(c, CCR, d->ccr | CCR_ENABLE);
>>>> }
>>>>
>>>> -static void omap_dma_stop(struct omap_chan *c)
>>>> +static int omap_dma_stop(struct omap_chan *c)
>>>> {
>>>> struct omap_dmadev *od = to_omap_dma_dev(c->vc.chan.device);
>>>> uint32_t val;
>>>> @@ -342,8 +342,26 @@ static void omap_dma_stop(struct omap_chan *c)
>>>>
>>>> omap_dma_glbl_write(od, OCP_SYSCONFIG, sysconfig);
>>>> } else {
>>>> + int i = 0;
>>>> +
>>>> + if (!(val & CCR_ENABLE))
>>>> + return -EINVAL;
>>>> +
>>>> val &= ~CCR_ENABLE;
>>>> omap_dma_chan_write(c, CCR, val);
>>>> + do {
>>>> + val = omap_dma_chan_read(c, CCR);
>>>> + if (!(val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE)))
>>>> + break;
>>>> + if (i > 100)
>>>
>>> if (++i > 100)
>>> break;
>>> to avoid infinite loop?
>>
>> Ah. So I forgot to increment the counter. A few lines above there is
>> the same loop as a workaround for something. This is the same loop. I
>> could merge the loop + warning if you prefer. to have those things in
>> one place. I could also just increment i. Merging the two loops might
>> be better.
>
> The other loop is for handling the ERRATA i541 and the two loops can not be
> merged since the errata handling also require to change in SYSCONFIG register.
yes, but I had in mind is to put the loop into one function so we gain:
+static void omap_dma_drain_chan(struct omap_chan *c)
+{
+ int i;
+ uint32_t val;
+
+ /* Wait for sDMA FIFO to drain */
+ for (i = 0; ; i++) {
+ val = omap_dma_chan_read(c, CCR);
+ if (!(val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE)))
+ break;
+
+ if (i > 100)
+ break;
+
+ udelay(5);
+ }
+
+ if (val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE))
+ dev_err(c->vc.chan.device->dev,
+ "DMA drain did not complete on lch %d\n",
+ c->dma_ch);
+}
which is invoked by both parts of the if case (handling the errata not
not) instead of having the same loop twice.
>>>> + break;
>>>> + udelay(5);
>>>> + } while (1);
>>>> +
>>>> + if (val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE))
>>>
>>> if (i > 100) ?
>>
>> While that would work, too I think it is more explicit to the reader if
>> you check for the condition that is important to you.
>
> Yeah, I see that the errata handling is doing the same, fine by me.
good.
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
