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
