[PATCH dev-5.0 0/4] spi-nor: aspeed: add support for the 4B opcodes

[PATCH dev-5.0 0/4] spi-nor: aspeed: add support for the 4B opcodes

[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

[PATCH dev-5.0 1/4] mtd: spi-nor: aspeed: introduce a aspeed_smc_default_read() helper

[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

Re: [PATCH dev-5.0 1/4] mtd: spi-nor: aspeed: introduce a aspeed_smc_default_read() helper

[Alexander Amelkin]

[-- 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 --]

Re: [PATCH dev-5.0 1/4] mtd: spi-nor: aspeed: introduce a aspeed_smc_default_read() helper

[Andrew Jeffery]

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

Re: [PATCH dev-5.0 1/4] mtd: spi-nor: aspeed: introduce a aspeed_smc_default_read() helper

[Cédric Le Goater]

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

[PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE address mode mask

[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

Re: [PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE address mode mask

[Andrew Jeffery]

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

[PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer

[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

Re: [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer

[Andrew Jeffery]

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

[PATCH dev-5.0 4/4] mtd: spi-nor: aspeed: add support for the 4B opcodes

[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

Re: [PATCH dev-5.0 4/4] mtd: spi-nor: aspeed: add support for the 4B opcodes

[Andrew Jeffery]

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

Re: [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer

[Milton Miller II]

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
>
>

Re: [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer

[Andrew Jeffery]

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
> >
> >
> 
>

Re: [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer

[Cédric Le Goater]

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
>>>
>>>
>>
>>

Re: [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer

[Andrew Jeffery]

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
> >>>
> >>>
> >>
> >>
> 
>

Re: [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer

[Cédric Le Goater]

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
>>>>>
>>>>>
>>>>
>>>>
>>
>>

Re: [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer

[Andrew Jeffery]

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
> >>>>>
> >>>>>
> >>>>
> >>>>
> >>
> >>
> 
>

Re: [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer

[Andrew Jeffery]

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

Re: [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer

[Milton Miller II]

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

Re: [PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE address mode mask

[Milton Miller II]

[-- Attachment #1: Type: text/html, Size: 2056 bytes --]

Re: [PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE address mode mask

[Cédric Le Goater]

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
>>
>>

Re: [PATCH dev-5.0 2/4] mtd: spi-nor: aspeed: clarify 4BYTE address mode mask

[Andrew Jeffery]

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
> >>
> >>
> 
>

Re: [PATCH dev-5.0 0/4] spi-nor: aspeed: add support for the 4B opcodes

[Milton Miller II]

[-- Attachment #1: Type: text/html, Size: 1387 bytes --]

Re: [PATCH dev-5.0 0/4] spi-nor: aspeed: add support for the 4B opcodes

[Cédric Le Goater]

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
> 
>