Message-ID: <aRyfoXaTdL1gvPOY@lizhi-Precision-Tower-5810>
Date: Tue, 18 Nov 2025 11:32:33 -0500
From: Frank Li <Frank.li@....com>
To: Haibo Chen <haibo.chen@....com>
Cc: Han Xu <han.xu@....com>, Mark Brown <broonie@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>, linux-spi@...r.kernel.org,
	imx@...ts.linux.dev, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 2/2] spi: add driver for NXP XSPI controller

On Tue, Nov 18, 2025 at 11:34:17AM +0800, Haibo Chen wrote:
> Add driver support for NXP XSPI controller.
>
> XSPI is a flexible SPI host controller which supports up to
> 2 external devices (2 CS). It support Single/Dual/Quad/Octal
> mode data transfer.
>
> The difference between XSPI and Flexspi:
> 1.the register layout is total different.
> 2.XSPI support multiple independent execution environments
> (EENVs) for HW virtualization with some limitations. Each EENV
> has its own interrupt and its own set of programming registers
> that exists in a specific offset range in the XSPI memory map.
> The main environment (EENV0) address space contains all of the
> registers for controlling EENV0 plus all of the general XSPI
> control and programming registers. The register mnemonics for
> the user environments (EENV1 to EENV4) have "_SUB_n" appended
> to the mnemonic for the corresponding main-environment register.
>
> Current driver based on EENV0, which means system already give
> EENV0 right to linux.
>
> This driver use SPI memory interface of the SPI framework to issue
> flash memory operations. Tested this driver with mtd_debug and
> UBIFS on NXP i.MX943 EVK board which has one MT35XU512ABA spi nor
> flash. NOw this driver has the following key features:
> - Support up to OCT DDR mode
> - Support AHB read
> - Support IP read and IP write
> - Support two CS
>
> Signed-off-by: Haibo Chen <haibo.chen@....com>
> ---
>  MAINTAINERS                |    1 +
>  drivers/spi/Kconfig        |   10 +
>  drivers/spi/Makefile       |    1 +
>  drivers/spi/spi-nxp-xspi.c | 1367 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 1379 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2f17f925ee23dd90acc1b4bf25f158070cd2b65e..527b4f284c3207fb9760ece5cc1d350e7ad8fe50 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18853,6 +18853,7 @@ L:	linux-spi@...r.kernel.org
>  L:	imx@...ts.linux.dev
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/spi/nxp,imx94-xspi.yaml
> +F:	drivers/spi/spi-nxp-xspi.c
>
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/jiffies.h>
> +#include <linux/kernel.h>
> +#include <linux/log2.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>
> +
> +/* Runtime pm timeout */
> +#define XSPI_RPM_TIMEOUT 50	/* 50ms */

Need unit XSPI_RPM_TIMEOUT_MS

> +/*
> + * The driver only uses one single LUT entry, that is updated on
> + * each call of exec_op(). Index 0 is preset at boot with a basic
> + * read operation, so let's use the last entry (15).
> + */
...
> + *  ---------------------------------------------------
> + *  | INSTR1 | PAD1 | OPRND1 | INSTR0 | PAD0 | OPRND0 |
> + *  ---------------------------------------------------
> + */
> +#define PAD_SHIFT		8
> +#define INSTR_SHIFT		10
> +#define OPRND_SHIFT		16
> +
> +/* Macros for constructing the LUT register. */
> +#define LUT_DEF(idx, ins, pad, opr)			  \
> +	((((ins) << INSTR_SHIFT) | ((pad) << PAD_SHIFT) | \
> +	(opr)) << (((idx) % 2) * OPRND_SHIFT))
> +
> +#define NXP_XSPI_MIN_IOMAP	SZ_4M
> +#define NXP_XSPI_MAX_CHIPSELECT		2
> +#define POLL_TOUT		5000

Need unit, POLL_TOUT_US

> +
> +/* Access flash memory using IP bus only */
> +#define XSPI_QUIRK_USE_IP_ONLY	BIT(0)
> +
> +struct nxp_xspi_devtype_data {
> +	unsigned int rxfifo;
> +	unsigned int txfifo;
> +	unsigned int ahb_buf_size;
> +	unsigned int quirks;
> +};
> +
> +static struct nxp_xspi_devtype_data imx94_data = {
> +	.rxfifo = SZ_512,       /* (128 * 4 bytes)  */
> +	.txfifo = SZ_1K,        /* (256 * 4 bytes)  */
> +	.ahb_buf_size = SZ_4K,  /* (1024 * 4 bytes)  */
> +};
> +
> +struct nxp_xspi {
> +	void __iomem *iobase;
> +	void __iomem *ahb_addr;
> +	u32 memmap_phy;
> +	u32 memmap_phy_size;
> +	u32 memmap_start;
> +	u32 memmap_len;
> +	struct clk *clk;
> +	struct device *dev;
> +	struct completion c;
> +	const struct nxp_xspi_devtype_data *devtype_data;
> +	/* mutex lock for each operation */
> +	struct mutex lock;
> +	int selected;
> +#define XSPI_DTR_PROTO		BIT(0)
> +	int flags;
> +	/* Save the previous operation clock rate */
> +	unsigned long pre_op_rate;
> +	/* The max clock rate xspi supported output to device */
> +	unsigned long support_max_rate;
> +};
> +
> +static inline int needs_ip_only(struct nxp_xspi *xspi)
> +{
> +	return xspi->devtype_data->quirks & XSPI_QUIRK_USE_IP_ONLY;
> +}
> +
> +static irqreturn_t nxp_xspi_irq_handler(int irq, void *dev_id)
> +{
> +	struct nxp_xspi *xspi = dev_id;
> +	u32 reg;
> +
> +	reg = readl(xspi->iobase + XSPI_FR);
> +	if (reg & XSPI_FR_TFF) {
> +		/* Clear interrupt */
> +		writel(XSPI_FR_TFF, xspi->iobase + XSPI_FR);
> +		complete(&xspi->c);
> +		return IRQ_HANDLED;
> +	} else {
> +		return IRQ_NONE;
> +	}

else branch is not neccesary,
you can directly

	return IRQ_NONE;

I remember there should be a warning for it.

> +}
> +
> +static int nxp_xspi_check_buswidth(struct nxp_xspi *xspi, u8 width)
> +{
> +	return (is_power_of_2(width) && width <= 8) ? 0 : -EOPNOTSUPP;
> +}
> +
...
> +	} else {
> +		nxp_xspi_disable_ddr(xspi);
> +		xspi->flags &= ~XSPI_DTR_PROTO;
> +	}
> +	rate = min(xspi->support_max_rate, op->max_freq);

use min_t

...
> +
...
> +
> +static int nxp_xspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +{
> +	struct nxp_xspi *xspi = spi_controller_get_devdata(mem->spi->controller);
> +	void __iomem *base = xspi->iobase;
> +	u32 reg;
> +	int err;
> +
> +	guard(mutex)(&xspi->lock);
> +
> +	err = pm_runtime_get_sync(xspi->dev);
> +	if (err < 0) {
> +		dev_err(xspi->dev, "Failed to enable clock %d\n", __LINE__);
> +		return err;
> +	}

Now you can use

	ACQUIRE(pm_runtime_active_auto, pm)(xspi->dev;
        if ((ret = ACQUIRE_ERR(pm_runtime_active_auto, &pm)))
                return ret;

So below pm_runtime_put_autosuspend() can be removed, actually
you missed pm_runtime_put_autosuspend() at below return err after
readl_poll_timeout().


> +
> +	/* Wait for controller being ready. */
> +	err = readl_poll_timeout(base + XSPI_SR, reg,
> +			      !(reg & XSPI_SR_BUSY), 1, POLL_TOUT);
> +	if (err) {
> +		dev_err(xspi->dev, "SR keeps in BUSY!");
> +		return err;
> +	}
> +
...
> +
> +	.swap16 = true,
> +};
> +
> +static void nxp_xspi_cleanup(void *data)
> +{
> +	struct nxp_xspi *xspi = data;
> +
> +	pm_runtime_get_sync(xspi->dev);
> +
> +	/* Disable interrupt */
> +	writel(0, xspi->iobase + XSPI_RSER);
> +	/* Clear all the internal logic flags */
> +	writel(0xFFFFFFFF, xspi->iobase + XSPI_FR);
> +	/* Disable the hardware */
> +	writel(XSPI_MCR_MDIS, xspi->iobase + XSPI_MCR);
> +
> +	clk_disable_unprepare(xspi->clk);
> +
> +	if (xspi->ahb_addr)
> +		iounmap(xspi->ahb_addr);
> +
> +	pm_runtime_disable(xspi->dev);
> +	pm_runtime_put_noidle(xspi->dev);

if use pm_runtime_put() here, needn't call clk_disable_unprepare() here.
after use runtime pm to manage clocks, it'd better use it all place.

> +}
> +
> +static int nxp_xspi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct spi_controller *ctlr;
> +	struct nxp_xspi *xspi;
> +	struct resource *res;
> +	int ret, irq;
> +
> +	ctlr = devm_spi_alloc_host(dev, sizeof(*xspi));
> +	if (!ctlr)
> +		return -ENOMEM;
> +
> +	ctlr->mode_bits = SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL |
> +			  SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL;
> +
> +	xspi = spi_controller_get_devdata(ctlr);
> +	xspi->dev = dev;
> +	xspi->devtype_data = device_get_match_data(dev);
> +	if (!xspi->devtype_data)
> +		return -ENODEV;
> +
> +	platform_set_drvdata(pdev, xspi);
> +
> +	/* Find the resources - configuration register address space */
> +	xspi->iobase = devm_platform_ioremap_resource_byname(pdev, "base");
> +	if (IS_ERR(xspi->iobase))
> +		return PTR_ERR(xspi->iobase);
> +
> +	/* Find the resources - controller memory mapped space */
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mmap");
> +	if (!res)
> +		return -ENODEV;
> +
> +	/* Assign memory mapped starting address and mapped size. */
> +	xspi->memmap_phy = res->start;
> +	xspi->memmap_phy_size = resource_size(res);
> +
> +	/* Find the clocks */
> +	xspi->clk = devm_clk_get(dev, "per");
> +	if (IS_ERR(xspi->clk))
> +		return PTR_ERR(xspi->clk);
> +
> +	/* Find the irq */
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return dev_err_probe(dev, irq,  "Failed to get irq source");
> +
> +	pm_runtime_set_autosuspend_delay(dev, XSPI_RPM_TIMEOUT);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_enable(dev);

devm_pm_runtime_enable(dev)

Frank
> +
> +	/* Enable clock */
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to enable clock");
> +
> +	/* Clear potential interrupt by write xspi errstat */
> +	writel(0xFFFFFFFF, xspi->iobase + XSPI_ERRSTAT);
> +	writel(0xFFFFFFFF, xspi->iobase + XSPI_FR);
> +
> +	nxp_xspi_default_setup(xspi);
> +
> +	ret = pm_runtime_put_sync(dev);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to disable clock");
> +
> +	ret = devm_request_irq(dev, irq,
> +			nxp_xspi_irq_handler, 0, pdev->name, xspi);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to request irq");
> +
> +	ret = devm_mutex_init(dev, &xspi->lock);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, nxp_xspi_cleanup, xspi);
> +	if (ret)
> +		return ret;
> +
> +	ctlr->bus_num = -1;
> +	ctlr->num_chipselect = NXP_XSPI_MAX_CHIPSELECT;
> +	ctlr->mem_ops = &nxp_xspi_mem_ops;
> +	ctlr->mem_caps = &nxp_xspi_mem_caps;
> +	ctlr->dev.of_node = dev->of_node;
> +
> +	return devm_spi_register_controller(dev, ctlr);
> +}
> +
> +static int nxp_xspi_runtime_suspend(struct device *dev)
> +{
> +	struct nxp_xspi *xspi = dev_get_drvdata(dev);
> +	u32 reg;
> +
> +	reg = readl(xspi->iobase + XSPI_MCR);
> +	reg |= XSPI_MCR_MDIS;
> +	writel(reg, xspi->iobase + XSPI_MCR);
> +
> +	clk_disable_unprepare(xspi->clk);
> +
> +	return 0;
> +}
> +
> +static int nxp_xspi_runtime_resume(struct device *dev)
> +{
> +	struct nxp_xspi *xspi = dev_get_drvdata(dev);
> +	u32 reg;
> +	int ret;
> +
> +	ret = clk_prepare_enable(xspi->clk);
> +	if (ret)
> +		return ret;
> +
> +	reg = readl(xspi->iobase + XSPI_MCR);
> +	reg &= ~XSPI_MCR_MDIS;
> +	writel(reg, xspi->iobase + XSPI_MCR);
> +
> +	return 0;
> +}
> +
> +static int nxp_xspi_suspend(struct device *dev)
> +{
> +	int ret;
> +
> +	ret = pinctrl_pm_select_sleep_state(dev);
> +	if (ret) {
> +		dev_err(dev, "select flexspi sleep pinctrl failed!\n");
> +		return ret;
> +	}
> +
> +	return pm_runtime_force_suspend(dev);
> +}
> +
> +static int nxp_xspi_resume(struct device *dev)
> +{
> +	struct nxp_xspi *xspi = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = pm_runtime_force_resume(dev);
> +	if (ret)
> +		return ret;
> +
> +	nxp_xspi_default_setup(xspi);
> +
> +	ret = pinctrl_pm_select_default_state(dev);
> +	if (ret)
> +		dev_err(dev, "select flexspi default pinctrl failed!\n");
> +
> +	return ret;
> +}
> +
> +
> +static const struct dev_pm_ops nxp_xspi_pm_ops = {
> +	RUNTIME_PM_OPS(nxp_xspi_runtime_suspend, nxp_xspi_runtime_resume, NULL)
> +	SYSTEM_SLEEP_PM_OPS(nxp_xspi_suspend, nxp_xspi_resume)
> +};
> +
> +static const struct of_device_id nxp_xspi_dt_ids[] = {
> +	{ .compatible = "nxp,imx94-xspi", .data = (void *)&imx94_data, },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, nxp_xspi_dt_ids);
> +
> +static struct platform_driver nxp_xspi_driver = {
> +	.driver = {
> +		.name	= "nxp-xspi",
> +		.of_match_table = nxp_xspi_dt_ids,
> +		.pm =   pm_ptr(&nxp_xspi_pm_ops),
> +	},
> +	.probe          = nxp_xspi_probe,
> +};
> +module_platform_driver(nxp_xspi_driver);
> +
> +MODULE_DESCRIPTION("NXP xSPI Controller Driver");
> +MODULE_AUTHOR("NXP Semiconductor");
> +MODULE_AUTHOR("Haibo Chen <haibo.chen@....com>");
> +MODULE_LICENSE("GPL");
>
> --
> 2.34.1
>

Powered by blists - more mailing lists