Hi Boris,

-----Original Message-----
From: Boris Brezillon [mailto:boris.brezillon at bootlin.com]
Sent: Saturday, September 29, 2018 9:10 PM
To: Yogesh Narayan Gaur <redacted>
Cc: linux-mtd at lists.infradead.org; marek.vasut at gmail.com; linux-
spi at vger.kernel.org; devicetree at vger.kernel.org; robh at kernel.org;
mark.rutland at arm.com; shawnguo at kernel.org; linux-arm-
kernel at lists.infradead.org; computersforpeace at gmail.com;
frieder.schrempf at exceet.de; linux-kernel at 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 [off-list ref] wrote:

quoted

+/* 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)

quoted

+#define FSPI_MCR0_IP_TIMEOUT_SHIFT	16
+#define FSPI_MCR0_IP_TIMEOUT_MASK	(0xFF <<

FSPI_MCR0_IP_TIMEOUT_SHIFT)

quoted

+#define FSPI_MCR0_LEARN_EN_SHIFT	15
+#define FSPI_MCR0_LEARN_EN_MASK		(1 <<

FSPI_MCR0_LEARN_EN_SHIFT)

quoted

+#define FSPI_MCR0_SCRFRUN_EN_SHIFT	14
+#define FSPI_MCR0_SCRFRUN_EN_MASK	(1 <<

FSPI_MCR0_SCRFRUN_EN_SHIFT)

quoted

+#define FSPI_MCR0_OCTCOMB_EN_SHIFT	13
+#define FSPI_MCR0_OCTCOMB_EN_MASK	(1 <<

FSPI_MCR0_OCTCOMB_EN_SHIFT)

quoted

+#define FSPI_MCR0_DOZE_EN_SHIFT		12
+#define FSPI_MCR0_DOZE_EN_MASK		(1 <<

FSPI_MCR0_DOZE_EN_SHIFT)

quoted

+#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)

quoted

+#define FSPI_MCR0_ATDF_EN_SHIFT		7
+#define FSPI_MCR0_ATDF_EN_MASK		(1 <<

FSPI_MCR0_ATDF_EN_SHIFT)

quoted

+#define FSPI_MCR0_ARDF_EN_SHIFT		6
+#define FSPI_MCR0_ARDF_EN_MASK		(1 <<

FSPI_MCR0_ARDF_EN_SHIFT)

quoted

+#define FSPI_MCR0_RXCLKSRC_SHIFT	4
+#define FSPI_MCR0_RXCLKSRC_MASK		(3 <<

FSPI_MCR0_RXCLKSRC_SHIFT)

quoted

+#define FSPI_MCR0_END_CFG_SHIFT		2
+#define FSPI_MCR0_END_CFG_MASK		(3 <<

FSPI_MCR0_END_CFG_SHIFT)

quoted

+#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.

quoted

+
+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.

quoted

+
+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.

quoted

+};

[...]

quoted

+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);
}

quoted

+
+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;
+}

[...]

quoted

+/*
+ * 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);

quoted

+}

Thanks, added readl_poll_timeout() functionality instead of busy looping.

[...]

quoted

+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

quoted

+}
+
+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.

[...]

quoted

+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