| Message ID | 711ef434-1208-4a9b-1c0d-519395251531@cogentembedded.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: renesas_sdhi_internal_dmac: mask DMAC interrupts | expand |
On Fri, Aug 17, 2018 at 07:54:09PM +0300, Sergei Shtylyov wrote: > I have encountered an interrupt storm during the eMMC chip probing (and > the chip finally didn't get detected). It turned out that U-Boot left > the SDHI DMA interrupts enabled while the Linux driver didn't use those. > Masking those interrupts in renesas_sdhi_internal_dmac_request_dma() gets > rid of both issues... > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Thanks for fixing this issue. > > --- > The patch is against Ulf Hansson's 'mmc.git' repo's 'fixes' branch. > > drivers/mmc/host/renesas_sdhi_internal_dmac.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > Index: mmc/drivers/mmc/host/renesas_sdhi_internal_dmac.c > =================================================================== > --- mmc.orig/drivers/mmc/host/renesas_sdhi_internal_dmac.c > +++ mmc/drivers/mmc/host/renesas_sdhi_internal_dmac.c > @@ -51,10 +51,12 @@ > #define INFO1_CLEAR 0 > #define INFO1_DTRANEND1 BIT(17) > #define INFO1_DTRANEND0 BIT(16) > +#define INFO1_RESERVED_BITS GENMASK_ULL(32, 0) 31? Also, RESERVED_BITS is not quite proper. Not all of those bits are reserved. Maybe CLEAR_MASK? > /* DM_CM_INFO2 and DM_CM_INFO2_MASK */ > #define INFO2_DTRANERR1 BIT(17) > #define INFO2_DTRANERR0 BIT(16) > +#define INFO2_RESERVED_BITS GENMASK_ULL(32, 0) Same as above. Maybe we even need one define only? > + /* > + * We don't use the DMA interrupts, but they might have been enabled > + * by a bootloader, so mask them to avoid an interrupt storm... > + */ Two spaces after ',' looks odd to me. Also, no need for "..." I'd even think with a name like CLEAR_MASK, the comment could even go. > + renesas_sdhi_internal_dmac_dm_write(host, DM_CM_INFO1_MASK, > + INFO1_RESERVED_BITS); > + renesas_sdhi_internal_dmac_dm_write(host, DM_CM_INFO2_MASK, > + INFO2_RESERVED_BITS);
Hello! On 08/20/2018 07:38 PM, Wolfram Sang wrote: >> I have encountered an interrupt storm during the eMMC chip probing (and >> the chip finally didn't get detected). It turned out that U-Boot left >> the SDHI DMA interrupts enabled while the Linux driver didn't use those. >> Masking those interrupts in renesas_sdhi_internal_dmac_request_dma() gets >> rid of both issues... >> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > Thanks for fixing this issue. > >> >> --- >> The patch is against Ulf Hansson's 'mmc.git' repo's 'fixes' branch. >> >> drivers/mmc/host/renesas_sdhi_internal_dmac.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> Index: mmc/drivers/mmc/host/renesas_sdhi_internal_dmac.c >> =================================================================== >> --- mmc.orig/drivers/mmc/host/renesas_sdhi_internal_dmac.c >> +++ mmc/drivers/mmc/host/renesas_sdhi_internal_dmac.c >> @@ -51,10 +51,12 @@ >> #define INFO1_CLEAR 0 >> #define INFO1_DTRANEND1 BIT(17) >> #define INFO1_DTRANEND0 BIT(16) >> +#define INFO1_RESERVED_BITS GENMASK_ULL(32, 0) > > 31? Indeed. Than RST_RESERVED_BITS (from which I copied w/o much thinking) is also wrong! > Also, RESERVED_BITS is not quite proper. Not all of those bits are > reserved. Maybe CLEAR_MASK? Indeed, would seem better... but RST_RESERVED_BITS also needs fixing then... >> /* DM_CM_INFO2 and DM_CM_INFO2_MASK */ >> #define INFO2_DTRANERR1 BIT(17) >> #define INFO2_DTRANERR0 BIT(16) >> +#define INFO2_RESERVED_BITS GENMASK_ULL(32, 0) > > Same as above. Maybe we even need one define only? No, I think 2 separate regs deserve 2 separate masks. >> + /* >> + * We don't use the DMA interrupts, but they might have been enabled >> + * by a bootloader, so mask them to avoid an interrupt storm... >> + */ > > Two spaces after ',' looks odd to me. Also, no need for "..." Both are my preferences. Yes, I probably should have worked in the typesetting industry... :-) > I'd even think with a name like CLEAR_MASK, the comment could even go. Disagree here. The register #defines don't provide enough info on what's going on... >> + renesas_sdhi_internal_dmac_dm_write(host, DM_CM_INFO1_MASK, >> + INFO1_RESERVED_BITS); >> + renesas_sdhi_internal_dmac_dm_write(host, DM_CM_INFO2_MASK, >> + INFO2_RESERVED_BITS); MBR, Sergei
> >> /* DM_CM_INFO2 and DM_CM_INFO2_MASK */ > >> #define INFO2_DTRANERR1 BIT(17) > >> #define INFO2_DTRANERR0 BIT(16) > >> +#define INFO2_RESERVED_BITS GENMASK_ULL(32, 0) > > > > Same as above. Maybe we even need one define only? > > No, I think 2 separate regs deserve 2 separate masks. Whatever. I am not into bike-shedding. It works, too. > >> + /* > >> + * We don't use the DMA interrupts, but they might have been enabled > >> + * by a bootloader, so mask them to avoid an interrupt storm... > >> + */ > > > > Two spaces after ',' looks odd to me. Also, no need for "..." > > Both are my preferences. Yes, I probably should have worked in the > typesetting industry... :-) But where is the continuation of "..."? > > I'd even think with a name like CLEAR_MASK, the comment could even go. > > Disagree here. The register #defines don't provide enough info on what's > going on... Masking out all interrupts at the beginning is pretty standard behaviour in my book. Well, at least, please shorten it to sth like "Disable interrupts, we don't use them".
Index: mmc/drivers/mmc/host/renesas_sdhi_internal_dmac.c =================================================================== --- mmc.orig/drivers/mmc/host/renesas_sdhi_internal_dmac.c +++ mmc/drivers/mmc/host/renesas_sdhi_internal_dmac.c @@ -51,10 +51,12 @@ #define INFO1_CLEAR 0 #define INFO1_DTRANEND1 BIT(17) #define INFO1_DTRANEND0 BIT(16) +#define INFO1_RESERVED_BITS GENMASK_ULL(32, 0) /* DM_CM_INFO2 and DM_CM_INFO2_MASK */ #define INFO2_DTRANERR1 BIT(17) #define INFO2_DTRANERR0 BIT(16) +#define INFO2_RESERVED_BITS GENMASK_ULL(32, 0) /* * Specification of this driver: @@ -236,6 +238,15 @@ renesas_sdhi_internal_dmac_request_dma(s { struct renesas_sdhi *priv = host_to_priv(host); + /* + * We don't use the DMA interrupts, but they might have been enabled + * by a bootloader, so mask them to avoid an interrupt storm... + */ + renesas_sdhi_internal_dmac_dm_write(host, DM_CM_INFO1_MASK, + INFO1_RESERVED_BITS); + renesas_sdhi_internal_dmac_dm_write(host, DM_CM_INFO2_MASK, + INFO2_RESERVED_BITS); + /* Each value is set to non-zero to assume "enabling" each DMA */ host->chan_rx = host->chan_tx = (void *)0xdeadbeaf;
I have encountered an interrupt storm during the eMMC chip probing (and the chip finally didn't get detected). It turned out that U-Boot left the SDHI DMA interrupts enabled while the Linux driver didn't use those. Masking those interrupts in renesas_sdhi_internal_dmac_request_dma() gets rid of both issues... Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- The patch is against Ulf Hansson's 'mmc.git' repo's 'fixes' branch. drivers/mmc/host/renesas_sdhi_internal_dmac.c | 11 +++++++++++ 1 file changed, 11 insertions(+)