Re: [PATCH v1 4/5] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller

[Dmitry Osipenko]

On 28.09.2017 19:22, Vinod Koul wrote:
> On Thu, Sep 28, 2017 at 05:35:59PM +0300, Dmitry Osipenko wrote:
>> On 28.09.2017 17:06, Dmitry Osipenko wrote:
>>> On 28.09.2017 12:29, Vinod Koul wrote:
>>>>> +	default:
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +
>>>>> +	ahb_seq  = burst << TEGRA_AHBDMA_CHANNEL_AHB_BURST_SHIFT;
>>>>> +	ahb_seq |= TEGRA_AHBDMA_CHANNEL_ADDR_WRAP;
>>>>> +	ahb_seq |= TEGRA_AHBDMA_CHANNEL_INTR_ENB;
>>>>> +
>>>>> +	writel_relaxed(ahb_seq,
>>>>> +		       ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_AHB_SEQ);
>>>>> +
>>>>> +	writel_relaxed(ahb_addr,
>>>>> +		       ahbdma_chan->regs + TEGRA_AHBDMA_CHANNEL_AHB_PTR);
>>>>
>>>> oh no, you don't write to HW here. This can be called anytime when you have
>>>> txn running! You should save these and use them in prep_ calls.
>>>>
>>>
>>> BTW, some of the DMA drivers have exactly the same problem. I now see that it is
>>> actually documented explicitly in provider.txt, but that's inconsistent across
>>> the actual drivers.
>>>
>>
>> Also, shouldn't prep_ and dma_slave_config be protected with locking? I don't
>> see DMA core doing any locking and seems none of the drivers too.
> 
> In prep when you modify the list yes (with vchan I suspect that maybe taken
> care), but in general yes driver needs to do that
> 

I meant that one CPU could modify channels config, while other CPU is preparing
the new TX using config that is in process of the modification. On the other
hand, this looks like something that DMA client should take care of.

-- 
Dmitry