diff mbox series

[v5,1/5] mmc: core: Extend mmc_of_parse() to parse CQE bindings

Message ID 1588031768-23677-2-git-send-email-chun-hung.wu@mediatek.com (mailing list archive)
State New, archived
Headers show
Series mmc: mediatek: add mmc cqhci support | expand

Commit Message

Chun-Hung Wu (巫駿宏) April 27, 2020, 11:56 p.m. UTC
Parse CQE bindings "supports-cqe" and "disable-cqe-dcmd"
in mmc_of_parse().

Signed-off-by: Chun-Hung Wu <chun-hung.wu@mediatek.com>
---
 drivers/mmc/core/host.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Veerabhadrarao Badiganti May 6, 2020, 1 p.m. UTC | #1
On 4/28/2020 5:26 AM, Chun-Hung Wu wrote:
> Parse CQE bindings "supports-cqe" and "disable-cqe-dcmd"
> in mmc_of_parse().
>
> Signed-off-by: Chun-Hung Wu <chun-hung.wu@mediatek.com>
> ---
>   drivers/mmc/core/host.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index c876872..47521c6 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -302,6 +302,11 @@ int mmc_of_parse(struct mmc_host *host)
>   		host->caps2 |= MMC_CAP2_NO_SD;
>   	if (device_property_read_bool(dev, "no-mmc"))
>   		host->caps2 |= MMC_CAP2_NO_MMC;
> +	if (device_property_read_bool(dev, "supports-cqe"))
> +		host->caps2 |= MMC_CAP2_CQE;

This change is breaking emmc driver on qcom platforms where this dt 
property is defined.

[    1.543453]  cqhci_deactivate+0xc/0x38
[    1.545627]  sdhci_msm_reset+0x40/0x58
[    1.549447]  sdhci_do_reset+0x48/0x7c
[    1.553180]  __sdhci_read_caps+0x7c/0x214
[    1.556913]  sdhci_setup_host+0x58/0xce8
[    1.560905]  sdhci_msm_probe+0x588/0x8a4
[    1.564900]  platform_drv_probe+0x4c/0xb0

So, we cant have this flag defined before sdhci_setup_host().

I will have to clear this cap and re-enable it in our initialization.

> +	if (!device_property_read_bool(dev, "disable-cqe-dcmd")) {
> +		host->caps2 |= MMC_CAP2_CQE_DCMD;
> +	}
>   
>   	/* Must be after "non-removable" check */
>   	if (device_property_read_u32(dev, "fixed-emmc-driver-type", &drv_type) == 0) {
Ulf Hansson May 6, 2020, 4:36 p.m. UTC | #2
On Wed, 6 May 2020 at 15:01, Veerabhadrarao Badiganti
<vbadigan@codeaurora.org> wrote:
>
>
> On 4/28/2020 5:26 AM, Chun-Hung Wu wrote:
> > Parse CQE bindings "supports-cqe" and "disable-cqe-dcmd"
> > in mmc_of_parse().
> >
> > Signed-off-by: Chun-Hung Wu <chun-hung.wu@mediatek.com>
> > ---
> >   drivers/mmc/core/host.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> > index c876872..47521c6 100644
> > --- a/drivers/mmc/core/host.c
> > +++ b/drivers/mmc/core/host.c
> > @@ -302,6 +302,11 @@ int mmc_of_parse(struct mmc_host *host)
> >               host->caps2 |= MMC_CAP2_NO_SD;
> >       if (device_property_read_bool(dev, "no-mmc"))
> >               host->caps2 |= MMC_CAP2_NO_MMC;
> > +     if (device_property_read_bool(dev, "supports-cqe"))
> > +             host->caps2 |= MMC_CAP2_CQE;
>
> This change is breaking emmc driver on qcom platforms where this dt
> property is defined.
>
> [    1.543453]  cqhci_deactivate+0xc/0x38
> [    1.545627]  sdhci_msm_reset+0x40/0x58
> [    1.549447]  sdhci_do_reset+0x48/0x7c
> [    1.553180]  __sdhci_read_caps+0x7c/0x214
> [    1.556913]  sdhci_setup_host+0x58/0xce8
> [    1.560905]  sdhci_msm_probe+0x588/0x8a4
> [    1.564900]  platform_drv_probe+0x4c/0xb0
>
> So, we cant have this flag defined before sdhci_setup_host().
>
> I will have to clear this cap and re-enable it in our initialization.

Thanks for reporting! I have dropped all the four patches from
Chun-Hung, so we can figure out how to fix this.

Please help to review the next version of the series.

>
> > +     if (!device_property_read_bool(dev, "disable-cqe-dcmd")) {
> > +             host->caps2 |= MMC_CAP2_CQE_DCMD;
> > +     }
> >
> >       /* Must be after "non-removable" check */
> >       if (device_property_read_u32(dev, "fixed-emmc-driver-type", &drv_type) == 0) {

Kind regards
Uffe
Veerabhadrarao Badiganti May 7, 2020, 4:33 p.m. UTC | #3
On 5/6/2020 10:06 PM, Ulf Hansson wrote:
> On Wed, 6 May 2020 at 15:01, Veerabhadrarao Badiganti
> <vbadigan@codeaurora.org> wrote:
>>
>> On 4/28/2020 5:26 AM, Chun-Hung Wu wrote:
>>> Parse CQE bindings "supports-cqe" and "disable-cqe-dcmd"
>>> in mmc_of_parse().
>>>
>>> Signed-off-by: Chun-Hung Wu <chun-hung.wu@mediatek.com>
>>> ---
>>>    drivers/mmc/core/host.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
>>> index c876872..47521c6 100644
>>> --- a/drivers/mmc/core/host.c
>>> +++ b/drivers/mmc/core/host.c
>>> @@ -302,6 +302,11 @@ int mmc_of_parse(struct mmc_host *host)
>>>                host->caps2 |= MMC_CAP2_NO_SD;
>>>        if (device_property_read_bool(dev, "no-mmc"))
>>>                host->caps2 |= MMC_CAP2_NO_MMC;
>>> +     if (device_property_read_bool(dev, "supports-cqe"))
>>> +             host->caps2 |= MMC_CAP2_CQE;
>> This change is breaking emmc driver on qcom platforms where this dt
>> property is defined.
>>
>> [    1.543453]  cqhci_deactivate+0xc/0x38
>> [    1.545627]  sdhci_msm_reset+0x40/0x58
>> [    1.549447]  sdhci_do_reset+0x48/0x7c
>> [    1.553180]  __sdhci_read_caps+0x7c/0x214
>> [    1.556913]  sdhci_setup_host+0x58/0xce8
>> [    1.560905]  sdhci_msm_probe+0x588/0x8a4
>> [    1.564900]  platform_drv_probe+0x4c/0xb0
>>
>> So, we cant have this flag defined before sdhci_setup_host().
>>
>> I will have to clear this cap and re-enable it in our initialization.
> Thanks for reporting! I have dropped all the four patches from
> Chun-Hung, so we can figure out how to fix this.
>
> Please help to review the next version of the series.

Thanks Ulf.

Hi Chun-Hung,

On qcom controller CQE also gets reset when SDHC is reset. So we have to 
explicitly disable CQE
by invoking  cqhci_deactivate() during sdhc reset

SDHC gets reset in sdhci_setup_host() even before cqe is initialized.
With MMC_CAP2_CQE_DCMD cap set even before sdhci_set_host(), we are 
getting null pointer access with cqhci_deactivate().

If CQE getting reset with SDHC reset is generic (applicable to other 
controllers) then you have revisit your logic.
If its not the case then only qcom driver would get affected.

I see you are updating sdhci-msm.c file as-well. How about including 
below change besides your change?

@@ -1658,6 +1658,8 @@ static int sdhci_msm_cqe_add_host(struct 
sdhci_host *host,
         if (host->caps & SDHCI_CAN_64BIT)
                 host->alloc_desc_sz = 16;

+       /* Clear the CQE cap during setup host */
+       msm_host->mmc->caps2 &= ~MMC_CAP2_CQE;
+
         ret = sdhci_setup_host(host);

>>> +     if (!device_property_read_bool(dev, "disable-cqe-dcmd")) {
>>> +             host->caps2 |= MMC_CAP2_CQE_DCMD;
>>> +     }
>>>
>>>        /* Must be after "non-removable" check */
>>>        if (device_property_read_u32(dev, "fixed-emmc-driver-type", &drv_type) == 0) {
> Kind regards
> Uffe
Ulf Hansson May 8, 2020, 5:05 a.m. UTC | #4
On Thu, 7 May 2020 at 18:33, Veerabhadrarao Badiganti
<vbadigan@codeaurora.org> wrote:
>
>
> On 5/6/2020 10:06 PM, Ulf Hansson wrote:
> > On Wed, 6 May 2020 at 15:01, Veerabhadrarao Badiganti
> > <vbadigan@codeaurora.org> wrote:
> >>
> >> On 4/28/2020 5:26 AM, Chun-Hung Wu wrote:
> >>> Parse CQE bindings "supports-cqe" and "disable-cqe-dcmd"
> >>> in mmc_of_parse().
> >>>
> >>> Signed-off-by: Chun-Hung Wu <chun-hung.wu@mediatek.com>
> >>> ---
> >>>    drivers/mmc/core/host.c | 5 +++++
> >>>    1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> >>> index c876872..47521c6 100644
> >>> --- a/drivers/mmc/core/host.c
> >>> +++ b/drivers/mmc/core/host.c
> >>> @@ -302,6 +302,11 @@ int mmc_of_parse(struct mmc_host *host)
> >>>                host->caps2 |= MMC_CAP2_NO_SD;
> >>>        if (device_property_read_bool(dev, "no-mmc"))
> >>>                host->caps2 |= MMC_CAP2_NO_MMC;
> >>> +     if (device_property_read_bool(dev, "supports-cqe"))
> >>> +             host->caps2 |= MMC_CAP2_CQE;
> >> This change is breaking emmc driver on qcom platforms where this dt
> >> property is defined.
> >>
> >> [    1.543453]  cqhci_deactivate+0xc/0x38
> >> [    1.545627]  sdhci_msm_reset+0x40/0x58
> >> [    1.549447]  sdhci_do_reset+0x48/0x7c
> >> [    1.553180]  __sdhci_read_caps+0x7c/0x214
> >> [    1.556913]  sdhci_setup_host+0x58/0xce8
> >> [    1.560905]  sdhci_msm_probe+0x588/0x8a4
> >> [    1.564900]  platform_drv_probe+0x4c/0xb0
> >>
> >> So, we cant have this flag defined before sdhci_setup_host().
> >>
> >> I will have to clear this cap and re-enable it in our initialization.
> > Thanks for reporting! I have dropped all the four patches from
> > Chun-Hung, so we can figure out how to fix this.
> >
> > Please help to review the next version of the series.
>
> Thanks Ulf.
>
> Hi Chun-Hung,
>
> On qcom controller CQE also gets reset when SDHC is reset. So we have to
> explicitly disable CQE
> by invoking  cqhci_deactivate() during sdhc reset
>
> SDHC gets reset in sdhci_setup_host() even before cqe is initialized.
> With MMC_CAP2_CQE_DCMD cap set even before sdhci_set_host(), we are
> getting null pointer access with cqhci_deactivate().
>
> If CQE getting reset with SDHC reset is generic (applicable to other
> controllers) then you have revisit your logic.
> If its not the case then only qcom driver would get affected.

Thanks for clarifying the problem, much appreciated.

To me, it looks like the DT parsing of the CQE properties are better
suited to be managed by each sdhci variant, to continue to leave some
room for flexibility.

Chun-Hung, can you please drop patch 1 and patch2 from the series and
adapt to this change in the mediatek variant?

[...]

Kind regards
Uffe
Chun-Hung Wu (巫駿宏) May 12, 2020, 1:16 a.m. UTC | #5
On Fri, 2020-05-08 at 13:05 +0800, Ulf Hansson wrote:
> On Thu, 7 May 2020 at 18:33, Veerabhadrarao Badiganti
> <vbadigan@codeaurora.org> wrote:
> >
> >
> > On 5/6/2020 10:06 PM, Ulf Hansson wrote:
> > > On Wed, 6 May 2020 at 15:01, Veerabhadrarao Badiganti
> > > <vbadigan@codeaurora.org> wrote:
> > >>
> > >> On 4/28/2020 5:26 AM, Chun-Hung Wu wrote:
> > >>> Parse CQE bindings "supports-cqe" and "disable-cqe-dcmd"
> > >>> in mmc_of_parse().
> > >>>
> > >>> Signed-off-by: Chun-Hung Wu <chun-hung.wu@mediatek.com>
> > >>> ---
> > >>>    drivers/mmc/core/host.c | 5 +++++
> > >>>    1 file changed, 5 insertions(+)
> > >>>
> > >>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> > >>> index c876872..47521c6 100644
> > >>> --- a/drivers/mmc/core/host.c
> > >>> +++ b/drivers/mmc/core/host.c
> > >>> @@ -302,6 +302,11 @@ int mmc_of_parse(struct mmc_host *host)
> > >>>                host->caps2 |= MMC_CAP2_NO_SD;
> > >>>        if (device_property_read_bool(dev, "no-mmc"))
> > >>>                host->caps2 |= MMC_CAP2_NO_MMC;
> > >>> +     if (device_property_read_bool(dev, "supports-cqe"))
> > >>> +             host->caps2 |= MMC_CAP2_CQE;
> > >> This change is breaking emmc driver on qcom platforms where this dt
> > >> property is defined.
> > >>
> > >> [    1.543453]  cqhci_deactivate+0xc/0x38
> > >> [    1.545627]  sdhci_msm_reset+0x40/0x58
> > >> [    1.549447]  sdhci_do_reset+0x48/0x7c
> > >> [    1.553180]  __sdhci_read_caps+0x7c/0x214
> > >> [    1.556913]  sdhci_setup_host+0x58/0xce8
> > >> [    1.560905]  sdhci_msm_probe+0x588/0x8a4
> > >> [    1.564900]  platform_drv_probe+0x4c/0xb0
> > >>
> > >> So, we cant have this flag defined before sdhci_setup_host().
> > >>
> > >> I will have to clear this cap and re-enable it in our initialization.
> > > Thanks for reporting! I have dropped all the four patches from
> > > Chun-Hung, so we can figure out how to fix this.
> > >
> > > Please help to review the next version of the series.
> >
> > Thanks Ulf.
> >
> > Hi Chun-Hung,
> >
> > On qcom controller CQE also gets reset when SDHC is reset. So we have to
> > explicitly disable CQE
> > by invoking  cqhci_deactivate() during sdhc reset
> >
> > SDHC gets reset in sdhci_setup_host() even before cqe is initialized.
> > With MMC_CAP2_CQE_DCMD cap set even before sdhci_set_host(), we are
> > getting null pointer access with cqhci_deactivate().
> >
> > If CQE getting reset with SDHC reset is generic (applicable to other
> > controllers) then you have revisit your logic.
> > If its not the case then only qcom driver would get affected.
> 
> Thanks for clarifying the problem, much appreciated.
> 
> To me, it looks like the DT parsing of the CQE properties are better
> suited to be managed by each sdhci variant, to continue to leave some
> room for flexibility.
> 
> Chun-Hung, can you please drop patch 1 and patch2 from the series and
> adapt to this change in the mediatek variant?
Ok, I will rollback patch1 and patch2 from series and keep DT parsing
of the CQE properties by each sdhci/msdc variant.
CQE properties will move to mediatek variant.
> 
> [...]
> 
> Kind regards
> Uffe
diff mbox series

Patch

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index c876872..47521c6 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -302,6 +302,11 @@  int mmc_of_parse(struct mmc_host *host)
 		host->caps2 |= MMC_CAP2_NO_SD;
 	if (device_property_read_bool(dev, "no-mmc"))
 		host->caps2 |= MMC_CAP2_NO_MMC;
+	if (device_property_read_bool(dev, "supports-cqe"))
+		host->caps2 |= MMC_CAP2_CQE;
+	if (!device_property_read_bool(dev, "disable-cqe-dcmd")) {
+		host->caps2 |= MMC_CAP2_CQE_DCMD;
+	}
 
 	/* Must be after "non-removable" check */
 	if (device_property_read_u32(dev, "fixed-emmc-driver-type", &drv_type) == 0) {