* [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
* 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 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
* [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
* 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 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
* [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 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
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.