From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wi0-f170.google.com ([209.85.212.170]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1YylEs-0002ZP-IG for linux-mtd@lists.infradead.org; Sat, 30 May 2015 18:12:07 +0000 Received: by wibut5 with SMTP id ut5so6955362wib.1 for ; Sat, 30 May 2015 11:11:44 -0700 (PDT) Message-ID: <5569FC99.1060706@vanguardiasur.com.ar> Date: Sat, 30 May 2015 15:08:25 -0300 From: Ezequiel Garcia MIME-Version: 1.0 To: Joachim Eastwood Subject: Re: [PATCH 1/2] mtd: spi-nor: add driver for NXP SPI Flash Interface (SPIFI) References: <1432921816-10765-1-git-send-email-manabian@gmail.com> <1432921816-10765-2-git-send-email-manabian@gmail.com> <5569DAB9.9070105@vanguardiasur.com.ar> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: devicetree@vger.kernel.org, Richard Weinberger , Ariel D'Alessandro , linux-mtd@lists.infradead.org, shijie.huang@intel.com, computersforpeace@gmail.com, dwmw2@infradead.org, "linux-arm-kernel@lists.infradead.org" List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , +Richard On 05/30/2015 01:51 PM, Joachim Eastwood wrote: > Hi Ezequiel, > > On 30 May 2015 at 17:43, Ezequiel Garcia wrote: >> Hi Joachim, >> >> Looks pretty neat. I've just a couple comments. >> >> On 05/29/2015 02:50 PM, Joachim Eastwood wrote: >>> Add SPI-NOR driver for the SPI Flash Interface (SPIFI) >>> controller that is found newer NXP MCU devices. >>> >>> The controller supports serial SPI Flash devices with 1-, 2- >>> and 4-bit width in either SPI mode 0 or 3. The controller >>> can operate in either command or memory mode. In memory mode >>> the Flash is exposed as normal memory and can be directly >>> accessed by the CPU. >>> >>> Signed-off-by: Joachim Eastwood >>> --- >>> drivers/mtd/spi-nor/Kconfig | 10 + >>> drivers/mtd/spi-nor/Makefile | 1 + >>> drivers/mtd/spi-nor/nxp-spifi.c | 508 ++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 519 insertions(+) >>> create mode 100644 drivers/mtd/spi-nor/nxp-spifi.c >>> >>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig >>> index 64a4f0edabc7..f10a37f1a4ef 100644 >>> --- a/drivers/mtd/spi-nor/Kconfig >>> +++ b/drivers/mtd/spi-nor/Kconfig >>> @@ -28,4 +28,14 @@ config SPI_FSL_QUADSPI >>> This enables support for the Quad SPI controller in master mode. >>> We only connect the NOR to this controller now. >>> >>> +config SPI_NXP_SPIFI >>> + tristate "NXP SPI Flash Interface (SPIFI)" >>> + depends on OF && (ARCH_LPC18XX || COMPILE_TEST) >> >> Since you are adding COMPILE_TEST, maybe you want >> 'depends on HAS_IOMEM' as well? > > Since MTD depends on GENERIC_IO is that needed? > Or am I confusing the config options here(?) > I think you need HAS_IOMEM for ioremap stuff. Maybe Brian or Richard will know better. >>> +static int nxp_spifi_wait_for_event(struct nxp_spifi *spifi, u32 event) >>> +{ >>> + int retry = 3; >>> + u32 stat; >>> + >>> + do { >>> + stat = readb(spifi->io_base + SPIFI_STAT); >>> + if (!(stat & event)) >>> + return 0; >>> + >>> + udelay(10); >>> + } while (retry--); >>> + >>> + return -ETIMEDOUT; >> >> This could be replaced with the readb_poll_timeout_atomic(). >> But it seems you would also use readb_poll_timeout(), since won't be >> called in atomic context. > > Didn't know about the readX_poll_timeout functions and does indeed > look usable. Thanks for the tip. > > >>> +static int nxp_spifi_set_memory_mode_on(struct nxp_spifi *spifi) >>> +{ >>> + int retry = 3; >>> + u32 stat; >>> + >>> + stat = readb(spifi->io_base + SPIFI_STAT); >>> + if (stat & SPIFI_STAT_MCINIT) >>> + return 0; >> >> Do you think it makes sense to cache the memory/command mode >> instead of reading it? > > I think that's good idea. > >> Would it affect throughput in any sense? > > Maybe. It's done for every read/write operation so making it a bit > faster is good idea. I don't see any downsides to implement it the way > you suggest either. > > >> I'm thinking these slow-clocked microcontrollers might benefit from >> such tricks, but I don't have actual numbers to back this up. >> >>> + >>> + writel(spifi->mcmd, spifi->io_base + SPIFI_MCMD); >>> + >>> + do { >>> + stat = readb(spifi->io_base + SPIFI_STAT); >>> + if (stat & SPIFI_STAT_MCINIT) >>> + return 0; >>> + >>> + udelay(10); >>> + } while (retry--); >> >> Same here about using iopoll variants. > > Yeah, got it. > > >>> + >>> + dev_err(spifi->dev, "unable to enter memory mode\n"); >>> + >>> + return -ETIMEDOUT; >>> +} >>> + >>> +static int nxp_spifi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len) >>> +{ >>> + struct nxp_spifi *spifi = nor->priv; >>> + u32 cmd; >>> + int ret; >>> + >>> + ret = nxp_spifi_set_memory_mode_off(spifi); >>> + if (ret) >>> + return ret; >>> + >>> + cmd = SPIFI_CMD_DATALEN(len) | >>> + SPIFI_CMD_OPCODE(opcode) | >>> + SPIFI_CMD_FIELDFORM_ALL_SERIAL | >>> + SPIFI_CMD_FRAMEFORM_OPCODE_ONLY; >>> + writel(cmd, spifi->io_base + SPIFI_CMD); >>> + >>> + while (len--) >>> + *buf++ = readb(spifi->io_base + SPIFI_DATA); >> >> Nit: extra whitespace here. > > Well spotted! > > >>> +static int nxp_spifi_probe(struct platform_device *pdev) >>> +{ >>> + struct device_node *flash_np; >>> + struct nxp_spifi *spifi; >>> + struct resource *res; >>> + int ret; >>> + >>> + spifi = devm_kzalloc(&pdev->dev, sizeof(*spifi), GFP_KERNEL); >>> + if (!spifi) >>> + return -ENOMEM; >>> + >>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "spifi"); >>> + spifi->io_base = devm_ioremap_resource(&pdev->dev, res); >>> + if (IS_ERR(spifi->io_base)) >>> + return PTR_ERR(spifi->io_base); >>> + >>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "flash"); >>> + spifi->flash_base = devm_ioremap_resource(&pdev->dev, res); >>> + if (IS_ERR(spifi->flash_base)) >>> + return PTR_ERR(spifi->flash_base); >>> + >> >> Just curious: is the memory mapping fixed? > > On LPC43xx you can chose between two different mappings; 0x1400 0000 > and 0x8000 0000 > > The first only support up 64 MB, as opposed to 128 MB for the second, > but is slightly faster according to the data sheet. See Table 444 in > the data sheet. > > Other LPC families have different mappings as well. > OK, so I guess the bootloader would be in charge of wiring this up and updating the dtb - if needed. -- Ezequiel Garcia, VanguardiaSur www.vanguardiasur.com.ar