Re: [PATCH] mmc: renesas_sdhi_internal_dmac: mask DMAC interrupts

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxxxx>
> 
> 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



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux