| Message ID | 20220613113030.1622723-1-s.hauer@pengutronix.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | mtd: rawnand: gpmi: Fix setting busy timeout setting | expand |
Hi Sascha, s.hauer@pengutronix.de wrote on Mon, 13 Jun 2022 13:30:30 +0200: > The DEVICE_BUSY_TIMEOUT value is described in the Reference Manual as: > > | Timeout waiting for NAND Ready/Busy or ATA IRQ. Used in WAIT_FOR_READY > | mode. This value is the number of GPMI_CLK cycles multiplied by 4096. > > So instead of multiplying the value in cycles with 4096, we have to > divide it by that value. Use DIV_ROUND_UP to make sure we are on the > safe side, especially when the calculated value in cycles is smaller > than 4096 as typically the case. > > This bug likely never triggered because any timeout != 0 usually will > do. In my case the busy timeout in cycles was originally calculated as > 2408, which multiplied with 4096 is 0x968000. The lower 16 bits were > taken for the 16 bit wide register field, so the register value was > 0x8000. With 2970bf5a32f0 ("mtd: rawnand: gpmi: fix controller timings > setting") however the value in cycles became 2384, which multiplied > with 4096 is 0x950000. The lower 16 bit are 0x0 now resulting in an > intermediate timeout when reading from NAND. > > Fixes: b1206122069aa ("mtd: rawnand: gpmi: use core timings instead of an empirical derivation") > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> Shouldn't we add a Cc: stable here? > --- > drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > index 0b68d05846e18..889e403299568 100644 > --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > @@ -890,7 +890,7 @@ static int gpmi_nfc_compute_timings(struct gpmi_nand_data *this, > hw->timing0 = BF_GPMI_TIMING0_ADDRESS_SETUP(addr_setup_cycles) | > BF_GPMI_TIMING0_DATA_HOLD(data_hold_cycles) | > BF_GPMI_TIMING0_DATA_SETUP(data_setup_cycles); > - hw->timing1 = BF_GPMI_TIMING1_BUSY_TIMEOUT(busy_timeout_cycles * 4096); > + hw->timing1 = BF_GPMI_TIMING1_BUSY_TIMEOUT(DIV_ROUND_UP(busy_timeout_cycles, 4096)); > > /* > * Derive NFC ideal delay from {3}: Thanks, Miquèl
On Mon, Jun 13, 2022 at 04:07:38PM +0200, Miquel Raynal wrote: > Hi Sascha, > > s.hauer@pengutronix.de wrote on Mon, 13 Jun 2022 13:30:30 +0200: > > > The DEVICE_BUSY_TIMEOUT value is described in the Reference Manual as: > > > > | Timeout waiting for NAND Ready/Busy or ATA IRQ. Used in WAIT_FOR_READY > > | mode. This value is the number of GPMI_CLK cycles multiplied by 4096. > > > > So instead of multiplying the value in cycles with 4096, we have to > > divide it by that value. Use DIV_ROUND_UP to make sure we are on the > > safe side, especially when the calculated value in cycles is smaller > > than 4096 as typically the case. > > > > This bug likely never triggered because any timeout != 0 usually will > > do. In my case the busy timeout in cycles was originally calculated as > > 2408, which multiplied with 4096 is 0x968000. The lower 16 bits were > > taken for the 16 bit wide register field, so the register value was > > 0x8000. With 2970bf5a32f0 ("mtd: rawnand: gpmi: fix controller timings > > setting") however the value in cycles became 2384, which multiplied > > with 4096 is 0x950000. The lower 16 bit are 0x0 now resulting in an > > intermediate timeout when reading from NAND. > > > > Fixes: b1206122069aa ("mtd: rawnand: gpmi: use core timings instead of an empirical derivation") > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > Shouldn't we add a Cc: stable here? I became a bit ignorant about Cc: stable because everything with the keyword "Fix" in the subject seems to end up in stable anyway no matter if I want it or not. But right, this one should go into stable, I just resent. Sascha
Hi Sascha, s.hauer@pengutronix.de wrote on Tue, 14 Jun 2022 10:34:37 +0200: > On Mon, Jun 13, 2022 at 04:07:38PM +0200, Miquel Raynal wrote: > > Hi Sascha, > > > > s.hauer@pengutronix.de wrote on Mon, 13 Jun 2022 13:30:30 +0200: > > > > > The DEVICE_BUSY_TIMEOUT value is described in the Reference Manual as: > > > > > > | Timeout waiting for NAND Ready/Busy or ATA IRQ. Used in WAIT_FOR_READY > > > | mode. This value is the number of GPMI_CLK cycles multiplied by 4096. > > > > > > So instead of multiplying the value in cycles with 4096, we have to > > > divide it by that value. Use DIV_ROUND_UP to make sure we are on the > > > safe side, especially when the calculated value in cycles is smaller > > > than 4096 as typically the case. > > > > > > This bug likely never triggered because any timeout != 0 usually will > > > do. In my case the busy timeout in cycles was originally calculated as > > > 2408, which multiplied with 4096 is 0x968000. The lower 16 bits were > > > taken for the 16 bit wide register field, so the register value was > > > 0x8000. With 2970bf5a32f0 ("mtd: rawnand: gpmi: fix controller timings > > > setting") however the value in cycles became 2384, which multiplied > > > with 4096 is 0x950000. The lower 16 bit are 0x0 now resulting in an > > > intermediate timeout when reading from NAND. > > > > > > Fixes: b1206122069aa ("mtd: rawnand: gpmi: use core timings instead of an empirical derivation") > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > > > Shouldn't we add a Cc: stable here? > > I became a bit ignorant about Cc: stable because everything with the > keyword "Fix" in the subject seems to end up in stable anyway no matter > if I want it or not. Yeah with the AUTOSEL logic many fixes are backported, but I think I've read GKH complaining that this was not a reason for forgetting the stable Cc when it was relevant... So I ask for it again now :) > > But right, this one should go into stable, I just resent. > > Sascha > > Thanks, Miquèl
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c index 0b68d05846e18..889e403299568 100644 --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c @@ -890,7 +890,7 @@ static int gpmi_nfc_compute_timings(struct gpmi_nand_data *this, hw->timing0 = BF_GPMI_TIMING0_ADDRESS_SETUP(addr_setup_cycles) | BF_GPMI_TIMING0_DATA_HOLD(data_hold_cycles) | BF_GPMI_TIMING0_DATA_SETUP(data_setup_cycles); - hw->timing1 = BF_GPMI_TIMING1_BUSY_TIMEOUT(busy_timeout_cycles * 4096); + hw->timing1 = BF_GPMI_TIMING1_BUSY_TIMEOUT(DIV_ROUND_UP(busy_timeout_cycles, 4096)); /* * Derive NFC ideal delay from {3}:
The DEVICE_BUSY_TIMEOUT value is described in the Reference Manual as: | Timeout waiting for NAND Ready/Busy or ATA IRQ. Used in WAIT_FOR_READY | mode. This value is the number of GPMI_CLK cycles multiplied by 4096. So instead of multiplying the value in cycles with 4096, we have to divide it by that value. Use DIV_ROUND_UP to make sure we are on the safe side, especially when the calculated value in cycles is smaller than 4096 as typically the case. This bug likely never triggered because any timeout != 0 usually will do. In my case the busy timeout in cycles was originally calculated as 2408, which multiplied with 4096 is 0x968000. The lower 16 bits were taken for the 16 bit wide register field, so the register value was 0x8000. With 2970bf5a32f0 ("mtd: rawnand: gpmi: fix controller timings setting") however the value in cycles became 2384, which multiplied with 4096 is 0x950000. The lower 16 bit are 0x0 now resulting in an intermediate timeout when reading from NAND. Fixes: b1206122069aa ("mtd: rawnand: gpmi: use core timings instead of an empirical derivation") Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)