| Message ID | 20200309084033.8752-1-s.hauer@pengutronix.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | [v2] mtd: spi-nor: Add support for cy15x104q | expand |
Hi, Any feedback to this one? Sascha On Mon, Mar 09, 2020 at 09:40:33AM +0100, Sascha Hauer wrote: > The Cypress cy15b104q and cy15v104q are 4Mbit serial SPI F-RAM devices. > Add support for them to the spi-nor driver. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > > Changes since v1: > - Instead of specifying 1024 sectors with a sector size of 512 specify > 512 * 1024 sectos with a sector size of 1. The device has no idea of > sectors and is not erasable, so a sector size of 1 seems to better > reflect reality. > > drivers/mtd/spi-nor/spi-nor.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 4fc632ec18fe..a5c1d684364c 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -2335,6 +2335,9 @@ static const struct flash_info spi_nor_ids[] = { > > { "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) }, > > + /* Cypress */ > + { "cy15x104q", INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1, SPI_NOR_NO_ERASE) }, > + > /* EON -- en25xxx */ > { "en25f32", INFO(0x1c3116, 0, 64 * 1024, 64, SECT_4K) }, > { "en25p32", INFO(0x1c2016, 0, 64 * 1024, 64, 0) }, > -- > 2.25.1 > >
On Tuesday, April 14, 2020 3:09:45 PM EEST Sascha Hauer wrote:
> Any feedback to this one?
Hi, Sascha,
I'm a bit busy but I'll try to allocate time to review patches sometime this
week. BTW, we moved the manufacturer specific code out of the core, we now
have a dedicated file for each manufacturer (this includes flash_info
entries), check the spi-nor/next branch. I know that it's not your fault that
your patch was left behind, so I volunteer to respin your patch if you don't
feel like doing it.
Cheers,
ta
Hi Tudor, On Tue, Apr 14, 2020 at 04:41:42PM +0000, Tudor.Ambarus@microchip.com wrote: > On Tuesday, April 14, 2020 3:09:45 PM EEST Sascha Hauer wrote: > > Any feedback to this one? > > Hi, Sascha, > > I'm a bit busy but I'll try to allocate time to review patches sometime this > week. BTW, we moved the manufacturer specific code out of the core, we now > have a dedicated file for each manufacturer (this includes flash_info > entries), check the spi-nor/next branch. I see. It's in master now btw. > I know that it's not your fault that > your patch was left behind, so I volunteer to respin your patch if you don't > feel like doing it. Don't worry, I can respin it. You want to have a cypress.c file, even though it has only a single entry, right? Sascha
On 2020/4/15 13:35, Sascha Hauer wrote: > Hi Tudor, > > On Tue, Apr 14, 2020 at 04:41:42PM +0000, Tudor.Ambarus@microchip.com wrote: >> On Tuesday, April 14, 2020 3:09:45 PM EEST Sascha Hauer wrote: >>> Any feedback to this one? >> Hi, Sascha, >> >> I'm a bit busy but I'll try to allocate time to review patches sometime this >> week. BTW, we moved the manufacturer specific code out of the core, we now >> have a dedicated file for each manufacturer (this includes flash_info >> entries), check the spi-nor/next branch. > I see. It's in master now btw. > >> I know that it's not your fault that >> your patch was left behind, so I volunteer to respin your patch if you don't >> feel like doing it. > Don't worry, I can respin it. You want to have a cypress.c file, even > though it has only a single entry, right? Hi Sascha, Maybe it's okay to put in spansion.c, as Spansion/Cypress may use same manufacturer id on some flash. Otherwise, we should spilt the Cypress ones from list in spansion.c to new cypress.c. Regards, Yicong > > Sascha >
On Wednesday, April 15, 2020 8:35:27 AM EEST Sascha Hauer wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Hi Tudor, Hi, Sascha, > > On Tue, Apr 14, 2020 at 04:41:42PM +0000, Tudor.Ambarus@microchip.com wrote: > > On Tuesday, April 14, 2020 3:09:45 PM EEST Sascha Hauer wrote: > > > Any feedback to this one? > > > > Hi, Sascha, > > > > I'm a bit busy but I'll try to allocate time to review patches sometime > > this week. BTW, we moved the manufacturer specific code out of the core, > > we now have a dedicated file for each manufacturer (this includes > > flash_info entries), check the spi-nor/next branch. > > I see. It's in master now btw. > > > I know that it's not your fault that > > your patch was left behind, so I volunteer to respin your patch if you > > don't feel like doing it. > > Don't worry, I can respin it. You want to have a cypress.c file, even > though it has only a single entry, right? > I think you can add this as the last entry in the spansion_parts[] array from spansion.c. Cypress and Spansion merged back in 2015, they are a single entity now. The fixup hook is not affecting this flash, we should be fine. For the moment I don't see a need for a spi_nor_cypress struct. Cheers, ta
On Monday, March 9, 2020 10:40:33 AM EEST Sascha Hauer wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > The Cypress cy15b104q and cy15v104q are 4Mbit serial SPI F-RAM devices. > Add support for them to the spi-nor driver. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > > Changes since v1: > - Instead of specifying 1024 sectors with a sector size of 512 specify > 512 * 1024 sectos with a sector size of 1. The device has no idea of > sectors and is not erasable, so a sector size of 1 seems to better > reflect reality. > > drivers/mtd/spi-nor/spi-nor.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 4fc632ec18fe..a5c1d684364c 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -2335,6 +2335,9 @@ static const struct flash_info spi_nor_ids[] = { > > { "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) }, > > + /* Cypress */ > + { "cy15x104q", INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1, > SPI_NOR_NO_ERASE) }, + Shouldn't the id start with 0x03 instead of 0x04? Check [1]. Otherwise looks good. Also, would you please specify in the commit message on which platform did you test the flash, or with which controller? We usually don't add new flash_info entries solely by datasheet info, we've seen some nasty bugs and we try to avoid adding flashes that are broken since day one. Cheers, ta [1] https://www.google.com/url? sa=t&rct=j&q=&esrc=s&source=web&cd=1&cad=rja&uact=8&ved=2ahUKEwikvb6A2- roAhUKjqQKHQRzAhEQFjAAegQIAhAB&url=https%3A%2F%2Fmedia.digikey.com%2Fpdf%2FData%2520Sheets%2FCypress%2520PDFs%2FCY15B104Q_CY15V104Q_RevB_2-3-20.pdf&usg=AOvVaw0BmuUvI2j3NR07ZN_Jd4bN
On 09/03/20 2:10 pm, Sascha Hauer wrote: > The Cypress cy15b104q and cy15v104q are 4Mbit serial SPI F-RAM devices. > Add support for them to the spi-nor driver. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > > Changes since v1: > - Instead of specifying 1024 sectors with a sector size of 512 specify > 512 * 1024 sectos with a sector size of 1. The device has no idea of > sectors and is not erasable, so a sector size of 1 seems to better > reflect reality. > > drivers/mtd/spi-nor/spi-nor.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 4fc632ec18fe..a5c1d684364c 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -2335,6 +2335,9 @@ static const struct flash_info spi_nor_ids[] = { > > { "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) }, > > + /* Cypress */ > + { "cy15x104q", INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1, SPI_NOR_NO_ERASE) }, This seems wrong As the datasheet for the device says: "Device ID The CY15B104Q device can be interrogated for its manufacturer, product identification, and die revision. The RDID opcode 9Fh allows the user to read the manufacturer ID and product ID, both of which are read-only bytes. The JEDEC-assigned manufacturer ID places the Cypress (Ramtron) identifier in bank 7; therefore, there are six bytes of the continuation code 7Fh followed by the single byte C2h. There are two bytes of product ID, which includes a family code, a density code, a sub code, and the product revision code." I am not sure how are you reading 0x4 as the first byte. It should have been 6 bytes of 0x7F followed by 0xc2 right? Also 0x7F is continuation code and not actual device ID (See JEDEC standard JEP106). You will have to extend spi_nor_read_id() function to take care of continuation code and read more bytes in order to get the actual Manuf/device ID > + > /* EON -- en25xxx */ > { "en25f32", INFO(0x1c3116, 0, 64 * 1024, 64, SECT_4K) }, > { "en25p32", INFO(0x1c2016, 0, 64 * 1024, 64, 0) }, >
On Wednesday, April 15, 2020 8:28:36 PM EEST Vignesh Raghavendra wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > On 09/03/20 2:10 pm, Sascha Hauer wrote: > > The Cypress cy15b104q and cy15v104q are 4Mbit serial SPI F-RAM devices. > > Add support for them to the spi-nor driver. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > > > Changes since v1: > > - Instead of specifying 1024 sectors with a sector size of 512 specify > > > > 512 * 1024 sectos with a sector size of 1. The device has no idea of > > sectors and is not erasable, so a sector size of 1 seems to better > > reflect reality. > > > > drivers/mtd/spi-nor/spi-nor.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > > index 4fc632ec18fe..a5c1d684364c 100644 > > --- a/drivers/mtd/spi-nor/spi-nor.c > > +++ b/drivers/mtd/spi-nor/spi-nor.c > > @@ -2335,6 +2335,9 @@ static const struct flash_info spi_nor_ids[] = { > > > > { "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) }, > > > > + /* Cypress */ > > + { "cy15x104q", INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1, > > SPI_NOR_NO_ERASE) }, > This seems wrong > > As the datasheet for the device says: > > "Device ID > The CY15B104Q device can be interrogated for its manufacturer, product > identification, and die revision. The RDID opcode 9Fh allows > the user to read the manufacturer ID and product ID, both of which are > read-only bytes. The JEDEC-assigned manufacturer ID places > the Cypress (Ramtron) identifier in bank 7; therefore, there are six > bytes of the continuation code 7Fh followed by the single byte C2h. > There are two bytes of product ID, which includes a family code, a > density code, a sub code, and the product revision code." > > I am not sure how are you reading 0x4 as the first byte. It should have > been 6 bytes of 0x7F followed by 0xc2 right? You're right, I skimmed too quickly through the id. > > Also 0x7F is continuation code and not actual device ID (See JEDEC > standard JEP106). You will have to extend spi_nor_read_id() function to > take care of continuation code and read more bytes in order to get the > actual Manuf/device ID Right, thanks for the intervention, Vignesh! Cheers, ta
Hi, On Wed, Apr 15, 2020 at 10:58:36PM +0530, Vignesh Raghavendra wrote: > > > On 09/03/20 2:10 pm, Sascha Hauer wrote: > > The Cypress cy15b104q and cy15v104q are 4Mbit serial SPI F-RAM devices. > > Add support for them to the spi-nor driver. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > > > Changes since v1: > > - Instead of specifying 1024 sectors with a sector size of 512 specify > > 512 * 1024 sectos with a sector size of 1. The device has no idea of > > sectors and is not erasable, so a sector size of 1 seems to better > > reflect reality. > > > > drivers/mtd/spi-nor/spi-nor.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > > index 4fc632ec18fe..a5c1d684364c 100644 > > --- a/drivers/mtd/spi-nor/spi-nor.c > > +++ b/drivers/mtd/spi-nor/spi-nor.c > > @@ -2335,6 +2335,9 @@ static const struct flash_info spi_nor_ids[] = { > > > > { "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) }, > > > > + /* Cypress */ > > + { "cy15x104q", INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1, SPI_NOR_NO_ERASE) }, > > This seems wrong > > As the datasheet for the device says: > > "Device ID > The CY15B104Q device can be interrogated for its manufacturer, product > identification, and die revision. The RDID opcode 9Fh allows > the user to read the manufacturer ID and product ID, both of which are > read-only bytes. The JEDEC-assigned manufacturer ID places > the Cypress (Ramtron) identifier in bank 7; therefore, there are six > bytes of the continuation code 7Fh followed by the single byte C2h. > There are two bytes of product ID, which includes a family code, a > density code, a sub code, and the product revision code." > > I am not sure how are you reading 0x4 as the first byte. It should have > been 6 bytes of 0x7F followed by 0xc2 right? > > Also 0x7F is continuation code and not actual device ID (See JEDEC > standard JEP106). You will have to extend spi_nor_read_id() function to > take care of continuation code and read more bytes in order to get the > actual Manuf/device ID The three times 0x7f looked fishy from the start. I'll dig a bit deeper what is happening here. Thanks for the input. Sascha
On Wed, Apr 15, 2020 at 10:58:36PM +0530, Vignesh Raghavendra wrote: > > > On 09/03/20 2:10 pm, Sascha Hauer wrote: > > The Cypress cy15b104q and cy15v104q are 4Mbit serial SPI F-RAM devices. > > Add support for them to the spi-nor driver. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > > > Changes since v1: > > - Instead of specifying 1024 sectors with a sector size of 512 specify > > 512 * 1024 sectos with a sector size of 1. The device has no idea of > > sectors and is not erasable, so a sector size of 1 seems to better > > reflect reality. > > > > drivers/mtd/spi-nor/spi-nor.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > > index 4fc632ec18fe..a5c1d684364c 100644 > > --- a/drivers/mtd/spi-nor/spi-nor.c > > +++ b/drivers/mtd/spi-nor/spi-nor.c > > @@ -2335,6 +2335,9 @@ static const struct flash_info spi_nor_ids[] = { > > > > { "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) }, > > > > + /* Cypress */ > > + { "cy15x104q", INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1, SPI_NOR_NO_ERASE) }, > > This seems wrong > > As the datasheet for the device says: > > "Device ID > The CY15B104Q device can be interrogated for its manufacturer, product > identification, and die revision. The RDID opcode 9Fh allows > the user to read the manufacturer ID and product ID, both of which are > read-only bytes. The JEDEC-assigned manufacturer ID places > the Cypress (Ramtron) identifier in bank 7; therefore, there are six > bytes of the continuation code 7Fh followed by the single byte C2h. > There are two bytes of product ID, which includes a family code, a > density code, a sub code, and the product revision code." > > I am not sure how are you reading 0x4 as the first byte. It should have > been 6 bytes of 0x7F followed by 0xc2 right? > > Also 0x7F is continuation code and not actual device ID (See JEDEC > standard JEP106). You will have to extend spi_nor_read_id() function to > take care of continuation code and read more bytes in order to get the > actual Manuf/device ID I digged a bit deeper. According to the datasheet the data sent as response to the read id command is: 7f 7f 7f 7f 7f 7f c2 2c 04 What I read from the device instead is: 04 2c c2 7f 7f 7f 7f 7f 7f You see the order of the values is reversed. And in fact this matches the datasheet. Read on in the paragraph you cited from: | Note: The least significant data byte (Byte 0) shifts out first and | the most significant data byte (Byte 8) shifts out last. This sounds very buggy to me. The number of continuation codes (0x7f) says which bank in jep106 I have to look at. I increased SPI_NOR_MAX_ID_LEN to see what the device sends after the ID above and the device happily sends more 0x7f continuation codes. How should I count the number of continuation codes when the device sends any desired number of them at the end of the message? I don't know what's going on here, to me it looks like they accidently mixed the order of the bytes and instead of fixing it a note was added in the datasheet. I hope it's different and you tell me what's wrong in my understanding ;) Sascha
On Wed, Apr 15, 2020 at 03:24:33PM +0000, Tudor.Ambarus@microchip.com wrote: > On Monday, March 9, 2020 10:40:33 AM EEST Sascha Hauer wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > > content is safe > > > > The Cypress cy15b104q and cy15v104q are 4Mbit serial SPI F-RAM devices. > > Add support for them to the spi-nor driver. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > > > Changes since v1: > > - Instead of specifying 1024 sectors with a sector size of 512 specify > > 512 * 1024 sectos with a sector size of 1. The device has no idea of > > sectors and is not erasable, so a sector size of 1 seems to better > > reflect reality. > > > > drivers/mtd/spi-nor/spi-nor.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > > index 4fc632ec18fe..a5c1d684364c 100644 > > --- a/drivers/mtd/spi-nor/spi-nor.c > > +++ b/drivers/mtd/spi-nor/spi-nor.c > > @@ -2335,6 +2335,9 @@ static const struct flash_info spi_nor_ids[] = { > > > > { "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) }, > > > > + /* Cypress */ > > + { "cy15x104q", INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1, > > SPI_NOR_NO_ERASE) }, + > > Shouldn't the id start with 0x03 instead of 0x04? Check [1]. The datasheet you are referencing describes only one of a whole family of chips. They all seem software compatible, here is a list of ids I found in various datasheets: CY15B104Q-PZXI 7F7F7F7F7F7FC22C03 51-85075 8-pin PDIP Industrial CY15B104QN-50SXA 7F7F7F7F7F7FC22C40 001-85261 8-pin SOIC (EIAJ) Automotive CY15V104QN-50SXI 7F7F7F7F7F7FC22C04 002-18131 8-pin GQFN Industrial CY15B104QN-20LPXC 7F7F7F7F7F7FC22CA1 002-18131 8-pin GQFN Commercial CY15B104QN-20LPXI 7F7F7F7F7F7FC22C01 002-18131 8-pin GQFN Industrial CY15V104QN-20LPXC 7F7F7F7F7F7FC22CA5 002-18131 8-pin GQFN Commercial CY15V104QN-20LPXI 7F7F7F7F7F7FC22C05 002-18131 8-pin GQFN Industrial CY15B104QN-50LPXI 7F7F7F7F7F7FC22C00 002-18131 8-pin GQFN Industrial Sascha
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 4fc632ec18fe..a5c1d684364c 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -2335,6 +2335,9 @@ static const struct flash_info spi_nor_ids[] = { { "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) }, + /* Cypress */ + { "cy15x104q", INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1, SPI_NOR_NO_ERASE) }, + /* EON -- en25xxx */ { "en25f32", INFO(0x1c3116, 0, 64 * 1024, 64, SECT_4K) }, { "en25p32", INFO(0x1c2016, 0, 64 * 1024, 64, 0) },
The Cypress cy15b104q and cy15v104q are 4Mbit serial SPI F-RAM devices. Add support for them to the spi-nor driver. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- Changes since v1: - Instead of specifying 1024 sectors with a sector size of 512 specify 512 * 1024 sectos with a sector size of 1. The device has no idea of sectors and is not erasable, so a sector size of 1 seems to better reflect reality. drivers/mtd/spi-nor/spi-nor.c | 3 +++ 1 file changed, 3 insertions(+)