[2/3] dt-bindings: mtd: atmel-quadspi: add an optional property 'dmacap, memcpy'

Message ID 143542c61ca674d53da4985bbabc142e8e6ebefc.1514087323.git.cyrille.pitchen@wedev4u.fr
State New
Delegated to: Cyrille Pitchen
Headers show
Series
  • mtd: spi-nor: fix DMA-unsafe buffer issue between MTD and SPI
Related show

Commit Message

Cyrille Pitchen Dec. 24, 2017, 4:36 a.m.
The optional 'dmacap,memcpy' DT property tells the Atmel QSPI controller
driver to reserve some DMA channel then to use it to perform DMA
memcpy() during data transfers. This feature relies on the generic
bounce buffer helper from spi-nor.c.

Signed-off-by: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
---
 Documentation/devicetree/bindings/mtd/atmel-quadspi.txt | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Rob Herring Dec. 26, 2017, 11:23 p.m. | #1
On Sun, Dec 24, 2017 at 05:36:05AM +0100, Cyrille Pitchen wrote:
> The optional 'dmacap,memcpy' DT property tells the Atmel QSPI controller
> driver to reserve some DMA channel then to use it to perform DMA
> memcpy() during data transfers. This feature relies on the generic
> bounce buffer helper from spi-nor.c.
> 
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
> ---
>  Documentation/devicetree/bindings/mtd/atmel-quadspi.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/atmel-quadspi.txt b/Documentation/devicetree/bindings/mtd/atmel-quadspi.txt
> index b93c1e2f25dd..002d3f0a445b 100644
> --- a/Documentation/devicetree/bindings/mtd/atmel-quadspi.txt
> +++ b/Documentation/devicetree/bindings/mtd/atmel-quadspi.txt
> @@ -12,6 +12,10 @@ Required properties:
>  - #address-cells: Should be <1>.
>  - #size-cells:    Should be <0>.
>  
> +Optional properties:
> +- dmacap,memcpy:  Reserve a DMA channel to perform DMA memcpy() between the
> +                  system memory and the QSPI mapped memory.

How is this a h/w property? Why would I not want to always enable DMA if 
possible?

Furthermore, you are reusing a property, but giving it a different 
meaning. The current definition is an indication whether a DMA 
controller supports memcpy operations or not. It is not a flag for 
clients to use memcpy channels.

Why don't you use "dmas" property to point to the DMA controller.

> +
>  Example:
>  
>  spi@f0020000 {
> @@ -24,6 +28,7 @@ spi@f0020000 {
>  	#size-cells = <0>;
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_spi0_default>;
> +	dmacap,memcpy;
>  
>  	m25p80@0 {
>  		...
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Cyrille Pitchen Dec. 27, 2017, 9:40 p.m. | #2
Hi Rob,

+ Ludovic Desroches, maintainer of the DMA controller drivers for AT91 SoCs.

Le 27/12/2017 à 00:23, Rob Herring a écrit :
> On Sun, Dec 24, 2017 at 05:36:05AM +0100, Cyrille Pitchen wrote:
>> The optional 'dmacap,memcpy' DT property tells the Atmel QSPI controller
>> driver to reserve some DMA channel then to use it to perform DMA
>> memcpy() during data transfers. This feature relies on the generic
>> bounce buffer helper from spi-nor.c.
>>
>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
>> ---
>>  Documentation/devicetree/bindings/mtd/atmel-quadspi.txt | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/atmel-quadspi.txt b/Documentation/devicetree/bindings/mtd/atmel-quadspi.txt
>> index b93c1e2f25dd..002d3f0a445b 100644
>> --- a/Documentation/devicetree/bindings/mtd/atmel-quadspi.txt
>> +++ b/Documentation/devicetree/bindings/mtd/atmel-quadspi.txt
>> @@ -12,6 +12,10 @@ Required properties:
>>  - #address-cells: Should be <1>.
>>  - #size-cells:    Should be <0>.
>>  
>> +Optional properties:
>> +- dmacap,memcpy:  Reserve a DMA channel to perform DMA memcpy() between the
>> +                  system memory and the QSPI mapped memory.
> 
> How is this a h/w property? Why would I not want to always enable DMA if 
> possible?

The number of DMA channels is limited for a given SoC. This number may be
lower than the number of enabled controllers (spi, i2c, qspi, aes, sha,
des, sdmmc, usart, ...).

So we use a DT property to explicitly tell the matching drivers to request
and reserved the DMA channels they need. This policy is not driver or even
SoC specific but board specific. It's very common to reserved DMA channels
for the most used or most performance dependent controllers, setting the
relevant properties in the device-tree then restricting remaining
controllers to their PIO mode.

> 
> Furthermore, you are reusing a property, but giving it a different 
> meaning. The current definition is an indication whether a DMA 
> controller supports memcpy operations or not. It is not a flag for 
> clients to use memcpy channels.
>

I don't mind changing the name. I thought it would be better to use some
existing one than creating another. However I was not confident on whether
"dmacap,memcpy" was actually a good candidate for what I do in the DMA
patch for the atmel-quadspi.c driver.

Actually, I was relying on your feedbacks :)

> Why don't you use "dmas" property to point to the DMA controller.

I didn't use the "dmas" property because I thought it would not have been
consistent with how this property is used in all other nodes of the sama5*
device-trees. The flags provided in the dma-cells are designed for "memory
to peripheral" or "peripheral to memory" DMA transfers only.

However, here I need to perform some "memory to memory" transfer: the SPI
NOR flash memory is mapped into the AHB bus of SAMA5D2 SoCs. Besides,
once in Serial Memory Mode, data can no longer be transfered through the
TDR or RDR registers of the QSPI controller, only through its AHB mapped
memory window.

So I cannot configured the DMA channel for "peripheral to memory" or
"memory to peripheral" like for other controllers embedded in the SoC but
only for "memory to memory".

Maybe we could extend the flags supported by the "dmas" property but I
guess it may require some little rework in the at_xdmac_xlate() function
of the at_xdmac.c driver.

Or maybe no change at all is required at the at_xdmac.c driver side: we
just don't care about the provided flags in the "dmas" property, especially
the "peripheral id". They would be ignored anyway when the atmel-quadspi.c
driver later calls dmaengine_prep_dma_memcpy(). So I could simply set the
dma cells to 0 in the device-tree?

Ludovic, what do you think about that ?


Best regards,

Cyrille

> 
>> +
>>  Example:
>>  
>>  spi@f0020000 {
>> @@ -24,6 +28,7 @@ spi@f0020000 {
>>  	#size-cells = <0>;
>>  	pinctrl-names = "default";
>>  	pinctrl-0 = <&pinctrl_spi0_default>;
>> +	dmacap,memcpy;
>>  
>>  	m25p80@0 {
>>  		...
>> -- 
>> 2.11.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Ludovic Desroches Jan. 2, 2018, 10:22 a.m. | #3
Hi Cyrille,

On Wed, Dec 27, 2017 at 10:40:00PM +0100, Cyrille Pitchen wrote:
> Hi Rob,
> 
> + Ludovic Desroches, maintainer of the DMA controller drivers for AT91 SoCs.
> 
> Le 27/12/2017 à 00:23, Rob Herring a écrit :
> > On Sun, Dec 24, 2017 at 05:36:05AM +0100, Cyrille Pitchen wrote:
> >> The optional 'dmacap,memcpy' DT property tells the Atmel QSPI controller
> >> driver to reserve some DMA channel then to use it to perform DMA
> >> memcpy() during data transfers. This feature relies on the generic
> >> bounce buffer helper from spi-nor.c.
> >>
> >> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
> >> ---
> >>  Documentation/devicetree/bindings/mtd/atmel-quadspi.txt | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/mtd/atmel-quadspi.txt b/Documentation/devicetree/bindings/mtd/atmel-quadspi.txt
> >> index b93c1e2f25dd..002d3f0a445b 100644
> >> --- a/Documentation/devicetree/bindings/mtd/atmel-quadspi.txt
> >> +++ b/Documentation/devicetree/bindings/mtd/atmel-quadspi.txt
> >> @@ -12,6 +12,10 @@ Required properties:
> >>  - #address-cells: Should be <1>.
> >>  - #size-cells:    Should be <0>.
> >>  
> >> +Optional properties:
> >> +- dmacap,memcpy:  Reserve a DMA channel to perform DMA memcpy() between the
> >> +                  system memory and the QSPI mapped memory.
> > 
> > How is this a h/w property? Why would I not want to always enable DMA if 
> > possible?
> 
> The number of DMA channels is limited for a given SoC. This number may be
> lower than the number of enabled controllers (spi, i2c, qspi, aes, sha,
> des, sdmmc, usart, ...).
> 
> So we use a DT property to explicitly tell the matching drivers to request
> and reserved the DMA channels they need. This policy is not driver or even
> SoC specific but board specific. It's very common to reserved DMA channels
> for the most used or most performance dependent controllers, setting the
> relevant properties in the device-tree then restricting remaining
> controllers to their PIO mode.
> 
> > 
> > Furthermore, you are reusing a property, but giving it a different 
> > meaning. The current definition is an indication whether a DMA 
> > controller supports memcpy operations or not. It is not a flag for 
> > clients to use memcpy channels.
> >
> 
> I don't mind changing the name. I thought it would be better to use some
> existing one than creating another. However I was not confident on whether
> "dmacap,memcpy" was actually a good candidate for what I do in the DMA
> patch for the atmel-quadspi.c driver.
> 
> Actually, I was relying on your feedbacks :)
> 
> > Why don't you use "dmas" property to point to the DMA controller.
> 
> I didn't use the "dmas" property because I thought it would not have been
> consistent with how this property is used in all other nodes of the sama5*
> device-trees. The flags provided in the dma-cells are designed for "memory
> to peripheral" or "peripheral to memory" DMA transfers only.
> 
> However, here I need to perform some "memory to memory" transfer: the SPI
> NOR flash memory is mapped into the AHB bus of SAMA5D2 SoCs. Besides,
> once in Serial Memory Mode, data can no longer be transfered through the
> TDR or RDR registers of the QSPI controller, only through its AHB mapped
> memory window.
> 
> So I cannot configured the DMA channel for "peripheral to memory" or
> "memory to peripheral" like for other controllers embedded in the SoC but
> only for "memory to memory".
> 
> Maybe we could extend the flags supported by the "dmas" property but I
> guess it may require some little rework in the at_xdmac_xlate() function
> of the at_xdmac.c driver.
> 

I have no objection about changing the at_xdmac_xlate() behavior.

From your cover-letter, I understand that the issue you're trying to
solve is not only related to the Atmel devices so you may also have to update
the xlate functions of the other DMA controllers.

> Or maybe no change at all is required at the at_xdmac.c driver side: we
> just don't care about the provided flags in the "dmas" property, especially
> the "peripheral id". They would be ignored anyway when the atmel-quadspi.c
> driver later calls dmaengine_prep_dma_memcpy(). So I could simply set the
> dma cells to 0 in the device-tree?
> 
> Ludovic, what do you think about that ?

It may work but I won't do this. Usually, channels requested through the xlate
function have usually their capaiblities set to DMA_SLAVE and not DMA_MEMCPY.
In the at_xdmac case, it won't be an issue but if you have a controller
which has channels which can support only mem-to-mem or peripheral, it
won't work.

Regards

Ludovic
Trent Piepho Jan. 2, 2018, 7:18 p.m. | #4
On Tue, 2018-01-02 at 11:22 +0100, Ludovic Desroches wrote:
> On Wed, Dec 27, 2017 at 10:40:00PM +0100, Cyrille Pitchen wrote:
> 
> > Or maybe no change at all is required at the at_xdmac.c driver side: we
> > just don't care about the provided flags in the "dmas" property, especially
> > the "peripheral id". They would be ignored anyway when the atmel-quadspi.c
> > driver later calls dmaengine_prep_dma_memcpy(). So I could simply set the
> > dma cells to 0 in the device-tree?
> > 
> > Ludovic, what do you think about that ?
> 
> It may work but I won't do this. Usually, channels requested through the xlate
> function have usually their capaiblities set to DMA_SLAVE and not DMA_MEMCPY.
> In the at_xdmac case, it won't be an issue but if you have a controller
> which has channels which can support only mem-to-mem or peripheral, it
> won't work.

Maybe one could create an "AT91_XDMAC_DT_" macro to indicate a memcpy
channel.  There are still unused bits for another flag.  It also looks
like at_xdma uses peripheral id 0x3f for memcpy transfers (will that
work with memcpy DMA on multiple channels at the same time?).  So
perhaps perid 0x3f could be the indication of wanting a memcpy channel,
rather than another flag bit.  But however it's done, one writes:

dmas = <&dma0 AT91_XDMAC_DT_MEMCPY>; dma-names = "rx-tx";

I think one could have the quadspi driver automatically fill in the dma
cell in the dma specifier if it is not present in the device tree.  So
one could write "dmas = <&dma0>" and the driver adds the
AT91_XDMAC_DT_MEMCPY cell before xlating.  I'm not sure if that's a
good idea or not.
Ludovic Desroches Jan. 3, 2018, 6:51 a.m. | #5
On Tue, Jan 02, 2018 at 07:18:58PM +0000, Trent Piepho wrote:
> On Tue, 2018-01-02 at 11:22 +0100, Ludovic Desroches wrote:
> > On Wed, Dec 27, 2017 at 10:40:00PM +0100, Cyrille Pitchen wrote:
> > 
> > > Or maybe no change at all is required at the at_xdmac.c driver side: we
> > > just don't care about the provided flags in the "dmas" property, especially
> > > the "peripheral id". They would be ignored anyway when the atmel-quadspi.c
> > > driver later calls dmaengine_prep_dma_memcpy(). So I could simply set the
> > > dma cells to 0 in the device-tree?
> > > 
> > > Ludovic, what do you think about that ?
> > 
> > It may work but I won't do this. Usually, channels requested through the xlate
> > function have usually their capaiblities set to DMA_SLAVE and not DMA_MEMCPY.
> > In the at_xdmac case, it won't be an issue but if you have a controller
> > which has channels which can support only mem-to-mem or peripheral, it
> > won't work.
> 
> Maybe one could create an "AT91_XDMAC_DT_" macro to indicate a memcpy
> channel.  There are still unused bits for another flag.  It also looks
> like at_xdma uses peripheral id 0x3f for memcpy transfers (will that
> work with memcpy DMA on multiple channels at the same time?).  So
> perhaps perid 0x3f could be the indication of wanting a memcpy channel,
> rather than another flag bit.  But however it's done, one writes:
> 
> dmas = <&dma0 AT91_XDMAC_DT_MEMCPY>; dma-names = "rx-tx";
> 

If have no objection about doing that, my concerns are:
- most (all ?) of the dma controllers used the xlate function to provide
  slave channel. Does it have to provide slave channel or can we
  use it for all kind of channel? From my point of view, we can do it,
  just need the confirmation.
- this set of patches if focused on the atmel qspi controller but other
  ones may be interested in doing the same thing so they would have to
  update the behavior of the xlate function of the DMA controller they
  are using. So having the request of a DMA_MEMCPY channel inside the
  spi/qspi controller doesn't seem to be a wrong idea. Moreover, it may
  be confusing for the user who don't know the context: why do I have to
  use memcpy and not slave as usal?

Honestly I have no opinion about the way to do it. Both have pros and
cons.

> I think one could have the quadspi driver automatically fill in the dma
> cell in the dma specifier if it is not present in the device tree.  So
> one could write "dmas = <&dma0>" and the driver adds the
> AT91_XDMAC_DT_MEMCPY cell before xlating.  I'm not sure if that's a
> good idea or not.

I don't think so, there is enough black magic, let's try to not add more
:p

Regards

Ludovic
Mark Brown Jan. 3, 2018, 11:51 a.m. | #6
On Wed, Dec 27, 2017 at 10:40:00PM +0100, Cyrille Pitchen wrote:
> Le 27/12/2017 à 00:23, Rob Herring a écrit :
> > On Sun, Dec 24, 2017 at 05:36:05AM +0100, Cyrille Pitchen wrote:

> >> +Optional properties:
> >> +- dmacap,memcpy:  Reserve a DMA channel to perform DMA memcpy() between the
> >> +                  system memory and the QSPI mapped memory.

> > How is this a h/w property? Why would I not want to always enable DMA if 
> > possible?

> The number of DMA channels is limited for a given SoC. This number may be
> lower than the number of enabled controllers (spi, i2c, qspi, aes, sha,
> des, sdmmc, usart, ...).

> So we use a DT property to explicitly tell the matching drivers to request
> and reserved the DMA channels they need. This policy is not driver or even
> SoC specific but board specific. It's very common to reserved DMA channels
> for the most used or most performance dependent controllers, setting the
> relevant properties in the device-tree then restricting remaining
> controllers to their PIO mode.

Why can't we just time share the DMA channels at runtime, why do we need
this static allocation?

Patch

diff --git a/Documentation/devicetree/bindings/mtd/atmel-quadspi.txt b/Documentation/devicetree/bindings/mtd/atmel-quadspi.txt
index b93c1e2f25dd..002d3f0a445b 100644
--- a/Documentation/devicetree/bindings/mtd/atmel-quadspi.txt
+++ b/Documentation/devicetree/bindings/mtd/atmel-quadspi.txt
@@ -12,6 +12,10 @@  Required properties:
 - #address-cells: Should be <1>.
 - #size-cells:    Should be <0>.
 
+Optional properties:
+- dmacap,memcpy:  Reserve a DMA channel to perform DMA memcpy() between the
+                  system memory and the QSPI mapped memory.
+
 Example:
 
 spi@f0020000 {
@@ -24,6 +28,7 @@  spi@f0020000 {
 	#size-cells = <0>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_spi0_default>;
+	dmacap,memcpy;
 
 	m25p80@0 {
 		...