Received-SPF: pass (google.com: domain of linux-kernel+bounces-198751-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <55cea03b-e685-4f7b-a93b-cb464417d364@intel.com>
Date: Mon, 3 Jun 2024 11:01:23 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] mmc: sdhci-of-dwcmshc: don't enable CQE without a
 suitable irq handler
To: Heiko Stuebner <heiko@sntech.de>, ulf.hansson@linaro.org
Cc: serghox@gmail.com, linux-mmc@vger.kernel.org,
 linux-kernel@vger.kernel.org, quentin.schulz@cherry.de,
 Heiko Stuebner <heiko.stuebner@cherry.de>
References: <20240530215532.2192423-1-heiko@sntech.de>
Content-Language: en-US
From: Adrian Hunter <adrian.hunter@intel.com>
Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki,
 Business Identity Code: 0357606 - 4, Domiciled in Helsinki
In-Reply-To: <20240530215532.2192423-1-heiko@sntech.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 31/05/24 00:55, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@cherry.de>
> 
> supports-cqe is an established dt property so can appear in devicetrees
> at any time. CQE support in the sdhci-of-dwcmshc driver does require a
> special irq handler in the platform-specific ops, to handle the CQE
> interrupt.
> 
> Without this special handler we end up with a spew of unhandled interrupt
> messages on devices with supports-cqe property but without irq handler:
> 
> [   11.624143] mmc0: Unexpected interrupt 0x00004000.
> [   11.629504] mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
> [   11.636711] mmc0: sdhci: Sys addr:  0x00000008 | Version:  0x00000005
> [   11.643919] mmc0: sdhci: Blk size:  0x00007200 | Blk cnt:  0x00000000
> [   11.651128] mmc0: sdhci: Argument:  0x00018000 | Trn mode: 0x00000033
> [   11.658336] mmc0: sdhci: Present:   0x13f700f0 | Host ctl: 0x00000034
> [   11.665545] mmc0: sdhci: Power:     0x00000001 | Blk gap:  0x00000000
> [   11.672753] mmc0: sdhci: Wake-up:   0x00000000 | Clock:    0x00000407
> [   11.679961] mmc0: sdhci: Timeout:   0x0000000e | Int stat: 0x00004000
> [   11.687169] mmc0: sdhci: Int enab:  0x02ff4000 | Sig enab: 0x02ff4000
> [   11.694378] mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000000
> [   11.701586] mmc0: sdhci: Caps:      0x226dc881 | Caps_1:   0x08000007
> [   11.708794] mmc0: sdhci: Cmd:       0x00000d1e | Max curr: 0x00000000
> [   11.716003] mmc0: sdhci: Resp[0]:   0x00000900 | Resp[1]:  0x00000000
> [   11.723211] mmc0: sdhci: Resp[2]:   0x328f5903 | Resp[3]:  0x000007cd
> [   11.730419] mmc0: sdhci: Host ctl2: 0x0000000f
> [   11.735392] mmc0: sdhci: ADMA Err:  0x00000000 | ADMA Ptr: 0xee28f008
> [   11.742600] mmc0: sdhci: ============================================
> 
> So don't enable CQE if a usable interrupt handler is not defined and warn
> instead about this fact.
> 
> Fixes: 53ab7f7fe412 ("mmc: sdhci-of-dwcmshc: Implement SDHCI CQE support")
> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
> ---
> My rk3588-tiger and rk3588-jaguar devicetrees had an accidential
> supports-cqe in their devicetree, which made me run into this problem
> with 6.10-rc1 .
> 
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 39edf04fedcf7..4410d4523728d 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -1254,10 +1254,14 @@ static int dwcmshc_probe(struct platform_device *pdev)
>  
>  	/* Setup Command Queue Engine if enabled */
>  	if (device_property_read_bool(&pdev->dev, "supports-cqe")) {
> -		priv->vendor_specific_area2 =
> -			sdhci_readw(host, DWCMSHC_P_VENDOR_AREA2);
> +		if (pltfm_data && pltfm_data->ops && pltfm_data->ops->irq) {

->irq() could be used for other things, so checking it for CQE
support is not appropriate.

If necessary, it would be better to flag which variants support
CQE in their platform data.

However that would probably mean introducing something
like struct dwcmshc_pltfm_data as described here:

https://lore.kernel.org/linux-mmc/ed900af1-f090-49a9-bc7e-363a28a4ac2b@intel.com/


> +			priv->vendor_specific_area2 =
> +				sdhci_readw(host, DWCMSHC_P_VENDOR_AREA2);
>  
> -		dwcmshc_cqhci_init(host, pdev);
> +			dwcmshc_cqhci_init(host, pdev);
> +		} else {
> +			dev_warn(&pdev->dev, "can't enable cqe support without irq handler\n");
> +		}
>  	}
>  
>  	if (rk_priv)