diff mbox series

[1/3] spi: spi-nxp-fspi: dynamically alloc AHB memory for FSPI

Message ID 20190710023128.13115-2-han.xu@nxp.com (mailing list archive)
State New, archived
Headers show
Series dynamically alloc AHB memory | expand

Commit Message

Han Xu July 10, 2019, 2:31 a.m. UTC 
From: Han Xu <han.xu@nxp.com>

dynamically alloc AHB memory for FSPI controller

Signed-off-by: Han Xu <han.xu@nxp.com>
---
 drivers/spi/spi-nxp-fspi.c | 39 ++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

Comments

Mark Brown July 10, 2019, 3:16 p.m. UTC | #1 
On Wed, Jul 10, 2019 at 10:31:26AM +0800, han.xu@nxp.com wrote:
> From: Han Xu <han.xu@nxp.com>
> 
> dynamically alloc AHB memory for FSPI controller

Why?  This is currently done at probe which is what you'd expect
to happen here, there's no explanation as to why this change is
being made.
Han Xu July 10, 2019, 3:35 p.m. UTC | #2 
> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Wednesday, July 10, 2019 10:16 AM
> To: Han Xu <han.xu@nxp.com>
> Cc: Ashish Kumar <ashish.kumar@nxp.com>; linux-spi@vger.kernel.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH 1/3] spi: spi-nxp-fspi: dynamically alloc AHB memory
> for FSPI
> 
> On Wed, Jul 10, 2019 at 10:31:26AM +0800, han.xu@nxp.com wrote:
> > From: Han Xu <han.xu@nxp.com>
> >
> > dynamically alloc AHB memory for FSPI controller
> 
> Why?  This is currently done at probe which is what you'd expect to happen
> here, there's no explanation as to why this change is being made.

Explained in cover letter, It failed to alloc the whole memory mapping area during
probe on some platforms, since the AHB memory area could be pretty large. The
error may look like:

[    1.129404] fsl-quadspi 1550000.spi: ioremap failed for resource [mem 0x40000000-0x7fffffff]
Mark Brown July 11, 2019, 12:41 p.m. UTC | #3 
On Wed, Jul 10, 2019 at 03:35:46PM +0000, Han Xu wrote:

> > > dynamically alloc AHB memory for FSPI controller

> > Why?  This is currently done at probe which is what you'd expect to happen
> > here, there's no explanation as to why this change is being made.

> Explained in cover letter, It failed to alloc the whole memory mapping area during
> probe on some platforms, since the AHB memory area could be pretty large. The
> error may look like:

> [    1.129404] fsl-quadspi 1550000.spi: ioremap failed for resource [mem 0x40000000-0x7fffffff]

The commit itself needs to have some explanation of what it's
doing so it's in the git log, particularly for something odd like
this.  More generally this just doesn't feel like it's solving
the problem - essentially we're just deferring the mapping and
then keep on failing operations until the allocation succeeds for
some reason.  That's going to be disruptive for users of the
device and it doesn't seem like it's going to be a robust
solution.  Why does the allocation not work initially and why is
it more likely to work later on?
Han Xu July 11, 2019, 7:45 p.m. UTC | #4 
> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Thursday, July 11, 2019 7:41 AM
> To: Han Xu <han.xu@nxp.com>
> Cc: Ashish Kumar <ashish.kumar@nxp.com>; linux-spi@vger.kernel.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [EXT] Re: [PATCH 1/3] spi: spi-nxp-fspi: dynamically alloc AHB memory
> for FSPI
> 
> On Wed, Jul 10, 2019 at 03:35:46PM +0000, Han Xu wrote:
> 
> > > > dynamically alloc AHB memory for FSPI controller
> 
> > > Why?  This is currently done at probe which is what you'd expect to
> > > happen here, there's no explanation as to why this change is being made.
> 
> > Explained in cover letter, It failed to alloc the whole memory mapping
> > area during probe on some platforms, since the AHB memory area could
> > be pretty large. The error may look like:
> 
> > [    1.129404] fsl-quadspi 1550000.spi: ioremap failed for resource [mem
> 0x40000000-0x7fffffff]
> 
> The commit itself needs to have some explanation of what it's doing so it's in the
> git log, particularly for something odd like this.  More generally this just doesn't feel
> like it's solving the problem - essentially we're just deferring the mapping and then
> keep on failing operations until the allocation succeeds for some reason.  That's
> going to be disruptive for users of the device and it doesn't seem like it's going to
> be a robust solution.  Why does the allocation not work initially and why is it more
> likely to work later on?

Yes, I will explain the reason in next version. To allocate the whole 256MB memory at one time exceed the vmalloc limit, so we dynamically allocate small amount of memory just as needed. There is no failing operation, just deferring and re-allocate if new access area beyond the previous allocate area.
diff mbox series

Patch

diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
index 8894f98cc99c..84edaca28e01 100644
--- a/drivers/spi/spi-nxp-fspi.c
+++ b/drivers/spi/spi-nxp-fspi.c
@@ -307,6 +307,7 @@ 
 
 #define POLL_TOUT		5000
 #define NXP_FSPI_MAX_CHIPSELECT		4
+#define NXP_FSPI_MIN_IOMAP	SZ_4M
 
 struct nxp_fspi_devtype_data {
 	unsigned int rxfifo;
@@ -329,6 +330,8 @@  struct nxp_fspi {
 	void __iomem *ahb_addr;
 	u32 memmap_phy;
 	u32 memmap_phy_size;
+	u32 memmap_start;
+	u32 memmap_len;
 	struct clk *clk, *clk_en;
 	struct device *dev;
 	struct completion c;
@@ -641,12 +644,34 @@  static void nxp_fspi_select_mem(struct nxp_fspi *f, struct spi_device *spi)
 	f->selected = spi->chip_select;
 }
 
-static void nxp_fspi_read_ahb(struct nxp_fspi *f, const struct spi_mem_op *op)
+static int nxp_fspi_read_ahb(struct nxp_fspi *f, const struct spi_mem_op *op)
 {
+	u32 start = op->addr.val;
 	u32 len = op->data.nbytes;
 
+	/* if necessary, ioremap before AHB read */
+	if ((!f->ahb_addr) || start < f->memmap_start ||
+	    start + len > f->memmap_start + f->memmap_len) {
+		if (f->ahb_addr)
+			iounmap(f->ahb_addr);
+
+		f->memmap_start = start;
+		f->memmap_len = len > NXP_FSPI_MIN_IOMAP ?
+				len : NXP_FSPI_MIN_IOMAP;
+
+		f->ahb_addr = ioremap_wc(f->memmap_phy + f->memmap_start,
+					 f->memmap_len);
+		if (!f->ahb_addr) {
+			dev_err(f->dev, "failed to alloc memory\n");
+			return -ENOMEM;
+		}
+	}
+
 	/* Read out the data directly from the AHB buffer. */
-	memcpy_fromio(op->data.buf.in, (f->ahb_addr + op->addr.val), len);
+	memcpy_fromio(op->data.buf.in,
+		      (f->ahb_addr + start - f->memmap_start), len);
+
+	return 0;
 }
 
 static void nxp_fspi_fill_txfifo(struct nxp_fspi *f,
@@ -806,7 +831,7 @@  static int nxp_fspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 	 */
 	if (op->data.nbytes > (f->devtype_data->rxfifo - 4) &&
 	    op->data.dir == SPI_MEM_DATA_IN) {
-		nxp_fspi_read_ahb(f, op);
+		err = nxp_fspi_read_ahb(f, op);
 	} else {
 		if (op->data.nbytes && op->data.dir == SPI_MEM_DATA_OUT)
 			nxp_fspi_fill_txfifo(f, op);
@@ -976,11 +1001,6 @@  static int nxp_fspi_probe(struct platform_device *pdev)
 
 	/* find the resources - controller memory mapped space */
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "fspi_mmap");
-	f->ahb_addr = devm_ioremap_resource(dev, res);
-	if (IS_ERR(f->ahb_addr)) {
-		ret = PTR_ERR(f->ahb_addr);
-		goto err_put_ctrl;
-	}
 
 	/* assign memory mapped starting address and mapped size. */
 	f->memmap_phy = res->start;
@@ -1059,6 +1079,9 @@  static int nxp_fspi_remove(struct platform_device *pdev)
 
 	mutex_destroy(&f->lock);
 
+	if (f->ahb_addr)
+		iounmap(f->ahb_addr);
+
 	return 0;
 }