From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm0-x22f.google.com ([2a00:1450:400c:c09::22f]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1cjMhn-0004S0-9I for linux-mtd@lists.infradead.org; Thu, 02 Mar 2017 09:07:25 +0000 Received: by mail-wm0-x22f.google.com with SMTP id i17so1448022wmf.0 for ; Thu, 02 Mar 2017 01:07:02 -0800 (PST) Subject: Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce buffer support To: Boris Brezillon , Vignesh R References: <20170227120839.16545-1-vigneshr@ti.com> <20170227120839.16545-3-vigneshr@ti.com> <8f999a27-c3ce-2650-452c-b21c3e44989d@ti.com> <20170301175506.202cb478@bbrezillon> Cc: Cyrille Pitchen , Richard Weinberger , David Woodhouse , Brian Norris , Marek Vasut , linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org, linux-spi@vger.kernel.org From: Frode Isaksen Message-ID: <09ffe06d-565d-afe8-8b7d-d1a0b575595b@baylibre.com> Date: Thu, 2 Mar 2017 10:06:59 +0100 MIME-Version: 1.0 In-Reply-To: <20170301175506.202cb478@bbrezillon> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On 01/03/2017 17:55, Boris Brezillon wrote: > On Wed, 1 Mar 2017 17:16:30 +0530 > Vignesh R wrote: > >> On Wednesday 01 March 2017 04:13 PM, Cyrille Pitchen wrote: >>> Le 01/03/2017 à 05:54, Vignesh R a écrit : >>>> >>>> On Wednesday 01 March 2017 03:11 AM, Richard Weinberger wrote: >>>>> Vignesh, >>>>> >>>>> Am 27.02.2017 um 13:08 schrieb Vignesh R: >>>>>> Many SPI controller drivers use DMA to read/write from m25p80 compatible >>>>>> flashes. Therefore enable bounce buffers support provided by spi-nor >>>>>> framework to take care of handling vmalloc'd buffers which may not be >>>>>> DMA'able. >>>>>> >>>>>> Signed-off-by: Vignesh R >>>>>> --- >>>>>> drivers/mtd/devices/m25p80.c | 1 + >>>>>> 1 file changed, 1 insertion(+) >>>>>> >>>>>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c >>>>>> index c4df3b1bded0..d05acf22eadf 100644 >>>>>> --- a/drivers/mtd/devices/m25p80.c >>>>>> +++ b/drivers/mtd/devices/m25p80.c >>>>>> @@ -241,6 +241,7 @@ static int m25p_probe(struct spi_device *spi) >>>>>> else >>>>>> flash_name = spi->modalias; >>>>>> >>>>>> + nor->flags |= SNOR_F_USE_BOUNCE_BUFFER; >>>>> Isn't there a better way to detect whether a bounce buffer is needed or not? >>>> >>> I agree with Richard: the bounce buffer should be enabled only if needed >>> by the SPI controller. >>> >>>> Yes, I can poke the spi->master struct to see of dma channels are >>>> populated and request SNOR_F_USE_BOUNCE_BUFFER accordingly: >>>> >>>> - nor->flags |= SNOR_F_USE_BOUNCE_BUFFER; >>>> + if (spi->master->dma_tx || spi->master->dma_rx) >>>> + nor->flags |= SNOR_F_USE_BOUNCE_BUFFER; >>>> + >>>> >>> However I don't agree with this solution: master->dma_{tx|rx} can be set >>> for SPI controllers which already rely on spi_map_msg() to handle >>> vmalloc'ed memory during DMA transfers. >>> Such SPI controllers don't need the spi-nor bounce buffer. >>> >>> spi_map_msg() can build a scatter-gather list from vmalloc'ed buffer >>> then map this sg list with dma_map_sg(). AFAIK, It is safe to do so for >>> architectures using PIPT caches since the possible cache aliases issue >>> present for VIPT or VIVT caches is always avoided for PIPT caches. >>> >>> For instance, the drivers/spi/spi-atmel.c driver relies on spi_map_sg() >>> to be called from the SPI sub-system to handle vmalloc'ed buffers and >>> both master->dma_tx and master->dma_rx are set by the this driver. >>> >>> >>> By the way, Is there any case where the same physical page is actually >>> mapped into two different virtual addresses for the buffers allocated by >>> the MTD sub-system? Because for a long time now I wonder whether the >>> cache aliases issue is a real or only theoretical issue but I have no >>> answer to that question. >>> >> I have atleast one evidence of VIVT aliasing causing problem. Please see >> this thread on DMA issues with davinci-spi driver >> https://www.spinics.net/lists/arm-kernel/msg563420.html >> https://www.spinics.net/lists/arm-kernel/msg563445.html >> >>> Then my next question: is spi_map_msg() enough in every case, even with >>> VIPT or VIVT caches? >>> >> Not really, I am debugging another issue with UBIFS on DRA74 EVM (ARM >> cortex-a15) wherein pages allocated by vmalloc are in highmem region >> that are not addressable using 32 bit addresses and is backed by LPAE. >> So, a 32 bit DMA cannot access these buffers at all. >> When dma_map_sg() is called to map these pages by spi_map_buf() the >> physical address is just truncated to 32 bit in pfn_to_dma() (as part of >> dma_map_sg() call). This results in random crashes as DMA starts >> accessing random memory during SPI read. >> >> IMO, there may be more undiscovered caveat with using dma_map_sg() for >> non kmalloc'd buffers and its better that spi-nor starts handling these >> buffers instead of relying on spi_map_msg() and working around every >> time something pops up. >> > Ok, I had a closer look at the SPI framework, and it seems there's a > way to tell to the core that a specific transfer cannot use DMA > (->can_dam()). The first thing you should do is fix the spi-davinci > driver: > > 1/ implement ->can_dma() > 2/ patch davinci_spi_bufs() to take the decision to do DMA or not on a > per-xfer basis and not on a per-device basis > > Then we can start thinking about how to improve perfs by using a bounce > buffer for large transfers, but I'm still not sure this should be done > at the MTD level... This has already been done, see http://lists.infradead.org/pipermail/linux-arm-kernel/2017-February/489761.html. I return false for can_dma() if the rx or tx buffer is a vmalloc'ed one. In that case the transfer gos back to PIO and you loose performance, but no data corruption. Thanks, Frode From mboxrd@z Thu Jan 1 00:00:00 1970 From: Frode Isaksen Subject: Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce buffer support Date: Thu, 2 Mar 2017 10:06:59 +0100 Message-ID: <09ffe06d-565d-afe8-8b7d-d1a0b575595b@baylibre.com> References: <20170227120839.16545-1-vigneshr@ti.com> <20170227120839.16545-3-vigneshr@ti.com> <8f999a27-c3ce-2650-452c-b21c3e44989d@ti.com> <20170301175506.202cb478@bbrezillon> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20170301175506.202cb478@bbrezillon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-mtd" Errors-To: linux-mtd-bounces+gldm-linux-mtd-36=gmane.org@lists.infradead.org To: Boris Brezillon , Vignesh R Cc: linux-omap@vger.kernel.org, Richard Weinberger , linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org, Marek Vasut , linux-mtd@lists.infradead.org, Cyrille Pitchen , Brian Norris , David Woodhouse List-Id: linux-omap@vger.kernel.org CgpPbiAwMS8wMy8yMDE3IDE3OjU1LCBCb3JpcyBCcmV6aWxsb24gd3JvdGU6Cj4gT24gV2VkLCAx IE1hciAyMDE3IDE3OjE2OjMwICswNTMwCj4gVmlnbmVzaCBSIDx2aWduZXNockB0aS5jb20+IHdy b3RlOgo+Cj4+IE9uIFdlZG5lc2RheSAwMSBNYXJjaCAyMDE3IDA0OjEzIFBNLCBDeXJpbGxlIFBp dGNoZW4gd3JvdGU6Cj4+PiBMZSAwMS8wMy8yMDE3IMOgIDA1OjU0LCBWaWduZXNoIFIgYSDDqWNy aXQgOiAgCj4+Pj4KPj4+PiBPbiBXZWRuZXNkYXkgMDEgTWFyY2ggMjAxNyAwMzoxMSBBTSwgUmlj aGFyZCBXZWluYmVyZ2VyIHdyb3RlOiAgCj4+Pj4+IFZpZ25lc2gsCj4+Pj4+Cj4+Pj4+IEFtIDI3 LjAyLjIwMTcgdW0gMTM6MDggc2NocmllYiBWaWduZXNoIFI6ICAKPj4+Pj4+IE1hbnkgU1BJIGNv bnRyb2xsZXIgZHJpdmVycyB1c2UgRE1BIHRvIHJlYWQvd3JpdGUgZnJvbSBtMjVwODAgY29tcGF0 aWJsZQo+Pj4+Pj4gZmxhc2hlcy4gVGhlcmVmb3JlIGVuYWJsZSBib3VuY2UgYnVmZmVycyBzdXBw b3J0IHByb3ZpZGVkIGJ5IHNwaS1ub3IKPj4+Pj4+IGZyYW1ld29yayB0byB0YWtlIGNhcmUgb2Yg aGFuZGxpbmcgdm1hbGxvYydkIGJ1ZmZlcnMgd2hpY2ggbWF5IG5vdCBiZQo+Pj4+Pj4gRE1BJ2Fi bGUuCj4+Pj4+Pgo+Pj4+Pj4gU2lnbmVkLW9mZi1ieTogVmlnbmVzaCBSIDx2aWduZXNockB0aS5j b20+Cj4+Pj4+PiAtLS0KPj4+Pj4+ICBkcml2ZXJzL210ZC9kZXZpY2VzL20yNXA4MC5jIHwgMSAr Cj4+Pj4+PiAgMSBmaWxlIGNoYW5nZWQsIDEgaW5zZXJ0aW9uKCspCj4+Pj4+Pgo+Pj4+Pj4gZGlm ZiAtLWdpdCBhL2RyaXZlcnMvbXRkL2RldmljZXMvbTI1cDgwLmMgYi9kcml2ZXJzL210ZC9kZXZp Y2VzL20yNXA4MC5jCj4+Pj4+PiBpbmRleCBjNGRmM2IxYmRlZDAuLmQwNWFjZjIyZWFkZiAxMDA2 NDQKPj4+Pj4+IC0tLSBhL2RyaXZlcnMvbXRkL2RldmljZXMvbTI1cDgwLmMKPj4+Pj4+ICsrKyBi L2RyaXZlcnMvbXRkL2RldmljZXMvbTI1cDgwLmMKPj4+Pj4+IEBAIC0yNDEsNiArMjQxLDcgQEAg c3RhdGljIGludCBtMjVwX3Byb2JlKHN0cnVjdCBzcGlfZGV2aWNlICpzcGkpCj4+Pj4+PiAgCWVs c2UKPj4+Pj4+ICAJCWZsYXNoX25hbWUgPSBzcGktPm1vZGFsaWFzOwo+Pj4+Pj4gIAo+Pj4+Pj4g Kwlub3ItPmZsYWdzIHw9IFNOT1JfRl9VU0VfQk9VTkNFX0JVRkZFUjsgIAo+Pj4+PiBJc24ndCB0 aGVyZSBhIGJldHRlciB3YXkgdG8gZGV0ZWN0IHdoZXRoZXIgYSBib3VuY2UgYnVmZmVyIGlzIG5l ZWRlZCBvciBub3Q/ICAKPj4+PiAgCj4+PiBJIGFncmVlIHdpdGggUmljaGFyZDogdGhlIGJvdW5j ZSBidWZmZXIgc2hvdWxkIGJlIGVuYWJsZWQgb25seSBpZiBuZWVkZWQKPj4+IGJ5IHRoZSBTUEkg Y29udHJvbGxlci4KPj4+ICAgCj4+Pj4gWWVzLCBJIGNhbiBwb2tlIHRoZSBzcGktPm1hc3RlciBz dHJ1Y3QgdG8gc2VlIG9mIGRtYSBjaGFubmVscyBhcmUKPj4+PiBwb3B1bGF0ZWQgYW5kIHJlcXVl c3QgU05PUl9GX1VTRV9CT1VOQ0VfQlVGRkVSIGFjY29yZGluZ2x5Ogo+Pj4+Cj4+Pj4gLSAgICAg ICBub3ItPmZsYWdzIHw9IFNOT1JfRl9VU0VfQk9VTkNFX0JVRkZFUjsKPj4+PiArICAgICAgIGlm IChzcGktPm1hc3Rlci0+ZG1hX3R4IHx8IHNwaS0+bWFzdGVyLT5kbWFfcngpCj4+Pj4gKyAgICAg ICAgICAgICAgIG5vci0+ZmxhZ3MgfD0gU05PUl9GX1VTRV9CT1VOQ0VfQlVGRkVSOwo+Pj4+ICsK Pj4+PiAgCj4+PiBIb3dldmVyIEkgZG9uJ3QgYWdyZWUgd2l0aCB0aGlzIHNvbHV0aW9uOiBtYXN0 ZXItPmRtYV97dHh8cnh9IGNhbiBiZSBzZXQKPj4+IGZvciBTUEkgY29udHJvbGxlcnMgd2hpY2gg YWxyZWFkeSByZWx5IG9uIHNwaV9tYXBfbXNnKCkgdG8gaGFuZGxlCj4+PiB2bWFsbG9jJ2VkIG1l bW9yeSBkdXJpbmcgRE1BIHRyYW5zZmVycy4KPj4+IFN1Y2ggU1BJIGNvbnRyb2xsZXJzIGRvbid0 IG5lZWQgdGhlIHNwaS1ub3IgYm91bmNlIGJ1ZmZlci4KPj4+Cj4+PiBzcGlfbWFwX21zZygpIGNh biBidWlsZCBhIHNjYXR0ZXItZ2F0aGVyIGxpc3QgZnJvbSB2bWFsbG9jJ2VkIGJ1ZmZlcgo+Pj4g dGhlbiBtYXAgdGhpcyBzZyBsaXN0IHdpdGggZG1hX21hcF9zZygpLiBBRkFJSywgSXQgaXMgc2Fm ZSB0byBkbyBzbyBmb3IKPj4+IGFyY2hpdGVjdHVyZXMgdXNpbmcgUElQVCBjYWNoZXMgc2luY2Ug dGhlIHBvc3NpYmxlIGNhY2hlIGFsaWFzZXMgaXNzdWUKPj4+IHByZXNlbnQgZm9yIFZJUFQgb3Ig VklWVCBjYWNoZXMgaXMgYWx3YXlzIGF2b2lkZWQgZm9yIFBJUFQgY2FjaGVzLgo+Pj4KPj4+IEZv ciBpbnN0YW5jZSwgdGhlIGRyaXZlcnMvc3BpL3NwaS1hdG1lbC5jIGRyaXZlciByZWxpZXMgb24g c3BpX21hcF9zZygpCj4+PiB0byBiZSBjYWxsZWQgZnJvbSB0aGUgU1BJIHN1Yi1zeXN0ZW0gdG8g aGFuZGxlIHZtYWxsb2MnZWQgYnVmZmVycyBhbmQKPj4+IGJvdGggbWFzdGVyLT5kbWFfdHggYW5k IG1hc3Rlci0+ZG1hX3J4IGFyZSBzZXQgYnkgdGhlIHRoaXMgZHJpdmVyLgo+Pj4KPj4+Cj4+PiBC eSB0aGUgd2F5LCBJcyB0aGVyZSBhbnkgY2FzZSB3aGVyZSB0aGUgc2FtZSBwaHlzaWNhbCBwYWdl IGlzIGFjdHVhbGx5Cj4+PiBtYXBwZWQgaW50byB0d28gZGlmZmVyZW50IHZpcnR1YWwgYWRkcmVz c2VzIGZvciB0aGUgYnVmZmVycyBhbGxvY2F0ZWQgYnkKPj4+IHRoZSBNVEQgc3ViLXN5c3RlbT8g QmVjYXVzZSBmb3IgYSBsb25nIHRpbWUgbm93IEkgd29uZGVyIHdoZXRoZXIgdGhlCj4+PiBjYWNo ZSBhbGlhc2VzIGlzc3VlIGlzIGEgcmVhbCBvciBvbmx5IHRoZW9yZXRpY2FsIGlzc3VlIGJ1dCBJ IGhhdmUgbm8KPj4+IGFuc3dlciB0byB0aGF0IHF1ZXN0aW9uLgo+Pj4gICAKPj4gSSBoYXZlIGF0 bGVhc3Qgb25lIGV2aWRlbmNlIG9mIFZJVlQgYWxpYXNpbmcgY2F1c2luZyBwcm9ibGVtLiBQbGVh c2Ugc2VlCj4+IHRoaXMgdGhyZWFkIG9uIERNQSBpc3N1ZXMgd2l0aCBkYXZpbmNpLXNwaSBkcml2 ZXIKPj4gaHR0cHM6Ly93d3cuc3Bpbmljcy5uZXQvbGlzdHMvYXJtLWtlcm5lbC9tc2c1NjM0MjAu aHRtbAo+PiBodHRwczovL3d3dy5zcGluaWNzLm5ldC9saXN0cy9hcm0ta2VybmVsL21zZzU2MzQ0 NS5odG1sCj4+Cj4+PiBUaGVuIG15IG5leHQgcXVlc3Rpb246IGlzIHNwaV9tYXBfbXNnKCkgZW5v dWdoIGluIGV2ZXJ5IGNhc2UsIGV2ZW4gd2l0aAo+Pj4gVklQVCBvciBWSVZUIGNhY2hlcz8KPj4+ ICAgCj4+IE5vdCByZWFsbHksIEkgYW0gZGVidWdnaW5nIGFub3RoZXIgaXNzdWUgd2l0aCBVQklG UyBvbiBEUkE3NCBFVk0gKEFSTQo+PiBjb3J0ZXgtYTE1KSB3aGVyZWluIHBhZ2VzIGFsbG9jYXRl ZCBieSB2bWFsbG9jIGFyZSBpbiBoaWdobWVtIHJlZ2lvbgo+PiB0aGF0IGFyZSBub3QgYWRkcmVz c2FibGUgdXNpbmcgMzIgYml0IGFkZHJlc3NlcyBhbmQgaXMgYmFja2VkIGJ5IExQQUUuCj4+IFNv LCBhIDMyIGJpdCBETUEgY2Fubm90IGFjY2VzcyB0aGVzZSBidWZmZXJzIGF0IGFsbC4KPj4gV2hl biBkbWFfbWFwX3NnKCkgaXMgY2FsbGVkIHRvIG1hcCB0aGVzZSBwYWdlcyBieSBzcGlfbWFwX2J1 ZigpIHRoZQo+PiBwaHlzaWNhbCBhZGRyZXNzIGlzIGp1c3QgdHJ1bmNhdGVkIHRvIDMyIGJpdCBp biBwZm5fdG9fZG1hKCkgKGFzIHBhcnQgb2YKPj4gZG1hX21hcF9zZygpIGNhbGwpLiBUaGlzIHJl c3VsdHMgaW4gcmFuZG9tIGNyYXNoZXMgYXMgRE1BIHN0YXJ0cwo+PiBhY2Nlc3NpbmcgcmFuZG9t IG1lbW9yeSBkdXJpbmcgU1BJIHJlYWQuCj4+Cj4+IElNTywgdGhlcmUgbWF5IGJlIG1vcmUgdW5k aXNjb3ZlcmVkIGNhdmVhdCB3aXRoIHVzaW5nIGRtYV9tYXBfc2coKSBmb3IKPj4gbm9uIGttYWxs b2MnZCBidWZmZXJzIGFuZCBpdHMgYmV0dGVyIHRoYXQgc3BpLW5vciBzdGFydHMgaGFuZGxpbmcg dGhlc2UKPj4gYnVmZmVycyBpbnN0ZWFkIG9mIHJlbHlpbmcgb24gc3BpX21hcF9tc2coKSBhbmQg d29ya2luZyBhcm91bmQgZXZlcnkKPj4gdGltZSBzb21ldGhpbmcgcG9wcyB1cC4KPj4KPiBPaywg SSBoYWQgYSBjbG9zZXIgbG9vayBhdCB0aGUgU1BJIGZyYW1ld29yaywgYW5kIGl0IHNlZW1zIHRo ZXJlJ3MgYQo+IHdheSB0byB0ZWxsIHRvIHRoZSBjb3JlIHRoYXQgYSBzcGVjaWZpYyB0cmFuc2Zl ciBjYW5ub3QgdXNlIERNQQo+ICgtPmNhbl9kYW0oKSkuIFRoZSBmaXJzdCB0aGluZyB5b3Ugc2hv dWxkIGRvIGlzIGZpeCB0aGUgc3BpLWRhdmluY2kKPiBkcml2ZXI6Cj4KPiAxLyBpbXBsZW1lbnQg LT5jYW5fZG1hKCkKPiAyLyBwYXRjaCBkYXZpbmNpX3NwaV9idWZzKCkgdG8gdGFrZSB0aGUgZGVj aXNpb24gdG8gZG8gRE1BIG9yIG5vdCBvbiBhCj4gICAgcGVyLXhmZXIgYmFzaXMgYW5kIG5vdCBv biBhIHBlci1kZXZpY2UgYmFzaXMKPgo+IFRoZW4gd2UgY2FuIHN0YXJ0IHRoaW5raW5nIGFib3V0 IGhvdyB0byBpbXByb3ZlIHBlcmZzIGJ5IHVzaW5nIGEgYm91bmNlCj4gYnVmZmVyIGZvciBsYXJn ZSB0cmFuc2ZlcnMsIGJ1dCBJJ20gc3RpbGwgbm90IHN1cmUgdGhpcyBzaG91bGQgYmUgZG9uZQo+ IGF0IHRoZSBNVEQgbGV2ZWwuLi4KVGhpcyBoYXMgYWxyZWFkeSBiZWVuIGRvbmUsIHNlZSBodHRw Oi8vbGlzdHMuaW5mcmFkZWFkLm9yZy9waXBlcm1haWwvbGludXgtYXJtLWtlcm5lbC8yMDE3LUZl YnJ1YXJ5LzQ4OTc2MS5odG1sLiBJIHJldHVybiBmYWxzZSBmb3IgY2FuX2RtYSgpIGlmIHRoZSBy eCBvciB0eCBidWZmZXIgaXMgYSB2bWFsbG9jJ2VkIG9uZS4KSW4gdGhhdCBjYXNlIHRoZSB0cmFu c2ZlciBnb3MgYmFjayB0byBQSU8gYW5kIHlvdSBsb29zZSBwZXJmb3JtYW5jZSwgYnV0IG5vIGRh dGEgY29ycnVwdGlvbi4KClRoYW5rcywKRnJvZGUKCgpfX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX18KTGludXggTVREIGRpc2N1c3Npb24gbWFpbGlu ZyBsaXN0Cmh0dHA6Ly9saXN0cy5pbmZyYWRlYWQub3JnL21haWxtYW4vbGlzdGluZm8vbGludXgt bXRkLwo=