| Message ID | 20230908064304.27757-2-jaimeliao.tw@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | Add octal DTR support for Macronix flash | expand |
Hi, Jaime, Thanks for the patch! I think we are getting closer to have this integrated, but I have some concerns that hopefully with your help we'll address and move forward. On 08.09.2023 09:42, Jaime Liao wrote: > From: JaimeLiao <jaimeliao@mxic.com.tw> > > Add manufacturer read id function because of some flash > may change data format when read id in octal dtr mode. I'm not convinced such a method is really needed, would you please elaborate the explanation why it's needed? I'm looking at the mx66lm1g45g datasheet. From what I see in "Figure 13. Read Identification (RDID) Sequence (DTR-OPI Mode)" looks like even if the flash is operated in 8d-8d-8d mode, what the flash actually uses is a 8d-8d-8s mode for the read id. Each ID byte is sent twice on both rising and falling edge of the clock, thus behaving like a 8d-8d-8s protocol. I see this flash supports 1-1-1, 8-8-8, and 8d-8d-8d, there are no mixed modes supported, thus a 8d-8d-8s mode seems just like a hardware bug to me. So my proposal is to leave the core away and to handle the read id hack just in the macronix driver. > > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw> > --- > drivers/mtd/spi-nor/core.c | 30 ++++++++++++++++++++++++++++-- > drivers/mtd/spi-nor/core.h | 6 ++++++ > drivers/mtd/spi-nor/macronix.c | 18 ++++++++++++++++++ > 3 files changed, 52 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 1b0c6770c14e..7ee624b16e17 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -408,7 +408,7 @@ int spi_nor_write_disable(struct spi_nor *nor) > } > > /** > - * spi_nor_read_id() - Read the JEDEC ID. > + * spi_nor_default_read_id() - Read the JEDEC ID. > * @nor: pointer to 'struct spi_nor'. > * @naddr: number of address bytes to send. Can be zero if the operation > * does not need to send an address. > @@ -420,7 +420,7 @@ int spi_nor_write_disable(struct spi_nor *nor) > * > * Return: 0 on success, -errno otherwise. > */ > -int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, > +int spi_nor_default_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, > enum spi_nor_protocol proto) > { > int ret; > @@ -438,6 +438,32 @@ int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, > return ret; > } > > +/** > + * spi_nor_read_id() - Read ID by manufacturer read id function. > + * @nor: pointer to 'struct spi_nor'. > + * @naddr: number of address bytes to send. Can be zero if the operation > + * does not need to send an address. > + * @ndummy: number of dummy bytes to send after an opcode or address. Can > + * be zero if the operation does not require dummy bytes. > + * @id: pointer to a DMA-able buffer where the value of the JEDEC ID > + * will be written. > + * @proto: the SPI protocol for register operation. > + * > + * Return: 0 on success, -errno otherwise. > + */ > +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, > + enum spi_nor_protocol proto) > +{ > + int ret; > + > + if (nor->manufacturer && nor->manufacturer->fixups && nor->manufacturer->fixups->read_id) > + ret = nor->manufacturer->fixups->read_id(nor, naddr, ndummy, id, proto); > + else > + ret = spi_nor_default_read_id(nor, naddr, ndummy, id, proto); > + > + return ret; > +} > + > /** > * spi_nor_read_sr() - Read the Status Register. > * @nor: pointer to 'struct spi_nor'. > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > index 9217379b9cfe..92cbc2d3f7fe 100644 > --- a/drivers/mtd/spi-nor/core.h > +++ b/drivers/mtd/spi-nor/core.h > @@ -424,6 +424,8 @@ struct spi_nor_flash_parameter { > * @late_init: used to initialize flash parameters that are not declared in the > * JESD216 SFDP standard, or where SFDP tables not defined at all. > * Will replace the default_init() hook. > + * @read_id: used to read id which may change format after enter into > + octal dtr mode. > * > * Those hooks can be used to tweak the SPI NOR configuration when the SFDP > * table is broken or not available. > @@ -435,6 +437,8 @@ struct spi_nor_fixups { > const struct sfdp_bfpt *bfpt); > int (*post_sfdp)(struct spi_nor *nor); > int (*late_init)(struct spi_nor *nor); > + int (*read_id)(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, > + enum spi_nor_protocol reg_proto); > }; > > /** > @@ -667,6 +671,8 @@ void spi_nor_unlock_and_unprep(struct spi_nor *nor); > int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor); > int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor); > int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor); > +int spi_nor_default_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, > + enum spi_nor_protocol reg_proto); > int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, > enum spi_nor_protocol reg_proto); > int spi_nor_read_sr(struct spi_nor *nor, u8 *sr); > diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c > index eb149e517c1f..8ab47691dfbb 100644 > --- a/drivers/mtd/spi-nor/macronix.c > +++ b/drivers/mtd/spi-nor/macronix.c > @@ -118,9 +118,27 @@ static int macronix_nor_late_init(struct spi_nor *nor) > return 0; > } > > +static int macronix_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, > + enum spi_nor_protocol proto) > +{ > + int i, ret; > + > + ret = spi_nor_default_read_id(nor, naddr, ndummy, id, proto); > + /* Retrieve odd array and re-sort id because of read id format will be A-A-B-B-C-C > + * after enter into octal dtr mode for Macronix flashes. > + */ > + if (proto == SNOR_PROTO_8_8_8_DTR) { > + for (i = 0; i < nor->info->id_len; i++) > + id[i] = id[i*2]; why do you overwrite the id? How about just checking that id[i] == id[i + 1]? why do you care if you print an a-a-b-b-c-c id? > + } > + > + return ret; > +} > + > static const struct spi_nor_fixups macronix_nor_fixups = { > .default_init = macronix_nor_default_init, > .late_init = macronix_nor_late_init, > + .read_id = macronix_nor_read_id, > }; > > const struct spi_nor_manufacturer spi_nor_macronix = {
On Wed, Sep 20 2023, Tudor Ambarus wrote: > Hi, Jaime, > > Thanks for the patch! I think we are getting closer to have this > integrated, but I have some concerns that hopefully with your help we'll > address and move forward. > > On 08.09.2023 09:42, Jaime Liao wrote: >> From: JaimeLiao <jaimeliao@mxic.com.tw> >> >> Add manufacturer read id function because of some flash >> may change data format when read id in octal dtr mode. > > I'm not convinced such a method is really needed, would you please > elaborate the explanation why it's needed? > > I'm looking at the mx66lm1g45g datasheet. From what I see in "Figure 13. > Read Identification (RDID) Sequence (DTR-OPI Mode)" looks like even if > the flash is operated in 8d-8d-8d mode, what the flash actually uses is > a 8d-8d-8s mode for the read id. Each ID byte is sent twice on both > rising and falling edge of the clock, thus behaving like a 8d-8d-8s > protocol. > > I see this flash supports 1-1-1, 8-8-8, and 8d-8d-8d, there are no mixed > modes supported, thus a 8d-8d-8s mode seems just like a hardware bug to > me. So my proposal is to leave the core away and to handle the read id > hack just in the macronix driver. +1 [...] >> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c >> index eb149e517c1f..8ab47691dfbb 100644 >> --- a/drivers/mtd/spi-nor/macronix.c >> +++ b/drivers/mtd/spi-nor/macronix.c >> @@ -118,9 +118,27 @@ static int macronix_nor_late_init(struct spi_nor *nor) >> return 0; >> } >> >> +static int macronix_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, >> + enum spi_nor_protocol proto) >> +{ >> + int i, ret; >> + >> + ret = spi_nor_default_read_id(nor, naddr, ndummy, id, proto); >> + /* Retrieve odd array and re-sort id because of read id format will be A-A-B-B-C-C >> + * after enter into octal dtr mode for Macronix flashes. >> + */ >> + if (proto == SNOR_PROTO_8_8_8_DTR) { >> + for (i = 0; i < nor->info->id_len; i++) >> + id[i] = id[i*2]; > > why do you overwrite the id? How about just checking that > id[i] == id[i + 1]? why do you care if you print an a-a-b-b-c-c id? Because id_len == 3 so you should only treat the ID as a-a-b. When you memcmp() the ID after reading it, you would not get a match. >> + } >> + >> + return ret; >> +} >> + >> static const struct spi_nor_fixups macronix_nor_fixups = { >> .default_init = macronix_nor_default_init, >> .late_init = macronix_nor_late_init, >> + .read_id = macronix_nor_read_id, >> }; >> >> const struct spi_nor_manufacturer spi_nor_macronix = {
Hi, >> I see this flash supports 1-1-1, 8-8-8, and 8d-8d-8d, there are no >> mixed >> modes supported, thus a 8d-8d-8s mode seems just like a hardware bug >> to >> me. So my proposal is to leave the core away and to handle the read id >> hack just in the macronix driver. > > +1 I've looked at the xspi spec. There is no RDID specified. So I'd argue, the only pseudo standard is that RDID was only ever used with 1s1s1s. But we added spi_nor_read_id() with parameters suited for the "unusual" 8d8d8d case with additional address and dummy cycles. Just for checking whether the octal-dtr switch was successful. Therefore, we've already added parameters to spi_nor_read_id() which are not standard. Then we can just add one more. It's just how macronix is doing it. Again there is no standard. If we'd only put standard (or for the 9F pseudo standard) things in the core, then spi_nor_read_id() would need to check whether the flash is in 1s1s1s mode. And no I wouldn't prefer that ;) -michael
On 10/5/23 12:43, Michael Walle wrote: > Hi, > hey >>> I see this flash supports 1-1-1, 8-8-8, and 8d-8d-8d, there are no mixed >>> modes supported, thus a 8d-8d-8s mode seems just like a hardware bug to >>> me. So my proposal is to leave the core away and to handle the read id >>> hack just in the macronix driver. >> >> +1 > > I've looked at the xspi spec. There is no RDID specified. So I'd argue, > the only pseudo standard is that RDID was only ever used with 1s1s1s. But > we added spi_nor_read_id() with parameters suited for the "unusual" 8d8d8d > case with additional address and dummy cycles. Just for checking whether > the > octal-dtr switch was successful. Therefore, we've already added > parameters to > spi_nor_read_id() which are not standard. Then we can just add one more. > It's > just how macronix is doing it. Again there is no standard. > If we'd only put standard (or for the 9F pseudo standard) things in the > core, > then spi_nor_read_id() would need to check whether the flash is in 1s1s1s > mode. And no I wouldn't prefer that ;) > If we really want to be catholic, we should switch to 8d-8d-8s mode and then issue the read id and the core will handle the readid correctly. But there is no such a thing, because macronix considers that it is in 8d-8d-8d mode at the time of issuing 8d-8d-8s read id. That's why I say it's a bug on their side. The core is meant to be generic, I don't want to pollute the core with manufacturer specific fixes. cheers, ta
Am 2023-10-05 15:19, schrieb Tudor Ambarus: > On 10/5/23 12:43, Michael Walle wrote: >> Hi, >> > hey > >>>> I see this flash supports 1-1-1, 8-8-8, and 8d-8d-8d, there are no >>>> mixed >>>> modes supported, thus a 8d-8d-8s mode seems just like a hardware bug >>>> to >>>> me. So my proposal is to leave the core away and to handle the read >>>> id >>>> hack just in the macronix driver. >>> >>> +1 >> >> I've looked at the xspi spec. There is no RDID specified. So I'd >> argue, >> the only pseudo standard is that RDID was only ever used with 1s1s1s. >> But >> we added spi_nor_read_id() with parameters suited for the "unusual" >> 8d8d8d >> case with additional address and dummy cycles. Just for checking >> whether >> the >> octal-dtr switch was successful. Therefore, we've already added >> parameters to >> spi_nor_read_id() which are not standard. Then we can just add one >> more. >> It's >> just how macronix is doing it. Again there is no standard. >> If we'd only put standard (or for the 9F pseudo standard) things in >> the >> core, >> then spi_nor_read_id() would need to check whether the flash is in >> 1s1s1s >> mode. And no I wouldn't prefer that ;) >> > > If we really want to be catholic, we should switch to 8d-8d-8s mode and > then issue the read id and the core will handle the readid correctly. > But there is no such a thing, because macronix considers that it is in > 8d-8d-8d mode at the time of issuing 8d-8d-8s read id. That's why I say > it's a bug on their side. The core is meant to be generic, I don't want > to pollute the core with manufacturer specific fixes. Then why does spi_nor_read_id() have the following parameters: * @naddr: number of address bytes to send. Can be zero if the operation * does not need to send an address. * @ndummy: number of dummy bytes to send after an opcode or address. Can * be zero if the operation does not require dummy bytes. * @proto: the SPI protocol for register operation. They aren't standard either. It's just the way spansion and micron transfers the ID via RDID in octal DTR mode. And now there's macronix who does it slightly different. But *neither* of them are standard. Why should one be in the core and one shouldn't. spi_nor_read_id() should just handle proto == SNOR_PROTO_8D_8D_8S. -michael
On 10/5/23 14:27, Michael Walle wrote: > Am 2023-10-05 15:19, schrieb Tudor Ambarus: >> On 10/5/23 12:43, Michael Walle wrote: >>> Hi, >>> >> hey >> >>>>> I see this flash supports 1-1-1, 8-8-8, and 8d-8d-8d, there are no >>>>> mixed >>>>> modes supported, thus a 8d-8d-8s mode seems just like a hardware >>>>> bug to >>>>> me. So my proposal is to leave the core away and to handle the read id >>>>> hack just in the macronix driver. >>>> >>>> +1 >>> >>> I've looked at the xspi spec. There is no RDID specified. So I'd argue, >>> the only pseudo standard is that RDID was only ever used with 1s1s1s. >>> But >>> we added spi_nor_read_id() with parameters suited for the "unusual" >>> 8d8d8d >>> case with additional address and dummy cycles. Just for checking whether >>> the >>> octal-dtr switch was successful. Therefore, we've already added >>> parameters to >>> spi_nor_read_id() which are not standard. Then we can just add one more. >>> It's >>> just how macronix is doing it. Again there is no standard. >>> If we'd only put standard (or for the 9F pseudo standard) things in the >>> core, >>> then spi_nor_read_id() would need to check whether the flash is in >>> 1s1s1s >>> mode. And no I wouldn't prefer that ;) >>> >> >> If we really want to be catholic, we should switch to 8d-8d-8s mode and >> then issue the read id and the core will handle the readid correctly. >> But there is no such a thing, because macronix considers that it is in >> 8d-8d-8d mode at the time of issuing 8d-8d-8s read id. That's why I say >> it's a bug on their side. The core is meant to be generic, I don't want >> to pollute the core with manufacturer specific fixes. > > Then why does spi_nor_read_id() have the following parameters: > > * @naddr: number of address bytes to send. Can be zero if the > operation > * does not need to send an address. > * @ndummy: number of dummy bytes to send after an opcode or > address. Can > * be zero if the operation does not require dummy bytes. > * @proto: the SPI protocol for register operation. > > They aren't standard either. It's just the way spansion and micron > transfers > the ID via RDID in octal DTR mode. And now there's macronix who does it > slightly > different. But *neither* of them are standard. Why should one be in the > core and > one shouldn't. > > spi_nor_read_id() should just handle proto == SNOR_PROTO_8D_8D_8S. Yes, it should, but mx is in 8d-8d-8d at the time it issues the read id, thus it passes the core proto == SNOR_PROTO_8D_8D_8D, isn't it? If you care about this, please send a patch addressing it, it's better to talk on code. I don't see yet how you will handle it, but I'm open to review some code.
Hi, >>>>>> I see this flash supports 1-1-1, 8-8-8, and 8d-8d-8d, there are no >>>>>> mixed >>>>>> modes supported, thus a 8d-8d-8s mode seems just like a hardware >>>>>> bug to >>>>>> me. So my proposal is to leave the core away and to handle the >>>>>> read id >>>>>> hack just in the macronix driver. >>>>> >>>>> +1 >>>> >>>> I've looked at the xspi spec. There is no RDID specified. So I'd >>>> argue, >>>> the only pseudo standard is that RDID was only ever used with >>>> 1s1s1s. >>>> But >>>> we added spi_nor_read_id() with parameters suited for the "unusual" >>>> 8d8d8d >>>> case with additional address and dummy cycles. Just for checking >>>> whether >>>> the >>>> octal-dtr switch was successful. Therefore, we've already added >>>> parameters to >>>> spi_nor_read_id() which are not standard. Then we can just add one >>>> more. >>>> It's >>>> just how macronix is doing it. Again there is no standard. >>>> If we'd only put standard (or for the 9F pseudo standard) things in >>>> the >>>> core, >>>> then spi_nor_read_id() would need to check whether the flash is in >>>> 1s1s1s >>>> mode. And no I wouldn't prefer that ;) >>>> >>> >>> If we really want to be catholic, we should switch to 8d-8d-8s mode >>> and >>> then issue the read id and the core will handle the readid correctly. >>> But there is no such a thing, because macronix considers that it is >>> in >>> 8d-8d-8d mode at the time of issuing 8d-8d-8s read id. That's why I >>> say >>> it's a bug on their side. The core is meant to be generic, I don't >>> want >>> to pollute the core with manufacturer specific fixes. >> >> Then why does spi_nor_read_id() have the following parameters: >> >> * @naddr: number of address bytes to send. Can be zero if the >> operation >> * does not need to send an address. >> * @ndummy: number of dummy bytes to send after an opcode or >> address. Can >> * be zero if the operation does not require dummy bytes. >> * @proto: the SPI protocol for register operation. >> >> They aren't standard either. It's just the way spansion and micron >> transfers >> the ID via RDID in octal DTR mode. And now there's macronix who does >> it >> slightly >> different. But *neither* of them are standard. Why should one be in >> the >> core and >> one shouldn't. >> >> spi_nor_read_id() should just handle proto == SNOR_PROTO_8D_8D_8S. > > Yes, it should, but mx is in 8d-8d-8d at the time it issues the read > id, > thus it passes the core proto == SNOR_PROTO_8D_8D_8D, isn't it? The flash device is in octal dtr mode, which means it will expect the opcode in octal dtr mode. But the data phase mode depends on the opcode (and theoretically, the address phase, too). If you use the (non-standard) RDID on this flash command has to be executed as 8d8d8s. > If you > care about this, please send a patch addressing it, it's better to talk > on code. I don't see yet how you will handle it, but I'm open to review > some code. I really don't have time right now. But something along: spi_nor_read_id(.., proto) { bool emulated_8d8d8s; op = assemble_op(.., proto); /* Emulate 8d8d8s if the controller doesn't support it */ if (!spi_mem_supports_op(op) && proto == 8d8d8s) { op = assemble_op(.., 8d8d8d); emulated_8d8d8s = true; } execute_op() if (emulated_8d8d8s) { /* discard every other byte of the response */ } } Later (or even now), that could be moved down the callstack into the spimem core. -michael
Hi, > > Hi, > > >>>>>> I see this flash supports 1-1-1, 8-8-8, and 8d-8d-8d, there are no > >>>>>> mixed > >>>>>> modes supported, thus a 8d-8d-8s mode seems just like a hardware > >>>>>> bug to > >>>>>> me. So my proposal is to leave the core away and to handle the > >>>>>> read id > >>>>>> hack just in the macronix driver. > >>>>> > >>>>> +1 > >>>> > >>>> I've looked at the xspi spec. There is no RDID specified. So I'd > >>>> argue, > >>>> the only pseudo standard is that RDID was only ever used with > >>>> 1s1s1s. > >>>> But > >>>> we added spi_nor_read_id() with parameters suited for the "unusual" > >>>> 8d8d8d > >>>> case with additional address and dummy cycles. Just for checking > >>>> whether > >>>> the > >>>> octal-dtr switch was successful. Therefore, we've already added > >>>> parameters to > >>>> spi_nor_read_id() which are not standard. Then we can just add one > >>>> more. > >>>> It's > >>>> just how macronix is doing it. Again there is no standard. > >>>> If we'd only put standard (or for the 9F pseudo standard) things in > >>>> the > >>>> core, > >>>> then spi_nor_read_id() would need to check whether the flash is in > >>>> 1s1s1s > >>>> mode. And no I wouldn't prefer that ;) > >>>> > >>> > >>> If we really want to be catholic, we should switch to 8d-8d-8s mode > >>> and > >>> then issue the read id and the core will handle the readid correctly. > >>> But there is no such a thing, because macronix considers that it is > >>> in > >>> 8d-8d-8d mode at the time of issuing 8d-8d-8s read id. That's why I > >>> say > >>> it's a bug on their side. The core is meant to be generic, I don't > >>> want > >>> to pollute the core with manufacturer specific fixes. > >> > >> Then why does spi_nor_read_id() have the following parameters: > >> > >> * @naddr: number of address bytes to send. Can be zero if the > >> operation > >> * does not need to send an address. > >> * @ndummy: number of dummy bytes to send after an opcode or > >> address. Can > >> * be zero if the operation does not require dummy bytes. > >> * @proto: the SPI protocol for register operation. > >> > >> They aren't standard either. It's just the way spansion and micron > >> transfers > >> the ID via RDID in octal DTR mode. And now there's macronix who does > >> it > >> slightly > >> different. But *neither* of them are standard. Why should one be in > >> the > >> core and > >> one shouldn't. > >> > >> spi_nor_read_id() should just handle proto == SNOR_PROTO_8D_8D_8S. > > > > Yes, it should, but mx is in 8d-8d-8d at the time it issues the read > > id, > > thus it passes the core proto == SNOR_PROTO_8D_8D_8D, isn't it? > > The flash device is in octal dtr mode, which means it will expect the > opcode in octal dtr mode. But the data phase mode depends on the opcode > (and theoretically, the address phase, too). If you use the > (non-standard) RDID on this flash command has to be executed as 8d8d8s. > > > If you > > care about this, please send a patch addressing it, it's better to talk > > on code. I don't see yet how you will handle it, but I'm open to review > > some code. > > I really don't have time right now. But something along: > > spi_nor_read_id(.., proto) { > bool emulated_8d8d8s; > > op = assemble_op(.., proto); > > /* Emulate 8d8d8s if the controller doesn't support it */ > if (!spi_mem_supports_op(op) && proto == 8d8d8s) { > op = assemble_op(.., 8d8d8d); > emulated_8d8d8s = true; > } > execute_op() > if (emulated_8d8d8s) { > /* discard every other byte of the response */ > } > } 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. Flash transfer data while DQS raising and falling edge exactlly. It is not a hardware bug or 8D-8D-8S. So that I think we only need a method for specific comparison after read id in 8d-8d-8d. > > Later (or even now), that could be moved down the callstack into > the spimem core. > > -michael Thanks Jaime
Hi, >> >>>>>> I see this flash supports 1-1-1, 8-8-8, and 8d-8d-8d, there are no >> >>>>>> mixed >> >>>>>> modes supported, thus a 8d-8d-8s mode seems just like a hardware >> >>>>>> bug to >> >>>>>> me. So my proposal is to leave the core away and to handle the >> >>>>>> read id >> >>>>>> hack just in the macronix driver. >> >>>>> >> >>>>> +1 >> >>>> >> >>>> I've looked at the xspi spec. There is no RDID specified. So I'd >> >>>> argue, >> >>>> the only pseudo standard is that RDID was only ever used with >> >>>> 1s1s1s. >> >>>> But >> >>>> we added spi_nor_read_id() with parameters suited for the "unusual" >> >>>> 8d8d8d >> >>>> case with additional address and dummy cycles. Just for checking >> >>>> whether >> >>>> the >> >>>> octal-dtr switch was successful. Therefore, we've already added >> >>>> parameters to >> >>>> spi_nor_read_id() which are not standard. Then we can just add one >> >>>> more. >> >>>> It's >> >>>> just how macronix is doing it. Again there is no standard. >> >>>> If we'd only put standard (or for the 9F pseudo standard) things in >> >>>> the >> >>>> core, >> >>>> then spi_nor_read_id() would need to check whether the flash is in >> >>>> 1s1s1s >> >>>> mode. And no I wouldn't prefer that ;) >> >>>> >> >>> >> >>> If we really want to be catholic, we should switch to 8d-8d-8s mode >> >>> and >> >>> then issue the read id and the core will handle the readid correctly. >> >>> But there is no such a thing, because macronix considers that it is >> >>> in >> >>> 8d-8d-8d mode at the time of issuing 8d-8d-8s read id. That's why I >> >>> say >> >>> it's a bug on their side. The core is meant to be generic, I don't >> >>> want >> >>> to pollute the core with manufacturer specific fixes. >> >> >> >> Then why does spi_nor_read_id() have the following parameters: >> >> >> >> * @naddr: number of address bytes to send. Can be zero if the >> >> operation >> >> * does not need to send an address. >> >> * @ndummy: number of dummy bytes to send after an opcode or >> >> address. Can >> >> * be zero if the operation does not require dummy bytes. >> >> * @proto: the SPI protocol for register operation. >> >> >> >> They aren't standard either. It's just the way spansion and micron >> >> transfers >> >> the ID via RDID in octal DTR mode. And now there's macronix who does >> >> it >> >> slightly >> >> different. But *neither* of them are standard. Why should one be in >> >> the >> >> core and >> >> one shouldn't. >> >> >> >> spi_nor_read_id() should just handle proto == SNOR_PROTO_8D_8D_8S. >> > >> > Yes, it should, but mx is in 8d-8d-8d at the time it issues the read >> > id, >> > thus it passes the core proto == SNOR_PROTO_8D_8D_8D, isn't it? >> >> The flash device is in octal dtr mode, which means it will expect the >> opcode in octal dtr mode. But the data phase mode depends on the >> opcode >> (and theoretically, the address phase, too). If you use the >> (non-standard) RDID on this flash command has to be executed as >> 8d8d8s. >> >> > If you >> > care about this, please send a patch addressing it, it's better to talk >> > on code. I don't see yet how you will handle it, but I'm open to review >> > some code. >> >> I really don't have time right now. But something along: >> >> spi_nor_read_id(.., proto) { >> bool emulated_8d8d8s; >> >> op = assemble_op(.., proto); >> >> /* Emulate 8d8d8s if the controller doesn't support it */ >> if (!spi_mem_supports_op(op) && proto == 8d8d8s) { >> op = assemble_op(.., 8d8d8d); >> emulated_8d8d8s = true; >> } >> execute_op() >> if (emulated_8d8d8s) { >> /* discard every other byte of the response */ >> } >> } > 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. -michael
Hi Michael > > Hi, > > >> >>>>>> I see this flash supports 1-1-1, 8-8-8, and 8d-8d-8d, there are no > >> >>>>>> mixed > >> >>>>>> modes supported, thus a 8d-8d-8s mode seems just like a hardware > >> >>>>>> bug to > >> >>>>>> me. So my proposal is to leave the core away and to handle the > >> >>>>>> read id > >> >>>>>> hack just in the macronix driver. > >> >>>>> > >> >>>>> +1 > >> >>>> > >> >>>> I've looked at the xspi spec. There is no RDID specified. So I'd > >> >>>> argue, > >> >>>> the only pseudo standard is that RDID was only ever used with > >> >>>> 1s1s1s. > >> >>>> But > >> >>>> we added spi_nor_read_id() with parameters suited for the "unusual" > >> >>>> 8d8d8d > >> >>>> case with additional address and dummy cycles. Just for checking > >> >>>> whether > >> >>>> the > >> >>>> octal-dtr switch was successful. Therefore, we've already added > >> >>>> parameters to > >> >>>> spi_nor_read_id() which are not standard. Then we can just add one > >> >>>> more. > >> >>>> It's > >> >>>> just how macronix is doing it. Again there is no standard. > >> >>>> If we'd only put standard (or for the 9F pseudo standard) things in > >> >>>> the > >> >>>> core, > >> >>>> then spi_nor_read_id() would need to check whether the flash is in > >> >>>> 1s1s1s > >> >>>> mode. And no I wouldn't prefer that ;) > >> >>>> > >> >>> > >> >>> If we really want to be catholic, we should switch to 8d-8d-8s mode > >> >>> and > >> >>> then issue the read id and the core will handle the readid correctly. > >> >>> But there is no such a thing, because macronix considers that it is > >> >>> in > >> >>> 8d-8d-8d mode at the time of issuing 8d-8d-8s read id. That's why I > >> >>> say > >> >>> it's a bug on their side. The core is meant to be generic, I don't > >> >>> want > >> >>> to pollute the core with manufacturer specific fixes. > >> >> > >> >> Then why does spi_nor_read_id() have the following parameters: > >> >> > >> >> * @naddr: number of address bytes to send. Can be zero if the > >> >> operation > >> >> * does not need to send an address. > >> >> * @ndummy: number of dummy bytes to send after an opcode or > >> >> address. Can > >> >> * be zero if the operation does not require dummy bytes. > >> >> * @proto: the SPI protocol for register operation. > >> >> > >> >> They aren't standard either. It's just the way spansion and micron > >> >> transfers > >> >> the ID via RDID in octal DTR mode. And now there's macronix who does > >> >> it > >> >> slightly > >> >> different. But *neither* of them are standard. Why should one be in > >> >> the > >> >> core and > >> >> one shouldn't. > >> >> > >> >> spi_nor_read_id() should just handle proto == SNOR_PROTO_8D_8D_8S. > >> > > >> > Yes, it should, but mx is in 8d-8d-8d at the time it issues the read > >> > id, > >> > thus it passes the core proto == SNOR_PROTO_8D_8D_8D, isn't it? > >> > >> The flash device is in octal dtr mode, which means it will expect the > >> opcode in octal dtr mode. But the data phase mode depends on the > >> opcode > >> (and theoretically, the address phase, too). If you use the > >> (non-standard) RDID on this flash command has to be executed as > >> 8d8d8s. > >> > >> > If you > >> > care about this, please send a patch addressing it, it's better to talk > >> > on code. I don't see yet how you will handle it, but I'm open to review > >> > some code. > >> > >> I really don't have time right now. But something along: > >> > >> spi_nor_read_id(.., proto) { > >> bool emulated_8d8d8s; > >> > >> op = assemble_op(.., proto); > >> > >> /* Emulate 8d8d8s if the controller doesn't support it */ > >> if (!spi_mem_supports_op(op) && proto == 8d8d8s) { > >> op = assemble_op(.., 8d8d8d); > >> emulated_8d8d8s = true; > >> } > >> execute_op() > >> if (emulated_8d8d8s) { > >> /* discard every other byte of the response */ > >> } > >> } > > 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 Most of vendor a, b and c are manufacturer ID, memory type and memory density. Some vendors may add extended information in read id like d, e and f. For Infineon, 03-05h provide extend in formations for flash. For Micron, 03h-19h(4-20 bytes) are called UID which are factory descriptions. For Macronix, a, b and c are all informations in read id operation. > > -michael Thanks Jaime
Hi, >> > 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 I'm wrong - the data strobe signal is optional. -michael
Hi Michael > > Hi, > > >> > 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. I think whether to use 8d or 8s depends on how many times data is latched within per cycle. > I'm wrong - the data strobe signal is optional. As I know, data strobe signal is needed when 8D-8D-8D mode. Infineon data strobe signal called "DS". Macronix and Micron data strobe signal called "DQS" > > -michael Thanks Jaime
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. 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. >> I'm wrong - the data strobe signal is optional. > As I know, data strobe signal is needed when 8D-8D-8D mode. > Infineon data strobe signal called "DS". > Macronix and Micron data strobe signal called "DQS" Maybe Pratyush can help here. But as far as I know, the data strobe is optional. E.g. on a layerscape ls1028 you can use an internal dummy strobe if it's not connected. The timing is then determined by pad delays. -michael
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; > > >> I'm wrong - the data strobe signal is optional. > > As I know, data strobe signal is needed when 8D-8D-8D mode. > > Infineon data strobe signal called "DS". > > Macronix and Micron data strobe signal called "DQS" > > Maybe Pratyush can help here. But as far as I know, the data strobe > is optional. E.g. on a layerscape ls1028 you can use an internal dummy > strobe if it's not connected. The timing is then determined by pad > delays. It would be great if Pratyush could provide additional information. > > -michael Thanks Jaime
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
On Fri, Oct 13 2023, Michael Walle wrote: > Hi, > > Am 2023-10-13 10:23, schrieb liao jaime: [...] >>> I'm wrong - the data strobe signal is optional. >> As I know, data strobe signal is needed when 8D-8D-8D mode. >> Infineon data strobe signal called "DS". >> Macronix and Micron data strobe signal called "DQS" > > Maybe Pratyush can help here. But as far as I know, the data strobe > is optional. E.g. on a layerscape ls1028 you can use an internal dummy > strobe if it's not connected. The timing is then determined by pad > delays. Correct. The data strobe is optional and the controller does not have to use it. Though for higher frequencies, it is usually needed to sample the data lines at the right time -- given the time window to do so is smaller and more precision is required. For example, see my patch here [0] which enabled DQS sampling for PHY reads since those are higher frequency. Since that patch has not yet made it to mainline, currently the Cadence Quadspi driver does not use DQS and still works fine with 8D-8D-8D mode. It does so based on its hard-coded delays in device tree [1] (see cdns,read-delay). [0] https://lore.kernel.org/linux-mtd/20210311191216.7363-5-p.yadav@ti.com/ [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/spi/cdns,qspi-nor-peripheral-props.yaml
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 1b0c6770c14e..7ee624b16e17 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -408,7 +408,7 @@ int spi_nor_write_disable(struct spi_nor *nor) } /** - * spi_nor_read_id() - Read the JEDEC ID. + * spi_nor_default_read_id() - Read the JEDEC ID. * @nor: pointer to 'struct spi_nor'. * @naddr: number of address bytes to send. Can be zero if the operation * does not need to send an address. @@ -420,7 +420,7 @@ int spi_nor_write_disable(struct spi_nor *nor) * * Return: 0 on success, -errno otherwise. */ -int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, +int spi_nor_default_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, enum spi_nor_protocol proto) { int ret; @@ -438,6 +438,32 @@ int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, return ret; } +/** + * spi_nor_read_id() - Read ID by manufacturer read id function. + * @nor: pointer to 'struct spi_nor'. + * @naddr: number of address bytes to send. Can be zero if the operation + * does not need to send an address. + * @ndummy: number of dummy bytes to send after an opcode or address. Can + * be zero if the operation does not require dummy bytes. + * @id: pointer to a DMA-able buffer where the value of the JEDEC ID + * will be written. + * @proto: the SPI protocol for register operation. + * + * Return: 0 on success, -errno otherwise. + */ +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, + enum spi_nor_protocol proto) +{ + int ret; + + if (nor->manufacturer && nor->manufacturer->fixups && nor->manufacturer->fixups->read_id) + ret = nor->manufacturer->fixups->read_id(nor, naddr, ndummy, id, proto); + else + ret = spi_nor_default_read_id(nor, naddr, ndummy, id, proto); + + return ret; +} + /** * spi_nor_read_sr() - Read the Status Register. * @nor: pointer to 'struct spi_nor'. diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h index 9217379b9cfe..92cbc2d3f7fe 100644 --- a/drivers/mtd/spi-nor/core.h +++ b/drivers/mtd/spi-nor/core.h @@ -424,6 +424,8 @@ struct spi_nor_flash_parameter { * @late_init: used to initialize flash parameters that are not declared in the * JESD216 SFDP standard, or where SFDP tables not defined at all. * Will replace the default_init() hook. + * @read_id: used to read id which may change format after enter into + octal dtr mode. * * Those hooks can be used to tweak the SPI NOR configuration when the SFDP * table is broken or not available. @@ -435,6 +437,8 @@ struct spi_nor_fixups { const struct sfdp_bfpt *bfpt); int (*post_sfdp)(struct spi_nor *nor); int (*late_init)(struct spi_nor *nor); + int (*read_id)(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, + enum spi_nor_protocol reg_proto); }; /** @@ -667,6 +671,8 @@ void spi_nor_unlock_and_unprep(struct spi_nor *nor); int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor); int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor); int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor); +int spi_nor_default_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, + enum spi_nor_protocol reg_proto); int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, enum spi_nor_protocol reg_proto); int spi_nor_read_sr(struct spi_nor *nor, u8 *sr); diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c index eb149e517c1f..8ab47691dfbb 100644 --- a/drivers/mtd/spi-nor/macronix.c +++ b/drivers/mtd/spi-nor/macronix.c @@ -118,9 +118,27 @@ static int macronix_nor_late_init(struct spi_nor *nor) return 0; } +static int macronix_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, + enum spi_nor_protocol proto) +{ + int i, ret; + + ret = spi_nor_default_read_id(nor, naddr, ndummy, id, proto); + /* Retrieve odd array and re-sort id because of read id format will be A-A-B-B-C-C + * after enter into octal dtr mode for Macronix flashes. + */ + if (proto == SNOR_PROTO_8_8_8_DTR) { + for (i = 0; i < nor->info->id_len; i++) + id[i] = id[i*2]; + } + + return ret; +} + static const struct spi_nor_fixups macronix_nor_fixups = { .default_init = macronix_nor_default_init, .late_init = macronix_nor_late_init, + .read_id = macronix_nor_read_id, }; const struct spi_nor_manufacturer spi_nor_macronix = {