Re: [PATCH v2 2/2] spi: ma35d1-qspi: Add Nuvoton MA35D1 QSPI controller support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The TX/RX loop unconditionally waits for the RX FIFO to fill, stalling and timing out during half-duplex transmit-only phases (Quad/Dual OUT).
- [High] A fast-read race condition exists in nuvoton_qspi_reset_fifo where the status bit is polled before the hardware asserts it.
- [Medium] SPI_CS_HIGH support is fundamentally broken due to omission from ctlr->mode_bits.
--

commit 822bfa8b5549aa56a2619eaa2c69adefdc89e29f
Author: Chi-Wen Weng <cwweng.linux@gmail.com>

spi: ma35d1-qspi: Add Nuvoton MA35D1 QSPI controller support
    
Add SPI controller driver support for the Nuvoton MA35D1 Quad SPI controller.

> diff --git a/drivers/spi/spi-ma35d1-qspi.c b/drivers/spi/spi-ma35d1-qspi.c
> new file mode 100644
> index 0000000000000..93b60b6142e53
> --- /dev/null
> +++ b/drivers/spi/spi-ma35d1-qspi.c

[ ... ]

> +static int nuvoton_qspi_reset_fifo(struct nuvoton_qspi *qspi)
> +{
> +	u32 val;
> +
> +	val = nuvoton_qspi_read(qspi, NUVOTON_QSPI_FIFOCTL_OFFSET);
> +	val |= NUVOTON_QSPI_FIFOCTL_TXRST_MASK |
> +	       NUVOTON_QSPI_FIFOCTL_RXRST_MASK;
> +	nuvoton_qspi_write(qspi, val, NUVOTON_QSPI_FIFOCTL_OFFSET);
> +
> +	/* FIFO reset is extremely fast, safe to keep atomic for this micro-wait */
> +	return readl_poll_timeout_atomic(qspi->regs + NUVOTON_QSPI_STATUS_OFFSET,
> +					 val,
> +					 !(val & NUVOTON_QSPI_STATUS_TXRXRST_MASK),
> +					 1, NUVOTON_QSPI_TIMEOUT_US);
> +}

[Severity: High]
Could this read a stale 0 before the hardware has time to assert the
TXRXRST_MASK bit?

If the IP takes a few clock cycles to propagate the reset and assert the
bit to 1, this polling might return success prematurely on the very first
iteration. This could lead to data loss if we start writing to the FIFOs
while they are still mid-reset.

[ ... ]

> +static void nuvoton_qspi_set_bus_width(struct nuvoton_qspi *qspi,
> +				       unsigned int buswidth,
> +				       enum spi_mem_data_dir dir)
> +{
> +	u32 ctl = 0;
> +
> +	if (buswidth == 4)
> +		ctl |= NUVOTON_QSPI_CTL_QUADIOEN_MASK;
> +	else if (buswidth == 2)
> +		ctl |= NUVOTON_QSPI_CTL_DUALIOEN_MASK;
> +
> +	if (buswidth > 1 && dir == SPI_MEM_DATA_OUT)
> +		ctl |= NUVOTON_QSPI_CTL_DATDIR_MASK;

[ ... ]

> +static int nuvoton_qspi_txrx(struct nuvoton_qspi *qspi, const void *txbuf,
> +			     void *rxbuf, unsigned int len)
> +{

[ ... ]

> +	/*
> +	 * Use conservative word-by-word PIO access. This keeps the initial driver
> +	 * simple and avoids relying on FIFO threshold interrupts or DMA support.
> +	 */
> +	for (i = 0; i < words; i++) {
> +		ret = nuvoton_qspi_wait_tx_not_full(qspi);
> +		if (ret) {
> +			dev_err(qspi->dev, "TX FIFO full timeout\n");
> +			return ret;
> +		}
> +
> +		nuvoton_qspi_write(qspi, nuvoton_qspi_tx_word(txbuf, i,
> +							      qspi->bits_per_word),
> +				   NUVOTON_QSPI_TX_OFFSET);
> +
> +		ret = nuvoton_qspi_wait_rx_not_empty(qspi);

[Severity: High]
Will this loop stall and time out during half-duplex transmit-only phases?

When the bus width is > 1 and direction is OUT, NUVOTON_QSPI_CTL_DATDIR_MASK
is set in nuvoton_qspi_set_bus_width(). This typically places the controller
in an OUT-only state and disables the RX path so no data enters the RX FIFO.

Because the loop here unconditionally calls nuvoton_qspi_wait_rx_not_empty()
for every transmitted word, it seems this will stall until the 10,000 us
timeout is reached, causing multi-IO transmit operations to fail.

[ ... ]

> +static void nuvoton_qspi_mem_set_cs(struct spi_device *spi, bool enable)
> +{
> +	struct nuvoton_qspi *qspi = spi_controller_get_devdata(spi->controller);
> +	bool assert = enable;
> +
> +	if (spi->mode & SPI_CS_HIGH)
> +		assert = !assert;
> +
> +	nuvoton_qspi_set_cs_level(qspi, spi_get_chipselect(spi, 0), assert);
> +}

[ ... ]

> +static int nuvoton_qspi_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	ctlr->bits_per_word_mask = SPI_BPW_MASK(8) | SPI_BPW_MASK(16) |
> +				   SPI_BPW_MASK(32);
> +	ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST |
> +			  SPI_RX_DUAL | SPI_TX_DUAL |
> +			  SPI_RX_QUAD | SPI_TX_QUAD;

[Severity: Medium]
Does the omission of SPI_CS_HIGH from mode_bits prevent active-high devices
from probing?

The driver implements logic to support active-high chip selects in
nuvoton_qspi_mem_set_cs() by inspecting spi->mode & SPI_CS_HIGH.

However, since SPI_CS_HIGH is not included in ctlr->mode_bits, the SPI
core subsystem's spi_setup() will immediately return -EINVAL if a device
tree node requests spi-cs-high. This renders the active-high logic
unreachable dead code.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608025009.1504971-1-cwweng@nuvoton.com?part=2