Hi Boris,
> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezil...@bootlin.com]
> Sent: Saturday, September 29, 2018 9:10 PM
> To: Yogesh Narayan Gaur <yogeshnarayan.g...@nxp.com>
> Cc: linux-...@lists.infradead.org; marek.va...@gmail.com; linux-
> s...@vger.kernel.org; devicet...@vger.kernel.org; r...@kernel.org;
> mark.rutl...@arm.com; shawn...@kernel.org; linux-arm-
> ker...@lists.infradead.org; computersforpe...@gmail.com;
> frieder.schre...@exceet.de; linux-kernel@vger.kernel.org
> 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.g...@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
>
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
