* [PATCH dev-5.0 0/4] spi-nor: aspeed: add support for the 4B opcodes @ 2019-04-17 13:39 Cédric Le Goater 2019-04-17 13:39 ` [PATCH dev-5.0 1/4] mtd: spi-nor: aspeed: introduce a aspeed_smc_default_read() helper Cédric Le Goater ` (6 more replies) 0 siblings, 7 replies; 24+ messages in thread From: Cédric Le Goater @ 2019-04-17 13:39 UTC (permalink / raw) To: openbmc; +Cc: Joel Stanley, Andrew Jeffery, Cédric Le Goater Here is a little series providing cleanups on the Aspeed SMC Controller driver and adding support for the 4B opcodes which were recently added to the Linux kernels 5.0.x. The use of the 4B opcodes was breaking the read of the golden buffer done at slow speed in the optimization read sequence. The code assumed that the chip was in 4B address mode, as if a EN4B opcode had been sent, but this is not the case anymore with 4B opcodes. The golden buffer is now read with a SPINOR_OP_READ_4B (0x13) when the chip supports 4B opcodes. It should fix accesses to the palmetto PNOR and the Witherspoon BMC flash modules. *Please test !* Thanks, C. Cédric Le Goater (4): mtd: spi-nor: aspeed: introduce a aspeed_smc_default_read() helper mtd: spi-nor: aspeed: clarify 4BYTE address mode mask mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer mtd: spi-nor: aspeed: add support for the 4B opcodes drivers/mtd/spi-nor/aspeed-smc.c | 40 +++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 11 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH dev-5.0 1/4] mtd: spi-nor: aspeed: introduce a aspeed_smc_default_read() helper 2019-04-17 13:39 [PATCH dev-5.0 0/4] spi-nor: aspeed: add support for the 4B opcodes Cédric Le Goater @ 2019-04-17 13:39 ` Cédric Le Goater 2019-04-17 18:09 ` Alexander Amelkin 2019-04-17 13:39 ` [PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE address mode mask Cédric Le Goater ` (5 subsequent siblings) 6 siblings, 1 reply; 24+ messages in thread From: Cédric Le Goater @ 2019-04-17 13:39 UTC (permalink / raw) To: openbmc; +Cc: Joel Stanley, Andrew Jeffery, Cédric Le Goater Signed-off-by: Cédric Le Goater <clg@kaod.org> --- drivers/mtd/spi-nor/aspeed-smc.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c index ddf7ae78aa0a..ee3059b27c07 100644 --- a/drivers/mtd/spi-nor/aspeed-smc.c +++ b/drivers/mtd/spi-nor/aspeed-smc.c @@ -882,6 +882,17 @@ static const uint32_t aspeed_smc_hclk_divs[] = { }; #define ASPEED_SMC_HCLK_DIV(i) (aspeed_smc_hclk_divs[(i) - 1] << 8) +static u32 aspeed_smc_default_read(struct aspeed_smc_chip *chip) +{ + return (chip->ctl_val[smc_read] & 0x2000) | + (0x00 << 28) | /* Single bit */ + (0x00 << 24) | /* CE# max */ + (0x03 << 16) | /* use normal reads */ + (0x00 << 8) | /* HCLK/16 */ + (0x00 << 6) | /* no dummy cycle */ + (0x00); /* normal mode */ +} + static int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip, u32 max_freq) { @@ -898,13 +909,7 @@ static int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip, /* We start with the dumbest setting (keep 4Byte bit) and read * some data */ - chip->ctl_val[smc_read] = (chip->ctl_val[smc_read] & 0x2000) | - (0x00 << 28) | /* Single bit */ - (0x00 << 24) | /* CE# max */ - (0x03 << 16) | /* use normal reads */ - (0x00 << 8) | /* HCLK/16 */ - (0x00 << 6) | /* no dummy cycle */ - (0x00); /* normal read */ + chip->ctl_val[smc_read] = aspeed_smc_default_read(chip); writel(chip->ctl_val[smc_read], chip->ctl); -- 2.20.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH dev-5.0 1/4] mtd: spi-nor: aspeed: introduce a aspeed_smc_default_read() helper 2019-04-17 13:39 ` [PATCH dev-5.0 1/4] mtd: spi-nor: aspeed: introduce a aspeed_smc_default_read() helper Cédric Le Goater @ 2019-04-17 18:09 ` Alexander Amelkin 2019-04-18 6:07 ` Andrew Jeffery 0 siblings, 1 reply; 24+ messages in thread From: Alexander Amelkin @ 2019-04-17 18:09 UTC (permalink / raw) To: openbmc [-- Attachment #1.1: Type: text/plain, Size: 981 bytes --] 17.04.2019 16:39, Cédric Le Goater wrote: > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > drivers/mtd/spi-nor/aspeed-smc.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c > index ddf7ae78aa0a..ee3059b27c07 100644 > --- a/drivers/mtd/spi-nor/aspeed-smc.c > +++ b/drivers/mtd/spi-nor/aspeed-smc.c > @@ -882,6 +882,17 @@ static const uint32_t aspeed_smc_hclk_divs[] = { > }; > #define ASPEED_SMC_HCLK_DIV(i) (aspeed_smc_hclk_divs[(i) - 1] << 8) > > +static u32 aspeed_smc_default_read(struct aspeed_smc_chip *chip) > +{ > + return (chip->ctl_val[smc_read] & 0x2000) | Cédric, isn't this a good time to get rid of these cached ctl_val values? Why not read/modify/store the actual register? What's the profit of this caching? With best regards, Alexander Amelkin, Leading BMC Software Engineer, YADRO https://yadro.com [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH dev-5.0 1/4] mtd: spi-nor: aspeed: introduce a aspeed_smc_default_read() helper 2019-04-17 18:09 ` Alexander Amelkin @ 2019-04-18 6:07 ` Andrew Jeffery 2019-04-18 6:23 ` Cédric Le Goater 0 siblings, 1 reply; 24+ messages in thread From: Andrew Jeffery @ 2019-04-18 6:07 UTC (permalink / raw) To: Alexander Amelkin, openbmc On Thu, 18 Apr 2019, at 03:40, Alexander Amelkin wrote: > 17.04.2019 16:39, Cédric Le Goater wrote: > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > > --- > > drivers/mtd/spi-nor/aspeed-smc.c | 19 ++++++++++++------- > > 1 file changed, 12 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c > > index ddf7ae78aa0a..ee3059b27c07 100644 > > --- a/drivers/mtd/spi-nor/aspeed-smc.c > > +++ b/drivers/mtd/spi-nor/aspeed-smc.c > > @@ -882,6 +882,17 @@ static const uint32_t aspeed_smc_hclk_divs[] = { > > }; > > #define ASPEED_SMC_HCLK_DIV(i) (aspeed_smc_hclk_divs[(i) - 1] << 8) > > > > +static u32 aspeed_smc_default_read(struct aspeed_smc_chip *chip) > > +{ > > + return (chip->ctl_val[smc_read] & 0x2000) | > > Cédric, isn't this a good time to get rid of these cached ctl_val values? > > Why not read/modify/store the actual register? > > What's the profit of this caching? Reasonable question, but I've merged the change as-is to dev-5.0 on the basis that it helps fix read corruption on AC-cycles. We can address this in a follow-up patch if necessary. Cheers, Andrew > > With best regards, > Alexander Amelkin, > Leading BMC Software Engineer, YADRO > https://yadro.com > > > > Attachments: > * signature.asc ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH dev-5.0 1/4] mtd: spi-nor: aspeed: introduce a aspeed_smc_default_read() helper 2019-04-18 6:07 ` Andrew Jeffery @ 2019-04-18 6:23 ` Cédric Le Goater 0 siblings, 0 replies; 24+ messages in thread From: Cédric Le Goater @ 2019-04-18 6:23 UTC (permalink / raw) To: openbmc On 4/18/19 8:07 AM, Andrew Jeffery wrote: > > > On Thu, 18 Apr 2019, at 03:40, Alexander Amelkin wrote: >> 17.04.2019 16:39, Cédric Le Goater wrote: >>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> --- >>> drivers/mtd/spi-nor/aspeed-smc.c | 19 ++++++++++++------- >>> 1 file changed, 12 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c >>> index ddf7ae78aa0a..ee3059b27c07 100644 >>> --- a/drivers/mtd/spi-nor/aspeed-smc.c >>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c >>> @@ -882,6 +882,17 @@ static const uint32_t aspeed_smc_hclk_divs[] = { >>> }; >>> #define ASPEED_SMC_HCLK_DIV(i) (aspeed_smc_hclk_divs[(i) - 1] << 8) >>> >>> +static u32 aspeed_smc_default_read(struct aspeed_smc_chip *chip) >>> +{ >>> + return (chip->ctl_val[smc_read] & 0x2000) | >> >> Cédric, isn't this a good time to get rid of these cached ctl_val values? >> >> Why not read/modify/store the actual register? >> >> What's the profit of this caching? These values are computed when the controller is probed and when the chip devices are scanned (which takes a couple of seconds to do the training). They are the default values used when the flash contents is accessed through the flash MMIO window. When in User mode, for register operation, erase, write, other values are used, see read/write_reg operations, and we don't want to recompute the same values, it takes time and the HW has not changed anyhow. It is also safer as some FW still hack the value in the back of the Linux driver so we should not trust how the HW has been configured. I believe HIOMAP is available for P8 also now ? The state of chips is another problem. If the 4B enablement in the chip was dropped and if the controller is still configured to use 4B, then that's a problem. I agree. Using 4B opcodes only should help in the future. > Reasonable question, but I've merged the change as-is to dev-5.0 on the > basis that it helps fix read corruption on AC-cycles. > > We can address this in a follow-up patch if necessary. spi-mem should be taken into account now, which requires a large rewrite of the driver. Cheers, C. > > Cheers, > > Andrew > >> >> With best regards, >> Alexander Amelkin, >> Leading BMC Software Engineer, YADRO >> https://yadro.com >> >> >> >> Attachments: >> * signature.asc ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE address mode mask 2019-04-17 13:39 [PATCH dev-5.0 0/4] spi-nor: aspeed: add support for the 4B opcodes Cédric Le Goater 2019-04-17 13:39 ` [PATCH dev-5.0 1/4] mtd: spi-nor: aspeed: introduce a aspeed_smc_default_read() helper Cédric Le Goater @ 2019-04-17 13:39 ` Cédric Le Goater 2019-04-18 6:09 ` Andrew Jeffery 2019-04-17 13:39 ` [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer Cédric Le Goater ` (4 subsequent siblings) 6 siblings, 1 reply; 24+ messages in thread From: Cédric Le Goater @ 2019-04-17 13:39 UTC (permalink / raw) To: openbmc; +Cc: Joel Stanley, Andrew Jeffery, Cédric Le Goater Signed-off-by: Cédric Le Goater <clg@kaod.org> --- drivers/mtd/spi-nor/aspeed-smc.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c index ee3059b27c07..1437732fdea1 100644 --- a/drivers/mtd/spi-nor/aspeed-smc.c +++ b/drivers/mtd/spi-nor/aspeed-smc.c @@ -884,7 +884,15 @@ static const uint32_t aspeed_smc_hclk_divs[] = { static u32 aspeed_smc_default_read(struct aspeed_smc_chip *chip) { - return (chip->ctl_val[smc_read] & 0x2000) | + /* + * Keep the 4Byte address mode on the AST2400 SPI controller. + * Other controllers set the 4Byte mode in the CE Control + * Register + */ + u32 ctl_mask = chip->controller->info == &spi_2400_info ? + CONTROL_IO_ADDRESS_4B : 0; + + return (chip->ctl_val[smc_read] & ctl_mask) | (0x00 << 28) | /* Single bit */ (0x00 << 24) | /* CE# max */ (0x03 << 16) | /* use normal reads */ -- 2.20.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE address mode mask 2019-04-17 13:39 ` [PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE address mode mask Cédric Le Goater @ 2019-04-18 6:09 ` Andrew Jeffery 0 siblings, 0 replies; 24+ messages in thread From: Andrew Jeffery @ 2019-04-18 6:09 UTC (permalink / raw) To: Cédric Le Goater, openbmc On Wed, 17 Apr 2019, at 23:10, Cédric Le Goater wrote: > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > drivers/mtd/spi-nor/aspeed-smc.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c > index ee3059b27c07..1437732fdea1 100644 > --- a/drivers/mtd/spi-nor/aspeed-smc.c > +++ b/drivers/mtd/spi-nor/aspeed-smc.c > @@ -884,7 +884,15 @@ static const uint32_t aspeed_smc_hclk_divs[] = { > > static u32 aspeed_smc_default_read(struct aspeed_smc_chip *chip) > { > - return (chip->ctl_val[smc_read] & 0x2000) | > + /* > + * Keep the 4Byte address mode on the AST2400 SPI controller. > + * Other controllers set the 4Byte mode in the CE Control > + * Register > + */ > + u32 ctl_mask = chip->controller->info == &spi_2400_info ? > + CONTROL_IO_ADDRESS_4B : 0; > + > + return (chip->ctl_val[smc_read] & ctl_mask) | > (0x00 << 28) | /* Single bit */ > (0x00 << 24) | /* CE# max */ > (0x03 << 16) | /* use normal reads */ > -- > 2.20.1 > > Applied to dev-5.0. Cheers, Andrew ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer 2019-04-17 13:39 [PATCH dev-5.0 0/4] spi-nor: aspeed: add support for the 4B opcodes Cédric Le Goater 2019-04-17 13:39 ` [PATCH dev-5.0 1/4] mtd: spi-nor: aspeed: introduce a aspeed_smc_default_read() helper Cédric Le Goater 2019-04-17 13:39 ` [PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE address mode mask Cédric Le Goater @ 2019-04-17 13:39 ` Cédric Le Goater 2019-04-18 6:10 ` Andrew Jeffery 2019-04-17 13:39 ` [PATCH dev-5.0 4/4] mtd: spi-nor: aspeed: add support for the 4B opcodes Cédric Le Goater ` (3 subsequent siblings) 6 siblings, 1 reply; 24+ messages in thread From: Cédric Le Goater @ 2019-04-17 13:39 UTC (permalink / raw) To: openbmc; +Cc: Joel Stanley, Andrew Jeffery, Cédric Le Goater aspeed_smc_read_from_ahb() only reads the first word which is not what we want. We want to capture a CALIBRATE_BUF_SIZE size window of the flash contents to optimize the read. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- drivers/mtd/spi-nor/aspeed-smc.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c index 1437732fdea1..7e289ecb1c99 100644 --- a/drivers/mtd/spi-nor/aspeed-smc.c +++ b/drivers/mtd/spi-nor/aspeed-smc.c @@ -796,8 +796,7 @@ static bool aspeed_smc_check_reads(struct aspeed_smc_chip *chip, int i; for (i = 0; i < 10; i++) { - aspeed_smc_read_from_ahb(test_buf, chip->ahb_base, - CALIBRATE_BUF_SIZE); + memcpy_fromio(test_buf, chip->ahb_base, CALIBRATE_BUF_SIZE); if (memcmp(test_buf, golden_buf, CALIBRATE_BUF_SIZE) != 0) return false; } @@ -921,8 +920,7 @@ static int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip, writel(chip->ctl_val[smc_read], chip->ctl); - aspeed_smc_read_from_ahb(golden_buf, chip->ahb_base, - CALIBRATE_BUF_SIZE); + memcpy_fromio(golden_buf, chip->ahb_base, CALIBRATE_BUF_SIZE); /* Establish our read mode with freq field set to 0 (HCLK/16) */ chip->ctl_val[smc_read] = save_read_val & 0xfffff0ff; -- 2.20.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer 2019-04-17 13:39 ` [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer Cédric Le Goater @ 2019-04-18 6:10 ` Andrew Jeffery 0 siblings, 0 replies; 24+ messages in thread From: Andrew Jeffery @ 2019-04-18 6:10 UTC (permalink / raw) To: Cédric Le Goater, openbmc On Wed, 17 Apr 2019, at 23:10, Cédric Le Goater wrote: > aspeed_smc_read_from_ahb() only reads the first word which is not what > we want. We want to capture a CALIBRATE_BUF_SIZE size window of the > flash contents to optimize the read. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > drivers/mtd/spi-nor/aspeed-smc.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/spi-nor/aspeed-smc.c > b/drivers/mtd/spi-nor/aspeed-smc.c > index 1437732fdea1..7e289ecb1c99 100644 > --- a/drivers/mtd/spi-nor/aspeed-smc.c > +++ b/drivers/mtd/spi-nor/aspeed-smc.c > @@ -796,8 +796,7 @@ static bool aspeed_smc_check_reads(struct > aspeed_smc_chip *chip, > int i; > > for (i = 0; i < 10; i++) { > - aspeed_smc_read_from_ahb(test_buf, chip->ahb_base, > - CALIBRATE_BUF_SIZE); > + memcpy_fromio(test_buf, chip->ahb_base, CALIBRATE_BUF_SIZE); > if (memcmp(test_buf, golden_buf, CALIBRATE_BUF_SIZE) != 0) > return false; > } > @@ -921,8 +920,7 @@ static int aspeed_smc_optimize_read(struct > aspeed_smc_chip *chip, > > writel(chip->ctl_val[smc_read], chip->ctl); > > - aspeed_smc_read_from_ahb(golden_buf, chip->ahb_base, > - CALIBRATE_BUF_SIZE); > + memcpy_fromio(golden_buf, chip->ahb_base, CALIBRATE_BUF_SIZE); > > /* Establish our read mode with freq field set to 0 (HCLK/16) */ > chip->ctl_val[smc_read] = save_read_val & 0xfffff0ff; > -- > 2.20.1 > > Applied to dev-5.0. Cheers, Andrew ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH dev-5.0 4/4] mtd: spi-nor: aspeed: add support for the 4B opcodes 2019-04-17 13:39 [PATCH dev-5.0 0/4] spi-nor: aspeed: add support for the 4B opcodes Cédric Le Goater ` (2 preceding siblings ...) 2019-04-17 13:39 ` [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer Cédric Le Goater @ 2019-04-17 13:39 ` Cédric Le Goater 2019-04-18 6:10 ` Andrew Jeffery 2019-04-18 21:23 ` [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer Milton Miller II ` (2 subsequent siblings) 6 siblings, 1 reply; 24+ messages in thread From: Cédric Le Goater @ 2019-04-17 13:39 UTC (permalink / raw) To: openbmc; +Cc: Joel Stanley, Andrew Jeffery, Cédric Le Goater Switch the default controller value to use the read mode in order to customize the command and use SPINOR_OP_READ_4B (0x13) when the chip supports 4B opcodes. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- drivers/mtd/spi-nor/aspeed-smc.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c index 7e289ecb1c99..1200b23416e3 100644 --- a/drivers/mtd/spi-nor/aspeed-smc.c +++ b/drivers/mtd/spi-nor/aspeed-smc.c @@ -890,14 +890,21 @@ static u32 aspeed_smc_default_read(struct aspeed_smc_chip *chip) */ u32 ctl_mask = chip->controller->info == &spi_2400_info ? CONTROL_IO_ADDRESS_4B : 0; + u8 cmd = chip->nor.flags & SNOR_F_4B_OPCODES ? SPINOR_OP_READ_4B : + SPINOR_OP_READ; + /* + * Use the "read command" mode to customize the opcode. In + * normal command mode, the value is necessarily READ (0x3) on + * the AST2400/2500 SoCs. + */ return (chip->ctl_val[smc_read] & ctl_mask) | (0x00 << 28) | /* Single bit */ (0x00 << 24) | /* CE# max */ - (0x03 << 16) | /* use normal reads */ + (cmd << 16) | /* use read mode to support 4B opcode */ (0x00 << 8) | /* HCLK/16 */ (0x00 << 6) | /* no dummy cycle */ - (0x00); /* normal mode */ + (0x01); /* read mode */ } static int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip, -- 2.20.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH dev-5.0 4/4] mtd: spi-nor: aspeed: add support for the 4B opcodes 2019-04-17 13:39 ` [PATCH dev-5.0 4/4] mtd: spi-nor: aspeed: add support for the 4B opcodes Cédric Le Goater @ 2019-04-18 6:10 ` Andrew Jeffery 0 siblings, 0 replies; 24+ messages in thread From: Andrew Jeffery @ 2019-04-18 6:10 UTC (permalink / raw) To: Cédric Le Goater, openbmc On Wed, 17 Apr 2019, at 23:10, Cédric Le Goater wrote: > Switch the default controller value to use the read mode in order to > customize the command and use SPINOR_OP_READ_4B (0x13) when the chip > supports 4B opcodes. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > drivers/mtd/spi-nor/aspeed-smc.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/spi-nor/aspeed-smc.c > b/drivers/mtd/spi-nor/aspeed-smc.c > index 7e289ecb1c99..1200b23416e3 100644 > --- a/drivers/mtd/spi-nor/aspeed-smc.c > +++ b/drivers/mtd/spi-nor/aspeed-smc.c > @@ -890,14 +890,21 @@ static u32 aspeed_smc_default_read(struct > aspeed_smc_chip *chip) > */ > u32 ctl_mask = chip->controller->info == &spi_2400_info ? > CONTROL_IO_ADDRESS_4B : 0; > + u8 cmd = chip->nor.flags & SNOR_F_4B_OPCODES ? SPINOR_OP_READ_4B : > + SPINOR_OP_READ; > > + /* > + * Use the "read command" mode to customize the opcode. In > + * normal command mode, the value is necessarily READ (0x3) on > + * the AST2400/2500 SoCs. > + */ > return (chip->ctl_val[smc_read] & ctl_mask) | > (0x00 << 28) | /* Single bit */ > (0x00 << 24) | /* CE# max */ > - (0x03 << 16) | /* use normal reads */ > + (cmd << 16) | /* use read mode to support 4B opcode */ > (0x00 << 8) | /* HCLK/16 */ > (0x00 << 6) | /* no dummy cycle */ > - (0x00); /* normal mode */ > + (0x01); /* read mode */ > } > > static int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip, > -- > 2.20.1 > > Applied to dev-5.0. Cheers, Andrew ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer 2019-04-17 13:39 [PATCH dev-5.0 0/4] spi-nor: aspeed: add support for the 4B opcodes Cédric Le Goater ` (3 preceding siblings ...) 2019-04-17 13:39 ` [PATCH dev-5.0 4/4] mtd: spi-nor: aspeed: add support for the 4B opcodes Cédric Le Goater @ 2019-04-18 21:23 ` Milton Miller II 2019-04-19 1:03 ` Andrew Jeffery 2019-04-18 22:27 ` [PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE address mode mask Milton Miller II 2019-04-18 22:41 ` [PATCH dev-5.0 0/4] spi-nor: aspeed: add support for the 4B opcodes Milton Miller II 6 siblings, 1 reply; 24+ messages in thread From: Milton Miller II @ 2019-04-18 21:23 UTC (permalink / raw) To: Cédric Le Goater; +Cc: openbmc, Andrew Jeffery About 04/17/2019 09:20AM in some timezone, Cédric Le Goater wrote: >aspeed_smc_read_from_ahb() only reads the first word which is not >what >we want. We want to capture a CALIBRATE_BUF_SIZE size window of the >flash contents to optimize the read. > NACK This justifcation is false. The routine reads the whole buffer because it calls the _rep routine and takes the size. In addition, the comment just before aspeed_smc_read_from_ahb tells why memcpy_fromio and memcpy_toio are broken on 32 bit arm, and this is still the case judging from the recent bug reportfrom a Nuvaton user [1]. [1] https://github.com/openbmc/openbmc/issues/3521 Andrew, Please revert this patch. >Signed-off-by: Cédric Le Goater <clg@kaod.org> >--- > drivers/mtd/spi-nor/aspeed-smc.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > >diff --git a/drivers/mtd/spi-nor/aspeed-smc.c >b/drivers/mtd/spi-nor/aspeed-smc.c >index 1437732fdea1..7e289ecb1c99 100644 >--- a/drivers/mtd/spi-nor/aspeed-smc.c >+++ b/drivers/mtd/spi-nor/aspeed-smc.c >@@ -796,8 +796,7 @@ static bool aspeed_smc_check_reads(struct >aspeed_smc_chip *chip, > int i; > > for (i = 0; i < 10; i++) { >- aspeed_smc_read_from_ahb(test_buf, chip->ahb_base, >- CALIBRATE_BUF_SIZE); >+ memcpy_fromio(test_buf, chip->ahb_base, CALIBRATE_BUF_SIZE); > if (memcmp(test_buf, golden_buf, CALIBRATE_BUF_SIZE) != 0) > return false; > } >@@ -921,8 +920,7 @@ static int aspeed_smc_optimize_read(struct >aspeed_smc_chip *chip, > > writel(chip->ctl_val[smc_read], chip->ctl); > >- aspeed_smc_read_from_ahb(golden_buf, chip->ahb_base, >- CALIBRATE_BUF_SIZE); >+ memcpy_fromio(golden_buf, chip->ahb_base, CALIBRATE_BUF_SIZE); > > /* Establish our read mode with freq field set to 0 (HCLK/16) */ > chip->ctl_val[smc_read] = save_read_val & 0xfffff0ff; >-- >2.20.1 > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer 2019-04-18 21:23 ` [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer Milton Miller II @ 2019-04-19 1:03 ` Andrew Jeffery 2019-04-19 6:02 ` Cédric Le Goater 0 siblings, 1 reply; 24+ messages in thread From: Andrew Jeffery @ 2019-04-19 1:03 UTC (permalink / raw) To: Milton Miller II, Cédric Le Goater; +Cc: openbmc On Fri, 19 Apr 2019, at 06:53, Milton Miller II wrote: > About 04/17/2019 09:20AM in some timezone, Cédric Le Goater wrote: > > > >aspeed_smc_read_from_ahb() only reads the first word which is not > >what > >we want. We want to capture a CALIBRATE_BUF_SIZE size window of the > >flash contents to optimize the read. > > > > NACK > > This justifcation is false. The routine reads the whole buffer > because it calls the _rep routine and takes the size. > > In addition, the comment just before aspeed_smc_read_from_ahb > tells why memcpy_fromio and memcpy_toio are broken on 32 bit > arm, and this is still the case judging from the recent bug > reportfrom a Nuvaton user [1]. > > [1] https://github.com/openbmc/openbmc/issues/3521 > > Andrew, Please revert this patch. Yeah, I had a briefly nagging thought about this when I applied it. I should have gone with that gut instinct and inspected it in more depth. Thanks for calling it out. Reverting for dev-5.0, this is an isolated issue (arguably should have been separated from the current series). Andrew > > >Signed-off-by: Cédric Le Goater <clg@kaod.org> > >--- > > drivers/mtd/spi-nor/aspeed-smc.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > >diff --git a/drivers/mtd/spi-nor/aspeed-smc.c > >b/drivers/mtd/spi-nor/aspeed-smc.c > >index 1437732fdea1..7e289ecb1c99 100644 > >--- a/drivers/mtd/spi-nor/aspeed-smc.c > >+++ b/drivers/mtd/spi-nor/aspeed-smc.c > >@@ -796,8 +796,7 @@ static bool aspeed_smc_check_reads(struct > >aspeed_smc_chip *chip, > > int i; > > > > for (i = 0; i < 10; i++) { > >- aspeed_smc_read_from_ahb(test_buf, chip->ahb_base, > >- CALIBRATE_BUF_SIZE); > >+ memcpy_fromio(test_buf, chip->ahb_base, CALIBRATE_BUF_SIZE); > > if (memcmp(test_buf, golden_buf, CALIBRATE_BUF_SIZE) != 0) > > return false; > > } > >@@ -921,8 +920,7 @@ static int aspeed_smc_optimize_read(struct > >aspeed_smc_chip *chip, > > > > writel(chip->ctl_val[smc_read], chip->ctl); > > > >- aspeed_smc_read_from_ahb(golden_buf, chip->ahb_base, > >- CALIBRATE_BUF_SIZE); > >+ memcpy_fromio(golden_buf, chip->ahb_base, CALIBRATE_BUF_SIZE); > > > > /* Establish our read mode with freq field set to 0 (HCLK/16) */ > > chip->ctl_val[smc_read] = save_read_val & 0xfffff0ff; > >-- > >2.20.1 > > > > > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer 2019-04-19 1:03 ` Andrew Jeffery @ 2019-04-19 6:02 ` Cédric Le Goater 2019-04-19 7:23 ` Andrew Jeffery 0 siblings, 1 reply; 24+ messages in thread From: Cédric Le Goater @ 2019-04-19 6:02 UTC (permalink / raw) To: Andrew Jeffery, Milton Miller II; +Cc: openbmc On 4/19/19 3:03 AM, Andrew Jeffery wrote: > > > On Fri, 19 Apr 2019, at 06:53, Milton Miller II wrote: >> About 04/17/2019 09:20AM in some timezone, Cédric Le Goater wrote: >> >> >>> aspeed_smc_read_from_ahb() only reads the first word which is not >>> what >>> we want. We want to capture a CALIBRATE_BUF_SIZE size window of the >>> flash contents to optimize the read. >>> >> >> NACK >> >> This justifcation is false. The routine reads the whole buffer >> because it calls the _rep routine and takes the size. It doesn't AFAICT looking at other drivers and also from my test. >> In addition, the comment just before aspeed_smc_read_from_ahb >> tells why memcpy_fromio and memcpy_toio are broken on 32 bit That might have been the case on Linux 4.7. It doesn't seem to be the case anymore. See below. >> arm, and this is still the case judging from the recent bug >> reportfrom a Nuvaton user [1]. >> >> [1] https://github.com/openbmc/openbmc/issues/3521 >> >> Andrew, Please revert this patch. I don't think you have enough convincing arguments for that. > Yeah, I had a briefly nagging thought about this when I applied it. I > should have gone with that gut instinct and inspected it in more > depth. > > Thanks for calling it out. I think this needs more thinking and tests. Witherspoon and palmetto are fine. What's the problem. > Reverting for dev-5.0, this is an isolated issue (arguably should have > been separated from the current series). We have been using memcpy_fromio for reads for a very long time since 4.13 in the Aspeed SMC driver on *all* platforms*. See aspeed_smc_read() for the "Command mode". C. > > Andrew > >> >>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> --- >>> drivers/mtd/spi-nor/aspeed-smc.c | 6 ++---- >>> 1 file changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c >>> b/drivers/mtd/spi-nor/aspeed-smc.c >>> index 1437732fdea1..7e289ecb1c99 100644 >>> --- a/drivers/mtd/spi-nor/aspeed-smc.c >>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c >>> @@ -796,8 +796,7 @@ static bool aspeed_smc_check_reads(struct >>> aspeed_smc_chip *chip, >>> int i; >>> >>> for (i = 0; i < 10; i++) { >>> - aspeed_smc_read_from_ahb(test_buf, chip->ahb_base, >>> - CALIBRATE_BUF_SIZE); >>> + memcpy_fromio(test_buf, chip->ahb_base, CALIBRATE_BUF_SIZE); >>> if (memcmp(test_buf, golden_buf, CALIBRATE_BUF_SIZE) != 0) >>> return false; >>> } >>> @@ -921,8 +920,7 @@ static int aspeed_smc_optimize_read(struct >>> aspeed_smc_chip *chip, >>> >>> writel(chip->ctl_val[smc_read], chip->ctl); >>> >>> - aspeed_smc_read_from_ahb(golden_buf, chip->ahb_base, >>> - CALIBRATE_BUF_SIZE); >>> + memcpy_fromio(golden_buf, chip->ahb_base, CALIBRATE_BUF_SIZE); >>> >>> /* Establish our read mode with freq field set to 0 (HCLK/16) */ >>> chip->ctl_val[smc_read] = save_read_val & 0xfffff0ff; >>> -- >>> 2.20.1 >>> >>> >> >> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer 2019-04-19 6:02 ` Cédric Le Goater @ 2019-04-19 7:23 ` Andrew Jeffery 2019-04-19 8:09 ` Cédric Le Goater 0 siblings, 1 reply; 24+ messages in thread From: Andrew Jeffery @ 2019-04-19 7:23 UTC (permalink / raw) To: Cédric Le Goater, Milton Miller II; +Cc: openbmc Hi Cédric On Fri, 19 Apr 2019, at 15:32, Cédric Le Goater wrote: > On 4/19/19 3:03 AM, Andrew Jeffery wrote: > > > > > > On Fri, 19 Apr 2019, at 06:53, Milton Miller II wrote: > >> About 04/17/2019 09:20AM in some timezone, Cédric Le Goater wrote: > >> > >> > >>> aspeed_smc_read_from_ahb() only reads the first word which is not > >>> what > >>> we want. We want to capture a CALIBRATE_BUF_SIZE size window of the > >>> flash contents to optimize the read. > >>> > >> > >> NACK > >> > >> This justifcation is false. The routine reads the whole buffer > >> because it calls the _rep routine and takes the size. > > It doesn't AFAICT looking at other drivers and also from my test. > > >> In addition, the comment just before aspeed_smc_read_from_ahb > >> tells why memcpy_fromio and memcpy_toio are broken on 32 bit > > That might have been the case on Linux 4.7. It doesn't seem to be > the case anymore. See below. > > >> arm, and this is still the case judging from the recent bug > >> reportfrom a Nuvaton user [1]. > >> > >> [1] https://github.com/openbmc/openbmc/issues/3521 > >> > >> Andrew, Please revert this patch. > > I don't think you have enough convincing arguments for that. That may be the case, but having seen the pain of the original corruption problems that drove the ioreadX_rep() implementation above, Milton's protest combined with my initial, briefly nagging concern was enough for me to revert. Two things here: 1. We've run without this patch for quite some time. Despite oddities, the existing implementation has been stable 2. With patch 4/4, you've resolved the 4B corruption problem. This was the immediate concern, as it was impacting teams developing and testing OpenBMC master. I appreciate the effort you put into hunting that down, the root cause was certainly not obvious. From *my* testing we appear to be stable both with and without this change, however my concern is that *my* testing is not complete enough to be confident that we're not going to hit the subtle corruption problems that drove the introduction of the existing code. For some additional context, I'm now on leave until the 30th, and Joel to the 29th. These patches have been through a process that has proceeded much more hastily than I would have liked, and that's lead to where we are now. With that in mind, less change is better, and so I have decided to back this patch out. It's a risk-based decision, not a reflection of the effort, desires or technicalities involved. So there are lessons in this for me too as I've felt squeezed by the impact the 4B corruption bug has had on teams testing master, and my impending leave. With that reasoning and 20/20 hindsight I should probably have held off to begin with. Next time. Some reflection is in order. Anyway, I hope that helps clarify. Time to resolve the disagreement here. Milton: Given your protest, can you please verify whether there continue to be issues with memcpy_fromio() that prevent us from making use of it. As Cédric points out we do make heavy use of it anyway, and using a well-worn implementation rather than something bespoke for this particular case would improve maintainability. Cheers, Andrew > > > Yeah, I had a briefly nagging thought about this when I applied it. I > > should have gone with that gut instinct and inspected it in more > > depth. > > > > Thanks for calling it out. > > I think this needs more thinking and tests. Witherspoon and palmetto > are fine. What's the problem. > > > Reverting for dev-5.0, this is an isolated issue (arguably should have > > been separated from the current series). > > We have been using memcpy_fromio for reads for a very long time > since 4.13 in the Aspeed SMC driver on *all* platforms*. > > See aspeed_smc_read() for the "Command mode". > > C. > > > > > > > > Andrew > > > >> > >>> Signed-off-by: Cédric Le Goater <clg@kaod.org> > >>> --- > >>> drivers/mtd/spi-nor/aspeed-smc.c | 6 ++---- > >>> 1 file changed, 2 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c > >>> b/drivers/mtd/spi-nor/aspeed-smc.c > >>> index 1437732fdea1..7e289ecb1c99 100644 > >>> --- a/drivers/mtd/spi-nor/aspeed-smc.c > >>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c > >>> @@ -796,8 +796,7 @@ static bool aspeed_smc_check_reads(struct > >>> aspeed_smc_chip *chip, > >>> int i; > >>> > >>> for (i = 0; i < 10; i++) { > >>> - aspeed_smc_read_from_ahb(test_buf, chip->ahb_base, > >>> - CALIBRATE_BUF_SIZE); > >>> + memcpy_fromio(test_buf, chip->ahb_base, CALIBRATE_BUF_SIZE); > >>> if (memcmp(test_buf, golden_buf, CALIBRATE_BUF_SIZE) != 0) > >>> return false; > >>> } > >>> @@ -921,8 +920,7 @@ static int aspeed_smc_optimize_read(struct > >>> aspeed_smc_chip *chip, > >>> > >>> writel(chip->ctl_val[smc_read], chip->ctl); > >>> > >>> - aspeed_smc_read_from_ahb(golden_buf, chip->ahb_base, > >>> - CALIBRATE_BUF_SIZE); > >>> + memcpy_fromio(golden_buf, chip->ahb_base, CALIBRATE_BUF_SIZE); > >>> > >>> /* Establish our read mode with freq field set to 0 (HCLK/16) */ > >>> chip->ctl_val[smc_read] = save_read_val & 0xfffff0ff; > >>> -- > >>> 2.20.1 > >>> > >>> > >> > >> > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer 2019-04-19 7:23 ` Andrew Jeffery @ 2019-04-19 8:09 ` Cédric Le Goater 2019-04-19 8:22 ` Andrew Jeffery ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Cédric Le Goater @ 2019-04-19 8:09 UTC (permalink / raw) To: Andrew Jeffery, Milton Miller II; +Cc: openbmc On 4/19/19 9:23 AM, Andrew Jeffery wrote: > Hi Cédric > > On Fri, 19 Apr 2019, at 15:32, Cédric Le Goater wrote: >> On 4/19/19 3:03 AM, Andrew Jeffery wrote: >>> >>> >>> On Fri, 19 Apr 2019, at 06:53, Milton Miller II wrote: >>>> About 04/17/2019 09:20AM in some timezone, Cédric Le Goater wrote: >>>> >>>> >>>>> aspeed_smc_read_from_ahb() only reads the first word which is not >>>>> what >>>>> we want. We want to capture a CALIBRATE_BUF_SIZE size window of the >>>>> flash contents to optimize the read. >>>>> >>>> >>>> NACK >>>> >>>> This justifcation is false. The routine reads the whole buffer >>>> because it calls the _rep routine and takes the size. >> >> It doesn't AFAICT looking at other drivers and also from my test. >> >>>> In addition, the comment just before aspeed_smc_read_from_ahb >>>> tells why memcpy_fromio and memcpy_toio are broken on 32 bit >> >> That might have been the case on Linux 4.7. It doesn't seem to be >> the case anymore. See below. >> >>>> arm, and this is still the case judging from the recent bug >>>> reportfrom a Nuvaton user [1]. >>>> >>>> [1] https://github.com/openbmc/openbmc/issues/3521 >>>> >>>> Andrew, Please revert this patch. >> >> I don't think you have enough convincing arguments for that. > > That may be the case, but having seen the pain of the original corruption > problems that drove the ioreadX_rep() implementation above, Milton's > protest combined with my initial, briefly nagging concern was enough for > me to revert. Two things here: > > 1. We've run without this patch for quite some time. Despite oddities, > the existing implementation has been stable > 2. With patch 4/4, you've resolved the 4B corruption problem. This was > the immediate concern, as it was impacting teams developing and > testing OpenBMC master. I appreciate the effort you put into hunting > that down, the root cause was certainly not obvious. > > From *my* testing we appear to be stable both with and without this > change, however my concern is that *my* testing is not complete enough > to be confident that we're not going to hit the subtle corruption problems > that drove the introduction of the existing code. QEMU would have caught this regression if SFDP was modeled. It does today if 4B opcodes are forced on the mx25l25635e. Given the time the team spent on this, I would say 1 or 2PM overall, QEMU is a good investment. ^ | Managers are you reading this ? ------------------+ > For some additional context, I'm now on leave until the 30th, and Joel to > the 29th. These patches have been through a process that has proceeded > much more hastily than I would have liked, and that's lead to where we > are now. > > With that in mind, less change is better, and so I have decided to back > this patch out. It's a risk-based decision, not a reflection of the effort, > desires or technicalities involved. Back to where we were before, it's fine as it works. The optimize reads are just reading the first 4 bytes : [ 14.130480] aspeed-smc 1e630000.spi: mx25l25635e (32768 Kbytes) [ 14.136664] aspeed-smc 1e630000.spi: write control register: 00122302 [ 14.143326] aspeed-smc 1e630000.spi: read control register: 203c2341 [ 14.149750] aspeed-smc 1e630000.spi: AHB frequency: 192 MHz [ 14.181561] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART [ 14.188894] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART [ 14.196230] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART [ 14.203558] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART [ 14.210751] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART [ 14.218067] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART [ 14.225397] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART [ 14.232722] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART [ 14.239912] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART [ 14.247232] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART with memcpy_fromio : [ 13.779087] aspeed-smc 1e630000.spi: mx25l25635e (32768 Kbytes) [ 13.785255] aspeed-smc 1e630000.spi: write control register: 00122302 [ 13.791762] aspeed-smc 1e630000.spi: read control register: 203c2341 [ 13.798326] aspeed-smc 1e630000.spi: AHB frequency: 192 MHz [ 13.815420] 50 41 52 54 00 00 00 01 00 00 00 01 00 00 00 80 PART............ [ 13.822759] 00 00 00 1b 00 00 10 00 00 00 20 00 00 00 00 00 .......... ..... [ 13.829946] 00 00 00 00 00 00 00 00 00 00 00 00 50 41 62 cf ............PAb. [ 13.837266] 70 61 72 74 00 00 00 00 00 00 00 00 00 00 00 00 part............ [ 13.844597] 00 00 00 00 00 00 00 01 ff ff ff ff 00 00 00 01 ................ [ 13.851788] 00 00 00 03 00 00 00 01 00 00 10 00 00 00 00 00 ................ [ 13.859105] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [ 13.866433] 00 40 00 00 00 00 00 00 00 00 00 00 00 00 00 00 .@.............. [ 13.873759] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [ 13.880951] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ [ 13.888267] 00 00 00 00 00 00 00 00 00 00 00 00 8f de 9d 89 ................ [ 13.895591] 48 42 49 00 00 00 00 00 00 00 00 00 00 00 00 00 HBI............. [ 13.902917] 00 00 00 10 00 00 05 a0 ff ff ff ff 00 00 00 02 ................ I should have added these tests in the commit log. Sorry about that. We will see later on. There are no hurries for this fix. Optimization is still being done. Thanks, C. > So there are lessons in this for me too as I've felt squeezed by the > impact the 4B corruption bug has had on teams testing master, and > my impending leave. With that reasoning and 20/20 hindsight I should > probably have held off to begin with. Next time. Some reflection is in > order. > > Anyway, I hope that helps clarify. Time to resolve the disagreement here. > > Milton: Given your protest, can you please verify whether there continue > to be issues with memcpy_fromio() that prevent us from making use of it. > As Cédric points out we do make heavy use of it anyway, and using a > well-worn implementation rather than something bespoke for this particular > case would improve maintainability. > > Cheers, > > Andrew > >> >>> Yeah, I had a briefly nagging thought about this when I applied it. I >>> should have gone with that gut instinct and inspected it in more >>> depth. >>> >>> Thanks for calling it out. >> >> I think this needs more thinking and tests. Witherspoon and palmetto >> are fine. What's the problem. >> >>> Reverting for dev-5.0, this is an isolated issue (arguably should have >>> been separated from the current series). >> >> We have been using memcpy_fromio for reads for a very long time >> since 4.13 in the Aspeed SMC driver on *all* platforms*. >> >> See aspeed_smc_read() for the "Command mode". >> >> C. >> >> >> >> >>> >>> Andrew >>> >>>> >>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>>> --- >>>>> drivers/mtd/spi-nor/aspeed-smc.c | 6 ++---- >>>>> 1 file changed, 2 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c >>>>> b/drivers/mtd/spi-nor/aspeed-smc.c >>>>> index 1437732fdea1..7e289ecb1c99 100644 >>>>> --- a/drivers/mtd/spi-nor/aspeed-smc.c >>>>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c >>>>> @@ -796,8 +796,7 @@ static bool aspeed_smc_check_reads(struct >>>>> aspeed_smc_chip *chip, >>>>> int i; >>>>> >>>>> for (i = 0; i < 10; i++) { >>>>> - aspeed_smc_read_from_ahb(test_buf, chip->ahb_base, >>>>> - CALIBRATE_BUF_SIZE); >>>>> + memcpy_fromio(test_buf, chip->ahb_base, CALIBRATE_BUF_SIZE); >>>>> if (memcmp(test_buf, golden_buf, CALIBRATE_BUF_SIZE) != 0) >>>>> return false; >>>>> } >>>>> @@ -921,8 +920,7 @@ static int aspeed_smc_optimize_read(struct >>>>> aspeed_smc_chip *chip, >>>>> >>>>> writel(chip->ctl_val[smc_read], chip->ctl); >>>>> >>>>> - aspeed_smc_read_from_ahb(golden_buf, chip->ahb_base, >>>>> - CALIBRATE_BUF_SIZE); >>>>> + memcpy_fromio(golden_buf, chip->ahb_base, CALIBRATE_BUF_SIZE); >>>>> >>>>> /* Establish our read mode with freq field set to 0 (HCLK/16) */ >>>>> chip->ctl_val[smc_read] = save_read_val & 0xfffff0ff; >>>>> -- >>>>> 2.20.1 >>>>> >>>>> >>>> >>>> >> >> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer 2019-04-19 8:09 ` Cédric Le Goater @ 2019-04-19 8:22 ` Andrew Jeffery 2019-05-02 3:53 ` Andrew Jeffery 2019-05-03 22:19 ` Milton Miller II 2 siblings, 0 replies; 24+ messages in thread From: Andrew Jeffery @ 2019-04-19 8:22 UTC (permalink / raw) To: Cédric Le Goater, Milton Miller II; +Cc: openbmc On Fri, 19 Apr 2019, at 17:39, Cédric Le Goater wrote: > On 4/19/19 9:23 AM, Andrew Jeffery wrote: > > Hi Cédric > > > > On Fri, 19 Apr 2019, at 15:32, Cédric Le Goater wrote: > >> On 4/19/19 3:03 AM, Andrew Jeffery wrote: > >>> > >>> > >>> On Fri, 19 Apr 2019, at 06:53, Milton Miller II wrote: > >>>> About 04/17/2019 09:20AM in some timezone, Cédric Le Goater wrote: > >>>> > >>>> > >>>>> aspeed_smc_read_from_ahb() only reads the first word which is not > >>>>> what > >>>>> we want. We want to capture a CALIBRATE_BUF_SIZE size window of the > >>>>> flash contents to optimize the read. > >>>>> > >>>> > >>>> NACK > >>>> > >>>> This justifcation is false. The routine reads the whole buffer > >>>> because it calls the _rep routine and takes the size. > >> > >> It doesn't AFAICT looking at other drivers and also from my test. > >> > >>>> In addition, the comment just before aspeed_smc_read_from_ahb > >>>> tells why memcpy_fromio and memcpy_toio are broken on 32 bit > >> > >> That might have been the case on Linux 4.7. It doesn't seem to be > >> the case anymore. See below. > >> > >>>> arm, and this is still the case judging from the recent bug > >>>> reportfrom a Nuvaton user [1]. > >>>> > >>>> [1] https://github.com/openbmc/openbmc/issues/3521 > >>>> > >>>> Andrew, Please revert this patch. > >> > >> I don't think you have enough convincing arguments for that. > > > > That may be the case, but having seen the pain of the original corruption > > problems that drove the ioreadX_rep() implementation above, Milton's > > protest combined with my initial, briefly nagging concern was enough for > > me to revert. Two things here: > > > > 1. We've run without this patch for quite some time. Despite oddities, > > the existing implementation has been stable > > 2. With patch 4/4, you've resolved the 4B corruption problem. This was > > the immediate concern, as it was impacting teams developing and > > testing OpenBMC master. I appreciate the effort you put into hunting > > that down, the root cause was certainly not obvious. > > > > From *my* testing we appear to be stable both with and without this > > change, however my concern is that *my* testing is not complete enough > > to be confident that we're not going to hit the subtle corruption problems > > that drove the introduction of the existing code. > > QEMU would have caught this regression if SFDP was modeled. It does today > if 4B opcodes are forced on the mx25l25635e. Given the time the team spent > on this, I would say 1 or 2PM overall, QEMU is a good investment. > ^ > | > Managers are you reading this ? ------------------+ > > > For some additional context, I'm now on leave until the 30th, and Joel to > > the 29th. These patches have been through a process that has proceeded > > much more hastily than I would have liked, and that's lead to where we > > are now. > > > > With that in mind, less change is better, and so I have decided to back > > this patch out. It's a risk-based decision, not a reflection of the effort, > > desires or technicalities involved. > > Back to where we were before, it's fine as it works. > > The optimize reads are just reading the first 4 bytes : > > [ 14.130480] aspeed-smc 1e630000.spi: mx25l25635e (32768 Kbytes) > [ 14.136664] aspeed-smc 1e630000.spi: write control register: 00122302 > [ 14.143326] aspeed-smc 1e630000.spi: read control register: 203c2341 > [ 14.149750] aspeed-smc 1e630000.spi: AHB frequency: 192 MHz > [ 14.181561] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > [ 14.188894] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > [ 14.196230] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > [ 14.203558] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > [ 14.210751] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > [ 14.218067] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > [ 14.225397] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > [ 14.232722] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > [ 14.239912] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > [ 14.247232] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > > with memcpy_fromio : > > [ 13.779087] aspeed-smc 1e630000.spi: mx25l25635e (32768 Kbytes) > [ 13.785255] aspeed-smc 1e630000.spi: write control register: 00122302 > [ 13.791762] aspeed-smc 1e630000.spi: read control register: 203c2341 > [ 13.798326] aspeed-smc 1e630000.spi: AHB frequency: 192 MHz > [ 13.815420] 50 41 52 54 00 00 00 01 00 00 00 01 00 00 00 80 PART............ > [ 13.822759] 00 00 00 1b 00 00 10 00 00 00 20 00 00 00 00 00 .......... ..... > [ 13.829946] 00 00 00 00 00 00 00 00 00 00 00 00 50 41 62 cf ............PAb. > [ 13.837266] 70 61 72 74 00 00 00 00 00 00 00 00 00 00 00 00 part............ > [ 13.844597] 00 00 00 00 00 00 00 01 ff ff ff ff 00 00 00 01 ................ > [ 13.851788] 00 00 00 03 00 00 00 01 00 00 10 00 00 00 00 00 ................ > [ 13.859105] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > [ 13.866433] 00 40 00 00 00 00 00 00 00 00 00 00 00 00 00 00 .@.............. > [ 13.873759] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > [ 13.880951] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > [ 13.888267] 00 00 00 00 00 00 00 00 00 00 00 00 8f de 9d 89 ................ > [ 13.895591] 48 42 49 00 00 00 00 00 00 00 00 00 00 00 00 00 HBI............. > [ 13.902917] 00 00 00 10 00 00 05 a0 ff ff ff ff 00 00 00 02 ................ > > > I should have added these tests in the commit log. Sorry about that. Not to worry! Thanks for the data. It paints a good picture of why the change is desirable :) > We will see later on. There are no hurries for this fix. Optimization > is still being done. Yes. A silly situation all told. Thanks for your effort again. Andrew > > Thanks, > > C. > > > > So there are lessons in this for me too as I've felt squeezed by the > > impact the 4B corruption bug has had on teams testing master, and > > my impending leave. With that reasoning and 20/20 hindsight I should > > probably have held off to begin with. Next time. Some reflection is in > > order. > > > > Anyway, I hope that helps clarify. Time to resolve the disagreement here. > > > > Milton: Given your protest, can you please verify whether there continue > > to be issues with memcpy_fromio() that prevent us from making use of it. > > As Cédric points out we do make heavy use of it anyway, and using a > > well-worn implementation rather than something bespoke for this particular > > case would improve maintainability. > > > > Cheers, > > > > Andrew > > > >> > >>> Yeah, I had a briefly nagging thought about this when I applied it. I > >>> should have gone with that gut instinct and inspected it in more > >>> depth. > >>> > >>> Thanks for calling it out. > >> > >> I think this needs more thinking and tests. Witherspoon and palmetto > >> are fine. What's the problem. > >> > >>> Reverting for dev-5.0, this is an isolated issue (arguably should have > >>> been separated from the current series). > >> > >> We have been using memcpy_fromio for reads for a very long time > >> since 4.13 in the Aspeed SMC driver on *all* platforms*. > >> > >> See aspeed_smc_read() for the "Command mode". > >> > >> C. > >> > >> > >> > >> > >>> > >>> Andrew > >>> > >>>> > >>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> > >>>>> --- > >>>>> drivers/mtd/spi-nor/aspeed-smc.c | 6 ++---- > >>>>> 1 file changed, 2 insertions(+), 4 deletions(-) > >>>>> > >>>>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c > >>>>> b/drivers/mtd/spi-nor/aspeed-smc.c > >>>>> index 1437732fdea1..7e289ecb1c99 100644 > >>>>> --- a/drivers/mtd/spi-nor/aspeed-smc.c > >>>>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c > >>>>> @@ -796,8 +796,7 @@ static bool aspeed_smc_check_reads(struct > >>>>> aspeed_smc_chip *chip, > >>>>> int i; > >>>>> > >>>>> for (i = 0; i < 10; i++) { > >>>>> - aspeed_smc_read_from_ahb(test_buf, chip->ahb_base, > >>>>> - CALIBRATE_BUF_SIZE); > >>>>> + memcpy_fromio(test_buf, chip->ahb_base, CALIBRATE_BUF_SIZE); > >>>>> if (memcmp(test_buf, golden_buf, CALIBRATE_BUF_SIZE) != 0) > >>>>> return false; > >>>>> } > >>>>> @@ -921,8 +920,7 @@ static int aspeed_smc_optimize_read(struct > >>>>> aspeed_smc_chip *chip, > >>>>> > >>>>> writel(chip->ctl_val[smc_read], chip->ctl); > >>>>> > >>>>> - aspeed_smc_read_from_ahb(golden_buf, chip->ahb_base, > >>>>> - CALIBRATE_BUF_SIZE); > >>>>> + memcpy_fromio(golden_buf, chip->ahb_base, CALIBRATE_BUF_SIZE); > >>>>> > >>>>> /* Establish our read mode with freq field set to 0 (HCLK/16) */ > >>>>> chip->ctl_val[smc_read] = save_read_val & 0xfffff0ff; > >>>>> -- > >>>>> 2.20.1 > >>>>> > >>>>> > >>>> > >>>> > >> > >> > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer 2019-04-19 8:09 ` Cédric Le Goater 2019-04-19 8:22 ` Andrew Jeffery @ 2019-05-02 3:53 ` Andrew Jeffery 2019-05-03 22:19 ` Milton Miller II 2 siblings, 0 replies; 24+ messages in thread From: Andrew Jeffery @ 2019-05-02 3:53 UTC (permalink / raw) To: Cédric Le Goater, Milton Miller II; +Cc: openbmc On Fri, 19 Apr 2019, at 17:39, Cédric Le Goater wrote: > On 4/19/19 9:23 AM, Andrew Jeffery wrote: > > Hi Cédric > > > > On Fri, 19 Apr 2019, at 15:32, Cédric Le Goater wrote: > >> On 4/19/19 3:03 AM, Andrew Jeffery wrote: > >>> > >>> > >>> On Fri, 19 Apr 2019, at 06:53, Milton Miller II wrote: > >>>> About 04/17/2019 09:20AM in some timezone, Cédric Le Goater wrote: > >>>> > >>>> > >>>>> aspeed_smc_read_from_ahb() only reads the first word which is not > >>>>> what > >>>>> we want. We want to capture a CALIBRATE_BUF_SIZE size window of the > >>>>> flash contents to optimize the read. > >>>>> > >>>> > >>>> NACK > >>>> > >>>> This justifcation is false. The routine reads the whole buffer > >>>> because it calls the _rep routine and takes the size. > >> > >> It doesn't AFAICT looking at other drivers and also from my test. > >> > >>>> In addition, the comment just before aspeed_smc_read_from_ahb > >>>> tells why memcpy_fromio and memcpy_toio are broken on 32 bit > >> > >> That might have been the case on Linux 4.7. It doesn't seem to be > >> the case anymore. See below. > >> > >>>> arm, and this is still the case judging from the recent bug > >>>> reportfrom a Nuvaton user [1]. > >>>> > >>>> [1] https://github.com/openbmc/openbmc/issues/3521 > >>>> > >>>> Andrew, Please revert this patch. > >> > >> I don't think you have enough convincing arguments for that. > > > > That may be the case, but having seen the pain of the original corruption > > problems that drove the ioreadX_rep() implementation above, Milton's > > protest combined with my initial, briefly nagging concern was enough for > > me to revert. Two things here: > > > > 1. We've run without this patch for quite some time. Despite oddities, > > the existing implementation has been stable > > 2. With patch 4/4, you've resolved the 4B corruption problem. This was > > the immediate concern, as it was impacting teams developing and > > testing OpenBMC master. I appreciate the effort you put into hunting > > that down, the root cause was certainly not obvious. > > > > From *my* testing we appear to be stable both with and without this > > change, however my concern is that *my* testing is not complete enough > > to be confident that we're not going to hit the subtle corruption problems > > that drove the introduction of the existing code. > > QEMU would have caught this regression if SFDP was modeled. It does today > if 4B opcodes are forced on the mx25l25635e. Given the time the team spent > on this, I would say 1 or 2PM overall, QEMU is a good investment. > ^ > | > Managers are you reading this ? ------------------+ > > > For some additional context, I'm now on leave until the 30th, and Joel to > > the 29th. These patches have been through a process that has proceeded > > much more hastily than I would have liked, and that's lead to where we > > are now. > > > > With that in mind, less change is better, and so I have decided to back > > this patch out. It's a risk-based decision, not a reflection of the effort, > > desires or technicalities involved. > > Back to where we were before, it's fine as it works. > > The optimize reads are just reading the first 4 bytes : > > [ 14.130480] aspeed-smc 1e630000.spi: mx25l25635e (32768 Kbytes) > [ 14.136664] aspeed-smc 1e630000.spi: write control register: 00122302 > [ 14.143326] aspeed-smc 1e630000.spi: read control register: 203c2341 > [ 14.149750] aspeed-smc 1e630000.spi: AHB frequency: 192 MHz > [ 14.181561] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > [ 14.188894] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > [ 14.196230] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > [ 14.203558] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > [ 14.210751] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > [ 14.218067] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > [ 14.225397] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > [ 14.232722] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > [ 14.239912] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > [ 14.247232] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 PARTPARTPARTPART > > with memcpy_fromio : > > [ 13.779087] aspeed-smc 1e630000.spi: mx25l25635e (32768 Kbytes) > [ 13.785255] aspeed-smc 1e630000.spi: write control register: 00122302 > [ 13.791762] aspeed-smc 1e630000.spi: read control register: 203c2341 > [ 13.798326] aspeed-smc 1e630000.spi: AHB frequency: 192 MHz > [ 13.815420] 50 41 52 54 00 00 00 01 00 00 00 01 00 00 00 80 PART............ > [ 13.822759] 00 00 00 1b 00 00 10 00 00 00 20 00 00 00 00 00 .......... ..... > [ 13.829946] 00 00 00 00 00 00 00 00 00 00 00 00 50 41 62 cf ............PAb. > [ 13.837266] 70 61 72 74 00 00 00 00 00 00 00 00 00 00 00 00 part............ > [ 13.844597] 00 00 00 00 00 00 00 01 ff ff ff ff 00 00 00 01 ................ > [ 13.851788] 00 00 00 03 00 00 00 01 00 00 10 00 00 00 00 00 ................ > [ 13.859105] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > [ 13.866433] 00 40 00 00 00 00 00 00 00 00 00 00 00 00 00 00 .@.............. > [ 13.873759] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > [ 13.880951] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > [ 13.888267] 00 00 00 00 00 00 00 00 00 00 00 00 8f de 9d 89 ................ > [ 13.895591] 48 42 49 00 00 00 00 00 00 00 00 00 00 00 00 00 HBI............. > [ 13.902917] 00 00 00 10 00 00 05 a0 ff ff ff ff 00 00 00 02 ................ > > > I should have added these tests in the commit log. Sorry about that. > We will see later on. There are no hurries for this fix. Optimization > is still being done. Milton: Given you NACK'ed the patch I'd appreciate a follow-up in light of this data. Cheers, Andrew ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer 2019-04-19 8:09 ` Cédric Le Goater 2019-04-19 8:22 ` Andrew Jeffery 2019-05-02 3:53 ` Andrew Jeffery @ 2019-05-03 22:19 ` Milton Miller II 2 siblings, 0 replies; 24+ messages in thread From: Milton Miller II @ 2019-05-03 22:19 UTC (permalink / raw) To: Andrew Jeffery; +Cc: Cédric Le Goater, openbmc On 05/01/2019 10:53PM in some timezone, Andrew Jeffery <andrew@aj.id.au> wrote: >On Fri, 19 Apr 2019, at 17:39, Cédric Le Goater wrote:>> On 4/19/19 9:23 AM, Andrew Jeffery wrote: >> > Hi Cédric >> > >> > On Fri, 19 Apr 2019, at 15:32, Cédric Le Goater wrote: >> >> On 4/19/19 3:03 AM, Andrew Jeffery wrote: >> >>> >> >>> >> >>> On Fri, 19 Apr 2019, at 06:53, Milton Miller II wrote: >> >>>> About 04/17/2019 09:20AM in some timezone, Cédric Le Goater >wrote: >> >>>> >> >>>> >> >>>>> aspeed_smc_read_from_ahb() only reads the first word which is >not >> >>>>> what >> >>>>> we want. We want to capture a CALIBRATE_BUF_SIZE size window >of the >> >>>>> flash contents to optimize the read. >> >>>>> >> >>>> >> >>>> NACK >> >>>> >> >>>> This justifcation is false. The routine reads the whole >buffer >> >>>> because it calls the _rep routine and takes the size. >> >> >> >> It doesn't AFAICT looking at other drivers and also from my >test. >> >> >> >>>> In addition, the comment just before aspeed_smc_read_from_ahb >> >>>> tells why memcpy_fromio and memcpy_toio are broken on 32 bit >> >> >> >> That might have been the case on Linux 4.7. It doesn't seem to >be >> >> the case anymore. See below. >> >> >> >>>> arm, and this is still the case judging from the recent bug >> >>>> reportfrom a Nuvaton user [1]. >> >>>> >> >>>> [1] >https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openb >mc_openbmc_issues_3521&d=DwIFaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=bvv7AJEECo >RKBU02rcu4F5DWd-EwX8As2xrXeO9ZSo4&m=sK1b4XTLYG8JeD8M-9ido3CQX_AOERqbR >DK4EyTZHWc&s=fYtUMc0yvOgIU3iNg2S3anMU3YSmstjFPxQR3JpCtco&e= >> >>>> >> >>>> Andrew, Please revert this patch. >> >> >> >> I don't think you have enough convincing arguments for that. >> > >> > That may be the case, but having seen the pain of the original >corruption >> > problems that drove the ioreadX_rep() implementation above, >Milton's >> > protest combined with my initial, briefly nagging concern was >enough for >> > me to revert. Two things here: >> > >> > 1. We've run without this patch for quite some time. Despite >oddities, >> > the existing implementation has been stable >> > 2. With patch 4/4, you've resolved the 4B corruption problem. >This was >> > the immediate concern, as it was impacting teams developing and >> > testing OpenBMC master. I appreciate the effort you put into >hunting >> > that down, the root cause was certainly not obvious. >> > >> > From *my* testing we appear to be stable both with and without >this >> > change, however my concern is that *my* testing is not complete >enough >> > to be confident that we're not going to hit the subtle corruption >problems >> > that drove the introduction of the existing code. >> >> QEMU would have caught this regression if SFDP was modeled. It does >today >> if 4B opcodes are forced on the mx25l25635e. Given the time the >team spent >> on this, I would say 1 or 2PM overall, QEMU is a good investment. >> ^ >> | >> Managers are you reading this ? ------------------+ >> >> > For some additional context, I'm now on leave until the 30th, and >Joel to >> > the 29th. These patches have been through a process that has >proceeded >> > much more hastily than I would have liked, and that's lead to >where we >> > are now. >> > >> > With that in mind, less change is better, and so I have decided >to back >> > this patch out. It's a risk-based decision, not a reflection of >the effort, >> > desires or technicalities involved. >> >> Back to where we were before, it's fine as it works. >> >> The optimize reads are just reading the first 4 bytes : >> >> [ 14.130480] aspeed-smc 1e630000.spi: mx25l25635e (32768 Kbytes) >> [ 14.136664] aspeed-smc 1e630000.spi: write control register: >00122302 >> [ 14.143326] aspeed-smc 1e630000.spi: read control register: >203c2341 >> [ 14.149750] aspeed-smc 1e630000.spi: AHB frequency: 192 MHz >> [ 14.181561] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 >PARTPARTPARTPART >> [ 14.188894] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 >PARTPARTPARTPART >> [ 14.196230] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 >PARTPARTPARTPART >> [ 14.203558] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 >PARTPARTPARTPART >> [ 14.210751] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 >PARTPARTPARTPART >> [ 14.218067] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 >PARTPARTPARTPART >> [ 14.225397] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 >PARTPARTPARTPART >> [ 14.232722] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 >PARTPARTPARTPART >> [ 14.239912] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 >PARTPARTPARTPART >> [ 14.247232] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54 >PARTPARTPARTPART >> >> with memcpy_fromio : >> >> [ 13.779087] aspeed-smc 1e630000.spi: mx25l25635e (32768 Kbytes) >> [ 13.785255] aspeed-smc 1e630000.spi: write control register: >00122302 >> [ 13.791762] aspeed-smc 1e630000.spi: read control register: >203c2341 >> [ 13.798326] aspeed-smc 1e630000.spi: AHB frequency: 192 MHz >> [ 13.815420] 50 41 52 54 00 00 00 01 00 00 00 01 00 00 00 80 >PART............ >> [ 13.822759] 00 00 00 1b 00 00 10 00 00 00 20 00 00 00 00 00 >.......... ..... >> [ 13.829946] 00 00 00 00 00 00 00 00 00 00 00 00 50 41 62 cf >............PAb. >> [ 13.837266] 70 61 72 74 00 00 00 00 00 00 00 00 00 00 00 00 >part............ >> [ 13.844597] 00 00 00 00 00 00 00 01 ff ff ff ff 00 00 00 01 >................ >> [ 13.851788] 00 00 00 03 00 00 00 01 00 00 10 00 00 00 00 00 >................ >> [ 13.859105] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >................ >> [ 13.866433] 00 40 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >.@.............. >> [ 13.873759] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >................ >> [ 13.880951] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >................ >> [ 13.888267] 00 00 00 00 00 00 00 00 00 00 00 00 8f de 9d 89 >................ >> [ 13.895591] 48 42 49 00 00 00 00 00 00 00 00 00 00 00 00 00 >HBI............. >> [ 13.902917] 00 00 00 10 00 00 05 a0 ff ff ff ff 00 00 00 02 >................ >> >> >> I should have added these tests in the commit log. Sorry about >that. >> We will see later on. There are no hurries for this fix. >Optimization >> is still being done. > >Milton: Given you NACK'ed the patch I'd appreciate a follow-up in >light of >this data. > Yes, I should have replied last week. I accept the difference between the case where it fails and the case where its used is the difference between read mode, where the chip verifies the address of each access, and user mode, where the region is decoded at the block level and the data is routed through a fifo, where extra reads or writes are problematic. That said, it will work for read, or where the source and destination have the same alignemnt. The memcpy routine will cause two transactions in the spi controller, but the result will function. I'm not sure a write would work. I withdraw my NACK. milton ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE address mode mask 2019-04-17 13:39 [PATCH dev-5.0 0/4] spi-nor: aspeed: add support for the 4B opcodes Cédric Le Goater ` (4 preceding siblings ...) 2019-04-18 21:23 ` [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer Milton Miller II @ 2019-04-18 22:27 ` Milton Miller II 2019-04-19 6:09 ` Cédric Le Goater 2019-04-18 22:41 ` [PATCH dev-5.0 0/4] spi-nor: aspeed: add support for the 4B opcodes Milton Miller II 6 siblings, 1 reply; 24+ messages in thread From: Milton Miller II @ 2019-04-18 22:27 UTC (permalink / raw) To: Cédric Le Goater; +Cc: openbmc, Andrew Jeffery [-- Attachment #1: Type: text/html, Size: 2056 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE address mode mask 2019-04-18 22:27 ` [PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE address mode mask Milton Miller II @ 2019-04-19 6:09 ` Cédric Le Goater 2019-04-19 6:41 ` Andrew Jeffery 0 siblings, 1 reply; 24+ messages in thread From: Cédric Le Goater @ 2019-04-19 6:09 UTC (permalink / raw) To: Milton Miller II; +Cc: openbmc, Andrew Jeffery On 4/19/19 12:27 AM, Milton Miller II wrote: > About 04/17/2019 09:18AM in some timezone, Cédric Le Goater wrote: > >>Subject: [PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE >>address mode mask >> > > Missing change log here. This is a oneliner patch. > >>Signed-off-by: Cédric Le Goater <clg@kaod.org <mailto:clg@kaod.org>> >>--- >> drivers/mtd/spi-nor/aspeed-smc.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >>diff --git a/drivers/mtd/spi-nor/aspeed-smc.c >>b/drivers/mtd/spi-nor/aspeed-smc.c >>index ee3059b27c07..1437732fdea1 100644 >>--- a/drivers/mtd/spi-nor/aspeed-smc.c >>+++ b/drivers/mtd/spi-nor/aspeed-smc.c >>@@ -884,7 +884,15 @@ static const uint32_t aspeed_smc_hclk_divs[] = { >> >> static u32 aspeed_smc_default_read(struct aspeed_smc_chip *chip) >> { >>- return (chip->ctl_val[smc_read] & 0x2000) | >>+ /* >>+ * Keep the 4Byte address mode on the AST2400 SPI controller. >>+ * Other controllers set the 4Byte mode in the CE Control >>+ * Register >>+ */ >>+ u32 ctl_mask = chip->controller->info == &spi_2400_info ? >>+ CONTROL_IO_ADDRESS_4B : 0; >>+ > > I dislike this patch because it violates the data driven model of the types. > > Either a dupicate method should be created or at least a member of the > type structure should be used instead of a compare to a specific instance. we could add a ops returning the bitmask. yes. I think this is a efficient considering the patch is not in mainline and was not reviewed by the Linux maintainers. C. > >>+ return (chip->ctl_val[smc_read] & ctl_mask) | >> (0x00 << 28) | /* Single bit */ >> (0x00 << 24) | /* CE# max */ >> (0x03 << 16) | /* use normal reads */ >>-- >>2.20.1 >> >> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE address mode mask 2019-04-19 6:09 ` Cédric Le Goater @ 2019-04-19 6:41 ` Andrew Jeffery 0 siblings, 0 replies; 24+ messages in thread From: Andrew Jeffery @ 2019-04-19 6:41 UTC (permalink / raw) To: Cédric Le Goater, Milton Miller II; +Cc: openbmc On Fri, 19 Apr 2019, at 15:39, Cédric Le Goater wrote: > On 4/19/19 12:27 AM, Milton Miller II wrote: > > About 04/17/2019 09:18AM in some timezone, Cédric Le Goater wrote: > > > >>Subject: [PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE > >>address mode mask > >> > > > > Missing change log here. > > This is a oneliner patch. Yes, I think the comment more than made up for the lack of a changelog. Easy enough to view both together, and I find that acceptable for now. Andrew > > > > >>Signed-off-by: Cédric Le Goater <clg@kaod.org <mailto:clg@kaod.org>> > >>--- > >> drivers/mtd/spi-nor/aspeed-smc.c | 10 +++++++++- > >> 1 file changed, 9 insertions(+), 1 deletion(-) > >> > >>diff --git a/drivers/mtd/spi-nor/aspeed-smc.c > >>b/drivers/mtd/spi-nor/aspeed-smc.c > >>index ee3059b27c07..1437732fdea1 100644 > >>--- a/drivers/mtd/spi-nor/aspeed-smc.c > >>+++ b/drivers/mtd/spi-nor/aspeed-smc.c > >>@@ -884,7 +884,15 @@ static const uint32_t aspeed_smc_hclk_divs[] = { > >> > >> static u32 aspeed_smc_default_read(struct aspeed_smc_chip *chip) > >> { > >>- return (chip->ctl_val[smc_read] & 0x2000) | > >>+ /* > >>+ * Keep the 4Byte address mode on the AST2400 SPI controller. > >>+ * Other controllers set the 4Byte mode in the CE Control > >>+ * Register > >>+ */ > >>+ u32 ctl_mask = chip->controller->info == &spi_2400_info ? > >>+ CONTROL_IO_ADDRESS_4B : 0; > >>+ > > > > I dislike this patch because it violates the data driven model of the types. > > > > Either a dupicate method should be created or at least a member of the > > type structure should be used instead of a compare to a specific instance. > > we could add a ops returning the bitmask. yes. I think this is a efficient > considering the patch is not in mainline and was not reviewed by the Linux > maintainers. > > C. > > > > > >>+ return (chip->ctl_val[smc_read] & ctl_mask) | > >> (0x00 << 28) | /* Single bit */ > >> (0x00 << 24) | /* CE# max */ > >> (0x03 << 16) | /* use normal reads */ > >>-- > >>2.20.1 > >> > >> > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH dev-5.0 0/4] spi-nor: aspeed: add support for the 4B opcodes 2019-04-17 13:39 [PATCH dev-5.0 0/4] spi-nor: aspeed: add support for the 4B opcodes Cédric Le Goater ` (5 preceding siblings ...) 2019-04-18 22:27 ` [PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE address mode mask Milton Miller II @ 2019-04-18 22:41 ` Milton Miller II 2019-04-19 7:08 ` Cédric Le Goater 6 siblings, 1 reply; 24+ messages in thread From: Milton Miller II @ 2019-04-18 22:41 UTC (permalink / raw) To: Cédric Le Goater; +Cc: openbmc, Andrew Jeffery [-- Attachment #1: Type: text/html, Size: 1387 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH dev-5.0 0/4] spi-nor: aspeed: add support for the 4B opcodes 2019-04-18 22:41 ` [PATCH dev-5.0 0/4] spi-nor: aspeed: add support for the 4B opcodes Milton Miller II @ 2019-04-19 7:08 ` Cédric Le Goater 0 siblings, 0 replies; 24+ messages in thread From: Cédric Le Goater @ 2019-04-19 7:08 UTC (permalink / raw) To: Milton Miller II; +Cc: openbmc, Andrew Jeffery On 4/19/19 12:41 AM, Milton Miller II wrote: > About 04/17/2019 08:55AM in some timezone, Cédric Le Goater wrote: > > >>Here is a little series providing cleanups on the Aspeed SMC >>Controller >>driver and adding support for the 4B opcodes which were recently >>added >>to the Linux kernels 5.0.x. >> >>The use of the 4B opcodes was breaking the read of the golden buffer >>done at slow speed in the optimization read sequence. The code >>assumed >>that the chip was in 4B address mode, as if a EN4B opcode had been >>sent, but this is not the case anymore with 4B opcodes. The golden >>buffer is now read with a SPINOR_OP_READ_4B (0x13) when the chip >>supports 4B opcodes. >> >>It should fix accesses to the palmetto PNOR and the Witherspoon BMC >>flash modules. >> >> >>*Please test !* > > > Cedric, > > did you notice that spi_nor_read_sfdp is now calling the nor->read method? yes. > Since the read method is hard-coding the opcode, this means that we are > returning random flash contents instead of the flash parameter block, which > probably confuses the kernel. I see your point. but, at the time of the sfdp read, the controller settings are very basic : [ 7.537426] aspeed-smc 1e630000.spi: default control register: 00000300 and the sfdp values are fine. Here is what we get for sfdp on a palmetto PNOR "mx25l25635e" : w5:fffffffe w6:ff00ffff w7:eb44ffff w8:520f200c w9:ff00d810 w10:00000000 This looks consistent with the mx25l25635f specs and with the test being done in Linux in mx25l25635_post_bfpt_fixups(). This driver needs a rewrite because the SPI-NOR layer has been changing a lot. I have started a new one on top of spi-mem but the core layer misses support for link training. So this is still work in progress. Help welcomed. The initial target should be *mainline*. Also, If someone had some time to model SFDP in QEMU, it would really help. We would have caught this regression earlier. One way to reproduce the problem in QEMU is to force 4B opcode in Linux but that's only useful when one knows about the problem already. useful to work on the fix but not to track regressions. Thanks, C. > > milton > > ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2019-05-03 22:19 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-17 13:39 [PATCH dev-5.0 0/4] spi-nor: aspeed: add support for the 4B opcodes Cédric Le Goater 2019-04-17 13:39 ` [PATCH dev-5.0 1/4] mtd: spi-nor: aspeed: introduce a aspeed_smc_default_read() helper Cédric Le Goater 2019-04-17 18:09 ` Alexander Amelkin 2019-04-18 6:07 ` Andrew Jeffery 2019-04-18 6:23 ` Cédric Le Goater 2019-04-17 13:39 ` [PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE address mode mask Cédric Le Goater 2019-04-18 6:09 ` Andrew Jeffery 2019-04-17 13:39 ` [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer Cédric Le Goater 2019-04-18 6:10 ` Andrew Jeffery 2019-04-17 13:39 ` [PATCH dev-5.0 4/4] mtd: spi-nor: aspeed: add support for the 4B opcodes Cédric Le Goater 2019-04-18 6:10 ` Andrew Jeffery 2019-04-18 21:23 ` [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer Milton Miller II 2019-04-19 1:03 ` Andrew Jeffery 2019-04-19 6:02 ` Cédric Le Goater 2019-04-19 7:23 ` Andrew Jeffery 2019-04-19 8:09 ` Cédric Le Goater 2019-04-19 8:22 ` Andrew Jeffery 2019-05-02 3:53 ` Andrew Jeffery 2019-05-03 22:19 ` Milton Miller II 2019-04-18 22:27 ` [PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE address mode mask Milton Miller II 2019-04-19 6:09 ` Cédric Le Goater 2019-04-19 6:41 ` Andrew Jeffery 2019-04-18 22:41 ` [PATCH dev-5.0 0/4] spi-nor: aspeed: add support for the 4B opcodes Milton Miller II 2019-04-19 7:08 ` 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.