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

["Cédric Le Goater" <clg@kaod.org>]

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