| 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: <20190131173207.56481a42@bbrezillon>
Date: Thu, 31 Jan 2019 17:32:07 +0100
From: Boris Brezillon <bbrezillon@...nel.org>
To: <Tudor.Ambarus@...rochip.com>
Cc: <broonie@...nel.org>, <robh+dt@...nel.org>, <mark.rutland@....com>,
<Nicolas.Ferre@...rochip.com>, <alexandre.belloni@...tlin.com>,
<Ludovic.Desroches@...rochip.com>, <linux-spi@...r.kernel.org>,
<devicetree@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <linux-mtd@...ts.infradead.org>
Subject: Re: [PATCH v2 10/10] spi: atmel-quadspi: add support for sam9x60
qspi controller
On Thu, 31 Jan 2019 16:15:51 +0000
<Tudor.Ambarus@...rochip.com> wrote:
> From: Tudor Ambarus <tudor.ambarus@...rochip.com>
>
> The sam9x60 qspi controller uses 2 clocks, one for the peripheral register
> access, the other for the qspi core and phy. Both are mandatory. It uses
> dedicated register for Read Instruction Code Register (RICR) and
> Write Instruction Code Register (WICR). ICR/RICR/WICR have identical
> fields.
>
> Tested with sst26vf064b jedec,spi-nor flash. Backward compatibility test
> done on sama5d2 qspi controller and mx25l25635e jedec,spi-nor flash.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@...rochip.com>
> ---
> v2:
> - rework clock handling
> - reorder setting of register values in set_cfg() calls -> move functions
> that can fail in the upper part of the function body.
>
> drivers/spi/atmel-quadspi.c | 296 +++++++++++++++++++++++++++++++++++---------
> 1 file changed, 239 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> index d3e76acf8517..80c934f3e479 100644
> --- a/drivers/spi/atmel-quadspi.c
> +++ b/drivers/spi/atmel-quadspi.c
> @@ -19,6 +19,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/of_platform.h>
> #include <linux/platform_device.h>
> #include <linux/spi/spi-mem.h>
>
> @@ -35,7 +36,9 @@
>
> #define QSPI_IAR 0x0030 /* Instruction Address Register */
> #define QSPI_ICR 0x0034 /* Instruction Code Register */
> +#define QSPI_WICR 0x0034 /* Write Instruction Code Register */
> #define QSPI_IFR 0x0038 /* Instruction Frame Register */
> +#define QSPI_RICR 0x003C /* Read Instruction Code Register */
>
> #define QSPI_SMR 0x0040 /* Scrambling Mode Register */
> #define QSPI_SKR 0x0044 /* Scrambling Key Register */
> @@ -88,7 +91,7 @@
> #define QSPI_SCR_DLYBS_MASK GENMASK(23, 16)
> #define QSPI_SCR_DLYBS(n) (((n) << 16) & QSPI_SCR_DLYBS_MASK)
>
> -/* Bitfields in QSPI_ICR (Instruction Code Register) */
> +/* Bitfields in QSPI_ICR (Read/Write Instruction Code Register) */
> #define QSPI_ICR_INST_MASK GENMASK(7, 0)
> #define QSPI_ICR_INST(inst) (((inst) << 0) & QSPI_ICR_INST_MASK)
> #define QSPI_ICR_OPT_MASK GENMASK(23, 16)
> @@ -113,6 +116,8 @@
> #define QSPI_IFR_OPTL_4BIT (2 << 8)
> #define QSPI_IFR_OPTL_8BIT (3 << 8)
> #define QSPI_IFR_ADDRL BIT(10)
> +#define QSPI_IFR_TFRTYP_TRSFR_MEM BIT(12)
> +#define QSPI_IFR_TFRTYP_TRSFR_REG (0 << 12)
You don't need to define TRSFR_REG, just set QSPI_IFR_TFRTYP_TRSFR_MEM
when you do a mem transfer and do nothing when this is a regular
transfer.
> #define QSPI_IFR_TFRTYP_MASK GENMASK(13, 12)
> #define QSPI_IFR_TFRTYP_TRSFR_READ (0 << 12)
> #define QSPI_IFR_TFRTYP_TRSFR_READ_MEM (1 << 12)
Looks like the read/write flag is on bit 13. Can we just add
#define QSPI_IFR_TFRTYP_TRSFR_WRITE BIT(13)
and drop all others def? This way the implementation is consistent
between sam9x60 and sama5d2.
> @@ -121,6 +126,8 @@
> #define QSPI_IFR_CRM BIT(14)
> #define QSPI_IFR_NBDUM_MASK GENMASK(20, 16)
> #define QSPI_IFR_NBDUM(n) (((n) << 16) & QSPI_IFR_NBDUM_MASK)
> +#define QSPI_IFR_APBTFRTYP_WRITE (0 << 24)
As for the other defs, I don't think you need to define _WRITE.
> +#define QSPI_IFR_APBTFRTYP_READ BIT(24)
>
> /* Bitfields in QSPI_SMR (Scrambling Mode Register) */
> #define QSPI_SMR_SCREN BIT(0)
> @@ -137,16 +144,37 @@
> #define QSPI_WPSR_WPVSRC(src) (((src) << 8) & QSPI_WPSR_WPVSRC)
>
>
> +/* Describes register values. */
> +struct atmel_qspi_cfg {
> + u32 icr;
> + u32 iar;
> + u32 ifr;
> +};
> +
> +struct atmel_qspi_caps;
> +
> struct atmel_qspi {
> void __iomem *regs;
> void __iomem *mem;
> struct clk *clk;
Can we rename that on pclk?
> + struct clk *qspick;
> struct platform_device *pdev;
> + const struct atmel_qspi_caps *caps;
> u32 pending;
> u32 mr;
> struct completion cmd_completion;
> };
>
...
> +
> +static int atmel_sam9x60_qspi_set_cfg(void __iomem *base,
> + const struct spi_mem_op *op,
> + struct atmel_qspi_cfg *cfg)
> +{
> + int ret = atmel_qspi_set_mode(cfg, op);
> +
> + if (ret)
> + return ret;
> +
> + ret = atmel_qspi_set_address_mode(cfg, op);
> + if (ret)
> + return ret;
> +
> + cfg->ifr |= QSPI_IFR_INSTEN;
> + cfg->icr |= QSPI_ICR_INST(op->cmd.opcode);
> +
> + /* Set data enable */
> + if (op->data.nbytes)
> + cfg->ifr |= QSPI_IFR_DATAEN;
> +
> + if (!op->addr.nbytes) {
> + cfg->ifr |= QSPI_IFR_TFRTYP_TRSFR_REG;
> + if (op->data.dir == SPI_MEM_DATA_OUT)
> + cfg->ifr |= QSPI_IFR_APBTFRTYP_WRITE;
> + else
> + cfg->ifr |= QSPI_IFR_APBTFRTYP_READ;
> + } else {
> + cfg->ifr |= QSPI_IFR_TFRTYP_TRSFR_MEM;
Can you try doing only regular transfers and let me know if it still
works. Support for mem transfers can then be added along with dirmap
support.
> + }
>
> /* Clear pending interrupts */
> (void)readl_relaxed(base + QSPI_SR);
>
> /* Set QSPI Instruction Frame registers */
> - writel_relaxed(iar, base + QSPI_IAR);
> - writel_relaxed(icr, base + QSPI_ICR);
> - writel_relaxed(ifr, base + QSPI_IFR);
> + writel_relaxed(cfg->iar, base + QSPI_IAR);
> + if (op->data.dir == SPI_MEM_DATA_OUT)
> + writel_relaxed(cfg->icr, base + QSPI_ICR);
> + else
> + writel_relaxed(cfg->icr, base + QSPI_RICR);
> + writel_relaxed(cfg->ifr, base + QSPI_IFR);
> +
> + return 0;
> +}
> +
...
> @@ -443,32 +578,52 @@ static int atmel_qspi_probe(struct platform_device *pdev)
> /* Enable the peripheral clock */
> err = clk_prepare_enable(aq->clk);
> if (err) {
> - dev_err(&pdev->dev, "failed to enable the peripheral clock\n");
> + dev_err(dev, "failed to enable the peripheral clock\n");
> goto exit;
> }
>
> + if (caps->has_qspick) {
> + /* Get the QSPI system clock */
> + aq->qspick = devm_clk_get(dev, "qspick");
> + if (IS_ERR(aq->qspick)) {
> + dev_err(dev, "missing system clock\n");
> + err = PTR_ERR(aq->qspick);
> + goto disable_clk;
> + }
> +
> + /* Enable the QSPI system clock */
> + err = clk_prepare_enable(aq->qspick);
> + if (err) {
> + dev_err(dev,
> + "failed to enable the QSPI system clock\n");
> + goto disable_clk;
> + }
> + }
> +
> /* Request the IRQ */
> irq = platform_get_irq(pdev, 0);
> if (irq < 0) {
> - dev_err(&pdev->dev, "missing IRQ\n");
> + dev_err(dev, "missing IRQ\n");
> err = irq;
> - goto disable_clk;
> + goto disable_qspick;
> }
> - err = devm_request_irq(&pdev->dev, irq, atmel_qspi_interrupt,
> - 0, dev_name(&pdev->dev), aq);
> + err = devm_request_irq(dev, irq, atmel_qspi_interrupt, 0,
> + dev_name(dev), aq);
> if (err)
> - goto disable_clk;
> + goto disable_qspick;
>
> err = atmel_qspi_init(aq);
> if (err)
> - goto disable_clk;
> + goto disable_qspick;
>
> err = spi_register_controller(ctrl);
> if (err)
> - goto disable_clk;
> + goto disable_qspick;
>
> return 0;
>
> +disable_qspick:
> + clk_disable_unprepare(aq->qspick);
> disable_clk:
We should probably rename this label disable_pclk.
> clk_disable_unprepare(aq->clk);
> exit:
Powered by blists - more mailing lists