Re: [PATCH v4 10/15] dmaengine: ti: New driver for K3 UDMA - split#2: probe/remove, xlate and filter_fn

[Peter Ujfalusi]

On 12/11/2019 7.34, Vinod Koul wrote:
> On 11-11-19, 11:16, Peter Ujfalusi wrote:
>>
>>
>> On 11/11/2019 7.33, Vinod Koul wrote:
>>> On 01-11-19, 10:41, Peter Ujfalusi wrote:
>>>
>>>> +static bool udma_dma_filter_fn(struct dma_chan *chan, void *param)
>>>> +{
>>>> +	struct psil_endpoint_config *ep_config;
>>>> +	struct udma_chan *uc;
>>>> +	struct udma_dev *ud;
>>>> +	u32 *args;
>>>> +
>>>> +	if (chan->device->dev->driver != &udma_driver.driver)
>>>> +		return false;
>>>> +
>>>> +	uc = to_udma_chan(chan);
>>>> +	ud = uc->ud;
>>>> +	args = param;
>>>> +	uc->remote_thread_id = args[0];
>>>> +
>>>> +	if (uc->remote_thread_id & K3_PSIL_DST_THREAD_ID_OFFSET)
>>>> +		uc->dir = DMA_MEM_TO_DEV;
>>>> +	else
>>>> +		uc->dir = DMA_DEV_TO_MEM;
>>>
>>> Can you explain this a bit?
>>
>> The UDMAP in K3 works between two PSI-L endpoint. The source and
>> destination needs to be paired to allow data flow.
>> Source thread IDs are in range of 0x0000 - 0x7fff, while destination
>> thread IDs are 0x8000 - 0xffff.
>>
>> If the remote thread ID have the bit 31 set (0x8000) then the transfer
>> is MEM_TO_DEV and I need to pick one unused tchan for it. If the remote
>> is the source then it can be handled by rchan.
>>
>> dmas = <&main_udmap 0xc400>, <&main_udmap 0x4400>;
>> dma-names = "tx", "rx";
>>
>> 0xc400 is a destination thread ID, so it is MEM_TO_DEV
>> 0x4400 is a source thread ID, so it is DEV_TO_MEM
>>
>> Even in MEM_TO_MEM case I need to pair two UDMAP channels:
>> UDMAP source threads are starting at offset 0x1000, UDMAP destination
>> threads are 0x9000+
> 
> Okay so a channel is set for a direction until teardown. Also this and
> other patch comments are quite useful, can we add them here?

The direction checks in the prep callbacks do print the reason why the
transfer is rejected when it comes to not matching direction.

Having said that, I can add comment to the udma_alloc_chan_resources()
function about this restriction, or better a dev_dbg() to say that the
given channel is allocated for a given direction.

>> Changing direction runtime is hardly possible as it would involve
>> tearing down the channel, removing interrupts, destroying rings,
>> removing the PSI-L pairing and redoing everything.
> 
> okay I would expect the prep_ to check for direction and reject the call
> if direction is different.

They do check, udma_prep_slave_sg() and udma_prep_dma_cyclic():
if (dir != uc->dir) {
	dev_err(chan->device->dev,
		"%s: chan%d is for %s, not supporting %s\n",
		__func__, uc->id, udma_get_dir_text(uc->dir),
		udma_get_dir_text(dir));
	return NULL;
}

udma_prep_dma_memcpy():
if (uc->dir != DMA_MEM_TO_MEM) {
	dev_err(chan->device->dev,
		"%s: chan%d is for %s, not supporting %s\n",
		__func__, uc->id, udma_get_dir_text(uc->dir),
		udma_get_dir_text(DMA_MEM_TO_MEM));
	return NULL;
}

> 

- PÃter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki