Re: [PATCH v4 1/6] mtd: spi-nor: Add manufacturer read id function

From: Michael Walle <michael@walle.cc>
To: liao jaime <jaimeliao.tw@gmail.com>
Cc: Tudor Ambarus <tudor.ambarus@linaro.org>,
	Pratyush Yadav <pratyush@kernel.org>,
	linux-mtd@lists.infradead.org, miquel.raynal@bootlin.com,
	leoyu@mxic.com.tw, jaimeliao@mxic.com.tw
Subject: Re: [PATCH v4 1/6] mtd: spi-nor: Add manufacturer read id function
Date: Fri, 13 Oct 2023 11:32:34 +0200	[thread overview]
Message-ID: <7b2bc98cee7120a510d22c443e0f3486@walle.cc> (raw)
In-Reply-To: <CAAQoYRnektLQaw__dkxXaOcRJ_B1gazC6ukW-qjEeU89W6oYuQ@mail.gmail.com>

Am 2023-10-13 11:14, schrieb liao jaime:
> Hi Michael
> 
> 
>> 
>> Hi,
>> 
>> Am 2023-10-13 10:23, schrieb liao jaime:
>> >> >> > After checking with Macronix designer, a-a-b-b-c-c is the data
>> >> >> > arrangement for
>> >> >> > read id operation of flash in 8D-8D-8D.
>> >> >>
>> >> >> Could you please point to any specification? I doubt there is one
>> >> >> and every vendor will do it slightly differently. I mean we already
>> >> >> have some flashes which (apparently) reply to RDID in 8d8d8d.
>> >> >>
>> >> >> For example, see the Semper flash datasheet:
>> >> >> https://www.infineon.com/dgdl/Infineon-S28HS256T_S28HS512T_S28HS01GT_S28HL256T_S28HL512T_S28HL01GT_256-Mb_(32-MB)_512-Mb_(64-MB)_1-Gb_(128-MB)_HS-T_(1.8-V)_HL-T_(3.0-V)_Semper_Flash_with_Octal_Interface-DataSheet-v03_00-EN.pdf?fileId=8ac78c8c7d0d8da4017d0ee6bca96f97&da=t
>> >> >>
>> >> >> Have a look at Table 78 (or search for RDIDN_4_0) and Figure 28.
>> >> > For Figure 28 in this datasheet, I think it means that data latch
>> >> > while DS raising and falling edge.
>> >> > The data arrangement of read id follow the 9.2(Manufacturer and Device
>> >> > ID).
>> >> > As below are the data arrangement for vendors.
>> >> > For Infineon, a-b-c-d-e-f
>> >> > For Micron, a-b-c-d-e-f
>> >> > For Macronix, a-a-b-b-c-c
>> >>
>> >> So there is no standard among vendors, infineon as well as micron is
>> >> using 8d8d8d and macronix is using 8d8d8s. And - please correct me if
>> > Macronix read id operation is not 8d8d8s, it just look like 8d8d8s on
>> > datasheet
>> > but flash send 2 bytes data per DQS cycle exactly.
>> 
>> Two DQS edges, yes.
>> 
>> > I think whether to use 8d or 8s depends on how many times data is
>> > latched
>> > within per cycle.
>> 
>> I see and I agree with you. The flash behaves like it's in 8d8d8d 
>> mode,
>> but
>> just send every byte of the id twice. Well, then I'd argue, it's a 
>> quirk
>> of
>> your flash.
> I cannot refute that.
> 
>> 
>> So Tudor and Pratyush want to hide that away in macronix.c. I would 
>> like
>> to
>> see this handled in the core, because all flashes do the very same 
>> after
>> switching to octal mode and that is trying to do a rdid and see 
>> whether
>> they got a sane response. And during cleanup I noticed that this code 
>> is
>> pretty much copy and paste among all these flashes and looks very
>> open-coded.
>> 
>> But to move forward here, keep it in macronix.c.
> Do you mean that I can keep v3 method as below for comparing ID after 
> enable
> 8D-8D-8D mode?
> 
> for (i = 0; i < nor->info->id_len; i++)
> if (buf[i * 2] != nor->info->id[i])
> return -EINVAL;

Technically, this is wrong. Because we only read SPI_NOR_MAX_ID_LEN.
and nor->info->id_len might be as large as SPI_NOR_MAX_ID_LEN. So you'll
have out of bounds accesses. spi_nor_read_id() isn't really useful here.
You'd need to cook up your own RDID command. And that results in even 
more
open coding...

Aside from that, I'd also check that buf[i] == buf[i+1] for i=0,2,4,...

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/