| Message ID | 1457175142-28665-5-git-send-email-kernel@martin.sperl.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
kernel@martin.sperl.org writes: > From: Martin Sperl <kernel@martin.sperl.org> > > Add binding documentation for the new shared interrupt properties: > * brcm,dma-channel-shared-mask > * brcm,dma-shared-irq-index > > Signed-off-by: Martin Sperl <kernel@martin.sperl.org> > --- > Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt b/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt > index 1396078..f9e84ee 100644 > --- a/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt > +++ b/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt > @@ -17,6 +17,10 @@ Required properties: > - brcm,dma-channel-mask: Bit mask representing the channels > not used by the firmware in ascending order, > i.e. first channel corresponds to LSB. > +- brcm,dma-channel-shared-mask: Bit mask representing the channels > + that use a shared interrupt > +- brcm,dma-shared-irq-index: index of which of the interrupts mentioned > + above is the shared interrupt This should be under "Optional properties" since there are appropriate defaults for the only compatible string listed. I still don't think exposing this in the DT is necessary (it's hardware block internals), but I'm not writing the code so I'm fine with it either way, really. Regardless, it would be really good to get the slave_sg part of the series in, which is really important for the platform.
> On 08.03.2016, at 00:24, Eric Anholt <eric@anholt.net> wrote: > > kernel@martin.sperl.org writes: > >> From: Martin Sperl <kernel@martin.sperl.org> >> >> Add binding documentation for the new shared interrupt properties: >> * brcm,dma-channel-shared-mask >> * brcm,dma-shared-irq-index >> >> Signed-off-by: Martin Sperl <kernel@martin.sperl.org> >> --- >> Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt b/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt >> index 1396078..f9e84ee 100644 >> --- a/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt >> +++ b/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt >> @@ -17,6 +17,10 @@ Required properties: >> - brcm,dma-channel-mask: Bit mask representing the channels >> not used by the firmware in ascending order, >> i.e. first channel corresponds to LSB. >> +- brcm,dma-channel-shared-mask: Bit mask representing the channels >> + that use a shared interrupt >> +- brcm,dma-shared-irq-index: index of which of the interrupts mentioned >> + above is the shared interrupt > > This should be under "Optional properties" since there are appropriate > defaults for the only compatible string listed. Well - IMO it is actually required and the defaults are only for backwards compatibility with older device-trees, but that is easy to change… > > I still don't think exposing this in the DT is necessary (it's hardware > block internals), but I'm not writing the code so I'm fine with it Actually this was a request by Vinod to make this configurable via the device-tree. > either way, really. Regardless, it would be really good to get the > slave_sg part of the series in, which is really important for the > platform. Yes, then we can bring the DMA implementations for mmc/sdhost forward. Martin
Hi, As a general note, please put DT bindings patches before any patches implementing them, as per Documentation/devicetree/bindings/submitting-patches.txt. That makes it possible to review a series in-order. On Sat, Mar 05, 2016 at 10:52:15AM +0000, kernel@martin.sperl.org wrote: > From: Martin Sperl <kernel@martin.sperl.org> > > Add binding documentation for the new shared interrupt properties: > * brcm,dma-channel-shared-mask > * brcm,dma-shared-irq-index > > Signed-off-by: Martin Sperl <kernel@martin.sperl.org> > --- > Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt b/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt > index 1396078..f9e84ee 100644 > --- a/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt > +++ b/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt > @@ -17,6 +17,10 @@ Required properties: > - brcm,dma-channel-mask: Bit mask representing the channels > not used by the firmware in ascending order, > i.e. first channel corresponds to LSB. > +- brcm,dma-channel-shared-mask: Bit mask representing the channels > + that use a shared interrupt Generally we don't like masks in DT (though I see this is in keeping with another property above). I won't push strongly here. I take it that this is a fixed HW property rather than a software configuration option? > +- brcm,dma-shared-irq-index: index of which of the interrupts mentioned > + above is the shared interrupt What is the usual order of the interrupts? How are they expected to be parsed if this is in the middle of the list, for example? I think this needs a more thorough description. Thanks, Mark.
On Tue, Mar 08, 2016 at 12:23:51PM +0100, Martin Sperl wrote: > > > On 08.03.2016, at 00:24, Eric Anholt <eric@anholt.net> wrote: > > > > kernel@martin.sperl.org writes: > > > >> From: Martin Sperl <kernel@martin.sperl.org> > >> > >> Add binding documentation for the new shared interrupt properties: > >> * brcm,dma-channel-shared-mask > >> * brcm,dma-shared-irq-index > >> > >> Signed-off-by: Martin Sperl <kernel@martin.sperl.org> > >> --- > >> Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt b/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt > >> index 1396078..f9e84ee 100644 > >> --- a/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt > >> +++ b/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt > >> @@ -17,6 +17,10 @@ Required properties: > >> - brcm,dma-channel-mask: Bit mask representing the channels > >> not used by the firmware in ascending order, > >> i.e. first channel corresponds to LSB. > >> +- brcm,dma-channel-shared-mask: Bit mask representing the channels > >> + that use a shared interrupt > >> +- brcm,dma-shared-irq-index: index of which of the interrupts mentioned > >> + above is the shared interrupt > > > > This should be under "Optional properties" since there are appropriate > > defaults for the only compatible string listed. > > Well - IMO it is actually required and the defaults are only for backwards > compatibility with older device-trees, but that is easy to change… > > > > I still don't think exposing this in the DT is necessary (it's hardware > > block internals), but I'm not writing the code so I'm fine with it > Actually this was a request by Vinod to make this configurable via the > device-tree. DT needs to provide the hardware details for driver to work across generations. Driver needs to get information like this to make decisions and not hard code and make driver not scale.. > > > either way, really. Regardless, it would be really good to get the > > slave_sg part of the series in, which is really important for the > > platform. > Yes, then we can bring the DMA implementations for mmc/sdhost forward. > > Martin > > > -- > To unsubscribe from this list: send the line "unsubscribe dmaengine" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
> On 10.03.2016, at 09:57, Mark Rutland <mark.rutland@arm.com> wrote: > > Hi, > > As a general note, please put DT bindings patches before any patches > implementing them, as per > Documentation/devicetree/bindings/submitting-patches.txt. > > That makes it possible to review a series in-order. > > On Sat, Mar 05, 2016 at 10:52:15AM +0000, kernel@martin.sperl.org wrote: >> From: Martin Sperl <kernel@martin.sperl.org> >> >> Add binding documentation for the new shared interrupt properties: >> * brcm,dma-channel-shared-mask >> * brcm,dma-shared-irq-index >> >> Signed-off-by: Martin Sperl <kernel@martin.sperl.org> >> --- >> Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt b/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt >> index 1396078..f9e84ee 100644 >> --- a/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt >> +++ b/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt >> @@ -17,6 +17,10 @@ Required properties: >> - brcm,dma-channel-mask: Bit mask representing the channels >> not used by the firmware in ascending order, >> i.e. first channel corresponds to LSB. >> +- brcm,dma-channel-shared-mask: Bit mask representing the channels >> + that use a shared interrupt > > Generally we don't like masks in DT (though I see this is in keeping with > another property above). I won't push strongly here. > > I take it that this is a fixed HW property rather than a software configuration > option? This is fixed HW property - the mask/mapping was originally proposed to be “hardcoded” inside the driver. Maybe some background on the dma-channel-mask property: it is changed during loading the dt by the firmware to mask out the channels that the firmware is using. > >> +- brcm,dma-shared-irq-index: index of which of the interrupts mentioned >> + above is the shared interrupt > > What is the usual order of the interrupts? How are they expected to be parsed > if this is in the middle of the list, for example? > > I think this needs a more thorough description. The reason that it was done this way is mostly because of DT-backwards compatibility the 11th irq is the shared interrupt -there were some wrong assumptions by the original author on the interrupts (i.e there is a dedicated interrupt per channel). Just hardcoding it inside the driver was not what Vinod wanted - even if the design stayed fixed for now 3 different chips: bcm2835, bcm2836 and bcm2837 Alternative options that have been considered: * there is unfortunately no “interrupt-names” property (like it exists for reg, dma), because then I would have preferred to used: interrupts = <...>, <...>, …; interrupt-name = “dma0”, “dma1”, ..., “dma10”, “dmashared”, “dmaall”; with something like this we could probably have avoided both properties and just added a legacy mapping This would require some changes to the irq framework (which I wanted to avoid) * I could have also written a custom parser in the driver to allow: interrupt-shared = <&int X>; but I would guess something like this to be frowned upon... * a total restructure of the DT to a more expressive format so that each channel would need to be described separately (with the corresponding custom dt parser). The big advantage here would be that we can define the reg and irq range for each channel separately. Could look something like: dma: dma@7e0070fe0 { reg = <0x7e0070fe0 0x20>; compatible = "brcm,bcm2835-dma”; interrupts = <irq-shared> <irq-all>; dma0: dma@7e007000 { reg = <0x7e007000 0x24>; interrupts = <irq-dma0>; }; dma1: dma@7e007100 { reg = <0x7e007100 0x24>; interrupts = <irq-dma1>; }; ... dma10: dma@7e007a00 { reg = <0x7e007a00 0x24>; interrupts = <irq-dma10>; }; /* channel with shared interrupts */ dma11: dma@7e007b00 { reg = <0x7e007b00 0x24>; /* no irq - use shared */ }; dma14: dma@7e007b00 { reg = <0x7e007b00 0x24>; /* no irq - use shared */ }; /* * dma channel 15 is a different type: * only triggers irq-all (with all its problems) * is in a different memory location */ dma15: dma@7ee05000 { reg = <0x7ee05000 0x24>; /* potentially mark the use of the “all” irq */ }; }; It would make the device tree a lot longer (at least 60 lines) for minimal gains. In addition it is a lot of effort for something that has been fixed for 3 chip designs (bcm2835, bcm2836 and bcm2837) Something like this has been proposed as an option end of January, but there was never a feedback if something like this was preferred. Please give guidance on the preferred solution. We really would like to get the slave-dma feature into the kernel so that we can start using it with the mmc/sdhost implementation. Thanks, Martin
Hi Mark! On 11.03.2016 09:51, Martin Sperl wrote: >> On 10.03.2016, at 09:57, Mark Rutland <mark.rutland@arm.com> wrote: >> >> Alternative options that have been considered: >> * there is unfortunately no “interrupt-names” property (like it exists for >> reg, dma), because then I would have preferred to used: >> interrupts = <...>, <...>, …; >> interrupt-name = “dma0”, “dma1”, ..., “dma10”, “dmashared”, “dmaall”; >> with something like this we could probably have avoided both properties >> and just added a legacy mapping >> This would require some changes to the irq framework (which I wanted >> to avoid) I have posted a patch based on this approach (after having found out that it is possible with the current framework using platform_get_irq_byname). Rob Herring has "Acked" the documentation patch: [PATCH 1/3] dt/bindings: bcm2835: add interrupt-names property Is this approach acceptable for you as well, so that we can try to get it merged? Thanks, Martin
On Tue, Mar 22, 2016 at 10:23:45AM +0100, Martin Sperl wrote: > Hi Mark! Hi Martin, Apologies for having gone silent on this. > On 11.03.2016 09:51, Martin Sperl wrote: > >>On 10.03.2016, at 09:57, Mark Rutland <mark.rutland@arm.com> wrote: > >> > >>Alternative options that have been considered: > >>* there is unfortunately no “interrupt-names” property (like it exists for > >> reg, dma), because then I would have preferred to used: > >> interrupts = <...>, <...>, …; > >> interrupt-name = “dma0”, “dma1”, ..., “dma10”, “dmashared”, “dmaall”; > >> with something like this we could probably have avoided both properties > >> and just added a legacy mapping > >> This would require some changes to the irq framework (which I wanted > >> to avoid) > > I have posted a patch based on this approach (after having found out > that it is possible > with the current framework using platform_get_irq_byname). > > Rob Herring has "Acked" the documentation patch: > [PATCH 1/3] dt/bindings: bcm2835: add interrupt-names property > > Is this approach acceptable for you as well, so that we can try to > get it merged? That approach looks good to me too. One minor nit: please explicitly describe the "dma-shared-all" interrupt-name in the description of interrupt-names. Otherwise, for the binding and the code (which I retains support for existing DTs): Acked-by: Mark Rutland <mark.rutland@arm.com> Thanks, Mark.
diff --git a/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt b/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt index 1396078..f9e84ee 100644 --- a/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt +++ b/Documentation/devicetree/bindings/dma/brcm,bcm2835-dma.txt @@ -17,6 +17,10 @@ Required properties: - brcm,dma-channel-mask: Bit mask representing the channels not used by the firmware in ascending order, i.e. first channel corresponds to LSB. +- brcm,dma-channel-shared-mask: Bit mask representing the channels + that use a shared interrupt +- brcm,dma-shared-irq-index: index of which of the interrupts mentioned + above is the shared interrupt Example: @@ -39,6 +43,8 @@ dma: dma@7e007000 { #dma-cells = <1>; brcm,dma-channel-mask = <0x7f35>; + brcm,dma-channel-shared-mask = <0x0780>; + brcm,dma-shared-irq-index = <11>; }; DMA clients connected to the BCM2835 DMA controller must use the format