Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 11 May 2020 23:54:55 +0530
From:   Pratyush Yadav <p.yadav@ti.com>
To:     Boris Brezillon <boris.brezillon@collabora.com>
CC:     <Tudor.Ambarus@microchip.com>, <alexandre.belloni@bootlin.com>,
        <vigneshr@ti.com>, <richard@nod.at>, <nsekhar@ti.com>,
        <Nicolas.Ferre@microchip.com>, <linux-kernel@vger.kernel.org>,
        <Ludovic.Desroches@microchip.com>, <broonie@kernel.org>,
        <linux-mtd@lists.infradead.org>, <miquel.raynal@bootlin.com>,
        <linux-spi@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v4 00/16] mtd: spi-nor: add xSPI Octal DTR support
Message-ID: <20200511182453.ivfwu4wlfn2ta5ag@ti.com>
References: <20200424184410.8578-1-p.yadav@ti.com>
 <3649933.zuh8VGJVCz@192.168.0.120>
 <20200511112712.466f7246@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20200511112712.466f7246@collabora.com>
User-Agent: NeoMutt/20171215
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 11/05/20 11:27AM, Boris Brezillon wrote:
> On Mon, 11 May 2020 09:00:35 +0000
> <Tudor.Ambarus@microchip.com> wrote:
> 
> > Hi, Pratyush, Boris,
> > 
> > On Friday, April 24, 2020 9:43:54 PM EEST Pratyush Yadav wrote:
> > > This series adds support for octal DTR flashes in the spi-nor framework,  
> > 
> > I'm still learning about this, but I can give you my 2 cents as of now, to 
> > open the discussion. Enabling 2-2-2, 4-4-4, and 8-8-8 modes is dangerous 
> > because the flash may not recover from unexpected resets. Entering one of 
> > these modes can be:
> > 1/ volatile selectable, the device return to the 1-1-1 protocol after the next 
> > power-on. I guess this is conditioned by the optional RESET pin, but I'll have 
> > to check. Also the flash can return to the 1-1-1 mode using the software reset 
> > or through writing to its Configuration Register, without power-on or power-
> > off.
> 
> My understanding is that there's no standard software reset procedure
> that guarantees no conflict with existing 1S commands, so even the
> software reset approach doesn't work here.
> 
> > 2/ non-volatile selectable in which RESET# and software reset are useless, the 
> > flash defaults to the mode selected in the non volatile Configuration Register 
> > bits. The only way to get back to 1-1-1 is to write to the Configuration 
> > Register.
> 
> I'm less worried about this case though, since I'd expect the ROM
> code and bootloaders to be able to deal with xD-xD-xD modes when the
> flash is set in this mode by default. That implies letting Linux know
> about this default mode of course, maybe through an extra DT
> property/cmdline param.
> 
> > 
> > Not recovering from unexpected resets is unacceptable. One should always 
> > prefer option 1/ and condition the entering in 2-2-2, 4-4-4 and 8-8-8 with the 
> > presence of the optional RESET pin.
> 
> Totally agree with you on that one, but we know what happens in
> practice...
> 
> > 
> > For the unfortunate flashes that support just option 2/, we should not enter 
> > these modes on our own, just by discovering the capabilities from the SFDP 
> > tables or by the flags in the flash_info struct. The best we can do for them 
> > is to move the responsibility to the user. Maybe to add a Kconfig option that 
> > is disabled by default with which we condition the entering in 2-2-2, 4-4-4 or 
> > 8-8-8 modes.
> 
> Hm, a Kconfig option doesn't sound like the right solution to the
> problem, since it should be a per-flash decision, not something you set
> system-wise.

Agreed.

Is there any such flash in use today? The two flashes the series adds 
support for both have volatile configuration for 8D mode. Unless we have 
to support a flash like this in practice, I think such a change is out 
of the scope of this series.
 
> > Once entered in one of these modes, if an unexpected reset comes, 
> > you most likely are doomed, because early stage bootloaders may not work in 
> > these modes and you'll not be able to boot the board. Assuming that one uses 
> > other environment to boot the board, we should at least make sure that the 
> > flash works in linux after an unexpected reset. We should try to determine in 
> > which mode we are at init, so maybe an extension of the default_init hook is 
> > needed. But all this looks like a BIG compromise, I'm not yet sure if we 
> > should adress 2/. Thoughts?
> 
> We should definitely not write non-volatile regs on our own, but
> instead use the mode that's been chosen there. I doubt anyone
> setting the non-volative conf to 8D-8D-8D will ever want to go back to
> 1S-1S-1S anyway, so 8D -> 1S transitions are not really an issue, right?
> 
> Of course, that still leaves us with the 'mode detection' issue, and I
> have no solution other than flagging it through the DT/cmdline for that
> one...

Correct. I tried doing it, and the best way I could figure out was to 
try reading the SFDP signature in 1S and 8D mode, and see where we get 
the correct value. But unfortunately, because the Read ID command is 
different in 8D mode for different flashes, we can't then figure out 
which flash it actually is.

-- 
Regards,
Pratyush Yadav
Texas Instruments India