Re: [PATCH 1/2] dt-bindings: spi: Add for Nuvoton MA35D1 SoC QSPI Controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Sending `op->cmd.opcode` by address fails on Big-Endian architectures and ignores opcode length.
- [High] Failure to unconditionally drain the RX FIFO during TX-only transfers can stall the SPI controller.
- [Medium] Silent mapping of invalid chip select numbers to `SS1`.
- [Medium] Missing `num-cs` constraint in the device tree binding allows invalid configurations.
- [Medium] Missing 'resets' property in the YAML schema properties list.
--

commit 4a6810e7446469e1e2c0b488d225f75f8420f815
Author: Chi-Wen Weng <cwweng.linux@gmail.com>

dt-bindings: spi: Add for Nuvoton MA35D1 SoC QSPI Controller

This series introduces the device tree bindings and driver support for the
Nuvoton MA35D1 Quad SPI controller. The driver implements standard master
mode operations, quad I/O, and SPI memory operations.

> diff --git a/Documentation/devicetree/bindings/spi/nuvoton,ma35d1-qspi.yaml b/Documentation/devicetree/bindings/spi/nuvoton,ma35d1-qspi.yaml
> new file mode 100644
> index 0000000000000..f7b9cb52d8e59
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/nuvoton,ma35d1-qspi.yaml
[ ... ]
> +properties:
> +  compatible:
> +    const: nuvoton,ma35d1-qspi
> +
> +  reg:
> +    maxItems: 1

[Severity: Medium]
Should the 'resets' property be included in this schema?

Since the MA35D1 hardware contains reset control lines for QSPI, omitting
'resets' means any board device tree that accurately specifies the hardware's
reset line will fail schema validation due to unevaluatedProperties: false.

[Severity: Medium]
Does this binding need a 'num-cs' constraint?

The MA35D1 controller physically supports a maximum of 2 chip selects. Because
the binding references spi-controller.yaml without capping num-cs, a device
tree specifying num-cs = <4> will pass validation and could cause
out-of-bounds indexing in the driver.

[ ... ]
> diff --git a/drivers/spi/spi-ma35d1-qspi.c b/drivers/spi/spi-ma35d1-qspi.c
> new file mode 100644
> index 0000000000000..119c06464cb01
> --- /dev/null
> +++ b/drivers/spi/spi-ma35d1-qspi.c
[ ... ]
> +static int nuvoton_qspi_txrx(struct nuvoton_qspi *qspi, const void *txbuf,
> +			     void *rxbuf, unsigned int len)
> +{
[ ... ]
> +	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);
> +
> +		if (rxbuf) {
> +			ret = nuvoton_qspi_wait_rx_not_empty(qspi);
> +			if (ret) {
> +				dev_err(qspi->dev, "RX FIFO empty timeout\n");
> +				return ret;
> +			}
> +
> +			val = nuvoton_qspi_read(qspi, NUVOTON_QSPI_RX_OFFSET);
> +			nuvoton_qspi_rx_word(rxbuf, i, val, qspi->bits_per_word);
> +		}
> +	}

[Severity: High]
Can this stall the SPI controller on large TX-only transfers?

Because SPI is a full-duplex shift register, every byte transmitted shifts a
byte into the RX FIFO. By only reading the RX FIFO when rxbuf is not NULL, a
long TX-only operation (like a 256-byte page program) will fill and overflow
the RX FIFO.

On many controllers this stalls the shift register and causes a hang or timeout
in nuvoton_qspi_wait_tx_not_full(). Should the RX FIFO be unconditionally
drained on every cycle?

[ ... ]
> +static void nuvoton_qspi_set_cs(struct spi_device *spi, bool enable)
> +{
> +	struct nuvoton_qspi *qspi = spi_controller_get_devdata(spi->controller);
> +	unsigned int cs = spi_get_chipselect(spi, 0);
> +	u32 mask;
> +	u32 val;
> +
> +	if (cs == 0)
> +		mask = NUVOTON_QSPI_SSCTL_SS0_MASK;
> +	else
> +		mask = NUVOTON_QSPI_SSCTL_SS1_MASK;

[Severity: Medium]
Is it safe to assume any non-zero chip select maps to SS1?

If an incorrect device tree specifies num-cs = <4>, the SPI core might pass
cs >= 2 here. This branch will silently map all invalid chip selects to
SS1_MASK.

If another peripheral is connected to SS1, could this cause a bus collision?

[ ... ]
> +static int nuvoton_qspi_mem_exec_op(struct spi_mem *mem,
> +				    const struct spi_mem_op *op)
> +{
[ ... ]
> +	nuvoton_qspi_set_cs(spi, true);
> +
> +	nuvoton_qspi_set_bus_width(qspi, op->cmd.buswidth, SPI_MEM_DATA_OUT);
> +	ret = nuvoton_qspi_txrx(qspi, &op->cmd.opcode, NULL, 1);

[Severity: High]
What happens here on big-endian systems or with multi-byte opcodes?

op->cmd.opcode is a 16-bit integer, but it's passed by address with a
hardcoded length of 1. On big-endian systems, reading a 16-bit integer via a
byte pointer reads the most significant byte (which is 0x00 for a typical
1-byte opcode like 0x03), breaking flash communication.

Additionally, this ignores op->cmd.nbytes, which would break operations that
require more than 1 byte for the opcode.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603043551.1062112-1-cwweng.linux@gmail.com?part=1