| Message ID | 1345444344-31340-1-git-send-email-voice.shen@atmel.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | Andreas Bießmann |
| Headers | show |
Dear Bo Shen, On 20.08.2012 08:32, Bo Shen wrote: > Using common spi flash operation function to replace private operation > funtion > > This patch is based on http://patchwork.ozlabs.org/patch/177896/ > which has been merged by Mike frysinger Mike, do you think this is a fix? Should it go into 2012.10? Will you take it? > Signed-off-by: Bo Shen <voice.shen@atmel.com> > --- > drivers/mtd/spi/atmel.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/mtd/spi/atmel.c b/drivers/mtd/spi/atmel.c > index 89ebe9d..006f6d5 100644 > --- a/drivers/mtd/spi/atmel.c > +++ b/drivers/mtd/spi/atmel.c > @@ -518,13 +518,19 @@ struct spi_flash *spi_flash_probe_atmel(struct spi_slave *spi, u8 *idcode) > asf->flash.erase = dataflash_erase_p2; > } > > + asf->flash.page_size = page_size; > + asf->flash.sector_size = page_size; > break; > > case DF_FAMILY_AT26F: > case DF_FAMILY_AT26DF: > asf->flash.read = spi_flash_cmd_read_fast; > - asf->flash.write = dataflash_write_p2; > - asf->flash.erase = dataflash_erase_p2; > + asf->flash.write = spi_flash_cmd_write_multi; > + asf->flash.erase = spi_flash_cmd_erase; > + asf->flash.page_size = page_size; > + asf->flash.sector_size = 4096; why do you take fixed value for sector size here? > + /* clear SPRL# bit for locked flash */ > + spi_flash_cmd_write_status(&asf->flash, 0); > break; > > default: > @@ -532,7 +538,6 @@ struct spi_flash *spi_flash_probe_atmel(struct spi_slave *spi, u8 *idcode) > goto err; > } > > - asf->flash.sector_size = page_size; > asf->flash.size = page_size * params->pages_per_block > * params->blocks_per_sector > * params->nr_sectors; And here you correlate number of sectors with page size ... but we may have page_size != sector_size for some at26 devices, aren't we? Best regards Andreas Bießmann
On Tuesday 21 August 2012 07:26:27 Andreas Bießmann wrote: > On 20.08.2012 08:32, Bo Shen wrote: > > Using common spi flash operation function to replace private operation > > funtion > > > > This patch is based on http://patchwork.ozlabs.org/patch/177896/ > > which has been merged by Mike frysinger > > Mike, do you think this is a fix? Should it go into 2012.10? Will you > take it? i'll take care of merging the changes to drivers/mtd/spi/*, but always happy to see people help to review the changes :). especially with the atmel driver as their dataflashes have the unique command set :(. -mike
Hi Andreas, On 8/21/2012 19:26, Andreas Bießmann wrote: > Dear Bo Shen, > > On 20.08.2012 08:32, Bo Shen wrote: >> Using common spi flash operation function to replace private operation >> funtion >> >> This patch is based on http://patchwork.ozlabs.org/patch/177896/ >> which has been merged by Mike frysinger > > Mike, do you think this is a fix? Should it go into 2012.10? Will you > take it? > >> Signed-off-by: Bo Shen <voice.shen@atmel.com> >> --- >> drivers/mtd/spi/atmel.c | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/mtd/spi/atmel.c b/drivers/mtd/spi/atmel.c >> index 89ebe9d..006f6d5 100644 >> --- a/drivers/mtd/spi/atmel.c >> +++ b/drivers/mtd/spi/atmel.c >> @@ -518,13 +518,19 @@ struct spi_flash *spi_flash_probe_atmel(struct spi_slave *spi, u8 *idcode) >> asf->flash.erase = dataflash_erase_p2; >> } >> >> + asf->flash.page_size = page_size; >> + asf->flash.sector_size = page_size; >> break; >> >> case DF_FAMILY_AT26F: >> case DF_FAMILY_AT26DF: >> asf->flash.read = spi_flash_cmd_read_fast; >> - asf->flash.write = dataflash_write_p2; >> - asf->flash.erase = dataflash_erase_p2; >> + asf->flash.write = spi_flash_cmd_write_multi; >> + asf->flash.erase = spi_flash_cmd_erase; >> + asf->flash.page_size = page_size; >> + asf->flash.sector_size = 4096; > > why do you take fixed value for sector size here? The fixed number fit for both AT25 and AT26 serials. So, I take it. Actually, we can calculate it as: asf->flash.sector_size = page_size * params->pages_per_block * params->blocks_per_sector So, may I need to change it? > >> + /* clear SPRL# bit for locked flash */ >> + spi_flash_cmd_write_status(&asf->flash, 0); >> break; >> >> default: >> @@ -532,7 +538,6 @@ struct spi_flash *spi_flash_probe_atmel(struct spi_slave *spi, u8 *idcode) >> goto err; >> } >> >> - asf->flash.sector_size = page_size; >> asf->flash.size = page_size * params->pages_per_block >> * params->blocks_per_sector >> * params->nr_sectors; > > And here you correlate number of sectors with page size ... but we may > have page_size != sector_size for some at26 devices, aren't we? This is from old driver and this line is moved away. For at26 devices, it is used the fixed number 4096 for sector size. Please take following as a reference. --------------------------------------------- case DF_FAMILY_AT26F: case DF_FAMILY_AT26DF: asf->flash.read = spi_flash_cmd_read_fast; - asf->flash.write = dataflash_write_p2; - asf->flash.erase = dataflash_erase_p2; + asf->flash.write = spi_flash_cmd_write_multi; + asf->flash.erase = spi_flash_cmd_erase; + asf->flash.page_size = page_size; + asf->flash.sector_size = 4096; --------------------------------------------- > > Best regards > > Andreas Bießmann >
Hi Mike, On 8/22/2012 2:55, Mike Frysinger wrote: > On Tuesday 21 August 2012 07:26:27 Andreas Bießmann wrote: >> On 20.08.2012 08:32, Bo Shen wrote: >>> Using common spi flash operation function to replace private operation >>> funtion >>> >>> This patch is based on http://patchwork.ozlabs.org/patch/177896/ >>> which has been merged by Mike frysinger >> >> Mike, do you think this is a fix? Should it go into 2012.10? Will you >> take it? > > i'll take care of merging the changes to drivers/mtd/spi/*, but always happy > to see people help to review the changes :). > > especially with the atmel driver as their dataflashes have the unique command > set :(. For atmel at25 and at26 series spi flash, they can support standard spi flash operation. So, this patch intent to use common spi flash operation. Will this patch be took? > -mike >
Dear Bo Shen, On 22.08.2012 03:33, Bo Shen wrote: > Hi Andreas, > > On 8/21/2012 19:26, Andreas Bießmann wrote: >> Dear Bo Shen, >> >> On 20.08.2012 08:32, Bo Shen wrote: >>> Using common spi flash operation function to replace private operation >>> funtion >>> >>> This patch is based on http://patchwork.ozlabs.org/patch/177896/ >>> which has been merged by Mike frysinger >> >> Mike, do you think this is a fix? Should it go into 2012.10? Will you >> take it? >> >>> Signed-off-by: Bo Shen <voice.shen@atmel.com> >>> --- >>> drivers/mtd/spi/atmel.c | 11 ++++++++--- >>> 1 file changed, 8 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/mtd/spi/atmel.c b/drivers/mtd/spi/atmel.c >>> index 89ebe9d..006f6d5 100644 >>> --- a/drivers/mtd/spi/atmel.c >>> +++ b/drivers/mtd/spi/atmel.c >>> @@ -518,13 +518,19 @@ struct spi_flash *spi_flash_probe_atmel(struct >>> spi_slave *spi, u8 *idcode) >>> asf->flash.erase = dataflash_erase_p2; >>> } >>> >>> + asf->flash.page_size = page_size; >>> + asf->flash.sector_size = page_size; >>> break; >>> >>> case DF_FAMILY_AT26F: >>> case DF_FAMILY_AT26DF: >>> asf->flash.read = spi_flash_cmd_read_fast; >>> - asf->flash.write = dataflash_write_p2; >>> - asf->flash.erase = dataflash_erase_p2; >>> + asf->flash.write = spi_flash_cmd_write_multi; >>> + asf->flash.erase = spi_flash_cmd_erase; >>> + asf->flash.page_size = page_size; >>> + asf->flash.sector_size = 4096; >> >> why do you take fixed value for sector size here? > > The fixed number fit for both AT25 and AT26 serials. So, I take it. > > Actually, we can calculate it as: q> asf->flash.sector_size = page_size * params->pages_per_block * > params->blocks_per_sector > So, may I need to change it? I got it. The current driver for AT45 is written to only support page erase, therefore the page_size == sector_size. The AT26 on the other hand will now use the generic API which uses sector erase instead. Therefore we have different values at all, am I right? The only supported DF of at26 type is currently [1] which in fact has 4KiB erase size. Unfortunately others (at least [2]) have another layout. But this could be adopted if such a chip is supported in the future. >> >>> + /* clear SPRL# bit for locked flash */ >>> + spi_flash_cmd_write_status(&asf->flash, 0); >>> break; >>> >>> default: >>> @@ -532,7 +538,6 @@ struct spi_flash *spi_flash_probe_atmel(struct >>> spi_slave *spi, u8 *idcode) >>> goto err; >>> } >>> >>> - asf->flash.sector_size = page_size; >>> asf->flash.size = page_size * params->pages_per_block >>> * params->blocks_per_sector >>> * params->nr_sectors; >> >> And here you correlate number of sectors with page size ... but we may >> have page_size != sector_size for some at26 devices, aren't we? > > This is from old driver and this line is moved away. > For at26 devices, it is used the fixed number 4096 for sector size. > Please take following as a reference. > --------------------------------------------- > case DF_FAMILY_AT26F: > case DF_FAMILY_AT26DF: > asf->flash.read = spi_flash_cmd_read_fast; > - asf->flash.write = dataflash_write_p2; > - asf->flash.erase = dataflash_erase_p2; > + asf->flash.write = spi_flash_cmd_write_multi; > + asf->flash.erase = spi_flash_cmd_erase; > + asf->flash.page_size = page_size; > + asf->flash.sector_size = 4096; > --------------------------------------------- Yea, I got that already. The only thing I asked why you correlate these sector related parameters (blocks_per_sector and nr_sectors) with the page_size. I think this change is Ok. Acked-by: Andreas Bießmann <andreas.devel@googlemail.com> @Mike: As I understand you I should take patch #2 to #4 while you will take this one, am I right? [1] http://www.atmel.com/Images/doc3633.pdf [2] http://www.atmel.com/Images/doc3495.pdf
Dear Bo Shen, On 20.08.2012 08:32, Bo Shen wrote: > Using common spi flash operation function to replace private operation > funtion > > This patch is based on http://patchwork.ozlabs.org/patch/177896/ > which has been merged by Mike frysinger > > Signed-off-by: Bo Shen <voice.shen@atmel.com> > --- applied to u-boot-atmel/master, thanks! Best regards Andreas Bießmann
diff --git a/drivers/mtd/spi/atmel.c b/drivers/mtd/spi/atmel.c index 89ebe9d..006f6d5 100644 --- a/drivers/mtd/spi/atmel.c +++ b/drivers/mtd/spi/atmel.c @@ -518,13 +518,19 @@ struct spi_flash *spi_flash_probe_atmel(struct spi_slave *spi, u8 *idcode) asf->flash.erase = dataflash_erase_p2; } + asf->flash.page_size = page_size; + asf->flash.sector_size = page_size; break; case DF_FAMILY_AT26F: case DF_FAMILY_AT26DF: asf->flash.read = spi_flash_cmd_read_fast; - asf->flash.write = dataflash_write_p2; - asf->flash.erase = dataflash_erase_p2; + asf->flash.write = spi_flash_cmd_write_multi; + asf->flash.erase = spi_flash_cmd_erase; + asf->flash.page_size = page_size; + asf->flash.sector_size = 4096; + /* clear SPRL# bit for locked flash */ + spi_flash_cmd_write_status(&asf->flash, 0); break; default: @@ -532,7 +538,6 @@ struct spi_flash *spi_flash_probe_atmel(struct spi_slave *spi, u8 *idcode) goto err; } - asf->flash.sector_size = page_size; asf->flash.size = page_size * params->pages_per_block * params->blocks_per_sector * params->nr_sectors;
Using common spi flash operation function to replace private operation funtion This patch is based on http://patchwork.ozlabs.org/patch/177896/ which has been merged by Mike frysinger Signed-off-by: Bo Shen <voice.shen@atmel.com> --- drivers/mtd/spi/atmel.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)