| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<DU0PR04MB9496647CDAB3D15A5245693390D1A@DU0PR04MB9496.eurprd04.prod.outlook.com>
Date: Tue, 25 Nov 2025 02:24:23 +0000
From: Bough Chen <haibo.chen@....com>
To: Frank Li <frank.li@....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"
<linux-spi@...r.kernel.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH v5 2/2] spi: add driver for NXP XSPI controller
> On Mon, Nov 24, 2025 at 05:25:22PM +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.
>
> Nit: please wrap at 75 char if need respin patch.
Okay.
>
> >
> > Current driver based on EENV0, which means system already give
> > EENV0 right to linux.
> >
> ...
> > + /* Wait for the CLR_RXF clear */
> > + ret = readl_poll_timeout(base + XSPI_MCR, reg,
> > + !(reg & XSPI_MCR_CLR_RXF), 1, POLL_TOUT_US);
> > + WARN_ON(ret);
> > +}
> > +
> > +static int nxp_xspi_do_op(struct nxp_xspi *xspi, const struct
> > +spi_mem_op *op) {
> > + void __iomem *base = xspi->iobase;
> > + int watermark, err = 0;
> > + u32 reg, len;
> > +
> > + len = op->data.nbytes;
> > + if (op->data.nbytes && op->data.dir == SPI_MEM_DATA_OUT) {
> > + /* Clear the TX FIFO. */
> > + reg = readl(base + XSPI_MCR);
> > + reg |= XSPI_MCR_CLR_TXF;
> > + writel(reg, base + XSPI_MCR);
> > + /* Wait for the CLR_TXF clear */
> > + err = readl_poll_timeout(base + XSPI_MCR, reg,
> > + !(reg & XSPI_MCR_CLR_TXF), 1, POLL_TOUT_US);
> > + WARN_ON(err);
>
> Is it enough by just print a warning? If timeout happen, is it to continue below
> operation? and other void helper function (such as above WARN_ON(ret);)
>
> If clean FIFO fail, suppose garbage data still is in FIFO. It think it should return err
> to user space to indicate op failure.
Return err seems safer, let me check whether other places also.
>
> Or are you sure our hardware can tolerant this error.
>
> > + /* Cover the no 4bytes alignment data length */
> > + watermark = (xspi->devtype_data->txfifo - ALIGN(op->data.nbytes, 4))
> / 4 + 1;
> > + reg = FIELD_PREP(XSPI_TBCT_WMRK_MASK, watermark);
> > + writel(reg, base + XSPI_TBCT);
> > + /*
> > + * According to the RM, for TBDR register, a write transaction on the
> > + * flash memory with data size of less than 32 bits leads to the
> removal
> > + * of one data entry from the TX buffer. The valid bits are used and
> the
> > + * rest of the bits are discarded.
> > + * But for data size large than 32 bits, according to test, for no 4bytes
> > + * alignment data, the last 1~3 bytes will lost, because TX buffer use
> > + * 4 bytes entries.
> > + * So here adjust the transfer data length to make it 4bytes
> alignment.
> > + * then will meet the upper watermark setting, trigger the 4bytes
> entries
> > + * pop out.
> > + * Will use extra 0xff to append, refer to nxp_xspi_fill_txfifo().
> > + */
> > + if (len > 4)
> > + len = ALIGN(op->data.nbytes, 4);
> > +
> > + } else if (op->data.nbytes && op->data.dir == SPI_MEM_DATA_IN) {
> > + /* Invalid RXFIFO first */
> > + reg = readl(base + XSPI_MCR);
> > + reg |= XSPI_MCR_CLR_RXF;
> > + writel(reg, base + XSPI_MCR);
> > + /* Wait for the CLR_RXF clear */
> > + err = readl_poll_timeout(base + XSPI_MCR, reg,
> > + !(reg & XSPI_MCR_CLR_RXF), 1, POLL_TOUT_US);
> > + WARN_ON(err);
> > + reg = FIELD_PREP(XSPI_RBCT_WMRK_MASK, 31);
> > + writel(reg, base + XSPI_RBCT);
> > + }
> > +
> > + init_completion(&xspi->c);
> > +
> > + /* Config the data address */
> > + writel(op->addr.val + xspi->memmap_phy, base + XSPI_SFP_TG_SFAR);
> > +
> > + /* Config the data size and lut id, trigger the transfer */
> > + reg = FIELD_PREP(XSPI_SFP_TG_IPCR_SEQID_MASK, XSPI_SEQID_LUT) |
> > + FIELD_PREP(XSPI_SFP_TG_IPCR_IDATSZ_MASK, len);
> > + writel(reg, base + XSPI_SFP_TG_IPCR);
> > +
> > + if (op->data.nbytes && op->data.dir == SPI_MEM_DATA_OUT)
> > + nxp_xspi_fill_txfifo(xspi, op);
> > +
> > + /* Wait for the interrupt. */
> > + if (!wait_for_completion_timeout(&xspi->c, msecs_to_jiffies(1000)))
> > + err = -ETIMEDOUT;
> > +
> > + /* Invoke IP data read, if request is of data read. */
> > + if (!err && op->data.nbytes && op->data.dir == SPI_MEM_DATA_IN)
> > + nxp_xspi_read_rxfifo(xspi, op);
> > +
> > + return err;
> > +}
> ...
> > +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_MS);
> > + pm_runtime_use_autosuspend(dev);
> > + devm_pm_runtime_enable(dev);
>
> need check return value here.
Okay.
Regards
Haibo Chen
>
> Frank
> > +
> > + PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
> > + ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> > + if (ret)
> > + 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 = 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); }
> > +
> ...
> > +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