There are infineon flashes [1] that require 8 dummy cycles for the
1-1-1 Read ID command. Since the command is not covered by JESD216
or any other standard, get the number of dummy cycles from DT and use
them to correctly identify the flash.
Link: https://www.infineon.com/dgdl/Infineon-CYRS17B512_512_MB_64_MB_SERIAL_NOR_FLASH_SPI_QSPI_3-DataSheet-v07_00-EN.pdf?fileId=8ac78c8c8fc2dd9c01900eee733d45f3 [1]
Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
drivers/mtd/spi-nor/core.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 19eb98bd68210f41acd716635c02a8936678a385..6452ae6eecee3325b52cdcc2cc9703355951e0db 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -7,6 +7,7 @@
* Copyright (C) 2014, Freescale Semiconductor, Inc.
*/
+#include <linux/bits.h>
#include <linux/err.h>
#include <linux/errno.h>
#include <linux/delay.h>
@@ -16,6 +17,7 @@
#include <linux/mtd/mtd.h>
#include <linux/mtd/spi-nor.h>
#include <linux/mutex.h>
+#include <linux/of.h>
#include <linux/of_platform.h>
#include <linux/regulator/consumer.h>
#include <linux/sched/task_stack.h>
@@ -2011,9 +2013,14 @@ static const struct flash_info *spi_nor_detect(struct spi_nor *nor)
{
const struct flash_info *info;
u8 *id = nor->bouncebuf;
+ u32 ndummy = 0;
int ret;
- ret = spi_nor_read_id(nor, 0, 0, id, nor->reg_proto);
+ if (!of_property_read_u32(nor->dev->of_node, "rdid-dummy-ncycles",
+ &ndummy))
+ ndummy /= BITS_PER_BYTE;
+
+ ret = spi_nor_read_id(nor, 0, ndummy, id, nor->reg_proto);
if (ret) {
dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
return ERR_PTR(ret);
--
2.34.1On Wed, Mar 19, 2025 at 06:47:44PM +0900, Takahiro Kuwano wrote: > There are infineon flashes [1] that require 8 dummy cycles for the > 1-1-1 Read ID command. Since the command is not covered by JESD216 > or any other standard, get the number of dummy cycles from DT and use > them to correctly identify the flash. If Read ID fails, then couldn't you just retry with dummy cycles? Or would unconditionally adding dummy cycles adversely affect other chips? Otherwise, add a specific compatible to imply this requirement. Adding quirk properties doesn't scale. Rob
Hi, Rob, On 3/19/25 11:30 PM, Rob Herring wrote: > On Wed, Mar 19, 2025 at 06:47:44PM +0900, Takahiro Kuwano wrote: >> There are infineon flashes [1] that require 8 dummy cycles for the >> 1-1-1 Read ID command. Since the command is not covered by JESD216 >> or any other standard, get the number of dummy cycles from DT and use >> them to correctly identify the flash. > > If Read ID fails, then couldn't you just retry with dummy cycles? Or I think Read ID won't fail when the op requires 8 dummy cycles, it probably just reads garbage on the first 8 cycles, so we risk to wrongly match other flash IDs. > would unconditionally adding dummy cycles adversely affect other chips? Adding 8 dummy cycles to chips that don't need it, would mean ignoring the first byte of the flash ID, thus we again risk to wrongly match against other flash IDs. > > Otherwise, add a specific compatible to imply this requirement. Adding > quirk properties doesn't scale. Do you mean a flash name compatible, like "cyrs17b512,spi-nor"? The problem that I see with that is that we no longer bind against the generic jedec,spi-nor compatible, so people need to update their DT in case they use/plug-in a different flash on their board. Thanks, ta
On Thu, Mar 20, 2025 at 2:44 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Hi, Rob, > > On 3/19/25 11:30 PM, Rob Herring wrote: > > On Wed, Mar 19, 2025 at 06:47:44PM +0900, Takahiro Kuwano wrote: > >> There are infineon flashes [1] that require 8 dummy cycles for the > >> 1-1-1 Read ID command. Since the command is not covered by JESD216 > >> or any other standard, get the number of dummy cycles from DT and use > >> them to correctly identify the flash. > > > > If Read ID fails, then couldn't you just retry with dummy cycles? Or > > I think Read ID won't fail when the op requires 8 dummy cycles, it > probably just reads garbage on the first 8 cycles, so we risk to wrongly > match other flash IDs. > > > would unconditionally adding dummy cycles adversely affect other chips? > > Adding 8 dummy cycles to chips that don't need it, would mean ignoring > the first byte of the flash ID, thus we again risk to wrongly match > against other flash IDs. > > > > > Otherwise, add a specific compatible to imply this requirement. Adding > > quirk properties doesn't scale. > > Do you mean a flash name compatible, like "cyrs17b512,spi-nor"? Yes, but that's not the format of compatible strings. > The > problem that I see with that is that we no longer bind against the > generic jedec,spi-nor compatible, so people need to update their DT in > case they use/plug-in a different flash on their board. This chip is clearly *not* compatible with a generic chip. You have the same problem with a property. Users have to add or remove the property if the flash changes. Anyone thinking they can use this chip as a compatible 2nd source is SOL. Rob
On 3/20/25 2:06 PM, Rob Herring wrote: > On Thu, Mar 20, 2025 at 2:44 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >> >> Hi, Rob, >> >> On 3/19/25 11:30 PM, Rob Herring wrote: >>> On Wed, Mar 19, 2025 at 06:47:44PM +0900, Takahiro Kuwano wrote: >>>> There are infineon flashes [1] that require 8 dummy cycles for the >>>> 1-1-1 Read ID command. Since the command is not covered by JESD216 >>>> or any other standard, get the number of dummy cycles from DT and use >>>> them to correctly identify the flash. >>> >>> If Read ID fails, then couldn't you just retry with dummy cycles? Or >> >> I think Read ID won't fail when the op requires 8 dummy cycles, it >> probably just reads garbage on the first 8 cycles, so we risk to wrongly >> match other flash IDs. >> >>> would unconditionally adding dummy cycles adversely affect other chips? >> >> Adding 8 dummy cycles to chips that don't need it, would mean ignoring >> the first byte of the flash ID, thus we again risk to wrongly match >> against other flash IDs. >> >>> >>> Otherwise, add a specific compatible to imply this requirement. Adding >>> quirk properties doesn't scale. >> >> Do you mean a flash name compatible, like "cyrs17b512,spi-nor"? > > Yes, but that's not the format of compatible strings. > >> The >> problem that I see with that is that we no longer bind against the >> generic jedec,spi-nor compatible, so people need to update their DT in >> case they use/plug-in a different flash on their board. > > This chip is clearly *not* compatible with a generic chip. I think it is compatible. The chip defines the SFDP (serial flash discoverable parameters) tables. At probe time we parse those tables and initialize the flash based on them. We don't even care about the chip ID, if all the flash parameters can be discovered via SFDP. Unfortunately these tables do not describe all the flash capabilities (block protection being one). Or worse, manufacturers mangle these tables. So vendors need to identify chips to either fix those tables via some quirks after the parsing is done, or to specify support that's not covered by those tables. For basic ops, flashes that get the SFDP tables right, don't even need a flash entry defined, we don't care about their ID, we just initialize the flash solely based on SFDP. In this particular case, this flash needs identification to fix some wrong SFDP field, it corrects just the mode cycles for the FAST READ command. All the other commands seem fine according to patch 3/3. > > You have the same problem with a property. Users have to add or remove True. It's the same problem. Even if we specify the dummy cycles via a property, the next plugged-in flash will use those. We can of course fallback to the SFDP only init if the ID doesn't match any flash entry, but the problem is the same. > the property if the flash changes. Anyone thinking they can use this > chip as a compatible 2nd source is SOL. > I think the property vs compatible decision resumes at whether we consider that the dummy cycles requirement for Read ID is/will be generic or not. I noticed that with higher frequencies or protocol modes (e.g, octal DTR), flashes tend to require more dummy cycles. I think with time, we'll have more flashes with such requirement. Takahiro can jump in and tell if it's already the case with IFX. Thus instead of having lots of new compatibles for this, I lean towards having this property. I'm still open for the compatible idea, I just wanted to explain better where we are. Thanks, ta
On 3/21/2025 12:45 AM, Tudor Ambarus wrote: > > > On 3/20/25 2:06 PM, Rob Herring wrote: >> On Thu, Mar 20, 2025 at 2:44 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >>> >>> Hi, Rob, >>> >>> On 3/19/25 11:30 PM, Rob Herring wrote: >>>> On Wed, Mar 19, 2025 at 06:47:44PM +0900, Takahiro Kuwano wrote: >>>>> There are infineon flashes [1] that require 8 dummy cycles for the >>>>> 1-1-1 Read ID command. Since the command is not covered by JESD216 >>>>> or any other standard, get the number of dummy cycles from DT and use >>>>> them to correctly identify the flash. >>>> >>>> If Read ID fails, then couldn't you just retry with dummy cycles? Or >>> >>> I think Read ID won't fail when the op requires 8 dummy cycles, it >>> probably just reads garbage on the first 8 cycles, so we risk to wrongly >>> match other flash IDs. >>> >>>> would unconditionally adding dummy cycles adversely affect other chips? >>> >>> Adding 8 dummy cycles to chips that don't need it, would mean ignoring >>> the first byte of the flash ID, thus we again risk to wrongly match >>> against other flash IDs. >>> >>>> >>>> Otherwise, add a specific compatible to imply this requirement. Adding >>>> quirk properties doesn't scale. >>> >>> Do you mean a flash name compatible, like "cyrs17b512,spi-nor"? >> >> Yes, but that's not the format of compatible strings. >> >>> The >>> problem that I see with that is that we no longer bind against the >>> generic jedec,spi-nor compatible, so people need to update their DT in >>> case they use/plug-in a different flash on their board. >> >> This chip is clearly *not* compatible with a generic chip. > > I think it is compatible. The chip defines the SFDP (serial flash > discoverable parameters) tables. At probe time we parse those tables and > initialize the flash based on them. > > We don't even care about the chip ID, if all the flash parameters can be > discovered via SFDP. Unfortunately these tables do not describe all the > flash capabilities (block protection being one). Or worse, manufacturers > mangle these tables. > > So vendors need to identify chips to either fix those tables via some > quirks after the parsing is done, or to specify support that's not > covered by those tables. > > For basic ops, flashes that get the SFDP tables right, don't even need a > flash entry defined, we don't care about their ID, we just initialize > the flash solely based on SFDP. > > In this particular case, this flash needs identification to fix some > wrong SFDP field, it corrects just the mode cycles for the FAST READ > command. All the other commands seem fine according to patch 3/3. > >> >> You have the same problem with a property. Users have to add or remove > > True. It's the same problem. Even if we specify the dummy cycles via a > property, the next plugged-in flash will use those. We can of course > fallback to the SFDP only init if the ID doesn't match any flash entry, > but the problem is the same. > >> the property if the flash changes. Anyone thinking they can use this >> chip as a compatible 2nd source is SOL. >> > > I think the property vs compatible decision resumes at whether we > consider that the dummy cycles requirement for Read ID is/will be > generic or not. > > I noticed that with higher frequencies or protocol modes (e.g, octal > DTR), flashes tend to require more dummy cycles. I think with time, > we'll have more flashes with such requirement. Takahiro can jump in and > tell if it's already the case with IFX. > Infineon SEMPER flash families (S25Hx-T/S28Hx-T) requires dummy cycles in Read ID, depending on Configuration Register setting. That is to support higher frequencies. By factory default dummy is 0 in 1S-1S-1S and it works up to 133MHz. Users can change the setting to support higher frequencies. > Thus instead of having lots of new compatibles for this, I lean towards > having this property. I'm still open for the compatible idea, I just > wanted to explain better where we are. > > Thanks, > ta >
Hi,
> >>>> There are infineon flashes [1] that require 8 dummy cycles for the
> >>>> 1-1-1 Read ID command. Since the command is not covered by JESD216
> >>>> or any other standard, get the number of dummy cycles from DT and use
> >>>> them to correctly identify the flash.
> >>>
> >>> If Read ID fails, then couldn't you just retry with dummy cycles? Or
> >>
> >> I think Read ID won't fail when the op requires 8 dummy cycles, it
> >> probably just reads garbage on the first 8 cycles, so we risk to wrongly
> >> match other flash IDs.
> >>
> >>> would unconditionally adding dummy cycles adversely affect other chips?
> >>
> >> Adding 8 dummy cycles to chips that don't need it, would mean ignoring
> >> the first byte of the flash ID, thus we again risk to wrongly match
> >> against other flash IDs.
> >>
> >>>
> >>> Otherwise, add a specific compatible to imply this requirement. Adding
> >>> quirk properties doesn't scale.
> >>
> >> Do you mean a flash name compatible, like "cyrs17b512,spi-nor"?
> >
> > Yes, but that's not the format of compatible strings.
> >
> >> The
> >> problem that I see with that is that we no longer bind against the
> >> generic jedec,spi-nor compatible, so people need to update their DT in
> >> case they use/plug-in a different flash on their board.
> >
> > This chip is clearly *not* compatible with a generic chip.
>
> I think it is compatible. The chip defines the SFDP (serial flash
> discoverable parameters) tables. At probe time we parse those tables and
> initialize the flash based on them.
I disagree. It's not compatible with "jedec,spi-nor", which is
defined as
SPI NOR flashes compatible with the JEDEC SFDP standard or which may be
identified with the READ ID opcode (0x9F) do not deserve a specific
compatible. They should instead only be matched against the generic
"jedec,spi-nor" compatible.
The first part was recently added and is a bit misleading. The old
definition was:
Must also include "jedec,spi-nor" for any SPI NOR flash that can be
identified by the JEDEC READ ID opcode (0x9F).
See my first reply, on how to possibly fix this mess (new
compatible if accepted, just use RDSFDP sequence which is backed by
the standard and do some fingerprinting).
FWIW, a new (or rather different) compatible is needed because we
cannot distinguish between random data returned during the dummy
cycles and a proper manufacturer id. So there is no way we could fix
this in the core itself.
At least if we keep the logic as it is for now, if we use RDSFDP to
fingerprint first and then fall back to RDID, we could get away with
the old compatible.
> We don't even care about the chip ID, if all the flash parameters can be
> discovered via SFDP. Unfortunately these tables do not describe all the
> flash capabilities (block protection being one). Or worse, manufacturers
> mangle these tables.
>
> So vendors need to identify chips to either fix those tables via some
> quirks after the parsing is done, or to specify support that's not
> covered by those tables.
>
> For basic ops, flashes that get the SFDP tables right, don't even need a
> flash entry defined, we don't care about their ID, we just initialize
> the flash solely based on SFDP.
>
> In this particular case, this flash needs identification to fix some
> wrong SFDP field, it corrects just the mode cycles for the FAST READ
> command. All the other commands seem fine according to patch 3/3.
>
> >
> > You have the same problem with a property. Users have to add or remove
>
> True. It's the same problem. Even if we specify the dummy cycles via a
> property, the next plugged-in flash will use those. We can of course
> fallback to the SFDP only init if the ID doesn't match any flash entry,
> but the problem is the same.
>
> > the property if the flash changes. Anyone thinking they can use this
> > chip as a compatible 2nd source is SOL.
> >
>
> I think the property vs compatible decision resumes at whether we
> consider that the dummy cycles requirement for Read ID is/will be
> generic or not.
It is not generic. Because it will break autodetection. And that is
the whole purpose of this. Adding that property means, we can just
autodetect flashes within this 'group'. And personally, I think this
is a bad precedent.
> I noticed that with higher frequencies or protocol modes (e.g, octal
> DTR), flashes tend to require more dummy cycles. I think with time,
> we'll have more flashes with such requirement. Takahiro can jump in and
> tell if it's already the case with IFX.
But hopefully not with RDID. Again this doesn't play nice with other
flashes (or all flashes for now). Instead of adding random delay
cycles one should rather define a max clock speed for this opcode.
-michael
> Thus instead of having lots of new compatibles for this, I lean towards
> having this property. I'm still open for the compatible idea, I just
> wanted to explain better where we are.
>
> Thanks,
> ta
Hi, Michael, Sorry, I somehow missed your replies. On 3/21/25 8:00 AM, Michael Walle wrote: cut >>>> The >>>> problem that I see with that is that we no longer bind against the >>>> generic jedec,spi-nor compatible, so people need to update their DT in >>>> case they use/plug-in a different flash on their board. >>> >>> This chip is clearly *not* compatible with a generic chip. >> >> I think it is compatible. The chip defines the SFDP (serial flash >> discoverable parameters) tables. At probe time we parse those tables and >> initialize the flash based on them. > > I disagree. It's not compatible with "jedec,spi-nor", which is > defined as > cut > > See my first reply, on how to possibly fix this mess (new > compatible if accepted, just use RDSFDP sequence which is backed by > the standard and do some fingerprinting). > this won't work unless there's a unique parameter or ID in the sfdp or vendors tables, which I doubt. Takahiro to confirm. > FWIW, a new (or rather different) compatible is needed because we > cannot distinguish between random data returned during the dummy > cycles and a proper manufacturer id. So there is no way we could fix > this in the core itself. Yes, I agree, new compatible it is then. cut >> I think the property vs compatible decision resumes at whether we >> consider that the dummy cycles requirement for Read ID is/will be >> generic or not. > > It is not generic. Because it will break autodetection. And that is > the whole purpose of this. Adding that property means, we can just > autodetect flashes within this 'group'. And personally, I think this > is a bad precedent. > yes, I agree. >> I noticed that with higher frequencies or protocol modes (e.g, octal >> DTR), flashes tend to require more dummy cycles. I think with time, >> we'll have more flashes with such requirement. Takahiro can jump in and >> tell if it's already the case with IFX. > > But hopefully not with RDID. Again this doesn't play nice with other > flashes (or all flashes for now). Instead of adding random delay > cycles one should rather define a max clock speed for this opcode. This could work, yes. But not for this flash. Or maybe encourage vendors to either contribute and enlarge the SFDP database or define their own vendor tables for all the flash properties that are not covered yet. It's strange how Block Protection is not yet covered by SFDP after all these years. Thanks, ta
On 3/26/2025 11:44 PM, Tudor Ambarus wrote: > Hi, Michael, > > Sorry, I somehow missed your replies. > > On 3/21/25 8:00 AM, Michael Walle wrote: > > cut > >>>>> The >>>>> problem that I see with that is that we no longer bind against the >>>>> generic jedec,spi-nor compatible, so people need to update their DT in >>>>> case they use/plug-in a different flash on their board. >>>> >>>> This chip is clearly *not* compatible with a generic chip. >>> >>> I think it is compatible. The chip defines the SFDP (serial flash >>> discoverable parameters) tables. At probe time we parse those tables and >>> initialize the flash based on them. >> >> I disagree. It's not compatible with "jedec,spi-nor", which is >> defined as >> > > cut > >> >> See my first reply, on how to possibly fix this mess (new >> compatible if accepted, just use RDSFDP sequence which is backed by >> the standard and do some fingerprinting). >> > > this won't work unless there's a unique parameter or ID in the sfdp or > vendors tables, which I doubt. Takahiro to confirm. > No, cyrs17b doesn't have it. >> FWIW, a new (or rather different) compatible is needed because we >> cannot distinguish between random data returned during the dummy >> cycles and a proper manufacturer id. So there is no way we could fix >> this in the core itself. > > Yes, I agree, new compatible it is then. > > cut > >>> I think the property vs compatible decision resumes at whether we >>> consider that the dummy cycles requirement for Read ID is/will be >>> generic or not. >> >> It is not generic. Because it will break autodetection. And that is >> the whole purpose of this. Adding that property means, we can just >> autodetect flashes within this 'group'. And personally, I think this >> is a bad precedent. >> > > yes, I agree. > >>> I noticed that with higher frequencies or protocol modes (e.g, octal >>> DTR), flashes tend to require more dummy cycles. I think with time, >>> we'll have more flashes with such requirement. Takahiro can jump in and >>> tell if it's already the case with IFX. >> >> But hopefully not with RDID. Again this doesn't play nice with other >> flashes (or all flashes for now). Instead of adding random delay >> cycles one should rather define a max clock speed for this opcode. > > This could work, yes. But not for this flash. Or maybe encourage vendors > to either contribute and enlarge the SFDP database or define their own > vendor tables for all the flash properties that are not covered yet. > It's strange how Block Protection is not yet covered by SFDP after all > these years. > > Thanks, > ta
On Thu Mar 20, 2025 at 12:30 AM CET, Rob Herring wrote: > On Wed, Mar 19, 2025 at 06:47:44PM +0900, Takahiro Kuwano wrote: > > There are infineon flashes [1] that require 8 dummy cycles for the > > 1-1-1 Read ID command. Since the command is not covered by JESD216 > > or any other standard, get the number of dummy cycles from DT and use > > them to correctly identify the flash. > > If Read ID fails, then couldn't you just retry with dummy cycles? Or > would unconditionally adding dummy cycles adversely affect other chips? Yes exactly. IIUC, the first byte of the auto discovery (RDID command) would return random data, because the output driver of the flash is disabled. The second byte would then return the manufacturer id and third and fourth the device id. This makes auto discovery fundamentally broken with this chip as the first byte might randomly match any manufacturer within our database. Takahiro, if you can reach designer of this chip, you might tell them, that this was not the greatest idea :o > Otherwise, add a specific compatible to imply this requirement. Adding > quirk properties doesn't scale. Since auto discovery doesn't work at all, you might just add, "infineon,cyrs17b512" *instead of* "jedec,spi-nor" (because it doesn't support the usual RDID command"). Alternatively, we could introduce a "generic" compatible which just uses the standard RDSFDP command instead of RDID for discovery. Rob? Then we'd need some SFDP fingerprinting mechanism to match the SFDP to a particular flash type. -michael
© 2016 - 2025 Red Hat, Inc.