Re: [PATCH] mmc: sdhci-of-dwcmshc: don't enable CQE without a suitable irq handler

[Heiko Stübner]

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@xxxxxxxxx>
> > 
> > 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@xxxxxxxxx>
> > ---
> > 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.

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.


For my own case I can also just live with the Rockchip irq handler
patch going in (ideally as a fix) and I just wanted to try to fix this
will definitly fail if irq-handler not present case.


Heiko


> 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@xxxxxxxxx/
> 
> 
> > +			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)
> 
>