From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.free-electrons.com ([62.4.15.54]) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1cj6kJ-0005a7-94 for linux-mtd@lists.infradead.org; Wed, 01 Mar 2017 16:04:57 +0000 Date: Wed, 1 Mar 2017 17:04:19 +0100 From: Boris Brezillon To: Vignesh R Cc: Cyrille Pitchen , Richard Weinberger , David Woodhouse , Brian Norris , Marek Vasut , , , , Frode Isaksen , Subject: Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce buffer support Message-ID: <20170301170419.7b6b2f81@bbrezillon> In-Reply-To: References: <20170227120839.16545-1-vigneshr@ti.com> <20170227120839.16545-3-vigneshr@ti.com> <8f999a27-c3ce-2650-452c-b21c3e44989d@ti.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 =C3=A0 05:54, Vignesh R a =C3=A9crit : =20 > >> > >> > >> On Wednesday 01 March 2017 03:11 AM, Richard Weinberger wrote: =20 > >>> Vignesh, > >>> > >>> Am 27.02.2017 um 13:08 schrieb Vignesh R: =20 > >>>> Many SPI controller drivers use DMA to read/write from m25p80 compat= ible > >>>> 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/m25p= 80.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 =3D spi->modalias; > >>>> =20 > >>>> + nor->flags |=3D SNOR_F_USE_BOUNCE_BUFFER; =20 > >>> > >>> Isn't there a better way to detect whether a bounce buffer is needed = or not? =20 > >> =20 > >=20 > > I agree with Richard: the bounce buffer should be enabled only if needed > > by the SPI controller. > > =20 > >> 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 |=3D SNOR_F_USE_BOUNCE_BUFFER; > >> + if (spi->master->dma_tx || spi->master->dma_rx) > >> + nor->flags |=3D SNOR_F_USE_BOUNCE_BUFFER; > >> + > >> =20 > >=20 > > 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. > >=20 > > 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. > >=20 > > 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. > > > >=20 > > 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. > > =20 >=20 > 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 >=20 > > Then my next question: is spi_map_msg() enough in every case, even with > > VIPT or VIVT caches? > > =20 >=20 > 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. >=20 > 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. >=20 Hm, I had a discussion with Cyrille on the spi-nor/m25p80 architecture, and I must admit I don't understand why the decision to use a bounce buffer is done at the m25p80 level. While it make sense to do that for dedicated spi-flash controllers, which can decide by themselves if they need a bounce buffer for non kmalloc-ed buffers, I don't understand why we have to do that for regular SPI controllers. I mean, the same problem can arise for other SPI device drivers if they decide to use vmalloc-ed or kmap-ed buffers, so this should be handled at the SPI level. If the SPI framework does not impose any restriction on the buffers it is passed, it should either take care of this, or ask SPI controller drivers to do so (which implies checking the buffer address directly inside the SPI controller driver and decide to use a bounce buffer or use PIO mode). Am I misunderstanding something here? From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Subject: Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce buffer support Date: Wed, 1 Mar 2017 17:04:19 +0100 Message-ID: <20170301170419.7b6b2f81@bbrezillon> References: <20170227120839.16545-1-vigneshr@ti.com> <20170227120839.16545-3-vigneshr@ti.com> <8f999a27-c3ce-2650-452c-b21c3e44989d@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: 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: Vignesh R Cc: Frode Isaksen , 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 T24gV2VkLCAxIE1hciAyMDE3IDE3OjE2OjMwICswNTMwClZpZ25lc2ggUiA8dmlnbmVzaHJAdGku Y29tPiB3cm90ZToKCj4gT24gV2VkbmVzZGF5IDAxIE1hcmNoIDIwMTcgMDQ6MTMgUE0sIEN5cmls bGUgUGl0Y2hlbiB3cm90ZToKPiA+IExlIDAxLzAzLzIwMTcgw6AgMDU6NTQsIFZpZ25lc2ggUiBh IMOpY3JpdCA6ICAKPiA+Pgo+ID4+Cj4gPj4gT24gV2VkbmVzZGF5IDAxIE1hcmNoIDIwMTcgMDM6 MTEgQU0sIFJpY2hhcmQgV2VpbmJlcmdlciB3cm90ZTogIAo+ID4+PiBWaWduZXNoLAo+ID4+Pgo+ ID4+PiBBbSAyNy4wMi4yMDE3IHVtIDEzOjA4IHNjaHJpZWIgVmlnbmVzaCBSOiAgCj4gPj4+PiBN YW55IFNQSSBjb250cm9sbGVyIGRyaXZlcnMgdXNlIERNQSB0byByZWFkL3dyaXRlIGZyb20gbTI1 cDgwIGNvbXBhdGlibGUKPiA+Pj4+IGZsYXNoZXMuIFRoZXJlZm9yZSBlbmFibGUgYm91bmNlIGJ1 ZmZlcnMgc3VwcG9ydCBwcm92aWRlZCBieSBzcGktbm9yCj4gPj4+PiBmcmFtZXdvcmsgdG8gdGFr ZSBjYXJlIG9mIGhhbmRsaW5nIHZtYWxsb2MnZCBidWZmZXJzIHdoaWNoIG1heSBub3QgYmUKPiA+ Pj4+IERNQSdhYmxlLgo+ID4+Pj4KPiA+Pj4+IFNpZ25lZC1vZmYtYnk6IFZpZ25lc2ggUiA8dmln bmVzaHJAdGkuY29tPgo+ID4+Pj4gLS0tCj4gPj4+PiAgZHJpdmVycy9tdGQvZGV2aWNlcy9tMjVw ODAuYyB8IDEgKwo+ID4+Pj4gIDEgZmlsZSBjaGFuZ2VkLCAxIGluc2VydGlvbigrKQo+ID4+Pj4K PiA+Pj4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL210ZC9kZXZpY2VzL20yNXA4MC5jIGIvZHJpdmVy cy9tdGQvZGV2aWNlcy9tMjVwODAuYwo+ID4+Pj4gaW5kZXggYzRkZjNiMWJkZWQwLi5kMDVhY2Yy MmVhZGYgMTAwNjQ0Cj4gPj4+PiAtLS0gYS9kcml2ZXJzL210ZC9kZXZpY2VzL20yNXA4MC5jCj4g Pj4+PiArKysgYi9kcml2ZXJzL210ZC9kZXZpY2VzL20yNXA4MC5jCj4gPj4+PiBAQCAtMjQxLDYg KzI0MSw3IEBAIHN0YXRpYyBpbnQgbTI1cF9wcm9iZShzdHJ1Y3Qgc3BpX2RldmljZSAqc3BpKQo+ ID4+Pj4gIAllbHNlCj4gPj4+PiAgCQlmbGFzaF9uYW1lID0gc3BpLT5tb2RhbGlhczsKPiA+Pj4+ ICAKPiA+Pj4+ICsJbm9yLT5mbGFncyB8PSBTTk9SX0ZfVVNFX0JPVU5DRV9CVUZGRVI7ICAKPiA+ Pj4KPiA+Pj4gSXNuJ3QgdGhlcmUgYSBiZXR0ZXIgd2F5IHRvIGRldGVjdCB3aGV0aGVyIGEgYm91 bmNlIGJ1ZmZlciBpcyBuZWVkZWQgb3Igbm90PyAgCj4gPj4gIAo+ID4gCj4gPiBJIGFncmVlIHdp dGggUmljaGFyZDogdGhlIGJvdW5jZSBidWZmZXIgc2hvdWxkIGJlIGVuYWJsZWQgb25seSBpZiBu ZWVkZWQKPiA+IGJ5IHRoZSBTUEkgY29udHJvbGxlci4KPiA+ICAgCj4gPj4gWWVzLCBJIGNhbiBw b2tlIHRoZSBzcGktPm1hc3RlciBzdHJ1Y3QgdG8gc2VlIG9mIGRtYSBjaGFubmVscyBhcmUKPiA+ PiBwb3B1bGF0ZWQgYW5kIHJlcXVlc3QgU05PUl9GX1VTRV9CT1VOQ0VfQlVGRkVSIGFjY29yZGlu Z2x5Ogo+ID4+Cj4gPj4gLSAgICAgICBub3ItPmZsYWdzIHw9IFNOT1JfRl9VU0VfQk9VTkNFX0JV RkZFUjsKPiA+PiArICAgICAgIGlmIChzcGktPm1hc3Rlci0+ZG1hX3R4IHx8IHNwaS0+bWFzdGVy LT5kbWFfcngpCj4gPj4gKyAgICAgICAgICAgICAgIG5vci0+ZmxhZ3MgfD0gU05PUl9GX1VTRV9C T1VOQ0VfQlVGRkVSOwo+ID4+ICsKPiA+PiAgCj4gPiAKPiA+IEhvd2V2ZXIgSSBkb24ndCBhZ3Jl ZSB3aXRoIHRoaXMgc29sdXRpb246IG1hc3Rlci0+ZG1hX3t0eHxyeH0gY2FuIGJlIHNldAo+ID4g Zm9yIFNQSSBjb250cm9sbGVycyB3aGljaCBhbHJlYWR5IHJlbHkgb24gc3BpX21hcF9tc2coKSB0 byBoYW5kbGUKPiA+IHZtYWxsb2MnZWQgbWVtb3J5IGR1cmluZyBETUEgdHJhbnNmZXJzLgo+ID4g U3VjaCBTUEkgY29udHJvbGxlcnMgZG9uJ3QgbmVlZCB0aGUgc3BpLW5vciBib3VuY2UgYnVmZmVy Lgo+ID4gCj4gPiBzcGlfbWFwX21zZygpIGNhbiBidWlsZCBhIHNjYXR0ZXItZ2F0aGVyIGxpc3Qg ZnJvbSB2bWFsbG9jJ2VkIGJ1ZmZlcgo+ID4gdGhlbiBtYXAgdGhpcyBzZyBsaXN0IHdpdGggZG1h X21hcF9zZygpLiBBRkFJSywgSXQgaXMgc2FmZSB0byBkbyBzbyBmb3IKPiA+IGFyY2hpdGVjdHVy ZXMgdXNpbmcgUElQVCBjYWNoZXMgc2luY2UgdGhlIHBvc3NpYmxlIGNhY2hlIGFsaWFzZXMgaXNz dWUKPiA+IHByZXNlbnQgZm9yIFZJUFQgb3IgVklWVCBjYWNoZXMgaXMgYWx3YXlzIGF2b2lkZWQg Zm9yIFBJUFQgY2FjaGVzLgo+ID4gCj4gPiBGb3IgaW5zdGFuY2UsIHRoZSBkcml2ZXJzL3NwaS9z cGktYXRtZWwuYyBkcml2ZXIgcmVsaWVzIG9uIHNwaV9tYXBfc2coKQo+ID4gdG8gYmUgY2FsbGVk IGZyb20gdGhlIFNQSSBzdWItc3lzdGVtIHRvIGhhbmRsZSB2bWFsbG9jJ2VkIGJ1ZmZlcnMgYW5k Cj4gPiBib3RoIG1hc3Rlci0+ZG1hX3R4IGFuZCBtYXN0ZXItPmRtYV9yeCBhcmUgc2V0IGJ5IHRo ZSB0aGlzIGRyaXZlci4KPiA+Cj4gPiAKPiA+IEJ5IHRoZSB3YXksIElzIHRoZXJlIGFueSBjYXNl IHdoZXJlIHRoZSBzYW1lIHBoeXNpY2FsIHBhZ2UgaXMgYWN0dWFsbHkKPiA+IG1hcHBlZCBpbnRv IHR3byBkaWZmZXJlbnQgdmlydHVhbCBhZGRyZXNzZXMgZm9yIHRoZSBidWZmZXJzIGFsbG9jYXRl ZCBieQo+ID4gdGhlIE1URCBzdWItc3lzdGVtPyBCZWNhdXNlIGZvciBhIGxvbmcgdGltZSBub3cg SSB3b25kZXIgd2hldGhlciB0aGUKPiA+IGNhY2hlIGFsaWFzZXMgaXNzdWUgaXMgYSByZWFsIG9y IG9ubHkgdGhlb3JldGljYWwgaXNzdWUgYnV0IEkgaGF2ZSBubwo+ID4gYW5zd2VyIHRvIHRoYXQg cXVlc3Rpb24uCj4gPiAgIAo+IAo+IEkgaGF2ZSBhdGxlYXN0IG9uZSBldmlkZW5jZSBvZiBWSVZU IGFsaWFzaW5nIGNhdXNpbmcgcHJvYmxlbS4gUGxlYXNlIHNlZQo+IHRoaXMgdGhyZWFkIG9uIERN QSBpc3N1ZXMgd2l0aCBkYXZpbmNpLXNwaSBkcml2ZXIKPiBodHRwczovL3d3dy5zcGluaWNzLm5l dC9saXN0cy9hcm0ta2VybmVsL21zZzU2MzQyMC5odG1sCj4gaHR0cHM6Ly93d3cuc3Bpbmljcy5u ZXQvbGlzdHMvYXJtLWtlcm5lbC9tc2c1NjM0NDUuaHRtbAo+IAo+ID4gVGhlbiBteSBuZXh0IHF1 ZXN0aW9uOiBpcyBzcGlfbWFwX21zZygpIGVub3VnaCBpbiBldmVyeSBjYXNlLCBldmVuIHdpdGgK PiA+IFZJUFQgb3IgVklWVCBjYWNoZXM/Cj4gPiAgIAo+IAo+IE5vdCByZWFsbHksIEkgYW0gZGVi dWdnaW5nIGFub3RoZXIgaXNzdWUgd2l0aCBVQklGUyBvbiBEUkE3NCBFVk0gKEFSTQo+IGNvcnRl eC1hMTUpIHdoZXJlaW4gcGFnZXMgYWxsb2NhdGVkIGJ5IHZtYWxsb2MgYXJlIGluIGhpZ2htZW0g cmVnaW9uCj4gdGhhdCBhcmUgbm90IGFkZHJlc3NhYmxlIHVzaW5nIDMyIGJpdCBhZGRyZXNzZXMg YW5kIGlzIGJhY2tlZCBieSBMUEFFLgo+IFNvLCBhIDMyIGJpdCBETUEgY2Fubm90IGFjY2VzcyB0 aGVzZSBidWZmZXJzIGF0IGFsbC4KPiBXaGVuIGRtYV9tYXBfc2coKSBpcyBjYWxsZWQgdG8gbWFw IHRoZXNlIHBhZ2VzIGJ5IHNwaV9tYXBfYnVmKCkgdGhlCj4gcGh5c2ljYWwgYWRkcmVzcyBpcyBq dXN0IHRydW5jYXRlZCB0byAzMiBiaXQgaW4gcGZuX3RvX2RtYSgpIChhcyBwYXJ0IG9mCj4gZG1h X21hcF9zZygpIGNhbGwpLiBUaGlzIHJlc3VsdHMgaW4gcmFuZG9tIGNyYXNoZXMgYXMgRE1BIHN0 YXJ0cwo+IGFjY2Vzc2luZyByYW5kb20gbWVtb3J5IGR1cmluZyBTUEkgcmVhZC4KPiAKPiBJTU8s IHRoZXJlIG1heSBiZSBtb3JlIHVuZGlzY292ZXJlZCBjYXZlYXQgd2l0aCB1c2luZyBkbWFfbWFw X3NnKCkgZm9yCj4gbm9uIGttYWxsb2MnZCBidWZmZXJzIGFuZCBpdHMgYmV0dGVyIHRoYXQgc3Bp LW5vciBzdGFydHMgaGFuZGxpbmcgdGhlc2UKPiBidWZmZXJzIGluc3RlYWQgb2YgcmVseWluZyBv biBzcGlfbWFwX21zZygpIGFuZCB3b3JraW5nIGFyb3VuZCBldmVyeQo+IHRpbWUgc29tZXRoaW5n IHBvcHMgdXAuCj4gCgpIbSwgSSBoYWQgYSBkaXNjdXNzaW9uIHdpdGggQ3lyaWxsZSBvbiB0aGUg c3BpLW5vci9tMjVwODAgYXJjaGl0ZWN0dXJlLAphbmQgSSBtdXN0IGFkbWl0IEkgZG9uJ3QgdW5k ZXJzdGFuZCB3aHkgdGhlIGRlY2lzaW9uIHRvIHVzZSBhIGJvdW5jZQpidWZmZXIgaXMgZG9uZSBh dCB0aGUgbTI1cDgwIGxldmVsLgpXaGlsZSBpdCBtYWtlIHNlbnNlIHRvIGRvIHRoYXQgZm9yIGRl ZGljYXRlZCBzcGktZmxhc2ggY29udHJvbGxlcnMsCndoaWNoIGNhbiBkZWNpZGUgYnkgdGhlbXNl bHZlcyBpZiB0aGV5IG5lZWQgYSBib3VuY2UgYnVmZmVyIGZvciBub24Ka21hbGxvYy1lZCBidWZm ZXJzLCBJIGRvbid0IHVuZGVyc3RhbmQgd2h5IHdlIGhhdmUgdG8gZG8gdGhhdCBmb3IKcmVndWxh ciBTUEkgY29udHJvbGxlcnMuIEkgbWVhbiwgdGhlIHNhbWUgcHJvYmxlbSBjYW4gYXJpc2UgZm9y IG90aGVyClNQSSBkZXZpY2UgZHJpdmVycyBpZiB0aGV5IGRlY2lkZSB0byB1c2Ugdm1hbGxvYy1l ZCBvciBrbWFwLWVkIGJ1ZmZlcnMsCnNvIHRoaXMgc2hvdWxkIGJlIGhhbmRsZWQgYXQgdGhlIFNQ SSBsZXZlbC4KCklmIHRoZSBTUEkgZnJhbWV3b3JrIGRvZXMgbm90IGltcG9zZSBhbnkgcmVzdHJp Y3Rpb24gb24gdGhlIGJ1ZmZlcnMgaXQKaXMgcGFzc2VkLCBpdCBzaG91bGQgZWl0aGVyIHRha2Ug Y2FyZSBvZiB0aGlzLCBvciBhc2sgU1BJIGNvbnRyb2xsZXIKZHJpdmVycyB0byBkbyBzbyAod2hp Y2ggaW1wbGllcyBjaGVja2luZyB0aGUgYnVmZmVyIGFkZHJlc3MgZGlyZWN0bHkKaW5zaWRlIHRo ZSBTUEkgY29udHJvbGxlciBkcml2ZXIgYW5kIGRlY2lkZSB0byB1c2UgYSBib3VuY2UgYnVmZmVy IG9yCnVzZSBQSU8gbW9kZSkuCgpBbSBJIG1pc3VuZGVyc3RhbmRpbmcgc29tZXRoaW5nIGhlcmU/ CgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18K TGludXggTVREIGRpc2N1c3Npb24gbWFpbGluZyBsaXN0Cmh0dHA6Ly9saXN0cy5pbmZyYWRlYWQu b3JnL21haWxtYW4vbGlzdGluZm8vbGludXgtbXRkLwo=