On 09.10.2017 12:51, Jon Hunter wrote:
>
>
> 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;
> }
>
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
