Re: [PATCH v2] mtd: spi-nor: handle unsupported FSR opcodes properly

From: <Tudor.Ambarus@microchip.com>
To: <mika.westerberg@linux.intel.com>, <oocheret@cisco.com>
Cc: <miquel.raynal@bootlin.com>, <p.yadav@ti.com>, <michael@walle.cc>,
	<richard@nod.at>, <vigneshr@ti.com>, <broonie@kernel.org>,
	<linux-mtd@lists.infradead.org>, <linux-spi@vger.kernel.org>,
	<mauro.lima@eclypsium.com>, <lee.jones@linaro.org>,
	<linux-kernel@vger.kernel.org>, <xe-linux-external@cisco.com>
Subject: Re: [PATCH v2] mtd: spi-nor: handle unsupported FSR opcodes properly
Date: Tue, 19 Jul 2022 09:12:05 +0000	[thread overview]
Message-ID: <9ff9b84d-dc86-fb6d-4446-b39dfd9430ea@microchip.com> (raw)
In-Reply-To: <YqsHcL5NPcZ4De77@lahna>

On 6/16/22 13:35, Mika Westerberg wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi,
> 
> On Thu, Jun 16, 2022 at 07:40:18AM +0000, Oleksandr Ocheretnyi -X (oocheret - GLOBALLOGIC INC at Cisco) wrote:
>>    Hi Mika,
>>
>>      > Originally commit 094d3b9 ("mtd: spi-nor: Add USE_FSR flag for
>>      n25q*
>>      > entries") and following one 8f93826 ("mtd: spi-nor: micron-st:
>>      convert
>>      > USE_FSR to a manufacturer flag") enabled SPINOR_OP_RDFSR opcode
>>      handling
>>      > ability, however some controller drivers still cannot handle it
>>      properly
>>      > in the micron_st_nor_ready() call what breaks some mtd callbacks
>>      with
>>      > next error logs:
>>      >
>>      > mtdblock: erase of region [address1, size1] on "BIOS" failed
>>      > mtdblock: erase of region [address2, size2] on "BIOS" failed
>>      >
>>      > The Intel SPI controller does not support low level operations,
>>      like
>>      > reading the flag status register (FSR). It only exposes a set of
>>      high
>>      > level operations for software to use. For this reason check the
>>      return
>>      > value of micron_st_nor_read_fsr() and if the operation was not
>>      > supported, use the status register value only. This allows the
>>      chip to
>>      > work even when attached to Intel SPI controller (there are such
>>      systems
>>      > out there).
>>      >
>>
>>    > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>>
>>      I don't think I signed this off.
>>
>>    I thought if I take your case (-EOPNOTSUPP) and update it with
>>    (-ENOTSUPP) I need to keep
>>
>>    your Sighed-off-by: note as well.
> 
> That's not how it typically works. People will give their tag explicitly
> and then you can add those.
> 
>>    > Signed-off-by: Oleksandr Ocheretnyi <oocheret@cisco.com>
>>    > Link: [1]https://lore.kernel.org/lkml/YmZUCIE%2FND82BlNh@lahna/
>>    > ---
>>
>>    What changed between v1 and v2?
>>
>>    I updated v1 patch taking into account your changes
>>    [2]https://lore.kernel.org/linux-mtd/20220506105158.43613-1-mika.wester
>>    berg@linux.intel.com to check -EOPNOTSUPP case as well. After I
>>    combined both patches I've got v2.
> 
> Please put that information after the '---' in the patch.
> 
>>    And did you take into consideration the comments I gave?
>>
>>    If you say about keeping -ENOTSUPP as intel driver errorcode - I took
>>    it however doubted to use it here because of note about nfs above.
>>    There is no problem to restore previous variant with -ENOTSUPP in intel
>>    driver errorcode.
> 
> Well we would need to get some feedback from SPI-NOR maintainers. I
> would personally keep using ENOTSUPP to be consistent with the rest of
> the code in SPI-NOR code (or convert it to use EOPNOTSUPP everywhere)

SPI NOR does not return -ENOTSUPP, but SPI MEM does. Let's use EOPNOTSUPP
in SPI NOR and verify if we can do a patch to s/ENOTSUPP/EOPNOTSUPP in SPI MEM.

> but it is not up to me ;-)

> 
> For Intel driver it is fine to use either (whetever the decision of
> SPI-NOR maintainers' is).

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