| Message ID | 20180314193051.16791-1-Evgeniy.Didin@synopsys.com |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: dw_mmc: fix falling from idmac to PIO mode when dw_mci_reset occurs | expand |
On 14 March 2018 at 20:30, Evgeniy Didin <Evgeniy.Didin@synopsys.com> wrote: > It was found that in IDMAC mode after soft-reset driver switches > to PIO mode. > > That's what happens in case of DTO timeout overflow calculation failure: > 1. soft-reset is called > 2. driver restarts dma > 3. descriptors states are checked, one of descriptor is owned by the IDMAC. > 4. driver can't use DMA and then switches to PIO mode. > > Failure was already fixed in: > https://www.spinics.net/lists/linux-mmc/msg48125.html. Evgeniy, It seems like I should squash this fix into the above commit? Makes sense? Kind regards Uffe > > Behaviour while soft-reset is not something we except or > even want to happen. So we switch from dw_mci_idmac_reset > to dw_mci_idmac_init, so descriptors are cleaned before starting dma. > > And while at it explicitly zero des0 which otherwise might > contain garbage as being allocated by dmam_alloc_coherent(). > > Signed-off-by: Evgeniy Didin <Evgeniy.Didin@synopsys.com> > Cc: Jaehoon Chung <jh80.chung@samsung.com> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > Cc: Shawn Lin <shawn.lin@rock-chips.com> > Cc: Alexey Brodkin <abrodkin@synopsys.com> > Cc: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> > Cc: linux-snps-arc@lists.infradead.org > --- > drivers/mmc/host/dw_mmc.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 0aa39975f33b..26f2ac107702 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -558,6 +558,7 @@ static int dw_mci_idmac_init(struct dw_mci *host) > (sizeof(struct idmac_desc_64addr) * > (i + 1))) >> 32; > /* Initialize reserved and buffer size fields to "0" */ > + p->des0 = 0; > p->des1 = 0; > p->des2 = 0; > p->des3 = 0; > @@ -580,6 +581,7 @@ static int dw_mci_idmac_init(struct dw_mci *host) > i++, p++) { > p->des3 = cpu_to_le32(host->sg_dma + > (sizeof(struct idmac_desc) * (i + 1))); > + p->des0 = 0; > p->des1 = 0; > } > > @@ -1795,8 +1797,8 @@ static bool dw_mci_reset(struct dw_mci *host) > } > > if (host->use_dma == TRANS_MODE_IDMAC) > - /* It is also recommended that we reset and reprogram idmac */ > - dw_mci_idmac_reset(host); > + /* It is also required that we reinit idmac */ > + dw_mci_idmac_init(host); > > ret = true; > > -- > 2.11.0 >
Hello Ulf, On Thu, 2018-03-15 at 10:57 +0100, Ulf Hansson wrote: > On 14 March 2018 at 20:30, Evgeniy Didin <Evgeniy.Didin@synopsys.com> wrote: > > It was found that in IDMAC mode after soft-reset driver switches > > to PIO mode. > > > > That's what happens in case of DTO timeout overflow calculation failure: > > 1. soft-reset is called > > 2. driver restarts dma > > 3. descriptors states are checked, one of descriptor is owned by the IDMAC. > > 4. driver can't use DMA and then switches to PIO mode. > > > > Failure was already fixed in: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_linux-2Dmmc_msg48125.html&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=vQk-RIbjwN0zvlwiMSpq3LYUTNf7Gqc4ujhosYI > > TtAw&m=6rPWpKUYQD3kY-2OEikJUWyEJvwKJljWHFC8rd2TCak&s=JjX0Dx8-eSyW2cATMsnG1eAzrKgoDkS1bcS5XYrVtlE&e=. > > Evgeniy, > > It seems like I should squash this fix into the above commit? Makes sense? IMHO it is not a good idea, because this fix is orthogonal to the above commit. Debugging drto overflow issue we found this strange behavior. This might be general issue of soft-reset. So I think this should be separate patch. Also if these patches are independent, it will be easier to backport them. Best regards, Evgeniy Didin > Kind regards > Uffe > > > > > Behaviour while soft-reset is not something we except or > > even want to happen. So we switch from dw_mci_idmac_reset > > to dw_mci_idmac_init, so descriptors are cleaned before starting dma. > > > > And while at it explicitly zero des0 which otherwise might > > contain garbage as being allocated by dmam_alloc_coherent(). > > > > Signed-off-by: Evgeniy Didin <Evgeniy.Didin@synopsys.com> > > Cc: Jaehoon Chung <jh80.chung@samsung.com> > > Cc: Ulf Hansson <ulf.hansson@linaro.org> > > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > > Cc: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > > Cc: Shawn Lin <shawn.lin@rock-chips.com> > > Cc: Alexey Brodkin <abrodkin@synopsys.com> > > Cc: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> > > Cc: linux-snps-arc@lists.infradead.org > > --- > > drivers/mmc/host/dw_mmc.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > > index 0aa39975f33b..26f2ac107702 100644 > > --- a/drivers/mmc/host/dw_mmc.c > > +++ b/drivers/mmc/host/dw_mmc.c > > @@ -558,6 +558,7 @@ static int dw_mci_idmac_init(struct dw_mci *host) > > (sizeof(struct idmac_desc_64addr) * > > (i + 1))) >> 32; > > /* Initialize reserved and buffer size fields to "0" */ > > + p->des0 = 0; > > p->des1 = 0; > > p->des2 = 0; > > p->des3 = 0; > > @@ -580,6 +581,7 @@ static int dw_mci_idmac_init(struct dw_mci *host) > > i++, p++) { > > p->des3 = cpu_to_le32(host->sg_dma + > > (sizeof(struct idmac_desc) * (i + 1))); > > + p->des0 = 0; > > p->des1 = 0; > > } > > > > @@ -1795,8 +1797,8 @@ static bool dw_mci_reset(struct dw_mci *host) > > } > > > > if (host->use_dma == TRANS_MODE_IDMAC) > > - /* It is also recommended that we reset and reprogram idmac */ > > - dw_mci_idmac_reset(host); > > + /* It is also required that we reinit idmac */ > > + dw_mci_idmac_init(host); > > > > ret = true; > > > > -- > > 2.11.0 > >
On 15 March 2018 at 14:18, Evgeniy Didin <Evgeniy.Didin@synopsys.com> wrote: > Hello Ulf, > > On Thu, 2018-03-15 at 10:57 +0100, Ulf Hansson wrote: >> On 14 March 2018 at 20:30, Evgeniy Didin <Evgeniy.Didin@synopsys.com> wrote: >> > It was found that in IDMAC mode after soft-reset driver switches >> > to PIO mode. >> > >> > That's what happens in case of DTO timeout overflow calculation failure: >> > 1. soft-reset is called >> > 2. driver restarts dma >> > 3. descriptors states are checked, one of descriptor is owned by the IDMAC. >> > 4. driver can't use DMA and then switches to PIO mode. >> > >> > Failure was already fixed in: >> > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_linux-2Dmmc_msg48125.html&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=vQk-RIbjwN0zvlwiMSpq3LYUTNf7Gqc4ujhosYI >> > TtAw&m=6rPWpKUYQD3kY-2OEikJUWyEJvwKJljWHFC8rd2TCak&s=JjX0Dx8-eSyW2cATMsnG1eAzrKgoDkS1bcS5XYrVtlE&e=. >> >> Evgeniy, >> >> It seems like I should squash this fix into the above commit? Makes sense? > IMHO it is not a good idea, because this fix is orthogonal to the above commit. > Debugging drto overflow issue we found this strange behavior. Okay! Thanks for clarifying. > This might be general issue of soft-reset. So I think this should be separate patch. > Also if these patches are independent, it will be easier to backport them. Should I add a stable tag then? [...] Kind regards Uffe
On Thu, 2018-03-15 at 14:39 +0100, Ulf Hansson wrote: > On 15 March 2018 at 14:18, Evgeniy Didin <Evgeniy.Didin@synopsys.com> wrote: > > Hello Ulf, > > > > On Thu, 2018-03-15 at 10:57 +0100, Ulf Hansson wrote: > > > On 14 March 2018 at 20:30, Evgeniy Didin <Evgeniy.Didin@synopsys.com> wrote: > > > > It was found that in IDMAC mode after soft-reset driver switches > > > > to PIO mode. > > > > > > > > That's what happens in case of DTO timeout overflow calculation failure: > > > > 1. soft-reset is called > > > > 2. driver restarts dma > > > > 3. descriptors states are checked, one of descriptor is owned by the IDMAC. > > > > 4. driver can't use DMA and then switches to PIO mode. > > > > > > > > Failure was already fixed in: > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_linux-2Dmmc_msg48125.html&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=vQk-RIbjwN0zvlwiMSpq3LYUTNf7Gqc4ujh > > > > osYI > > > > TtAw&m=6rPWpKUYQD3kY-2OEikJUWyEJvwKJljWHFC8rd2TCak&s=JjX0Dx8-eSyW2cATMsnG1eAzrKgoDkS1bcS5XYrVtlE&e=. > > > > > > Evgeniy, > > > > > > It seems like I should squash this fix into the above commit? Makes sense? > > > > IMHO it is not a good idea, because this fix is orthogonal to the above commit. > > Debugging drto overflow issue we found this strange behavior. > > Okay! Thanks for clarifying. > > > This might be general issue of soft-reset. So I think this should be separate patch. > > Also if these patches are independent, it will be easier to backport them. > > Should I add a stable tag then? Sure. The issue can happen since v3.17, because starting with this version function dw_mci_reset was implemented. Commit 3a33a94ce270 ("mmc: dw_mmc: change to use recommended reset procedure"). Best regards, Evgeniy Didin
On 15 March 2018 at 18:24, Evgeniy Didin <Evgeniy.Didin@synopsys.com> wrote: > On Thu, 2018-03-15 at 14:39 +0100, Ulf Hansson wrote: >> On 15 March 2018 at 14:18, Evgeniy Didin <Evgeniy.Didin@synopsys.com> wrote: >> > Hello Ulf, >> > >> > On Thu, 2018-03-15 at 10:57 +0100, Ulf Hansson wrote: >> > > On 14 March 2018 at 20:30, Evgeniy Didin <Evgeniy.Didin@synopsys.com> wrote: >> > > > It was found that in IDMAC mode after soft-reset driver switches >> > > > to PIO mode. >> > > > >> > > > That's what happens in case of DTO timeout overflow calculation failure: >> > > > 1. soft-reset is called >> > > > 2. driver restarts dma >> > > > 3. descriptors states are checked, one of descriptor is owned by the IDMAC. >> > > > 4. driver can't use DMA and then switches to PIO mode. >> > > > >> > > > Failure was already fixed in: >> > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_linux-2Dmmc_msg48125.html&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=vQk-RIbjwN0zvlwiMSpq3LYUTNf7Gqc4ujh >> > > > osYI >> > > > TtAw&m=6rPWpKUYQD3kY-2OEikJUWyEJvwKJljWHFC8rd2TCak&s=JjX0Dx8-eSyW2cATMsnG1eAzrKgoDkS1bcS5XYrVtlE&e=. >> > > >> > > Evgeniy, >> > > >> > > It seems like I should squash this fix into the above commit? Makes sense? >> > >> > IMHO it is not a good idea, because this fix is orthogonal to the above commit. >> > Debugging drto overflow issue we found this strange behavior. >> >> Okay! Thanks for clarifying. >> >> > This might be general issue of soft-reset. So I think this should be separate patch. >> > Also if these patches are independent, it will be easier to backport them. >> >> Should I add a stable tag then? > > Sure. The issue can happen since v3.17, because starting with this version function dw_mci_reset was implemented. > Commit 3a33a94ce270 ("mmc: dw_mmc: change to use recommended reset procedure"). Alright. I tried applying it, and the first kernel version that seems to work is 4.4. Thus I am adding that tag, if you or other people want it for an earlier version, please to send a backported version to @stable. Kind regards Uffe
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 0aa39975f33b..26f2ac107702 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -558,6 +558,7 @@ static int dw_mci_idmac_init(struct dw_mci *host) (sizeof(struct idmac_desc_64addr) * (i + 1))) >> 32; /* Initialize reserved and buffer size fields to "0" */ + p->des0 = 0; p->des1 = 0; p->des2 = 0; p->des3 = 0; @@ -580,6 +581,7 @@ static int dw_mci_idmac_init(struct dw_mci *host) i++, p++) { p->des3 = cpu_to_le32(host->sg_dma + (sizeof(struct idmac_desc) * (i + 1))); + p->des0 = 0; p->des1 = 0; } @@ -1795,8 +1797,8 @@ static bool dw_mci_reset(struct dw_mci *host) } if (host->use_dma == TRANS_MODE_IDMAC) - /* It is also recommended that we reset and reprogram idmac */ - dw_mci_idmac_reset(host); + /* It is also required that we reinit idmac */ + dw_mci_idmac_init(host); ret = true;
It was found that in IDMAC mode after soft-reset driver switches to PIO mode. That's what happens in case of DTO timeout overflow calculation failure: 1. soft-reset is called 2. driver restarts dma 3. descriptors states are checked, one of descriptor is owned by the IDMAC. 4. driver can't use DMA and then switches to PIO mode. Failure was already fixed in: https://www.spinics.net/lists/linux-mmc/msg48125.html. Behaviour while soft-reset is not something we except or even want to happen. So we switch from dw_mci_idmac_reset to dw_mci_idmac_init, so descriptors are cleaned before starting dma. And while at it explicitly zero des0 which otherwise might contain garbage as being allocated by dmam_alloc_coherent(). Signed-off-by: Evgeniy Didin <Evgeniy.Didin@synopsys.com> Cc: Jaehoon Chung <jh80.chung@samsung.com> Cc: Ulf Hansson <ulf.hansson@linaro.org> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Jisheng Zhang <Jisheng.Zhang@synaptics.com> Cc: Shawn Lin <shawn.lin@rock-chips.com> Cc: Alexey Brodkin <abrodkin@synopsys.com> Cc: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> Cc: linux-snps-arc@lists.infradead.org --- drivers/mmc/host/dw_mmc.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)