Currently, BUS_NONSPI = BUS_PARALLEL | BUS_LPC | BUS_FWH so it does not include BUS_PROG, and of course not BUS_SPI. It seems to cover what is needed?
I also need to correct my little piece of code, it actually should be if (flash->chip->bustype & BUS_NONSPI) It seems like something everyone can agree on? The patch saves 1s for every flashrom run, for almost all users, so it's a good thing. If it can be unblocked now just by adding 1-line condition, let's unblock it now. Perhaps there is a bigger question of Parallel/LPC/FWH flash chips which need special treatment, but rarely used nowadays. I want to discuss, but as a topic by itself. I don't think we need to resolve all of the problems in one patch :) On Fri, Apr 26, 2024 at 8:54 AM Brian Norris <briannor...@chromium.org> wrote: > > Hi Angel, > > On Thu, Apr 25, 2024 at 11:44:47AM +0000, Angel Pons wrote: > > On Thu, Apr 25, 2024 at 10:26 AM Anastasia Klimchuk <a...@chromium.org> > > wrote: > > > /* > > * Work around chips which need some time to calm down. > > * Not needed for SPI flash chips because of the bus' strict timing. > > */ > > if (flash->chip->bustype != BUS_SPI) > > programmer_delay(flashctx, 1000*1000); > > For the record, linux_mtd is a primary target for all Arm Chromebooks, > and it registers an opaque programme with BUG_PROG. Presumably an > "opaque" chip qualifies as "safe enough" too, because the opaque > abstraction should be taking care of timing details? If not, then this > is not a sufficient solution for me. > > I guess my proposal would probably enumerate the old buses > (BUS_PARALLEL, BUS_LPC, BUS_FWH) specifically, to avoid further > unintentional inclusion. > > Anyway, I appreciate the general thrust of your thoughts and > suggestions. I'll reply to a few bits below. > > > [The following is a textual representation of my thought processes; > > it's not particularly necessary but I felt it could be interesting] > > Appreciated! > > > My idea is to maintain backwards compatibility while still enabling > > new features and improvements. [...] > > I appreciate the attempt at a middle ground here. > > > > On Thu, Apr 25, 2024 at 7:35 AM Angel Pons <th3fan...@gmail.com> wrote: > > > > On Wed, Apr 24, 2024 at 9:16 PM Brian Norris <briannor...@chromium.org> > > > > wrote: > > > > > > > > > > Background: https://review.coreboot.org/c/flashrom/+/80807 > > > > > > > > > > A long time ago, in the pre-git times [1], flashrom added a 1 second > > > > > delay to all verification, and claimed that some chips "need some time > > > > > to calm down." The commit message claims it "fixes a few reports where > > > > > verify directly after erase had unpleasant side effects like > > > > > corrupting flash or at least getting incorrect verify results." It > > > > > provides no details of what systems, chips, or programmers were > > > > > involved. > > > > > > > > Back then, SPI flash chips weren't as ubiquitous as they currently > > > > are. This workaround is most likely for Parallel/LPC/FWH flash chips, > > > > which can actually be quite weird. > > Interesting callout. I've dealt with some parallel NOR as well in the > past, and I don't really recall magical sleeps there either. Look around > at Linux's drivers/mtd/chips/cfi* for example. There's some incidence of > udelay()/msleep()/etc., but they're coupled with status checks and > polling loops, similar to any SPI protocol. The only exception I found > is a verbosely documented cfi_fixup_m29ew_delay_after_resume() sleep. > And even that's only 0.5 milliseconds. > > But I suppose the existence of good parallel flash drivers doesn't > preclude the existence of poor ones elsewhere. > > Do you have any kind of examples of what "weirdness" might be present > here? Or is this just a general feeling and guess based on the year of > the original change? > > > > > If we don't know for sure whether this delay is no longer needed, I > > > > wouldn't risk re-introducing issues in flashrom. > > I don't think that condition is possible to satisfy given sufficiently > under-documented "fixes" from ancient history. I appreciate that we > don't want to regress things, but when there's no evidence or details of > the initial problem, it's logically impossible to prove its > non-existence now. > > But toward a non-guaranteed proof: see consideration #3 in my > aforementioned patch. We already perform verify-after-erase steps > without any such delay. Wouldn't this path be affected in the same way, > if it was still a real problem? Similarly, we have no such protection > against a sequence of `flashrom --noverify --write; flashrom --read`. So > even if the delay was serving some purpose, it seems a wholly > insufficient one that feels little better than a band-aid. > > Still, maybe y'all consider the band-aid better than ripping it off. And > if so, I'll consider incorporating your suggestion. > > > > > Do Chromebooks use non-SPI flash chips? They probably don't. > > Not that I know of. > > > > > Did you conside making it so that only SPI programmers / flash chips > > > > skip the delay? The SPI bus' strict timing leaves no room for this > > > > problem to occur, so it should be safe to skip this delay. And this > > > > would keep non-SPI unaffected (which is most likely what needs this > > > > extra delay). > > I did not consider this yet, but I'm considering it now! If this is the > best semi-conservative solution we can come up with, I suppose I'm fine > with that. Per the above, I don't think non-SPI is relevant to anything > I care about, so maybe it's good enough. > > Thanks for your thoughts, > Brian -- Anastasia. _______________________________________________ flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-le...@flashrom.org
