Re: [PATCH 4/5] mtd: m25p80: remove M25PXX_USE_FAST_READ Kconfig

[Marek Vasut <marex@denx.de>]

Dear Brian Norris,

> Hi Marek,
> 
> On Fri, Nov 01, 2013 at 01:26:15PM +0100, Marek Vasut wrote:
> > Hi Brian,
> > 
> > > Hi Marek,
> > > 
> > > On Thu, Oct 31, 2013 at 10:21:42AM +0100, Marek Vasut wrote:
> > > > Dear Brian Norris,
> > > > 
> > > > > On Sun, Oct 27, 2013 at 05:32:42PM +0100, Marek Vasut wrote:
> > > > > > Hi Brian,
> > > > > > 
> > > > > > > --- a/drivers/mtd/devices/m25p80.c
> > > > > > > +++ b/drivers/mtd/devices/m25p80.c
> > > > > > > @@ -1055,13 +1055,14 @@ static int m25p_probe(struct spi_device
> > > > > > > *spi)
> > > > > > > 
> > > > > > >  	flash->page_size = info->page_size;
> > > > > > >  	flash->mtd.writebufsize = flash->page_size;
> > > > > > > 
> > > > > > > -	flash->fast_read = false;
> > > > > > > -	if (np && of_property_read_bool(np, "m25p,fast-read"))
> > > > > > > +	if (np)
> > > > > > > +		/* If we were instantiated by DT, use it */
> > > > > > > +		flash->fast_read = of_property_read_bool(np,
> > > > > > > "m25p,fast-read"); +	else
> > > > > > > +		/* If we weren't instantiated by DT, default to fast-
read */
> > > > > > > 
> > > > > > >  		flash->fast_read = true;
> > > > > > 
> > > > > > We should default to FALSE , unless explicitly requested by DT,
> > > > > > am I wrong? Otherwise this might break the old chips.
> > > > > 
> > > > > I believe my patch is simply a refactoring of the existing code
> > > > > with the assumption that M25PXX_USE_FAST_READ=y.
> > > 
> > > Ah, my statement was wrong: for DT-instantiated devices, my patch
> > > "defaulted" as if M25PXX_USE_FAST_READ=n; for devices without a DT
> > > node, it defaulted as if M25PXX_USE_FAST_READ=y.
> > 
> > I'd say old devices (which are not DT capable even) might actually be
> > less able to support the FAST READ opcode. But I think we're reaching a
> > point where we cannot clearly tell and either way we will break some
> > devices here. If that's the case, let's do it ;-)
> 
> I agree, let's just do it.
> 
> > > > > (In my experience, everyone has
> > > > > it set to y. Perhaps I'm wrong.)
> > > > 
> > > > OK, this is where we diverged. I always set FAST_READ to =n and only
> > > > enabled it via this DT prop if I was dead sure the chip could do it.
> > > 
> > > Right, that actually makes sense, and your mode is not affected by my
> > > patch. For DT-enabled devices, we default to the DT property.
> > > 
> > > The only mode that has less flexibility is for platform devices
> > > (non-DT), where we only use normal read for devices that can't handle
> > > FAST READ.
> > 
> > And there we should disable it by default to play safe, no ?
> 
> Unless the device table M25P_NO_FR flag is imprecisely-used (which it
> may be), I don't think we need a "play-it-safe" option here. The current
> code seems to have the right level of precision.

Yes, I found this NO_FR flag just yesterday (shame on me!) when I was searching 
for some strange behavior on one of my boards here. I think we're good.

> > > > My understanding of FAST_READ is that after you send FAST_READ
> > > > opcode, you can read all you want until CS is toggled, only then you
> > > > can send another opcode. The "slow" READ opcode on the other hand
> > > > has to be sent for each block.
> > > 
> > > As Sourav said in his response, I don't believe this is true. AIUI, the
> > > only differences are the dummy cycles and the maximum clock frequency.
> > 
> > Aka. "slow" READ can also read as many bytes as seen fit ?
> 
> Yes, according to the data sheet I read.
> 
> > > > > If you still object to this patch, I can drop the patch from
> > > > > l2-mtd.git for now. Then Sourav will need to rebase his patch
> > > > > again.
> > > 
> > > So what do you think? I feel like my current patch is the right way to
> > > go, but I think I could update the description to be more verbose,
> > > since there are obviously a few other issues coming into play here.
> > 
> > Yes, I won't hold it any longer. If someone complains, we will know where
> > the wind blows from anyway and then we can fix his platform properly.
> 
> Sounds good. I'm updating the commit to include the following
> description (no code changes, so no need to rebase, Sourav):

Cool, thanks!

> ------------------ BEGIN DESCRIPTION ------------------
> Remove the compile-time option for FAST_READ, since we have run-time
> support for detecting it. This refactors the logic for enabling
> fast-read, such that for DT-enabled devices, we honor the
> "m25p,fast-read" property but for non-DT devices, we default to using
> FAST_READ whenever the flash device supports it according to our
> m25p_ids[] table.
> 
> Normal READ and FAST_READ differ only in the following:
> 
>   * FAST_READ supports SPI higher clock frequencies [1]
> 
>   * number of dummy cycles; FAST_READ requires 8 dummy cycles (whereas
>     READ requires 0) to allow the flash sufficient setup time, even when
>     running at higher clock speeds
> 
> Thus, for flash chips which support FAST_READ, there is otherwise no
> limiting reason why we cannot use the FAST_READ opcode instead of READ.
> It simply allows the SPI controller to run at higher clock rates. So
> theoretically, nobody should be needing the compile-time option anyway.
> 
>   [1] I have a Spansion S25FL128S datasheet which says:
> 
>     "The maximum operating clock frequency for the READ command is 50
>     MHz."
> 
>   And:
> 
>     "The maximum operating clock frequency for FAST READ command is 133
>     MHz."
> -------------------- END DESCRIPTION ------------------
> 
> Brian