Message ID | 20230625100251.31589-3-amit.kumar-mahapatra@amd.com |
---|---|
State | Superseded |
Delegated to: | Ambarus Tudor |
Headers | show |
Series | mtd: spi-nor: Avoid setting SRWD bit in SR | expand |
Am 2023-06-25 12:02, schrieb Amit Kumar Mahapatra: > Setting the status register write disable (SRWD) bit in the status > register (SR) with WP# signal of the flash left floating or wrongly > tied to > GND (that includes internal pull-downs), will configure the SR > permanently > as read-only. If WP# signal is left floating or wrongly tied to GND, > avoid > setting SRWD bit while writing the SR during flash protection. > > Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> > --- > drivers/mtd/spi-nor/core.c | 3 +++ > drivers/mtd/spi-nor/core.h | 1 + > drivers/mtd/spi-nor/swp.c | 9 +++++++-- > 3 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 0bb0ad14a2fc..520f5ab86d2b 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -2864,6 +2864,9 @@ static void spi_nor_init_flags(struct spi_nor > *nor) > if (flags & NO_CHIP_ERASE) > nor->flags |= SNOR_F_NO_OP_CHIP_ERASE; > > + if (of_property_read_bool(np, "no-wp")) > + nor->flags |= SNOR_F_NO_WP; > + Please put it below the of_property_read_bool() which is already there, just to keep things sorted. > if (flags & SPI_NOR_RWW && nor->info->n_banks > 1 && > !nor->controller_ops) > nor->flags |= SNOR_F_RWW; > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > index 4fb5ff09c63a..55b5e7abce6e 100644 > --- a/drivers/mtd/spi-nor/core.h > +++ b/drivers/mtd/spi-nor/core.h > @@ -132,6 +132,7 @@ enum spi_nor_option_flags { > SNOR_F_SWP_IS_VOLATILE = BIT(13), > SNOR_F_RWW = BIT(14), > SNOR_F_ECC = BIT(15), > + SNOR_F_NO_WP = BIT(16), See the comment right above this enum :/ > }; > > struct spi_nor_read_command { > diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c > index 0ba716e84377..cfaba41d74d6 100644 > --- a/drivers/mtd/spi-nor/swp.c > +++ b/drivers/mtd/spi-nor/swp.c > @@ -214,8 +214,13 @@ static int spi_nor_sr_lock(struct spi_nor *nor, > loff_t ofs, uint64_t len) > > status_new = (status_old & ~mask & ~tb_mask) | val; > > - /* Disallow further writes if WP pin is asserted */ > - status_new |= SR_SRWD; > + /* > + * Disallow further writes if WP# pin is neither left floating nor > + * wrongly tied to GND(that includes internal pull-downs). nit: space missing Otherwise looks good. Thanks, -michael > + * WP# pin hard strapped to GND can be a valid use case. > + */ > + if (!(nor->flags & SNOR_F_NO_WP)) > + status_new |= SR_SRWD; > > if (!use_top) > status_new |= tb_mask;
On 6/27/23 07:14, Michael Walle wrote: > Am 2023-06-25 12:02, schrieb Amit Kumar Mahapatra: >> Setting the status register write disable (SRWD) bit in the status >> register (SR) with WP# signal of the flash left floating or wrongly tied to >> GND (that includes internal pull-downs), will configure the SR permanently >> as read-only. If WP# signal is left floating or wrongly tied to GND, avoid >> setting SRWD bit while writing the SR during flash protection. >> >> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> >> --- >> drivers/mtd/spi-nor/core.c | 3 +++ >> drivers/mtd/spi-nor/core.h | 1 + >> drivers/mtd/spi-nor/swp.c | 9 +++++++-- >> 3 files changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >> index 0bb0ad14a2fc..520f5ab86d2b 100644 >> --- a/drivers/mtd/spi-nor/core.c >> +++ b/drivers/mtd/spi-nor/core.c >> @@ -2864,6 +2864,9 @@ static void spi_nor_init_flags(struct spi_nor *nor) >> if (flags & NO_CHIP_ERASE) >> nor->flags |= SNOR_F_NO_OP_CHIP_ERASE; >> >> + if (of_property_read_bool(np, "no-wp")) >> + nor->flags |= SNOR_F_NO_WP; >> + > > Please put it below the of_property_read_bool() which is already > there, just to keep things sorted. > >> if (flags & SPI_NOR_RWW && nor->info->n_banks > 1 && >> !nor->controller_ops) >> nor->flags |= SNOR_F_RWW; >> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h >> index 4fb5ff09c63a..55b5e7abce6e 100644 >> --- a/drivers/mtd/spi-nor/core.h >> +++ b/drivers/mtd/spi-nor/core.h >> @@ -132,6 +132,7 @@ enum spi_nor_option_flags { >> SNOR_F_SWP_IS_VOLATILE = BIT(13), >> SNOR_F_RWW = BIT(14), >> SNOR_F_ECC = BIT(15), >> + SNOR_F_NO_WP = BIT(16), > > See the comment right above this enum :/ > >> }; >> >> struct spi_nor_read_command { >> diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c >> index 0ba716e84377..cfaba41d74d6 100644 >> --- a/drivers/mtd/spi-nor/swp.c >> +++ b/drivers/mtd/spi-nor/swp.c >> @@ -214,8 +214,13 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, uint64_t len) >> >> status_new = (status_old & ~mask & ~tb_mask) | val; >> >> - /* Disallow further writes if WP pin is asserted */ >> - status_new |= SR_SRWD; >> + /* >> + * Disallow further writes if WP# pin is neither left floating nor >> + * wrongly tied to GND(that includes internal pull-downs). > > nit: space missing > > Otherwise looks good. > Thanks, Michael. Amit, would be good if you can address Michael's comments and resubmit. If not, I'll amend the patch by myself when applying. Cheers, ta
Hello Tudor, > -----Original Message----- > From: Tudor Ambarus <tudor.ambarus@linaro.org> > Sent: Friday, June 30, 2023 2:18 PM > To: Michael Walle <michael@walle.cc>; Mahapatra, Amit Kumar > <amit.kumar-mahapatra@amd.com> > Cc: pratyush@kernel.org; miquel.raynal@bootlin.com; richard@nod.at; > vigneshr@ti.com; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; > conor+dt@kernel.org; git (AMD-Xilinx) <git@amd.com>; linux- > mtd@lists.infradead.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; amitrkcian2002@gmail.com > Subject: Re: [PATCH v3 2/2] mtd: spi-nor: Avoid setting SRWD bit in SR if WP# > signal not connected > > > > On 6/27/23 07:14, Michael Walle wrote: > > Am 2023-06-25 12:02, schrieb Amit Kumar Mahapatra: > >> Setting the status register write disable (SRWD) bit in the status > >> register (SR) with WP# signal of the flash left floating or wrongly > >> tied to GND (that includes internal pull-downs), will configure the > >> SR permanently as read-only. If WP# signal is left floating or > >> wrongly tied to GND, avoid setting SRWD bit while writing the SR during > flash protection. > >> > >> Signed-off-by: Amit Kumar Mahapatra <amit.kumar- > mahapatra@amd.com> > >> --- > >> drivers/mtd/spi-nor/core.c | 3 +++ > >> drivers/mtd/spi-nor/core.h | 1 + > >> drivers/mtd/spi-nor/swp.c | 9 +++++++-- > >> 3 files changed, 11 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > >> index 0bb0ad14a2fc..520f5ab86d2b 100644 > >> --- a/drivers/mtd/spi-nor/core.c > >> +++ b/drivers/mtd/spi-nor/core.c > >> @@ -2864,6 +2864,9 @@ static void spi_nor_init_flags(struct spi_nor > >> *nor) > >> if (flags & NO_CHIP_ERASE) > >> nor->flags |= SNOR_F_NO_OP_CHIP_ERASE; > >> > >> + if (of_property_read_bool(np, "no-wp")) > >> + nor->flags |= SNOR_F_NO_WP; > >> + > > > > Please put it below the of_property_read_bool() which is already > > there, just to keep things sorted. > > > >> if (flags & SPI_NOR_RWW && nor->info->n_banks > 1 && > >> !nor->controller_ops) > >> nor->flags |= SNOR_F_RWW; > >> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > >> index 4fb5ff09c63a..55b5e7abce6e 100644 > >> --- a/drivers/mtd/spi-nor/core.h > >> +++ b/drivers/mtd/spi-nor/core.h > >> @@ -132,6 +132,7 @@ enum spi_nor_option_flags { > >> SNOR_F_SWP_IS_VOLATILE = BIT(13), > >> SNOR_F_RWW = BIT(14), > >> SNOR_F_ECC = BIT(15), > >> + SNOR_F_NO_WP = BIT(16), > > > > See the comment right above this enum :/ > > > >> }; > >> > >> struct spi_nor_read_command { > >> diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c > >> index 0ba716e84377..cfaba41d74d6 100644 > >> --- a/drivers/mtd/spi-nor/swp.c > >> +++ b/drivers/mtd/spi-nor/swp.c > >> @@ -214,8 +214,13 @@ static int spi_nor_sr_lock(struct spi_nor *nor, > >> loff_t ofs, uint64_t len) > >> > >> status_new = (status_old & ~mask & ~tb_mask) | val; > >> > >> - /* Disallow further writes if WP pin is asserted */ > >> - status_new |= SR_SRWD; > >> + /* > >> + * Disallow further writes if WP# pin is neither left floating > >> +nor > >> + * wrongly tied to GND(that includes internal pull-downs). > > > > nit: space missing > > > > Otherwise looks good. > > > > Thanks, Michael. > > Amit, would be good if you can address Michael's comments and resubmit. If > not, I'll amend the patch by myself when applying. I will address Michael's comments and send the next series. Regards, Amit > > Cheers, > ta
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 0bb0ad14a2fc..520f5ab86d2b 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -2864,6 +2864,9 @@ static void spi_nor_init_flags(struct spi_nor *nor) if (flags & NO_CHIP_ERASE) nor->flags |= SNOR_F_NO_OP_CHIP_ERASE; + if (of_property_read_bool(np, "no-wp")) + nor->flags |= SNOR_F_NO_WP; + if (flags & SPI_NOR_RWW && nor->info->n_banks > 1 && !nor->controller_ops) nor->flags |= SNOR_F_RWW; diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h index 4fb5ff09c63a..55b5e7abce6e 100644 --- a/drivers/mtd/spi-nor/core.h +++ b/drivers/mtd/spi-nor/core.h @@ -132,6 +132,7 @@ enum spi_nor_option_flags { SNOR_F_SWP_IS_VOLATILE = BIT(13), SNOR_F_RWW = BIT(14), SNOR_F_ECC = BIT(15), + SNOR_F_NO_WP = BIT(16), }; struct spi_nor_read_command { diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c index 0ba716e84377..cfaba41d74d6 100644 --- a/drivers/mtd/spi-nor/swp.c +++ b/drivers/mtd/spi-nor/swp.c @@ -214,8 +214,13 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, uint64_t len) status_new = (status_old & ~mask & ~tb_mask) | val; - /* Disallow further writes if WP pin is asserted */ - status_new |= SR_SRWD; + /* + * Disallow further writes if WP# pin is neither left floating nor + * wrongly tied to GND(that includes internal pull-downs). + * WP# pin hard strapped to GND can be a valid use case. + */ + if (!(nor->flags & SNOR_F_NO_WP)) + status_new |= SR_SRWD; if (!use_top) status_new |= tb_mask;
Setting the status register write disable (SRWD) bit in the status register (SR) with WP# signal of the flash left floating or wrongly tied to GND (that includes internal pull-downs), will configure the SR permanently as read-only. If WP# signal is left floating or wrongly tied to GND, avoid setting SRWD bit while writing the SR during flash protection. Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> --- drivers/mtd/spi-nor/core.c | 3 +++ drivers/mtd/spi-nor/core.h | 1 + drivers/mtd/spi-nor/swp.c | 9 +++++++-- 3 files changed, 11 insertions(+), 2 deletions(-)