Re: [PATCHv4] dmaengine: Add support for BCM2835

[Florian Meier <florian.meier@koalo.de>]

On 13.11.2013 21:39, Tomasz Figa wrote:
> On Wednesday 13 of November 2013 20:35:22 Florian Meier wrote:
>> On 13.11.2013 19:43, Tomasz Figa wrote:
>>>> ....
>>>> diff --git a/Documentation/devicetree/bindings/dma/bcm2835-dma.txt b/Documentation/devicetree/bindings/dma/bcm2835-dma.txt
>>>> new file mode 100644
>>>> index 0000000..bca5e84
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/dma/bcm2835-dma.txt
>>>> @@ -0,0 +1,59 @@
>>>> +* BCM2835 DMA controller
>>>> +
>>>> +Required properties:
>>>> +- compatible: Should be "brcm,bcm2835-dma".
>>>> +- reg: Should contain DMA registers location and length.
>>>> +- interrupts: Should contain the DMA interrupts associated
>>>> +		to the DMA channels in ascending order.
>>>> +		First cell is the IRQ bank.
>>>> +		Second cell is the IRQ number.
>>>> +- #dma-cells: Must be <1>, used to represent the number of integer cells in
>>>> +		the dmas property of client devices.
>>>> +- dma-channels: Maximum number of DMA channels available.
>>>> +- dma-requests: Number of DMA Requests.
>>>
>>> The two properties above do not seem to be used anywhere in the driver.
>>
>> Aren't they necessary for the DMA engine core?
> 
> Honestly, looking at the DMA bindings documentation, these two are quite
> a mystery for me. They don't seem to be used in DMA engine core, but
> instead several drivers use them for private purposes.
> 
> So, since they are optional and you don't seem to need them, I wouldn't
> list them.

Ok

>>>> +- brcm,dma-channel-mask: Bit mask representing the channels available.
>>>
>>> What does the value of this property depend on? Could you describe the
>>> structure of this DMA controller?
>>>
>>>> +
>>>> +Example:
>>>> +
>>>> +dma: dma@7e007000 {
>>>> +	compatible = "brcm,bcm2835-dma";
>>>> +	reg = <0x7e007000 0xf00>;
>>>> +	interrupts = <1 16
>>>> +		      1 17
>>>> +		      1 18
>>>> +		      1 19
>>>> +		      1 20
>>>> +		      1 21
>>>> +		      1 22
>>>> +		      1 23
>>>> +		      1 24
>>>> +		      1 25
>>>> +		      1 26
>>>> +		      1 27
>>>> +		      1 28>;
>>>
>>> There are 13 interrupts specified here, but...
>>>
>>>> +
>>>> +	#dma-cells = <1>;
>>>> +	dma-channels = <16>;
>>>
>>> ...16 channels here...
>>>
>>>> +	dma-requests = <32>;
>>>> +	brcm,dma-channel-mask = <0x7f35>;
>>>
>>> ...and 11 set bits here. May I ask you to explain this to me, please?
>>
>> How I understand this DMA controller:
>> There are 16 DMA channels in the DMA controller, but only 13 interrupts
>> are available at the IRQ controller. Therefore, the upper DMA channels
>> can just not be used. Maybe because there are to many other IRQs and
>> they didn't want to implement another IRQ bank.
>> Furthermore, some of the DMA channels are already used by the
>> VideoCore/GPU/firmware. This is what dma-channel-mask indicates. This
>> should be automatically set by the firmware in the future.
>> Finally, there are some channels with special functionality that should
>> not be used by DMA engine, too. Therefore, these lines:
>> 		/* do not use the FIQ and BULK channels */
>> 		chans_available &= ~0xD;
> 
> OK, this makes it much more clear.
> 
> So, my only comment remaining here is that you shouldn't include the
> channels without interrupt signal in the mask. This would allow you
> to define it as a mask of channels that are operable and then just
> iterate over all set bits in the driver, instead of using tricks with
> interrupt resources. What do you think?

Since the mask will come directly from the firmware, this would require
patching the firmware. I think that is not worth the effort.

>>> [snip]
>>>> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
>>>> new file mode 100644
>>>> index 0000000..baf072e
>>>> --- /dev/null
>>>> +++ b/drivers/dma/bcm2835-dma.c
>>> [snip]
>>>> +static int bcm2835_dma_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct bcm2835_dmadev *od;
>>>> +	struct resource *dma_res = NULL;
>>>> +	void __iomem *dma_base = NULL;
>>>> +	int rc = 0;
>>>> +	int i = 0;
>>>> +	int irq;
>>>> +	uint32_t chans_available;
>>> [snip]
>>>> +	if (pdev->dev.of_node) {
>>>
>>> Is this driver supposed to support non-DT based instantation (aka board
>>> files)? If not, maybe it would be cleaner to simply check for
>>> !pdev->dev.of_node at the beginning of probe and return an error?
>>
>> I would like to maintain the possibility for board file based
>> instatiation, because the Raspberry Pi downstream kernel still doesn't
>> support device tree. If this is a no-go, I will accept that.
> 
> Sure, you are free to do so.
> 
> What I meant is that your probe won't call bcm2835_dma_chan_init() at all
> if there is no pdev->dev.of_node, because the loop iterating over channels
> is under the if clause.

Yes you are right, but I think it will make the patching easier, later.
Currently, nothing bad happens without device tree - it just allocates
no channels.

Greetings,
Florian