Re: [PATCH v2 2/3] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

[...]

>>> +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.

>>> +
>>> +		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 :-)

>>> +
>>> +		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?

>>> +	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.

> 	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.

Cheers
Jon

-- 
nvpublic
--
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




[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux PCI]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux