From: Heiko Stuebner <[email protected]>
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 <[email protected]>
---
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) {
+ 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)
--
2.39.2
On 31/05/24 00:55, Heiko Stuebner wrote:
> From: Heiko Stuebner <[email protected]>
>
> 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 <[email protected]>
> ---
> 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/[email protected]/
> + 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)
Am Montag, 3. Juni 2024, 10:01:23 CEST schrieb Adrian Hunter:
> On 31/05/24 00:55, Heiko Stuebner wrote:
> > From: Heiko Stuebner <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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/[email protected]/
>
>
> > + 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)
>
>
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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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.
>
>
> 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/[email protected]/
>>
>>
>>> + 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)
>>
>>
>
>
>
>
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 <[email protected]>
> >>>
> >>> 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 <[email protected]>
> >>> ---
> >>> 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?
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.
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 <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> ---
>>>>> 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.
>