[PATCH 2/2] mmc: esdhc: add quirk to support 3.3v for T4240

[PATCH 2/2] mmc: esdhc: add quirk to support 3.3v for T4240

[Yangbo Lu]

Add SDHCI_QUIRK2_CIRCUIT_SUPPORT_VS33 for T4240

Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
---
 drivers/mmc/host/sdhci-of-esdhc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index 22e9111..4f5fe42 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -369,6 +369,9 @@ static int sdhci_esdhc_probe(struct platform_device *pdev)
 		host->quirks2 |= SDHCI_QUIRK2_BROKEN_HOST_CONTROL;
 	}
 
+	if (of_device_is_compatible(np, "fsl,t4240-esdhc"))
+		host->quirks2 |= SDHCI_QUIRK2_CIRCUIT_SUPPORT_VS33;
+
 	/* call to generic mmc_of_parse to support additional capabilities */
 	ret = mmc_of_parse(host->mmc);
 	if (ret)
-- 
2.1.0.27.g96db324

Re: [PATCH 2/2] mmc: esdhc: add quirk to support 3.3v for T4240

[Ulf Hansson]

On 2 June 2015 at 09:09, Yangbo Lu <yangbo.lu@freescale.com> wrote:
> Add SDHCI_QUIRK2_CIRCUIT_SUPPORT_VS33 for T4240
>
> Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
> ---
>  drivers/mmc/host/sdhci-of-esdhc.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
> index 22e9111..4f5fe42 100644
> --- a/drivers/mmc/host/sdhci-of-esdhc.c
> +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> @@ -369,6 +369,9 @@ static int sdhci_esdhc_probe(struct platform_device *pdev)
>                 host->quirks2 |= SDHCI_QUIRK2_BROKEN_HOST_CONTROL;
>         }
>
> +       if (of_device_is_compatible(np, "fsl,t4240-esdhc"))
> +               host->quirks2 |= SDHCI_QUIRK2_CIRCUIT_SUPPORT_VS33;
> +

Instead of checking the compatible, could you check the "ocr_avail"
mask which mmc_of_parse() create from the vmmc regulator?

>         /* call to generic mmc_of_parse to support additional capabilities */
>         ret = mmc_of_parse(host->mmc);
>         if (ret)
> --
> 2.1.0.27.g96db324
>

Kind regards
Uffe

Re: [PATCH 2/2] mmc: esdhc: add quirk to support 3.3v for T4240

[Ulf Hansson]

On 4 June 2015 at 11:56, Lu Y.B. <yangbo.lu@freescale.com> wrote:
> Pls see my comments below.
>
>> -----Original Message-----
>> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
>> Sent: Thursday, June 04, 2015 3:51 PM
>> To: Lu Yangbo-B47093
>> Cc: linux-mmc; Chris Ball
>> Subject: Re: [PATCH 2/2] mmc: esdhc: add quirk to support 3.3v for T4240
>>
>> On 2 June 2015 at 09:09, Yangbo Lu <yangbo.lu@freescale.com> wrote:
>> > Add SDHCI_QUIRK2_CIRCUIT_SUPPORT_VS33 for T4240
>> >
>> > Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
>> > ---
>> >  drivers/mmc/host/sdhci-of-esdhc.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/drivers/mmc/host/sdhci-of-esdhc.c
>> b/drivers/mmc/host/sdhci-of-esdhc.c
>> > index 22e9111..4f5fe42 100644
>> > --- a/drivers/mmc/host/sdhci-of-esdhc.c
>> > +++ b/drivers/mmc/host/sdhci-of-esdhc.c
>> > @@ -369,6 +369,9 @@ static int sdhci_esdhc_probe(struct platform_device
>> *pdev)
>> >                 host->quirks2 |= SDHCI_QUIRK2_BROKEN_HOST_CONTROL;
>> >         }
>> >
>> > +       if (of_device_is_compatible(np, "fsl,t4240-esdhc"))
>> > +               host->quirks2 |= SDHCI_QUIRK2_CIRCUIT_SUPPORT_VS33;
>> > +
>>
>> Instead of checking the compatible, could you check the "ocr_avail"
>> mask which mmc_of_parse() create from the vmmc regulator?
>
> It could get ocr_mask value supporting 3.3v from dts, and get ocr_avail not supporting 3.3v from capability register.
> But in sdhci.c, the ocr_avail will "&" the ocr_mask value, this makes 3.3v supporting bit cleaned.
>
> if (host->ocr_mask)
>         ocr_avail &= host->ocr_mask;

I have to admit that this looks a bit odd...

Anyway, as long as you don't specify the "host->ocr_mask" the above
"if" will not change the ocr_avail mask.

Looking a bit further up in the code in sdhci_add_host(), you will
find that the ocr_avail mask is created by calling
mmc_regulator_get_supply() (and not mmc_of_parse() as told you
before). If there are external regulators, the ocr_avail mask will
then override the values from SDHCI's caps register.

Anyway, as this patch seems to intended fixing something related to
the IO voltage, it all looks wrong.

[...]

Kind regards
Uffe

Re: [PATCH 2/2] mmc: esdhc: add quirk to support 3.3v for T4240

[Ulf Hansson]

On 5 June 2015 at 09:10, Lu Y.B. <yangbo.lu@freescale.com> wrote:
> Thanks a lot, see my comments.
>
>> -----Original Message-----
>> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
>> Sent: Thursday, June 04, 2015 7:36 PM
>> To: Lu Yangbo-B47093
>> Cc: linux-mmc; Chris Ball
>> Subject: Re: [PATCH 2/2] mmc: esdhc: add quirk to support 3.3v for T4240
>>
>> On 4 June 2015 at 11:56, Lu Y.B. <yangbo.lu@freescale.com> wrote:
>> > Pls see my comments below.
>> >
>> >> -----Original Message-----
>> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
>> >> Sent: Thursday, June 04, 2015 3:51 PM
>> >> To: Lu Yangbo-B47093
>> >> Cc: linux-mmc; Chris Ball
>> >> Subject: Re: [PATCH 2/2] mmc: esdhc: add quirk to support 3.3v for
>> >> T4240
>> >>
>> >> On 2 June 2015 at 09:09, Yangbo Lu <yangbo.lu@freescale.com> wrote:
>> >> > Add SDHCI_QUIRK2_CIRCUIT_SUPPORT_VS33 for T4240
>> >> >
>> >> > Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
>> >> > ---
>> >> >  drivers/mmc/host/sdhci-of-esdhc.c | 3 +++
>> >> >  1 file changed, 3 insertions(+)
>> >> >
>> >> > diff --git a/drivers/mmc/host/sdhci-of-esdhc.c
>> >> b/drivers/mmc/host/sdhci-of-esdhc.c
>> >> > index 22e9111..4f5fe42 100644
>> >> > --- a/drivers/mmc/host/sdhci-of-esdhc.c
>> >> > +++ b/drivers/mmc/host/sdhci-of-esdhc.c
>> >> > @@ -369,6 +369,9 @@ static int sdhci_esdhc_probe(struct
>> >> > platform_device
>> >> *pdev)
>> >> >                 host->quirks2 |= SDHCI_QUIRK2_BROKEN_HOST_CONTROL;
>> >> >         }
>> >> >
>> >> > +       if (of_device_is_compatible(np, "fsl,t4240-esdhc"))
>> >> > +               host->quirks2 |= SDHCI_QUIRK2_CIRCUIT_SUPPORT_VS33;
>> >> > +
>> >>
>> >> Instead of checking the compatible, could you check the "ocr_avail"
>> >> mask which mmc_of_parse() create from the vmmc regulator?
>> >
>> > It could get ocr_mask value supporting 3.3v from dts, and get ocr_avail
>> not supporting 3.3v from capability register.
>> > But in sdhci.c, the ocr_avail will "&" the ocr_mask value, this makes
>> 3.3v supporting bit cleaned.
>> >
>> > if (host->ocr_mask)
>> >         ocr_avail &= host->ocr_mask;
>>
>> I have to admit that this looks a bit odd...
>>
>> Anyway, as long as you don't specify the "host->ocr_mask" the above "if"
>> will not change the ocr_avail mask.
>
> Actually the sdhci-of-esdhc driver would set host->ocr_mask when it probes.
> mmc_of_parse_voltage(np, &host->ocr_mask);

Okay, got it - thanks!

So I tried to understand when the voltage-range binding should be
used, but the DT documentation is quite poor for it.

Anyway, I assumes the binding is there to describe internal HW
characteristics of the what regulator levels the mmc controller can
support. And the regulator levels are for the power to the card, *not*
for the IO voltage.

So, is this according to what you expects as well, especially since
you were aiming to fix something for the IO voltage in patch1?

>
> But the "&" operation will make host->ocr_mask to have no use for other voltage supporting.
> I think using "|" instead or just assigning host->ocr_mask to ocr_avail should be ok.
> If so, there is no need to add this quirk.

This is kind of a policy change for how to treat the configuration
from DT. I have cc:ed Haijun Zhang, which invented the binding to see
if we can get some feeback from him.

The commit is: 6e9e318b304fd7373a0754805a76a02ddbc69a41 ("mmc: core:
parse voltage from device-tree")

>
>>
>> Looking a bit further up in the code in sdhci_add_host(), you will find
>> that the ocr_avail mask is created by calling
>> mmc_regulator_get_supply() (and not mmc_of_parse() as told you before).
>> If there are external regulators, the ocr_avail mask will then override
>> the values from SDHCI's caps register.
>
> But there is no external regulator on boards that using eSDHC...
>

Quoted from your response in patch1:
"Although the controller only supports 1.8v, the hardware circuit has
a level translator for supporting 3.3v".

Again, that seems to concern the IO voltage, but if not - could it be
modelled as regulator and thus used to fetch the ocr mask from?

It's quite common that we use GPIO regulators in the mmc subsystem for
this matter.

Kind regards
Uffe

Re: [PATCH 2/2] mmc: esdhc: add quirk to support 3.3v for T4240

[Ulf Hansson]

[...]

>> But the "&" operation will make host->ocr_mask to have no use for other voltage supporting.
>> I think using "|" instead or just assigning host->ocr_mask to ocr_avail should be ok.
>> If so, there is no need to add this quirk.
>
> This is kind of a policy change for how to treat the configuration
> from DT. I have cc:ed Haijun Zhang, which invented the binding to see
> if we can get some feeback from him.
>
> The commit is: 6e9e318b304fd7373a0754805a76a02ddbc69a41 ("mmc: core:
> parse voltage from device-tree")
>

The email of Haijun Zhang, bounced so I guess we shouldn't expect any
response from him.

Kind regards
Uffe

Re: [PATCH 2/2] mmc: esdhc: add quirk to support 3.3v for T4240

[Ulf Hansson]

[...]

>> > Actually the sdhci-of-esdhc driver would set host->ocr_mask when it
>> probes.
>> > mmc_of_parse_voltage(np, &host->ocr_mask);
>>
>> Okay, got it - thanks!
>>
>> So I tried to understand when the voltage-range binding should be used,
>> but the DT documentation is quite poor for it.
>>
>> Anyway, I assumes the binding is there to describe internal HW
>> characteristics of the what regulator levels the mmc controller can
>> support. And the regulator levels are for the power to the card, *not*
>> for the IO voltage.
>>
>> So, is this according to what you expects as well, especially since you
>> were aiming to fix something for the IO voltage in patch1?
>>
>> >
>> > But the "&" operation will make host->ocr_mask to have no use for other
>> voltage supporting.
>> > I think using "|" instead or just assigning host->ocr_mask to ocr_avail
>> should be ok.
>> > If so, there is no need to add this quirk.
>>
>> This is kind of a policy change for how to treat the configuration from
>> DT. I have cc:ed Haijun Zhang, which invented the binding to see if we
>> can get some feeback from him.
>>
>> The commit is: 6e9e318b304fd7373a0754805a76a02ddbc69a41 ("mmc: core:
>> parse voltage from device-tree")
>
> Haijun Zhang had left his job. And I have worked instead of him recently.
> I have new find about this.
>
> commit c0b887b66c95fe5abaac071b8332b8c21113d84b
> Author: Haijun Zhang <Haijun.Zhang@freescale.com>
> Date:   Mon Aug 26 09:19:23 2013 +0800
>
>     mmc: sdhci: get voltage from sdhc host
>
>     We use host->ocr_mask to hold the voltage get from device-tree
>     node, In case host->ocr_mask was available, we use host->ocr_mask
>     as the final available voltage can be used by MMC/SD/SDIO card.
>
>     Signed-off-by: Haijun Zhang <haijun.zhang@freescale.com>
>     Reviewed-by: Anton Vorontsov <anton@enomsg.org>
>     Signed-off-by: Chris Ball <cjb@laptop.org>
>
> This was change by commit 3a48edc4bd68f841c07c7bc86358d2f02133f247.
> But I didn’t see the reason.

Oh, that's unfortunate. I think it shouldn't have done that.

>
>>
>> >
>> >>
>> >> Looking a bit further up in the code in sdhci_add_host(), you will
>> >> find that the ocr_avail mask is created by calling
>> >> mmc_regulator_get_supply() (and not mmc_of_parse() as told you before).
>> >> If there are external regulators, the ocr_avail mask will then
>> >> override the values from SDHCI's caps register.
>> >
>> > But there is no external regulator on boards that using eSDHC...
>> >
>>
>> Quoted from your response in patch1:
>> "Although the controller only supports 1.8v, the hardware circuit has a
>> level translator for supporting 3.3v".
>>
>> Again, that seems to concern the IO voltage, but if not - could it be
>> modelled as regulator and thus used to fetch the ocr mask from?
>>
>> It's quite common that we use GPIO regulators in the mmc subsystem for
>> this matter.
>>
> I am afraid not.
> Such as, some boards only support 1.8v, but we used adapter card that makes sd work on 3.3v power voltage and 3.3v IO voltage.
> And some boards may be integrated the level translator on the board to make sd card work on 3.3v power and 3.3v IO voltage.
>
>

I believe am starting to understand the problem now. You are likely
suffering from the change in 3a48edc4bd68f841c07c7bc86358d2f02133f247.
I will cook a patch for you that you can test.

Kind regards
Uffe