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