Message ID | 20190824120027.14452-8-tudor.ambarus@microchip.com |
---|---|
State | Changes Requested |
Delegated to: | Ambarus Tudor |
Headers | show |
Series | mtd: spi-nor: move manuf out of the core - batch 1 | expand |
On Sat, 24 Aug 2019 12:00:48 +0000 <Tudor.Ambarus@microchip.com> wrote: > From: Tudor Ambarus <tudor.ambarus@microchip.com> > > Get rid of MFR handling and implement specific manufacturer > default_init() fixup hooks. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/spi-nor.c | 30 ++++++++++++++++++++---------- > 1 file changed, 20 insertions(+), 10 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index fc9e14777212..f4e9fcca619f 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -4146,6 +4146,16 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor, > return err; > } > > +static void atmel_set_default_init(struct spi_nor *nor) > +{ > + nor->params.disable_block_protection = spi_nor_clear_sr_bp; > +} > + > +static void intel_set_default_init(struct spi_nor *nor) > +{ > + nor->params.disable_block_protection = spi_nor_clear_sr_bp; That's weird: you can unlock blocks but locking is not explicitly flagged as supported (SNOR_F_HAS_LOCK is not set). Is that expected? > +} > + > static void macronix_set_default_init(struct spi_nor *nor) > { > nor->params.quad_enable = macronix_quad_enable; > @@ -4173,6 +4183,14 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor) > { > /* Init flash parameters based on MFR */ > switch (JEDEC_MFR(nor->info)) { > + case SNOR_MFR_ATMEL: > + atmel_set_default_init(nor); > + break; > + > + case SNOR_MFR_INTEL: > + intel_set_default_init(nor); > + break; > + > case SNOR_MFR_MACRONIX: > macronix_set_default_init(nor); > break; > @@ -4760,18 +4778,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > if (info->flags & SPI_S3AN) > nor->flags |= SNOR_F_READY_XSR_RDY; > > - if (info->flags & SPI_NOR_HAS_LOCK) > + if (info->flags & SPI_NOR_HAS_LOCK) { If this flag implies SR_BP-based locking we should really rename it into SPI_NOR_HAS_SR_BP_LOCK to avoid any confusion. > nor->flags |= SNOR_F_HAS_LOCK; > - > - /* > - * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up > - * with the software protection bits set. > - */ > - if (JEDEC_MFR(nor->info) == SNOR_MFR_ATMEL || > - JEDEC_MFR(nor->info) == SNOR_MFR_INTEL || > - JEDEC_MFR(nor->info) == SNOR_MFR_SST || > - nor->info->flags & SPI_NOR_HAS_LOCK) > nor->params.disable_block_protection = spi_nor_clear_sr_bp; > + } > > /* Init flash parameters based on flash_info struct and SFDP */ > spi_nor_init_params(nor);
On 08/25/2019 03:24 PM, Boris Brezillon wrote: > On Sat, 24 Aug 2019 12:00:48 +0000 > <Tudor.Ambarus@microchip.com> wrote: > >> From: Tudor Ambarus <tudor.ambarus@microchip.com> >> >> Get rid of MFR handling and implement specific manufacturer >> default_init() fixup hooks. >> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >> --- >> drivers/mtd/spi-nor/spi-nor.c | 30 ++++++++++++++++++++---------- >> 1 file changed, 20 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >> index fc9e14777212..f4e9fcca619f 100644 >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> @@ -4146,6 +4146,16 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor, >> return err; >> } >> >> +static void atmel_set_default_init(struct spi_nor *nor) >> +{ >> + nor->params.disable_block_protection = spi_nor_clear_sr_bp; >> +} >> + >> +static void intel_set_default_init(struct spi_nor *nor) >> +{ >> + nor->params.disable_block_protection = spi_nor_clear_sr_bp; > > That's weird: you can unlock blocks but locking is not > explicitly flagged as supported (SNOR_F_HAS_LOCK is not set). Is that > expected? Yes. Manufacturers have different methods for locking/unlocking blocks of memory. Right now we support just the stm/sr locking operations. sst26vf064b for example, uses dedicated registers for reading/writing which blocks are protected, and not the Status Register. The reason for having disable_block_protection(), is that some spi-nor flashes are write protected by default after a power-on reset cycle, in order to avoid inadvertent writes during power-up. Backward compatibility imposes to disable the write block protection at power-up by default, so that you can erase/write the memory without having to send an unlock-all command. Which is bad in my opinion and that's why I proposed https://patchwork.ozlabs.org/patch/1133278/. Even if sst26vf064b does not yet have the lock ops implemented (SNOR_F_HAS_LOCK is not set), I would like to be able to interact with it, so to disable the block protection at power-up. This flash, and others, support a Global Unlock Command which unlocks the entire memory array in a single cycle. We can't determine who supports this command purely by manufacturer type, and it's not discoverable through SFDP, so we'll have to add a nor->info flag for it: UNLOCK_GLOBAL_BLOCK (see https://patchwork.ozlabs.org/patch/1152606/). In conclusion, even if SNOR_F_HAS_LOCK is not set (the locking ops are not implemented), we can still have disable_block_protection() mechanisms to unlock the entire flash at power-up. > >> +} >> + >> static void macronix_set_default_init(struct spi_nor *nor) >> { >> nor->params.quad_enable = macronix_quad_enable; >> @@ -4173,6 +4183,14 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor) >> { >> /* Init flash parameters based on MFR */ >> switch (JEDEC_MFR(nor->info)) { >> + case SNOR_MFR_ATMEL: >> + atmel_set_default_init(nor); >> + break; >> + >> + case SNOR_MFR_INTEL: >> + intel_set_default_init(nor); >> + break; >> + >> case SNOR_MFR_MACRONIX: >> macronix_set_default_init(nor); >> break; >> @@ -4760,18 +4778,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, >> if (info->flags & SPI_S3AN) >> nor->flags |= SNOR_F_READY_XSR_RDY; >> >> - if (info->flags & SPI_NOR_HAS_LOCK) >> + if (info->flags & SPI_NOR_HAS_LOCK) { > > If this flag implies SR_BP-based locking we should really rename it into > SPI_NOR_HAS_SR_BP_LOCK to avoid any confusion. Not only SR-based locking, should be a general flag that indicates that locking ops are supported whichever they are. I would keep the SPI_NOR_HAS_LOCK and let the manufacturer set its locking ops using the ->default_init() hook. > >> nor->flags |= SNOR_F_HAS_LOCK; >> - >> - /* >> - * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up >> - * with the software protection bits set. >> - */ >> - if (JEDEC_MFR(nor->info) == SNOR_MFR_ATMEL || >> - JEDEC_MFR(nor->info) == SNOR_MFR_INTEL || >> - JEDEC_MFR(nor->info) == SNOR_MFR_SST || >> - nor->info->flags & SPI_NOR_HAS_LOCK) >> nor->params.disable_block_protection = spi_nor_clear_sr_bp; >> + } >> >> /* Init flash parameters based on flash_info struct and SFDP */ >> spi_nor_init_params(nor); > >
On Sun, 25 Aug 2019 12:57:35 +0000 <Tudor.Ambarus@microchip.com> wrote: > On 08/25/2019 03:24 PM, Boris Brezillon wrote: > > On Sat, 24 Aug 2019 12:00:48 +0000 > > <Tudor.Ambarus@microchip.com> wrote: > > > >> From: Tudor Ambarus <tudor.ambarus@microchip.com> > >> > >> Get rid of MFR handling and implement specific manufacturer > >> default_init() fixup hooks. > >> > >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > >> --- > >> drivers/mtd/spi-nor/spi-nor.c | 30 ++++++++++++++++++++---------- > >> 1 file changed, 20 insertions(+), 10 deletions(-) > >> > >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > >> index fc9e14777212..f4e9fcca619f 100644 > >> --- a/drivers/mtd/spi-nor/spi-nor.c > >> +++ b/drivers/mtd/spi-nor/spi-nor.c > >> @@ -4146,6 +4146,16 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor, > >> return err; > >> } > >> > >> +static void atmel_set_default_init(struct spi_nor *nor) > >> +{ > >> + nor->params.disable_block_protection = spi_nor_clear_sr_bp; > >> +} > >> + > >> +static void intel_set_default_init(struct spi_nor *nor) > >> +{ > >> + nor->params.disable_block_protection = spi_nor_clear_sr_bp; > > > > That's weird: you can unlock blocks but locking is not > > explicitly flagged as supported (SNOR_F_HAS_LOCK is not set). Is that > > expected? > > Yes. Manufacturers have different methods for locking/unlocking blocks of > memory. Right now we support just the stm/sr locking operations. sst26vf064b for > example, uses dedicated registers for reading/writing which blocks are > protected, and not the Status Register. > > The reason for having disable_block_protection(), is that some spi-nor flashes > are write protected by default after a power-on reset cycle, in order to avoid > inadvertent writes during power-up. Backward compatibility imposes to disable > the write block protection at power-up by default, so that you can erase/write > the memory without having to send an unlock-all command. Which is bad in my > opinion and that's why I proposed https://patchwork.ozlabs.org/patch/1133278/. > > Even if sst26vf064b does not yet have the lock ops implemented (SNOR_F_HAS_LOCK > is not set), I would like to be able to interact with it, so to disable the > block protection at power-up. This flash, and others, support a Global Unlock > Command which unlocks the entire memory array in a single cycle. We can't > determine who supports this command purely by manufacturer type, and it's not > discoverable through SFDP, so we'll have to add a nor->info flag for it: > UNLOCK_GLOBAL_BLOCK (see https://patchwork.ozlabs.org/patch/1152606/). > > In conclusion, even if SNOR_F_HAS_LOCK is not set (the locking ops are not > implemented), we can still have disable_block_protection() mechanisms to unlock > the entire flash at power-up. Hm, okay, but what about those atmel/intel chips that support SR_BP-based global unlock? Shouldn't they also support SR_BP-based locking/unlocking? > > > > >> +} > >> + > >> static void macronix_set_default_init(struct spi_nor *nor) > >> { > >> nor->params.quad_enable = macronix_quad_enable; > >> @@ -4173,6 +4183,14 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor) > >> { > >> /* Init flash parameters based on MFR */ > >> switch (JEDEC_MFR(nor->info)) { > >> + case SNOR_MFR_ATMEL: > >> + atmel_set_default_init(nor); > >> + break; > >> + > >> + case SNOR_MFR_INTEL: > >> + intel_set_default_init(nor); > >> + break; > >> + > >> case SNOR_MFR_MACRONIX: > >> macronix_set_default_init(nor); > >> break; > >> @@ -4760,18 +4778,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > >> if (info->flags & SPI_S3AN) > >> nor->flags |= SNOR_F_READY_XSR_RDY; > >> > >> - if (info->flags & SPI_NOR_HAS_LOCK) > >> + if (info->flags & SPI_NOR_HAS_LOCK) { > > > > If this flag implies SR_BP-based locking we should really rename it into > > SPI_NOR_HAS_SR_BP_LOCK to avoid any confusion. > > Not only SR-based locking, should be a general flag that indicates that locking > ops are supported whichever they are. I would keep the SPI_NOR_HAS_LOCK and let > the manufacturer set its locking ops using the ->default_init() hook. Okay, sounds good as long as the locking scheme is selected on a per-manufacturer basis, not a per-chip basis. > > > > >> nor->flags |= SNOR_F_HAS_LOCK; > >> - > >> - /* > >> - * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up > >> - * with the software protection bits set. > >> - */ > >> - if (JEDEC_MFR(nor->info) == SNOR_MFR_ATMEL || > >> - JEDEC_MFR(nor->info) == SNOR_MFR_INTEL || > >> - JEDEC_MFR(nor->info) == SNOR_MFR_SST || > >> - nor->info->flags & SPI_NOR_HAS_LOCK) > >> nor->params.disable_block_protection = spi_nor_clear_sr_bp; > >> + } > >> > >> /* Init flash parameters based on flash_info struct and SFDP */ > >> spi_nor_init_params(nor); > > > >
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index fc9e14777212..f4e9fcca619f 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -4146,6 +4146,16 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor, return err; } +static void atmel_set_default_init(struct spi_nor *nor) +{ + nor->params.disable_block_protection = spi_nor_clear_sr_bp; +} + +static void intel_set_default_init(struct spi_nor *nor) +{ + nor->params.disable_block_protection = spi_nor_clear_sr_bp; +} + static void macronix_set_default_init(struct spi_nor *nor) { nor->params.quad_enable = macronix_quad_enable; @@ -4173,6 +4183,14 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor) { /* Init flash parameters based on MFR */ switch (JEDEC_MFR(nor->info)) { + case SNOR_MFR_ATMEL: + atmel_set_default_init(nor); + break; + + case SNOR_MFR_INTEL: + intel_set_default_init(nor); + break; + case SNOR_MFR_MACRONIX: macronix_set_default_init(nor); break; @@ -4760,18 +4778,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, if (info->flags & SPI_S3AN) nor->flags |= SNOR_F_READY_XSR_RDY; - if (info->flags & SPI_NOR_HAS_LOCK) + if (info->flags & SPI_NOR_HAS_LOCK) { nor->flags |= SNOR_F_HAS_LOCK; - - /* - * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up - * with the software protection bits set. - */ - if (JEDEC_MFR(nor->info) == SNOR_MFR_ATMEL || - JEDEC_MFR(nor->info) == SNOR_MFR_INTEL || - JEDEC_MFR(nor->info) == SNOR_MFR_SST || - nor->info->flags & SPI_NOR_HAS_LOCK) nor->params.disable_block_protection = spi_nor_clear_sr_bp; + } /* Init flash parameters based on flash_info struct and SFDP */ spi_nor_init_params(nor);