* [PATCH linux 0/3] ASPEED SPI driver support for dual IO mode
@ 2017-01-10 1:05 Robert Lippert
2017-01-10 1:05 ` [PATCH linux 1/3] mtd: spi-nor: add SPI_NOR_DUAL_READ to mx66l51235l Robert Lippert
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Robert Lippert @ 2017-01-10 1:05 UTC (permalink / raw)
To: openbmc; +Cc: clg, Robert Lippert
Adds support for dual IO SPI mode. This speeds up SPI accesses, dropping
time to dump an entire 64MB Macronix chip from ~12s to ~6s using:
dd if=/dev/mtdblock0 bs=64MB count=1 of=/dev/null
The dual IO bit mode should not depend on any board-specific properties
as it uses the same IO lines as single bit mode. Only issue could be
if a board used a one-way level tranlator or mux on the MOSI line
but that seems unlikely to me.
Tested on Zaius board with AST2500 chip, not tested on AST2400 but I
expect it to work since registers are identical.
Note dropped TODO about quad mode since ASPEED SPI controller does
not support quad mode.
Robert Lippert (3):
mtd: spi-nor: add SPI_NOR_DUAL_READ to mx66l51235l
mtd: spi-nor: aspeed: add support for SPI dual IO read mode
mtd: spi-nor: aspeed: fix DMA access on AST2500
drivers/mtd/spi-nor/aspeed-smc.c | 30 +++++++++++++++++++++++-------
drivers/mtd/spi-nor/spi-nor.c | 2 +-
2 files changed, 24 insertions(+), 8 deletions(-)
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH linux 1/3] mtd: spi-nor: add SPI_NOR_DUAL_READ to mx66l51235l
2017-01-10 1:05 [PATCH linux 0/3] ASPEED SPI driver support for dual IO mode Robert Lippert
@ 2017-01-10 1:05 ` Robert Lippert
2017-01-10 7:54 ` Cédric Le Goater
2017-01-10 1:05 ` [PATCH linux 2/3] mtd: spi-nor: aspeed: add support for SPI dual IO read mode Robert Lippert
2017-01-10 1:05 ` [PATCH linux 3/3] mtd: spi-nor: aspeed: fix DMA access on AST2500 Robert Lippert
2 siblings, 1 reply; 12+ messages in thread
From: Robert Lippert @ 2017-01-10 1:05 UTC (permalink / raw)
To: openbmc; +Cc: clg, Robert Lippert
Signed-off-by: Robert Lippert <rlippert@google.com>
---
drivers/mtd/spi-nor/spi-nor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index a68073afe8a8..e508c25a47f7 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -857,7 +857,7 @@ static const struct flash_info spi_nor_ids[] = {
{ "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) },
{ "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) },
{ "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) },
- { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_QUAD_READ) },
+ { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
{ "mx66l1g45g", INFO(0xc2201b, 0, 64 * 1024, 2048, SECT_4K | SPI_NOR_QUAD_READ) },
{ "mx66l1g55g", INFO(0xc2261b, 0, 64 * 1024, 2048, SPI_NOR_QUAD_READ) },
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH linux 2/3] mtd: spi-nor: aspeed: add support for SPI dual IO read mode
2017-01-10 1:05 [PATCH linux 0/3] ASPEED SPI driver support for dual IO mode Robert Lippert
2017-01-10 1:05 ` [PATCH linux 1/3] mtd: spi-nor: add SPI_NOR_DUAL_READ to mx66l51235l Robert Lippert
@ 2017-01-10 1:05 ` Robert Lippert
2017-01-10 8:10 ` Cédric Le Goater
2017-01-10 1:05 ` [PATCH linux 3/3] mtd: spi-nor: aspeed: fix DMA access on AST2500 Robert Lippert
2 siblings, 1 reply; 12+ messages in thread
From: Robert Lippert @ 2017-01-10 1:05 UTC (permalink / raw)
To: openbmc; +Cc: clg, Robert Lippert
Implements support for the dual IO read mode on aspeed SMC/FMC
controllers which uses both MISO and MOSI lines for data during a read
to double the read bandwidth.
Signed-off-by: Robert Lippert <rlippert@google.com>
---
drivers/mtd/spi-nor/aspeed-smc.c | 28 ++++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index d3bed34f5aa0..a8ca2ab308d7 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -540,6 +540,9 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
{
struct aspeed_smc_chip *chip = nor->priv;
int ret;
+ u32 ctl;
+ int i;
+ u8 dummy = 0;
mutex_lock(&chip->controller->mutex);
@@ -557,6 +560,18 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
aspeed_smc_start_user(nor);
aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
+
+ /* Send dummy bytes */
+ for (i = 0; i < (chip->nor.read_dummy / 8); ++i)
+ aspeed_smc_write_to_ahb(chip->base, &dummy, 1);
+
+ if (chip->nor.flash_read == SPI_NOR_DUAL) {
+ /* Switch to dual I/O mode for data cycle */
+ ctl = readl(chip->ctl) & ~CONTROL_SPI_IO_MODE_MASK;
+ ctl |= CONTROL_SPI_IO_DUAL_DATA;
+ writel(ctl, chip->ctl);
+ }
+
aspeed_smc_read_from_ahb(read_buf, chip->base, len);
aspeed_smc_stop_user(nor);
@@ -868,6 +883,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
cmd = CONTROL_SPI_COMMAND_MODE_NORMAL;
break;
case SPI_NOR_FAST:
+ case SPI_NOR_DUAL:
cmd = CONTROL_SPI_COMMAND_MODE_FREAD;
break;
default:
@@ -876,9 +892,13 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
}
chip->ctl_val[smc_read] |= cmd |
+ spi_control_fill_opcode(chip->nor.read_opcode) |
CONTROL_SPI_IO_DUMMY_CYCLES_SET(chip->nor.read_dummy / 8);
- dev_dbg(controller->dev, "base control register: %08x\n",
+ if (chip->nor.flash_read == SPI_NOR_DUAL)
+ chip->ctl_val[smc_read] |= CONTROL_SPI_IO_DUAL_DATA;
+
+ dev_info(controller->dev, "read control register: %08x\n",
chip->ctl_val[smc_read]);
return 0;
}
@@ -980,11 +1000,7 @@ static int aspeed_smc_probe(struct platform_device *pdev)
if (err)
continue;
- /*
- * XXX Add support for SPI_NOR_QUAD and SPI_NOR_DUAL attach
- * when board support is present as determined by of property.
- */
- err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_NORMAL);
+ err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_DUAL);
if (err)
continue;
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH linux 3/3] mtd: spi-nor: aspeed: fix DMA access on AST2500
2017-01-10 1:05 [PATCH linux 0/3] ASPEED SPI driver support for dual IO mode Robert Lippert
2017-01-10 1:05 ` [PATCH linux 1/3] mtd: spi-nor: add SPI_NOR_DUAL_READ to mx66l51235l Robert Lippert
2017-01-10 1:05 ` [PATCH linux 2/3] mtd: spi-nor: aspeed: add support for SPI dual IO read mode Robert Lippert
@ 2017-01-10 1:05 ` Robert Lippert
2017-01-10 7:54 ` Cédric Le Goater
2 siblings, 1 reply; 12+ messages in thread
From: Robert Lippert @ 2017-01-10 1:05 UTC (permalink / raw)
To: openbmc; +Cc: clg, Robert Lippert
AST2500 has additional bits in the dma_addr field. Its easier
to just write the full address into the register as the hardware
will handle the masking properly.
Signed-off-by: Robert Lippert <rlippert@google.com>
---
drivers/mtd/spi-nor/aspeed-smc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index a8ca2ab308d7..6cf159741acd 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -349,7 +349,7 @@ static int aspeed_smc_dma_wait(struct aspeed_smc_chip *chip)
}
#define DMA_LENGTH(x) (((x) - 4) & ~0xFE000003)
-#define DMA_ADDR(x) ((x) & ~0xE0000003)
+#define DMA_ADDR(x) ((x) & ~0x00000003)
static inline void aspeed_smc_chip_configure(struct aspeed_smc_chip *chip,
u32 ctl)
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH linux 1/3] mtd: spi-nor: add SPI_NOR_DUAL_READ to mx66l51235l
2017-01-10 1:05 ` [PATCH linux 1/3] mtd: spi-nor: add SPI_NOR_DUAL_READ to mx66l51235l Robert Lippert
@ 2017-01-10 7:54 ` Cédric Le Goater
2017-01-10 22:55 ` Rob Lippert
0 siblings, 1 reply; 12+ messages in thread
From: Cédric Le Goater @ 2017-01-10 7:54 UTC (permalink / raw)
To: Robert Lippert, openbmc
On 01/10/2017 02:05 AM, Robert Lippert wrote:
> Signed-off-by: Robert Lippert <rlippert@google.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
This would be good to upstream I think. As I need to send
a little patchset for the label support in mainline Linux,
I can include it if you want.
C.
> ---
>
> drivers/mtd/spi-nor/spi-nor.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index a68073afe8a8..e508c25a47f7 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -857,7 +857,7 @@ static const struct flash_info spi_nor_ids[] = {
> { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) },
> { "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) },
> { "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) },
> - { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_QUAD_READ) },
> + { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> { "mx66l1g45g", INFO(0xc2201b, 0, 64 * 1024, 2048, SECT_4K | SPI_NOR_QUAD_READ) },
> { "mx66l1g55g", INFO(0xc2261b, 0, 64 * 1024, 2048, SPI_NOR_QUAD_READ) },
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH linux 3/3] mtd: spi-nor: aspeed: fix DMA access on AST2500
2017-01-10 1:05 ` [PATCH linux 3/3] mtd: spi-nor: aspeed: fix DMA access on AST2500 Robert Lippert
@ 2017-01-10 7:54 ` Cédric Le Goater
0 siblings, 0 replies; 12+ messages in thread
From: Cédric Le Goater @ 2017-01-10 7:54 UTC (permalink / raw)
To: Robert Lippert, openbmc
On 01/10/2017 02:05 AM, Robert Lippert wrote:
> AST2500 has additional bits in the dma_addr field. Its easier
> to just write the full address into the register as the hardware
> will handle the masking properly.
Yes definitely,
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Not for mainline yet though.
C.
> Signed-off-by: Robert Lippert <rlippert@google.com>
> ---
>
> drivers/mtd/spi-nor/aspeed-smc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
> index a8ca2ab308d7..6cf159741acd 100644
> --- a/drivers/mtd/spi-nor/aspeed-smc.c
> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
> @@ -349,7 +349,7 @@ static int aspeed_smc_dma_wait(struct aspeed_smc_chip *chip)
> }
>
> #define DMA_LENGTH(x) (((x) - 4) & ~0xFE000003)
> -#define DMA_ADDR(x) ((x) & ~0xE0000003)
> +#define DMA_ADDR(x) ((x) & ~0x00000003)
>
> static inline void aspeed_smc_chip_configure(struct aspeed_smc_chip *chip,
> u32 ctl)
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH linux 2/3] mtd: spi-nor: aspeed: add support for SPI dual IO read mode
2017-01-10 1:05 ` [PATCH linux 2/3] mtd: spi-nor: aspeed: add support for SPI dual IO read mode Robert Lippert
@ 2017-01-10 8:10 ` Cédric Le Goater
2017-01-10 22:54 ` Rob Lippert
0 siblings, 1 reply; 12+ messages in thread
From: Cédric Le Goater @ 2017-01-10 8:10 UTC (permalink / raw)
To: Robert Lippert, openbmc
Hello Robert,
On 01/10/2017 02:05 AM, Robert Lippert wrote:
> Implements support for the dual IO read mode on aspeed SMC/FMC
> controllers which uses both MISO and MOSI lines for data during a read
> to double the read bandwidth.
>
> Signed-off-by: Robert Lippert <rlippert@google.com>
> ---
>
> drivers/mtd/spi-nor/aspeed-smc.c | 28 ++++++++++++++++++++++------
> 1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
> index d3bed34f5aa0..a8ca2ab308d7 100644
> --- a/drivers/mtd/spi-nor/aspeed-smc.c
> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
> @@ -540,6 +540,9 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
> {
> struct aspeed_smc_chip *chip = nor->priv;
> int ret;
> + u32 ctl;
> + int i;
> + u8 dummy = 0;
OxFF would be a better value.
> mutex_lock(&chip->controller->mutex);
>
> @@ -557,6 +560,18 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
>
> aspeed_smc_start_user(nor);
> aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
> +
> + /* Send dummy bytes */
> + for (i = 0; i < (chip->nor.read_dummy / 8); ++i)
the parentheses are not needed I think.
> + aspeed_smc_write_to_ahb(chip->base, &dummy, 1);
> +
> + if (chip->nor.flash_read == SPI_NOR_DUAL) {
> + /* Switch to dual I/O mode for data cycle */
> + ctl = readl(chip->ctl) & ~CONTROL_SPI_IO_MODE_MASK;
> + ctl |= CONTROL_SPI_IO_DUAL_DATA;
> + writel(ctl, chip->ctl);
> + }
So why can not we set the controller in aspeed_smc_chip_setup_finish()
once and for all ?
> +
> aspeed_smc_read_from_ahb(read_buf, chip->base, len);
> aspeed_smc_stop_user(nor);
>
> @@ -868,6 +883,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
> cmd = CONTROL_SPI_COMMAND_MODE_NORMAL;
> break;
> case SPI_NOR_FAST:
> + case SPI_NOR_DUAL:
> cmd = CONTROL_SPI_COMMAND_MODE_FREAD;
> break;
> default:
> @@ -876,9 +892,13 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
> }
>
> chip->ctl_val[smc_read] |= cmd |
> + spi_control_fill_opcode(chip->nor.read_opcode) |
> CONTROL_SPI_IO_DUMMY_CYCLES_SET(chip->nor.read_dummy / 8);
>
> - dev_dbg(controller->dev, "base control register: %08x\n",
> + if (chip->nor.flash_read == SPI_NOR_DUAL)
> + chip->ctl_val[smc_read] |= CONTROL_SPI_IO_DUAL_DATA;
may be we can move that part in the switch setting above ?
> + dev_info(controller->dev, "read control register: %08x\n",
> chip->ctl_val[smc_read]);
may be keep dev_dbg, but the change of 'base' by 'read' is ok.
Thanks,
C.
> return 0;
> }
> @@ -980,11 +1000,7 @@ static int aspeed_smc_probe(struct platform_device *pdev)
> if (err)
> continue;
>
> - /*
> - * XXX Add support for SPI_NOR_QUAD and SPI_NOR_DUAL attach
> - * when board support is present as determined by of property.
> - */
> - err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_NORMAL);
> + err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_DUAL);
> if (err)
> continue;
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH linux 2/3] mtd: spi-nor: aspeed: add support for SPI dual IO read mode
2017-01-10 8:10 ` Cédric Le Goater
@ 2017-01-10 22:54 ` Rob Lippert
2017-01-11 11:43 ` Cédric Le Goater
0 siblings, 1 reply; 12+ messages in thread
From: Rob Lippert @ 2017-01-10 22:54 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: openbmc, Robert Lippert
On Tue, Jan 10, 2017 at 12:10 AM, Cédric Le Goater <clg@kaod.org> wrote:
>
> Hello Robert,
>
> On 01/10/2017 02:05 AM, Robert Lippert wrote:
> > Implements support for the dual IO read mode on aspeed SMC/FMC
> > controllers which uses both MISO and MOSI lines for data during a read
> > to double the read bandwidth.
> >
> > Signed-off-by: Robert Lippert <rlippert@google.com>
> > ---
> >
> > drivers/mtd/spi-nor/aspeed-smc.c | 28 ++++++++++++++++++++++------
> > 1 file changed, 22 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
> > index d3bed34f5aa0..a8ca2ab308d7 100644
> > --- a/drivers/mtd/spi-nor/aspeed-smc.c
> > +++ b/drivers/mtd/spi-nor/aspeed-smc.c
> > @@ -540,6 +540,9 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
> > {
> > struct aspeed_smc_chip *chip = nor->priv;
> > int ret;
> > + u32 ctl;
> > + int i;
> > + u8 dummy = 0;
>
> OxFF would be a better value.
OK.
>
> > mutex_lock(&chip->controller->mutex);
> >
> > @@ -557,6 +560,18 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
> >
> > aspeed_smc_start_user(nor);
> > aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
> > +
> > + /* Send dummy bytes */
> > + for (i = 0; i < (chip->nor.read_dummy / 8); ++i)
>
> the parentheses are not needed I think.
Yes not needed, removed.
>
> > + aspeed_smc_write_to_ahb(chip->base, &dummy, 1);
> > +
> > + if (chip->nor.flash_read == SPI_NOR_DUAL) {
> > + /* Switch to dual I/O mode for data cycle */
> > + ctl = readl(chip->ctl) & ~CONTROL_SPI_IO_MODE_MASK;
> > + ctl |= CONTROL_SPI_IO_DUAL_DATA;
> > + writel(ctl, chip->ctl);
> > + }
>
> So why can not we set the controller in aspeed_smc_chip_setup_finish()
> once and for all ?
It needs to make sure to only effect the data part of the operation
(the command+addr writes to the device should not be dual-IO mode),
and also needs to not interfere with the aspeed_smc_read_reg read
calls in which the data phase cannot be dual IO mode.
>
> > +
> > aspeed_smc_read_from_ahb(read_buf, chip->base, len);
> > aspeed_smc_stop_user(nor);
> >
> > @@ -868,6 +883,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
> > cmd = CONTROL_SPI_COMMAND_MODE_NORMAL;
> > break;
> > case SPI_NOR_FAST:
> > + case SPI_NOR_DUAL:
> > cmd = CONTROL_SPI_COMMAND_MODE_FREAD;
> > break;
> > default:
> > @@ -876,9 +892,13 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
> > }
> >
> > chip->ctl_val[smc_read] |= cmd |
> > + spi_control_fill_opcode(chip->nor.read_opcode) |
> > CONTROL_SPI_IO_DUMMY_CYCLES_SET(chip->nor.read_dummy / 8);
> >
> > - dev_dbg(controller->dev, "base control register: %08x\n",
> > + if (chip->nor.flash_read == SPI_NOR_DUAL)
> > + chip->ctl_val[smc_read] |= CONTROL_SPI_IO_DUAL_DATA;
>
> may be we can move that part in the switch setting above ?
Done.
>
> > + dev_info(controller->dev, "read control register: %08x\n",
> > chip->ctl_val[smc_read]);
>
> may be keep dev_dbg, but the change of 'base' by 'read' is ok.
OK.
>
> Thanks,
>
> C.
>
> > return 0;
> > }
> > @@ -980,11 +1000,7 @@ static int aspeed_smc_probe(struct platform_device *pdev)
> > if (err)
> > continue;
> >
> > - /*
> > - * XXX Add support for SPI_NOR_QUAD and SPI_NOR_DUAL attach
> > - * when board support is present as determined by of property.
> > - */
> > - err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_NORMAL);
> > + err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_DUAL);
> > if (err)
> > continue;
> >
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH linux 1/3] mtd: spi-nor: add SPI_NOR_DUAL_READ to mx66l51235l
2017-01-10 7:54 ` Cédric Le Goater
@ 2017-01-10 22:55 ` Rob Lippert
0 siblings, 0 replies; 12+ messages in thread
From: Rob Lippert @ 2017-01-10 22:55 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: openbmc, Robert Lippert
On Mon, Jan 9, 2017 at 11:54 PM, Cédric Le Goater <clg@kaod.org> wrote:
> On 01/10/2017 02:05 AM, Robert Lippert wrote:
>> Signed-off-by: Robert Lippert <rlippert@google.com>
>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>
> This would be good to upstream I think. As I need to send
> a little patchset for the label support in mainline Linux,
> I can include it if you want.
Sounds good to me, no issue with including this change in your patchset.
>
> C.
>
>> ---
>>
>> drivers/mtd/spi-nor/spi-nor.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index a68073afe8a8..e508c25a47f7 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -857,7 +857,7 @@ static const struct flash_info spi_nor_ids[] = {
>> { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) },
>> { "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) },
>> { "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) },
>> - { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_QUAD_READ) },
>> + { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>> { "mx66l1g45g", INFO(0xc2201b, 0, 64 * 1024, 2048, SECT_4K | SPI_NOR_QUAD_READ) },
>> { "mx66l1g55g", INFO(0xc2261b, 0, 64 * 1024, 2048, SPI_NOR_QUAD_READ) },
>>
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH linux 2/3] mtd: spi-nor: aspeed: add support for SPI dual IO read mode
2017-01-10 22:54 ` Rob Lippert
@ 2017-01-11 11:43 ` Cédric Le Goater
2017-01-11 18:34 ` Rob Lippert
0 siblings, 1 reply; 12+ messages in thread
From: Cédric Le Goater @ 2017-01-11 11:43 UTC (permalink / raw)
To: Rob Lippert; +Cc: openbmc, Robert Lippert
On 01/10/2017 11:54 PM, Rob Lippert wrote:
> On Tue, Jan 10, 2017 at 12:10 AM, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> Hello Robert,
>>
>> On 01/10/2017 02:05 AM, Robert Lippert wrote:
>>> Implements support for the dual IO read mode on aspeed SMC/FMC
>>> controllers which uses both MISO and MOSI lines for data during a read
>>> to double the read bandwidth.
>>>
>>> Signed-off-by: Robert Lippert <rlippert@google.com>
>>> ---
>>>
>>> drivers/mtd/spi-nor/aspeed-smc.c | 28 ++++++++++++++++++++++------
>>> 1 file changed, 22 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>>> index d3bed34f5aa0..a8ca2ab308d7 100644
>>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>>> @@ -540,6 +540,9 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
>>> {
>>> struct aspeed_smc_chip *chip = nor->priv;
>>> int ret;
>>> + u32 ctl;
>>> + int i;
>>> + u8 dummy = 0;
>>
>> OxFF would be a better value.
>
> OK.
>
>>
>>> mutex_lock(&chip->controller->mutex);
>>>
>>> @@ -557,6 +560,18 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
>>>
>>> aspeed_smc_start_user(nor);
>>> aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
>>> +
>>> + /* Send dummy bytes */
>>> + for (i = 0; i < (chip->nor.read_dummy / 8); ++i)
>>
>> the parentheses are not needed I think.
>
> Yes not needed, removed.
>
>>
>>> + aspeed_smc_write_to_ahb(chip->base, &dummy, 1);
>>> +
>>> + if (chip->nor.flash_read == SPI_NOR_DUAL) {
>>> + /* Switch to dual I/O mode for data cycle */
>>> + ctl = readl(chip->ctl) & ~CONTROL_SPI_IO_MODE_MASK;
>>> + ctl |= CONTROL_SPI_IO_DUAL_DATA;
>>> + writel(ctl, chip->ctl);
>>> + }
>>
>> So why can not we set the controller in aspeed_smc_chip_setup_finish()
>> once and for all ?
>
> It needs to make sure to only effect the data part of the operation
> (the command+addr writes to the device should not be dual-IO mode),
> and also needs to not interfere with the aspeed_smc_read_reg read
> calls in which the data phase cannot be dual IO mode.
ok. it makes sense. Is it much faster ?
Thanks,
C.
>>
>>> +
>>> aspeed_smc_read_from_ahb(read_buf, chip->base, len);
>>> aspeed_smc_stop_user(nor);
>>>
>>> @@ -868,6 +883,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>> cmd = CONTROL_SPI_COMMAND_MODE_NORMAL;
>>> break;
>>> case SPI_NOR_FAST:
>>> + case SPI_NOR_DUAL:
>>> cmd = CONTROL_SPI_COMMAND_MODE_FREAD;
>>> break;
>>> default:
>>> @@ -876,9 +892,13 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>> }
>>>
>>> chip->ctl_val[smc_read] |= cmd |
>>> + spi_control_fill_opcode(chip->nor.read_opcode) |
>>> CONTROL_SPI_IO_DUMMY_CYCLES_SET(chip->nor.read_dummy / 8);
>>>
>>> - dev_dbg(controller->dev, "base control register: %08x\n",
>>> + if (chip->nor.flash_read == SPI_NOR_DUAL)
>>> + chip->ctl_val[smc_read] |= CONTROL_SPI_IO_DUAL_DATA;
>>
>> may be we can move that part in the switch setting above ?
>
> Done.
>
>>
>>> + dev_info(controller->dev, "read control register: %08x\n",
>>> chip->ctl_val[smc_read]);
>>
>> may be keep dev_dbg, but the change of 'base' by 'read' is ok.
>
> OK.
>
>>
>> Thanks,
>>
>> C.
>>
>>> return 0;
>>> }
>>> @@ -980,11 +1000,7 @@ static int aspeed_smc_probe(struct platform_device *pdev)
>>> if (err)
>>> continue;
>>>
>>> - /*
>>> - * XXX Add support for SPI_NOR_QUAD and SPI_NOR_DUAL attach
>>> - * when board support is present as determined by of property.
>>> - */
>>> - err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_NORMAL);
>>> + err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_DUAL);
>>> if (err)
>>> continue;
>>>
>>>
>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH linux 2/3] mtd: spi-nor: aspeed: add support for SPI dual IO read mode
2017-01-11 11:43 ` Cédric Le Goater
@ 2017-01-11 18:34 ` Rob Lippert
2017-01-11 18:50 ` Cédric Le Goater
0 siblings, 1 reply; 12+ messages in thread
From: Rob Lippert @ 2017-01-11 18:34 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: openbmc, Robert Lippert
On Wed, Jan 11, 2017 at 3:43 AM, Cédric Le Goater <clg@kaod.org> wrote:
> On 01/10/2017 11:54 PM, Rob Lippert wrote:
>> On Tue, Jan 10, 2017 at 12:10 AM, Cédric Le Goater <clg@kaod.org> wrote:
>>>
>>> Hello Robert,
>>>
>>> On 01/10/2017 02:05 AM, Robert Lippert wrote:
>>>> Implements support for the dual IO read mode on aspeed SMC/FMC
>>>> controllers which uses both MISO and MOSI lines for data during a read
>>>> to double the read bandwidth.
>>>>
>>>> Signed-off-by: Robert Lippert <rlippert@google.com>
>>>> ---
>>>>
>>>> drivers/mtd/spi-nor/aspeed-smc.c | 28 ++++++++++++++++++++++------
>>>> 1 file changed, 22 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>>>> index d3bed34f5aa0..a8ca2ab308d7 100644
>>>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>>>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>>>> @@ -540,6 +540,9 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
>>>> {
>>>> struct aspeed_smc_chip *chip = nor->priv;
>>>> int ret;
>>>> + u32 ctl;
>>>> + int i;
>>>> + u8 dummy = 0;
>>>
>>> OxFF would be a better value.
>>
>> OK.
>>
>>>
>>>> mutex_lock(&chip->controller->mutex);
>>>>
>>>> @@ -557,6 +560,18 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
>>>>
>>>> aspeed_smc_start_user(nor);
>>>> aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
>>>> +
>>>> + /* Send dummy bytes */
>>>> + for (i = 0; i < (chip->nor.read_dummy / 8); ++i)
>>>
>>> the parentheses are not needed I think.
>>
>> Yes not needed, removed.
>>
>>>
>>>> + aspeed_smc_write_to_ahb(chip->base, &dummy, 1);
>>>> +
>>>> + if (chip->nor.flash_read == SPI_NOR_DUAL) {
>>>> + /* Switch to dual I/O mode for data cycle */
>>>> + ctl = readl(chip->ctl) & ~CONTROL_SPI_IO_MODE_MASK;
>>>> + ctl |= CONTROL_SPI_IO_DUAL_DATA;
>>>> + writel(ctl, chip->ctl);
>>>> + }
>>>
>>> So why can not we set the controller in aspeed_smc_chip_setup_finish()
>>> once and for all ?
>>
>> It needs to make sure to only effect the data part of the operation
>> (the command+addr writes to the device should not be dual-IO mode),
>> and also needs to not interfere with the aspeed_smc_read_reg read
>> calls in which the data phase cannot be dual IO mode.
>
> ok. it makes sense. Is it much faster ?
Yea it is about twice as much bandwidth, as I mentioned in the cover
letter it drops the time to dump an entire 64MB flash from 12s to 7s.
With a companion change to u-boot to double the SPI frequency to
104Mhz that time drops to 3s and speeds up the entire bmc boot process
(including uboot) by about 15s.
Thanks,
-Rob
>
> Thanks,
>
> C.
>
>>>
>>>> +
>>>> aspeed_smc_read_from_ahb(read_buf, chip->base, len);
>>>> aspeed_smc_stop_user(nor);
>>>>
>>>> @@ -868,6 +883,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>>> cmd = CONTROL_SPI_COMMAND_MODE_NORMAL;
>>>> break;
>>>> case SPI_NOR_FAST:
>>>> + case SPI_NOR_DUAL:
>>>> cmd = CONTROL_SPI_COMMAND_MODE_FREAD;
>>>> break;
>>>> default:
>>>> @@ -876,9 +892,13 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>>> }
>>>>
>>>> chip->ctl_val[smc_read] |= cmd |
>>>> + spi_control_fill_opcode(chip->nor.read_opcode) |
>>>> CONTROL_SPI_IO_DUMMY_CYCLES_SET(chip->nor.read_dummy / 8);
>>>>
>>>> - dev_dbg(controller->dev, "base control register: %08x\n",
>>>> + if (chip->nor.flash_read == SPI_NOR_DUAL)
>>>> + chip->ctl_val[smc_read] |= CONTROL_SPI_IO_DUAL_DATA;
>>>
>>> may be we can move that part in the switch setting above ?
>>
>> Done.
>>
>>>
>>>> + dev_info(controller->dev, "read control register: %08x\n",
>>>> chip->ctl_val[smc_read]);
>>>
>>> may be keep dev_dbg, but the change of 'base' by 'read' is ok.
>>
>> OK.
>>
>>>
>>> Thanks,
>>>
>>> C.
>>>
>>>> return 0;
>>>> }
>>>> @@ -980,11 +1000,7 @@ static int aspeed_smc_probe(struct platform_device *pdev)
>>>> if (err)
>>>> continue;
>>>>
>>>> - /*
>>>> - * XXX Add support for SPI_NOR_QUAD and SPI_NOR_DUAL attach
>>>> - * when board support is present as determined by of property.
>>>> - */
>>>> - err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_NORMAL);
>>>> + err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_DUAL);
>>>> if (err)
>>>> continue;
>>>>
>>>>
>>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH linux 2/3] mtd: spi-nor: aspeed: add support for SPI dual IO read mode
2017-01-11 18:34 ` Rob Lippert
@ 2017-01-11 18:50 ` Cédric Le Goater
0 siblings, 0 replies; 12+ messages in thread
From: Cédric Le Goater @ 2017-01-11 18:50 UTC (permalink / raw)
To: Rob Lippert; +Cc: openbmc, Robert Lippert
On 01/11/2017 07:34 PM, Rob Lippert wrote:
> On Wed, Jan 11, 2017 at 3:43 AM, Cédric Le Goater <clg@kaod.org> wrote:
>> On 01/10/2017 11:54 PM, Rob Lippert wrote:
>>> On Tue, Jan 10, 2017 at 12:10 AM, Cédric Le Goater <clg@kaod.org> wrote:
>>>>
>>>> Hello Robert,
>>>>
>>>> On 01/10/2017 02:05 AM, Robert Lippert wrote:
>>>>> Implements support for the dual IO read mode on aspeed SMC/FMC
>>>>> controllers which uses both MISO and MOSI lines for data during a read
>>>>> to double the read bandwidth.
>>>>>
>>>>> Signed-off-by: Robert Lippert <rlippert@google.com>
>>>>> ---
>>>>>
>>>>> drivers/mtd/spi-nor/aspeed-smc.c | 28 ++++++++++++++++++++++------
>>>>> 1 file changed, 22 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>>>>> index d3bed34f5aa0..a8ca2ab308d7 100644
>>>>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>>>>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>>>>> @@ -540,6 +540,9 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
>>>>> {
>>>>> struct aspeed_smc_chip *chip = nor->priv;
>>>>> int ret;
>>>>> + u32 ctl;
>>>>> + int i;
>>>>> + u8 dummy = 0;
>>>>
>>>> OxFF would be a better value.
>>>
>>> OK.
>>>
>>>>
>>>>> mutex_lock(&chip->controller->mutex);
>>>>>
>>>>> @@ -557,6 +560,18 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
>>>>>
>>>>> aspeed_smc_start_user(nor);
>>>>> aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
>>>>> +
>>>>> + /* Send dummy bytes */
>>>>> + for (i = 0; i < (chip->nor.read_dummy / 8); ++i)
>>>>
>>>> the parentheses are not needed I think.
>>>
>>> Yes not needed, removed.
>>>
>>>>
>>>>> + aspeed_smc_write_to_ahb(chip->base, &dummy, 1);
>>>>> +
>>>>> + if (chip->nor.flash_read == SPI_NOR_DUAL) {
>>>>> + /* Switch to dual I/O mode for data cycle */
>>>>> + ctl = readl(chip->ctl) & ~CONTROL_SPI_IO_MODE_MASK;
>>>>> + ctl |= CONTROL_SPI_IO_DUAL_DATA;
>>>>> + writel(ctl, chip->ctl);
>>>>> + }
>>>>
>>>> So why can not we set the controller in aspeed_smc_chip_setup_finish()
>>>> once and for all ?
>>>
>>> It needs to make sure to only effect the data part of the operation
>>> (the command+addr writes to the device should not be dual-IO mode),
>>> and also needs to not interfere with the aspeed_smc_read_reg read
>>> calls in which the data phase cannot be dual IO mode.
>>
>> ok. it makes sense. Is it much faster ?
>
> Yea it is about twice as much bandwidth, as I mentioned in the cover
> letter it drops the time to dump an entire 64MB flash from 12s to 7s.
Good. Sorry for asking, I scanned the cover a bit quickly it seems.
> With a companion change to u-boot to double the SPI frequency to
> 104Mhz that time drops to 3s and speeds up the entire bmc boot process
> (including uboot) by about 15s.
yes we need to add that to the driver also. I have a few other patches
improving the timing settings but I have been focusing on upstreaming
first.
Thanks,
C.
> Thanks,
> -Rob
>
>>
>> Thanks,
>>
>> C.
>>
>>>>
>>>>> +
>>>>> aspeed_smc_read_from_ahb(read_buf, chip->base, len);
>>>>> aspeed_smc_stop_user(nor);
>>>>>
>>>>> @@ -868,6 +883,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>>>> cmd = CONTROL_SPI_COMMAND_MODE_NORMAL;
>>>>> break;
>>>>> case SPI_NOR_FAST:
>>>>> + case SPI_NOR_DUAL:
>>>>> cmd = CONTROL_SPI_COMMAND_MODE_FREAD;
>>>>> break;
>>>>> default:
>>>>> @@ -876,9 +892,13 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>>>> }
>>>>>
>>>>> chip->ctl_val[smc_read] |= cmd |
>>>>> + spi_control_fill_opcode(chip->nor.read_opcode) |
>>>>> CONTROL_SPI_IO_DUMMY_CYCLES_SET(chip->nor.read_dummy / 8);
>>>>>
>>>>> - dev_dbg(controller->dev, "base control register: %08x\n",
>>>>> + if (chip->nor.flash_read == SPI_NOR_DUAL)
>>>>> + chip->ctl_val[smc_read] |= CONTROL_SPI_IO_DUAL_DATA;
>>>>
>>>> may be we can move that part in the switch setting above ?
>>>
>>> Done.
>>>
>>>>
>>>>> + dev_info(controller->dev, "read control register: %08x\n",
>>>>> chip->ctl_val[smc_read]);
>>>>
>>>> may be keep dev_dbg, but the change of 'base' by 'read' is ok.
>>>
>>> OK.
>>>
>>>>
>>>> Thanks,
>>>>
>>>> C.
>>>>
>>>>> return 0;
>>>>> }
>>>>> @@ -980,11 +1000,7 @@ static int aspeed_smc_probe(struct platform_device *pdev)
>>>>> if (err)
>>>>> continue;
>>>>>
>>>>> - /*
>>>>> - * XXX Add support for SPI_NOR_QUAD and SPI_NOR_DUAL attach
>>>>> - * when board support is present as determined by of property.
>>>>> - */
>>>>> - err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_NORMAL);
>>>>> + err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_DUAL);
>>>>> if (err)
>>>>> continue;
>>>>>
>>>>>
>>>>
>>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-01-11 19:00 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-10 1:05 [PATCH linux 0/3] ASPEED SPI driver support for dual IO mode Robert Lippert
2017-01-10 1:05 ` [PATCH linux 1/3] mtd: spi-nor: add SPI_NOR_DUAL_READ to mx66l51235l Robert Lippert
2017-01-10 7:54 ` Cédric Le Goater
2017-01-10 22:55 ` Rob Lippert
2017-01-10 1:05 ` [PATCH linux 2/3] mtd: spi-nor: aspeed: add support for SPI dual IO read mode Robert Lippert
2017-01-10 8:10 ` Cédric Le Goater
2017-01-10 22:54 ` Rob Lippert
2017-01-11 11:43 ` Cédric Le Goater
2017-01-11 18:34 ` Rob Lippert
2017-01-11 18:50 ` Cédric Le Goater
2017-01-10 1:05 ` [PATCH linux 3/3] mtd: spi-nor: aspeed: fix DMA access on AST2500 Robert Lippert
2017-01-10 7:54 ` Cédric Le Goater
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.