| Message ID | 1592312547-19239-3-git-send-email-yangyicong@hisilicon.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | None | expand |
On 6/16/20 4:02 PM, Yicong Yang wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > If the flash's quad mode is enabled, it'll remain in the quad mode when > it's removed. If we drive the flash next time in SPI/Dual mode, then > problem occurs as the flash's quad enable bit is not cleared. Please describe the problems that occur. When QE bit is one the flash operates in Standard/Dual/Quad SPI modes. WP# and RESET#/HOLD# are affected as they change their functionality to IO2 and IO3 when QE is 1. Is there anything else? While I find the intention good, there might be some problems here: 1/ w25q jvm variants come with QE "fixed" to 1. This probably means that QE is not writable, and a writing of QE to zero will be ignored, but we have to check. 2/S25FS128S: CR1NV[1] can set the default power-on state for the CR1V[1] to 1, i.e. QE to be set to 1 at power-on by default. The logic here complicates a bit, and maybe we'll have to amend the patch. > > Disable the quad mode in spi_nor_restore(), the flash will leave > quad mode when remove. This will make sure the flash always enter the > correct mode when loaded. s/correct/ Standard/Dual SPI Cheers, ta
On 02/07/20 11:02AM, Tudor.Ambarus@microchip.com wrote: > On 6/16/20 4:02 PM, Yicong Yang wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > If the flash's quad mode is enabled, it'll remain in the quad mode when > > it's removed. If we drive the flash next time in SPI/Dual mode, then > > problem occurs as the flash's quad enable bit is not cleared. > > Please describe the problems that occur. When QE bit is one the flash > operates in Standard/Dual/Quad SPI modes. WP# and RESET#/HOLD# are > affected as they change their functionality to IO2 and IO3 when QE > is 1. Is there anything else? IIUC if we do anything that introduces a state on the flash, we want to clear that state up on restore. That's what we (will) do for 8D mode and for 4-byte addressing mode. Does that not apply here? > While I find the intention good, there might be some problems here: > 1/ w25q jvm variants come with QE "fixed" to 1. This probably means > that QE is not writable, and a writing of QE to zero will be ignored, > but we have to check. In that case they shouldn't have a quad_enable() hook, no? > 2/S25FS128S: CR1NV[1] can set the default power-on state for the > CR1V[1] to 1, i.e. QE to be set to 1 at power-on by default. The > logic here complicates a bit, and maybe we'll have to amend the > patch. > > > > > Disable the quad mode in spi_nor_restore(), the flash will leave > > quad mode when remove. This will make sure the flash always enter the > > correct mode when loaded. > s/correct/ Standard/Dual SPI > > Cheers, > ta
On 7/3/20 2:19 PM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 02/07/20 11:02AM, Tudor.Ambarus@microchip.com wrote: >> On 6/16/20 4:02 PM, Yicong Yang wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> If the flash's quad mode is enabled, it'll remain in the quad mode when >>> it's removed. If we drive the flash next time in SPI/Dual mode, then >>> problem occurs as the flash's quad enable bit is not cleared. >> >> Please describe the problems that occur. When QE bit is one the flash >> operates in Standard/Dual/Quad SPI modes. WP# and RESET#/HOLD# are >> affected as they change their functionality to IO2 and IO3 when QE >> is 1. Is there anything else? > > IIUC if we do anything that introduces a state on the flash, we want to > clear that state up on restore. That's what we (will) do for 8D mode and correct > for 4-byte addressing mode. Does that not apply here? yes, it does. I've just asked Yicong to describe in the commit message the problems that he encounters, for better understanding. Standard and Dual modes should still work with QE = 1. The only problem that I see is that WP# and RESET#/HOLD# are changing their functionality to IO2 and IO3 when QE is 1. Is there anything else that I miss? > >> While I find the intention good, there might be some problems here: >> 1/ w25q jvm variants come with QE "fixed" to 1. This probably means >> that QE is not writable, and a writing of QE to zero will be ignored, >> but we have to check. > > In that case they shouldn't have a quad_enable() hook, no? Right. Although this scenario should be a false positive, probably the write of QE bit is ignored. There is a superfluous write of QE indeed, but maybe we can live with it. > >> 2/S25FS128S: CR1NV[1] can set the default power-on state for the >> CR1V[1] to 1, i.e. QE to be set to 1 at power-on by default. The >> logic here complicates a bit, and maybe we'll have to amend the >> patch. >> We can come with a patch on top of these for 2/. Yicong, please address the minor comments and resubmit. Cheers. >>> >>> Disable the quad mode in spi_nor_restore(), the flash will leave >>> quad mode when remove. This will make sure the flash always enter the >>> correct mode when loaded. >> s/correct/ Standard/Dual SPI >> >> Cheers, >> ta > > -- > Regards, > Pratyush Yadav > Texas Instruments India >
Hi, Thanks for reviewing the patch. On 2020/7/3 19:52, Tudor.Ambarus@microchip.com wrote: > On 7/3/20 2:19 PM, Pratyush Yadav wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> On 02/07/20 11:02AM, Tudor.Ambarus@microchip.com wrote: >>> On 6/16/20 4:02 PM, Yicong Yang wrote: >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>>> >>>> If the flash's quad mode is enabled, it'll remain in the quad mode when >>>> it's removed. If we drive the flash next time in SPI/Dual mode, then >>>> problem occurs as the flash's quad enable bit is not cleared. >>> Please describe the problems that occur. When QE bit is one the flash >>> operates in Standard/Dual/Quad SPI modes. WP# and RESET#/HOLD# are >>> affected as they change their functionality to IO2 and IO3 when QE >>> is 1. Is there anything else? >> IIUC if we do anything that introduces a state on the flash, we want to >> clear that state up on restore. That's what we (will) do for 8D mode and > correct > >> for 4-byte addressing mode. Does that not apply here? > yes, it does. I've just asked Yicong to describe in the commit message > the problems that he encounters, for better understanding. Standard and > Dual modes should still work with QE = 1. The only problem that I see > is that WP# and RESET#/HOLD# are changing their functionality to IO2 > and IO3 when QE is 1. Is there anything else that I miss? I think I have mixup the issues. The problem I met is when I load the driver in Quad mode first and reload it in Standard SPI/Dual mode, and tested the flash's read/write but I got mirrored data which differs from what I wrote. But seems the QE bit won't cause this, (thanks for your illustration and it may not be the same issue) so it seems improper to mention it here and I'll reword the commit. (PS: the flash I tested is Cypress s25fs128s1 with spi-hisi-sfc-v3xx controller. ) Thanks, Yicong >>> While I find the intention good, there might be some problems here: >>> 1/ w25q jvm variants come with QE "fixed" to 1. This probably means >>> that QE is not writable, and a writing of QE to zero will be ignored, >>> but we have to check. >> In that case they shouldn't have a quad_enable() hook, no? > Right. Although this scenario should be a false positive, probably the > write of QE bit is ignored. There is a superfluous write of QE indeed, > but maybe we can live with it. > >>> 2/S25FS128S: CR1NV[1] can set the default power-on state for the >>> CR1V[1] to 1, i.e. QE to be set to 1 at power-on by default. The >>> logic here complicates a bit, and maybe we'll have to amend the >>> patch. >>> > We can come with a patch on top of these for 2/. Yicong, please address > the minor comments and resubmit. > Cheers. > >>>> Disable the quad mode in spi_nor_restore(), the flash will leave >>>> quad mode when remove. This will make sure the flash always enter the >>>> correct mode when loaded. >>> s/correct/ Standard/Dual SPI >>> >>> Cheers, >>> ta >> -- >> Regards, >> Pratyush Yadav >> Texas Instruments India >>
Am Di., 16. Juni 2020 um 15:03 Uhr schrieb Yicong Yang <yangyicong@hisilicon.com>: > If the flash's quad mode is enabled, it'll remain in the quad mode when > it's removed. If we drive the flash next time in SPI/Dual mode, then > problem occurs as the flash's quad enable bit is not cleared. On flash devices with a non-volatile quad enable bit (we use S25FL512S) this will wear out the quad enable bit as on every boot the bit is set and reset on shutdown. Or do I miss something here? > Disable the quad mode in spi_nor_restore(), the flash will leave > quad mode when remove. This will make sure the flash always enter the > correct mode when loaded. We have a system which relies on an enabled quad mode on boot (bootloader uses quad mode without enabling it) so using this patch will prevent our device from booting. Regards, Matthias
Hi, On 01/09/20 08:16AM, Matthias Weißer wrote: > Am Di., 16. Juni 2020 um 15:03 Uhr schrieb Yicong Yang > <yangyicong@hisilicon.com>: > > If the flash's quad mode is enabled, it'll remain in the quad mode when > > it's removed. If we drive the flash next time in SPI/Dual mode, then > > problem occurs as the flash's quad enable bit is not cleared. > > On flash devices with a non-volatile quad enable bit (we use S25FL512S) > this will wear out the quad enable bit as on every boot the bit is > set and reset on shutdown. Or do I miss something here? Yes, I think it will. If you always want the bit enabled (and it being non-volatile suggests that is the intended use), why not set nor->params->quad_enable() to NULL? That way it won't be touched at all, neither on boot nor on shutdown. > > Disable the quad mode in spi_nor_restore(), the flash will leave > > quad mode when remove. This will make sure the flash always enter the > > correct mode when loaded. > > We have a system which relies on an enabled quad mode on boot > (bootloader uses quad mode without enabling it) so using this patch > will prevent our device from booting. Out of curiosity, how does the flash get detected by SPI NOR? It issues the Read ID command in 1S-1S-1S mode but the flash is expecting 1S-4S-4S commands. Do you have any extra patches applied to make it work? I tried solving this problem for 8D-8D-8D mode but never really found a good solution.
Am Di., 1. Sept. 2020 um 11:48 Uhr schrieb Pratyush Yadav <p.yadav@ti.com>: > On 01/09/20 08:16AM, Matthias Weißer wrote: > > Am Di., 16. Juni 2020 um 15:03 Uhr schrieb Yicong Yang > > <yangyicong@hisilicon.com>: > > > If the flash's quad mode is enabled, it'll remain in the quad mode when > > > it's removed. If we drive the flash next time in SPI/Dual mode, then > > > problem occurs as the flash's quad enable bit is not cleared. > > > > On flash devices with a non-volatile quad enable bit (we use S25FL512S) > > this will wear out the quad enable bit as on every boot the bit is > > set and reset on shutdown. Or do I miss something here? > > Yes, I think it will. If you always want the bit enabled (and it being > non-volatile suggests that is the intended use), why not set > nor->params->quad_enable() to NULL? That way it won't be touched at all, > neither on boot nor on shutdown. Because we want to use plain mainline kernel without any local patches. > > > Disable the quad mode in spi_nor_restore(), the flash will leave > > > quad mode when remove. This will make sure the flash always enter the > > > correct mode when loaded. > > > > We have a system which relies on an enabled quad mode on boot > > (bootloader uses quad mode without enabling it) so using this patch > > will prevent our device from booting. > > Out of curiosity, how does the flash get detected by SPI NOR? It issues > the Read ID command in 1S-1S-1S mode but the flash is expecting 1S-4S-4S > commands. Do you have any extra patches applied to make it work? The RDID command (0x9F) is always a 1S-1S-1S command regardless of the QE bit state as e.g. the READ command (0x03). The only reason for the QE bit is that it remaps the usage of the !WP and !HOLD signals to IO2 and IO3. The SPI controller (QSPI on imx6 in our case) have to be programmed accordingly to execute the RDID command in 1S-1S-1S mode but then the state of the QE bit doesn't matter. > I tried solving this problem for 8D-8D-8D mode but never really found a > good solution. I don't know your hardware but I am sure that should work that way to. The current way it is done in your patch may wear out the QE bit (and maybe others in status/configuration register) quite quickly if you have a device which is booted frequently. Regards, Matthias
On 01/09/20 12:08PM, Matthias Weißer wrote: > Am Di., 1. Sept. 2020 um 11:48 Uhr schrieb Pratyush Yadav <p.yadav@ti.com>: > > On 01/09/20 08:16AM, Matthias Weißer wrote: > > > Am Di., 16. Juni 2020 um 15:03 Uhr schrieb Yicong Yang > > > <yangyicong@hisilicon.com>: > > > > If the flash's quad mode is enabled, it'll remain in the quad mode when > > > > it's removed. If we drive the flash next time in SPI/Dual mode, then > > > > problem occurs as the flash's quad enable bit is not cleared. > > > > > > On flash devices with a non-volatile quad enable bit (we use S25FL512S) > > > this will wear out the quad enable bit as on every boot the bit is > > > set and reset on shutdown. Or do I miss something here? > > > > Yes, I think it will. If you always want the bit enabled (and it being > > non-volatile suggests that is the intended use), why not set > > nor->params->quad_enable() to NULL? That way it won't be touched at all, > > neither on boot nor on shutdown. > > Because we want to use plain mainline kernel without any local patches. IMO SPI NOR should not touch non-volatile bits at all. They should be set by some other software and SPI NOR should only read them. So I can see a change like this justifiable for merging into mainline. The other option would be to put the call to spi_nor_quad_enable() behind a check for SNOR_F_BROKEN_RESET, like we do for 4-byte addressing mode. Either way, the patch has already landed in mainline. You will have to submit a patch with whichever option you think is better. > > > > Disable the quad mode in spi_nor_restore(), the flash will leave > > > > quad mode when remove. This will make sure the flash always enter the > > > > correct mode when loaded. > > > > > > We have a system which relies on an enabled quad mode on boot > > > (bootloader uses quad mode without enabling it) so using this patch > > > will prevent our device from booting. > > > > Out of curiosity, how does the flash get detected by SPI NOR? It issues > > the Read ID command in 1S-1S-1S mode but the flash is expecting 1S-4S-4S > > commands. Do you have any extra patches applied to make it work? > > The RDID command (0x9F) is always a 1S-1S-1S command regardless of the > QE bit state as e.g. the READ command (0x03). The only reason for the QE > bit is that it remaps the usage of the !WP and !HOLD signals to IO2 and IO3. > > The SPI controller (QSPI on imx6 in our case) have to be programmed > accordingly to execute the RDID command in 1S-1S-1S mode but then the > state of the QE bit doesn't matter. > > > I tried solving this problem for 8D-8D-8D mode but never really found a > > good solution. > > I don't know your hardware but I am sure that should work that way to. Unfortunately for the hardware I'm using (S28HS512T) once Octal mode is enabled RDID is also in 8D-8D-8D mode. So this isn't applicable there. Still, thanks for the explanation. > The current way it is done in your patch may wear out the QE bit (and > maybe others in status/configuration register) quite quickly if you > have a device which is booted frequently. This isn't my patch :-)
Hi Matthias, On 2020/9/1 14:16, Matthias Weißer wrote: > Am Di., 16. Juni 2020 um 15:03 Uhr schrieb Yicong Yang > <yangyicong@hisilicon.com>: >> If the flash's quad mode is enabled, it'll remain in the quad mode when >> it's removed. If we drive the flash next time in SPI/Dual mode, then >> problem occurs as the flash's quad enable bit is not cleared. > On flash devices with a non-volatile quad enable bit (we use S25FL512S) > this will wear out the quad enable bit as on every boot the bit is > set and reset on shutdown. Or do I miss something here? > >> Disable the quad mode in spi_nor_restore(), the flash will leave >> quad mode when remove. This will make sure the flash always enter the >> correct mode when loaded. > We have a system which relies on an enabled quad mode on boot > (bootloader uses quad mode without enabling it) so using this patch > will prevent our device from booting. For flashes boot with standard spi mode and with quad mode enabled in kernel, it will stay in quad mode when shutdown and next time boot. This patch intends to address this issue as we should restore the flash's original state. For the flashes boot with quad mode, seems we don't `restore` the flash's state correctly. It's better to record the flash's quad mode state and if it's in quad mode when boot, we'll not disable it when shutdown or remove. Regards, Yicong > > Regards, > Matthias > . >
Hi Mathhias and Pratyush, I've tested the following patch with s25fs128s1. I left the flash quad enabled before managed by spi-nor driver, and it'll stay QE after removed. So I think it'll also address the issue mentioned. Please have a test. Regards, Yicong From 43aa1afa12a10036f722b272a9a39b8c83218f33 Mon Sep 17 00:00:00 2001 From: Yicong Yang <yangyicong@hisilicon.com> Date: Tue, 1 Sep 2020 22:09:46 +0800 Subject: [PATCH] mtd: spi-nor: don't disable quad mode of flash whose is originally enabled Currently we'll disable the flash's Quad mode when remove/shutdown in spi_nor_restore(), no matter whether it's Quad enabled or not. For flashes originally in Quad mode, it'll clear the flash's QE bit and restore an incorrect state. Record the flash's original QE state, and don't disable the Quad mode of these originally Quad enabled flash. Reported-by: Matthias Weisser <m.weisser.m@gmail.com> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> --- drivers/mtd/spi-nor/core.c | 30 +++++++++++++++++++++++------- include/linux/mtd/spi-nor.h | 3 +++ 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 65eff4c..b13b3b3 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -1924,8 +1924,11 @@ int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor, bool enable) if (ret) return ret; - if ((enable && (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) || - (!enable && !(nor->bouncebuf[0] & SR1_QUAD_EN_BIT6))) + if (enable && (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) { + nor->orig_qe_state = true; + return 0; + } + if (!enable && !(nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) return 0; if (enable) @@ -1958,8 +1961,12 @@ int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor, bool enable) if (ret) return ret; - if ((enable && (nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)) || - (!enable && !(nor->bouncebuf[0] & SR2_QUAD_EN_BIT1))) + if (enable && (nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)) { + nor->orig_qe_state = true; + return 0; + } + + if (!enable && !(nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)) return 0; if (enable) @@ -1993,8 +2000,12 @@ int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor, bool enable) ret = spi_nor_read_sr2(nor, sr2); if (ret) return ret; - if ((enable && (*sr2 & SR2_QUAD_EN_BIT7)) || - (!enable && !(*sr2 & SR2_QUAD_EN_BIT7))) + if (enable && (*sr2 & SR2_QUAD_EN_BIT7)) { + nor->orig_qe_state = true; + return 0; + } + + if (!enable && !(*sr2 & SR2_QUAD_EN_BIT7)) return 0; /* Update the Quad Enable bit. */ @@ -3001,7 +3012,12 @@ void spi_nor_restore(struct spi_nor *nor) nor->flags & SNOR_F_BROKEN_RESET) nor->params->set_4byte_addr_mode(nor, false); - spi_nor_quad_enable(nor, false); + /* + * restore the flash's quad mode. if the flash's quad mode is + * enabled originally, we'll not disable it. + */ + if (!nor->orig_qe_state) + spi_nor_quad_enable(nor, false); } EXPORT_SYMBOL_GPL(spi_nor_restore); diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index 60bac2c..f343416 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -344,6 +344,8 @@ struct spi_nor_flash_parameter; * @read_dummy: the dummy needed by the read operation * @program_opcode: the program opcode * @sst_write_second: used by the SST write operation + * @orig_qe_state: used to indicate the flash's original Quad mode enable + * state. True for enabled and false for disabled. * @flags: flag options for the current SPI NOR (SNOR_F_*) * @read_proto: the SPI protocol for read operations * @write_proto: the SPI protocol for write operations @@ -375,6 +377,7 @@ struct spi_nor { enum spi_nor_protocol write_proto; enum spi_nor_protocol reg_proto; bool sst_write_second; + bool orig_qe_state; u32 flags; const struct spi_nor_controller_ops *controller_ops;
Hi Yicong, On 9/1/20 7:50 PM, Yicong Yang wrote: > Hi Mathhias and Pratyush, > > I've tested the following patch with s25fs128s1. > I left the flash quad enabled before managed by spi-nor driver, > and it'll stay QE after removed. So I think it'll also address the issue > mentioned. Please have a test. > > Regards, > Yicong > > > From 43aa1afa12a10036f722b272a9a39b8c83218f33 Mon Sep 17 00:00:00 2001 > From: Yicong Yang <yangyicong@hisilicon.com> > Date: Tue, 1 Sep 2020 22:09:46 +0800 > Subject: [PATCH] mtd: spi-nor: don't disable quad mode of flash whose is > originally enabled > > Currently we'll disable the flash's Quad mode when remove/shutdown in > spi_nor_restore(), no matter whether it's Quad enabled or not. > For flashes originally in Quad mode, it'll clear the flash's QE bit > and restore an incorrect state. > > Record the flash's original QE state, and don't disable the Quad mode > of these originally Quad enabled flash. > This will break backward compatibility... Imagine a new board being flashed from Kernel. Before this series, QE bit would be set at the end of flashing and ROM/bootloader (such as the one reported by Matthias) would work fine. After this series, QE bit would no longer be set and would most likely break boot.. I still am unable to understand what is the underlying problem that is being addressed here? You mention addressing issue loading the driver in Quad mode first and reload it in Standard SPI/Dual mode. But per s25fs128s data sheet: " Quad Data Width (QUAD) CR1V[1]: When set to 1, this bit switches the data width of the device to 4-bit Quad Mode. That is, WP# becomes IO2 and IO3 / RESET# becomes an active I/O signal when CS# is low or the RESET# input when CS# is high. The WP# input is not monitored for its normal function and is internally set to high (inactive). The commands for Serial, and Dual I/O Read still function normally but, there is no need to drive the WP# input for those commands when switching between commands using different data path widths. Similarly, there is no requirement to drive the IO3 / RESET# during those commands (while CS# is low)." So setting QE bit should have no impact for serial/dual IO modes? Regards Vignesh > Reported-by: Matthias Weisser <m.weisser.m@gmail.com> > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > --- > drivers/mtd/spi-nor/core.c | 30 +++++++++++++++++++++++------- > include/linux/mtd/spi-nor.h | 3 +++ > 2 files changed, 26 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 65eff4c..b13b3b3 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -1924,8 +1924,11 @@ int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor, bool enable) > if (ret) > return ret; > > - if ((enable && (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) || > - (!enable && !(nor->bouncebuf[0] & SR1_QUAD_EN_BIT6))) > + if (enable && (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) { > + nor->orig_qe_state = true; > + return 0; > + } > + if (!enable && !(nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) > return 0; > > if (enable) > @@ -1958,8 +1961,12 @@ int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor, bool enable) > if (ret) > return ret; > > - if ((enable && (nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)) || > - (!enable && !(nor->bouncebuf[0] & SR2_QUAD_EN_BIT1))) > + if (enable && (nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)) { > + nor->orig_qe_state = true; > + return 0; > + } > + > + if (!enable && !(nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)) > return 0; > > if (enable) > @@ -1993,8 +2000,12 @@ int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor, bool enable) > ret = spi_nor_read_sr2(nor, sr2); > if (ret) > return ret; > - if ((enable && (*sr2 & SR2_QUAD_EN_BIT7)) || > - (!enable && !(*sr2 & SR2_QUAD_EN_BIT7))) > + if (enable && (*sr2 & SR2_QUAD_EN_BIT7)) { > + nor->orig_qe_state = true; > + return 0; > + } > + > + if (!enable && !(*sr2 & SR2_QUAD_EN_BIT7)) > return 0; > > /* Update the Quad Enable bit. */ > @@ -3001,7 +3012,12 @@ void spi_nor_restore(struct spi_nor *nor) > nor->flags & SNOR_F_BROKEN_RESET) > nor->params->set_4byte_addr_mode(nor, false); > > - spi_nor_quad_enable(nor, false); > + /* > + * restore the flash's quad mode. if the flash's quad mode is > + * enabled originally, we'll not disable it. > + */ > + if (!nor->orig_qe_state) > + spi_nor_quad_enable(nor, false); > } > EXPORT_SYMBOL_GPL(spi_nor_restore); > > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index 60bac2c..f343416 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -344,6 +344,8 @@ struct spi_nor_flash_parameter; > * @read_dummy: the dummy needed by the read operation > * @program_opcode: the program opcode > * @sst_write_second: used by the SST write operation > + * @orig_qe_state: used to indicate the flash's original Quad mode enable > + * state. True for enabled and false for disabled. > * @flags: flag options for the current SPI NOR (SNOR_F_*) > * @read_proto: the SPI protocol for read operations > * @write_proto: the SPI protocol for write operations > @@ -375,6 +377,7 @@ struct spi_nor { > enum spi_nor_protocol write_proto; > enum spi_nor_protocol reg_proto; > bool sst_write_second; > + bool orig_qe_state; > u32 flags; > > const struct spi_nor_controller_ops *controller_ops; >
Hi Vignesh, On 2020/9/2 15:50, Vignesh Raghavendra wrote: > Hi Yicong, > > On 9/1/20 7:50 PM, Yicong Yang wrote: >> Hi Mathhias and Pratyush, >> >> I've tested the following patch with s25fs128s1. >> I left the flash quad enabled before managed by spi-nor driver, >> and it'll stay QE after removed. So I think it'll also address the issue >> mentioned. Please have a test. >> >> Regards, >> Yicong >> >> >> From 43aa1afa12a10036f722b272a9a39b8c83218f33 Mon Sep 17 00:00:00 2001 >> From: Yicong Yang <yangyicong@hisilicon.com> >> Date: Tue, 1 Sep 2020 22:09:46 +0800 >> Subject: [PATCH] mtd: spi-nor: don't disable quad mode of flash whose is >> originally enabled >> >> Currently we'll disable the flash's Quad mode when remove/shutdown in >> spi_nor_restore(), no matter whether it's Quad enabled or not. >> For flashes originally in Quad mode, it'll clear the flash's QE bit >> and restore an incorrect state. >> >> Record the flash's original QE state, and don't disable the Quad mode >> of these originally Quad enabled flash. >> > This will break backward compatibility... Imagine a new board being > flashed from Kernel. Before this series, QE bit would be set at the end of flashing > and ROM/bootloader (such as the one reported by Matthias) would work fine. > After this series, QE bit would no longer be set and would most likely > break boot.. > > I still am unable to understand what is the underlying problem that is > being addressed here? > > You mention addressing issue loading the driver in Quad mode first and reload it in > Standard SPI/Dual mode. But per s25fs128s data sheet: > " > Quad Data Width (QUAD) CR1V[1]: When set to 1, this bit switches the data width of the device to 4-bit Quad Mode. That is, WP# > becomes IO2 and IO3 / RESET# becomes an active I/O signal when CS# is low or the RESET# input when CS# is high. The WP# > input is not monitored for its normal function and is internally set to high (inactive). The commands for Serial, and Dual I/O Read still > function normally but, there is no need to drive the WP# input for those commands when switching between commands using > different data path widths. Similarly, there is no requirement to drive the IO3 / RESET# during those commands (while CS# is low)." > > So setting QE bit should have no impact for serial/dual IO modes? yes. and I reword the commit like below, as suggested by Tudor and send a v2 patch. This thread is the v1 one. "If the flash's quad mode is enabled, it'll remain in the quad mode when it's removed. If we drive the flash next time in Standard/Dual SPI mode, the QE bit is not cleared and the function of flash's WP# and RESET#/HOLD# have been switched to IO2 and IO3 and are not restored." I believe we should restore the state of the flash when it's unloaded from the kernel. In previous code, if we load the flash in Quad mode (originally in Standard SPI mode) and shutdown, its WP# and RESET# won't be restored correctly. Seems the patch doesn't consider the condition that the flash has already in Quad mode before loaded and restore the flash in a wrong state. Regards Yicong > > Regards > Vignesh > >> Reported-by: Matthias Weisser <m.weisser.m@gmail.com> >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> >> --- >> drivers/mtd/spi-nor/core.c | 30 +++++++++++++++++++++++------- >> include/linux/mtd/spi-nor.h | 3 +++ >> 2 files changed, 26 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >> index 65eff4c..b13b3b3 100644 >> --- a/drivers/mtd/spi-nor/core.c >> +++ b/drivers/mtd/spi-nor/core.c >> @@ -1924,8 +1924,11 @@ int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor, bool enable) >> if (ret) >> return ret; >> >> - if ((enable && (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) || >> - (!enable && !(nor->bouncebuf[0] & SR1_QUAD_EN_BIT6))) >> + if (enable && (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) { >> + nor->orig_qe_state = true; >> + return 0; >> + } >> + if (!enable && !(nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) >> return 0; >> >> if (enable) >> @@ -1958,8 +1961,12 @@ int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor, bool enable) >> if (ret) >> return ret; >> >> - if ((enable && (nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)) || >> - (!enable && !(nor->bouncebuf[0] & SR2_QUAD_EN_BIT1))) >> + if (enable && (nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)) { >> + nor->orig_qe_state = true; >> + return 0; >> + } >> + >> + if (!enable && !(nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)) >> return 0; >> >> if (enable) >> @@ -1993,8 +2000,12 @@ int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor, bool enable) >> ret = spi_nor_read_sr2(nor, sr2); >> if (ret) >> return ret; >> - if ((enable && (*sr2 & SR2_QUAD_EN_BIT7)) || >> - (!enable && !(*sr2 & SR2_QUAD_EN_BIT7))) >> + if (enable && (*sr2 & SR2_QUAD_EN_BIT7)) { >> + nor->orig_qe_state = true; >> + return 0; >> + } >> + >> + if (!enable && !(*sr2 & SR2_QUAD_EN_BIT7)) >> return 0; >> >> /* Update the Quad Enable bit. */ >> @@ -3001,7 +3012,12 @@ void spi_nor_restore(struct spi_nor *nor) >> nor->flags & SNOR_F_BROKEN_RESET) >> nor->params->set_4byte_addr_mode(nor, false); >> >> - spi_nor_quad_enable(nor, false); >> + /* >> + * restore the flash's quad mode. if the flash's quad mode is >> + * enabled originally, we'll not disable it. >> + */ >> + if (!nor->orig_qe_state) >> + spi_nor_quad_enable(nor, false); >> } >> EXPORT_SYMBOL_GPL(spi_nor_restore); >> >> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h >> index 60bac2c..f343416 100644 >> --- a/include/linux/mtd/spi-nor.h >> +++ b/include/linux/mtd/spi-nor.h >> @@ -344,6 +344,8 @@ struct spi_nor_flash_parameter; >> * @read_dummy: the dummy needed by the read operation >> * @program_opcode: the program opcode >> * @sst_write_second: used by the SST write operation >> + * @orig_qe_state: used to indicate the flash's original Quad mode enable >> + * state. True for enabled and false for disabled. >> * @flags: flag options for the current SPI NOR (SNOR_F_*) >> * @read_proto: the SPI protocol for read operations >> * @write_proto: the SPI protocol for write operations >> @@ -375,6 +377,7 @@ struct spi_nor { >> enum spi_nor_protocol write_proto; >> enum spi_nor_protocol reg_proto; >> bool sst_write_second; >> + bool orig_qe_state; >> u32 flags; >> >> const struct spi_nor_controller_ops *controller_ops; >> > . >
Hi Vignesh Am Di., 1. Sept. 2020 um 16:20 Uhr schrieb Yicong Yang <yangyicong@hisilicon.com>: > I've tested the following patch with s25fs128s1. > I left the flash quad enabled before managed by spi-nor driver, > and it'll stay QE after removed. So I think it'll also address the issue > mentioned. Please have a test. Thanks for the patch. I can confirm that current 5.9-rc3 bricks my hardware and your patch on top of it fixes that. Therefore Tested-by: Matthias Weisser <m.weisser.m@gmail.com> But I am still concerned about wearing out nonvolatile QE bits (not in my case, but there may be others) which may also brick hardware after a lot of boots (by set and reset QE bit on every boot). So, from my limited point of view, your patch fixes my problem but overall I think the original approach should be thought-out a bit more. Regards, Matthias
Hi Matthias, On 2020/9/2 20:15, Matthias Weißer wrote: > Hi Vignesh > > Am Di., 1. Sept. 2020 um 16:20 Uhr schrieb Yicong Yang > <yangyicong@hisilicon.com>: >> I've tested the following patch with s25fs128s1. >> I left the flash quad enabled before managed by spi-nor driver, >> and it'll stay QE after removed. So I think it'll also address the issue >> mentioned. Please have a test. > Thanks for the patch. I can confirm that current 5.9-rc3 bricks my hardware > and your patch on top of it fixes that. Therefore > > Tested-by: Matthias Weisser <m.weisser.m@gmail.com> > > But I am still concerned about wearing out nonvolatile QE bits (not in my > case, but there may be others) which may also brick hardware after a lot of > boots (by set and reset QE bit on every boot). > > So, from my limited point of view, your patch fixes my problem but overall > I think the original approach should be thought-out a bit more. Thanks for testing the patch. The driver will try to set the QE bit when 1) in ->quad_enable() if QE bit is not set and 2) previously without this patch in spi_nor_restore() to try to disable the flash quad mode. With this patch, if the QE bit is originally set the driver will only read and record it and will not disable the quad mode in spi_nor_restore(). What the driver will do is to read the bit, without trying to set or reset it. So I think it will not wear out the those nonvolatile QE bits. Regards, Yicong > > Regards, > Matthias > . >
Am Do., 3. Sept. 2020 um 05:04 Uhr schrieb Yicong Yang <yangyicong@hisilicon.com>: > > Hi Matthias, > On 2020/9/2 20:15, Matthias Weißer wrote: > > Hi Vignesh > > > > Am Di., 1. Sept. 2020 um 16:20 Uhr schrieb Yicong Yang > > <yangyicong@hisilicon.com>: > >> I've tested the following patch with s25fs128s1. > >> I left the flash quad enabled before managed by spi-nor driver, > >> and it'll stay QE after removed. So I think it'll also address the issue > >> mentioned. Please have a test. > > Thanks for the patch. I can confirm that current 5.9-rc3 bricks my hardware > > and your patch on top of it fixes that. Therefore > > > > Tested-by: Matthias Weisser <m.weisser.m@gmail.com> > > > > But I am still concerned about wearing out nonvolatile QE bits (not in my > > case, but there may be others) which may also brick hardware after a lot of > > boots (by set and reset QE bit on every boot). > > > > So, from my limited point of view, your patch fixes my problem but overall > > I think the original approach should be thought-out a bit more. > > Thanks for testing the patch. > > The driver will try to set the QE bit when 1) in ->quad_enable() if QE bit is not set and > 2) previously without this patch in spi_nor_restore() to try to disable the flash quad mode. > > With this patch, if the QE bit is originally set the driver will only read and record it and > will not disable the quad mode in spi_nor_restore(). What the driver will do is to read > the bit, without trying to set or reset it. So I think it will not wear out the those nonvolatile > QE bits. I think it will. Expect a flash with the QE bit not set. Kernel boots, set the bit which will cause an erase cycle (setting it from 0 to 1). On shutdown the bit is reset (transition from 1 to 0) which will cause a program cycle. So, every boot -> shutdown cycle causes at least one program-erase cycle which will wear out that area of the flash. See also this quote from the datasheet of F25FL512S: "Non-volatile bits have the same cycling (erase and program) endurance as the main flash array." I still think the reset of the QE (or any other non-volatile) bit on shutdown should be prevented and only allowed by explicitly enable it (e.g. per system in DT or per flash in flash_info struct). Regards, Matthias
On 9/2/20 3:42 PM, Yicong Yang wrote: > Hi Vignesh, > > > On 2020/9/2 15:50, Vignesh Raghavendra wrote: >> Hi Yicong, >> >> On 9/1/20 7:50 PM, Yicong Yang wrote: >>> Hi Mathhias and Pratyush, >>> [...] >>> >> This will break backward compatibility... Imagine a new board being >> flashed from Kernel. Before this series, QE bit would be set at the end of flashing >> and ROM/bootloader (such as the one reported by Matthias) would work fine. >> After this series, QE bit would no longer be set and would most likely >> break boot.. >> >> I still am unable to understand what is the underlying problem that is >> being addressed here? >> >> You mention addressing issue loading the driver in Quad mode first and reload it in >> Standard SPI/Dual mode. But per s25fs128s data sheet: >> " >> Quad Data Width (QUAD) CR1V[1]: When set to 1, this bit switches the data width of the device to 4-bit Quad Mode. That is, WP# >> becomes IO2 and IO3 / RESET# becomes an active I/O signal when CS# is low or the RESET# input when CS# is high. The WP# >> input is not monitored for its normal function and is internally set to high (inactive). The commands for Serial, and Dual I/O Read still >> function normally but, there is no need to drive the WP# input for those commands when switching between commands using >> different data path widths. Similarly, there is no requirement to drive the IO3 / RESET# during those commands (while CS# is low)." >> >> So setting QE bit should have no impact for serial/dual IO modes? > > yes. and I reword the commit like below, as suggested by Tudor and send a v2 patch. This thread is the v1 one. > "If the flash's quad mode is enabled, it'll remain in the quad mode when > it's removed. If we drive the flash next time in Standard/Dual SPI mode, > the QE bit is not cleared and the function of flash's WP# and RESET#/HOLD# > have been switched to IO2 and IO3 and are not restored." > > I believe we should restore the state of the flash when it's unloaded from the kernel. In previous code, if we load the flash > in Quad mode (originally in Standard SPI mode) and shutdown, its WP# and RESET# won't be restored correctly. Seems > the patch doesn't consider the condition that the flash has already in Quad mode before loaded and restore the flash > in a wrong state. > How do you load driver in Quad mode first and then reload in Single/Dual mode later on? What is the use case? I don't think relying on WP# and RESET#/HOLD# functionality for a QSPI flash is right thing to do as these lines would act as data lines in Quad mode and thus WP# wont really protect contents of the flash when in Quad mode. Also, below patch is not fool proof even for hypothetical case that you are trying to solve: Consider a scenario of kernel crash or hard reset, then there will be no chance to call spi_nor_restore() and you would end up with QE bit set.. Upon reboot, kernel will find that QE bit and will simply restore the same back on shutdown. Given the fact that setting and unsetting NV bit causes wearing of this rather important bit and also breaks backward compatibility of tools that expect Kernel to set QE bit on flashing, I suggest reverting these patches: cc59e6bb6cd6 mtd: spi-nor: Disable the flash quad mode in spi_nor_restore() be192209d5a3 mtd: spi-nor: Add capability to disable flash quad mode Regards Vignesh
On 2020/9/3 13:59, Vignesh Raghavendra wrote: > > On 9/2/20 3:42 PM, Yicong Yang wrote: >> Hi Vignesh, >> >> >> On 2020/9/2 15:50, Vignesh Raghavendra wrote: >>> Hi Yicong, >>> >>> On 9/1/20 7:50 PM, Yicong Yang wrote: >>>> Hi Mathhias and Pratyush, >>>> > [...] >>> This will break backward compatibility... Imagine a new board being >>> flashed from Kernel. Before this series, QE bit would be set at the end of flashing >>> and ROM/bootloader (such as the one reported by Matthias) would work fine. >>> After this series, QE bit would no longer be set and would most likely >>> break boot.. >>> >>> I still am unable to understand what is the underlying problem that is >>> being addressed here? >>> >>> You mention addressing issue loading the driver in Quad mode first and reload it in >>> Standard SPI/Dual mode. But per s25fs128s data sheet: >>> " >>> Quad Data Width (QUAD) CR1V[1]: When set to 1, this bit switches the data width of the device to 4-bit Quad Mode. That is, WP# >>> becomes IO2 and IO3 / RESET# becomes an active I/O signal when CS# is low or the RESET# input when CS# is high. The WP# >>> input is not monitored for its normal function and is internally set to high (inactive). The commands for Serial, and Dual I/O Read still >>> function normally but, there is no need to drive the WP# input for those commands when switching between commands using >>> different data path widths. Similarly, there is no requirement to drive the IO3 / RESET# during those commands (while CS# is low)." >>> >>> So setting QE bit should have no impact for serial/dual IO modes? >> yes. and I reword the commit like below, as suggested by Tudor and send a v2 patch. This thread is the v1 one. >> "If the flash's quad mode is enabled, it'll remain in the quad mode when >> it's removed. If we drive the flash next time in Standard/Dual SPI mode, >> the QE bit is not cleared and the function of flash's WP# and RESET#/HOLD# >> have been switched to IO2 and IO3 and are not restored." >> >> I believe we should restore the state of the flash when it's unloaded from the kernel. In previous code, if we load the flash >> in Quad mode (originally in Standard SPI mode) and shutdown, its WP# and RESET# won't be restored correctly. Seems >> the patch doesn't consider the condition that the flash has already in Quad mode before loaded and restore the flash >> in a wrong state. >> > How do you load driver in Quad mode first and then reload in Single/Dual > mode later on? What is the use case? we use module paramters to indicate the bus width in a private version of our driver, yet this hasn't been upstreamed. > > I don't think relying on WP# and RESET#/HOLD# functionality for a QSPI > flash is right thing to do as these lines would act as data lines in > Quad mode and thus WP# wont really protect contents of the flash > when in Quad mode. > > Also, below patch is not fool proof even for hypothetical case that you are > trying to solve: > Consider a scenario of kernel crash or hard reset, then there will be no > chance to call spi_nor_restore() and you would end up with QE bit set.. > Upon reboot, kernel will find that QE bit and will simply restore the same > back on shutdown. well this make sense to me. > > Given the fact that setting and unsetting NV bit causes wearing of this > rather important bit and also breaks backward compatibility of tools > that expect Kernel to set QE bit on flashing, I suggest reverting these patches: > > cc59e6bb6cd6 mtd: spi-nor: Disable the flash quad mode in spi_nor_restore() > be192209d5a3 mtd: spi-nor: Add capability to disable flash quad mode I've send the revert patches. You may found at : https://lore.kernel.org/linux-mtd/1599205640-26690-1-git-send-email-yangyicong@hisilicon.com/ but I still have something uncertain, I think we should avoid setting the non-volatile bits in spi-nor driver, should we? Regards Yicong > > > Regards > Vignesh > . >
On 2020/9/3 13:33, Matthias Weißer wrote: > Am Do., 3. Sept. 2020 um 05:04 Uhr schrieb Yicong Yang > <yangyicong@hisilicon.com>: >> Hi Matthias, >> On 2020/9/2 20:15, Matthias Weißer wrote: >>> Hi Vignesh >>> >>> Am Di., 1. Sept. 2020 um 16:20 Uhr schrieb Yicong Yang >>> <yangyicong@hisilicon.com>: >>>> I've tested the following patch with s25fs128s1. >>>> I left the flash quad enabled before managed by spi-nor driver, >>>> and it'll stay QE after removed. So I think it'll also address the issue >>>> mentioned. Please have a test. >>> Thanks for the patch. I can confirm that current 5.9-rc3 bricks my hardware >>> and your patch on top of it fixes that. Therefore >>> >>> Tested-by: Matthias Weisser <m.weisser.m@gmail.com> >>> >>> But I am still concerned about wearing out nonvolatile QE bits (not in my >>> case, but there may be others) which may also brick hardware after a lot of >>> boots (by set and reset QE bit on every boot). >>> >>> So, from my limited point of view, your patch fixes my problem but overall >>> I think the original approach should be thought-out a bit more. >> Thanks for testing the patch. >> >> The driver will try to set the QE bit when 1) in ->quad_enable() if QE bit is not set and >> 2) previously without this patch in spi_nor_restore() to try to disable the flash quad mode. >> >> With this patch, if the QE bit is originally set the driver will only read and record it and >> will not disable the quad mode in spi_nor_restore(). What the driver will do is to read >> the bit, without trying to set or reset it. So I think it will not wear out the those nonvolatile >> QE bits. > I think it will. Expect a flash with the QE bit not set. Kernel boots, set the > bit which will cause an erase cycle (setting it from 0 to 1). On shutdown the > bit is reset (transition from 1 to 0) which will cause a program cycle. So, > every boot -> shutdown cycle causes at least one program-erase cycle which will > wear out that area of the flash. > > See also this quote from the datasheet of F25FL512S: > "Non-volatile bits have the same cycling (erase and program) endurance > as the main > flash array." > > I still think the reset of the QE (or any other non-volatile) bit on > shutdown should > be prevented and only allowed by explicitly enable it (e.g. per system in DT or > per flash in flash_info struct). you may find the revert patches at: https://lore.kernel.org/linux-mtd/1599205640-26690-1-git-send-email-yangyicong@hisilicon.com/ thanks for the illustration. Regards, Yicong > > Regards, > Matthias > . >
Am Fr., 4. Sept. 2020 um 09:55 Uhr schrieb Yicong Yang <yangyicong@hisilicon.com>: > > Given the fact that setting and unsetting NV bit causes wearing of this > > rather important bit and also breaks backward compatibility of tools > > that expect Kernel to set QE bit on flashing, I suggest reverting these patches: > > > > cc59e6bb6cd6 mtd: spi-nor: Disable the flash quad mode in spi_nor_restore() > > be192209d5a3 mtd: spi-nor: Add capability to disable flash quad mode > > I've send the revert patches. You may found at : > https://lore.kernel.org/linux-mtd/1599205640-26690-1-git-send-email-yangyicong@hisilicon.com/ Thanks. I think this is currently the way to go. > but I still have something uncertain, I think we should avoid setting the non-volatile bits > in spi-nor driver, should we? Why not? If quad mode should be used the QE bit has to be set. There is no other way to enable quad mode (on the devices I am aware of). Regards, Matthias
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index ad5498f..4c9d88b 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -3002,6 +3002,8 @@ void spi_nor_restore(struct spi_nor *nor) if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES) && nor->flags & SNOR_F_BROKEN_RESET) nor->params->set_4byte_addr_mode(nor, false); + + spi_nor_quad_enable(nor, false); } EXPORT_SYMBOL_GPL(spi_nor_restore);