Hi Boris, > -----Original Message----- > From: Boris Brezillon [mailto:boris.brezillon@xxxxxxxxxxx] > Sent: Saturday, September 29, 2018 9:10 PM > To: Yogesh Narayan Gaur <yogeshnarayan.gaur@xxxxxxx> > Cc: linux-mtd@xxxxxxxxxxxxxxxxxxx; marek.vasut@xxxxxxxxx; linux- > spi@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; robh@xxxxxxxxxx; > mark.rutland@xxxxxxx; shawnguo@xxxxxxxxxx; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx; computersforpeace@xxxxxxxxx; > frieder.schrempf@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v3 1/5] spi: spi-mem: Add driver for NXP FlexSPI controller > > Hi Yogesh, > > On Fri, 21 Sep 2018 15:51:59 +0530 > Yogesh Gaur <yogeshnarayan.gaur@xxxxxxx> 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 > Changes done in next version of the patch series, v4 [1]. > #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. Done, in v4. > > > + > > +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? > Done in v4, default make as little_endian to reduce indirect branch checking. > > +}; > > [...] > > > +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); > > > +} > Thanks, added readl_poll_timeout() functionality instead of busy looping. > [...] > > > +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 ;-). > Read using DMA is not supported by the controller in AHB mode, only supported in IP mode. Have to use memcpy_fromio() calls. Maximum data size can be read in single call is 0x800 using AHB read. -- Regards Yogesh Gaur. [1] https://patchwork.ozlabs.org/project/linux-mtd/list/?series=69535 > > +} > > + > > +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
