Date: Fri, 14 Jun 2024 14:30:43 +0300
From: Adrian Hunter <adrian.hunter@...el.com>
To: Heiko Stuebner <heiko@...ech.de>, ulf.hansson@...aro.org
Cc: serghox@...il.com, linux-mmc@...r.kernel.org,
 linux-kernel@...r.kernel.org, quentin.schulz@...rry.de,
 Heiko Stuebner <heiko.stuebner@...rry.de>
Subject: Re: [PATCH] mmc: sdhci-of-dwcmshc: don't enable CQE without a
 suitable irq handler

On 3/06/24 15:40, Heiko Stuebner wrote:
> Am Montag, 3. Juni 2024, 13:49:34 CEST schrieb Adrian Hunter:
>> On 3/06/24 14:26, Heiko Stübner wrote:
>>> Am Montag, 3. Juni 2024, 10:01:23 CEST schrieb Adrian Hunter:
>>>> On 31/05/24 00:55, Heiko Stuebner wrote:
>>>>> From: Heiko Stuebner <heiko.stuebner@...rry.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@...rry.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.
>>>
>>> though here we are in the very limited scope of only the dwcmshc sdhci
>>> controller.
>>
>> I am afraid that does not justify it.  It can still create
>> problems in the future if that very limited scope changes
>> i.e. someone wants to use the ->irq() callback in
>> sdhci-of-dwcmshc.c for something else.
>>
>>>
>>> And at this point, any controller using the generic sdhci_dwcmshc_ops
>>> will always get the CQE irq handler, while _all other_ controllers
>>> will need to define one to handle the CQE irqs.
>>>
>>> So any variant trying to enable CQE needs to define an irq handler, hence
>>> the check simply was meant to not allow CQE enablement without any irq
>>> handler, because that will always cause those unhandled irq issues.
>>>
>>>> If necessary, it would be better to flag which variants support
>>>> CQE in their platform data.
>>>
>>> I guess we can assume that all of the dwcmshc IP variants support
>>> CQE, it's just that the implementation is slightly strange in that
>>> a DT can enable CQE support and this will cause the driver to enable the
>>> CQE interrupt, even if nothing is there to handle it.
>>
>> Isn't the problem that the IP may support CQE but the driver does
>> not for the given variant, but switches it on anyway.
> 
> exactly ... that is the problem I have with that accidential "supports-cqe"
> in that devicetree.
> 
> 
> The sdhci-of-dwmshc has already some
> 	if (pltfm_data == &sdhci_dwcmshc_rk35xx_pdata)
> conditionals in its probe function for variant-specific init, so I guess
> we could also just move the CQE init into those?

Those are going away, after which it can just be a driver data flag:

	https://lore.kernel.org/linux-mmc/2182f65bad83e394a7ffb9259b2c1aa130912961.1718241495.git.unicorn_wang@outlook.com/


> 
> bool supports_cqe = false;
> 
> if (pltfm_data == &sdhci_dwcmshc_pdata)
> 	supports_cqe = true;
> 
> if (pltfm_data == &sdhci_dwcmshc_rk35xx_pdata)
> {
> ....
> 	supports_cqe = true;
> }
> 
> Because in the end I would guess once every variant has check it's CQE
> support, we'll have all of them supporting it anyway, so that can
> handling could than get removed again.
> 

Powered by blists - more mailing lists