| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <DM5PR07MB2780DC9981927880E1ED1B37D4200@DM5PR07MB2780.namprd07.prod.outlook.com>
Date: Fri, 10 Mar 2017 14:09:51 +0000
From: Artur Jedrysek <jartur@...ence.com>
To: Marek Vasut <marek.vasut@...il.com>,
"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Cyrille Pitchen <cyrille.pitchen@...el.com>,
David Woodhouse <dwmw2@...radead.org>,
Brian Norris <computersforpeace@...il.com>,
Boris Brezillon <boris.brezillon@...e-electrons.com>,
Richard Weinberger <richard@....at>
Subject: RE: [v2, 2/4] mtd: spi-nor: Add Octal SPI support to Cadence QSPI
driver.
From: Marek Vasut [mailto:marek.vasut@...il.com]
Sent: 10 March 2017 13:50
> On 03/10/2017 01:00 PM, Artur Jedrysek wrote:
> >
> > From: Marek Vasut [mailto:marek.vasut@...il.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@...ence.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
Powered by blists - more mailing lists