| Message ID | 20211012090224.4101984-2-s.riedmueller@phytec.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [RFC,1/2] mtd: rawnand: gpmi: Remove explicit default gpmi clock setting for i.MX6 | expand |
On 21/10/12 11:02AM, Stefan Riedmueller wrote: > According to i.MX 6 erratum ERR007117 gpmi clocks need to be gated off > when doing a gpmi_io clk rate change. So gate the clocks off before > the rate change in nfc_apply_timings and ungate them again after the > change. > > Otherwise this rate change can lead to an unresponsive GPMI core which > results in DMA timeouts and failed driver probe: > > [ 4.072318] gpmi-nand 112000.gpmi-nand: DMA timeout, last DMA > ... > [ 4.370355] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -110 > ... > [ 4.375988] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > [ 4.381524] gpmi-nand 112000.gpmi-nand: Error in ECC-based read: -22 > [ 4.387988] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > [ 4.393535] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > ... > > Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de> > --- > Hi, > > I'm not sure about the error handling here, if it is actually neccessary. > So some feedback would be nice here. > > Thanks, > Stefan > --- > drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > index a1f7000f033e..326c8a895f1f 100644 > --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > @@ -713,15 +713,28 @@ static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this, > (use_half_period ? BM_GPMI_CTRL1_HALF_PERIOD : 0); > } > > -static void gpmi_nfc_apply_timings(struct gpmi_nand_data *this) > +static int gpmi_nfc_apply_timings(struct gpmi_nand_data *this) > { > struct gpmi_nfc_hardware_timing *hw = &this->hw; > struct resources *r = &this->resources; > void __iomem *gpmi_regs = r->gpmi_regs; > unsigned int dll_wait_time_us; > + int ret; > + > + if (GPMI_IS_MX6Q(this)) { Not only for 6Q but for GPMI_IS_MX6 > + ret = __gpmi_enable_clk(this, false); > + if (ret) > + return ret; > + } > > clk_set_rate(r->clock[0], hw->clk_rate); > > + if (GPMI_IS_MX6Q(this)) { > + ret = __gpmi_enable_clk(this, true); > + if (ret) > + return ret; > + } > + > writel(hw->timing0, gpmi_regs + HW_GPMI_TIMING0); > writel(hw->timing1, gpmi_regs + HW_GPMI_TIMING1); > > @@ -739,6 +752,8 @@ static void gpmi_nfc_apply_timings(struct gpmi_nand_data *this) > > /* Wait for the DLL to settle. */ > udelay(dll_wait_time_us); > + > + return 0; > } > > static int gpmi_setup_interface(struct nand_chip *chip, int chipnr, > @@ -2271,7 +2286,9 @@ static int gpmi_nfc_exec_op(struct nand_chip *chip, > */ > if (this->hw.must_apply_timings) { > this->hw.must_apply_timings = false; > - gpmi_nfc_apply_timings(this); > + ret = gpmi_nfc_apply_timings(this); > + if (ret) > + return ret; > } > > dev_dbg(this->dev, "%s: %d instructions\n", __func__, op->ninstrs); > -- > 2.25.1 >
On Tuesday, 12 October 2021, 11:02:24 CEST, Stefan Riedmueller wrote: > According to i.MX 6 erratum ERR007117 gpmi clocks need to be gated off > when doing a gpmi_io clk rate change. So gate the clocks off before > the rate change in nfc_apply_timings and ungate them again after the > change. > > Otherwise this rate change can lead to an unresponsive GPMI core which > results in DMA timeouts and failed driver probe: > > [ 4.072318] gpmi-nand 112000.gpmi-nand: DMA timeout, last DMA > ... > [ 4.370355] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -110 > ... > [ 4.375988] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > [ 4.381524] gpmi-nand 112000.gpmi-nand: Error in ECC-based read: -22 > [ 4.387988] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > [ 4.393535] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > ... > > Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de> > --- > Hi, Hi Stefan, thanks for providing the patches! I am suffering from this problem for some time now and I am happy to get this fixed! > > I'm not sure about the error handling here, if it is actually neccessary. > So some feedback would be nice here. IMHO errors should always be handled. > > Thanks, > Stefan > --- > drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > index a1f7000f033e..326c8a895f1f 100644 > --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > @@ -713,15 +713,28 @@ static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this, > (use_half_period ? BM_GPMI_CTRL1_HALF_PERIOD : 0); > } > > -static void gpmi_nfc_apply_timings(struct gpmi_nand_data *this) > +static int gpmi_nfc_apply_timings(struct gpmi_nand_data *this) > { > struct gpmi_nfc_hardware_timing *hw = &this->hw; > struct resources *r = &this->resources; > void __iomem *gpmi_regs = r->gpmi_regs; > unsigned int dll_wait_time_us; > + int ret; > + > + if (GPMI_IS_MX6Q(this)) { I would like to explicitly see i.MX6UL(L) here (in order to make clear that not only Solo/Dual/Quad are affected): #define GPMI_IS_MX6UL(x) ((x)->devdata->type == IS_MX6Q) ... if (GPMI_IS_MX6Q(this) || GPMI_IS_MX6UL(this)) { But if Han Xu is sure that also SoloX and i.MX7 are affected, GPMI_IS_MX6(x) is fine for me. In this case I would like to explicitly mention this in the commit message, so that nobody will change this later because ERR007117 exists only for Solo/Dual/Quad. > + ret = __gpmi_enable_clk(this, false); > + if (ret) > + return ret; > + } > > clk_set_rate(r->clock[0], hw->clk_rate); > > + if (GPMI_IS_MX6Q(this)) { > + ret = __gpmi_enable_clk(this, true); > + if (ret) > + return ret; > + } > + > writel(hw->timing0, gpmi_regs + HW_GPMI_TIMING0); > writel(hw->timing1, gpmi_regs + HW_GPMI_TIMING1); > > @@ -739,6 +752,8 @@ static void gpmi_nfc_apply_timings(struct gpmi_nand_data *this) > > /* Wait for the DLL to settle. */ > udelay(dll_wait_time_us); > + > + return 0; > } > > static int gpmi_setup_interface(struct nand_chip *chip, int chipnr, > @@ -2271,7 +2286,9 @@ static int gpmi_nfc_exec_op(struct nand_chip *chip, > */ > if (this->hw.must_apply_timings) { > this->hw.must_apply_timings = false; > - gpmi_nfc_apply_timings(this); > + ret = gpmi_nfc_apply_timings(this); > + if (ret) > + return ret; > } > > dev_dbg(this->dev, "%s: %d instructions\n", __func__, op->ninstrs); > Tested-by: Christian Eggers <ceggers@arri.de> # on i.MX6ULL with MT29F4G08ABADAH4 Cc: stable@vger.kernel.org
Hi Han, sorry it took me some time to get back to you. On Wed, 2021-10-13 at 00:01 -0500, Han Xu wrote: > On 21/10/12 11:02AM, Stefan Riedmueller wrote: > > According to i.MX 6 erratum ERR007117 gpmi clocks need to be gated off > > when doing a gpmi_io clk rate change. So gate the clocks off before > > the rate change in nfc_apply_timings and ungate them again after the > > change. > > > > Otherwise this rate change can lead to an unresponsive GPMI core which > > results in DMA timeouts and failed driver probe: > > > > [ 4.072318] gpmi-nand 112000.gpmi-nand: DMA timeout, last DMA > > ... > > [ 4.370355] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -110 > > ... > > [ 4.375988] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > > [ 4.381524] gpmi-nand 112000.gpmi-nand: Error in ECC-based read: -22 > > [ 4.387988] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > > [ 4.393535] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > > ... > > > > Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de> > > --- > > Hi, > > > > I'm not sure about the error handling here, if it is actually neccessary. > > So some feedback would be nice here. > > > > Thanks, > > Stefan > > --- > > drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 21 +++++++++++++++++++-- > > 1 file changed, 19 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > index a1f7000f033e..326c8a895f1f 100644 > > --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > @@ -713,15 +713,28 @@ static void gpmi_nfc_compute_timings(struct > > gpmi_nand_data *this, > > (use_half_period ? BM_GPMI_CTRL1_HALF_PERIOD : > > 0); > > } > > > > -static void gpmi_nfc_apply_timings(struct gpmi_nand_data *this) > > +static int gpmi_nfc_apply_timings(struct gpmi_nand_data *this) > > { > > struct gpmi_nfc_hardware_timing *hw = &this->hw; > > struct resources *r = &this->resources; > > void __iomem *gpmi_regs = r->gpmi_regs; > > unsigned int dll_wait_time_us; > > + int ret; > > + > > + if (GPMI_IS_MX6Q(this)) { > > Not only for 6Q but for GPMI_IS_MX6 Can you confirm that i.MX 6SX and i.MX 7 are affected by the ERR007117 erratum as well? Because the i.MX 6UL/ULL are included by the GPMI_IS_MX6Q already. Or do we need another macro for i.MX 6UL/ULL as Christian has suggested earlier? Thanks, Stefan > > > + ret = __gpmi_enable_clk(this, false); > > + if (ret) > > + return ret; > > + } > > > > clk_set_rate(r->clock[0], hw->clk_rate); > > > > + if (GPMI_IS_MX6Q(this)) { > > + ret = __gpmi_enable_clk(this, true); > > + if (ret) > > + return ret; > > + } > > + > > writel(hw->timing0, gpmi_regs + HW_GPMI_TIMING0); > > writel(hw->timing1, gpmi_regs + HW_GPMI_TIMING1); > > > > @@ -739,6 +752,8 @@ static void gpmi_nfc_apply_timings(struct > > gpmi_nand_data *this) > > > > /* Wait for the DLL to settle. */ > > udelay(dll_wait_time_us); > > + > > + return 0; > > } > > > > static int gpmi_setup_interface(struct nand_chip *chip, int chipnr, > > @@ -2271,7 +2286,9 @@ static int gpmi_nfc_exec_op(struct nand_chip *chip, > > */ > > if (this->hw.must_apply_timings) { > > this->hw.must_apply_timings = false; > > - gpmi_nfc_apply_timings(this); > > + ret = gpmi_nfc_apply_timings(this); > > + if (ret) > > + return ret; > > } > > > > dev_dbg(this->dev, "%s: %d instructions\n", __func__, op->ninstrs); > > -- > > 2.25.1 > >
On 21/10/22 08:45AM, Stefan Riedmüller wrote: > Hi Han, > > sorry it took me some time to get back to you. > > On Wed, 2021-10-13 at 00:01 -0500, Han Xu wrote: > > On 21/10/12 11:02AM, Stefan Riedmueller wrote: > > > According to i.MX 6 erratum ERR007117 gpmi clocks need to be gated off > > > when doing a gpmi_io clk rate change. So gate the clocks off before > > > the rate change in nfc_apply_timings and ungate them again after the > > > change. > > > > > > Otherwise this rate change can lead to an unresponsive GPMI core which > > > results in DMA timeouts and failed driver probe: > > > > > > [ 4.072318] gpmi-nand 112000.gpmi-nand: DMA timeout, last DMA > > > ... > > > [ 4.370355] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -110 > > > ... > > > [ 4.375988] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > > > [ 4.381524] gpmi-nand 112000.gpmi-nand: Error in ECC-based read: -22 > > > [ 4.387988] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > > > [ 4.393535] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > > > ... > > > > > > Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de> > > > --- > > > Hi, > > > > > > I'm not sure about the error handling here, if it is actually neccessary. > > > So some feedback would be nice here. > > > > > > Thanks, > > > Stefan > > > --- > > > drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 21 +++++++++++++++++++-- > > > 1 file changed, 19 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > > b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > > index a1f7000f033e..326c8a895f1f 100644 > > > --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > > +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > > @@ -713,15 +713,28 @@ static void gpmi_nfc_compute_timings(struct > > > gpmi_nand_data *this, > > > (use_half_period ? BM_GPMI_CTRL1_HALF_PERIOD : > > > 0); > > > } > > > > > > -static void gpmi_nfc_apply_timings(struct gpmi_nand_data *this) > > > +static int gpmi_nfc_apply_timings(struct gpmi_nand_data *this) > > > { > > > struct gpmi_nfc_hardware_timing *hw = &this->hw; > > > struct resources *r = &this->resources; > > > void __iomem *gpmi_regs = r->gpmi_regs; > > > unsigned int dll_wait_time_us; > > > + int ret; > > > + > > > + if (GPMI_IS_MX6Q(this)) { > > > > Not only for 6Q but for GPMI_IS_MX6 > > Can you confirm that i.MX 6SX and i.MX 7 are affected by the ERR007117 erratum > as well? Because the i.MX 6UL/ULL are included by the GPMI_IS_MX6Q already. i.MX6SX has the glitch issue for sure, but I can double check if it's due to the same errata. I will also go check if the errata will affect i.MX7 > > Or do we need another macro for i.MX 6UL/ULL as Christian has suggested > earlier? > > Thanks, > Stefan > > > > > > + ret = __gpmi_enable_clk(this, false); > > > + if (ret) > > > + return ret; > > > + } > > > > > > clk_set_rate(r->clock[0], hw->clk_rate); > > > > > > + if (GPMI_IS_MX6Q(this)) { > > > + ret = __gpmi_enable_clk(this, true); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > writel(hw->timing0, gpmi_regs + HW_GPMI_TIMING0); > > > writel(hw->timing1, gpmi_regs + HW_GPMI_TIMING1); > > > > > > @@ -739,6 +752,8 @@ static void gpmi_nfc_apply_timings(struct > > > gpmi_nand_data *this) > > > > > > /* Wait for the DLL to settle. */ > > > udelay(dll_wait_time_us); > > > + > > > + return 0; > > > } > > > > > > static int gpmi_setup_interface(struct nand_chip *chip, int chipnr, > > > @@ -2271,7 +2286,9 @@ static int gpmi_nfc_exec_op(struct nand_chip *chip, > > > */ > > > if (this->hw.must_apply_timings) { > > > this->hw.must_apply_timings = false; > > > - gpmi_nfc_apply_timings(this); > > > + ret = gpmi_nfc_apply_timings(this); > > > + if (ret) > > > + return ret; > > > } > > > > > > dev_dbg(this->dev, "%s: %d instructions\n", __func__, op->ninstrs); > > > -- > > > 2.25.1 > > >
Hi Han, On Fri, 2021-10-22 at 09:35 -0500, han.xu@nxp.com wrote: > On 21/10/22 08:45AM, Stefan Riedmüller wrote: > > Hi Han, > > > > sorry it took me some time to get back to you. > > > > On Wed, 2021-10-13 at 00:01 -0500, Han Xu wrote: > > > On 21/10/12 11:02AM, Stefan Riedmueller wrote: > > > > According to i.MX 6 erratum ERR007117 gpmi clocks need to be gated off > > > > when doing a gpmi_io clk rate change. So gate the clocks off before > > > > the rate change in nfc_apply_timings and ungate them again after the > > > > change. > > > > > > > > Otherwise this rate change can lead to an unresponsive GPMI core which > > > > results in DMA timeouts and failed driver probe: > > > > > > > > [ 4.072318] gpmi-nand 112000.gpmi-nand: DMA timeout, last DMA > > > > ... > > > > [ 4.370355] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -110 > > > > ... > > > > [ 4.375988] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > > > > [ 4.381524] gpmi-nand 112000.gpmi-nand: Error in ECC-based read: > > > > -22 > > > > [ 4.387988] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > > > > [ 4.393535] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > > > > ... > > > > > > > > Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de> > > > > --- > > > > Hi, > > > > > > > > I'm not sure about the error handling here, if it is actually > > > > neccessary. > > > > So some feedback would be nice here. > > > > > > > > Thanks, > > > > Stefan > > > > --- > > > > drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 21 +++++++++++++++++++-- > > > > 1 file changed, 19 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > > > b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > > > index a1f7000f033e..326c8a895f1f 100644 > > > > --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > > > +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > > > @@ -713,15 +713,28 @@ static void gpmi_nfc_compute_timings(struct > > > > gpmi_nand_data *this, > > > > (use_half_period ? > > > > BM_GPMI_CTRL1_HALF_PERIOD : > > > > 0); > > > > } > > > > > > > > -static void gpmi_nfc_apply_timings(struct gpmi_nand_data *this) > > > > +static int gpmi_nfc_apply_timings(struct gpmi_nand_data *this) > > > > { > > > > struct gpmi_nfc_hardware_timing *hw = &this->hw; > > > > struct resources *r = &this->resources; > > > > void __iomem *gpmi_regs = r->gpmi_regs; > > > > unsigned int dll_wait_time_us; > > > > + int ret; > > > > + > > > > + if (GPMI_IS_MX6Q(this)) { > > > > > > Not only for 6Q but for GPMI_IS_MX6 > > > > Can you confirm that i.MX 6SX and i.MX 7 are affected by the ERR007117 > > erratum > > as well? Because the i.MX 6UL/ULL are included by the GPMI_IS_MX6Q > > already. > > i.MX6SX has the glitch issue for sure, but I can double check if it's due to > the > same errata. I will also go check if the errata will affect i.MX7 Thanks, that'll be great! I'll wait for your response. Thanks, Stefan > > > Or do we need another macro for i.MX 6UL/ULL as Christian has suggested > > earlier? > > > > Thanks, > > Stefan > > > > > > + ret = __gpmi_enable_clk(this, false); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > > > > > clk_set_rate(r->clock[0], hw->clk_rate); > > > > > > > > + if (GPMI_IS_MX6Q(this)) { > > > > + ret = __gpmi_enable_clk(this, true); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > + > > > > writel(hw->timing0, gpmi_regs + HW_GPMI_TIMING0); > > > > writel(hw->timing1, gpmi_regs + HW_GPMI_TIMING1); > > > > > > > > @@ -739,6 +752,8 @@ static void gpmi_nfc_apply_timings(struct > > > > gpmi_nand_data *this) > > > > > > > > /* Wait for the DLL to settle. */ > > > > udelay(dll_wait_time_us); > > > > + > > > > + return 0; > > > > } > > > > > > > > static int gpmi_setup_interface(struct nand_chip *chip, int chipnr, > > > > @@ -2271,7 +2286,9 @@ static int gpmi_nfc_exec_op(struct nand_chip > > > > *chip, > > > > */ > > > > if (this->hw.must_apply_timings) { > > > > this->hw.must_apply_timings = false; > > > > - gpmi_nfc_apply_timings(this); > > > > + ret = gpmi_nfc_apply_timings(this); > > > > + if (ret) > > > > + return ret; > > > > } > > > > > > > > dev_dbg(this->dev, "%s: %d instructions\n", __func__, op- > > > > >ninstrs); > > > > -- > > > > 2.25.1 > > > >
Hi Han, On Fri, 2021-10-22 at 09:35 -0500, han.xu@nxp.com wrote: > On 21/10/22 08:45AM, Stefan Riedmüller wrote: > > Hi Han, > > > > sorry it took me some time to get back to you. > > > > On Wed, 2021-10-13 at 00:01 -0500, Han Xu wrote: > > > On 21/10/12 11:02AM, Stefan Riedmueller wrote: > > > > According to i.MX 6 erratum ERR007117 gpmi clocks need to be gated off > > > > when doing a gpmi_io clk rate change. So gate the clocks off before > > > > the rate change in nfc_apply_timings and ungate them again after the > > > > change. > > > > > > > > Otherwise this rate change can lead to an unresponsive GPMI core which > > > > results in DMA timeouts and failed driver probe: > > > > > > > > [ 4.072318] gpmi-nand 112000.gpmi-nand: DMA timeout, last DMA > > > > ... > > > > [ 4.370355] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -110 > > > > ... > > > > [ 4.375988] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > > > > [ 4.381524] gpmi-nand 112000.gpmi-nand: Error in ECC-based read: > > > > -22 > > > > [ 4.387988] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > > > > [ 4.393535] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > > > > ... > > > > > > > > Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de> > > > > --- > > > > Hi, > > > > > > > > I'm not sure about the error handling here, if it is actually > > > > neccessary. > > > > So some feedback would be nice here. > > > > > > > > Thanks, > > > > Stefan > > > > --- > > > > drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 21 +++++++++++++++++++-- > > > > 1 file changed, 19 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > > > b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > > > index a1f7000f033e..326c8a895f1f 100644 > > > > --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > > > +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > > > @@ -713,15 +713,28 @@ static void gpmi_nfc_compute_timings(struct > > > > gpmi_nand_data *this, > > > > (use_half_period ? > > > > BM_GPMI_CTRL1_HALF_PERIOD : > > > > 0); > > > > } > > > > > > > > -static void gpmi_nfc_apply_timings(struct gpmi_nand_data *this) > > > > +static int gpmi_nfc_apply_timings(struct gpmi_nand_data *this) > > > > { > > > > struct gpmi_nfc_hardware_timing *hw = &this->hw; > > > > struct resources *r = &this->resources; > > > > void __iomem *gpmi_regs = r->gpmi_regs; > > > > unsigned int dll_wait_time_us; > > > > + int ret; > > > > + > > > > + if (GPMI_IS_MX6Q(this)) { > > > > > > Not only for 6Q but for GPMI_IS_MX6 > > > > Can you confirm that i.MX 6SX and i.MX 7 are affected by the ERR007117 > > erratum > > as well? Because the i.MX 6UL/ULL are included by the GPMI_IS_MX6Q > > already. > > i.MX6SX has the glitch issue for sure, but I can double check if it's due to > the > same errata. I will also go check if the errata will affect i.MX7 Any updates yet? Thanks, Stefan > > > Or do we need another macro for i.MX 6UL/ULL as Christian has suggested > > earlier? > > > > Thanks, > > Stefan > > > > > > + ret = __gpmi_enable_clk(this, false); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > > > > > clk_set_rate(r->clock[0], hw->clk_rate); > > > > > > > > + if (GPMI_IS_MX6Q(this)) { > > > > + ret = __gpmi_enable_clk(this, true); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > + > > > > writel(hw->timing0, gpmi_regs + HW_GPMI_TIMING0); > > > > writel(hw->timing1, gpmi_regs + HW_GPMI_TIMING1); > > > > > > > > @@ -739,6 +752,8 @@ static void gpmi_nfc_apply_timings(struct > > > > gpmi_nand_data *this) > > > > > > > > /* Wait for the DLL to settle. */ > > > > udelay(dll_wait_time_us); > > > > + > > > > + return 0; > > > > } > > > > > > > > static int gpmi_setup_interface(struct nand_chip *chip, int chipnr, > > > > @@ -2271,7 +2286,9 @@ static int gpmi_nfc_exec_op(struct nand_chip > > > > *chip, > > > > */ > > > > if (this->hw.must_apply_timings) { > > > > this->hw.must_apply_timings = false; > > > > - gpmi_nfc_apply_timings(this); > > > > + ret = gpmi_nfc_apply_timings(this); > > > > + if (ret) > > > > + return ret; > > > > } > > > > > > > > dev_dbg(this->dev, "%s: %d instructions\n", __func__, op- > > > > >ninstrs); > > > > -- > > > > 2.25.1 > > > >
On 21/10/28 09:28AM, Stefan Riedmüller wrote: > Hi Han, > > On Fri, 2021-10-22 at 09:35 -0500, han.xu@nxp.com wrote: > > On 21/10/22 08:45AM, Stefan Riedmüller wrote: > > > Hi Han, > > > > > > sorry it took me some time to get back to you. > > > > > > On Wed, 2021-10-13 at 00:01 -0500, Han Xu wrote: > > > > On 21/10/12 11:02AM, Stefan Riedmueller wrote: > > > > > According to i.MX 6 erratum ERR007117 gpmi clocks need to be gated off > > > > > when doing a gpmi_io clk rate change. So gate the clocks off before > > > > > the rate change in nfc_apply_timings and ungate them again after the > > > > > change. > > > > > > > > > > Otherwise this rate change can lead to an unresponsive GPMI core which > > > > > results in DMA timeouts and failed driver probe: > > > > > > > > > > [ 4.072318] gpmi-nand 112000.gpmi-nand: DMA timeout, last DMA > > > > > ... > > > > > [ 4.370355] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -110 > > > > > ... > > > > > [ 4.375988] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > > > > > [ 4.381524] gpmi-nand 112000.gpmi-nand: Error in ECC-based read: > > > > > -22 > > > > > [ 4.387988] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > > > > > [ 4.393535] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 > > > > > ... > > > > > > > > > > Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de> > > > > > --- > > > > > Hi, > > > > > > > > > > I'm not sure about the error handling here, if it is actually > > > > > neccessary. > > > > > So some feedback would be nice here. > > > > > > > > > > Thanks, > > > > > Stefan > > > > > --- > > > > > drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 21 +++++++++++++++++++-- > > > > > 1 file changed, 19 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > > > > b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > > > > index a1f7000f033e..326c8a895f1f 100644 > > > > > --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > > > > +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c > > > > > @@ -713,15 +713,28 @@ static void gpmi_nfc_compute_timings(struct > > > > > gpmi_nand_data *this, > > > > > (use_half_period ? > > > > > BM_GPMI_CTRL1_HALF_PERIOD : > > > > > 0); > > > > > } > > > > > > > > > > -static void gpmi_nfc_apply_timings(struct gpmi_nand_data *this) > > > > > +static int gpmi_nfc_apply_timings(struct gpmi_nand_data *this) > > > > > { > > > > > struct gpmi_nfc_hardware_timing *hw = &this->hw; > > > > > struct resources *r = &this->resources; > > > > > void __iomem *gpmi_regs = r->gpmi_regs; > > > > > unsigned int dll_wait_time_us; > > > > > + int ret; > > > > > + > > > > > + if (GPMI_IS_MX6Q(this)) { > > > > > > > > Not only for 6Q but for GPMI_IS_MX6 > > > > > > Can you confirm that i.MX 6SX and i.MX 7 are affected by the ERR007117 > > > erratum > > > as well? Because the i.MX 6UL/ULL are included by the GPMI_IS_MX6Q > > > already. > > > > i.MX6SX has the glitch issue for sure, but I can double check if it's due to > > the > > same errata. I will also go check if the errata will affect i.MX7 > > Any updates yet? > > Thanks, > Stefan Sorry Stefan, I didn't get useful info so I involved more people for this question. Hopefully can get an answer soon. Or you can just add 6Q and 6UL/ULL first. > > > > > > Or do we need another macro for i.MX 6UL/ULL as Christian has suggested > > > earlier? > > > > > > Thanks, > > > Stefan > > > > > > > > + ret = __gpmi_enable_clk(this, false); > > > > > + if (ret) > > > > > + return ret; > > > > > + } > > > > > > > > > > clk_set_rate(r->clock[0], hw->clk_rate); > > > > > > > > > > + if (GPMI_IS_MX6Q(this)) { > > > > > + ret = __gpmi_enable_clk(this, true); > > > > > + if (ret) > > > > > + return ret; > > > > > + } > > > > > + > > > > > writel(hw->timing0, gpmi_regs + HW_GPMI_TIMING0); > > > > > writel(hw->timing1, gpmi_regs + HW_GPMI_TIMING1); > > > > > > > > > > @@ -739,6 +752,8 @@ static void gpmi_nfc_apply_timings(struct > > > > > gpmi_nand_data *this) > > > > > > > > > > /* Wait for the DLL to settle. */ > > > > > udelay(dll_wait_time_us); > > > > > + > > > > > + return 0; > > > > > } > > > > > > > > > > static int gpmi_setup_interface(struct nand_chip *chip, int chipnr, > > > > > @@ -2271,7 +2286,9 @@ static int gpmi_nfc_exec_op(struct nand_chip > > > > > *chip, > > > > > */ > > > > > if (this->hw.must_apply_timings) { > > > > > this->hw.must_apply_timings = false; > > > > > - gpmi_nfc_apply_timings(this); > > > > > + ret = gpmi_nfc_apply_timings(this); > > > > > + if (ret) > > > > > + return ret; > > > > > } > > > > > > > > > > dev_dbg(this->dev, "%s: %d instructions\n", __func__, op- > > > > > >ninstrs); > > > > > -- > > > > > 2.25.1 > > > > >
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c index a1f7000f033e..326c8a895f1f 100644 --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c @@ -713,15 +713,28 @@ static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this, (use_half_period ? BM_GPMI_CTRL1_HALF_PERIOD : 0); } -static void gpmi_nfc_apply_timings(struct gpmi_nand_data *this) +static int gpmi_nfc_apply_timings(struct gpmi_nand_data *this) { struct gpmi_nfc_hardware_timing *hw = &this->hw; struct resources *r = &this->resources; void __iomem *gpmi_regs = r->gpmi_regs; unsigned int dll_wait_time_us; + int ret; + + if (GPMI_IS_MX6Q(this)) { + ret = __gpmi_enable_clk(this, false); + if (ret) + return ret; + } clk_set_rate(r->clock[0], hw->clk_rate); + if (GPMI_IS_MX6Q(this)) { + ret = __gpmi_enable_clk(this, true); + if (ret) + return ret; + } + writel(hw->timing0, gpmi_regs + HW_GPMI_TIMING0); writel(hw->timing1, gpmi_regs + HW_GPMI_TIMING1); @@ -739,6 +752,8 @@ static void gpmi_nfc_apply_timings(struct gpmi_nand_data *this) /* Wait for the DLL to settle. */ udelay(dll_wait_time_us); + + return 0; } static int gpmi_setup_interface(struct nand_chip *chip, int chipnr, @@ -2271,7 +2286,9 @@ static int gpmi_nfc_exec_op(struct nand_chip *chip, */ if (this->hw.must_apply_timings) { this->hw.must_apply_timings = false; - gpmi_nfc_apply_timings(this); + ret = gpmi_nfc_apply_timings(this); + if (ret) + return ret; } dev_dbg(this->dev, "%s: %d instructions\n", __func__, op->ninstrs);
According to i.MX 6 erratum ERR007117 gpmi clocks need to be gated off when doing a gpmi_io clk rate change. So gate the clocks off before the rate change in nfc_apply_timings and ungate them again after the change. Otherwise this rate change can lead to an unresponsive GPMI core which results in DMA timeouts and failed driver probe: [ 4.072318] gpmi-nand 112000.gpmi-nand: DMA timeout, last DMA ... [ 4.370355] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -110 ... [ 4.375988] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 [ 4.381524] gpmi-nand 112000.gpmi-nand: Error in ECC-based read: -22 [ 4.387988] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 [ 4.393535] gpmi-nand 112000.gpmi-nand: Chip: 0, Error -22 ... Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de> --- Hi, I'm not sure about the error handling here, if it is actually neccessary. So some feedback would be nice here. Thanks, Stefan --- drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)