[PATCH] spi: pl022: Add OF binding to disable DMA usage

[PATCH] spi: pl022: Add OF binding to disable DMA usage

[Alexander Sverdlin]

Legacy platform instantiation of PL022 had an ability to configure DMA
usage on controller level. If PL022 is being instantiated from DT it still
claims couple of DMA channels capable of DMA_SLAVE unconditionally even if
there are no DMA channels specified in the DT.

Depending on the slave devices' configuration this might be waste of DMA
channels or this might even claim some precious DMA channels if there are
only few of them in the system.

Add a new boolean property to disable DMA usage on the controller level:
	"pl022,dma-disable"

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 Documentation/devicetree/bindings/spi/spi_pl022.txt | 1 +
 drivers/spi/spi-pl022.c                             | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/spi/spi_pl022.txt b/Documentation/devicetree/bindings/spi/spi_pl022.txt
index 7638b4968ddb..d877a0871a11 100644
--- a/Documentation/devicetree/bindings/spi/spi_pl022.txt
+++ b/Documentation/devicetree/bindings/spi/spi_pl022.txt
@@ -21,6 +21,7 @@ Optional properties:
 - dma-names: Names for the dma channels, if present. There must be at
 	     least one channel named "tx" for transmit and named "rx" for
              receive.
+- pl022,dma-disable : disable DMA usage
 
 
 SPI slave nodes must be children of the SPI master node and can
diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index 1af8c96b940e..43039c7428fe 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -2086,7 +2086,7 @@ pl022_platform_data_dt_get(struct device *dev)
 		return NULL;
 
 	pd->bus_id = -1;
-	pd->enable_dma = 1;
+	pd->enable_dma = !of_property_read_bool(np, "pl022,dma-disable");
 	of_property_read_u32(np, "num-cs", &tmp);
 	pd->num_chipselect = tmp;
 	of_property_read_u32(np, "pl022,autosuspend-delay",
-- 
2.18.0

Re: [PATCH] spi: pl022: Add OF binding to disable DMA usage

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]

On Tue, Jul 24, 2018 at 01:27:53PM +0200, Alexander Sverdlin wrote:

> Legacy platform instantiation of PL022 had an ability to configure DMA
> usage on controller level. If PL022 is being instantiated from DT it still
> claims couple of DMA channels capable of DMA_SLAVE unconditionally even if
> there are no DMA channels specified in the DT.

> Depending on the slave devices' configuration this might be waste of DMA
> channels or this might even claim some precious DMA channels if there are
> only few of them in the system.

Hrm.  This makes sense as an expedient solution for constrained systems
however I'm wondering if it's a good idea to bake it into the ABI like
this or if we shouldn't instead be looking at improving the driver to
work better in systems with limited channels, for example by only
claiming the channels when it's active (since it can fall back to PIO if
it doesn't get them).  That might be too heavyweight though, possibly
even impact performance for systems that do have abundant channels and
could interfere with other devices that aren't so able to do the
fallback stuff.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: pl022: Add OF binding to disable DMA usage

[Alexander Sverdlin]

Hello Mark,

On 25/07/18 19:06, Mark Brown wrote:
>> Legacy platform instantiation of PL022 had an ability to configure DMA
>> usage on controller level. If PL022 is being instantiated from DT it still
>> claims couple of DMA channels capable of DMA_SLAVE unconditionally even if
>> there are no DMA channels specified in the DT.
>> Depending on the slave devices' configuration this might be waste of DMA
>> channels or this might even claim some precious DMA channels if there are
>> only few of them in the system.
> Hrm.  This makes sense as an expedient solution for constrained systems
> however I'm wondering if it's a good idea to bake it into the ABI like
> this or if we shouldn't instead be looking at improving the driver to
> work better in systems with limited channels, for example by only
> claiming the channels when it's active (since it can fall back to PIO if
> it doesn't get them).  That might be too heavyweight though, possibly
> even impact performance for systems that do have abundant channels and
> could interfere with other devices that aren't so able to do the
> fallback stuff.

yes, this is an option as well, but at the time we need to take this decision
the bus scan has not yet been performed. We could scan all the devices in
the DT and check if any of them requires DMA. This would mean, that
probe() of the PL022 driver will include similar code to the bus scan
that anyway will happen later just to take the decision on DMA usage.

But I'm fine with that. If this sounds better than new boolean DT binding,
I'll send another patch.

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH] spi: pl022: Add OF binding to disable DMA usage

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]

On Thu, Jul 26, 2018 at 11:04:24AM +0200, Alexander Sverdlin wrote:
> On 25/07/18 19:06, Mark Brown wrote:

> > this or if we shouldn't instead be looking at improving the driver to
> > work better in systems with limited channels, for example by only
> > claiming the channels when it's active (since it can fall back to PIO if
> > it doesn't get them).  That might be too heavyweight though, possibly

> yes, this is an option as well, but at the time we need to take this decision
> the bus scan has not yet been performed. We could scan all the devices in
> the DT and check if any of them requires DMA. This would mean, that
> probe() of the PL022 driver will include similar code to the bus scan
> that anyway will happen later just to take the decision on DMA usage.

> But I'm fine with that. If this sounds better than new boolean DT binding,
> I'll send another patch.

I'm not 100% clear I follow what you mean by bus scan here but I *think*
that sounds about right.  The channel request/release could be factored
out into helper functions to minimize duplication.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: pl022: Add OF binding to disable DMA usage

[Alexander Sverdlin]

Hello Mark,

On 01/08/18 12:39, Mark Brown wrote:
>>> this or if we shouldn't instead be looking at improving the driver to
>>> work better in systems with limited channels, for example by only
>>> claiming the channels when it's active (since it can fall back to PIO if
>>> it doesn't get them).  That might be too heavyweight though, possibly
>> yes, this is an option as well, but at the time we need to take this decision
>> the bus scan has not yet been performed. We could scan all the devices in
>> the DT and check if any of them requires DMA. This would mean, that
>> probe() of the PL022 driver will include similar code to the bus scan
>> that anyway will happen later just to take the decision on DMA usage.
>> But I'm fine with that. If this sounds better than new boolean DT binding,
>> I'll send another patch.
> I'm not 100% clear I follow what you mean by bus scan here but I *think*
> that sounds about right.  The channel request/release could be factored
> out into helper functions to minimize duplication.

Rob has an opinion that the driver should only claim the channels explicitly
specified in DT. This sounds about right, but this would be behavioral change.
What do you think?

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH] spi: pl022: Add OF binding to disable DMA usage

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 708 bytes --]

On Wed, Aug 01, 2018 at 12:47:01PM +0200, Alexander Sverdlin wrote:
> On 01/08/18 12:39, Mark Brown wrote:

> > I'm not 100% clear I follow what you mean by bus scan here but I *think*
> > that sounds about right.  The channel request/release could be factored
> > out into helper functions to minimize duplication.

> Rob has an opinion that the driver should only claim the channels explicitly
> specified in DT. This sounds about right, but this would be behavioral change.
> What do you think?

If it results in platforms that previously used DMA not using DMA that
doesn't seem like a good idea.  I'd have thought that if we didn't have
DMA configured we'd already not be able to request a channel TBH.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: pl022: Add OF binding to disable DMA usage

[Rob Herring]

On Tue, Jul 24, 2018 at 01:27:53PM +0200, Alexander Sverdlin wrote:
> Legacy platform instantiation of PL022 had an ability to configure DMA
> usage on controller level. If PL022 is being instantiated from DT it still
> claims couple of DMA channels capable of DMA_SLAVE unconditionally even if
> there are no DMA channels specified in the DT.

How does that work without specifying the channels in DT? Perhaps the 
driver should stop doing that. We can whitelist any platforms that need 
current behavior if someone complains.

> 
> Depending on the slave devices' configuration this might be waste of DMA
> channels or this might even claim some precious DMA channels if there are
> only few of them in the system.
> 
> Add a new boolean property to disable DMA usage on the controller level:
> 	"pl022,dma-disable"
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> ---
>  Documentation/devicetree/bindings/spi/spi_pl022.txt | 1 +
>  drivers/spi/spi-pl022.c                             | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)