Messages in this thread | | | From | Artur Jedrysek <> | Subject | RE: [v2, 2/4] mtd: spi-nor: Add Octal SPI support to Cadence QSPI driver. | Date | Fri, 10 Mar 2017 14:09:51 +0000 |
| |
From: Marek Vasut [mailto:marek.vasut@gmail.com] Sent: 10 March 2017 13:50 > On 03/10/2017 01:00 PM, Artur Jedrysek wrote: > > > > From: Marek Vasut [mailto:marek.vasut@gmail.com] > > Sent: 10 March 2017 04:37 > >> On 03/08/2017 09:02 AM, Artur Jedrysek wrote: > >>> Recent versions of Cadence QSPI controller support Octal SPI transfers > >>> as well. This patch updates existing driver to support such feature. > >>> > >>> It is not possible to determine whether or not octal mode is supported > >>> just by looking at revision register alone. To solve that, an additional > >>> compatible in Device Tree is added to indicate such capability. > >>> Both (revision and compatible) are used to determine, which mode to > >>> pass to spi_nor_scan() call. > >>> > >>> Signed-off-by: Artur Jedrysek <jartur@cadence.com> > >>> --- > >>> Changelog: > >>> v2: Use new compatible in DT, instead of boolean property, to indicate > >>> Octal SPI support. > >>> Extracted Kconfig update to seperate patch. > >>> --- > >>> drivers/mtd/spi-nor/cadence-quadspi.c | 69 +++++++++++++++++++++++++++++++---- > >>> 1 file changed, 61 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c > >>> index 9f8102d..a96471d 100644 > >>> --- a/drivers/mtd/spi-nor/cadence-quadspi.c > >>> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c > >>> @@ -76,6 +76,7 @@ struct cqspi_st { > >>> u32 fifo_depth; > >>> u32 fifo_width; > >>> u32 trigger_address; > >>> + u32 max_mode; > >> > >> I think it's time to introduce flags instead, > >> ie. #define CQSPI_FLAG_SUPPORTS_OCTAL BIT(0) > >> > >>> struct cqspi_flash_pdata f_pdata[CQSPI_MAX_CHIPSELECT]; > >>> }; > >>> > >>> @@ -87,6 +88,10 @@ struct cqspi_st { > >>> #define CQSPI_INST_TYPE_SINGLE 0 > >>> #define CQSPI_INST_TYPE_DUAL 1 > >>> #define CQSPI_INST_TYPE_QUAD 2 > >>> +#define CQSPI_INST_TYPE_OCTAL 3 > >>> + > >>> +#define CQSPI_MAX_MODE_QUAD 0 > >>> +#define CQSPI_MAX_MODE_OCTAL 1 > >>> > >>> #define CQSPI_DUMMY_CLKS_PER_BYTE 8 > >>> #define CQSPI_DUMMY_BYTES_MAX 4 > >>> @@ -204,6 +209,14 @@ struct cqspi_st { > >>> #define CQSPI_REG_CMDWRITEDATALOWER 0xA8 > >>> #define CQSPI_REG_CMDWRITEDATAUPPER 0xAC > >>> > >>> +#define CQSPI_REG_MODULEID 0xFC > >>> +#define CQSPI_REG_MODULEID_CONF_ID_MASK 0x3 > >>> +#define CQSPI_REG_MODULEID_CONF_ID_LSB 0 > >>> +#define CQSPI_REG_MODULEID_CONF_ID_OCTAL_PHY 0x0 > >>> +#define CQSPI_REG_MODULEID_CONF_ID_OCTAL 0x1 > >>> +#define CQSPI_REG_MODULEID_CONF_ID_QUAD_PHY 0x2 > >>> +#define CQSPI_REG_MODULEID_CONF_ID_QUAD 0x3 > >>> + > >>> /* Interrupt status bits */ > >>> #define CQSPI_REG_IRQ_MODE_ERR BIT(0) > >>> #define CQSPI_REG_IRQ_UNDERFLOW BIT(1) > >>> @@ -866,6 +879,9 @@ static int cqspi_set_protocol(struct spi_nor *nor, const int read) > >>> case SPI_NOR_QUAD: > >>> f_pdata->data_width = CQSPI_INST_TYPE_QUAD; > >>> break; > >>> + case SPI_NOR_OCTAL: > >>> + f_pdata->data_width = CQSPI_INST_TYPE_OCTAL; > >>> + break; > >>> default: > >>> return -EINVAL; > >>> } > >>> @@ -977,6 +993,17 @@ static int cqspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len) > >>> return ret; > >>> } > >>> > >>> +static const u32 cqspi_max_mode_quad = CQSPI_MAX_MODE_QUAD; > >>> +static const u32 cqspi_max_mode_octal = CQSPI_MAX_MODE_OCTAL; > >>> + > >>> +static struct of_device_id const cqspi_dt_ids[] = { > >>> + { .compatible = "cdns,qspi-nor", .data = &cqspi_max_mode_quad }, > >>> + { .compatible = "cdns,ospi-nor", .data = &cqspi_max_mode_octal }, > >> > >> .data = (void *)CQSPI_FLAG_SUPPORTS_OCTAL , then you don't need that > >> static const stuff ... > >> > > > > I had some doubts regarding that approach, as it may be dependent on the > > CPU architecture (32-bit, 64-bit) and endianness. .data needs to be first > > casted to unsigned long for reading, to ensure correct access and to > > allow bitwise operations on it. That solution works, and if such approach > > is preferred, then it will be used in next version of the patch. > > Good > > >>> + { /* end of table */ } > >>> +}; > >>> + > >>> +MODULE_DEVICE_TABLE(of, cqspi_dt_ids); > >>> + > >>> static int cqspi_of_get_flash_pdata(struct platform_device *pdev, > >>> struct cqspi_flash_pdata *f_pdata, > >>> struct device_node *np) > >>> @@ -1018,6 +1045,13 @@ static int cqspi_of_get_pdata(struct platform_device *pdev) > >>> { > >>> struct device_node *np = pdev->dev.of_node; > >>> struct cqspi_st *cqspi = platform_get_drvdata(pdev); > >>> + const struct of_device_id *match; > >>> + > >>> + cqspi->max_mode = CQSPI_MAX_MODE_QUAD; > >>> + > >>> + match = of_match_node(cqspi_dt_ids, np); > >>> + if (match && match->data) > >>> + cqspi->max_mode = *((u32 *)match->data); > >> > >> Use flags instead, see above. > >> > >>> cqspi->is_decoded_cs = of_property_read_bool(np, "cdns,is-decoded-cs"); > >>> > >>> @@ -1074,9 +1108,35 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node > *np) > >>> struct cqspi_flash_pdata *f_pdata; > >>> struct spi_nor *nor; > >>> struct mtd_info *mtd; > >>> + enum read_mode mode; > >>> + enum read_mode dt_mode = SPI_NOR_QUAD; > >>> unsigned int cs; > >>> + unsigned int rev_reg; > >>> int i, ret; > >>> > >>> + /* Determine, whether or not octal transfer MAY be supported */ > >> > >> But you already know that from DT, no ? > >> > > > > Cadence Octal (formerly Quad) SPI Controller is sold as an IP, and is > > configurable. This includes max SPI mode. It is possible to detect, that > > Octal SPI controller is configured (during hardware compilation) to support > > up to Quad mode, using revision register. > > So the octal-spi controller always has this config register, but the > quad-spi controller may or may not have this register ? >
This register was always present. In Quad-SPI, however, it didn't contain information about maximum possible mode, as only quad was possible, and meaning of bits checked here was different.
> >>> + rev_reg = readl(cqspi->iobase + CQSPI_REG_MODULEID); > >>> + dev_info(dev, "CQSPI Module id %x\n", rev_reg); > >>> + > >>> + switch (rev_reg & CQSPI_REG_MODULEID_CONF_ID_MASK) { > >>> + case CQSPI_REG_MODULEID_CONF_ID_OCTAL_PHY: > >>> + case CQSPI_REG_MODULEID_CONF_ID_OCTAL: > >>> + mode = SPI_NOR_OCTAL; > >>> + break; > >>> + case CQSPI_REG_MODULEID_CONF_ID_QUAD: > >>> + case CQSPI_REG_MODULEID_CONF_ID_QUAD_PHY: > >> > >> Does this work on all revisions of CQSPI ? > >> > > > > After having a more thorough look at specification of older IP version > > (quad only) it seems, that revision register format has indeed changed. > > This will be fixed in the next version of the patch. > > Can the quad-spi controller be configured only as dual or single ? > What about the octal one ? These cases should probably be handled > somehow too, right ? >
Quad-SPI controller can always support single, dual and quad. There was no option to configure max mode. Octal-SPI controller can be configured to support either octal or quad mode. No controller could be configured (during hardware compilation/synthesis) to support only single/dual SPI mode. To put it shortly: single, dual and quad is always supported.
> >>> + mode = SPI_NOR_QUAD; > >>> + break; > >>> + } > >>> + > >>> + if (cqspi->max_mode == CQSPI_MAX_MODE_OCTAL) > >>> + dt_mode = SPI_NOR_OCTAL; > >>> + > >>> + if (mode == SPI_NOR_QUAD && dt_mode == SPI_NOR_OCTAL) > >>> + dev_warn(dev, "Requested octal mode is not supported by the device."); > >>> + else if (mode == SPI_NOR_OCTAL && dt_mode == SPI_NOR_QUAD) > >>> + mode = SPI_NOR_QUAD; > >>> + > >>> /* Get flash device data */ > >>> for_each_available_child_of_node(dev->of_node, np) { > >>> ret = of_property_read_u32(np, "reg", &cs); > >>> @@ -1123,7 +1183,7 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np) > >>> goto err; > >>> } > >>> > >>> - ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD); > >>> + ret = spi_nor_scan(nor, NULL, mode); > >>> if (ret) > >>> goto err; > >>> > >>> @@ -1277,13 +1337,6 @@ static const struct dev_pm_ops cqspi__dev_pm_ops = { > >>> #define CQSPI_DEV_PM_OPS NULL > >>> #endif > >>> > >>> -static struct of_device_id const cqspi_dt_ids[] = { > >>> - {.compatible = "cdns,qspi-nor",}, > >>> - { /* end of table */ } > >>> -}; > >>> - > >>> -MODULE_DEVICE_TABLE(of, cqspi_dt_ids); > >>> - > >>> static struct platform_driver cqspi_platform_driver = { > >>> .probe = cqspi_probe, > >>> .remove = cqspi_remove, > >>> > >> > >> > >> -- > >> Best regards, > >> Marek Vasut > > > -- > Best regards, > Marek Vasut
| |