| Message ID | 20161124165608.29816-1-ricardo.ribalda@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 11/24/2016 05:56 PM, Ricardo Ribalda Delgado wrote: > Xilinx Spartan-3AN FPGAs contain an In-System Flash where they keep > their configuration data and (optionally) some user data. > > The protocol of this flash follows most of the spi-nor standard. With > the following differences: > > - Page size might not be a power of two. > - The address calculation (default addressing mode). > - The spi nor commands used. > > Protocol is described on Xilinx User Guide UG333 > > Reviewed-by: Cyrille Pitchen <cyrille.pitchen@atmel.com> > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > Cc: Brian Norris <computersforpeace@gmail.com> > --- Hi, minor questions/nits below. [...] > -Remove inline qualifier > -Improve documentation of Default Addressing Mode > -Convert function callbacks into SNOR_F_ > -Fix missmatch braces > -Improve documentation of SPI_S3AN flag > > drivers/mtd/spi-nor/spi-nor.c | 134 ++++++++++++++++++++++++++++++++++++++++-- > include/linux/mtd/spi-nor.h | 12 ++++ > 2 files changed, 141 insertions(+), 5 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index d0fc165d7d66..cf0410d4e258 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -75,6 +75,12 @@ struct flash_info { > * bit. Must be used with > * SPI_NOR_HAS_LOCK. > */ > +#define SPI_S3AN BIT(10) /* > + * Xilinx Spartan 3AN In-System Flash > + * (MFR cannot be used for probing > + * because it has the same value as > + * ATMEL flashes) > + */ I have possibly off-topic question. Altera has something very similar -- EPCS/EPCQ flash which cannot be detected using standard READID . Would this patch help with supporting those degenerate flashes too? > }; > > #define JEDEC_MFR(info) ((info)->id[0]) [...] > @@ -1170,8 +1241,15 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len, > > for (i = 0; i < len; ) { > ssize_t written; > + loff_t addr = to + i; > + > + if (hweight32(nor->page_size) == 1) { > + page_offset = addr & (nor->page_size - 1); > + } else { > + uint64_t aux = addr; > > - page_offset = (to + i) & (nor->page_size - 1); > + page_offset = do_div(aux, nor->page_size); > + } Why is this part necessary ? > WARN_ONCE(page_offset, > "Writing at offset %zu into a NOR page. Writing partial pages may decrease reliability and increase wear of NOR flash.", > page_offset); [...] > +static int s3an_nor_scan(const struct flash_info *info, struct spi_nor *nor) > +{ > + int ret; > + u8 val; > + > + ret = nor->read_reg(nor, SPINOR_OP_XRDSR, &val, 1); > + if (ret < 0) { > + dev_err(nor->dev, "error %d reading XRDSR\n", (int) ret); > + return ret; > + } > + > + nor->erase_opcode = SPINOR_OP_XSE; > + nor->program_opcode = SPINOR_OP_XPP; > + nor->read_opcode = SPINOR_OP_READ; > + nor->flags |= SNOR_F_NO_OP_CHIP_ERASE; > + > + /* Flash in Power of 2 mode */ > + if (val & XSR_PAGESIZE) { > + nor->page_size = (nor->page_size == 264) ? 256 : 512; 264 is due to some ECC ? > + nor->mtd.writebufsize = nor->page_size; > + nor->mtd.size = 8 * nor->page_size * info->n_sectors; > + nor->mtd.erasesize = 8 * nor->page_size; > + } else { > + nor->flags |= SNOR_F_S3AN_ADDR_DEFAULT; > + } > + > + return 0; > +} [...]
Hi Marek Thanks for your review On Thu, Dec 1, 2016 at 5:05 PM, Marek Vasut <marek.vasut@gmail.com> wrote: > > On 11/24/2016 05:56 PM, Ricardo Ribalda Delgado wrote: >> +#define SPI_S3AN BIT(10) /* >> + * Xilinx Spartan 3AN In-System Flash >> + * (MFR cannot be used for probing >> + * because it has the same value as >> + * ATMEL flashes) >> + */ > > I have possibly off-topic question. Altera has something very similar -- > EPCS/EPCQ flash which cannot be detected using standard READID . > Would this patch help with supporting those degenerate flashes too? > >> }; >> I dont know, but I love the term degenerated flash, please let me use it :) > > for (i = 0; i < len; ) { > > ssize_t written; > > + loff_t addr = to + i; > > + > > + if (hweight32(nor->page_size) == 1) { > > + page_offset = addr & (nor->page_size - 1); > > + } else { > > + uint64_t aux = addr; > > > > - page_offset = (to + i) & (nor->page_size - 1); > > + page_offset = do_div(aux, nor->page_size); > > + } > > Why is this part necessary ? If page_size is not a power of 2 (264,528), the & (size-1) operation for getting the offset does not work, you need to do a real modulus. > > + > > + /* Flash in Power of 2 mode */ > > + if (val & XSR_PAGESIZE) { > > + nor->page_size = (nor->page_size == 264) ? 256 : 512; > > 264 is due to some ECC ? The flash can be in standard mode or in power of two mode. You need to check the status register to know if the chip is in one mode or the other. The flash is in standard mode from factory, you can change the mode to power of two, but the data is corrupted and you cannot change back to standard mode. I guess they are using some bits reserved to ECC for data and that way you can squeeze some bits for user data. > > > + nor->mtd.writebufsize = nor->page_size; > > + nor->mtd.size = 8 * nor->page_size * info->n_sectors; > > + nor->mtd.erasesize = 8 * nor->page_size; > > + } else { > > + nor->flags |= SNOR_F_S3AN_ADDR_DEFAULT; > > + } > > + > > + return 0; > > +} > > [...] > > -- > Best regards, > Marek Vasut Thanks Again
On 12/01/2016 06:52 PM, Ricardo Ribalda Delgado wrote: > Hi Marek Hi, > Thanks for your review > > On Thu, Dec 1, 2016 at 5:05 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >> >> On 11/24/2016 05:56 PM, Ricardo Ribalda Delgado wrote: > >>> +#define SPI_S3AN BIT(10) /* >>> + * Xilinx Spartan 3AN In-System Flash >>> + * (MFR cannot be used for probing >>> + * because it has the same value as >>> + * ATMEL flashes) >>> + */ >> >> I have possibly off-topic question. Altera has something very similar -- >> EPCS/EPCQ flash which cannot be detected using standard READID . >> Would this patch help with supporting those degenerate flashes too? >> >>> }; >>> > > I dont know, but I love the term degenerated flash, please let me use it :) Hehe. It'd be great to know whether we don't have a possibility for a generic usecase here. Can you briefly check that ? >>> for (i = 0; i < len; ) { >>> ssize_t written; >>> + loff_t addr = to + i; >>> + >>> + if (hweight32(nor->page_size) == 1) { >>> + page_offset = addr & (nor->page_size - 1); >>> + } else { >>> + uint64_t aux = addr; >>> >>> - page_offset = (to + i) & (nor->page_size - 1); >>> + page_offset = do_div(aux, nor->page_size); >>> + } >> >> Why is this part necessary ? > > If page_size is not a power of 2 (264,528), the & (size-1) operation > for getting the offset does not work, you need to do a real modulus. Aaah, now it makes sense. Ew, but all right, thanks for clarifying. This could use a comment, somewhere along these lines. >>> + >>> + /* Flash in Power of 2 mode */ >>> + if (val & XSR_PAGESIZE) { >>> + nor->page_size = (nor->page_size == 264) ? 256 : 512; >> >> 264 is due to some ECC ? > > The flash can be in standard mode or in power of two mode. You need to > check the status register to know if the chip is in one mode or the > other. > > The flash is in standard mode from factory, you can change the mode to > power of two, but the data is corrupted and you cannot change back to > standard mode. > > I guess they are using some bits reserved to ECC for data and that way > you can squeeze some bits for user data. OK, comment could help clarify this, so please add one. >>> + nor->mtd.writebufsize = nor->page_size; >>> + nor->mtd.size = 8 * nor->page_size * info->n_sectors; >>> + nor->mtd.erasesize = 8 * nor->page_size; >>> + } else { >>> + nor->flags |= SNOR_F_S3AN_ADDR_DEFAULT; >>> + } >>> + >>> + return 0; >>> +} >> >> [...] >> >> -- >> Best regards, >> Marek Vasut > > Thanks Again > > > >
Hi Marek On Thu, Dec 1, 2016 at 7:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote: > On 12/01/2016 06:52 PM, Ricardo Ribalda Delgado wrote: >> Hi Marek > > Hi, > >> Thanks for your review >> >> On Thu, Dec 1, 2016 at 5:05 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >>> >>> On 11/24/2016 05:56 PM, Ricardo Ribalda Delgado wrote: >> >>>> +#define SPI_S3AN BIT(10) /* >>>> + * Xilinx Spartan 3AN In-System Flash >>>> + * (MFR cannot be used for probing >>>> + * because it has the same value as >>>> + * ATMEL flashes) >>>> + */ >>> >>> I have possibly off-topic question. Altera has something very similar -- >>> EPCS/EPCQ flash which cannot be detected using standard READID . >>> Would this patch help with supporting those degenerate flashes too? >>> >>>> }; >>>> >> >> I dont know, but I love the term degenerated flash, please let me use it :) > > Hehe. It'd be great to know whether we don't have a possibility for a > generic usecase here. Can you briefly check that ? I have taken a brief look to https://www.altera.com/content/dam/altera-www/global/en_US/pdfs/literature/hb/cfg/cfg_cf52012.pdf and they seem different enough to not reuse the flag :(. >> I guess they are using some bits reserved to ECC for data and that way >> you can squeeze some bits for user data. > > OK, comment could help clarify this, so please add one. Will send a v9 Thanks!
On 12/02/2016 11:52 AM, Ricardo Ribalda Delgado wrote: > Hi Marek Hi, > On Thu, Dec 1, 2016 at 7:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >> On 12/01/2016 06:52 PM, Ricardo Ribalda Delgado wrote: >>> Hi Marek >> >> Hi, >> >>> Thanks for your review >>> >>> On Thu, Dec 1, 2016 at 5:05 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >>>> >>>> On 11/24/2016 05:56 PM, Ricardo Ribalda Delgado wrote: >>> >>>>> +#define SPI_S3AN BIT(10) /* >>>>> + * Xilinx Spartan 3AN In-System Flash >>>>> + * (MFR cannot be used for probing >>>>> + * because it has the same value as >>>>> + * ATMEL flashes) >>>>> + */ >>>> >>>> I have possibly off-topic question. Altera has something very similar -- >>>> EPCS/EPCQ flash which cannot be detected using standard READID . >>>> Would this patch help with supporting those degenerate flashes too? >>>> >>>>> }; >>>>> >>> >>> I dont know, but I love the term degenerated flash, please let me use it :) >> >> Hehe. It'd be great to know whether we don't have a possibility for a >> generic usecase here. Can you briefly check that ? > > I have taken a brief look to > https://www.altera.com/content/dam/altera-www/global/en_US/pdfs/literature/hb/cfg/cfg_cf52012.pdf > > and they seem different enough to not reuse the flag :(. OK, fine, thanks for checking. >>> I guess they are using some bits reserved to ECC for data and that way >>> you can squeeze some bits for user data. >> >> OK, comment could help clarify this, so please add one. > > Will send a v9 Thanks!
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index d0fc165d7d66..cf0410d4e258 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -75,6 +75,12 @@ struct flash_info { * bit. Must be used with * SPI_NOR_HAS_LOCK. */ +#define SPI_S3AN BIT(10) /* + * Xilinx Spartan 3AN In-System Flash + * (MFR cannot be used for probing + * because it has the same value as + * ATMEL flashes) + */ }; #define JEDEC_MFR(info) ((info)->id[0]) @@ -217,6 +223,21 @@ static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info, return nor->write_reg(nor, SPINOR_OP_BRWR, nor->cmd_buf, 1); } } + +static int s3an_sr_ready(struct spi_nor *nor) +{ + int ret; + u8 val; + + ret = nor->read_reg(nor, SPINOR_OP_XRDSR, &val, 1); + if (ret < 0) { + dev_err(nor->dev, "error %d reading XRDSR\n", (int) ret); + return ret; + } + + return !!(val & XSR_RDY); +} + static inline int spi_nor_sr_ready(struct spi_nor *nor) { int sr = read_sr(nor); @@ -238,7 +259,11 @@ static inline int spi_nor_fsr_ready(struct spi_nor *nor) static int spi_nor_ready(struct spi_nor *nor) { int sr, fsr; - sr = spi_nor_sr_ready(nor); + + if (nor->flags & SNOR_F_READY_XSR_RDY) + sr = s3an_sr_ready(nor); + else + sr = spi_nor_sr_ready(nor); if (sr < 0) return sr; fsr = nor->flags & SNOR_F_USE_FSR ? spi_nor_fsr_ready(nor) : 1; @@ -320,6 +345,24 @@ static void spi_nor_unlock_and_unprep(struct spi_nor *nor, enum spi_nor_ops ops) } /* + * This code converts an address to the Default Address Mode, that has non + * power of two page sizes. We must support this mode because it is the default + * mode supported by Xilinx tools, it can access the whole flash area and + * changing over to the Power-of-two mode is irreversible and corrupts the + * original data. + * Addr can safely be unsigned int, the biggest S3AN device is smaller than + * 4 MiB. + */ +static loff_t spi_nor_s3an_addr_convert(struct spi_nor *nor, unsigned int addr) +{ + unsigned int offset = addr; + + offset %= nor->page_size; + + return ((addr - offset) << 1) | offset; +} + +/* * Initiate the erasure of a single sector */ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr) @@ -327,6 +370,9 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr) u8 buf[SPI_NOR_MAX_ADDR_WIDTH]; int i; + if (nor->flags & SNOR_F_S3AN_ADDR_DEFAULT) + addr = spi_nor_s3an_addr_convert(nor, addr); + if (nor->erase) return nor->erase(nor, addr); @@ -368,7 +414,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) return ret; /* whole-chip erase? */ - if (len == mtd->size) { + if (len == mtd->size && !(nor->flags & SNOR_F_NO_OP_CHIP_ERASE)) { unsigned long timeout; write_enable(nor); @@ -782,6 +828,19 @@ static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len) .addr_width = (_addr_width), \ .flags = (_flags), +#define S3AN_INFO(_jedec_id, _n_sectors, _page_size) \ + .id = { \ + ((_jedec_id) >> 16) & 0xff, \ + ((_jedec_id) >> 8) & 0xff, \ + (_jedec_id) & 0xff \ + }, \ + .id_len = 3, \ + .sector_size = (8*_page_size), \ + .n_sectors = (_n_sectors), \ + .page_size = _page_size, \ + .addr_width = 3, \ + .flags = SPI_NOR_NO_FR | SPI_S3AN, + /* NOTE: double check command sets and memory organization when you add * more nor chips. This current list focusses on newer chips, which * have been converging on command sets which including JEDEC ID. @@ -1009,6 +1068,13 @@ static const struct flash_info spi_nor_ids[] = { { "cat25c09", CAT25_INFO( 128, 8, 32, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) }, { "cat25c17", CAT25_INFO( 256, 8, 32, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) }, { "cat25128", CAT25_INFO(2048, 8, 64, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) }, + + /* Xilinx S3AN Internal Flash */ + { "3S50AN", S3AN_INFO(0x1f2200, 64, 264) }, + { "3S200AN", S3AN_INFO(0x1f2400, 256, 264) }, + { "3S400AN", S3AN_INFO(0x1f2400, 256, 264) }, + { "3S700AN", S3AN_INFO(0x1f2500, 512, 264) }, + { "3S1400AN", S3AN_INFO(0x1f2600, 512, 528) }, { }, }; @@ -1049,7 +1115,12 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, return ret; while (len) { - ret = nor->read(nor, from, len, buf); + loff_t addr = from; + + if (nor->flags & SNOR_F_S3AN_ADDR_DEFAULT) + addr = spi_nor_s3an_addr_convert(nor, addr); + + ret = nor->read(nor, addr, len, buf); if (ret == 0) { /* We shouldn't see 0-length reads */ ret = -EIO; @@ -1170,8 +1241,15 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len, for (i = 0; i < len; ) { ssize_t written; + loff_t addr = to + i; + + if (hweight32(nor->page_size) == 1) { + page_offset = addr & (nor->page_size - 1); + } else { + uint64_t aux = addr; - page_offset = (to + i) & (nor->page_size - 1); + page_offset = do_div(aux, nor->page_size); + } WARN_ONCE(page_offset, "Writing at offset %zu into a NOR page. Writing partial pages may decrease reliability and increase wear of NOR flash.", page_offset); @@ -1179,8 +1257,11 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len, page_remain = min_t(size_t, nor->page_size - page_offset, len - i); + if (nor->flags & SNOR_F_S3AN_ADDR_DEFAULT) + addr = spi_nor_s3an_addr_convert(nor, addr); + write_enable(nor); - ret = nor->write(nor, to + i, page_remain, buf + i); + ret = nor->write(nor, addr, page_remain, buf + i); if (ret < 0) goto write_err; written = ret; @@ -1300,6 +1381,35 @@ static int spi_nor_check(struct spi_nor *nor) return 0; } +static int s3an_nor_scan(const struct flash_info *info, struct spi_nor *nor) +{ + int ret; + u8 val; + + ret = nor->read_reg(nor, SPINOR_OP_XRDSR, &val, 1); + if (ret < 0) { + dev_err(nor->dev, "error %d reading XRDSR\n", (int) ret); + return ret; + } + + nor->erase_opcode = SPINOR_OP_XSE; + nor->program_opcode = SPINOR_OP_XPP; + nor->read_opcode = SPINOR_OP_READ; + nor->flags |= SNOR_F_NO_OP_CHIP_ERASE; + + /* Flash in Power of 2 mode */ + if (val & XSR_PAGESIZE) { + nor->page_size = (nor->page_size == 264) ? 256 : 512; + nor->mtd.writebufsize = nor->page_size; + nor->mtd.size = 8 * nor->page_size * info->n_sectors; + nor->mtd.erasesize = 8 * nor->page_size; + } else { + nor->flags |= SNOR_F_S3AN_ADDR_DEFAULT; + } + + return 0; +} + int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) { const struct flash_info *info = NULL; @@ -1348,6 +1458,14 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) mutex_init(&nor->lock); /* + * Make sure the XSR_RDY flag is set before calling + * spi_nor_wait_till_ready(). Xilinx S3AN share MFR + * with Atmel spi-nor + */ + if (info->flags & SPI_S3AN) + nor->flags |= SNOR_F_READY_XSR_RDY; + + /* * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up * with the software protection bits set */ @@ -1505,6 +1623,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) nor->read_dummy = spi_nor_read_dummy_cycles(nor); + if (info->flags & SPI_S3AN) { + ret = s3an_nor_scan(info, nor); + if (ret) + return ret; + } + dev_info(dev, "%s (%lld Kbytes)\n", info->name, (long long)mtd->size >> 10); diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index c425c7b4c2a0..4950b2ef08c0 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -68,6 +68,15 @@ #define SPINOR_OP_WRDI 0x04 /* Write disable */ #define SPINOR_OP_AAI_WP 0xad /* Auto address increment word program */ +/* Used for S3AN flashes only */ +#define SPINOR_OP_XSE 0x50 /* Sector erase */ +#define SPINOR_OP_XPP 0x82 /* Page program */ +#define SPINOR_OP_XRDSR 0xd7 /* Read status register */ + +#define XSR_PAGESIZE BIT(0) /* Page size in Po2 or Linear */ +#define XSR_RDY BIT(7) /* Ready */ + + /* Used for Macronix and Winbond flashes. */ #define SPINOR_OP_EN4B 0xb7 /* Enter 4-byte mode */ #define SPINOR_OP_EX4B 0xe9 /* Exit 4-byte mode */ @@ -119,6 +128,9 @@ enum spi_nor_ops { enum spi_nor_option_flags { SNOR_F_USE_FSR = BIT(0), SNOR_F_HAS_SR_TB = BIT(1), + SNOR_F_NO_OP_CHIP_ERASE = BIT(2), + SNOR_F_S3AN_ADDR_DEFAULT = BIT(3), + SNOR_F_READY_XSR_RDY = BIT(4), }; /**