Re: [PATCH v3 09/13] spi: cadence-quadspi: reject 2-byte-address DDR ops on PHY-tunable hardware

[Santhosh Kumar K]

Hello Miquel,

On 28/05/26 14:31, Miquel Raynal wrote:
> On 27/05/2026 at 23:25:23 +0530, Santhosh Kumar K <s-k6@xxxxxx> wrote:
>
> > Erratum i2383 affects the AM654 OSPI controller: in PHY DDR mode,
> > operations with a 2-byte address cause an internal state machine to
> > mis-compare the transmitted address byte count against 1 instead of 2,
> > locking up the address phase. [0]
> >
> > Add a CQSPI_NO_2BYTE_ADDR_PHY_DDR quirk flag and set it on the am654_ospi
> > platform data. In cqspi_supports_mem_op(), when a controller carries this
> > quirk and has PHY tuning support, reject DDR operations that use 2-byte
> > addressing.
> >
> > [0] https://www.ti.com/lit/er/sprz544c/sprz544c.pdf
> >
> > Signed-off-by: Santhosh Kumar K <s-k6@xxxxxx>
> > ---
> >   drivers/spi/spi-cadence-quadspi.c | 15 ++++++++++++++-
> >   1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> > index 508bc5bc4ab5..72208d376305 100644
> > --- a/drivers/spi/spi-cadence-quadspi.c
> > +++ b/drivers/spi/spi-cadence-quadspi.c
> > @@ -49,6 +49,7 @@ static_assert(CQSPI_MAX_CHIPSELECT <= SPI_DEVICE_CS_CNT_MAX);
> >   #define CQSPI_DISABLE_RUNTIME_PM	BIT(10)
> >   #define CQSPI_NO_INDIRECT_MODE		BIT(11)
> >   #define CQSPI_HAS_WR_PROTECT		BIT(12)
> > +#define CQSPI_NO_2BYTE_ADDR_PHY_DDR	BIT(13)
> >   
> >   /* Capabilities */
> >   #define CQSPI_SUPPORTS_OCTAL		BIT(0)
> > @@ -1627,6 +1628,18 @@ static bool cqspi_supports_mem_op(struct spi_mem *mem,
> >   		if (op->data.nbytes && op->data.buswidth != 8)
> >   			return false;
> >   
> > +		/*
> > +		 * Erratum i2383: In PHY DDR mode, 2-byte addressing causes an
> > +		 * internal state machine to mis-compare the transmitted
> > +		 * address byte count against 1 instead of 2, locking up the
> > +		 * address phase. Reject such ops on controllers that need it.
> > +		 */
> > +		if (cqspi->ddata &&
> > +		    (cqspi->ddata->quirks & CQSPI_NO_2BYTE_ADDR_PHY_DDR)) {
> > +			if (op->addr.nbytes == 2 && cqspi->ddata->execute_tuning)
> > +				return false;
> > +		}
>
> I don't think this is a valid approach. What we want is to prevent
> tuning in octal DTR mode with 2 bytes addressing, instead of preventing
> reads/writes in octal DTR modes after tuning. Have you tried on an AM62A LP
> SK? I bet probe fails..
>
> The quirk should be handled at the beginning of the tuning procedure, so
> we skip tuning entirely in this case.

I see your point. However, in my testing on AM62Ax LP SK, the controller
and the flash probes and operates correctly in 8S PHY mode.

The reason I handled this in supports_op() is that, for these devices,
simply skipping tuning when a 2-byte DDR operation is selected may not
lead to the best achievable operating point. The performance ordering I
measured is:

8S non-PHY < 8D non-PHY < 8S PHY

So falling back from 8D PHY to 8D non-PHY would leave performance on the
table compared to selecting and tuning an 8S operation.

That said, I agree that encoding this as an operation support
restriction is probably not the right place to handle it. I'll rework
the initialization flow so that operation selection can take
PHY-achievable frequencies and controller-specific restrictions such as
i2383 into account when choosing the final operations.

Let me prototype this approach and will report back with the results.

Thanks,
Santhosh.

> Thanks,
> Miquèl