Re: [PATCH] mtd: spi-nor: winbond: add Zetta ZD25Q128C support

From: Pratyush Yadav <pratyush@kernel.org>
To: "Michael Walle" <mwalle@kernel.org>
Cc: "Pratyush Yadav" <pratyush@kernel.org>,
	 "Tudor Ambarus" <tudor.ambarus@linaro.org>,
	 "Miquel Raynal" <miquel.raynal@bootlin.com>,
	"Richard Weinberger" <richard@nod.at>,
	 "Vignesh Raghavendra" <vigneshr@ti.com>,
	 <linux-mtd@lists.infradead.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] mtd: spi-nor: winbond: add Zetta ZD25Q128C support
Date: Mon, 12 Aug 2024 16:45:45 +0200	[thread overview]
Message-ID: <mafs0r0atztnq.fsf@kernel.org> (raw)
In-Reply-To: <D3E0BKGQIZ9R.1ULZZ93E18VGM@kernel.org> (Michael Walle's message of "Mon, 12 Aug 2024 16:36:11 +0200")

On Mon, Aug 12 2024, Michael Walle wrote:

> Hi,
>
>> > Zetta normally uses BAh as its vendor ID. But for the ZD25Q128C they
>> > took the one from Winbond and messed up the size parameters in SFDP.
>> > Most functions seem compatible with the W25Q128, we just have to fix up
>> > the size.
>> >
>> > Link: http://www.zettadevice.com/upload/file/20150821/DS_Zetta_25Q128_RevA.pdf
>> > Link: https://www.lcsc.com/datasheet/lcsc_datasheet_2312081757_Zetta-ZD25Q128CSIGT_C19626875.pdf
>> > Signed-off-by: Michael Walle <mwalle@kernel.org>
>> > ---
>> [...]
>> > diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
>> > index e065e4fd42a3..9f7ce5763e71 100644
>> > --- a/drivers/mtd/spi-nor/winbond.c
>> > +++ b/drivers/mtd/spi-nor/winbond.c
>> > @@ -17,6 +17,31 @@
>> >  		   SPI_MEM_OP_NO_DUMMY,					\
>> >  		   SPI_MEM_OP_DATA_OUT(1, buf, 0))
>> >  
>> > +static int
>> > +w25q128_post_bfpt_fixups(struct spi_nor *nor,
>> > +			 const struct sfdp_parameter_header *bfpt_header,
>> > +			 const struct sfdp_bfpt *bfpt)
>> > +{
>> > +	/*
>> > +	 * Zetta ZD25Q128C is a clone of the Winbond device. But the encoded
>> > +	 * size is really wrong. It seems that they confused Mbit with MiB.
>> > +	 * Thus the flash is discovered as a 2MiB device.
>> > +	 */
>> > +	if (bfpt_header->major == SFDP_JESD216_MAJOR &&
>> > +	    bfpt_header->minor == SFDP_JESD216_MINOR &&
>> > +	    nor->params->size == SZ_2M &&
>> > +	    nor->params->erase_map.regions[0].size == SZ_2M) {
>> > +		nor->params->size = SZ_16M;
>> > +		nor->params->erase_map.regions[0].size = SZ_16M;
>> > +	}
>>
>> Since the size is 16 MiB for both Zetta and Winbond, why do you have
>> these conditions here? Why not just do it unconditionally? What
>> situation do you want to protect against?
>
> Two things, I want to make sure, the values I'll overwrite (which
> were set indirectly by the SFDP data I check here) are really the
> "before" value I expect them to be. And secondly, I want to
> fingerprint the SFDP as accurately as possible. I mean you wouldn't
> need the SFDP version either. Think of a preparation for a possible
> .match() callback in zetta.c. That would probably look like this,
> i.e. check the flash id and these four conditions.

Hmm, I see. I personally find doing it unconditional a bit easier to
grok but this is fine too I think.

Reviewed-by: Pratyush Yadav <pratyush@kernel.org>

> What are your concerns?

None. I was mostly curious why you did it this way.

-- 
Regards,
Pratyush Yadav

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