[PATCH V2 3/8] dmaengine: bcm2835: use shared interrupt for channel 11 to 14.

[Vinod Koul]

On Wed, Jan 13, 2016 at 02:30:32PM +0100, Martin Sperl wrote:
> On 13.01.2016 13:26, Vinod Koul wrote:
> >On Thu, Jan 07, 2016 at 05:33:01PM +0000, kernel at martin.sperl.org wrote:
> >>@@ -638,13 +666,21 @@ static int bcm2835_dma_probe(struct platform_device *pdev)
> >>  		goto err_no_dma;
> >>  	}
> >>
> >>-	for (i = 0; i < pdev->num_resources; i++) {
> >>-		irq = platform_get_irq(pdev, i);
> >>+	for (i = 0; i <= BCM2835_DMA_MAX_CHANNEL_NUMBER; i++) {
> >>+		if (BCM2835_DMA_IRQ_SHARED_MASK & BIT(i)) {
> >
> >Ideally this should be done thru DT data and not hard coded in kernel. I
> >dont think this assumption will hold good for next gen of this device, so
> >better to get this from DT!
> 
> The ideal solution would be breaking the DT in such a way that we could
> define a register range and interrupt per dma-channel looking something
> like this:
> 
> diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
> index 83d9787..9526b91 100644
> --- a/arch/arm/boot/dts/bcm2835.dtsi
> +++ b/arch/arm/boot/dts/bcm2835.dtsi
> @@ -31,8 +31,28 @@
> 
>                 dma: dma at 7e007000 {
>                         compatible = "brcm,bcm2835-dma";
> -                       reg = <0x7e007000 0xf00>;
> -                       interrupts = <1 16>,
> +                       reg = <0x7e007f00 0x100>, /* status reg */
> +                             <0x7e007000 0x100>,
> +                             <0x7e007100 0x100>,
> +                             <0x7e007200 0x100>,
> +                             <0x7e007300 0x100>,
> +                             <0x7e007400 0x100>,
> +                             <0x7e007500 0x100>,
> +                             <0x7e007600 0x100>,
> +                             <0x7e007700 0x100>,
> +                             <0x7e007800 0x100>,
> +                             <0x7e007900 0x100>,
> +                             <0x7e007a00 0x100>,
> +                             <0x7e007b00 0x100>,
> +                             <0x7e007c00 0x100>,
> +                             <0x7e007d00 0x100>,
> +                             <0x7e007e00 0x100>,
> +                             /* dma channel 15 uses a different base */
> +                             <0x7ee05000 0x100>;
> +                       interrupts = <1 28>, /* catch all DMA-interrupts */
> +                                    /* dma channel 0-10 interrupts */
> +                                    <1 16>,
>                                      <1 17>,
>                                      <1 18>,
>                                      <1 19>,
> @@ -43,9 +63,30 @@
>                                      <1 24>,
>                                      <1 25>,
>                                      <1 26>,
> +                                    /* dma channel 11-14 share irq */
>                                      <1 27>,
> -                                    <1 28>;
> -
> +                                    <1 27>,
> +                                    <1 27>,
> +                                    <1 27>,
> +                                    /* no irq support for dma channel 15 */
> +                                    < 0 >;
> +                       dma-names = "shared",
> +                                   "dma0",
> +                                   "dma1",
> +                                   "dma2",
> +                                   "dma3",
> +                                   "dma4",
> +                                   "dma5",
> +                                   "dma6",
> +                                   "dma7",
> +                                   "dma8",
> +                                   "dma9",
> +                                   "dma10",
> +                                   "dma11",
> +                                   "dma12",
> +                                   "dma13",
> +                                   "dma14",
> +                                   "dma15";
>                         #dma-cells = <1>;
>                         brcm,dma-channel-mask = <0x7f35>;
> (or similar)
> 
> This actually would allow us to make "brcm,dma-channel-mask" redundant,
> as we could remove those dma channels that are owned by the firmware
> directly from the list.
> 
> That way we could also map other capabilities via the DT.
> 
> It would also allow a transparent addition of additional dma channels
> with newer versions of the HW - mostly - by modifying the DT.

Precisely

> 
> But that would be frowned upon, so I had to come up with the approach
> taken, which makes the following assumptions:

DT was designed to move this info and hardcoding from kernel into
DT, so why cant we do that?

> * the DT maps only the interrupts that are assigned to the HW block
> * the driver knows about the number of DMA channels in HW
> * the driver knows about the mapping of shared interrupts
>   (11-14 share irq).
> 
> It is not optimal, but at least it works with the least amount of
> change to the DT - and what about all those assumptions that we
> would need to hard-code to be backwards compatible to the DT without?
> 
> I guess we could replace BCM2835_DMA_MAX_CHANNEL_NUMBER with:
>   /* we do not support dma channel 15 with this driver */
>   #define BCM2835_DMA_MAX_CHANNEL_SUPPORTED 14
>   ...
>   for (i = 0;
>        i <= min_t(int, flv(chans_available),
> BCM2835_DMA_MAX_CHANNEL_SUPPORTED);
>        i++) {
> 
> So which way would you prefer this to go - I got another few days
> before I leave on vacation.

I still think DT is the right way to go here, unless I hear some other
convincing answer..

-- 
~Vinod