[PATCH 6/7] spi: atmel-quadspi: Add support for classic SPI mode

[Alexander Dahl]

Hello Eugen,

thanks for looking at this.  My remarks below.

Am Thu, Jan 30, 2025 at 11:49:30AM +0200 schrieb Eugen Hristev:
> Hello Alex,
> 
> On 1/23/25 14:12, Alexander Dahl wrote:
> > The qspi controller on sama5d2 and sam9x60 supports "classic" SPI mode
> > without spi-mem enhancements and accelerations, very similar to the old
> > SPI controller on sam9g20 or the modern flexcom controllers of the same
> > SoC family.
> > 
> > Register interface differs somewhat, especially because only one
> > hardware controlled CS line is supported.  Some fields are missing, some
> > are in different registers, but in principal it works similar.  So code
> > is very much inspired by the old atmel-spi driver.
> > 
> > Tested on sam9x60 with a non-mainline driver to configure an FPGA.

[…]

> > +static int atmel_qspi_xfer(struct udevice *dev, unsigned int bitlen,
> > +			   const void *dout, void *din, unsigned long flags)
> > +{
> > +	struct udevice *bus = dev_get_parent(dev);
> > +	struct atmel_qspi *aq = dev_get_priv(bus);
> > +	unsigned int len, len_rx, len_tx;
> > +	const u8 *txp = dout;
> > +	u8 *rxp = din;
> > +	u32 reg;
> > +	int ret;
> > +
> > +	if (bitlen == 0)
> > +		goto out;
> > +
> > +	if (bitlen % 8) {
> > +		flags |= SPI_XFER_END;
> > +		goto out;
> > +	}
> > +
> > +	len = bitlen / 8;
> > +
> > +	if (flags & SPI_XFER_BEGIN) {
> > +		ret = atmel_qspi_set_cs(dev, 1);
> > +		if (ret)
> > +			return log_ret(ret);
> > +		reg = atmel_qspi_read(aq, QSPI_RD);
> > +	}
> > +
> > +	for (len_tx = 0, len_rx = 0; len_rx < len; ) {
> > +		u32 status = atmel_qspi_read(aq, QSPI_SR);
> > +		u8 value;
> > +
> > +		if (status & QSPI_SR_OVRES)
> > +			return log_ret(-1);
> 
> Any specific meaning for '-1' ?
> 
> > +
> > +		if (len_tx < len && (status & QSPI_SR_TDRE)) {
> > +			if (txp)
> > +				value = *txp++;
> > +			else
> > +				value = 0;
> > +			atmel_qspi_write(value, aq, QSPI_TD);
> > +			len_tx++;
> > +		}
> > +
> > +		if (status & QSPI_SR_RDRF) {
> > +			value = atmel_qspi_read(aq, QSPI_RD);
> > +			if (rxp)
> > +				*rxp++ = value;
> > +			len_rx++;
> 
> As this loop is finished only when len_rx == len, can it happen that
> this increment is always skipped thus forever looping ?

The whole loop was copied from the old atmel_spi driver.  Code is
untouched since it was introduced with commit 0eafd4b77615 ("dm: at91:
Add driver model support for the spi driver") back in 2016.  The code
of that loop for the dm based driver is the same as for the old non-dm
implementation which dates back to 2008 introduced with commit
60445cb5c3eb ("atmel_spi: Driver for the Atmel SPI controller").

I don't expect to get any answers from the original author(s) anymore? ^^

My interpretation is this is a very simple way to implement the xfer,
without all the complexity of interrupts and/or dma (which is good
enough for my usecase).

The OVRES flag is set in case of an receive data overrun.  What would
be better to return here instead of -1?  And should we port that to
the old driver, too?  The linux driver seems to use -EIO in a
comparable case.

I think in practice this won't loop forever if the microprocessor
works fine.  RDR and TDR are two holding registers with a shift
register in between clocking in and out the bits from and to the data
lines.  You need to write to TDR to be able to read from RDR.  But
this is just my interpretation of the old code here.

> > +		}
> > +	}
> > +
> > +out:
> > +	if (flags & SPI_XFER_END) {
> > +		readl_poll_timeout(aq->regs + QSPI_SR, reg,
> > +				   reg & QSPI_SR_TXEMPTY,
> > +				   ATMEL_QSPI_TIMEOUT);
> > +
> > +		atmel_qspi_write(QSPI_CR_LASTXFER, aq, QSPI_CR);
> > +
> > +		ret = atmel_qspi_set_cs(dev, 0);
> > +		if (ret)
> > +			return log_ret(ret);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int atmel_qspi_set_speed(struct udevice *bus, uint hz)
> >  {
> >  	struct atmel_qspi *aq = dev_get_priv(bus);
> > @@ -1089,6 +1222,23 @@ static int atmel_qspi_probe(struct udevice *dev)
> >  	else
> >  		aq->ops = &atmel_qspi_priv_ops;
> >  
> > +	if (IS_ENABLED(CONFIG_DM_GPIO)) {
> > +		ret = gpio_request_list_by_name(dev, "cs-gpios", aq->cs_gpios,
> > +						ARRAY_SIZE(aq->cs_gpios), 0);
> > +		if (ret < 0) {
> > +			pr_err("Can't get %s gpios! Error: %d", dev->name, ret);
> 
> Is cs-gpios mandatory ? Apparently binding does not enforce it
> 
> > +			return ret;
> > +		}
> > +
> > +		for (int i = 0; i < ARRAY_SIZE(aq->cs_gpios); i++) {
> > +			if (!dm_gpio_is_valid(&aq->cs_gpios[i]))
> > +				continue;
> in here you just skip if gpio isn't valid apparently.
> > +
> > +			dm_gpio_set_dir_flags(&aq->cs_gpios[i],
> > +					      GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
> > +		}
> > +	}

No cs-gpios is not mandatory.  This code is also copied from the
atmel_spi driver.  There are two reasons why this is not mandatory.

First: It should work with native CS handling by the SoC without a
GPIO line acting as CS.  However I could not test this, I need a
different pin for CS in my usecase than the native one.

Second: the probe function is also for the non-classic spi-mem mode
where the native CS handling of the SoC _is_ used.  That mode did not
use cs-gpios before, and won't with this patch.  Should this be made
more clear somewhere?

Greets
Alex