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 18:23, Dmitry Osipenko wrote:
> 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:

...

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

I am simply saying that we do not need this 'of_slave' variable in
addition to the 'of_req_sel'. If we verify that 'args[0] <
TEGRA_AHBDMA_REQ_N_A' in this xlate function and set 'of_req_sel =
args[0]', then in tegra_ahbdma_config() we just have ...

	if (ahbdma_chan->of_req_sel || sconfig->device_fc) {
		if (ahbdma_chan->of_req_sel)
			slave_id = ahbdma_chan->of_req_sel;
		else if (sconfig->slave_id < TEGRA_AHBDMA_REQ_N_A)
			slave_id = sconfig->slave_id;
		else
			return -EINVAL;

		ahb_seq |= AHBDMA_CH_ADDR_WRAP;

		csr |= slave_id << AHBDMA_CH_REQ_SEL_SHIFT;
		csr |= AHBDMA_CH_FLOW;
	}

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