Re: [PATCH RFC 0/4] Add set_iofv() callback

From: Michael Walle <michael@walle.cc>
To: Biju Das <biju.das.jz@bp.renesas.com>
Cc: Mark Brown <broonie@kernel.org>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
	linux-spi@vger.kernel.org, linux-mtd@lists.infradead.org,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	"biju.das.au" <biju.das.au@gmail.com>,
	linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH RFC 0/4] Add set_iofv() callback
Date: Thu, 09 Nov 2023 11:48:14 +0100	[thread overview]
Message-ID: <b9831be88008b9f9960d1d79cd0e5a3a@walle.cc> (raw)
In-Reply-To: <TYVPR01MB11279E535835F2998335F770A86AFA@TYVPR01MB11279.jpnprd01.prod.outlook.com>

Hi Biju,

>> > As per section 8.14 on the AT25QL128A hardware manual[1],
>> > IO0..IO3 must be set to Hi-Z state for this flash for fast read quad
>> > IO.
>> > Snippet from HW manual section 8.14:
>> > The upper nibble of the Mode(M7-4) controls the length of the next
>> > FAST Read Quad IO instruction through the inclusion or exclusion of
>> > the first byte instruction code. The lower nibble bits of the
>> > Mode(M3-0) are don't care. However, the IO pins must be high-impedance
>> > before the falling edge of the first data out clock.
>> 
>> I'm still not sure what you are trying to fix here. For any quad I/O 
>> mode,
>> the pins of the controller must be in hiZ during the data phase on a 
>> read
>> operation. Otherwise the flash couldn't send any data, there would be 
>> two
>> drivers for one signal. So being in hiZ state should be the default 
>> and
>> shouldn't depend on any connected flash.
> 
> OK, I will make hiZ state as the default.

I still think this iofv setting is the wrong approach, though. Do you
have a link to the spi controller datasheet where I can look up what
the controller is doing.

This seem to be a general problem with what we are sending during the
command phase and I'm curious why there wasn't more reports on non
working micron flashes for now.

>> You've mentioned the micron flash which needs a '1' on its hold/reset 
>> pin.
>> I would have expected a fixup for this flash, not for the flash which
>> behaves normal.
> 
> I will drop fixup for Renesas AT25QL128A  and will add fixup for micron 
> flash.

btw, what will happen if you always use the {3,3,3,1} setting? I guess
the atmel flash will also work? because HiZ should mean "don't care" 
from
the point of view of the flash.

> 
> With iofv settings {3,3,3,3} (all pins on Hi-Z state) with Micron flash
> -----------------------------------------------------------------------
> 
> ./rpcif_t_001.sh
> [   37.950986] spi-nor spi1.0: unrecognized JEDEC id bytes: ff ff ff ff 
> ff ff

As mentioned earlier, I suspect that HiZ on IO3 means low and the flash
will be in reset. Could you perhaps verify that by probing IO3?
I know that other flashes will *either* support RESET#/HOLD# or quad 
mode.
Thus I was saying, that we probably wont support that and the easiest
fix should be to disable this behavior for the atmel flash (there was
nv setting).

I guess, the correct fix would be to somehow add support to control
IO1-IO3 during the (single bit) command phase.

-michael

> 
> EXIT|FAIL|rpcif_t_001.sh|[00:00:01] Failed to detect mt25qu512a 
> flash!||
> 
> 
> With iofv settings {3,3,3,1} with Micron falsh
> ---------------------------------------------
> root@smarc-rzg2l:/cip-test-scripts# ./rpcif_t_001.sh
> [   26.500035] spi-nor spi1.0: mt25qu512a (65536 Kbytes)
> [   26.533995] 2 fixed-partitions partitions found on MTD device spi1.0
> [   26.540410] Creating 2 MTD partitions on "spi1.0":
> [   26.545239] 0x000000000000-0x000002000000 : "boot"
> [   26.554381] 0x000002000000-0x000004000000 : "user"
> 
> EXIT|PASS|rpcif_t_001.sh|[00:03:01] ||
> 
> Cheers,
> Biju