Re: [U-Boot] [PATCH 1/2] spi: cadence_qspi: Move to spi-mem framework
On Thu, Oct 17, 2019 at 3:48 PM wrote:
>
> Hi, Simon, Vignesh,
>
> On 10/17/2019 02:20 PM, Simon Goldschmidt wrote:
> > On Mon, Oct 14, 2019 at 3:27 PM Vignesh Raghavendra wrote:
> >> Current Cadence QSPI driver has few limitations. It assumes all read
> >> operations to be in Quad mode and thus does not support SFDP parsing.
> >> Also, adding support for new mode such as Octal mode would not be
> >> possible with current configuration. Therefore move the driver over to
> >> spi-mem
> >> framework. This has added advantage that driver can be used to support
> >> SPI NAND memories too.
> >> Hence, move driver over to new spi-mem APIs.
> >>
> >> Please note that this gets rid of mode bit setting done when
> >> CONFIG_SPL_SPI_XIP is defined as there does not seem to be any user to
> >> that config option.
> > I just have tried this on an socfgpa cylone5 board with an mt25ql256a, but
> > it does not seem to work: when leaving spi-rx-bus-width and spi-tx-bus-width
> > at 4 in my devicetree, SFDP parsing works, but reading data afterwards
> > produces invalid results (I haven't tested what's wrong there).
> >
> > It works as expected when not parsing SFDP or setting the bus-width to 1.
> > So the change itself probably works, but SFDP parsing is broken?
>
> This can happen if the quad enable method is not correctly set/called. Would
> you
> try the patch form below?
I tried, but of course that didn't help. And I fail to see why it would, unless
you do something wrong in the flash device manufacturer selection.
Doing the patch like below would only make sense if enabling SFDP parsing would
at the same time completely remove the manufacturer selection (e.g. manufacturer
selection in Kconfig would depend on SFDP not being set).
Regards,
Simon
>
> I don't see much benefit in having those guards, especially that we have
> SPI_FLASH_SFDP_SUPPORT defined - it trims most of the SFDP logic.
>
> More, these #ifdef guards are not scalable and with the addition of flashes
> that
> support SFDP the #ifdefs will look uglier and uglier.
>
> Cheers,
> ta
>
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index e5b9899c64b2..3002f97a7342 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -188,7 +188,6 @@ static int read_fsr(struct spi_nor *nor)
> * location. Return the configuration register value.
> * Returns negative if error occurred.
> */
> -#if defined(CONFIG_SPI_FLASH_SPANSION) || defined(CONFIG_SPI_FLASH_WINBOND)
> static int read_cr(struct spi_nor *nor)
> {
> int ret;
> @@ -202,7 +201,6 @@ static int read_cr(struct spi_nor *nor)
>
> return val;
> }
> -#endif
>
> /*
> * Write status register 1 byte
> @@ -591,7 +589,6 @@ erase_err:
> return ret;
> }
>
> -#if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST)
> /* Write status register and ensure bits in mask match written values */
> static int write_sr_and_check(struct spi_nor *nor, u8 status_new, u8 mask)
> {
> @@ -877,7 +874,6 @@ static int stm_is_locked(struct spi_nor *nor, loff_t ofs,
> uint64_t len)
>
> return stm_is_locked_sr(nor, ofs, len, status);
> }
> -#endif /* CONFIG_SPI_FLASH_STMICRO */
>
> static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
> {
> @@ -1116,7 +1112,6 @@ write_err:
> return ret;
> }
>
> -#ifdef CONFIG_SPI_FLASH_MACRONIX
> /**
> * macronix_quad_enable() - set QE bit in Status Register.
> * @nor: pointer to a 'struct spi_nor'
> @@ -1153,9 +1148,7 @@ static int macronix_quad_enable(struct spi_nor *nor)
>
> return 0;
> }
> -#endif
>
> -#if defined(CONFIG_SPI_FLASH_SPANSION) || defined(CONFIG_SPI_FLASH_WINBOND)
> /*
> * Write status Register and configuration register with 2 bytes
> * The first byte will be written to the status register, while the
> @@ -1269,7 +1262,6 @@ static int spansion_no_read_cr_quad_enable(struct
> spi_nor
> *nor)
> }
>
> #endif /* CONFIG_SPI_FLASH_SFDP_SUPPORT */
> -#endif /* CONFIG_SPI_FLASH_SPANSION */
>
> struct spi_nor_read_command {
> u8 num_mode_clocks;
> @@ -1787,22 +1779,16 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
> case BFPT_DWORD15_QER_NONE:
> params->quad_enable = NULL;
> break;
> -#if defined(CONFIG_SPI_FLASH_SPANSION) || defined(CONFIG_SPI_FLASH_WINBOND)
> case BFPT_DWORD15_QER_SR2_BIT1_BUGGY:
> case BFPT_DWORD15_QER_SR2_BIT1_NO_RD:
> params->quad_enable = spansion_no_read_cr_quad_enable;
> break;
> -#endif
> -#ifdef CONFIG_SPI_FLASH_MACRONIX
> case BFPT_DWORD15_QER_SR1_BIT6:
> params->quad_enable = macronix_quad_enable;
> break;
> -#endif
> -#if defined(CONFIG_SPI_FLASH_SPANSION) || defined(CONFIG_SPI_FLASH_WINBOND)
> case BFPT_DWORD15_QER_SR2_BIT1:
> params->quad_enable = spansion_read_cr_quad_enable;
>
Re: [U-Boot] [PATCH 1/2] spi: cadence_qspi: Move to spi-mem framework
Hi Simon,
On 07/11/19 1:25 AM, Simon Goldschmidt wrote:
> Hi Vignesh,
>
> On Thu, Oct 17, 2019 at 2:31 PM Vignesh Raghavendra wrote:
>>
>> Hi Simon,
>>
>> On 17/10/19 4:50 PM, Simon Goldschmidt wrote:
>>> On Mon, Oct 14, 2019 at 3:27 PM Vignesh Raghavendra wrote:
Current Cadence QSPI driver has few limitations. It assumes all read
operations to be in Quad mode and thus does not support SFDP parsing.
Also, adding support for new mode such as Octal mode would not be
possible with current configuration. Therefore move the driver over to
spi-mem
framework. This has added advantage that driver can be used to support
SPI NAND memories too.
Hence, move driver over to new spi-mem APIs.
Please note that this gets rid of mode bit setting done when
CONFIG_SPL_SPI_XIP is defined as there does not seem to be any user to
that config option.
>>>
>>> I just have tried this on an socfgpa cylone5 board with an mt25ql256a, but
>>> it does not seem to work: when leaving spi-rx-bus-width and spi-tx-bus-width
>>> at 4 in my devicetree, SFDP parsing works, but reading data afterwards
>>> produces invalid results (I haven't tested what's wrong there).
>>>
>>
>> Thanks for testing!
>>
>> spi-tx-bus-width = 4 was not supported before so I haven't added support
>> for that mode in this series. That change will be a separate series.
>>
>> Could you try with spi-tx-bus-width = 1 and spi-rx-bus-width = 4 and see
>> if that works?
>>
>> If that does not work then could you disable SFDP parsing (but keep
>> spi-rx-bus-width = 4) and see if that works. This should narrow down
>> whether SFDP parsing is broken or if driver has an issue.
>
> Sorry, I still haven't found the time at work to test this. I'll keep trying.
> Keeping that aside, if this driver is converted to spi-mem, is there any
> chance of getting this into SPL while not getting standard SPI drivers in?
>
> On socfpga, I have a standard SPI driver (designware_spi) enabled but that
> results in both the cadence_qspi and designware_spi being included in SPL
> aside
> well when boot-from-SPI is enabled.
>
> Does that somehow change when cadence_qspi is an spi-mem driver?
>
I dont think so. Only solution I can think of is to create a separate
SPL specific config for designware_spi and disable that for socfpga.
Regards
Vignesh
> Thanks,
> Simon
>
>>
>> Regards
>> Vignesh
>>
>>> It works as expected when not parsing SFDP or setting the bus-width to 1.
>>> So the change itself probably works, but SFDP parsing is broken?
>>>
>>> I did the tests with this applied first:
>>> https://patchwork.ozlabs.org/project/uboot/list/?series=135505
>>>
>>> Regards,
>>> Simon
>>>
>>>
Signed-off-by: Vignesh Raghavendra
---
drivers/spi/cadence_qspi.c | 136 +
drivers/spi/cadence_qspi.h | 9 +--
drivers/spi/cadence_qspi_apb.c | 124 --
3 files changed, 91 insertions(+), 178 deletions(-)
diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
index e2e54cd27723..673a2e9a6c4c 100644
--- a/drivers/spi/cadence_qspi.c
+++ b/drivers/spi/cadence_qspi.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include
#include "cadence_qspi.h"
@@ -34,12 +35,21 @@ static int cadence_spi_write_speed(struct udevice
*bus, uint hz)
return 0;
}
+static int cadence_spi_read_id(void *reg_base, u8 len, u8 *idcode)
+{
+ struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(0x9F, 1),
+ SPI_MEM_OP_NO_ADDR,
+ SPI_MEM_OP_NO_DUMMY,
+ SPI_MEM_OP_DATA_IN(len, idcode,
1));
+
+ return cadence_qspi_apb_command_read(reg_base, );
+}
+
/* Calibration sequence to determine the read data capture delay register
*/
static int spi_calibration(struct udevice *bus, uint hz)
{
struct cadence_spi_priv *priv = dev_get_priv(bus);
void *base = priv->regbase;
- u8 opcode_rdid = 0x9F;
unsigned int idcode = 0, temp = 0;
int err = 0, i, range_lo = -1, range_hi = -1;
@@ -53,8 +63,7 @@ static int spi_calibration(struct udevice *bus, uint hz)
cadence_qspi_apb_controller_enable(base);
/* read the ID which will be our golden value */
- err = cadence_qspi_apb_command_read(base, 1, _rdid,
- 3, (u8 *));
+ err = cadence_spi_read_id(base, 3, (u8 *));
if (err) {
puts("SF: Calibration failed (read)\n");
return err;
@@ -73,8 +82,7 @@ static int spi_calibration(struct udevice *bus, uint hz)
cadence_qspi_apb_controller_enable(base);
Re: [U-Boot] [PATCH 1/2] spi: cadence_qspi: Move to spi-mem framework
Hi Vignesh,
On Thu, Oct 17, 2019 at 2:31 PM Vignesh Raghavendra wrote:
>
> Hi Simon,
>
> On 17/10/19 4:50 PM, Simon Goldschmidt wrote:
> > On Mon, Oct 14, 2019 at 3:27 PM Vignesh Raghavendra wrote:
> >>
> >> Current Cadence QSPI driver has few limitations. It assumes all read
> >> operations to be in Quad mode and thus does not support SFDP parsing.
> >> Also, adding support for new mode such as Octal mode would not be
> >> possible with current configuration. Therefore move the driver over to
> >> spi-mem
> >> framework. This has added advantage that driver can be used to support
> >> SPI NAND memories too.
> >> Hence, move driver over to new spi-mem APIs.
> >>
> >> Please note that this gets rid of mode bit setting done when
> >> CONFIG_SPL_SPI_XIP is defined as there does not seem to be any user to
> >> that config option.
> >
> > I just have tried this on an socfgpa cylone5 board with an mt25ql256a, but
> > it does not seem to work: when leaving spi-rx-bus-width and spi-tx-bus-width
> > at 4 in my devicetree, SFDP parsing works, but reading data afterwards
> > produces invalid results (I haven't tested what's wrong there).
> >
>
> Thanks for testing!
>
> spi-tx-bus-width = 4 was not supported before so I haven't added support
> for that mode in this series. That change will be a separate series.
>
> Could you try with spi-tx-bus-width = 1 and spi-rx-bus-width = 4 and see
> if that works?
>
> If that does not work then could you disable SFDP parsing (but keep
> spi-rx-bus-width = 4) and see if that works. This should narrow down
> whether SFDP parsing is broken or if driver has an issue.
Sorry, I still haven't found the time at work to test this. I'll keep trying.
Keeping that aside, if this driver is converted to spi-mem, is there any
chance of getting this into SPL while not getting standard SPI drivers in?
On socfpga, I have a standard SPI driver (designware_spi) enabled but that
results in both the cadence_qspi and designware_spi being included in SPL aside
well when boot-from-SPI is enabled.
Does that somehow change when cadence_qspi is an spi-mem driver?
Thanks,
Simon
>
> Regards
> Vignesh
>
> > It works as expected when not parsing SFDP or setting the bus-width to 1.
> > So the change itself probably works, but SFDP parsing is broken?
> >
> > I did the tests with this applied first:
> > https://patchwork.ozlabs.org/project/uboot/list/?series=135505
> >
> > Regards,
> > Simon
> >
> >
> >>
> >> Signed-off-by: Vignesh Raghavendra
> >> ---
> >> drivers/spi/cadence_qspi.c | 136 +
> >> drivers/spi/cadence_qspi.h | 9 +--
> >> drivers/spi/cadence_qspi_apb.c | 124 --
> >> 3 files changed, 91 insertions(+), 178 deletions(-)
> >>
> >> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> >> index e2e54cd27723..673a2e9a6c4c 100644
> >> --- a/drivers/spi/cadence_qspi.c
> >> +++ b/drivers/spi/cadence_qspi.c
> >> @@ -10,6 +10,7 @@
> >> #include
> >> #include
> >> #include
> >> +#include
> >> #include
> >> #include "cadence_qspi.h"
> >>
> >> @@ -34,12 +35,21 @@ static int cadence_spi_write_speed(struct udevice
> >> *bus, uint hz)
> >> return 0;
> >> }
> >>
> >> +static int cadence_spi_read_id(void *reg_base, u8 len, u8 *idcode)
> >> +{
> >> + struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(0x9F, 1),
> >> + SPI_MEM_OP_NO_ADDR,
> >> + SPI_MEM_OP_NO_DUMMY,
> >> + SPI_MEM_OP_DATA_IN(len, idcode,
> >> 1));
> >> +
> >> + return cadence_qspi_apb_command_read(reg_base, );
> >> +}
> >> +
> >> /* Calibration sequence to determine the read data capture delay register
> >> */
> >> static int spi_calibration(struct udevice *bus, uint hz)
> >> {
> >> struct cadence_spi_priv *priv = dev_get_priv(bus);
> >> void *base = priv->regbase;
> >> - u8 opcode_rdid = 0x9F;
> >> unsigned int idcode = 0, temp = 0;
> >> int err = 0, i, range_lo = -1, range_hi = -1;
> >>
> >> @@ -53,8 +63,7 @@ static int spi_calibration(struct udevice *bus, uint hz)
> >> cadence_qspi_apb_controller_enable(base);
> >>
> >> /* read the ID which will be our golden value */
> >> - err = cadence_qspi_apb_command_read(base, 1, _rdid,
> >> - 3, (u8 *));
> >> + err = cadence_spi_read_id(base, 3, (u8 *));
> >> if (err) {
> >> puts("SF: Calibration failed (read)\n");
> >> return err;
> >> @@ -73,8 +82,7 @@ static int spi_calibration(struct udevice *bus, uint hz)
> >> cadence_qspi_apb_controller_enable(base);
> >>
> >> /* issue a RDID to get the ID value */
> >> - err = cadence_qspi_apb_command_read(base, 1, _rdid,
> >> - 3, (u8 *));
> >> + err = cadence_spi_read_id(base, 3, (u8 *));
> >> if (err) {
Re: [U-Boot] [PATCH 1/2] spi: cadence_qspi: Move to spi-mem framework
Hi, Simon, Vignesh,
On 10/17/2019 02:20 PM, Simon Goldschmidt wrote:
> On Mon, Oct 14, 2019 at 3:27 PM Vignesh Raghavendra wrote:
>> Current Cadence QSPI driver has few limitations. It assumes all read
>> operations to be in Quad mode and thus does not support SFDP parsing.
>> Also, adding support for new mode such as Octal mode would not be
>> possible with current configuration. Therefore move the driver over to
>> spi-mem
>> framework. This has added advantage that driver can be used to support
>> SPI NAND memories too.
>> Hence, move driver over to new spi-mem APIs.
>>
>> Please note that this gets rid of mode bit setting done when
>> CONFIG_SPL_SPI_XIP is defined as there does not seem to be any user to
>> that config option.
> I just have tried this on an socfgpa cylone5 board with an mt25ql256a, but
> it does not seem to work: when leaving spi-rx-bus-width and spi-tx-bus-width
> at 4 in my devicetree, SFDP parsing works, but reading data afterwards
> produces invalid results (I haven't tested what's wrong there).
>
> It works as expected when not parsing SFDP or setting the bus-width to 1.
> So the change itself probably works, but SFDP parsing is broken?
This can happen if the quad enable method is not correctly set/called. Would you
try the patch form below?
I don't see much benefit in having those guards, especially that we have
SPI_FLASH_SFDP_SUPPORT defined - it trims most of the SFDP logic.
More, these #ifdef guards are not scalable and with the addition of flashes that
support SFDP the #ifdefs will look uglier and uglier.
Cheers,
ta
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index e5b9899c64b2..3002f97a7342 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -188,7 +188,6 @@ static int read_fsr(struct spi_nor *nor)
* location. Return the configuration register value.
* Returns negative if error occurred.
*/
-#if defined(CONFIG_SPI_FLASH_SPANSION) || defined(CONFIG_SPI_FLASH_WINBOND)
static int read_cr(struct spi_nor *nor)
{
int ret;
@@ -202,7 +201,6 @@ static int read_cr(struct spi_nor *nor)
return val;
}
-#endif
/*
* Write status register 1 byte
@@ -591,7 +589,6 @@ erase_err:
return ret;
}
-#if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST)
/* Write status register and ensure bits in mask match written values */
static int write_sr_and_check(struct spi_nor *nor, u8 status_new, u8 mask)
{
@@ -877,7 +874,6 @@ static int stm_is_locked(struct spi_nor *nor, loff_t ofs,
uint64_t len)
return stm_is_locked_sr(nor, ofs, len, status);
}
-#endif /* CONFIG_SPI_FLASH_STMICRO */
static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
{
@@ -1116,7 +1112,6 @@ write_err:
return ret;
}
-#ifdef CONFIG_SPI_FLASH_MACRONIX
/**
* macronix_quad_enable() - set QE bit in Status Register.
* @nor: pointer to a 'struct spi_nor'
@@ -1153,9 +1148,7 @@ static int macronix_quad_enable(struct spi_nor *nor)
return 0;
}
-#endif
-#if defined(CONFIG_SPI_FLASH_SPANSION) || defined(CONFIG_SPI_FLASH_WINBOND)
/*
* Write status Register and configuration register with 2 bytes
* The first byte will be written to the status register, while the
@@ -1269,7 +1262,6 @@ static int spansion_no_read_cr_quad_enable(struct spi_nor
*nor)
}
#endif /* CONFIG_SPI_FLASH_SFDP_SUPPORT */
-#endif /* CONFIG_SPI_FLASH_SPANSION */
struct spi_nor_read_command {
u8 num_mode_clocks;
@@ -1787,22 +1779,16 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
case BFPT_DWORD15_QER_NONE:
params->quad_enable = NULL;
break;
-#if defined(CONFIG_SPI_FLASH_SPANSION) || defined(CONFIG_SPI_FLASH_WINBOND)
case BFPT_DWORD15_QER_SR2_BIT1_BUGGY:
case BFPT_DWORD15_QER_SR2_BIT1_NO_RD:
params->quad_enable = spansion_no_read_cr_quad_enable;
break;
-#endif
-#ifdef CONFIG_SPI_FLASH_MACRONIX
case BFPT_DWORD15_QER_SR1_BIT6:
params->quad_enable = macronix_quad_enable;
break;
-#endif
-#if defined(CONFIG_SPI_FLASH_SPANSION) || defined(CONFIG_SPI_FLASH_WINBOND)
case BFPT_DWORD15_QER_SR2_BIT1:
params->quad_enable = spansion_read_cr_quad_enable;
break;
-#endif
default:
return -EINVAL;
}
@@ -2013,20 +1999,16 @@ static int spi_nor_init_params(struct spi_nor *nor,
if (params->hwcaps.mask & (SNOR_HWCAPS_READ_QUAD |
SNOR_HWCAPS_PP_QUAD)) {
switch (JEDEC_MFR(info)) {
-#ifdef CONFIG_SPI_FLASH_MACRONIX
case SNOR_MFR_MACRONIX:
params->quad_enable = macronix_quad_enable;
break;
-#endif
case SNOR_MFR_ST:
case SNOR_MFR_MICRON:
break;
default:
-#if
Re: [U-Boot] [PATCH 1/2] spi: cadence_qspi: Move to spi-mem framework
Hi Simon,
On 17/10/19 4:50 PM, Simon Goldschmidt wrote:
> On Mon, Oct 14, 2019 at 3:27 PM Vignesh Raghavendra wrote:
>>
>> Current Cadence QSPI driver has few limitations. It assumes all read
>> operations to be in Quad mode and thus does not support SFDP parsing.
>> Also, adding support for new mode such as Octal mode would not be
>> possible with current configuration. Therefore move the driver over to
>> spi-mem
>> framework. This has added advantage that driver can be used to support
>> SPI NAND memories too.
>> Hence, move driver over to new spi-mem APIs.
>>
>> Please note that this gets rid of mode bit setting done when
>> CONFIG_SPL_SPI_XIP is defined as there does not seem to be any user to
>> that config option.
>
> I just have tried this on an socfgpa cylone5 board with an mt25ql256a, but
> it does not seem to work: when leaving spi-rx-bus-width and spi-tx-bus-width
> at 4 in my devicetree, SFDP parsing works, but reading data afterwards
> produces invalid results (I haven't tested what's wrong there).
>
Thanks for testing!
spi-tx-bus-width = 4 was not supported before so I haven't added support
for that mode in this series. That change will be a separate series.
Could you try with spi-tx-bus-width = 1 and spi-rx-bus-width = 4 and see
if that works?
If that does not work then could you disable SFDP parsing (but keep
spi-rx-bus-width = 4) and see if that works. This should narrow down
whether SFDP parsing is broken or if driver has an issue.
Regards
Vignesh
> It works as expected when not parsing SFDP or setting the bus-width to 1.
> So the change itself probably works, but SFDP parsing is broken?
>
> I did the tests with this applied first:
> https://patchwork.ozlabs.org/project/uboot/list/?series=135505
>
> Regards,
> Simon
>
>
>>
>> Signed-off-by: Vignesh Raghavendra
>> ---
>> drivers/spi/cadence_qspi.c | 136 +
>> drivers/spi/cadence_qspi.h | 9 +--
>> drivers/spi/cadence_qspi_apb.c | 124 --
>> 3 files changed, 91 insertions(+), 178 deletions(-)
>>
>> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
>> index e2e54cd27723..673a2e9a6c4c 100644
>> --- a/drivers/spi/cadence_qspi.c
>> +++ b/drivers/spi/cadence_qspi.c
>> @@ -10,6 +10,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include "cadence_qspi.h"
>>
>> @@ -34,12 +35,21 @@ static int cadence_spi_write_speed(struct udevice *bus,
>> uint hz)
>> return 0;
>> }
>>
>> +static int cadence_spi_read_id(void *reg_base, u8 len, u8 *idcode)
>> +{
>> + struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(0x9F, 1),
>> + SPI_MEM_OP_NO_ADDR,
>> + SPI_MEM_OP_NO_DUMMY,
>> + SPI_MEM_OP_DATA_IN(len, idcode,
>> 1));
>> +
>> + return cadence_qspi_apb_command_read(reg_base, );
>> +}
>> +
>> /* Calibration sequence to determine the read data capture delay register */
>> static int spi_calibration(struct udevice *bus, uint hz)
>> {
>> struct cadence_spi_priv *priv = dev_get_priv(bus);
>> void *base = priv->regbase;
>> - u8 opcode_rdid = 0x9F;
>> unsigned int idcode = 0, temp = 0;
>> int err = 0, i, range_lo = -1, range_hi = -1;
>>
>> @@ -53,8 +63,7 @@ static int spi_calibration(struct udevice *bus, uint hz)
>> cadence_qspi_apb_controller_enable(base);
>>
>> /* read the ID which will be our golden value */
>> - err = cadence_qspi_apb_command_read(base, 1, _rdid,
>> - 3, (u8 *));
>> + err = cadence_spi_read_id(base, 3, (u8 *));
>> if (err) {
>> puts("SF: Calibration failed (read)\n");
>> return err;
>> @@ -73,8 +82,7 @@ static int spi_calibration(struct udevice *bus, uint hz)
>> cadence_qspi_apb_controller_enable(base);
>>
>> /* issue a RDID to get the ID value */
>> - err = cadence_qspi_apb_command_read(base, 1, _rdid,
>> - 3, (u8 *));
>> + err = cadence_spi_read_id(base, 3, (u8 *));
>> if (err) {
>> puts("SF: Calibration failed (read)\n");
>> return err;
>> @@ -195,96 +203,56 @@ static int cadence_spi_set_mode(struct udevice *bus,
>> uint mode)
>> return 0;
>> }
>>
>> -static int cadence_spi_xfer(struct udevice *dev, unsigned int bitlen,
>> - const void *dout, void *din, unsigned long flags)
>> +static int cadence_spi_mem_exec_op(struct spi_slave *spi,
>> + const struct spi_mem_op *op)
>> {
>> - struct udevice *bus = dev->parent;
>> + struct udevice *bus = spi->dev->parent;
>> struct cadence_spi_platdata *plat = bus->platdata;
>> struct cadence_spi_priv *priv = dev_get_priv(bus);
>> - struct dm_spi_slave_platdata *dm_plat =
Re: [U-Boot] [PATCH 1/2] spi: cadence_qspi: Move to spi-mem framework
On Mon, Oct 14, 2019 at 3:27 PM Vignesh Raghavendra wrote:
>
> Current Cadence QSPI driver has few limitations. It assumes all read
> operations to be in Quad mode and thus does not support SFDP parsing.
> Also, adding support for new mode such as Octal mode would not be
> possible with current configuration. Therefore move the driver over to spi-mem
> framework. This has added advantage that driver can be used to support
> SPI NAND memories too.
> Hence, move driver over to new spi-mem APIs.
>
> Please note that this gets rid of mode bit setting done when
> CONFIG_SPL_SPI_XIP is defined as there does not seem to be any user to
> that config option.
I just have tried this on an socfgpa cylone5 board with an mt25ql256a, but
it does not seem to work: when leaving spi-rx-bus-width and spi-tx-bus-width
at 4 in my devicetree, SFDP parsing works, but reading data afterwards
produces invalid results (I haven't tested what's wrong there).
It works as expected when not parsing SFDP or setting the bus-width to 1.
So the change itself probably works, but SFDP parsing is broken?
I did the tests with this applied first:
https://patchwork.ozlabs.org/project/uboot/list/?series=135505
Regards,
Simon
>
> Signed-off-by: Vignesh Raghavendra
> ---
> drivers/spi/cadence_qspi.c | 136 +
> drivers/spi/cadence_qspi.h | 9 +--
> drivers/spi/cadence_qspi_apb.c | 124 --
> 3 files changed, 91 insertions(+), 178 deletions(-)
>
> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> index e2e54cd27723..673a2e9a6c4c 100644
> --- a/drivers/spi/cadence_qspi.c
> +++ b/drivers/spi/cadence_qspi.c
> @@ -10,6 +10,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include "cadence_qspi.h"
>
> @@ -34,12 +35,21 @@ static int cadence_spi_write_speed(struct udevice *bus,
> uint hz)
> return 0;
> }
>
> +static int cadence_spi_read_id(void *reg_base, u8 len, u8 *idcode)
> +{
> + struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(0x9F, 1),
> + SPI_MEM_OP_NO_ADDR,
> + SPI_MEM_OP_NO_DUMMY,
> + SPI_MEM_OP_DATA_IN(len, idcode, 1));
> +
> + return cadence_qspi_apb_command_read(reg_base, );
> +}
> +
> /* Calibration sequence to determine the read data capture delay register */
> static int spi_calibration(struct udevice *bus, uint hz)
> {
> struct cadence_spi_priv *priv = dev_get_priv(bus);
> void *base = priv->regbase;
> - u8 opcode_rdid = 0x9F;
> unsigned int idcode = 0, temp = 0;
> int err = 0, i, range_lo = -1, range_hi = -1;
>
> @@ -53,8 +63,7 @@ static int spi_calibration(struct udevice *bus, uint hz)
> cadence_qspi_apb_controller_enable(base);
>
> /* read the ID which will be our golden value */
> - err = cadence_qspi_apb_command_read(base, 1, _rdid,
> - 3, (u8 *));
> + err = cadence_spi_read_id(base, 3, (u8 *));
> if (err) {
> puts("SF: Calibration failed (read)\n");
> return err;
> @@ -73,8 +82,7 @@ static int spi_calibration(struct udevice *bus, uint hz)
> cadence_qspi_apb_controller_enable(base);
>
> /* issue a RDID to get the ID value */
> - err = cadence_qspi_apb_command_read(base, 1, _rdid,
> - 3, (u8 *));
> + err = cadence_spi_read_id(base, 3, (u8 *));
> if (err) {
> puts("SF: Calibration failed (read)\n");
> return err;
> @@ -195,96 +203,56 @@ static int cadence_spi_set_mode(struct udevice *bus,
> uint mode)
> return 0;
> }
>
> -static int cadence_spi_xfer(struct udevice *dev, unsigned int bitlen,
> - const void *dout, void *din, unsigned long flags)
> +static int cadence_spi_mem_exec_op(struct spi_slave *spi,
> + const struct spi_mem_op *op)
> {
> - struct udevice *bus = dev->parent;
> + struct udevice *bus = spi->dev->parent;
> struct cadence_spi_platdata *plat = bus->platdata;
> struct cadence_spi_priv *priv = dev_get_priv(bus);
> - struct dm_spi_slave_platdata *dm_plat = dev_get_parent_platdata(dev);
> void *base = priv->regbase;
> - u8 *cmd_buf = priv->cmd_buf;
> - size_t data_bytes;
> int err = 0;
> - u32 mode = CQSPI_STIG_WRITE;
> -
> - if (flags & SPI_XFER_BEGIN) {
> - /* copy command to local buffer */
> - priv->cmd_len = bitlen / 8;
> - memcpy(cmd_buf, dout, priv->cmd_len);
> - }
> -
> - if (flags == (SPI_XFER_BEGIN | SPI_XFER_END)) {
> - /* if start and end bit are set, the data bytes is 0. */
> - data_bytes = 0;
> - } else {
> - data_bytes = bitlen / 8;
> - }
> -
[U-Boot] [PATCH 1/2] spi: cadence_qspi: Move to spi-mem framework
Current Cadence QSPI driver has few limitations. It assumes all read
operations to be in Quad mode and thus does not support SFDP parsing.
Also, adding support for new mode such as Octal mode would not be
possible with current configuration. Therefore move the driver over to spi-mem
framework. This has added advantage that driver can be used to support
SPI NAND memories too.
Hence, move driver over to new spi-mem APIs.
Please note that this gets rid of mode bit setting done when
CONFIG_SPL_SPI_XIP is defined as there does not seem to be any user to
that config option.
Signed-off-by: Vignesh Raghavendra
---
drivers/spi/cadence_qspi.c | 136 +
drivers/spi/cadence_qspi.h | 9 +--
drivers/spi/cadence_qspi_apb.c | 124 --
3 files changed, 91 insertions(+), 178 deletions(-)
diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
index e2e54cd27723..673a2e9a6c4c 100644
--- a/drivers/spi/cadence_qspi.c
+++ b/drivers/spi/cadence_qspi.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include
#include "cadence_qspi.h"
@@ -34,12 +35,21 @@ static int cadence_spi_write_speed(struct udevice *bus,
uint hz)
return 0;
}
+static int cadence_spi_read_id(void *reg_base, u8 len, u8 *idcode)
+{
+ struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(0x9F, 1),
+ SPI_MEM_OP_NO_ADDR,
+ SPI_MEM_OP_NO_DUMMY,
+ SPI_MEM_OP_DATA_IN(len, idcode, 1));
+
+ return cadence_qspi_apb_command_read(reg_base, );
+}
+
/* Calibration sequence to determine the read data capture delay register */
static int spi_calibration(struct udevice *bus, uint hz)
{
struct cadence_spi_priv *priv = dev_get_priv(bus);
void *base = priv->regbase;
- u8 opcode_rdid = 0x9F;
unsigned int idcode = 0, temp = 0;
int err = 0, i, range_lo = -1, range_hi = -1;
@@ -53,8 +63,7 @@ static int spi_calibration(struct udevice *bus, uint hz)
cadence_qspi_apb_controller_enable(base);
/* read the ID which will be our golden value */
- err = cadence_qspi_apb_command_read(base, 1, _rdid,
- 3, (u8 *));
+ err = cadence_spi_read_id(base, 3, (u8 *));
if (err) {
puts("SF: Calibration failed (read)\n");
return err;
@@ -73,8 +82,7 @@ static int spi_calibration(struct udevice *bus, uint hz)
cadence_qspi_apb_controller_enable(base);
/* issue a RDID to get the ID value */
- err = cadence_qspi_apb_command_read(base, 1, _rdid,
- 3, (u8 *));
+ err = cadence_spi_read_id(base, 3, (u8 *));
if (err) {
puts("SF: Calibration failed (read)\n");
return err;
@@ -195,96 +203,56 @@ static int cadence_spi_set_mode(struct udevice *bus, uint
mode)
return 0;
}
-static int cadence_spi_xfer(struct udevice *dev, unsigned int bitlen,
- const void *dout, void *din, unsigned long flags)
+static int cadence_spi_mem_exec_op(struct spi_slave *spi,
+ const struct spi_mem_op *op)
{
- struct udevice *bus = dev->parent;
+ struct udevice *bus = spi->dev->parent;
struct cadence_spi_platdata *plat = bus->platdata;
struct cadence_spi_priv *priv = dev_get_priv(bus);
- struct dm_spi_slave_platdata *dm_plat = dev_get_parent_platdata(dev);
void *base = priv->regbase;
- u8 *cmd_buf = priv->cmd_buf;
- size_t data_bytes;
int err = 0;
- u32 mode = CQSPI_STIG_WRITE;
-
- if (flags & SPI_XFER_BEGIN) {
- /* copy command to local buffer */
- priv->cmd_len = bitlen / 8;
- memcpy(cmd_buf, dout, priv->cmd_len);
- }
-
- if (flags == (SPI_XFER_BEGIN | SPI_XFER_END)) {
- /* if start and end bit are set, the data bytes is 0. */
- data_bytes = 0;
- } else {
- data_bytes = bitlen / 8;
- }
- debug("%s: len=%zu [bytes]\n", __func__, data_bytes);
+ u32 mode;
/* Set Chip select */
- cadence_qspi_apb_chipselect(base, spi_chip_select(dev),
+ cadence_qspi_apb_chipselect(base, spi_chip_select(spi->dev),
plat->is_decoded_cs);
- if ((flags & SPI_XFER_END) || (flags == 0)) {
- if (priv->cmd_len == 0) {
- printf("QSPI: Error, command is empty.\n");
- return -1;
- }
-
- if (din && data_bytes) {
- /* read */
- /* Use STIG if no address. */
- if (!CQSPI_IS_ADDR(priv->cmd_len))
- mode = CQSPI_STIG_READ;
- else
-
