| Message ID | 20190204100910.26701-1-tudor.ambarus@microchip.com |
|---|---|
| Headers | show |
| Series | spi: atmel-quadspi: introduce sam9x60 qspi controller | expand |
On Mon, 4 Feb 2019 10:09:22 +0000 <Tudor.Ambarus@microchip.com> wrote: > From: Tudor Ambarus <tudor.ambarus@microchip.com> > > Set the controller by default in Serial Memory Mode (SMM) at probe. > Cache Mode Register (MR) value to avoid write access when setting > the controller in serial memory mode at exec_op(). > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> Reviewed-by: Boris Brezillon <bbrezillon@kernel.org> > --- > v4: s/smm/mr, init controller in serial memory mode by default > v3: update smm value when different. rename mr/smm > v2: cache MR value instead of moving the write access at probe > > drivers/spi/atmel-quadspi.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c > index ddc712410812..d6864d29f294 100644 > --- a/drivers/spi/atmel-quadspi.c > +++ b/drivers/spi/atmel-quadspi.c > @@ -155,6 +155,7 @@ struct atmel_qspi { > struct clk *clk; > struct platform_device *pdev; > u32 pending; > + u32 mr; > struct completion cmd_completion; > }; > > @@ -238,7 +239,14 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > icr = QSPI_ICR_INST(op->cmd.opcode); > ifr = QSPI_IFR_INSTEN; > > - qspi_writel(aq, QSPI_MR, QSPI_MR_SMM); > + /* > + * If the QSPI controller is set in regular SPI mode, set it in > + * Serial Memory Mode (SMM). > + */ > + if (aq->mr != QSPI_MR_SMM) { > + qspi_writel(aq, QSPI_MR, QSPI_MR_SMM); > + aq->mr = QSPI_MR_SMM; > + } > > mode = find_mode(op); > if (mode < 0) > @@ -381,6 +389,10 @@ static int atmel_qspi_init(struct atmel_qspi *aq) > /* Reset the QSPI controller */ > qspi_writel(aq, QSPI_CR, QSPI_CR_SWRST); > > + /* Set the QSPI controller by default in Serial Memory Mode */ > + qspi_writel(aq, QSPI_MR, QSPI_MR_SMM); > + aq->mr = QSPI_MR_SMM; > + > /* Enable the QSPI controller */ > qspi_writel(aq, QSPI_CR, QSPI_CR_QSPIEN); >
On Mon, 4 Feb 2019 10:09:53 +0000 <Tudor.Ambarus@microchip.com> wrote: > From: Tudor Ambarus <tudor.ambarus@microchip.com> > > Split the TFRTYP_TRSFR_ bitfields in 2: one bit encoding the > mem/reg transfer type and one bit encoding the direction of > the transfer (read/write). > > Remove NOP when setting read transfer type. Remove useless > setting of write transfer type when > op->data.dir == SPI_MEM_DATA_IN && !op->data.nbytes. > > QSPI_IFR_TFRTYP_TRSFR_WRITE is specific just to sama5d2 qspi, > rename it to QSPI_IFR_SAMA5D2_WRITE_TRSFR. > > Suggested-by: Boris Brezillon <bbrezillon@kernel.org> > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> Reviewed-by: Boris Brezillon <bbrezillon@kernel.org> > --- > v4: introduce QSPI_IFR_TFRTYP_MEM, reword commit > v3: new patch > > drivers/spi/atmel-quadspi.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) > > diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c > index ce4f8a648f45..19a3980775ad 100644 > --- a/drivers/spi/atmel-quadspi.c > +++ b/drivers/spi/atmel-quadspi.c > @@ -113,11 +113,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_MASK GENMASK(13, 12) > -#define QSPI_IFR_TFRTYP_TRSFR_READ (0 << 12) > -#define QSPI_IFR_TFRTYP_TRSFR_READ_MEM (1 << 12) > -#define QSPI_IFR_TFRTYP_TRSFR_WRITE (2 << 12) > -#define QSPI_IFR_TFRTYP_TRSFR_WRITE_MEM (3 << 13) > +#define QSPI_IFR_TFRTYP_MEM BIT(12) > +#define QSPI_IFR_SAMA5D2_WRITE_TRSFR BIT(13) > #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) > @@ -275,10 +272,8 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > if (op->data.nbytes) > ifr |= QSPI_IFR_DATAEN; > > - if (op->data.dir == SPI_MEM_DATA_IN && op->data.nbytes) > - ifr |= QSPI_IFR_TFRTYP_TRSFR_READ; > - else > - ifr |= QSPI_IFR_TFRTYP_TRSFR_WRITE; > + if (op->data.dir == SPI_MEM_DATA_OUT) > + ifr |= QSPI_IFR_SAMA5D2_WRITE_TRSFR; > > /* Clear pending interrupts */ > (void)readl_relaxed(aq->regs + QSPI_SR);
On Mon, 4 Feb 2019 10:10:21 +0000 <Tudor.Ambarus@microchip.com> wrote: > + > +static void atmel_qspi_sam9x60_write_regs(const struct atmel_qspi *aq, > + const struct spi_mem_op *op, > + const struct atmel_qspi_cfg *cfg) > +{ > + /* Clear pending interrupts */ > + (void)readl_relaxed(aq->regs + QSPI_SR); > + > + /* Set QSPI Instruction Frame registers */ > + writel_relaxed(cfg->iar, aq->regs + QSPI_IAR); > + if (op->data.dir == SPI_MEM_DATA_IN) > + writel_relaxed(cfg->icr, aq->regs + QSPI_RICR); > + else > + writel_relaxed(cfg->icr, aq->regs + QSPI_ICR); Can you use WICR here (even if ICR == WICR)? > + writel_relaxed(cfg->ifr, aq->regs + QSPI_IFR); > +} Hm, so the only difference we have is the RICR vs ICR reg and the APBTFRTYP_READ vs SAMA5D2_WRITE_TRSFR bit. Not sure it deserves creating 2 hooks for that. Can we have something like ->has_ricr in the caps and then have an if/else block directly in atmel_qspi_set_cfg()?
On 02/04/2019 04:16 PM, Boris Brezillon wrote: > On Mon, 4 Feb 2019 10:10:21 +0000 > <Tudor.Ambarus@microchip.com> wrote: > >> + >> +static void atmel_qspi_sam9x60_write_regs(const struct atmel_qspi *aq, >> + const struct spi_mem_op *op, >> + const struct atmel_qspi_cfg *cfg) >> +{ >> + /* Clear pending interrupts */ >> + (void)readl_relaxed(aq->regs + QSPI_SR); >> + >> + /* Set QSPI Instruction Frame registers */ >> + writel_relaxed(cfg->iar, aq->regs + QSPI_IAR); >> + if (op->data.dir == SPI_MEM_DATA_IN) >> + writel_relaxed(cfg->icr, aq->regs + QSPI_RICR); >> + else >> + writel_relaxed(cfg->icr, aq->regs + QSPI_ICR); > > Can you use WICR here (even if ICR == WICR)? yes, good catch. > >> + writel_relaxed(cfg->ifr, aq->regs + QSPI_IFR); >> +} > > Hm, so the only difference we have is the RICR vs ICR reg and the > APBTFRTYP_READ vs SAMA5D2_WRITE_TRSFR bit. Not sure it deserves > creating 2 hooks for that. Can we have something like ->has_ricr in > the caps and then have an if/else block directly in > atmel_qspi_set_cfg()? > Correct. It is a cost of an extra if, I tried to avoid it. I like it better with these two hooks, but if you have a strong opinion I'll do it, just confirm it again. Thanks, ta
On Mon, 4 Feb 2019 14:28:27 +0000 <Tudor.Ambarus@microchip.com> wrote: > > > >> + writel_relaxed(cfg->ifr, aq->regs + QSPI_IFR); > >> +} > > > > Hm, so the only difference we have is the RICR vs ICR reg and the > > APBTFRTYP_READ vs SAMA5D2_WRITE_TRSFR bit. Not sure it deserves > > creating 2 hooks for that. Can we have something like ->has_ricr in > > the caps and then have an if/else block directly in > > atmel_qspi_set_cfg()? > > > > Correct. It is a cost of an extra if, I tried to avoid it. I like it better with > these two hooks, but if you have a strong opinion I'll do it, just confirm it again. The cost of an indirect call is actually higher than an extra if/else block. I'm not against paying this extra cost when implementations are completely different, but that does not seem to be the case here. Moreover, if you get rid of these hooks, you can also get rid of the cfg struct.
From: Tudor Ambarus <tudor.ambarus@microchip.com> Patches from 1 to 11 are minor fixes or cosmetics. Patches 12 and 13 introduce the sam9x60 qspi controller. sam9x60 qspi controller tested with sst26vf064b jedec,spi-nor flash. Backward compatibility test done on sama5d2 qspi controller and mx25l25635e jedec,spi-nor flash. The patches are generated on top of for-next branch. v4: - s/smm/mr, init controller in serial memory mode by default - drop local variables that kept aq->regs and &pdev->dev, the compiler should be smart enough to store them in a register - introduce QSPI_IFR_TFRTYP_MEM - add comment saying QSPI_IFR_APBTFRTYP_READ is defined in sam9x60 - s/sama5d2_qspi_modes/atmel_qspi_modes, modes are the same both controllers - fix kernel doc header - move comment in function body - collect R-b tags v3: - update smm value when different. - treat just regular spi transfers when introducing sam9x60 qspi IP. Mem transfers will be added together with dirmap support. - reorganize the code and change ops functions pointers to avoid code duplication. - rename aq->clk to aq->pclk to indicate that it's a peripheral clock. - drop unused and NOP transfer macros. - add Suggested-by tags, reword some commits. v2: - cache MR value, - drop iomem wrappers, - make "pclk" clock-name mandatory even for sama5d2, - rework clock handling, - reorder setting of register values in set_cfg() calls, - collect R-b tags. Tudor Ambarus (13): spi: atmel-quadspi: cache MR value to avoid a write access spi: atmel-quadspi: order header files inclusion alphabetically spi: atmel-quadspi: drop wrappers for iomem accesses spi: atmel-quadspi: fix naming scheme spi: atmel-quadspi: remove unnecessary cast spi: atmel-quadspi: return appropriate error code spi: atmel-quadspi: switch to SPDX license identifiers spi: atmel-quadspi: rework transfer macros dt-bindings: spi: atmel-quadspi: update example to new clock binding dt-bindings: spi: atmel-quadspi: make "pclk" mandatory spi: atmel-quadspi: add support for named peripheral clock dt-bindings: spi: atmel-quadspi: QuadSPI driver for Microchip SAM9X60 spi: atmel-quadspi: add support for sam9x60 qspi controller .../devicetree/bindings/spi/atmel-quadspi.txt | 12 +- drivers/spi/atmel-quadspi.c | 389 +++++++++++++++------ 2 files changed, 284 insertions(+), 117 deletions(-)