Hi Tejas,
+Ian Roberts,
(Apologies for the double mail--I accidentally dropped the u-boot
mailing list in version 1)
We have been developing OSPI support against the same Cadence IP in
the Analog parts (SC594 and SC598) and we have some specific feedback
as this patch series will affect the driver's functionality on our
platform and require changes to our upcoming patches to work together.
> {
> @@ -353,6 +649,9 @@ static int cadence_spi_mem_exec_op(struct spi_slave *spi,
> break;
> }
>
> + if (op->cmd.dtr)
> + err = cadence_spi_setup_ddrmode(spi, op);
> +
> return err;
> }
This appears to trigger calibration with the first spi command
received with the dtr flag set (cadence_spi_mem_exec_op ->
cadence_spi_setup_ddrmode -> cadence_spi_setdlldelay ->
cadence_qspi_rx_dll_tuning). However the calibration code assumes that
this command is a read ID command, because code has been added to
spi_nor_micro_octal_dtr_enable specifically to read the ID first. This
will cause problems if we are adding support to another chip and are
unaware that this host SPI controller detail needs to be added to the
DTR enable function inside spi-nor-core.
in cadence_qspi_rx_dll_tuning:
> + id_matched = true;
> + for (j = 0; j < op->data.nbytes; j++) {
> + if (spi->device_id[j] != id[j]) {
> + id_matched = false;
> + break;
> + }
> + }
This should probably just be a call to memcmp.
Additionally, this assumes that the ID has already been read into
spi->device_id, which happens in spi_nor_read_id. We believe that
setup_ddrmode should be self contained, for example generating a
non-dtr ID read and store (locally) first, then reconfiguring for DTR
and issuing a DTR read.
Based on our experience with the ADI EZKIT, reading a 3 or 6 byte ID
is not always sufficient to perform robust calibration, as some data
lines may end up being "calibrated" against a constant value. For
example, the ISSI flash chips capable of DTR include a "data learning
pattern" register which could be used to ensure that calibration is
performed against a 0 to 1 transition and a 1 to 0 transition. Such
functionality will be chip-specific, so we need to a way to connect
what happens in the dtr enable function in spi-nor-core with what
happens inside the driver (possibly another ops function).
in priv_setup_ddrmode:
> + /* Disable DAC mode */
> + if (priv->use_dac_mode) {
> + clrbits_le32(regbase + CQSPI_REG_CONFIG,
> + CQSPI_REG_CONFIG_DIRECT);
> + priv->use_dac_mode = false;
> + }
Why is DAC forcefully disabled? On our platform we can only achieve
max PHY mode performance with DAC enabled. Can we provide a
platform-tunable flag to control this?
In priv_setup_ddrmode:
> + clrsetbits_le32(regbase + CQSPI_REG_RD_DATA_CAPTURE,
> + (CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK <<
> + CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB),
> + CQSPI_REG_READCAPTURE_DQS_ENABLE);
DQS is not required for PHY operation. Some chips can operate in DDR
mode and don't have a DQS line; we have one of these on our boards.
Please make this configurable
> +__weak int cadence_qspi_versal_set_dll_mode(struct udevice *dev)
> +{
> + return -ENOTSUPP;
> +}
> +
Setting the DLL mode will not be a Versal-specific function. Can we
rename this (and other Versal-specific names) in order to make it
clearer that each platform integrating this IP is providing its own
implementation for this function, rather than it being a uniquely
versal-related capability? For DLL mode in particular, it may be
sufficient to check for a device tree property, which can be included
in the generic driver without any platform-specific functions.
In cadence_spi_setdlldelay:
> + priv->extra_dummy = false;
> + for (extra_dummy = 0; extra_dummy <= 1; extra_dummy++) {
> + if (extra_dummy)
> + priv->extra_dummy = true;
> +
> + rxtap = cadence_qspi_rx_dll_tuning(spi, op, txtap, extra_dummy);
> + if (extra_dummy && rxtap < 0) {
> + printf("Failed RX dll tuning\n");
> + return rxtap;
> + }
> + }
Can you please explain the extra dummy cycles? Why are more needed
outside of what is called out for in flash datasheets? This looks to
us like magic compensating for a calibration failure or a
configuration failure where the flash chip is not providing the
expected number of dummy cycles.
in cadence_qspi_rx_dll_tuning:
> + ret = cadence_qspi_apb_command_read_setup(priv, op);
> + if (!ret) {
> + ret = cadence_qspi_apb_command_read(priv, op);
> + if (ret < 0) {
> + printf("error %d reading JEDEC ID\n", ret);
> + return ret;
> + }
> +
I see that this cannot use cadence_spi_mem_exec_op() because it would
recursively trigger itself as the calibration was not yet completed,
but as a result this does not support all of the read modes that the
device could be configured for. I think this is further reason to have
a specialized function for enabling DTR rather than triggering it on
the first DTR operation.
> +static int cadence_spi_child_pre_probe(struct udevice *bus)
> +{
> + struct spi_slave *slave = dev_get_parent_priv(bus);
> +
> + slave->bytemode = SPI_4BYTE_MODE;
> +
> + return 0;
> +}
> +
> +__weak int cadence_qspi_versal_set_dll_mode(struct udevice *dev)
> +{
> + return -ENOTSUPP;
> +}
Although it is likely that any flash supporting DTR uses 4 byte
addressing, we should not set it here.
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index faf02c7778..5895b5de09 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
Can we split the micron-specific changes into a separate patch? I
realize currently that they are tightly coupled with the calibration
implementation, but after changing the calibration implementation
perhaps this will be possible.
Some additional comments not specific to any part of the code:
How does this patch address the case when the calibrated frequency is
faster than the non-calibrated mode can run at? For example, if
spi-max-frequency is 125MHz, but single IO operation breaks down at
speeds greater than 50MHz? (This also happens on our board). Also, the
calibration algorithm doesn't appear to re-calibrate the read capture
delay register, which is required whenever the clock or IO mode
changes.
At Timesys we have a set of patches that implement DDR and PHY
calibration for the same Cadence IP that presents different solutions
to the above issues. However, our patches only support bypass mode
calibration and direct mode, because they are the only options that
work on our hardware. Would you be interested in looking at our
version and working together to create a new version of this patchset
that can support both of our platforms at once?
Thanks,
Greg
--
Greg Malysa
Timesys Corporation
