[PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer
Cédric Le Goater
clg at kaod.org
Fri Apr 19 18:09:17 AEST 2019
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 . at ..............
[ 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 at 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
>>>>>
>>>>>
>>>>
>>>>
>>
>>
More information about the openbmc
mailing list