| Message ID | 1444662074-2424-6-git-send-email-jteki@openedev.com |
|---|---|
| State | Accepted |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
On 12 October 2015 at 20:30, Jagan Teki <jteki@openedev.com> wrote: > Use the flash->flags for generic usage, not only for dm-spi-flash, > this will be used for future flag additions. > > Signed-off-by: Jagan Teki <jteki@openedev.com> > [Correct the spi flash flags detect logic] > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > Tested-by: Bin Meng <bmeng.cn@gmail.com> > --- Applied to u-boot-spi/master
Hi Jagan, On Mon, Oct 12, 2015 at 11:00 PM, Jagan Teki <jteki@openedev.com> wrote: > Use the flash->flags for generic usage, not only for dm-spi-flash, > this will be used for future flag additions. > > Signed-off-by: Jagan Teki <jteki@openedev.com> > [Correct the spi flash flags detect logic] > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > Tested-by: Bin Meng <bmeng.cn@gmail.com> > --- > Changes for v4: > - Fixed SNOR_F_SST_WR > Changes for v3, v2: > - none > It turns out this patch breaks the Intel Crown Bay SPI flash. I compared my original submitted patch with this patch you applied, and surprisingly found there are differences ... My version is http://patchwork.ozlabs.org/patch/517704/ You version below seems to modify some places which you thought would be correct, but that's unfortunately wrong. If you reworked my patch, I think you should remove at least my "Tested-by:" tag and ask me to retest. Can you please help me understand what happened? I expected that you should just grab my version and include it in your patch series. > drivers/mtd/spi/sf_internal.h | 4 ++++ > drivers/mtd/spi/sf_probe.c | 10 +++++----- > include/spi_flash.h | 4 ++-- > 3 files changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h > index 9c95d56..53998fc 100644 > --- a/drivers/mtd/spi/sf_internal.h > +++ b/drivers/mtd/spi/sf_internal.h > @@ -51,6 +51,10 @@ enum { > > #define SST_WR (SST_BP | SST_WP) > > +enum spi_nor_option_flags { > + SNOR_F_SST_WR = (1 << 0), > +}; > + > #define SPI_FLASH_3B_ADDR_LEN 3 > #define SPI_FLASH_CMD_LEN (1 + SPI_FLASH_3B_ADDR_LEN) > #define SPI_FLASH_16MB_BOUN 0x1000000 > diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c > index f17ec17..2634e90 100644 > --- a/drivers/mtd/spi/sf_probe.c > +++ b/drivers/mtd/spi/sf_probe.c > @@ -163,15 +163,15 @@ static int spi_flash_validate_params(struct spi_slave *spi, u8 *idcode, > flash->name = params->name; > flash->memory_map = spi->memory_map; > flash->dual_flash = flash->spi->option; > -#ifdef CONFIG_DM_SPI_FLASH > - flash->flags = params->flags; > -#endif > > /* Assign spi_flash ops */ > #ifndef CONFIG_DM_SPI_FLASH > flash->write = spi_flash_cmd_write_ops; > #if defined(CONFIG_SPI_FLASH_SST) > - if (params->flags & SST_WR) { > + if (params->flags & SST_WR) > + flash->flags |= SNOR_F_SST_WR; > + > + if (params->flags & SNOR_F_SST_WR) { > if (flash->spi->op_mode_tx & SPI_OPM_TX_BP) > flash->write = sst_write_bp; > else > @@ -466,7 +466,7 @@ int spi_flash_std_write(struct udevice *dev, u32 offset, size_t len, > struct spi_flash *flash = dev_get_uclass_priv(dev); > > #if defined(CONFIG_SPI_FLASH_SST) > - if (flash->flags & SST_WR) { > + if (flash->flags & SNOR_F_SST_WR) { > if (flash->spi->op_mode_tx & SPI_OPM_TX_BP) > return sst_write_bp(flash, offset, len, buf); > else > diff --git a/include/spi_flash.h b/include/spi_flash.h > index 3b2d555..8d85468 100644 > --- a/include/spi_flash.h > +++ b/include/spi_flash.h > @@ -38,10 +38,10 @@ struct spi_slave; > * > * @spi: SPI slave > * @dev: SPI flash device > - * @flags: Indication of spi flash flags > * @name: Name of SPI flash > * @dual_flash: Indicates dual flash memories - dual stacked, parallel > * @shift: Flash shift useful in dual parallel > + * @flags: Indication of spi flash flags > * @size: Total flash size > * @page_size: Write (page) size > * @sector_size: Sector size > @@ -67,11 +67,11 @@ struct spi_flash { > struct spi_slave *spi; > #ifdef CONFIG_DM_SPI_FLASH > struct udevice *dev; > - u16 flags; > #endif > const char *name; > u8 dual_flash; > u8 shift; > + u16 flags; > > u32 size; > u32 page_size; > -- Regards, Bin
Hi Jagan, On Mon, Nov 16, 2015 at 10:59 AM, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Jagan, > > On Mon, Oct 12, 2015 at 11:00 PM, Jagan Teki <jteki@openedev.com> wrote: >> Use the flash->flags for generic usage, not only for dm-spi-flash, >> this will be used for future flag additions. >> >> Signed-off-by: Jagan Teki <jteki@openedev.com> >> [Correct the spi flash flags detect logic] >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> Tested-by: Bin Meng <bmeng.cn@gmail.com> >> --- >> Changes for v4: >> - Fixed SNOR_F_SST_WR >> Changes for v3, v2: >> - none >> > > It turns out this patch breaks the Intel Crown Bay SPI flash. I > compared my original submitted patch with this patch you applied, and > surprisingly found there are differences ... > > My version is http://patchwork.ozlabs.org/patch/517704/ > You version below seems to modify some places which you thought would > be correct, but that's unfortunately wrong. If you reworked my patch, > I think you should remove at least my "Tested-by:" tag and ask me to > retest. > > Can you please help me understand what happened? I expected that you > should just grab my version and include it in your patch series. > I see you were replying yesterday, but did not see any response on this one. [snip] Regards, Bin
On Wed, Nov 18, 2015 at 09:14:01AM +0800, Bin Meng wrote: > Hi Jagan, > > On Mon, Nov 16, 2015 at 10:59 AM, Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi Jagan, > > > > On Mon, Oct 12, 2015 at 11:00 PM, Jagan Teki <jteki@openedev.com> wrote: > >> Use the flash->flags for generic usage, not only for dm-spi-flash, > >> this will be used for future flag additions. > >> > >> Signed-off-by: Jagan Teki <jteki@openedev.com> > >> [Correct the spi flash flags detect logic] > >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > >> Tested-by: Bin Meng <bmeng.cn@gmail.com> > >> --- > >> Changes for v4: > >> - Fixed SNOR_F_SST_WR > >> Changes for v3, v2: > >> - none > >> > > > > It turns out this patch breaks the Intel Crown Bay SPI flash. I > > compared my original submitted patch with this patch you applied, and > > surprisingly found there are differences ... > > > > My version is http://patchwork.ozlabs.org/patch/517704/ > > You version below seems to modify some places which you thought would > > be correct, but that's unfortunately wrong. If you reworked my patch, > > I think you should remove at least my "Tested-by:" tag and ask me to > > retest. > > > > Can you please help me understand what happened? I expected that you > > should just grab my version and include it in your patch series. > > > > I see you were replying yesterday, but did not see any response on this one. Modifying patches between the mailing list and applying, outside of making things apply again or extremely obvious fixes is really really frowned upon.
On 16 November 2015 at 08:29, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Jagan, > > On Mon, Oct 12, 2015 at 11:00 PM, Jagan Teki <jteki@openedev.com> wrote: >> Use the flash->flags for generic usage, not only for dm-spi-flash, >> this will be used for future flag additions. >> >> Signed-off-by: Jagan Teki <jteki@openedev.com> >> [Correct the spi flash flags detect logic] >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> Tested-by: Bin Meng <bmeng.cn@gmail.com> >> --- >> Changes for v4: >> - Fixed SNOR_F_SST_WR >> Changes for v3, v2: >> - none >> > > It turns out this patch breaks the Intel Crown Bay SPI flash. I > compared my original submitted patch with this patch you applied, and > surprisingly found there are differences ... > > My version is http://patchwork.ozlabs.org/patch/517704/ > You version below seems to modify some places which you thought would > be correct, but that's unfortunately wrong. If you reworked my patch, > I think you should remove at least my "Tested-by:" tag and ask me to > retest. > > Can you please help me understand what happened? I expected that you > should just grab my version and include it in your patch series. I have moved flash->flags |= SNOR_F_SST_WR; assignment to #ifdef *_SST since it's been part of SST flash, but seems like dm will require that flags to call the respective sst write ops - it's a mistake. I thought you may comment the same since I CCed you. I will grab the fix patch you sent. > >> drivers/mtd/spi/sf_internal.h | 4 ++++ >> drivers/mtd/spi/sf_probe.c | 10 +++++----- >> include/spi_flash.h | 4 ++-- >> 3 files changed, 11 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h >> index 9c95d56..53998fc 100644 >> --- a/drivers/mtd/spi/sf_internal.h >> +++ b/drivers/mtd/spi/sf_internal.h >> @@ -51,6 +51,10 @@ enum { >> >> #define SST_WR (SST_BP | SST_WP) >> >> +enum spi_nor_option_flags { >> + SNOR_F_SST_WR = (1 << 0), >> +}; >> + >> #define SPI_FLASH_3B_ADDR_LEN 3 >> #define SPI_FLASH_CMD_LEN (1 + SPI_FLASH_3B_ADDR_LEN) >> #define SPI_FLASH_16MB_BOUN 0x1000000 >> diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c >> index f17ec17..2634e90 100644 >> --- a/drivers/mtd/spi/sf_probe.c >> +++ b/drivers/mtd/spi/sf_probe.c >> @@ -163,15 +163,15 @@ static int spi_flash_validate_params(struct spi_slave *spi, u8 *idcode, >> flash->name = params->name; >> flash->memory_map = spi->memory_map; >> flash->dual_flash = flash->spi->option; >> -#ifdef CONFIG_DM_SPI_FLASH >> - flash->flags = params->flags; >> -#endif >> >> /* Assign spi_flash ops */ >> #ifndef CONFIG_DM_SPI_FLASH >> flash->write = spi_flash_cmd_write_ops; >> #if defined(CONFIG_SPI_FLASH_SST) >> - if (params->flags & SST_WR) { >> + if (params->flags & SST_WR) >> + flash->flags |= SNOR_F_SST_WR; >> + >> + if (params->flags & SNOR_F_SST_WR) { >> if (flash->spi->op_mode_tx & SPI_OPM_TX_BP) >> flash->write = sst_write_bp; >> else >> @@ -466,7 +466,7 @@ int spi_flash_std_write(struct udevice *dev, u32 offset, size_t len, >> struct spi_flash *flash = dev_get_uclass_priv(dev); >> >> #if defined(CONFIG_SPI_FLASH_SST) >> - if (flash->flags & SST_WR) { >> + if (flash->flags & SNOR_F_SST_WR) { >> if (flash->spi->op_mode_tx & SPI_OPM_TX_BP) >> return sst_write_bp(flash, offset, len, buf); >> else >> diff --git a/include/spi_flash.h b/include/spi_flash.h >> index 3b2d555..8d85468 100644 >> --- a/include/spi_flash.h >> +++ b/include/spi_flash.h >> @@ -38,10 +38,10 @@ struct spi_slave; >> * >> * @spi: SPI slave >> * @dev: SPI flash device >> - * @flags: Indication of spi flash flags >> * @name: Name of SPI flash >> * @dual_flash: Indicates dual flash memories - dual stacked, parallel >> * @shift: Flash shift useful in dual parallel >> + * @flags: Indication of spi flash flags >> * @size: Total flash size >> * @page_size: Write (page) size >> * @sector_size: Sector size >> @@ -67,11 +67,11 @@ struct spi_flash { >> struct spi_slave *spi; >> #ifdef CONFIG_DM_SPI_FLASH >> struct udevice *dev; >> - u16 flags; >> #endif >> const char *name; >> u8 dual_flash; >> u8 shift; >> + u16 flags; >> >> u32 size; >> u32 page_size; >> -- thanks!
diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h index 9c95d56..53998fc 100644 --- a/drivers/mtd/spi/sf_internal.h +++ b/drivers/mtd/spi/sf_internal.h @@ -51,6 +51,10 @@ enum { #define SST_WR (SST_BP | SST_WP) +enum spi_nor_option_flags { + SNOR_F_SST_WR = (1 << 0), +}; + #define SPI_FLASH_3B_ADDR_LEN 3 #define SPI_FLASH_CMD_LEN (1 + SPI_FLASH_3B_ADDR_LEN) #define SPI_FLASH_16MB_BOUN 0x1000000 diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c index f17ec17..2634e90 100644 --- a/drivers/mtd/spi/sf_probe.c +++ b/drivers/mtd/spi/sf_probe.c @@ -163,15 +163,15 @@ static int spi_flash_validate_params(struct spi_slave *spi, u8 *idcode, flash->name = params->name; flash->memory_map = spi->memory_map; flash->dual_flash = flash->spi->option; -#ifdef CONFIG_DM_SPI_FLASH - flash->flags = params->flags; -#endif /* Assign spi_flash ops */ #ifndef CONFIG_DM_SPI_FLASH flash->write = spi_flash_cmd_write_ops; #if defined(CONFIG_SPI_FLASH_SST) - if (params->flags & SST_WR) { + if (params->flags & SST_WR) + flash->flags |= SNOR_F_SST_WR; + + if (params->flags & SNOR_F_SST_WR) { if (flash->spi->op_mode_tx & SPI_OPM_TX_BP) flash->write = sst_write_bp; else @@ -466,7 +466,7 @@ int spi_flash_std_write(struct udevice *dev, u32 offset, size_t len, struct spi_flash *flash = dev_get_uclass_priv(dev); #if defined(CONFIG_SPI_FLASH_SST) - if (flash->flags & SST_WR) { + if (flash->flags & SNOR_F_SST_WR) { if (flash->spi->op_mode_tx & SPI_OPM_TX_BP) return sst_write_bp(flash, offset, len, buf); else diff --git a/include/spi_flash.h b/include/spi_flash.h index 3b2d555..8d85468 100644 --- a/include/spi_flash.h +++ b/include/spi_flash.h @@ -38,10 +38,10 @@ struct spi_slave; * * @spi: SPI slave * @dev: SPI flash device - * @flags: Indication of spi flash flags * @name: Name of SPI flash * @dual_flash: Indicates dual flash memories - dual stacked, parallel * @shift: Flash shift useful in dual parallel + * @flags: Indication of spi flash flags * @size: Total flash size * @page_size: Write (page) size * @sector_size: Sector size @@ -67,11 +67,11 @@ struct spi_flash { struct spi_slave *spi; #ifdef CONFIG_DM_SPI_FLASH struct udevice *dev; - u16 flags; #endif const char *name; u8 dual_flash; u8 shift; + u16 flags; u32 size; u32 page_size;