From: Boris Brezillon <boris.brezillon@bootlin.com> To: Yogesh Gaur <yogeshnarayan.gaur@nxp.com> Cc: linux-mtd@lists.infradead.org, marek.vasut@gmail.com, linux-spi@vger.kernel.org, devicetree@vger.kernel.org, robh@kernel.org, mark.rutland@arm.com, shawnguo@kernel.org, linux-arm-kernel@lists.infradead.org, computersforpeace@gmail.com, frieder.schrempf@exceet.de, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 1/5] spi: spi-mem: Add driver for NXP FlexSPI controller Date: Sat, 29 Sep 2018 17:40:23 +0200 [thread overview] Message-ID: <20180929174023.51b1e284@bbrezillon> (raw) In-Reply-To: <1537525323-20730-2-git-send-email-yogeshnarayan.gaur@nxp.com> Hi Yogesh, On Fri, 21 Sep 2018 15:51:59 +0530 Yogesh Gaur <yogeshnarayan.gaur@nxp.com> wrote: > +/* Registers used by the driver */ > +#define FSPI_MCR0 0x00 > +#define FSPI_MCR0_AHB_TIMEOUT_SHIFT 24 > +#define FSPI_MCR0_AHB_TIMEOUT_MASK (0xFF << FSPI_MCR0_AHB_TIMEOUT_SHIFT) > +#define FSPI_MCR0_IP_TIMEOUT_SHIFT 16 > +#define FSPI_MCR0_IP_TIMEOUT_MASK (0xFF << FSPI_MCR0_IP_TIMEOUT_SHIFT) > +#define FSPI_MCR0_LEARN_EN_SHIFT 15 > +#define FSPI_MCR0_LEARN_EN_MASK (1 << FSPI_MCR0_LEARN_EN_SHIFT) > +#define FSPI_MCR0_SCRFRUN_EN_SHIFT 14 > +#define FSPI_MCR0_SCRFRUN_EN_MASK (1 << FSPI_MCR0_SCRFRUN_EN_SHIFT) > +#define FSPI_MCR0_OCTCOMB_EN_SHIFT 13 > +#define FSPI_MCR0_OCTCOMB_EN_MASK (1 << FSPI_MCR0_OCTCOMB_EN_SHIFT) > +#define FSPI_MCR0_DOZE_EN_SHIFT 12 > +#define FSPI_MCR0_DOZE_EN_MASK (1 << FSPI_MCR0_DOZE_EN_SHIFT) > +#define FSPI_MCR0_HSEN_SHIFT 11 > +#define FSPI_MCR0_HSEN_MASK (1 << FSPI_MCR0_HSEN_SHIFT) > +#define FSPI_MCR0_SERCLKDIV_SHIFT 8 > +#define FSPI_MCR0_SERCLKDIV_MASK (7 << FSPI_MCR0_SERCLKDIV_SHIFT) > +#define FSPI_MCR0_ATDF_EN_SHIFT 7 > +#define FSPI_MCR0_ATDF_EN_MASK (1 << FSPI_MCR0_ATDF_EN_SHIFT) > +#define FSPI_MCR0_ARDF_EN_SHIFT 6 > +#define FSPI_MCR0_ARDF_EN_MASK (1 << FSPI_MCR0_ARDF_EN_SHIFT) > +#define FSPI_MCR0_RXCLKSRC_SHIFT 4 > +#define FSPI_MCR0_RXCLKSRC_MASK (3 << FSPI_MCR0_RXCLKSRC_SHIFT) > +#define FSPI_MCR0_END_CFG_SHIFT 2 > +#define FSPI_MCR0_END_CFG_MASK (3 << FSPI_MCR0_END_CFG_SHIFT) > +#define FSPI_MCR0_MDIS_SHIFT 1 > +#define FSPI_MCR0_MDIS_MASK (1 << FSPI_MCR0_MDIS_SHIFT) > +#define FSPI_MCR0_SWRST_SHIFT 0 > +#define FSPI_MCR0_SWRST_MASK (1 << FSPI_MCR0_SWRST_SHIFT) Do we really need all those _SHIFT/_MASK defs? I mean #define FSPI_MCR0_SWRST BIT(0) or #define FSPI_MCR0_AHB_TIMEOUT(x) ((x) << 24) #define FSPI_MCR0_AHB_TIMEOUT_MASK GENMASK(31, 24) are just fine. > + > +enum nxp_fspi_devtype { > + NXP_FSPI_LX2160A, > +}; I'm pretty sure you don't need this enum if you describe all dev caps in the nxp_fspi_devtype_data struct. > + > +struct nxp_fspi_devtype_data { > + enum nxp_fspi_devtype devtype; > + unsigned int rxfifo; > + unsigned int txfifo; > + unsigned int ahb_buf_size; > + unsigned int quirks; > + bool endianness; How about renaming this variable big_endian and dropping the {L,B}_ENDIAN macros? > +}; [...] > +struct nxp_fspi { > + void __iomem *iobase; > + void __iomem *ahb_addr; > + u32 memmap_phy; > + u32 memmap_phy_size; > + struct clk *clk, *clk_en; > + struct device *dev; > + struct completion c; > + const struct nxp_fspi_devtype_data *devtype_data; > + struct mutex lock; > + struct pm_qos_request pm_qos_req; > + int selected; > + void (*write)(u32 val, void __iomem *addr); > + u32 (*read)(void __iomem *addr); > +}; > + > +static void fspi_writel_be(u32 val, void __iomem *addr) > +{ > + iowrite32be(val, addr); > +} > + > +static void fspi_writel(u32 val, void __iomem *addr) > +{ > + iowrite32(val, addr); > +} > + > +static u32 fspi_readl_be(void __iomem *addr) > +{ > + return ioread32be(addr); > +} > + > +static u32 fspi_readl(void __iomem *addr) > +{ > + return ioread32(addr); > +} Hm, I'd recommend dropping the ->read/write() hooks and providing the following functions: static void fspi_writel(struct nxp_fspi *f, u32 val, void __iomem *addr) { if (f->big_endian) iowrite32be(val, addr); else iowrite32(val, addr); } static u32 fspi_readl(struct nxp_fspi *f, void __iomem *addr) { if (f->big_endian) return ioread32be(addr); else return ioread32(addr); } > + > +static irqreturn_t nxp_fspi_irq_handler(int irq, void *dev_id) > +{ > + struct nxp_fspi *f = dev_id; > + u32 reg; > + > + /* clear interrupt */ > + reg = f->read(f->iobase + FSPI_INTR); > + f->write(FSPI_INTR_IPCMDDONE_MASK, f->iobase + FSPI_INTR); > + > + if (reg & FSPI_INTR_IPCMDDONE_MASK) > + complete(&f->c); > + > + return IRQ_HANDLED; > +} [...] > +/* > + * If the slave device content being changed by Write/Erase, need to > + * invalidate the AHB buffer. This can be achieved by doing the reset > + * of controller after setting MCR0[SWRESET] bit. > + */ > +static inline void nxp_fspi_invalid(struct nxp_fspi *f) > +{ > + u32 reg; > + > + reg = f->read(f->iobase + FSPI_MCR0); > + f->write(reg | FSPI_MCR0_SWRST_MASK, f->iobase + FSPI_MCR0); > + > + while (f->read(f->iobase + FSPI_MCR0) & FSPI_MCR0_SWRST_MASK) > + ; Did you consider using readl_poll_timeout[_atomic]()? if (f->big_endian) mask = (u32)cpu_to_be32(FSPI_MCR0_SWRST_MASK); else mask = (u32)cpu_to_be32(FSPI_MCR0_SWRST_MASK); ret = readl_poll_timeout(f->iobase + FSPI_MCR0, reg, reg & mask, 0, FSPI_SWRST_TIMEOUT); WARN_ON(ret); > +} [...] > +static void nxp_fspi_read_ahb(struct nxp_fspi *f, const struct spi_mem_op *op) > +{ > + u32 len = op->data.nbytes; > + > + /* Read out the data directly from the AHB buffer. */ > + memcpy_fromio(op->data.buf.in, (f->ahb_addr + op->addr.val), len); Don't know if it's supported, but if it is, I recommend using DMA to do this copy, because otherwise you might stall the CPU for quite a long time if the flash is operating in a low-speed mode, and RT maintainers will complain about that at some point ;-). > +} > + > +static void nxp_fspi_fill_txfifo(struct nxp_fspi *f, > + const struct spi_mem_op *op) > +{ > + void __iomem *base = f->iobase; > + int i, j; > + int size, tmp_size, wm_size; > + u32 data = 0; > + u32 *txbuf = (u32 *) op->data.buf.out; > + > + /* clear the TX FIFO. */ > + f->write(FSPI_IPTXFCR_CLR_MASK, base + FSPI_IPTXFCR); > + > + /* Default value of water mark level is 8 bytes. */ > + wm_size = 8; > + size = op->data.nbytes / wm_size; > + for (i = 0; i < size; i++) { > + /* Wait for TXFIFO empty */ > + while (!(f->read(base + FSPI_INTR) & FSPI_INTR_IPTXWE_MASK)) > + ; Use readl_poll_timeout(), or even better, provide an helper (fspi_readl_poll_timeout()?) that hides the BE/LE stuff, so that you can reuse it when this pattern occurs. [...] > +static int nxp_fspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > +{ > + struct nxp_fspi *f = spi_controller_get_devdata(mem->spi->master); > + void __iomem *base = f->iobase; > + int err = 0; > + unsigned int timeout = 1000; > + > + mutex_lock(&f->lock); > + > + /* wait for the controller being ready */ > + do { > + u32 status; > + > + status = f->read(base + FSPI_STS0); > + if ((status & FSPI_STS0_ARB_IDLE_MASK) && > + (status & FSPI_STS0_SEQ_IDLE_MASK)) > + break; > + udelay(1); > + dev_dbg(f->dev, "The controller is busy, 0x%x\n", status); Same here. Note that I didn't spend time looking at how the IP works, which explains why I focus on tiny details here. Unfortunately, I won't have time to review the driver in more details, so I'll leave that to someone else, or let Mark decides if he's happy enough with the current version. Regards, Boris
WARNING: multiple messages have this Message-ID (diff)
From: boris.brezillon@bootlin.com (Boris Brezillon) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v3 1/5] spi: spi-mem: Add driver for NXP FlexSPI controller Date: Sat, 29 Sep 2018 17:40:23 +0200 [thread overview] Message-ID: <20180929174023.51b1e284@bbrezillon> (raw) In-Reply-To: <1537525323-20730-2-git-send-email-yogeshnarayan.gaur@nxp.com> Hi Yogesh, On Fri, 21 Sep 2018 15:51:59 +0530 Yogesh Gaur <yogeshnarayan.gaur@nxp.com> wrote: > +/* Registers used by the driver */ > +#define FSPI_MCR0 0x00 > +#define FSPI_MCR0_AHB_TIMEOUT_SHIFT 24 > +#define FSPI_MCR0_AHB_TIMEOUT_MASK (0xFF << FSPI_MCR0_AHB_TIMEOUT_SHIFT) > +#define FSPI_MCR0_IP_TIMEOUT_SHIFT 16 > +#define FSPI_MCR0_IP_TIMEOUT_MASK (0xFF << FSPI_MCR0_IP_TIMEOUT_SHIFT) > +#define FSPI_MCR0_LEARN_EN_SHIFT 15 > +#define FSPI_MCR0_LEARN_EN_MASK (1 << FSPI_MCR0_LEARN_EN_SHIFT) > +#define FSPI_MCR0_SCRFRUN_EN_SHIFT 14 > +#define FSPI_MCR0_SCRFRUN_EN_MASK (1 << FSPI_MCR0_SCRFRUN_EN_SHIFT) > +#define FSPI_MCR0_OCTCOMB_EN_SHIFT 13 > +#define FSPI_MCR0_OCTCOMB_EN_MASK (1 << FSPI_MCR0_OCTCOMB_EN_SHIFT) > +#define FSPI_MCR0_DOZE_EN_SHIFT 12 > +#define FSPI_MCR0_DOZE_EN_MASK (1 << FSPI_MCR0_DOZE_EN_SHIFT) > +#define FSPI_MCR0_HSEN_SHIFT 11 > +#define FSPI_MCR0_HSEN_MASK (1 << FSPI_MCR0_HSEN_SHIFT) > +#define FSPI_MCR0_SERCLKDIV_SHIFT 8 > +#define FSPI_MCR0_SERCLKDIV_MASK (7 << FSPI_MCR0_SERCLKDIV_SHIFT) > +#define FSPI_MCR0_ATDF_EN_SHIFT 7 > +#define FSPI_MCR0_ATDF_EN_MASK (1 << FSPI_MCR0_ATDF_EN_SHIFT) > +#define FSPI_MCR0_ARDF_EN_SHIFT 6 > +#define FSPI_MCR0_ARDF_EN_MASK (1 << FSPI_MCR0_ARDF_EN_SHIFT) > +#define FSPI_MCR0_RXCLKSRC_SHIFT 4 > +#define FSPI_MCR0_RXCLKSRC_MASK (3 << FSPI_MCR0_RXCLKSRC_SHIFT) > +#define FSPI_MCR0_END_CFG_SHIFT 2 > +#define FSPI_MCR0_END_CFG_MASK (3 << FSPI_MCR0_END_CFG_SHIFT) > +#define FSPI_MCR0_MDIS_SHIFT 1 > +#define FSPI_MCR0_MDIS_MASK (1 << FSPI_MCR0_MDIS_SHIFT) > +#define FSPI_MCR0_SWRST_SHIFT 0 > +#define FSPI_MCR0_SWRST_MASK (1 << FSPI_MCR0_SWRST_SHIFT) Do we really need all those _SHIFT/_MASK defs? I mean #define FSPI_MCR0_SWRST BIT(0) or #define FSPI_MCR0_AHB_TIMEOUT(x) ((x) << 24) #define FSPI_MCR0_AHB_TIMEOUT_MASK GENMASK(31, 24) are just fine. > + > +enum nxp_fspi_devtype { > + NXP_FSPI_LX2160A, > +}; I'm pretty sure you don't need this enum if you describe all dev caps in the nxp_fspi_devtype_data struct. > + > +struct nxp_fspi_devtype_data { > + enum nxp_fspi_devtype devtype; > + unsigned int rxfifo; > + unsigned int txfifo; > + unsigned int ahb_buf_size; > + unsigned int quirks; > + bool endianness; How about renaming this variable big_endian and dropping the {L,B}_ENDIAN macros? > +}; [...] > +struct nxp_fspi { > + void __iomem *iobase; > + void __iomem *ahb_addr; > + u32 memmap_phy; > + u32 memmap_phy_size; > + struct clk *clk, *clk_en; > + struct device *dev; > + struct completion c; > + const struct nxp_fspi_devtype_data *devtype_data; > + struct mutex lock; > + struct pm_qos_request pm_qos_req; > + int selected; > + void (*write)(u32 val, void __iomem *addr); > + u32 (*read)(void __iomem *addr); > +}; > + > +static void fspi_writel_be(u32 val, void __iomem *addr) > +{ > + iowrite32be(val, addr); > +} > + > +static void fspi_writel(u32 val, void __iomem *addr) > +{ > + iowrite32(val, addr); > +} > + > +static u32 fspi_readl_be(void __iomem *addr) > +{ > + return ioread32be(addr); > +} > + > +static u32 fspi_readl(void __iomem *addr) > +{ > + return ioread32(addr); > +} Hm, I'd recommend dropping the ->read/write() hooks and providing the following functions: static void fspi_writel(struct nxp_fspi *f, u32 val, void __iomem *addr) { if (f->big_endian) iowrite32be(val, addr); else iowrite32(val, addr); } static u32 fspi_readl(struct nxp_fspi *f, void __iomem *addr) { if (f->big_endian) return ioread32be(addr); else return ioread32(addr); } > + > +static irqreturn_t nxp_fspi_irq_handler(int irq, void *dev_id) > +{ > + struct nxp_fspi *f = dev_id; > + u32 reg; > + > + /* clear interrupt */ > + reg = f->read(f->iobase + FSPI_INTR); > + f->write(FSPI_INTR_IPCMDDONE_MASK, f->iobase + FSPI_INTR); > + > + if (reg & FSPI_INTR_IPCMDDONE_MASK) > + complete(&f->c); > + > + return IRQ_HANDLED; > +} [...] > +/* > + * If the slave device content being changed by Write/Erase, need to > + * invalidate the AHB buffer. This can be achieved by doing the reset > + * of controller after setting MCR0[SWRESET] bit. > + */ > +static inline void nxp_fspi_invalid(struct nxp_fspi *f) > +{ > + u32 reg; > + > + reg = f->read(f->iobase + FSPI_MCR0); > + f->write(reg | FSPI_MCR0_SWRST_MASK, f->iobase + FSPI_MCR0); > + > + while (f->read(f->iobase + FSPI_MCR0) & FSPI_MCR0_SWRST_MASK) > + ; Did you consider using readl_poll_timeout[_atomic]()? if (f->big_endian) mask = (u32)cpu_to_be32(FSPI_MCR0_SWRST_MASK); else mask = (u32)cpu_to_be32(FSPI_MCR0_SWRST_MASK); ret = readl_poll_timeout(f->iobase + FSPI_MCR0, reg, reg & mask, 0, FSPI_SWRST_TIMEOUT); WARN_ON(ret); > +} [...] > +static void nxp_fspi_read_ahb(struct nxp_fspi *f, const struct spi_mem_op *op) > +{ > + u32 len = op->data.nbytes; > + > + /* Read out the data directly from the AHB buffer. */ > + memcpy_fromio(op->data.buf.in, (f->ahb_addr + op->addr.val), len); Don't know if it's supported, but if it is, I recommend using DMA to do this copy, because otherwise you might stall the CPU for quite a long time if the flash is operating in a low-speed mode, and RT maintainers will complain about that at some point ;-). > +} > + > +static void nxp_fspi_fill_txfifo(struct nxp_fspi *f, > + const struct spi_mem_op *op) > +{ > + void __iomem *base = f->iobase; > + int i, j; > + int size, tmp_size, wm_size; > + u32 data = 0; > + u32 *txbuf = (u32 *) op->data.buf.out; > + > + /* clear the TX FIFO. */ > + f->write(FSPI_IPTXFCR_CLR_MASK, base + FSPI_IPTXFCR); > + > + /* Default value of water mark level is 8 bytes. */ > + wm_size = 8; > + size = op->data.nbytes / wm_size; > + for (i = 0; i < size; i++) { > + /* Wait for TXFIFO empty */ > + while (!(f->read(base + FSPI_INTR) & FSPI_INTR_IPTXWE_MASK)) > + ; Use readl_poll_timeout(), or even better, provide an helper (fspi_readl_poll_timeout()?) that hides the BE/LE stuff, so that you can reuse it when this pattern occurs. [...] > +static int nxp_fspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > +{ > + struct nxp_fspi *f = spi_controller_get_devdata(mem->spi->master); > + void __iomem *base = f->iobase; > + int err = 0; > + unsigned int timeout = 1000; > + > + mutex_lock(&f->lock); > + > + /* wait for the controller being ready */ > + do { > + u32 status; > + > + status = f->read(base + FSPI_STS0); > + if ((status & FSPI_STS0_ARB_IDLE_MASK) && > + (status & FSPI_STS0_SEQ_IDLE_MASK)) > + break; > + udelay(1); > + dev_dbg(f->dev, "The controller is busy, 0x%x\n", status); Same here. Note that I didn't spend time looking at how the IP works, which explains why I focus on tiny details here. Unfortunately, I won't have time to review the driver in more details, so I'll leave that to someone else, or let Mark decides if he's happy enough with the current version. Regards, Boris
next prev parent reply other threads:[~2018-09-29 15:40 UTC|newest] Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-09-21 10:21 [PATCH v3 0/5] spi: spi-mem: Add driver for NXP FlexSPI controller Yogesh Gaur 2018-09-21 10:21 ` Yogesh Gaur 2018-09-21 10:21 ` [PATCH v3 1/5] " Yogesh Gaur 2018-09-21 10:21 ` Yogesh Gaur 2018-09-29 15:40 ` Boris Brezillon [this message] 2018-09-29 15:40 ` Boris Brezillon 2018-10-01 6:18 ` Frieder Schrempf 2018-10-01 6:18 ` Frieder Schrempf 2018-10-01 6:18 ` Frieder Schrempf 2018-10-01 9:02 ` Yogesh Narayan Gaur 2018-10-01 9:02 ` Yogesh Narayan Gaur 2018-10-01 9:02 ` Yogesh Narayan Gaur 2018-10-01 9:09 ` Boris Brezillon 2018-10-01 9:09 ` Boris Brezillon 2018-10-01 9:09 ` Boris Brezillon 2018-10-08 11:21 ` Yogesh Narayan Gaur 2018-10-08 11:21 ` Yogesh Narayan Gaur 2018-10-08 11:21 ` Yogesh Narayan Gaur 2018-10-08 12:32 ` Boris Brezillon 2018-10-08 12:32 ` Boris Brezillon 2018-10-08 12:32 ` Boris Brezillon 2018-09-21 10:22 ` [PATCH v3 2/5] dt-bindings: spi: add binding file " Yogesh Gaur 2018-09-21 10:22 ` Yogesh Gaur 2018-09-27 19:10 ` Rob Herring 2018-09-27 19:10 ` Rob Herring 2018-09-21 10:22 ` [PATCH v3 3/5] arm64: dts: lx2160a: add FlexSPI node property Yogesh Gaur 2018-09-21 10:22 ` Yogesh Gaur 2018-09-21 10:22 ` [PATCH v3 4/5] arm64: defconfig: enable NXP FlexSPI driver Yogesh Gaur 2018-09-21 10:22 ` Yogesh Gaur 2018-09-21 10:22 ` [PATCH v3 5/5] MAINTAINERS: add maintainers for the " Yogesh Gaur 2018-09-21 10:22 ` Yogesh Gaur
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180929174023.51b1e284@bbrezillon \ --to=boris.brezillon@bootlin.com \ --cc=computersforpeace@gmail.com \ --cc=devicetree@vger.kernel.org \ --cc=frieder.schrempf@exceet.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mtd@lists.infradead.org \ --cc=linux-spi@vger.kernel.org \ --cc=marek.vasut@gmail.com \ --cc=mark.rutland@arm.com \ --cc=robh@kernel.org \ --cc=shawnguo@kernel.org \ --cc=yogeshnarayan.gaur@nxp.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.