On 06.10.2017 18:50, Jon Hunter wrote:
>
> On 06/10/17 16:26, Dmitry Osipenko wrote:
>> On 06.10.2017 16:11, Jon Hunter wrote:
>>>
>>> On 04/10/17 00:58, Dmitry Osipenko wrote:
>>>> AHB DMA controller presents on Tegra20/30 SoC's, it supports transfers
>>>> memory <-> AHB bus peripherals as well as mem-to-mem transfers. Driver
>>>> doesn't yet implement transfers larger than 64K and scatter-gather
>>>> transfers that have NENT > 1, HW doesn't have native support for these
>>>> cases, mem-to-mem isn't implemented as well.
>>>
>>> The APB DMA does not have h/w support for sg-transfers either, but
>>> transfer request are placed on a list. Can we not do the same for AHB?
>>>
>>
>> We can, but I'm not going to implement it without a use-case. It could be done
>> later if needed.
>
> OK, that's fine, maybe state that above.
>
Well, I think it is explicitly mentioned in the commit message.
"Driver doesn't yet implement transfers larger than 64K and scatter-gather
transfers that have NENT > 1".
Isn't it enough?
> [...]
>
>>>> +static void tegra_ahbdma_issue_next_tx(struct tegra_ahbdma_chan *chan)
>>>> +{
>>>> + struct tegra_ahbdma_tx_desc *tx = tegra_ahbdma_get_next_tx(chan);
>>>> +
>>>> + if (tx) {
>>>> + writel_relaxed(tx->ahb_seq, chan->regs + AHBDMA_CH_AHB_SEQ);
>>>> + writel_relaxed(tx->ahb_addr, chan->regs + AHBDMA_CH_AHB_PTR);
>>>> + writel_relaxed(tx->mem_addr, chan->regs + AHBDMA_CH_XMB_PTR);
>>>> + writel_relaxed(tx->csr, chan->regs + AHBDMA_CH_CSR);
>>>> +
>>>> + reinit_completion(&chan->idling);
>>>
>>> Should this be done before actually starting the DMA?
>
> OK, then that's fine.
>
> [...]
>
>>>> + else {
>>>> + tx = to_ahbdma_tx_desc(vdesc);
>>>> +
>>>> + if (tx == ahbdma_chan->active_tx)
>>>> + residual = tegra_ahbdma_residual(ahbdma_chan);
>>>> + else
>>>> + residual = tx->csr & AHBDMA_CH_WCOUNT_MASK;
>>>> +
>>>> + residual += sizeof(u32);
>>>> + }
>>>> +
>>>> + dma_set_residue(state, residual);
>>>
>>> I believe residue needs to be bytes.
>
> Oops yes indeed!
>
>>>> +static int tegra_ahbdma_terminate_all(struct dma_chan *chan)
>>>> +{
>>>> + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan);
>>>> + unsigned long flags;
>>>> + LIST_HEAD(head);
>>>> + u32 csr;
>>>> +
>>>> + spin_lock_irqsave(&ahbdma_chan->vchan.lock, flags);
>>>> +
>>>> + csr = readl_relaxed(ahbdma_chan->regs + AHBDMA_CH_CSR);
>>>> + writel_relaxed(csr & ~AHBDMA_CH_ENABLE,
>>>> + ahbdma_chan->regs + AHBDMA_CH_CSR);
>>>> +
>>>> + if (ahbdma_chan->active_tx) {
>>>> + udelay(AHBDMA_BURST_COMPLETE_TIME);
>>>
>>> Why not poll the status register and wait for the channel to stop?
>>>
>>
>> That probably would also work. But I'm not sure whether status depends on the
>> channels "enable" state..
>
> Well if it is not enabled, then we probably don't care about the state.
> However, a quick test should tell us.
>
Okay, I'll try to check it.
>>>> +
>>>> + writel_relaxed(AHBDMA_CH_IS_EOC,
>>>> + ahbdma_chan->regs + AHBDMA_CH_STA);
>>>> +
>>>> + ahbdma_chan->active_tx = NULL;
>>>> + }
>>>> +
>>>> + vchan_get_all_descriptors(&ahbdma_chan->vchan, &head);
>>>> + complete_all(&ahbdma_chan->idling);
>>>> +
>>>> + spin_unlock_irqrestore(&ahbdma_chan->vchan.lock, flags);
>>>> +
>>>> + vchan_dma_desc_free_list(&ahbdma_chan->vchan, &head);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int tegra_ahbdma_config(struct dma_chan *chan,
>>>> + struct dma_slave_config *sconfig)
>>>> +{
>>>> + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan);
>>>> + enum dma_transfer_direction dir = sconfig->direction;
>>>> + u32 burst, ahb_seq, csr;
>>>> + unsigned int slave_id;
>>>> + phys_addr_t ahb_addr;
>>>> +
>>>> + if (sconfig->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES ||
>>>> + sconfig->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES)
>>>> + return -EINVAL;
>>>> +
>>>> + switch (dir) {
>>>> + case DMA_DEV_TO_MEM:
>>>> + burst = sconfig->src_maxburst;
>>>> + ahb_addr = sconfig->src_addr;
>>>> + break;
>>>> + case DMA_MEM_TO_DEV:
>>>> + burst = sconfig->dst_maxburst;
>>>> + ahb_addr = sconfig->dst_addr;
>>>> + break;
>>>> + default:
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + switch (burst) {
>>>> + case 1:
>>>> + burst = AHBDMA_CH_AHB_BURST_1;
>>>> + break;
>>>> + case 4:
>>>> + burst = AHBDMA_CH_AHB_BURST_4;
>>>> + break;
>>>> + case 8:
>>>> + burst = AHBDMA_CH_AHB_BURST_8;
>>>> + break;
>>>> + default:
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + if (ahb_addr & 3)
>>>> + return -EINVAL;
>>>> +
>>>> + ahb_seq = burst << AHBDMA_CH_AHB_BURST_SHIFT;
>>>> + ahb_seq |= AHBDMA_CH_INTR_ENB;
>>>> +
>>>> + csr = AHBDMA_CH_ENABLE;
>>>> + csr |= AHBDMA_CH_IE_EOC;
>>>> +
>>>> + if (ahbdma_chan->of_slave || sconfig->device_fc) {
>>>> + if (ahbdma_chan->of_req_sel < TEGRA_AHBDMA_REQ_N_A)
>>>> + slave_id = ahbdma_chan->of_req_sel;
>>>> + else
>>>> + slave_id = sconfig->slave_id;
>>>> +
>>>> + if (slave_id > 15)
>>>> + return -EINVAL;
>>>
>>> Why not ...
>>>
>>> if (ahbdma_chan->of_req_sel < TEGRA_AHBDMA_REQ_N_A)
>>> slave_id = ahbdma_chan->of_req_sel;
>>> else if (slave_id = sconfig->slave_id < TEGRA_AHBDMA_REQ_N_A)
>>> slave_id = sconfig->slave_id;
>>> else
>>> return -EINVAL;
>>>
>>
>> Because I'm finding variant like yours more difficult to read. I'll stick with
>> my variant if you don't mind.
>
> Ha! I prefer mine :-)
>
Okay, I'll switch to yours ;)
>>>> +
>>>> + ahb_seq |= AHBDMA_CH_ADDR_WRAP;
>>>> +
>>>> + csr |= slave_id << AHBDMA_CH_REQ_SEL_SHIFT;
>>>> + csr |= AHBDMA_CH_FLOW;
>>>> + }
>>>> +
>>>> + ahbdma_chan->csr = csr;
>>>> + ahbdma_chan->ahb_seq = ahb_seq;
>>>> + ahbdma_chan->ahb_addr = ahb_addr;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void tegra_ahbdma_synchronize(struct dma_chan *chan)
>>>> +{
>>>> + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan);
>>>> +
>>>> + wait_for_completion(&ahbdma_chan->idling);
>>>> + vchan_synchronize(&ahbdma_chan->vchan);
>>>> +}
>>>> +
>>>> +static void tegra_ahbdma_free_chan_resources(struct dma_chan *chan)
>>>> +{
>>>> + vchan_free_chan_resources(to_virt_chan(chan));
>>>> +}
>>>> +
>>>> +static void tegra_ahbdma_init_channel(struct tegra_ahbdma *tdma,
>>>> + unsigned int chan_id)
>>>> +{
>>>> + struct tegra_ahbdma_chan *ahbdma_chan = &tdma->channels[chan_id];
>>>> + struct dma_device *dma_dev = &tdma->dma_dev;
>>>> +
>>>> + vchan_init(&ahbdma_chan->vchan, dma_dev);
>>>> + init_completion(&ahbdma_chan->idling);
>>>> + complete(&ahbdma_chan->idling);
>>>> +
>>>> + ahbdma_chan->regs = tdma->regs + AHBDMA_CH_BASE(chan_id);
>>>> + ahbdma_chan->vchan.desc_free = tegra_ahbdma_tx_desc_free;
>>>> + ahbdma_chan->of_req_sel = TEGRA_AHBDMA_REQ_N_A;
>>>> +}
>>>> +
>>>> +static struct dma_chan *tegra_ahbdma_of_xlate(struct of_phandle_args *dma_spec,
>>>> + struct of_dma *ofdma)
>>>> +{
>>>> + struct tegra_ahbdma *tdma = ofdma->of_dma_data;
>>>> + struct dma_chan *chan;
>>>> +
>>>> + chan = dma_get_any_slave_channel(&tdma->dma_dev);
>>>> + if (!chan)
>>>> + return NULL;
>>>> +
>>>> + to_ahbdma_chan(chan)->of_req_sel = dma_spec->args[0];
>>>
>>> Test for args[0] < TEGRA_AHBDMA_REQ_N_A?
>>>
>>
>> It would duplicate slave_id checking done in tegra_ahbdma_config(), so not
>> needed here.
>
> But surely we should not let them request a channel in the first place?
>
If allowing client to disable flow control is okay, as you mentioned below, then
I agree that it is fine. I'll make this change.
>>>> + to_ahbdma_chan(chan)->of_slave = true;
>>>
>>> Is this really needed? Doesn't a value of 0..TEGRA_AHBDMA_REQ_N_A-1 tell
>>> us it is valid?
>>>
>>
>> I think we should enforce channels flow control in a case of OF xlate'd channel,
>> no? To avoid abusing channels usage by client. Seems tegra_ahbdma_config isn't
>> correct, should be:
>
> Absolutely. However, I don't see the need for the additional 'of_slave'
> variable. If we validate the slave id here, we can get rid of the extra
> variable. It does not simplify the code really by adding this IMO.
>
'of_slave' enforces flow control enable. If I understand you correctly, you are
suggesting that it is okay to leave ability for clients to override flow
control. Well, that's probably is fine indeed, just keep an eye on client drivers.
>> if (ahbdma_chan->of_slave || sconfig->device_fc) {
>> - if (ahbdma_chan->of_req_sel < TEGRA_AHBDMA_REQ_N_A)
>> + if (ahbdma_chan->of_slave)
>> slave_id = ahbdma_chan->of_req_sel;
>> else
>> slave_id = sconfig->slave_id;
>>
>> if (slave_id >= TEGRA_AHBDMA_REQ_N_A)
>> return -EINVAL;
>>
>> I'm finding OF-requsted channel + ability of DMA API to override requested
>> channels parameters quite vague. So I'm not exactly sure how to handle it
>> correctly. It looks like each driver is free to do its own thing, which is kinda
>> a mess. Suggestions?
>
> I think it is fine how you have it and limit OF-requested channels to
> actual REQ_SEL values.
>
Okay.
--
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
