Re: [PATCH 3/3] dmaengine: tegra-adma: Add support for Tegra210 ADMA

[Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On 28/09/15 17:36, Stephen Warren wrote:
> On 09/28/2015 08:57 AM, Jon Hunter wrote:
>>
>> On 25/09/15 16:47, Arnd Bergmann wrote:
>>> On Friday 25 September 2015 16:38:55 Jon Hunter wrote:
>>>> On 25/09/15 16:17, Jon Hunter wrote:
>>>>>
>>>>> On 25/09/15 16:03, Arnd Bergmann wrote:
>>>>>> On Friday 25 September 2015 15:56:40 Jon Hunter wrote:
>>>>>>> +       case DMA_MEM_TO_DEV:
>>>>>>> +               burst_size = fls(tdc->config.dst_maxburst);
>>>>>>> +               ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs
>>>>>>> - 1);
>>>>>>> +               ch_regs->ctrl =
>>>>>>> ADMA_CH_CTRL_XFER_DIR(ADMA_MEM_TO_AHUB) |
>>>>>>> +                              
>>>>>>> ADMA_CH_CTRL_TX_REQ(tdc->config.slave_id);
>>>>>>> +               ch_regs->src_addr = buf_addr;
>>>>>>> +               break;
>>>>>>> +
>>>>>>> +       case DMA_DEV_TO_MEM:
>>>>>>> +               burst_size = fls(tdc->config.src_maxburst);
>>>>>>> +               ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs
>>>>>>> - 1);
>>>>>>> +               ch_regs->ctrl =
>>>>>>> ADMA_CH_CTRL_XFER_DIR(ADMA_AHUB_TO_MEM) |
>>>>>>> +                              
>>>>>>> ADMA_CH_CTRL_RX_REQ(tdc->config.slave_id);
>>>>>>> +               ch_regs->trg_addr = buf_addr;
>>>>>>> +               break;
>>>>>>
>>>>>> Do not use the 'slave_id' field here to identify the slave device,
>>>>>> that
>>>>>> concept is broken. Instead, put the slave identification into the
>>>>>> dma specifier in DT and read it out in your xlate function.
>>>>>
>>>>> Why is it broken?
>>>>>
>>>>> What happens if I don't know the slave-id? In other words, the
>>>>> slave-id
>>>>> can be dynamically allocated and associated with a given slave.
>>>>
>>>> I guess thinking about it some more, the driver could assign an id
>>>> itself to a given channel and I could avoid using slave_id here. There
>>>> are 22 channels and 10 tx and 10 rx requests.
>>>
>>> This sounds roughly right. So you could pick the ch_regs->ctrl value
>>> when you allocate the tegra_adma_chan structure, and then map it to
>>> the slave in the xlate() function.
>>
>> Actually, what I mentioned about would not work as it is not the DMA
>> that should assign the requests to the channel.
>>
>> I think that I have poorly described the hardware and how it works, so
>> let me try and explain a bit more.
>>
>>  From a hardware perspective it looks like the following ...
>>
>> memory <-> adma <-> adma-if <-> xbar <-> clients
>>
>> where:
>> - memory is the system memory
>> - adma is the dma controller
>> - adma-if is the dma interface to a crossbar
>> - xbar is the crossbar
>> - clients are various audio interfaces, such as i2s, etc
>>
>> The adma-if is essentially a mux with 10 tx and 10 rx ports. Any of the
>> 22 adma channels can be mapped to any of the 10 tx or rx ports. The
>> request signal used by the adma is determined by which port it is
>> configured to use. However, what makes this even more configurable is
>> the xbar is also a mux that can route adma-if ports to the various
>> clients.
>>
>> So potentially, you could use any adma channel and any port to route
>> audio to any of the clients. However, the adma-if needs to know which
>> adma channel is mapped to which port
> 
> It does? I'm pretty sure it didn't in earlier chips; what changed?

I *believe* that T210 is the first one to have the ADMA controller where
as previous chips used the APB-DMA controller. Looking at the APB-DMA on
T124 I can see that there is a fixed REQ_SEL value for each of the APBIF
(equivalent of the ADMA-IF on T210).

> For earlier chips, I believe all that's required is:
> 
> When programming the DMA engine, you need to know which ADMA-IF is in
> use, so the correct DMA request selector can be programmed into the DMA
> engine for flow-control.
> 
> ADMA-IF simply receives the data from DMA, and forwards it to the XBAR
> tagged with the ADMA-IF's own ID.
> 
> The XBAR programming selects which data source (ADMA-IF TX, I2S RX, ...)
> each sink (ADMARX, I2S TX, ...) receives.

Yes, exactly, this part sounds the same. However, just the ADMA itself
allows for even more configuration.

> Ideally, when an I2S controller needs to start transmitting data, it
> should dynamically allocate an ADMA-IF, query it for its DMA slave
> request ID, and then forward this information to the ASoC code that sets
> up the DMA transfer.

Agree.

> In practice, this means that since any I2S module could use any ADMA-IF,
> you probably need to list all DMA request selectors possible in the
> I2S's DMA-related properties, so it can choose which one to use.

Possibly, but really I think that the i2s only cares about the ADMA-IF
and the hardware request used by the ADMA can be abstracted by the
ADMA-IF. In other words, if you allocate an ADMA channel to work with a
specific ADMA-IF, then let the ADMA-IF select the hardware request
because as long as one is available, you don't care which.

> Or perhaps the XBAR binding should list all the DMA requestors so that
> each I2S node doesn't have to.

Yes, however, I think that the ADMA-IF would make sense as it really
cares about the mapping of hardware request to the ADMA-IF port.

Cheers
Jon