[PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Dmitry Osipenko]

Several people asked me about the MMC warnings in the KMSG log and
I had to tell to ignore them because these warning are irrelevant to
pre-Tegra210 SoCs. It should be up to a board's device-tree writer to
properly describe all the necessary properties. Secondly, eventually all
device-tree bindings will be converted to YAML, which allows to validate
board DT files, giving a warning about missing properties. Hence let's
remove the noisy warnings to stop the confusion.

Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/mmc/host/sdhci-tegra.c | 28 ++++------------------------
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 3e2c5101291d..83867629013d 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -607,46 +607,26 @@ static void tegra_sdhci_parse_pad_autocal_dt(struct sdhci_host *host)
 	err = device_property_read_u32(host->mmc->parent,
 			"nvidia,pad-autocal-pull-up-offset-3v3-timeout",
 			&autocal->pull_up_3v3_timeout);
-	if (err) {
-		if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
-			(tegra_host->pinctrl_state_3v3_drv == NULL))
-			pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
-				mmc_hostname(host->mmc));
+	if (err)
 		autocal->pull_up_3v3_timeout = 0;
-	}
 
 	err = device_property_read_u32(host->mmc->parent,
 			"nvidia,pad-autocal-pull-down-offset-3v3-timeout",
 			&autocal->pull_down_3v3_timeout);
-	if (err) {
-		if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
-			(tegra_host->pinctrl_state_3v3_drv == NULL))
-			pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
-				mmc_hostname(host->mmc));
+	if (err)
 		autocal->pull_down_3v3_timeout = 0;
-	}
 
 	err = device_property_read_u32(host->mmc->parent,
 			"nvidia,pad-autocal-pull-up-offset-1v8-timeout",
 			&autocal->pull_up_1v8_timeout);
-	if (err) {
-		if (!IS_ERR(tegra_host->pinctrl_state_1v8) &&
-			(tegra_host->pinctrl_state_1v8_drv == NULL))
-			pr_warn("%s: Missing autocal timeout 1v8-pad drvs\n",
-				mmc_hostname(host->mmc));
+	if (err)
 		autocal->pull_up_1v8_timeout = 0;
-	}
 
 	err = device_property_read_u32(host->mmc->parent,
 			"nvidia,pad-autocal-pull-down-offset-1v8-timeout",
 			&autocal->pull_down_1v8_timeout);
-	if (err) {
-		if (!IS_ERR(tegra_host->pinctrl_state_1v8) &&
-			(tegra_host->pinctrl_state_1v8_drv == NULL))
-			pr_warn("%s: Missing autocal timeout 1v8-pad drvs\n",
-				mmc_hostname(host->mmc));
+	if (err)
 		autocal->pull_down_1v8_timeout = 0;
-	}
 
 	err = device_property_read_u32(host->mmc->parent,
 			"nvidia,pad-autocal-pull-up-offset-sdr104",
-- 
2.26.0

[PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Dmitry Osipenko]

Several people asked me about the MMC warnings in the KMSG log and
I had to tell to ignore them because these warning are irrelevant to
pre-Tegra210 SoCs. It should be up to a board's device-tree writer to
properly describe all the necessary properties. Secondly, eventually all
device-tree bindings will be converted to YAML, which allows to validate
board DT files, giving a warning about missing properties. Hence let's
remove the noisy warnings to stop the confusion.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/mmc/host/sdhci-tegra.c | 28 ++++------------------------
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 3e2c5101291d..83867629013d 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -607,46 +607,26 @@ static void tegra_sdhci_parse_pad_autocal_dt(struct sdhci_host *host)
 	err = device_property_read_u32(host->mmc->parent,
 			"nvidia,pad-autocal-pull-up-offset-3v3-timeout",
 			&autocal->pull_up_3v3_timeout);
-	if (err) {
-		if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
-			(tegra_host->pinctrl_state_3v3_drv == NULL))
-			pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
-				mmc_hostname(host->mmc));
+	if (err)
 		autocal->pull_up_3v3_timeout = 0;
-	}
 
 	err = device_property_read_u32(host->mmc->parent,
 			"nvidia,pad-autocal-pull-down-offset-3v3-timeout",
 			&autocal->pull_down_3v3_timeout);
-	if (err) {
-		if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
-			(tegra_host->pinctrl_state_3v3_drv == NULL))
-			pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
-				mmc_hostname(host->mmc));
+	if (err)
 		autocal->pull_down_3v3_timeout = 0;
-	}
 
 	err = device_property_read_u32(host->mmc->parent,
 			"nvidia,pad-autocal-pull-up-offset-1v8-timeout",
 			&autocal->pull_up_1v8_timeout);
-	if (err) {
-		if (!IS_ERR(tegra_host->pinctrl_state_1v8) &&
-			(tegra_host->pinctrl_state_1v8_drv == NULL))
-			pr_warn("%s: Missing autocal timeout 1v8-pad drvs\n",
-				mmc_hostname(host->mmc));
+	if (err)
 		autocal->pull_up_1v8_timeout = 0;
-	}
 
 	err = device_property_read_u32(host->mmc->parent,
 			"nvidia,pad-autocal-pull-down-offset-1v8-timeout",
 			&autocal->pull_down_1v8_timeout);
-	if (err) {
-		if (!IS_ERR(tegra_host->pinctrl_state_1v8) &&
-			(tegra_host->pinctrl_state_1v8_drv == NULL))
-			pr_warn("%s: Missing autocal timeout 1v8-pad drvs\n",
-				mmc_hostname(host->mmc));
+	if (err)
 		autocal->pull_down_1v8_timeout = 0;
-	}
 
 	err = device_property_read_u32(host->mmc->parent,
 			"nvidia,pad-autocal-pull-up-offset-sdr104",
-- 
2.26.0

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Ulf Hansson]

On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> Several people asked me about the MMC warnings in the KMSG log and
> I had to tell to ignore them because these warning are irrelevant to
> pre-Tegra210 SoCs.

Why are the warnings irrelevant?

> It should be up to a board's device-tree writer to
> properly describe all the necessary properties. Secondly, eventually all
> device-tree bindings will be converted to YAML, which allows to validate
> board DT files, giving a warning about missing properties. Hence let's
> remove the noisy warnings to stop the confusion.

Yep, makes sense. However, perhaps we should do this conversion then,
rather than first drop the warnings?

Kind regards
Uffe

>
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/mmc/host/sdhci-tegra.c | 28 ++++------------------------
>  1 file changed, 4 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index 3e2c5101291d..83867629013d 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -607,46 +607,26 @@ static void tegra_sdhci_parse_pad_autocal_dt(struct sdhci_host *host)
>         err = device_property_read_u32(host->mmc->parent,
>                         "nvidia,pad-autocal-pull-up-offset-3v3-timeout",
>                         &autocal->pull_up_3v3_timeout);
> -       if (err) {
> -               if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
> -                       (tegra_host->pinctrl_state_3v3_drv == NULL))
> -                       pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
> -                               mmc_hostname(host->mmc));
> +       if (err)
>                 autocal->pull_up_3v3_timeout = 0;
> -       }
>
>         err = device_property_read_u32(host->mmc->parent,
>                         "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
>                         &autocal->pull_down_3v3_timeout);
> -       if (err) {
> -               if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
> -                       (tegra_host->pinctrl_state_3v3_drv == NULL))
> -                       pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
> -                               mmc_hostname(host->mmc));
> +       if (err)
>                 autocal->pull_down_3v3_timeout = 0;
> -       }
>
>         err = device_property_read_u32(host->mmc->parent,
>                         "nvidia,pad-autocal-pull-up-offset-1v8-timeout",
>                         &autocal->pull_up_1v8_timeout);
> -       if (err) {
> -               if (!IS_ERR(tegra_host->pinctrl_state_1v8) &&
> -                       (tegra_host->pinctrl_state_1v8_drv == NULL))
> -                       pr_warn("%s: Missing autocal timeout 1v8-pad drvs\n",
> -                               mmc_hostname(host->mmc));
> +       if (err)
>                 autocal->pull_up_1v8_timeout = 0;
> -       }
>
>         err = device_property_read_u32(host->mmc->parent,
>                         "nvidia,pad-autocal-pull-down-offset-1v8-timeout",
>                         &autocal->pull_down_1v8_timeout);
> -       if (err) {
> -               if (!IS_ERR(tegra_host->pinctrl_state_1v8) &&
> -                       (tegra_host->pinctrl_state_1v8_drv == NULL))
> -                       pr_warn("%s: Missing autocal timeout 1v8-pad drvs\n",
> -                               mmc_hostname(host->mmc));
> +       if (err)
>                 autocal->pull_down_1v8_timeout = 0;
> -       }
>
>         err = device_property_read_u32(host->mmc->parent,
>                         "nvidia,pad-autocal-pull-up-offset-sdr104",
> --
> 2.26.0
>

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Ulf Hansson]

On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> Several people asked me about the MMC warnings in the KMSG log and
> I had to tell to ignore them because these warning are irrelevant to
> pre-Tegra210 SoCs.

Why are the warnings irrelevant?

> It should be up to a board's device-tree writer to
> properly describe all the necessary properties. Secondly, eventually all
> device-tree bindings will be converted to YAML, which allows to validate
> board DT files, giving a warning about missing properties. Hence let's
> remove the noisy warnings to stop the confusion.

Yep, makes sense. However, perhaps we should do this conversion then,
rather than first drop the warnings?

Kind regards
Uffe

>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/mmc/host/sdhci-tegra.c | 28 ++++------------------------
>  1 file changed, 4 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index 3e2c5101291d..83867629013d 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -607,46 +607,26 @@ static void tegra_sdhci_parse_pad_autocal_dt(struct sdhci_host *host)
>         err = device_property_read_u32(host->mmc->parent,
>                         "nvidia,pad-autocal-pull-up-offset-3v3-timeout",
>                         &autocal->pull_up_3v3_timeout);
> -       if (err) {
> -               if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
> -                       (tegra_host->pinctrl_state_3v3_drv == NULL))
> -                       pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
> -                               mmc_hostname(host->mmc));
> +       if (err)
>                 autocal->pull_up_3v3_timeout = 0;
> -       }
>
>         err = device_property_read_u32(host->mmc->parent,
>                         "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
>                         &autocal->pull_down_3v3_timeout);
> -       if (err) {
> -               if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
> -                       (tegra_host->pinctrl_state_3v3_drv == NULL))
> -                       pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
> -                               mmc_hostname(host->mmc));
> +       if (err)
>                 autocal->pull_down_3v3_timeout = 0;
> -       }
>
>         err = device_property_read_u32(host->mmc->parent,
>                         "nvidia,pad-autocal-pull-up-offset-1v8-timeout",
>                         &autocal->pull_up_1v8_timeout);
> -       if (err) {
> -               if (!IS_ERR(tegra_host->pinctrl_state_1v8) &&
> -                       (tegra_host->pinctrl_state_1v8_drv == NULL))
> -                       pr_warn("%s: Missing autocal timeout 1v8-pad drvs\n",
> -                               mmc_hostname(host->mmc));
> +       if (err)
>                 autocal->pull_up_1v8_timeout = 0;
> -       }
>
>         err = device_property_read_u32(host->mmc->parent,
>                         "nvidia,pad-autocal-pull-down-offset-1v8-timeout",
>                         &autocal->pull_down_1v8_timeout);
> -       if (err) {
> -               if (!IS_ERR(tegra_host->pinctrl_state_1v8) &&
> -                       (tegra_host->pinctrl_state_1v8_drv == NULL))
> -                       pr_warn("%s: Missing autocal timeout 1v8-pad drvs\n",
> -                               mmc_hostname(host->mmc));
> +       if (err)
>                 autocal->pull_down_1v8_timeout = 0;
> -       }
>
>         err = device_property_read_u32(host->mmc->parent,
>                         "nvidia,pad-autocal-pull-up-offset-sdr104",
> --
> 2.26.0
>

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Dmitry Osipenko]

19.05.2020 10:28, Ulf Hansson пишет:
> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> Several people asked me about the MMC warnings in the KMSG log and
>> I had to tell to ignore them because these warning are irrelevant to
>> pre-Tegra210 SoCs.
> 
> Why are the warnings irrelevant?

That's what the DT binding doc says [1].

[1]
https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt

Although, looking at the driver's code and TRM docs, it seems that all
those properties are really irrelevant only to the older Terga20 SoC. So
the binding doc is a bit misleading.

Nevertheless, the binding explicitly says that the properties are
optional, which is correct.

>> It should be up to a board's device-tree writer to
>> properly describe all the necessary properties. Secondly, eventually all
>> device-tree bindings will be converted to YAML, which allows to validate
>> board DT files, giving a warning about missing properties. Hence let's
>> remove the noisy warnings to stop the confusion.
> 
> Yep, makes sense. However, perhaps we should do this conversion then,
> rather than first drop the warnings?

I don't mind to postpone this patch. But again, IIUC, all these
properties are optional, and thus, there is no critical need to verify
them in DT right now, it could be done later on.

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Ulf Hansson]

On Tue, 19 May 2020 at 16:05, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 19.05.2020 10:28, Ulf Hansson пишет:
> > On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com> wrote:
> >>
> >> Several people asked me about the MMC warnings in the KMSG log and
> >> I had to tell to ignore them because these warning are irrelevant to
> >> pre-Tegra210 SoCs.
> >
> > Why are the warnings irrelevant?
>
> That's what the DT binding doc says [1].
>
> [1]
> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
>
> Although, looking at the driver's code and TRM docs, it seems that all
> those properties are really irrelevant only to the older Terga20 SoC. So
> the binding doc is a bit misleading.
>
> Nevertheless, the binding explicitly says that the properties are
> optional, which is correct.
>
> >> It should be up to a board's device-tree writer to
> >> properly describe all the necessary properties. Secondly, eventually all
> >> device-tree bindings will be converted to YAML, which allows to validate
> >> board DT files, giving a warning about missing properties. Hence let's
> >> remove the noisy warnings to stop the confusion.
> >
> > Yep, makes sense. However, perhaps we should do this conversion then,
> > rather than first drop the warnings?
>
> I don't mind to postpone this patch. But again, IIUC, all these
> properties are optional, and thus, there is no critical need to verify
> them in DT right now, it could be done later on.

Ok, fair enough.

Applied for next, thanks!

Kind regards
Uffe

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2563 bytes --]

On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
> 19.05.2020 10:28, Ulf Hansson пишет:
> > On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>
> >> Several people asked me about the MMC warnings in the KMSG log and
> >> I had to tell to ignore them because these warning are irrelevant to
> >> pre-Tegra210 SoCs.
> > 
> > Why are the warnings irrelevant?
> 
> That's what the DT binding doc says [1].
> 
> [1]
> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
> 
> Although, looking at the driver's code and TRM docs, it seems that all
> those properties are really irrelevant only to the older Terga20 SoC. So
> the binding doc is a bit misleading.
> 
> Nevertheless, the binding explicitly says that the properties are
> optional, which is correct.

Optional only means that drivers must not fail if these properties
aren't found, it doesn't mean that the driver can't warn that they
are missing.

Quite possibly the only reason why they were made optional is because
they weren't part of the bindings since the beginning. Anything added
to a binding after the first public release has to be optional by
definition, otherwise DT ABI wouldn't be stable.

I think these warnings were added on purpose, though I also see that
there are only values for these in device tree for Tegra186 and Tegra194
but not Tegra210 where these should also be necessary.

Adding Sowjanya who wrote this code. Perhaps she can clarify why the
warnings were added. If these values /should/ be there on a subset of
Tegra, then I think we should keep them, or add them again, but perhaps
add a better way of identifying when they are necessary and when it is
safe to work without them.

That said, looking at those checks I wonder if they are perhaps just
wrong. Or at the very least they seem redundant. It looks to me like
they can just be:

	if (tegra_host->pinctrl_state_XYZ == NULL) {
		...
	}

That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
also obvious why we're warning about them on platforms where these
properties don't exist in DT.

So I think we either need to add those values where appropriate so that
the warning doesn't show, or we need to narrow down where they are
really needed and add a corresponding condition.

But again, perhaps Sowjanya can help clarify if these really are only
needed on Tegra210 and later or if these also apply to older chips.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2533 bytes --]

On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
> 19.05.2020 10:28, Ulf Hansson пишет:
> > On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com> wrote:
> >>
> >> Several people asked me about the MMC warnings in the KMSG log and
> >> I had to tell to ignore them because these warning are irrelevant to
> >> pre-Tegra210 SoCs.
> > 
> > Why are the warnings irrelevant?
> 
> That's what the DT binding doc says [1].
> 
> [1]
> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
> 
> Although, looking at the driver's code and TRM docs, it seems that all
> those properties are really irrelevant only to the older Terga20 SoC. So
> the binding doc is a bit misleading.
> 
> Nevertheless, the binding explicitly says that the properties are
> optional, which is correct.

Optional only means that drivers must not fail if these properties
aren't found, it doesn't mean that the driver can't warn that they
are missing.

Quite possibly the only reason why they were made optional is because
they weren't part of the bindings since the beginning. Anything added
to a binding after the first public release has to be optional by
definition, otherwise DT ABI wouldn't be stable.

I think these warnings were added on purpose, though I also see that
there are only values for these in device tree for Tegra186 and Tegra194
but not Tegra210 where these should also be necessary.

Adding Sowjanya who wrote this code. Perhaps she can clarify why the
warnings were added. If these values /should/ be there on a subset of
Tegra, then I think we should keep them, or add them again, but perhaps
add a better way of identifying when they are necessary and when it is
safe to work without them.

That said, looking at those checks I wonder if they are perhaps just
wrong. Or at the very least they seem redundant. It looks to me like
they can just be:

	if (tegra_host->pinctrl_state_XYZ == NULL) {
		...
	}

That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
also obvious why we're warning about them on platforms where these
properties don't exist in DT.

So I think we either need to add those values where appropriate so that
the warning doesn't show, or we need to narrow down where they are
really needed and add a corresponding condition.

But again, perhaps Sowjanya can help clarify if these really are only
needed on Tegra210 and later or if these also apply to older chips.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Dmitry Osipenko]

19.05.2020 19:24, Thierry Reding пишет:
> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
>> 19.05.2020 10:28, Ulf Hansson пишет:
>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>
>>>> Several people asked me about the MMC warnings in the KMSG log and
>>>> I had to tell to ignore them because these warning are irrelevant to
>>>> pre-Tegra210 SoCs.
>>>
>>> Why are the warnings irrelevant?
>>
>> That's what the DT binding doc says [1].
>>
>> [1]
>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
>>
>> Although, looking at the driver's code and TRM docs, it seems that all
>> those properties are really irrelevant only to the older Terga20 SoC. So
>> the binding doc is a bit misleading.
>>
>> Nevertheless, the binding explicitly says that the properties are
>> optional, which is correct.
> 
> Optional only means that drivers must not fail if these properties
> aren't found, it doesn't mean that the driver can't warn that they
> are missing.
> 
> Quite possibly the only reason why they were made optional is because
> they weren't part of the bindings since the beginning. Anything added
> to a binding after the first public release has to be optional by
> definition, otherwise DT ABI wouldn't be stable.
> 
> I think these warnings were added on purpose, though I also see that
> there are only values for these in device tree for Tegra186 and Tegra194
> but not Tegra210 where these should also be necessary.
> 
> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
> warnings were added. If these values /should/ be there on a subset of
> Tegra, then I think we should keep them, or add them again, but perhaps
> add a better way of identifying when they are necessary and when it is
> safe to work without them.
> 
> That said, looking at those checks I wonder if they are perhaps just
> wrong. Or at the very least they seem redundant. It looks to me like
> they can just be:
> 
> 	if (tegra_host->pinctrl_state_XYZ == NULL) {
> 		...
> 	}
> 
> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
> also obvious why we're warning about them on platforms where these
> properties don't exist in DT.
> 
> So I think we either need to add those values where appropriate so that
> the warning doesn't show, or we need to narrow down where they are
> really needed and add a corresponding condition.
> 
> But again, perhaps Sowjanya can help clarify if these really are only
> needed on Tegra210 and later or if these also apply to older chips.

Either way will be cleaner to convert the DT binding to YAML rather than
clutter the driver, IMO.

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Sowjanya Komatineni]

On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
> 19.05.2020 19:24, Thierry Reding пишет:
>> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
>>> 19.05.2020 10:28, Ulf Hansson пишет:
>>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>> Several people asked me about the MMC warnings in the KMSG log and
>>>>> I had to tell to ignore them because these warning are irrelevant to
>>>>> pre-Tegra210 SoCs.
>>>> Why are the warnings irrelevant?
>>> That's what the DT binding doc says [1].
>>>
>>> [1]
>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
>>>
>>> Although, looking at the driver's code and TRM docs, it seems that all
>>> those properties are really irrelevant only to the older Terga20 SoC. So
>>> the binding doc is a bit misleading.
>>>
>>> Nevertheless, the binding explicitly says that the properties are
>>> optional, which is correct.
>> Optional only means that drivers must not fail if these properties
>> aren't found, it doesn't mean that the driver can't warn that they
>> are missing.
>>
>> Quite possibly the only reason why they were made optional is because
>> they weren't part of the bindings since the beginning. Anything added
>> to a binding after the first public release has to be optional by
>> definition, otherwise DT ABI wouldn't be stable.
>>
>> I think these warnings were added on purpose, though I also see that
>> there are only values for these in device tree for Tegra186 and Tegra194
>> but not Tegra210 where these should also be necessary.

dt binding doc we have is common for MMC, SD and SDIO of all Tegras. Its 
not mandatory to have both 3v3 and 1v8 in device tree as based on signal 
mode.

As these driver strengths are SoC specific, they are part of Tegra SoC 
specific device tree where same values will be applicable to all Tegra 
SoC specific platforms.

Based on interface usage on platform, we use one or both of them like 
sdcard supports dual voltage and we use both 3V3 and 1V8 but if same 
interface is used for WIFI SD we use 1V8 only.

So made these dt properties as optional.

Other reason they are optional is, Tegra210 and prior has drive strength 
settings part of apb_misc and Tegra186 and later has driver strengths 
part of SDMMC controller. So,

- Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths are 
applicable for Tegra210 and prior.
- dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are for 
T186 onwards for driver strengths

Looks like dt binding doc need fix to clearly document these based on 
SoC or agree with Yaml we can conditionally specify pinctrl or dt 
properties based on SoC dependent.


>> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
>> warnings were added. If these values /should/ be there on a subset of
>> Tegra, then I think we should keep them, or add them again, but perhaps
>> add a better way of identifying when they are necessary and when it is
>> safe to work without them.
>>
>> That said, looking at those checks I wonder if they are perhaps just
>> wrong. Or at the very least they seem redundant. It looks to me like
>> they can just be:
>>
>> 	if (tegra_host->pinctrl_state_XYZ == NULL) {
>> 		...
>> 	}
>>
>> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
>> also obvious why we're warning about them on platforms where these
>> properties don't exist in DT.

As drive strengths are through dt properties for T186 and later and thru 
pinctrl for T210 and prior, driver first checks for dt autocal timeout 
pull-up/down properties and if they are not found, it then checks for 
presence of pinctrl_state_xyx_drv only when valid pinctrl_state_xyz is 
present.

Driver expects either pinctrl or dt properties and shows warning when 
neither of them are present as its mandatory to use fixed driver 
strengths when auto calibration fails.

     err = device_property_read_u32(host->mmc->parent,
             "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
             &autocal->pull_down_3v3_timeout);
     if (err) {
         if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
             (tegra_host->pinctrl_state_3v3_drv == NULL))
             pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
                 mmc_hostname(host->mmc));
         autocal->pull_down_3v3_timeout = 0;
     }

>>
>> So I think we either need to add those values where appropriate so that
>> the warning doesn't show, or we need to narrow down where they are
>> really needed and add a corresponding condition.
>>
>> But again, perhaps Sowjanya can help clarify if these really are only
>> needed on Tegra210 and later or if these also apply to older chips.
> Either way will be cleaner to convert the DT binding to YAML rather than
> clutter the driver, IMO.
>

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Sowjanya Komatineni]

On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
> 19.05.2020 19:24, Thierry Reding пишет:
>> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
>>> 19.05.2020 10:28, Ulf Hansson пишет:
>>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>> Several people asked me about the MMC warnings in the KMSG log and
>>>>> I had to tell to ignore them because these warning are irrelevant to
>>>>> pre-Tegra210 SoCs.
>>>> Why are the warnings irrelevant?
>>> That's what the DT binding doc says [1].
>>>
>>> [1]
>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
>>>
>>> Although, looking at the driver's code and TRM docs, it seems that all
>>> those properties are really irrelevant only to the older Terga20 SoC. So
>>> the binding doc is a bit misleading.
>>>
>>> Nevertheless, the binding explicitly says that the properties are
>>> optional, which is correct.
>> Optional only means that drivers must not fail if these properties
>> aren't found, it doesn't mean that the driver can't warn that they
>> are missing.
>>
>> Quite possibly the only reason why they were made optional is because
>> they weren't part of the bindings since the beginning. Anything added
>> to a binding after the first public release has to be optional by
>> definition, otherwise DT ABI wouldn't be stable.
>>
>> I think these warnings were added on purpose, though I also see that
>> there are only values for these in device tree for Tegra186 and Tegra194
>> but not Tegra210 where these should also be necessary.

dt binding doc we have is common for MMC, SD and SDIO of all Tegras. Its 
not mandatory to have both 3v3 and 1v8 in device tree as based on signal 
mode.

As these driver strengths are SoC specific, they are part of Tegra SoC 
specific device tree where same values will be applicable to all Tegra 
SoC specific platforms.

Based on interface usage on platform, we use one or both of them like 
sdcard supports dual voltage and we use both 3V3 and 1V8 but if same 
interface is used for WIFI SD we use 1V8 only.

So made these dt properties as optional.

Other reason they are optional is, Tegra210 and prior has drive strength 
settings part of apb_misc and Tegra186 and later has driver strengths 
part of SDMMC controller. So,

- Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths are 
applicable for Tegra210 and prior.
- dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are for 
T186 onwards for driver strengths

Looks like dt binding doc need fix to clearly document these based on 
SoC or agree with Yaml we can conditionally specify pinctrl or dt 
properties based on SoC dependent.


>> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
>> warnings were added. If these values /should/ be there on a subset of
>> Tegra, then I think we should keep them, or add them again, but perhaps
>> add a better way of identifying when they are necessary and when it is
>> safe to work without them.
>>
>> That said, looking at those checks I wonder if they are perhaps just
>> wrong. Or at the very least they seem redundant. It looks to me like
>> they can just be:
>>
>> 	if (tegra_host->pinctrl_state_XYZ == NULL) {
>> 		...
>> 	}
>>
>> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
>> also obvious why we're warning about them on platforms where these
>> properties don't exist in DT.

As drive strengths are through dt properties for T186 and later and thru 
pinctrl for T210 and prior, driver first checks for dt autocal timeout 
pull-up/down properties and if they are not found, it then checks for 
presence of pinctrl_state_xyx_drv only when valid pinctrl_state_xyz is 
present.

Driver expects either pinctrl or dt properties and shows warning when 
neither of them are present as its mandatory to use fixed driver 
strengths when auto calibration fails.

     err = device_property_read_u32(host->mmc->parent,
             "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
             &autocal->pull_down_3v3_timeout);
     if (err) {
         if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
             (tegra_host->pinctrl_state_3v3_drv == NULL))
             pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
                 mmc_hostname(host->mmc));
         autocal->pull_down_3v3_timeout = 0;
     }

>>
>> So I think we either need to add those values where appropriate so that
>> the warning doesn't show, or we need to narrow down where they are
>> really needed and add a corresponding condition.
>>
>> But again, perhaps Sowjanya can help clarify if these really are only
>> needed on Tegra210 and later or if these also apply to older chips.
> Either way will be cleaner to convert the DT binding to YAML rather than
> clutter the driver, IMO.
>

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Sowjanya Komatineni]

On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
>
> On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
>> 19.05.2020 19:24, Thierry Reding пишет:
>>> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
>>>> 19.05.2020 10:28, Ulf Hansson пишет:
>>>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 
>>>>> wrote:
>>>>>> Several people asked me about the MMC warnings in the KMSG log and
>>>>>> I had to tell to ignore them because these warning are irrelevant to
>>>>>> pre-Tegra210 SoCs.
>>>>> Why are the warnings irrelevant?
>>>> That's what the DT binding doc says [1].
>>>>
>>>> [1]
>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt 
>>>>
>>>>
>>>> Although, looking at the driver's code and TRM docs, it seems that all
>>>> those properties are really irrelevant only to the older Terga20 
>>>> SoC. So
>>>> the binding doc is a bit misleading.
>>>>
>>>> Nevertheless, the binding explicitly says that the properties are
>>>> optional, which is correct.
>>> Optional only means that drivers must not fail if these properties
>>> aren't found, it doesn't mean that the driver can't warn that they
>>> are missing.
>>>
>>> Quite possibly the only reason why they were made optional is because
>>> they weren't part of the bindings since the beginning. Anything added
>>> to a binding after the first public release has to be optional by
>>> definition, otherwise DT ABI wouldn't be stable.
>>>
>>> I think these warnings were added on purpose, though I also see that
>>> there are only values for these in device tree for Tegra186 and 
>>> Tegra194
>>> but not Tegra210 where these should also be necessary.
>
> dt binding doc we have is common for MMC, SD and SDIO of all Tegras. 
> Its not mandatory to have both 3v3 and 1v8 in device tree as based on 
> signal mode.
>
> As these driver strengths are SoC specific, they are part of Tegra SoC 
> specific device tree where same values will be applicable to all Tegra 
> SoC specific platforms.
>
> Based on interface usage on platform, we use one or both of them like 
> sdcard supports dual voltage and we use both 3V3 and 1V8 but if same 
> interface is used for WIFI SD we use 1V8 only.
>
> So made these dt properties as optional.
>
> Other reason they are optional is, Tegra210 and prior has drive 
> strength settings part of apb_misc and Tegra186 and later has driver 
> strengths part of SDMMC controller. So,
>
> - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths 
> are applicable for Tegra210 and prior.
> - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are 
> for T186 onwards for driver strengths
>
> Looks like dt binding doc need fix to clearly document these based on 
> SoC or agree with Yaml we can conditionally specify pinctrl or dt 
> properties based on SoC dependent.
>
>
>>> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
>>> warnings were added. If these values /should/ be there on a subset of
>>> Tegra, then I think we should keep them, or add them again, but perhaps
>>> add a better way of identifying when they are necessary and when it is
>>> safe to work without them.
>>>
>>> That said, looking at those checks I wonder if they are perhaps just
>>> wrong. Or at the very least they seem redundant. It looks to me like
>>> they can just be:
>>>
>>>     if (tegra_host->pinctrl_state_XYZ == NULL) {
>>>         ...
>>>     }
>>>
>>> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
>>> also obvious why we're warning about them on platforms where these
>>> properties don't exist in DT.
>
> As drive strengths are through dt properties for T186 and later and 
> thru pinctrl for T210 and prior, driver first checks for dt autocal 
> timeout pull-up/down properties and if they are not found, it then 
> checks for presence of pinctrl_state_xyx_drv only when valid 
> pinctrl_state_xyz is present.
>
> Driver expects either pinctrl or dt properties and shows warning when 
> neither of them are present as its mandatory to use fixed driver 
> strengths when auto calibration fails.
>
>     err = device_property_read_u32(host->mmc->parent,
>             "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
>             &autocal->pull_down_3v3_timeout);
>     if (err) {
>         if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
>             (tegra_host->pinctrl_state_3v3_drv == NULL))
>             pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
>                 mmc_hostname(host->mmc));
>         autocal->pull_down_3v3_timeout = 0;
>     }
>
>>>
>>> So I think we either need to add those values where appropriate so that
>>> the warning doesn't show, or we need to narrow down where they are
>>> really needed and add a corresponding condition.
>>>
>>> But again, perhaps Sowjanya can help clarify if these really are only
>>> needed on Tegra210 and later or if these also apply to older chips.
>> Either way will be cleaner to convert the DT binding to YAML rather than
>> clutter the driver, IMO.
>>
>
>
>
Auto calibration is present from Tegra30 onward and looking into change 
where autocalibration was added to sdhci driver somehow it was enabled 
only for T30/T210/T186/T194.

tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration was 
added to driver and I see this dt parse is being done irrespective of 
NVQUIRK_HAS_PADCALIB quirk so even on platforms without auto cal enabled 
in driver, these messages shows up.

This should be fixed in driver to allow 
tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set 
to avoid dt parsing to happen on platforms that don't have auto cal enabled.

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Sowjanya Komatineni]

On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
>
> On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
>> 19.05.2020 19:24, Thierry Reding пишет:
>>> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
>>>> 19.05.2020 10:28, Ulf Hansson пишет:
>>>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com> 
>>>>> wrote:
>>>>>> Several people asked me about the MMC warnings in the KMSG log and
>>>>>> I had to tell to ignore them because these warning are irrelevant to
>>>>>> pre-Tegra210 SoCs.
>>>>> Why are the warnings irrelevant?
>>>> That's what the DT binding doc says [1].
>>>>
>>>> [1]
>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt 
>>>>
>>>>
>>>> Although, looking at the driver's code and TRM docs, it seems that all
>>>> those properties are really irrelevant only to the older Terga20 
>>>> SoC. So
>>>> the binding doc is a bit misleading.
>>>>
>>>> Nevertheless, the binding explicitly says that the properties are
>>>> optional, which is correct.
>>> Optional only means that drivers must not fail if these properties
>>> aren't found, it doesn't mean that the driver can't warn that they
>>> are missing.
>>>
>>> Quite possibly the only reason why they were made optional is because
>>> they weren't part of the bindings since the beginning. Anything added
>>> to a binding after the first public release has to be optional by
>>> definition, otherwise DT ABI wouldn't be stable.
>>>
>>> I think these warnings were added on purpose, though I also see that
>>> there are only values for these in device tree for Tegra186 and 
>>> Tegra194
>>> but not Tegra210 where these should also be necessary.
>
> dt binding doc we have is common for MMC, SD and SDIO of all Tegras. 
> Its not mandatory to have both 3v3 and 1v8 in device tree as based on 
> signal mode.
>
> As these driver strengths are SoC specific, they are part of Tegra SoC 
> specific device tree where same values will be applicable to all Tegra 
> SoC specific platforms.
>
> Based on interface usage on platform, we use one or both of them like 
> sdcard supports dual voltage and we use both 3V3 and 1V8 but if same 
> interface is used for WIFI SD we use 1V8 only.
>
> So made these dt properties as optional.
>
> Other reason they are optional is, Tegra210 and prior has drive 
> strength settings part of apb_misc and Tegra186 and later has driver 
> strengths part of SDMMC controller. So,
>
> - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths 
> are applicable for Tegra210 and prior.
> - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are 
> for T186 onwards for driver strengths
>
> Looks like dt binding doc need fix to clearly document these based on 
> SoC or agree with Yaml we can conditionally specify pinctrl or dt 
> properties based on SoC dependent.
>
>
>>> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
>>> warnings were added. If these values /should/ be there on a subset of
>>> Tegra, then I think we should keep them, or add them again, but perhaps
>>> add a better way of identifying when they are necessary and when it is
>>> safe to work without them.
>>>
>>> That said, looking at those checks I wonder if they are perhaps just
>>> wrong. Or at the very least they seem redundant. It looks to me like
>>> they can just be:
>>>
>>>     if (tegra_host->pinctrl_state_XYZ == NULL) {
>>>         ...
>>>     }
>>>
>>> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
>>> also obvious why we're warning about them on platforms where these
>>> properties don't exist in DT.
>
> As drive strengths are through dt properties for T186 and later and 
> thru pinctrl for T210 and prior, driver first checks for dt autocal 
> timeout pull-up/down properties and if they are not found, it then 
> checks for presence of pinctrl_state_xyx_drv only when valid 
> pinctrl_state_xyz is present.
>
> Driver expects either pinctrl or dt properties and shows warning when 
> neither of them are present as its mandatory to use fixed driver 
> strengths when auto calibration fails.
>
>     err = device_property_read_u32(host->mmc->parent,
>             "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
>             &autocal->pull_down_3v3_timeout);
>     if (err) {
>         if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
>             (tegra_host->pinctrl_state_3v3_drv == NULL))
>             pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
>                 mmc_hostname(host->mmc));
>         autocal->pull_down_3v3_timeout = 0;
>     }
>
>>>
>>> So I think we either need to add those values where appropriate so that
>>> the warning doesn't show, or we need to narrow down where they are
>>> really needed and add a corresponding condition.
>>>
>>> But again, perhaps Sowjanya can help clarify if these really are only
>>> needed on Tegra210 and later or if these also apply to older chips.
>> Either way will be cleaner to convert the DT binding to YAML rather than
>> clutter the driver, IMO.
>>
>
>
>
Auto calibration is present from Tegra30 onward and looking into change 
where autocalibration was added to sdhci driver somehow it was enabled 
only for T30/T210/T186/T194.

tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration was 
added to driver and I see this dt parse is being done irrespective of 
NVQUIRK_HAS_PADCALIB quirk so even on platforms without auto cal enabled 
in driver, these messages shows up.

This should be fixed in driver to allow 
tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set 
to avoid dt parsing to happen on platforms that don't have auto cal enabled.

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Sowjanya Komatineni]

On 5/19/20 11:41 AM, Sowjanya Komatineni wrote:
>
> On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
>>
>> On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
>>> 19.05.2020 19:24, Thierry Reding пишет:
>>>> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
>>>>> 19.05.2020 10:28, Ulf Hansson пишет:
>>>>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com> 
>>>>>> wrote:
>>>>>>> Several people asked me about the MMC warnings in the KMSG log and
>>>>>>> I had to tell to ignore them because these warning are 
>>>>>>> irrelevant to
>>>>>>> pre-Tegra210 SoCs.
>>>>>> Why are the warnings irrelevant?
>>>>> That's what the DT binding doc says [1].
>>>>>
>>>>> [1]
>>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt 
>>>>>
>>>>>
>>>>> Although, looking at the driver's code and TRM docs, it seems that 
>>>>> all
>>>>> those properties are really irrelevant only to the older Terga20 
>>>>> SoC. So
>>>>> the binding doc is a bit misleading.
>>>>>
>>>>> Nevertheless, the binding explicitly says that the properties are
>>>>> optional, which is correct.
>>>> Optional only means that drivers must not fail if these properties
>>>> aren't found, it doesn't mean that the driver can't warn that they
>>>> are missing.
>>>>
>>>> Quite possibly the only reason why they were made optional is because
>>>> they weren't part of the bindings since the beginning. Anything added
>>>> to a binding after the first public release has to be optional by
>>>> definition, otherwise DT ABI wouldn't be stable.
>>>>
>>>> I think these warnings were added on purpose, though I also see that
>>>> there are only values for these in device tree for Tegra186 and 
>>>> Tegra194
>>>> but not Tegra210 where these should also be necessary.
>>
>> dt binding doc we have is common for MMC, SD and SDIO of all Tegras. 
>> Its not mandatory to have both 3v3 and 1v8 in device tree as based on 
>> signal mode.
>>
>> As these driver strengths are SoC specific, they are part of Tegra 
>> SoC specific device tree where same values will be applicable to all 
>> Tegra SoC specific platforms.
>>
>> Based on interface usage on platform, we use one or both of them like 
>> sdcard supports dual voltage and we use both 3V3 and 1V8 but if same 
>> interface is used for WIFI SD we use 1V8 only.
>>
>> So made these dt properties as optional.
>>
>> Other reason they are optional is, Tegra210 and prior has drive 
>> strength settings part of apb_misc and Tegra186 and later has driver 
>> strengths part of SDMMC controller. So,
>>
>> - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths 
>> are applicable for Tegra210 and prior.
>> - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are 
>> for T186 onwards for driver strengths
>>
>> Looks like dt binding doc need fix to clearly document these based on 
>> SoC or agree with Yaml we can conditionally specify pinctrl or dt 
>> properties based on SoC dependent.
>>
>>
>>>> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
>>>> warnings were added. If these values /should/ be there on a subset of
>>>> Tegra, then I think we should keep them, or add them again, but 
>>>> perhaps
>>>> add a better way of identifying when they are necessary and when it is
>>>> safe to work without them.
>>>>
>>>> That said, looking at those checks I wonder if they are perhaps just
>>>> wrong. Or at the very least they seem redundant. It looks to me like
>>>> they can just be:
>>>>
>>>>     if (tegra_host->pinctrl_state_XYZ == NULL) {
>>>>         ...
>>>>     }
>>>>
>>>> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
>>>> also obvious why we're warning about them on platforms where these
>>>> properties don't exist in DT.
>>
>> As drive strengths are through dt properties for T186 and later and 
>> thru pinctrl for T210 and prior, driver first checks for dt autocal 
>> timeout pull-up/down properties and if they are not found, it then 
>> checks for presence of pinctrl_state_xyx_drv only when valid 
>> pinctrl_state_xyz is present.
>>
>> Driver expects either pinctrl or dt properties and shows warning when 
>> neither of them are present as its mandatory to use fixed driver 
>> strengths when auto calibration fails.
>>
>>     err = device_property_read_u32(host->mmc->parent,
>>             "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
>>             &autocal->pull_down_3v3_timeout);
>>     if (err) {
>>         if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
>>             (tegra_host->pinctrl_state_3v3_drv == NULL))
>>             pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
>>                 mmc_hostname(host->mmc));
>>         autocal->pull_down_3v3_timeout = 0;
>>     }
>>
>>>>
>>>> So I think we either need to add those values where appropriate so 
>>>> that
>>>> the warning doesn't show, or we need to narrow down where they are
>>>> really needed and add a corresponding condition.
>>>>
>>>> But again, perhaps Sowjanya can help clarify if these really are only
>>>> needed on Tegra210 and later or if these also apply to older chips.
>>> Either way will be cleaner to convert the DT binding to YAML rather 
>>> than
>>> clutter the driver, IMO.
>>>
>>
>>
>>
> Auto calibration is present from Tegra30 onward and looking into 
> change where autocalibration was added to sdhci driver somehow it was 
> enabled only for T30/T210/T186/T194.
>
> tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration was 
> added to driver and I see this dt parse is being done irrespective of 
> NVQUIRK_HAS_PADCALIB quirk so even on platforms without auto cal 
> enabled in driver, these messages shows up.
>
> This should be fixed in driver to allow 
> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is 
> set to avoid dt parsing to happen on platforms that don't have auto 
> cal enabled.

Warning on missing drive strengths when auto cal is enabled should be 
present as we should switch to fixed recommended drive strengths when 
auto cal fails.

So probably proper fix should be

- allow tegra_sdhci_parse_pad_autocal_dt() only when 
NVQUIRK_HAS_PADCALIB is set

- current driver sets NVQUIRK_HAS_PADCALIB for T30 as well so need to 
add pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" to Tegra30 device tree.

- Keep warning message of missing auto cal timeouts as its mandatory to 
use fixed recommended driver strengths when auto cal fails.

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Sowjanya Komatineni]

On 5/19/20 12:07 PM, Sowjanya Komatineni wrote:
>
> On 5/19/20 11:41 AM, Sowjanya Komatineni wrote:
>>
>> On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
>>>
>>> On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
>>>> 19.05.2020 19:24, Thierry Reding пишет:
>>>>> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
>>>>>> 19.05.2020 10:28, Ulf Hansson пишет:
>>>>>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 
>>>>>>> wrote:
>>>>>>>> Several people asked me about the MMC warnings in the KMSG log and
>>>>>>>> I had to tell to ignore them because these warning are 
>>>>>>>> irrelevant to
>>>>>>>> pre-Tegra210 SoCs.
>>>>>>> Why are the warnings irrelevant?
>>>>>> That's what the DT binding doc says [1].
>>>>>>
>>>>>> [1]
>>>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt 
>>>>>>
>>>>>>
>>>>>> Although, looking at the driver's code and TRM docs, it seems 
>>>>>> that all
>>>>>> those properties are really irrelevant only to the older Terga20 
>>>>>> SoC. So
>>>>>> the binding doc is a bit misleading.
>>>>>>
>>>>>> Nevertheless, the binding explicitly says that the properties are
>>>>>> optional, which is correct.
>>>>> Optional only means that drivers must not fail if these properties
>>>>> aren't found, it doesn't mean that the driver can't warn that they
>>>>> are missing.
>>>>>
>>>>> Quite possibly the only reason why they were made optional is because
>>>>> they weren't part of the bindings since the beginning. Anything added
>>>>> to a binding after the first public release has to be optional by
>>>>> definition, otherwise DT ABI wouldn't be stable.
>>>>>
>>>>> I think these warnings were added on purpose, though I also see that
>>>>> there are only values for these in device tree for Tegra186 and 
>>>>> Tegra194
>>>>> but not Tegra210 where these should also be necessary.
>>>
>>> dt binding doc we have is common for MMC, SD and SDIO of all Tegras. 
>>> Its not mandatory to have both 3v3 and 1v8 in device tree as based 
>>> on signal mode.
>>>
>>> As these driver strengths are SoC specific, they are part of Tegra 
>>> SoC specific device tree where same values will be applicable to all 
>>> Tegra SoC specific platforms.
>>>
>>> Based on interface usage on platform, we use one or both of them 
>>> like sdcard supports dual voltage and we use both 3V3 and 1V8 but if 
>>> same interface is used for WIFI SD we use 1V8 only.
>>>
>>> So made these dt properties as optional.
>>>
>>> Other reason they are optional is, Tegra210 and prior has drive 
>>> strength settings part of apb_misc and Tegra186 and later has driver 
>>> strengths part of SDMMC controller. So,
>>>
>>> - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths 
>>> are applicable for Tegra210 and prior.
>>> - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are 
>>> for T186 onwards for driver strengths
>>>
>>> Looks like dt binding doc need fix to clearly document these based 
>>> on SoC or agree with Yaml we can conditionally specify pinctrl or dt 
>>> properties based on SoC dependent.
>>>
>>>
>>>>> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
>>>>> warnings were added. If these values /should/ be there on a subset of
>>>>> Tegra, then I think we should keep them, or add them again, but 
>>>>> perhaps
>>>>> add a better way of identifying when they are necessary and when 
>>>>> it is
>>>>> safe to work without them.
>>>>>
>>>>> That said, looking at those checks I wonder if they are perhaps just
>>>>> wrong. Or at the very least they seem redundant. It looks to me like
>>>>> they can just be:
>>>>>
>>>>>     if (tegra_host->pinctrl_state_XYZ == NULL) {
>>>>>         ...
>>>>>     }
>>>>>
>>>>> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
>>>>> also obvious why we're warning about them on platforms where these
>>>>> properties don't exist in DT.
>>>
>>> As drive strengths are through dt properties for T186 and later and 
>>> thru pinctrl for T210 and prior, driver first checks for dt autocal 
>>> timeout pull-up/down properties and if they are not found, it then 
>>> checks for presence of pinctrl_state_xyx_drv only when valid 
>>> pinctrl_state_xyz is present.
>>>
>>> Driver expects either pinctrl or dt properties and shows warning 
>>> when neither of them are present as its mandatory to use fixed 
>>> driver strengths when auto calibration fails.
>>>
>>>     err = device_property_read_u32(host->mmc->parent,
>>>             "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
>>>             &autocal->pull_down_3v3_timeout);
>>>     if (err) {
>>>         if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
>>>             (tegra_host->pinctrl_state_3v3_drv == NULL))
>>>             pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
>>>                 mmc_hostname(host->mmc));
>>>         autocal->pull_down_3v3_timeout = 0;
>>>     }
>>>
>>>>>
>>>>> So I think we either need to add those values where appropriate so 
>>>>> that
>>>>> the warning doesn't show, or we need to narrow down where they are
>>>>> really needed and add a corresponding condition.
>>>>>
>>>>> But again, perhaps Sowjanya can help clarify if these really are only
>>>>> needed on Tegra210 and later or if these also apply to older chips.
>>>> Either way will be cleaner to convert the DT binding to YAML rather 
>>>> than
>>>> clutter the driver, IMO.
>>>>
>>>
>>>
>>>
>> Auto calibration is present from Tegra30 onward and looking into 
>> change where autocalibration was added to sdhci driver somehow it was 
>> enabled only for T30/T210/T186/T194.
>>
>> tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration 
>> was added to driver and I see this dt parse is being done 
>> irrespective of NVQUIRK_HAS_PADCALIB quirk so even on platforms 
>> without auto cal enabled in driver, these messages shows up.
>>
>> This should be fixed in driver to allow 
>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is 
>> set to avoid dt parsing to happen on platforms that don't have auto 
>> cal enabled.
>
> Warning on missing drive strengths when auto cal is enabled should be 
> present as we should switch to fixed recommended drive strengths when 
> auto cal fails.
>
> So probably proper fix should be
>
> - allow tegra_sdhci_parse_pad_autocal_dt() only when 
> NVQUIRK_HAS_PADCALIB is set
>
> - current driver sets NVQUIRK_HAS_PADCALIB for T30 as well so need to 
> add pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" to Tegra30 device tree.
[Correction] T30 has same drive strengths to use irrespective of signal 
voltage and it doesn't have pad control. So for T3- we can update device 
tree to specify "default" pinctrl with drvup/dn settings.
>
> - Keep warning message of missing auto cal timeouts as its mandatory 
> to use fixed recommended driver strengths when auto cal fails.
>
Regarding warnings, I guess simpler and easy fix is to remove warning 
message on missing 3v3/1v8 drive strengths as pinctrl/dt properties were 
already added for T210/186/194 where we need and old device tree don't 
have them but the case where auto cal can fail is very rare.

Otherwise should update driver to allow 
tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set 
and also within tegra_sdhci_parse_pad_autocal_dt() show warning of 
missing 3v3/1v8 settings only when NVQUIRK_NEEDS_PAD_CONTROL is set.

Thierry, please suggest if you prefer to removing warnings or fix driver 
to show warning based on PADCALIB and PAD_CONTROL quirks.

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Sowjanya Komatineni]

On 5/19/20 12:07 PM, Sowjanya Komatineni wrote:
>
> On 5/19/20 11:41 AM, Sowjanya Komatineni wrote:
>>
>> On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
>>>
>>> On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
>>>> 19.05.2020 19:24, Thierry Reding пишет:
>>>>> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
>>>>>> 19.05.2020 10:28, Ulf Hansson пишет:
>>>>>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com> 
>>>>>>> wrote:
>>>>>>>> Several people asked me about the MMC warnings in the KMSG log and
>>>>>>>> I had to tell to ignore them because these warning are 
>>>>>>>> irrelevant to
>>>>>>>> pre-Tegra210 SoCs.
>>>>>>> Why are the warnings irrelevant?
>>>>>> That's what the DT binding doc says [1].
>>>>>>
>>>>>> [1]
>>>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt 
>>>>>>
>>>>>>
>>>>>> Although, looking at the driver's code and TRM docs, it seems 
>>>>>> that all
>>>>>> those properties are really irrelevant only to the older Terga20 
>>>>>> SoC. So
>>>>>> the binding doc is a bit misleading.
>>>>>>
>>>>>> Nevertheless, the binding explicitly says that the properties are
>>>>>> optional, which is correct.
>>>>> Optional only means that drivers must not fail if these properties
>>>>> aren't found, it doesn't mean that the driver can't warn that they
>>>>> are missing.
>>>>>
>>>>> Quite possibly the only reason why they were made optional is because
>>>>> they weren't part of the bindings since the beginning. Anything added
>>>>> to a binding after the first public release has to be optional by
>>>>> definition, otherwise DT ABI wouldn't be stable.
>>>>>
>>>>> I think these warnings were added on purpose, though I also see that
>>>>> there are only values for these in device tree for Tegra186 and 
>>>>> Tegra194
>>>>> but not Tegra210 where these should also be necessary.
>>>
>>> dt binding doc we have is common for MMC, SD and SDIO of all Tegras. 
>>> Its not mandatory to have both 3v3 and 1v8 in device tree as based 
>>> on signal mode.
>>>
>>> As these driver strengths are SoC specific, they are part of Tegra 
>>> SoC specific device tree where same values will be applicable to all 
>>> Tegra SoC specific platforms.
>>>
>>> Based on interface usage on platform, we use one or both of them 
>>> like sdcard supports dual voltage and we use both 3V3 and 1V8 but if 
>>> same interface is used for WIFI SD we use 1V8 only.
>>>
>>> So made these dt properties as optional.
>>>
>>> Other reason they are optional is, Tegra210 and prior has drive 
>>> strength settings part of apb_misc and Tegra186 and later has driver 
>>> strengths part of SDMMC controller. So,
>>>
>>> - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths 
>>> are applicable for Tegra210 and prior.
>>> - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are 
>>> for T186 onwards for driver strengths
>>>
>>> Looks like dt binding doc need fix to clearly document these based 
>>> on SoC or agree with Yaml we can conditionally specify pinctrl or dt 
>>> properties based on SoC dependent.
>>>
>>>
>>>>> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
>>>>> warnings were added. If these values /should/ be there on a subset of
>>>>> Tegra, then I think we should keep them, or add them again, but 
>>>>> perhaps
>>>>> add a better way of identifying when they are necessary and when 
>>>>> it is
>>>>> safe to work without them.
>>>>>
>>>>> That said, looking at those checks I wonder if they are perhaps just
>>>>> wrong. Or at the very least they seem redundant. It looks to me like
>>>>> they can just be:
>>>>>
>>>>>     if (tegra_host->pinctrl_state_XYZ == NULL) {
>>>>>         ...
>>>>>     }
>>>>>
>>>>> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
>>>>> also obvious why we're warning about them on platforms where these
>>>>> properties don't exist in DT.
>>>
>>> As drive strengths are through dt properties for T186 and later and 
>>> thru pinctrl for T210 and prior, driver first checks for dt autocal 
>>> timeout pull-up/down properties and if they are not found, it then 
>>> checks for presence of pinctrl_state_xyx_drv only when valid 
>>> pinctrl_state_xyz is present.
>>>
>>> Driver expects either pinctrl or dt properties and shows warning 
>>> when neither of them are present as its mandatory to use fixed 
>>> driver strengths when auto calibration fails.
>>>
>>>     err = device_property_read_u32(host->mmc->parent,
>>>             "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
>>>             &autocal->pull_down_3v3_timeout);
>>>     if (err) {
>>>         if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
>>>             (tegra_host->pinctrl_state_3v3_drv == NULL))
>>>             pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
>>>                 mmc_hostname(host->mmc));
>>>         autocal->pull_down_3v3_timeout = 0;
>>>     }
>>>
>>>>>
>>>>> So I think we either need to add those values where appropriate so 
>>>>> that
>>>>> the warning doesn't show, or we need to narrow down where they are
>>>>> really needed and add a corresponding condition.
>>>>>
>>>>> But again, perhaps Sowjanya can help clarify if these really are only
>>>>> needed on Tegra210 and later or if these also apply to older chips.
>>>> Either way will be cleaner to convert the DT binding to YAML rather 
>>>> than
>>>> clutter the driver, IMO.
>>>>
>>>
>>>
>>>
>> Auto calibration is present from Tegra30 onward and looking into 
>> change where autocalibration was added to sdhci driver somehow it was 
>> enabled only for T30/T210/T186/T194.
>>
>> tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration 
>> was added to driver and I see this dt parse is being done 
>> irrespective of NVQUIRK_HAS_PADCALIB quirk so even on platforms 
>> without auto cal enabled in driver, these messages shows up.
>>
>> This should be fixed in driver to allow 
>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is 
>> set to avoid dt parsing to happen on platforms that don't have auto 
>> cal enabled.
>
> Warning on missing drive strengths when auto cal is enabled should be 
> present as we should switch to fixed recommended drive strengths when 
> auto cal fails.
>
> So probably proper fix should be
>
> - allow tegra_sdhci_parse_pad_autocal_dt() only when 
> NVQUIRK_HAS_PADCALIB is set
>
> - current driver sets NVQUIRK_HAS_PADCALIB for T30 as well so need to 
> add pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" to Tegra30 device tree.
[Correction] T30 has same drive strengths to use irrespective of signal 
voltage and it doesn't have pad control. So for T3- we can update device 
tree to specify "default" pinctrl with drvup/dn settings.
>
> - Keep warning message of missing auto cal timeouts as its mandatory 
> to use fixed recommended driver strengths when auto cal fails.
>
Regarding warnings, I guess simpler and easy fix is to remove warning 
message on missing 3v3/1v8 drive strengths as pinctrl/dt properties were 
already added for T210/186/194 where we need and old device tree don't 
have them but the case where auto cal can fail is very rare.

Otherwise should update driver to allow 
tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set 
and also within tegra_sdhci_parse_pad_autocal_dt() show warning of 
missing 3v3/1v8 settings only when NVQUIRK_NEEDS_PAD_CONTROL is set.

Thierry, please suggest if you prefer to removing warnings or fix driver 
to show warning based on PADCALIB and PAD_CONTROL quirks.

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Dmitry Osipenko]

19.05.2020 23:44, Sowjanya Komatineni пишет:
> 
> On 5/19/20 12:07 PM, Sowjanya Komatineni wrote:
>>
>> On 5/19/20 11:41 AM, Sowjanya Komatineni wrote:
>>>
>>> On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
>>>>
>>>> On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
>>>>> 19.05.2020 19:24, Thierry Reding пишет:
>>>>>> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
>>>>>>> 19.05.2020 10:28, Ulf Hansson пишет:
>>>>>>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com>
>>>>>>>> wrote:
>>>>>>>>> Several people asked me about the MMC warnings in the KMSG log and
>>>>>>>>> I had to tell to ignore them because these warning are
>>>>>>>>> irrelevant to
>>>>>>>>> pre-Tegra210 SoCs.
>>>>>>>> Why are the warnings irrelevant?
>>>>>>> That's what the DT binding doc says [1].
>>>>>>>
>>>>>>> [1]
>>>>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
>>>>>>>
>>>>>>>
>>>>>>> Although, looking at the driver's code and TRM docs, it seems
>>>>>>> that all
>>>>>>> those properties are really irrelevant only to the older Terga20
>>>>>>> SoC. So
>>>>>>> the binding doc is a bit misleading.
>>>>>>>
>>>>>>> Nevertheless, the binding explicitly says that the properties are
>>>>>>> optional, which is correct.
>>>>>> Optional only means that drivers must not fail if these properties
>>>>>> aren't found, it doesn't mean that the driver can't warn that they
>>>>>> are missing.
>>>>>>
>>>>>> Quite possibly the only reason why they were made optional is because
>>>>>> they weren't part of the bindings since the beginning. Anything added
>>>>>> to a binding after the first public release has to be optional by
>>>>>> definition, otherwise DT ABI wouldn't be stable.
>>>>>>
>>>>>> I think these warnings were added on purpose, though I also see that
>>>>>> there are only values for these in device tree for Tegra186 and
>>>>>> Tegra194
>>>>>> but not Tegra210 where these should also be necessary.
>>>>
>>>> dt binding doc we have is common for MMC, SD and SDIO of all Tegras.
>>>> Its not mandatory to have both 3v3 and 1v8 in device tree as based
>>>> on signal mode.
>>>>
>>>> As these driver strengths are SoC specific, they are part of Tegra
>>>> SoC specific device tree where same values will be applicable to all
>>>> Tegra SoC specific platforms.
>>>>
>>>> Based on interface usage on platform, we use one or both of them
>>>> like sdcard supports dual voltage and we use both 3V3 and 1V8 but if
>>>> same interface is used for WIFI SD we use 1V8 only.
>>>>
>>>> So made these dt properties as optional.
>>>>
>>>> Other reason they are optional is, Tegra210 and prior has drive
>>>> strength settings part of apb_misc and Tegra186 and later has driver
>>>> strengths part of SDMMC controller. So,
>>>>
>>>> - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths
>>>> are applicable for Tegra210 and prior.
>>>> - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are
>>>> for T186 onwards for driver strengths
>>>>
>>>> Looks like dt binding doc need fix to clearly document these based
>>>> on SoC or agree with Yaml we can conditionally specify pinctrl or dt
>>>> properties based on SoC dependent.
>>>>
>>>>
>>>>>> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
>>>>>> warnings were added. If these values /should/ be there on a subset of
>>>>>> Tegra, then I think we should keep them, or add them again, but
>>>>>> perhaps
>>>>>> add a better way of identifying when they are necessary and when
>>>>>> it is
>>>>>> safe to work without them.
>>>>>>
>>>>>> That said, looking at those checks I wonder if they are perhaps just
>>>>>> wrong. Or at the very least they seem redundant. It looks to me like
>>>>>> they can just be:
>>>>>>
>>>>>>     if (tegra_host->pinctrl_state_XYZ == NULL) {
>>>>>>         ...
>>>>>>     }
>>>>>>
>>>>>> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
>>>>>> also obvious why we're warning about them on platforms where these
>>>>>> properties don't exist in DT.
>>>>
>>>> As drive strengths are through dt properties for T186 and later and
>>>> thru pinctrl for T210 and prior, driver first checks for dt autocal
>>>> timeout pull-up/down properties and if they are not found, it then
>>>> checks for presence of pinctrl_state_xyx_drv only when valid
>>>> pinctrl_state_xyz is present.
>>>>
>>>> Driver expects either pinctrl or dt properties and shows warning
>>>> when neither of them are present as its mandatory to use fixed
>>>> driver strengths when auto calibration fails.
>>>>
>>>>     err = device_property_read_u32(host->mmc->parent,
>>>>             "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
>>>>             &autocal->pull_down_3v3_timeout);
>>>>     if (err) {
>>>>         if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
>>>>             (tegra_host->pinctrl_state_3v3_drv == NULL))
>>>>             pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
>>>>                 mmc_hostname(host->mmc));
>>>>         autocal->pull_down_3v3_timeout = 0;
>>>>     }
>>>>
>>>>>>
>>>>>> So I think we either need to add those values where appropriate so
>>>>>> that
>>>>>> the warning doesn't show, or we need to narrow down where they are
>>>>>> really needed and add a corresponding condition.
>>>>>>
>>>>>> But again, perhaps Sowjanya can help clarify if these really are only
>>>>>> needed on Tegra210 and later or if these also apply to older chips.
>>>>> Either way will be cleaner to convert the DT binding to YAML rather
>>>>> than
>>>>> clutter the driver, IMO.
>>>>>
>>>>
>>>>
>>>>
>>> Auto calibration is present from Tegra30 onward and looking into
>>> change where autocalibration was added to sdhci driver somehow it was
>>> enabled only for T30/T210/T186/T194.
>>>
>>> tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration
>>> was added to driver and I see this dt parse is being done
>>> irrespective of NVQUIRK_HAS_PADCALIB quirk so even on platforms
>>> without auto cal enabled in driver, these messages shows up.
>>>
>>> This should be fixed in driver to allow
>>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is
>>> set to avoid dt parsing to happen on platforms that don't have auto
>>> cal enabled.
>>
>> Warning on missing drive strengths when auto cal is enabled should be
>> present as we should switch to fixed recommended drive strengths when
>> auto cal fails.
>>
>> So probably proper fix should be
>>
>> - allow tegra_sdhci_parse_pad_autocal_dt() only when
>> NVQUIRK_HAS_PADCALIB is set
>>
>> - current driver sets NVQUIRK_HAS_PADCALIB for T30 as well so need to
>> add pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" to Tegra30 device tree.
> [Correction] T30 has same drive strengths to use irrespective of signal
> voltage and it doesn't have pad control. So for T3- we can update device
> tree to specify "default" pinctrl with drvup/dn settings.
>>
>> - Keep warning message of missing auto cal timeouts as its mandatory
>> to use fixed recommended driver strengths when auto cal fails.
>>
> Regarding warnings, I guess simpler and easy fix is to remove warning
> message on missing 3v3/1v8 drive strengths as pinctrl/dt properties were
> already added for T210/186/194 where we need and old device tree don't
> have them but the case where auto cal can fail is very rare.
> 
> Otherwise should update driver to allow
> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set
> and also within tegra_sdhci_parse_pad_autocal_dt() show warning of
> missing 3v3/1v8 settings only when NVQUIRK_NEEDS_PAD_CONTROL is set.
> 
> Thierry, please suggest if you prefer to removing warnings or fix driver
> to show warning based on PADCALIB and PAD_CONTROL quirks.

The SDIO PINCTRL drive-strengths are usually a part of the board's
default PINCTRL state, which is either preset by bootloader or by
PINCTRL driver early at a boot time.

The SDIO drive-strengths values should be board-specific and not
SoC-specific because they should depend on the electrical properties of
the board, IIUC.

If the SDIO PINCTRL states are mandatory for the SDHCI nodes in the
device-trees, then the DT binding is wrong since it says that all
properties are optional. But I think that the current binding is okay,
since today SDHCI PINCTRL drive-strengths are specified implicitly in
the device-trees, and thus, there is no real need to emit the noisy
warnings in this case.

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Ulf Hansson]

On Wed, 20 May 2020 at 04:00, Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> 19.05.2020 23:44, Sowjanya Komatineni пишет:
> >
> > On 5/19/20 12:07 PM, Sowjanya Komatineni wrote:
> >>
> >> On 5/19/20 11:41 AM, Sowjanya Komatineni wrote:
> >>>
> >>> On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
> >>>>
> >>>> On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
> >>>>> 19.05.2020 19:24, Thierry Reding пишет:
> >>>>>> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
> >>>>>>> 19.05.2020 10:28, Ulf Hansson пишет:
> >>>>>>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>>>>>>> wrote:
> >>>>>>>>> Several people asked me about the MMC warnings in the KMSG log and
> >>>>>>>>> I had to tell to ignore them because these warning are
> >>>>>>>>> irrelevant to
> >>>>>>>>> pre-Tegra210 SoCs.
> >>>>>>>> Why are the warnings irrelevant?
> >>>>>>> That's what the DT binding doc says [1].
> >>>>>>>
> >>>>>>> [1]
> >>>>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
> >>>>>>>
> >>>>>>>
> >>>>>>> Although, looking at the driver's code and TRM docs, it seems
> >>>>>>> that all
> >>>>>>> those properties are really irrelevant only to the older Terga20
> >>>>>>> SoC. So
> >>>>>>> the binding doc is a bit misleading.
> >>>>>>>
> >>>>>>> Nevertheless, the binding explicitly says that the properties are
> >>>>>>> optional, which is correct.
> >>>>>> Optional only means that drivers must not fail if these properties
> >>>>>> aren't found, it doesn't mean that the driver can't warn that they
> >>>>>> are missing.
> >>>>>>
> >>>>>> Quite possibly the only reason why they were made optional is because
> >>>>>> they weren't part of the bindings since the beginning. Anything added
> >>>>>> to a binding after the first public release has to be optional by
> >>>>>> definition, otherwise DT ABI wouldn't be stable.
> >>>>>>
> >>>>>> I think these warnings were added on purpose, though I also see that
> >>>>>> there are only values for these in device tree for Tegra186 and
> >>>>>> Tegra194
> >>>>>> but not Tegra210 where these should also be necessary.
> >>>>
> >>>> dt binding doc we have is common for MMC, SD and SDIO of all Tegras.
> >>>> Its not mandatory to have both 3v3 and 1v8 in device tree as based
> >>>> on signal mode.
> >>>>
> >>>> As these driver strengths are SoC specific, they are part of Tegra
> >>>> SoC specific device tree where same values will be applicable to all
> >>>> Tegra SoC specific platforms.
> >>>>
> >>>> Based on interface usage on platform, we use one or both of them
> >>>> like sdcard supports dual voltage and we use both 3V3 and 1V8 but if
> >>>> same interface is used for WIFI SD we use 1V8 only.
> >>>>
> >>>> So made these dt properties as optional.
> >>>>
> >>>> Other reason they are optional is, Tegra210 and prior has drive
> >>>> strength settings part of apb_misc and Tegra186 and later has driver
> >>>> strengths part of SDMMC controller. So,
> >>>>
> >>>> - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths
> >>>> are applicable for Tegra210 and prior.
> >>>> - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are
> >>>> for T186 onwards for driver strengths
> >>>>
> >>>> Looks like dt binding doc need fix to clearly document these based
> >>>> on SoC or agree with Yaml we can conditionally specify pinctrl or dt
> >>>> properties based on SoC dependent.
> >>>>
> >>>>
> >>>>>> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
> >>>>>> warnings were added. If these values /should/ be there on a subset of
> >>>>>> Tegra, then I think we should keep them, or add them again, but
> >>>>>> perhaps
> >>>>>> add a better way of identifying when they are necessary and when
> >>>>>> it is
> >>>>>> safe to work without them.
> >>>>>>
> >>>>>> That said, looking at those checks I wonder if they are perhaps just
> >>>>>> wrong. Or at the very least they seem redundant. It looks to me like
> >>>>>> they can just be:
> >>>>>>
> >>>>>>     if (tegra_host->pinctrl_state_XYZ == NULL) {
> >>>>>>         ...
> >>>>>>     }
> >>>>>>
> >>>>>> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
> >>>>>> also obvious why we're warning about them on platforms where these
> >>>>>> properties don't exist in DT.
> >>>>
> >>>> As drive strengths are through dt properties for T186 and later and
> >>>> thru pinctrl for T210 and prior, driver first checks for dt autocal
> >>>> timeout pull-up/down properties and if they are not found, it then
> >>>> checks for presence of pinctrl_state_xyx_drv only when valid
> >>>> pinctrl_state_xyz is present.
> >>>>
> >>>> Driver expects either pinctrl or dt properties and shows warning
> >>>> when neither of them are present as its mandatory to use fixed
> >>>> driver strengths when auto calibration fails.
> >>>>
> >>>>     err = device_property_read_u32(host->mmc->parent,
> >>>>             "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
> >>>>             &autocal->pull_down_3v3_timeout);
> >>>>     if (err) {
> >>>>         if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
> >>>>             (tegra_host->pinctrl_state_3v3_drv == NULL))
> >>>>             pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
> >>>>                 mmc_hostname(host->mmc));
> >>>>         autocal->pull_down_3v3_timeout = 0;
> >>>>     }
> >>>>
> >>>>>>
> >>>>>> So I think we either need to add those values where appropriate so
> >>>>>> that
> >>>>>> the warning doesn't show, or we need to narrow down where they are
> >>>>>> really needed and add a corresponding condition.
> >>>>>>
> >>>>>> But again, perhaps Sowjanya can help clarify if these really are only
> >>>>>> needed on Tegra210 and later or if these also apply to older chips.
> >>>>> Either way will be cleaner to convert the DT binding to YAML rather
> >>>>> than
> >>>>> clutter the driver, IMO.
> >>>>>
> >>>>
> >>>>
> >>>>
> >>> Auto calibration is present from Tegra30 onward and looking into
> >>> change where autocalibration was added to sdhci driver somehow it was
> >>> enabled only for T30/T210/T186/T194.
> >>>
> >>> tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration
> >>> was added to driver and I see this dt parse is being done
> >>> irrespective of NVQUIRK_HAS_PADCALIB quirk so even on platforms
> >>> without auto cal enabled in driver, these messages shows up.
> >>>
> >>> This should be fixed in driver to allow
> >>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is
> >>> set to avoid dt parsing to happen on platforms that don't have auto
> >>> cal enabled.
> >>
> >> Warning on missing drive strengths when auto cal is enabled should be
> >> present as we should switch to fixed recommended drive strengths when
> >> auto cal fails.
> >>
> >> So probably proper fix should be
> >>
> >> - allow tegra_sdhci_parse_pad_autocal_dt() only when
> >> NVQUIRK_HAS_PADCALIB is set
> >>
> >> - current driver sets NVQUIRK_HAS_PADCALIB for T30 as well so need to
> >> add pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" to Tegra30 device tree.
> > [Correction] T30 has same drive strengths to use irrespective of signal
> > voltage and it doesn't have pad control. So for T3- we can update device
> > tree to specify "default" pinctrl with drvup/dn settings.
> >>
> >> - Keep warning message of missing auto cal timeouts as its mandatory
> >> to use fixed recommended driver strengths when auto cal fails.
> >>
> > Regarding warnings, I guess simpler and easy fix is to remove warning
> > message on missing 3v3/1v8 drive strengths as pinctrl/dt properties were
> > already added for T210/186/194 where we need and old device tree don't
> > have them but the case where auto cal can fail is very rare.
> >
> > Otherwise should update driver to allow
> > tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set
> > and also within tegra_sdhci_parse_pad_autocal_dt() show warning of
> > missing 3v3/1v8 settings only when NVQUIRK_NEEDS_PAD_CONTROL is set.
> >
> > Thierry, please suggest if you prefer to removing warnings or fix driver
> > to show warning based on PADCALIB and PAD_CONTROL quirks.
>
> The SDIO PINCTRL drive-strengths are usually a part of the board's
> default PINCTRL state, which is either preset by bootloader or by
> PINCTRL driver early at a boot time.
>
> The SDIO drive-strengths values should be board-specific and not
> SoC-specific because they should depend on the electrical properties of
> the board, IIUC.
>
> If the SDIO PINCTRL states are mandatory for the SDHCI nodes in the
> device-trees, then the DT binding is wrong since it says that all
> properties are optional. But I think that the current binding is okay,
> since today SDHCI PINCTRL drive-strengths are specified implicitly in
> the device-trees, and thus, there is no real need to emit the noisy
> warnings in this case.

For now I will keep $subject patch applied, but please tell me if I
should drop it so we can start over.

In any case, I would appreciate it if someone could have a stab at
converting sdhci and tegra DT bindings to yaml.

Kind regards
Uffe

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Ulf Hansson]

On Wed, 20 May 2020 at 04:00, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 19.05.2020 23:44, Sowjanya Komatineni пишет:
> >
> > On 5/19/20 12:07 PM, Sowjanya Komatineni wrote:
> >>
> >> On 5/19/20 11:41 AM, Sowjanya Komatineni wrote:
> >>>
> >>> On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
> >>>>
> >>>> On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
> >>>>> 19.05.2020 19:24, Thierry Reding пишет:
> >>>>>> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
> >>>>>>> 19.05.2020 10:28, Ulf Hansson пишет:
> >>>>>>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com>
> >>>>>>>> wrote:
> >>>>>>>>> Several people asked me about the MMC warnings in the KMSG log and
> >>>>>>>>> I had to tell to ignore them because these warning are
> >>>>>>>>> irrelevant to
> >>>>>>>>> pre-Tegra210 SoCs.
> >>>>>>>> Why are the warnings irrelevant?
> >>>>>>> That's what the DT binding doc says [1].
> >>>>>>>
> >>>>>>> [1]
> >>>>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
> >>>>>>>
> >>>>>>>
> >>>>>>> Although, looking at the driver's code and TRM docs, it seems
> >>>>>>> that all
> >>>>>>> those properties are really irrelevant only to the older Terga20
> >>>>>>> SoC. So
> >>>>>>> the binding doc is a bit misleading.
> >>>>>>>
> >>>>>>> Nevertheless, the binding explicitly says that the properties are
> >>>>>>> optional, which is correct.
> >>>>>> Optional only means that drivers must not fail if these properties
> >>>>>> aren't found, it doesn't mean that the driver can't warn that they
> >>>>>> are missing.
> >>>>>>
> >>>>>> Quite possibly the only reason why they were made optional is because
> >>>>>> they weren't part of the bindings since the beginning. Anything added
> >>>>>> to a binding after the first public release has to be optional by
> >>>>>> definition, otherwise DT ABI wouldn't be stable.
> >>>>>>
> >>>>>> I think these warnings were added on purpose, though I also see that
> >>>>>> there are only values for these in device tree for Tegra186 and
> >>>>>> Tegra194
> >>>>>> but not Tegra210 where these should also be necessary.
> >>>>
> >>>> dt binding doc we have is common for MMC, SD and SDIO of all Tegras.
> >>>> Its not mandatory to have both 3v3 and 1v8 in device tree as based
> >>>> on signal mode.
> >>>>
> >>>> As these driver strengths are SoC specific, they are part of Tegra
> >>>> SoC specific device tree where same values will be applicable to all
> >>>> Tegra SoC specific platforms.
> >>>>
> >>>> Based on interface usage on platform, we use one or both of them
> >>>> like sdcard supports dual voltage and we use both 3V3 and 1V8 but if
> >>>> same interface is used for WIFI SD we use 1V8 only.
> >>>>
> >>>> So made these dt properties as optional.
> >>>>
> >>>> Other reason they are optional is, Tegra210 and prior has drive
> >>>> strength settings part of apb_misc and Tegra186 and later has driver
> >>>> strengths part of SDMMC controller. So,
> >>>>
> >>>> - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths
> >>>> are applicable for Tegra210 and prior.
> >>>> - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are
> >>>> for T186 onwards for driver strengths
> >>>>
> >>>> Looks like dt binding doc need fix to clearly document these based
> >>>> on SoC or agree with Yaml we can conditionally specify pinctrl or dt
> >>>> properties based on SoC dependent.
> >>>>
> >>>>
> >>>>>> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
> >>>>>> warnings were added. If these values /should/ be there on a subset of
> >>>>>> Tegra, then I think we should keep them, or add them again, but
> >>>>>> perhaps
> >>>>>> add a better way of identifying when they are necessary and when
> >>>>>> it is
> >>>>>> safe to work without them.
> >>>>>>
> >>>>>> That said, looking at those checks I wonder if they are perhaps just
> >>>>>> wrong. Or at the very least they seem redundant. It looks to me like
> >>>>>> they can just be:
> >>>>>>
> >>>>>>     if (tegra_host->pinctrl_state_XYZ == NULL) {
> >>>>>>         ...
> >>>>>>     }
> >>>>>>
> >>>>>> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
> >>>>>> also obvious why we're warning about them on platforms where these
> >>>>>> properties don't exist in DT.
> >>>>
> >>>> As drive strengths are through dt properties for T186 and later and
> >>>> thru pinctrl for T210 and prior, driver first checks for dt autocal
> >>>> timeout pull-up/down properties and if they are not found, it then
> >>>> checks for presence of pinctrl_state_xyx_drv only when valid
> >>>> pinctrl_state_xyz is present.
> >>>>
> >>>> Driver expects either pinctrl or dt properties and shows warning
> >>>> when neither of them are present as its mandatory to use fixed
> >>>> driver strengths when auto calibration fails.
> >>>>
> >>>>     err = device_property_read_u32(host->mmc->parent,
> >>>>             "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
> >>>>             &autocal->pull_down_3v3_timeout);
> >>>>     if (err) {
> >>>>         if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
> >>>>             (tegra_host->pinctrl_state_3v3_drv == NULL))
> >>>>             pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
> >>>>                 mmc_hostname(host->mmc));
> >>>>         autocal->pull_down_3v3_timeout = 0;
> >>>>     }
> >>>>
> >>>>>>
> >>>>>> So I think we either need to add those values where appropriate so
> >>>>>> that
> >>>>>> the warning doesn't show, or we need to narrow down where they are
> >>>>>> really needed and add a corresponding condition.
> >>>>>>
> >>>>>> But again, perhaps Sowjanya can help clarify if these really are only
> >>>>>> needed on Tegra210 and later or if these also apply to older chips.
> >>>>> Either way will be cleaner to convert the DT binding to YAML rather
> >>>>> than
> >>>>> clutter the driver, IMO.
> >>>>>
> >>>>
> >>>>
> >>>>
> >>> Auto calibration is present from Tegra30 onward and looking into
> >>> change where autocalibration was added to sdhci driver somehow it was
> >>> enabled only for T30/T210/T186/T194.
> >>>
> >>> tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration
> >>> was added to driver and I see this dt parse is being done
> >>> irrespective of NVQUIRK_HAS_PADCALIB quirk so even on platforms
> >>> without auto cal enabled in driver, these messages shows up.
> >>>
> >>> This should be fixed in driver to allow
> >>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is
> >>> set to avoid dt parsing to happen on platforms that don't have auto
> >>> cal enabled.
> >>
> >> Warning on missing drive strengths when auto cal is enabled should be
> >> present as we should switch to fixed recommended drive strengths when
> >> auto cal fails.
> >>
> >> So probably proper fix should be
> >>
> >> - allow tegra_sdhci_parse_pad_autocal_dt() only when
> >> NVQUIRK_HAS_PADCALIB is set
> >>
> >> - current driver sets NVQUIRK_HAS_PADCALIB for T30 as well so need to
> >> add pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" to Tegra30 device tree.
> > [Correction] T30 has same drive strengths to use irrespective of signal
> > voltage and it doesn't have pad control. So for T3- we can update device
> > tree to specify "default" pinctrl with drvup/dn settings.
> >>
> >> - Keep warning message of missing auto cal timeouts as its mandatory
> >> to use fixed recommended driver strengths when auto cal fails.
> >>
> > Regarding warnings, I guess simpler and easy fix is to remove warning
> > message on missing 3v3/1v8 drive strengths as pinctrl/dt properties were
> > already added for T210/186/194 where we need and old device tree don't
> > have them but the case where auto cal can fail is very rare.
> >
> > Otherwise should update driver to allow
> > tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set
> > and also within tegra_sdhci_parse_pad_autocal_dt() show warning of
> > missing 3v3/1v8 settings only when NVQUIRK_NEEDS_PAD_CONTROL is set.
> >
> > Thierry, please suggest if you prefer to removing warnings or fix driver
> > to show warning based on PADCALIB and PAD_CONTROL quirks.
>
> The SDIO PINCTRL drive-strengths are usually a part of the board's
> default PINCTRL state, which is either preset by bootloader or by
> PINCTRL driver early at a boot time.
>
> The SDIO drive-strengths values should be board-specific and not
> SoC-specific because they should depend on the electrical properties of
> the board, IIUC.
>
> If the SDIO PINCTRL states are mandatory for the SDHCI nodes in the
> device-trees, then the DT binding is wrong since it says that all
> properties are optional. But I think that the current binding is okay,
> since today SDHCI PINCTRL drive-strengths are specified implicitly in
> the device-trees, and thus, there is no real need to emit the noisy
> warnings in this case.

For now I will keep $subject patch applied, but please tell me if I
should drop it so we can start over.

In any case, I would appreciate it if someone could have a stab at
converting sdhci and tegra DT bindings to yaml.

Kind regards
Uffe

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Sowjanya Komatineni]

On 5/20/20 4:26 AM, Ulf Hansson wrote:
> On Wed, 20 May 2020 at 04:00, Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> 19.05.2020 23:44, Sowjanya Komatineni пишет:
>>> On 5/19/20 12:07 PM, Sowjanya Komatineni wrote:
>>>> On 5/19/20 11:41 AM, Sowjanya Komatineni wrote:
>>>>> On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
>>>>>> On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
>>>>>>> 19.05.2020 19:24, Thierry Reding пишет:
>>>>>>>> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
>>>>>>>>> 19.05.2020 10:28, Ulf Hansson пишет:
>>>>>>>>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>>>>>>> wrote:
>>>>>>>>>>> Several people asked me about the MMC warnings in the KMSG log and
>>>>>>>>>>> I had to tell to ignore them because these warning are
>>>>>>>>>>> irrelevant to
>>>>>>>>>>> pre-Tegra210 SoCs.
>>>>>>>>>> Why are the warnings irrelevant?
>>>>>>>>> That's what the DT binding doc says [1].
>>>>>>>>>
>>>>>>>>> [1]
>>>>>>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Although, looking at the driver's code and TRM docs, it seems
>>>>>>>>> that all
>>>>>>>>> those properties are really irrelevant only to the older Terga20
>>>>>>>>> SoC. So
>>>>>>>>> the binding doc is a bit misleading.
>>>>>>>>>
>>>>>>>>> Nevertheless, the binding explicitly says that the properties are
>>>>>>>>> optional, which is correct.
>>>>>>>> Optional only means that drivers must not fail if these properties
>>>>>>>> aren't found, it doesn't mean that the driver can't warn that they
>>>>>>>> are missing.
>>>>>>>>
>>>>>>>> Quite possibly the only reason why they were made optional is because
>>>>>>>> they weren't part of the bindings since the beginning. Anything added
>>>>>>>> to a binding after the first public release has to be optional by
>>>>>>>> definition, otherwise DT ABI wouldn't be stable.
>>>>>>>>
>>>>>>>> I think these warnings were added on purpose, though I also see that
>>>>>>>> there are only values for these in device tree for Tegra186 and
>>>>>>>> Tegra194
>>>>>>>> but not Tegra210 where these should also be necessary.
>>>>>> dt binding doc we have is common for MMC, SD and SDIO of all Tegras.
>>>>>> Its not mandatory to have both 3v3 and 1v8 in device tree as based
>>>>>> on signal mode.
>>>>>>
>>>>>> As these driver strengths are SoC specific, they are part of Tegra
>>>>>> SoC specific device tree where same values will be applicable to all
>>>>>> Tegra SoC specific platforms.
>>>>>>
>>>>>> Based on interface usage on platform, we use one or both of them
>>>>>> like sdcard supports dual voltage and we use both 3V3 and 1V8 but if
>>>>>> same interface is used for WIFI SD we use 1V8 only.
>>>>>>
>>>>>> So made these dt properties as optional.
>>>>>>
>>>>>> Other reason they are optional is, Tegra210 and prior has drive
>>>>>> strength settings part of apb_misc and Tegra186 and later has driver
>>>>>> strengths part of SDMMC controller. So,
>>>>>>
>>>>>> - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths
>>>>>> are applicable for Tegra210 and prior.
>>>>>> - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are
>>>>>> for T186 onwards for driver strengths
>>>>>>
>>>>>> Looks like dt binding doc need fix to clearly document these based
>>>>>> on SoC or agree with Yaml we can conditionally specify pinctrl or dt
>>>>>> properties based on SoC dependent.
>>>>>>
>>>>>>
>>>>>>>> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
>>>>>>>> warnings were added. If these values /should/ be there on a subset of
>>>>>>>> Tegra, then I think we should keep them, or add them again, but
>>>>>>>> perhaps
>>>>>>>> add a better way of identifying when they are necessary and when
>>>>>>>> it is
>>>>>>>> safe to work without them.
>>>>>>>>
>>>>>>>> That said, looking at those checks I wonder if they are perhaps just
>>>>>>>> wrong. Or at the very least they seem redundant. It looks to me like
>>>>>>>> they can just be:
>>>>>>>>
>>>>>>>>      if (tegra_host->pinctrl_state_XYZ == NULL) {
>>>>>>>>          ...
>>>>>>>>      }
>>>>>>>>
>>>>>>>> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
>>>>>>>> also obvious why we're warning about them on platforms where these
>>>>>>>> properties don't exist in DT.
>>>>>> As drive strengths are through dt properties for T186 and later and
>>>>>> thru pinctrl for T210 and prior, driver first checks for dt autocal
>>>>>> timeout pull-up/down properties and if they are not found, it then
>>>>>> checks for presence of pinctrl_state_xyx_drv only when valid
>>>>>> pinctrl_state_xyz is present.
>>>>>>
>>>>>> Driver expects either pinctrl or dt properties and shows warning
>>>>>> when neither of them are present as its mandatory to use fixed
>>>>>> driver strengths when auto calibration fails.
>>>>>>
>>>>>>      err = device_property_read_u32(host->mmc->parent,
>>>>>>              "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
>>>>>>              &autocal->pull_down_3v3_timeout);
>>>>>>      if (err) {
>>>>>>          if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
>>>>>>              (tegra_host->pinctrl_state_3v3_drv == NULL))
>>>>>>              pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
>>>>>>                  mmc_hostname(host->mmc));
>>>>>>          autocal->pull_down_3v3_timeout = 0;
>>>>>>      }
>>>>>>
>>>>>>>> So I think we either need to add those values where appropriate so
>>>>>>>> that
>>>>>>>> the warning doesn't show, or we need to narrow down where they are
>>>>>>>> really needed and add a corresponding condition.
>>>>>>>>
>>>>>>>> But again, perhaps Sowjanya can help clarify if these really are only
>>>>>>>> needed on Tegra210 and later or if these also apply to older chips.
>>>>>>> Either way will be cleaner to convert the DT binding to YAML rather
>>>>>>> than
>>>>>>> clutter the driver, IMO.
>>>>>>>
>>>>>>
>>>>>>
>>>>> Auto calibration is present from Tegra30 onward and looking into
>>>>> change where autocalibration was added to sdhci driver somehow it was
>>>>> enabled only for T30/T210/T186/T194.
>>>>>
>>>>> tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration
>>>>> was added to driver and I see this dt parse is being done
>>>>> irrespective of NVQUIRK_HAS_PADCALIB quirk so even on platforms
>>>>> without auto cal enabled in driver, these messages shows up.
>>>>>
>>>>> This should be fixed in driver to allow
>>>>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is
>>>>> set to avoid dt parsing to happen on platforms that don't have auto
>>>>> cal enabled.
>>>> Warning on missing drive strengths when auto cal is enabled should be
>>>> present as we should switch to fixed recommended drive strengths when
>>>> auto cal fails.
>>>>
>>>> So probably proper fix should be
>>>>
>>>> - allow tegra_sdhci_parse_pad_autocal_dt() only when
>>>> NVQUIRK_HAS_PADCALIB is set
>>>>
>>>> - current driver sets NVQUIRK_HAS_PADCALIB for T30 as well so need to
>>>> add pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" to Tegra30 device tree.
>>> [Correction] T30 has same drive strengths to use irrespective of signal
>>> voltage and it doesn't have pad control. So for T3- we can update device
>>> tree to specify "default" pinctrl with drvup/dn settings.
>>>> - Keep warning message of missing auto cal timeouts as its mandatory
>>>> to use fixed recommended driver strengths when auto cal fails.
>>>>
>>> Regarding warnings, I guess simpler and easy fix is to remove warning
>>> message on missing 3v3/1v8 drive strengths as pinctrl/dt properties were
>>> already added for T210/186/194 where we need and old device tree don't
>>> have them but the case where auto cal can fail is very rare.
>>>
>>> Otherwise should update driver to allow
>>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set
>>> and also within tegra_sdhci_parse_pad_autocal_dt() show warning of
>>> missing 3v3/1v8 settings only when NVQUIRK_NEEDS_PAD_CONTROL is set.
>>>
>>> Thierry, please suggest if you prefer to removing warnings or fix driver
>>> to show warning based on PADCALIB and PAD_CONTROL quirks.
>> The SDIO PINCTRL drive-strengths are usually a part of the board's
>> default PINCTRL state, which is either preset by bootloader or by
>> PINCTRL driver early at a boot time.
>>
>> The SDIO drive-strengths values should be board-specific and not
>> SoC-specific because they should depend on the electrical properties of
>> the board, IIUC.

Drive strengths we program here when auto calibration fails are 
recommended values based on pre-SI circuit analysis and characterized 
across PVT.

So,  these fail safe values are same for all boards of specific SoC as 
all platform designs follow the design guidelines.

>> If the SDIO PINCTRL states are mandatory for the SDHCI nodes in the
>> device-trees, then the DT binding is wrong since it says that all
>> properties are optional. But I think that the current binding is okay,
>> since today SDHCI PINCTRL drive-strengths are specified implicitly in
>> the device-trees, and thus, there is no real need to emit the noisy
>> warnings in this case.
> For now I will keep $subject patch applied, but please tell me if I
> should drop it so we can start over.
>
> In any case, I would appreciate it if someone could have a stab at
> converting sdhci and tegra DT bindings to yaml.
>
> Kind regards
> Uffe

HI Uffe,

Please drop $subject patch. Will send patch that allows parsing for 
these properties only for SoC where auto cal is enabled as that's where 
driver needs these properties.

So with this fix, warning will not show up on systems where autocal is 
not enabled.

Thanks

Sowjanya

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Sowjanya Komatineni]

On 5/20/20 4:26 AM, Ulf Hansson wrote:
> On Wed, 20 May 2020 at 04:00, Dmitry Osipenko <digetx@gmail.com> wrote:
>> 19.05.2020 23:44, Sowjanya Komatineni пишет:
>>> On 5/19/20 12:07 PM, Sowjanya Komatineni wrote:
>>>> On 5/19/20 11:41 AM, Sowjanya Komatineni wrote:
>>>>> On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
>>>>>> On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
>>>>>>> 19.05.2020 19:24, Thierry Reding пишет:
>>>>>>>> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
>>>>>>>>> 19.05.2020 10:28, Ulf Hansson пишет:
>>>>>>>>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com>
>>>>>>>>>> wrote:
>>>>>>>>>>> Several people asked me about the MMC warnings in the KMSG log and
>>>>>>>>>>> I had to tell to ignore them because these warning are
>>>>>>>>>>> irrelevant to
>>>>>>>>>>> pre-Tegra210 SoCs.
>>>>>>>>>> Why are the warnings irrelevant?
>>>>>>>>> That's what the DT binding doc says [1].
>>>>>>>>>
>>>>>>>>> [1]
>>>>>>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Although, looking at the driver's code and TRM docs, it seems
>>>>>>>>> that all
>>>>>>>>> those properties are really irrelevant only to the older Terga20
>>>>>>>>> SoC. So
>>>>>>>>> the binding doc is a bit misleading.
>>>>>>>>>
>>>>>>>>> Nevertheless, the binding explicitly says that the properties are
>>>>>>>>> optional, which is correct.
>>>>>>>> Optional only means that drivers must not fail if these properties
>>>>>>>> aren't found, it doesn't mean that the driver can't warn that they
>>>>>>>> are missing.
>>>>>>>>
>>>>>>>> Quite possibly the only reason why they were made optional is because
>>>>>>>> they weren't part of the bindings since the beginning. Anything added
>>>>>>>> to a binding after the first public release has to be optional by
>>>>>>>> definition, otherwise DT ABI wouldn't be stable.
>>>>>>>>
>>>>>>>> I think these warnings were added on purpose, though I also see that
>>>>>>>> there are only values for these in device tree for Tegra186 and
>>>>>>>> Tegra194
>>>>>>>> but not Tegra210 where these should also be necessary.
>>>>>> dt binding doc we have is common for MMC, SD and SDIO of all Tegras.
>>>>>> Its not mandatory to have both 3v3 and 1v8 in device tree as based
>>>>>> on signal mode.
>>>>>>
>>>>>> As these driver strengths are SoC specific, they are part of Tegra
>>>>>> SoC specific device tree where same values will be applicable to all
>>>>>> Tegra SoC specific platforms.
>>>>>>
>>>>>> Based on interface usage on platform, we use one or both of them
>>>>>> like sdcard supports dual voltage and we use both 3V3 and 1V8 but if
>>>>>> same interface is used for WIFI SD we use 1V8 only.
>>>>>>
>>>>>> So made these dt properties as optional.
>>>>>>
>>>>>> Other reason they are optional is, Tegra210 and prior has drive
>>>>>> strength settings part of apb_misc and Tegra186 and later has driver
>>>>>> strengths part of SDMMC controller. So,
>>>>>>
>>>>>> - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths
>>>>>> are applicable for Tegra210 and prior.
>>>>>> - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are
>>>>>> for T186 onwards for driver strengths
>>>>>>
>>>>>> Looks like dt binding doc need fix to clearly document these based
>>>>>> on SoC or agree with Yaml we can conditionally specify pinctrl or dt
>>>>>> properties based on SoC dependent.
>>>>>>
>>>>>>
>>>>>>>> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
>>>>>>>> warnings were added. If these values /should/ be there on a subset of
>>>>>>>> Tegra, then I think we should keep them, or add them again, but
>>>>>>>> perhaps
>>>>>>>> add a better way of identifying when they are necessary and when
>>>>>>>> it is
>>>>>>>> safe to work without them.
>>>>>>>>
>>>>>>>> That said, looking at those checks I wonder if they are perhaps just
>>>>>>>> wrong. Or at the very least they seem redundant. It looks to me like
>>>>>>>> they can just be:
>>>>>>>>
>>>>>>>>      if (tegra_host->pinctrl_state_XYZ == NULL) {
>>>>>>>>          ...
>>>>>>>>      }
>>>>>>>>
>>>>>>>> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
>>>>>>>> also obvious why we're warning about them on platforms where these
>>>>>>>> properties don't exist in DT.
>>>>>> As drive strengths are through dt properties for T186 and later and
>>>>>> thru pinctrl for T210 and prior, driver first checks for dt autocal
>>>>>> timeout pull-up/down properties and if they are not found, it then
>>>>>> checks for presence of pinctrl_state_xyx_drv only when valid
>>>>>> pinctrl_state_xyz is present.
>>>>>>
>>>>>> Driver expects either pinctrl or dt properties and shows warning
>>>>>> when neither of them are present as its mandatory to use fixed
>>>>>> driver strengths when auto calibration fails.
>>>>>>
>>>>>>      err = device_property_read_u32(host->mmc->parent,
>>>>>>              "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
>>>>>>              &autocal->pull_down_3v3_timeout);
>>>>>>      if (err) {
>>>>>>          if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
>>>>>>              (tegra_host->pinctrl_state_3v3_drv == NULL))
>>>>>>              pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
>>>>>>                  mmc_hostname(host->mmc));
>>>>>>          autocal->pull_down_3v3_timeout = 0;
>>>>>>      }
>>>>>>
>>>>>>>> So I think we either need to add those values where appropriate so
>>>>>>>> that
>>>>>>>> the warning doesn't show, or we need to narrow down where they are
>>>>>>>> really needed and add a corresponding condition.
>>>>>>>>
>>>>>>>> But again, perhaps Sowjanya can help clarify if these really are only
>>>>>>>> needed on Tegra210 and later or if these also apply to older chips.
>>>>>>> Either way will be cleaner to convert the DT binding to YAML rather
>>>>>>> than
>>>>>>> clutter the driver, IMO.
>>>>>>>
>>>>>>
>>>>>>
>>>>> Auto calibration is present from Tegra30 onward and looking into
>>>>> change where autocalibration was added to sdhci driver somehow it was
>>>>> enabled only for T30/T210/T186/T194.
>>>>>
>>>>> tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration
>>>>> was added to driver and I see this dt parse is being done
>>>>> irrespective of NVQUIRK_HAS_PADCALIB quirk so even on platforms
>>>>> without auto cal enabled in driver, these messages shows up.
>>>>>
>>>>> This should be fixed in driver to allow
>>>>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is
>>>>> set to avoid dt parsing to happen on platforms that don't have auto
>>>>> cal enabled.
>>>> Warning on missing drive strengths when auto cal is enabled should be
>>>> present as we should switch to fixed recommended drive strengths when
>>>> auto cal fails.
>>>>
>>>> So probably proper fix should be
>>>>
>>>> - allow tegra_sdhci_parse_pad_autocal_dt() only when
>>>> NVQUIRK_HAS_PADCALIB is set
>>>>
>>>> - current driver sets NVQUIRK_HAS_PADCALIB for T30 as well so need to
>>>> add pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" to Tegra30 device tree.
>>> [Correction] T30 has same drive strengths to use irrespective of signal
>>> voltage and it doesn't have pad control. So for T3- we can update device
>>> tree to specify "default" pinctrl with drvup/dn settings.
>>>> - Keep warning message of missing auto cal timeouts as its mandatory
>>>> to use fixed recommended driver strengths when auto cal fails.
>>>>
>>> Regarding warnings, I guess simpler and easy fix is to remove warning
>>> message on missing 3v3/1v8 drive strengths as pinctrl/dt properties were
>>> already added for T210/186/194 where we need and old device tree don't
>>> have them but the case where auto cal can fail is very rare.
>>>
>>> Otherwise should update driver to allow
>>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set
>>> and also within tegra_sdhci_parse_pad_autocal_dt() show warning of
>>> missing 3v3/1v8 settings only when NVQUIRK_NEEDS_PAD_CONTROL is set.
>>>
>>> Thierry, please suggest if you prefer to removing warnings or fix driver
>>> to show warning based on PADCALIB and PAD_CONTROL quirks.
>> The SDIO PINCTRL drive-strengths are usually a part of the board's
>> default PINCTRL state, which is either preset by bootloader or by
>> PINCTRL driver early at a boot time.
>>
>> The SDIO drive-strengths values should be board-specific and not
>> SoC-specific because they should depend on the electrical properties of
>> the board, IIUC.

Drive strengths we program here when auto calibration fails are 
recommended values based on pre-SI circuit analysis and characterized 
across PVT.

So,  these fail safe values are same for all boards of specific SoC as 
all platform designs follow the design guidelines.

>> If the SDIO PINCTRL states are mandatory for the SDHCI nodes in the
>> device-trees, then the DT binding is wrong since it says that all
>> properties are optional. But I think that the current binding is okay,
>> since today SDHCI PINCTRL drive-strengths are specified implicitly in
>> the device-trees, and thus, there is no real need to emit the noisy
>> warnings in this case.
> For now I will keep $subject patch applied, but please tell me if I
> should drop it so we can start over.
>
> In any case, I would appreciate it if someone could have a stab at
> converting sdhci and tegra DT bindings to yaml.
>
> Kind regards
> Uffe

HI Uffe,

Please drop $subject patch. Will send patch that allows parsing for 
these properties only for SoC where auto cal is enabled as that's where 
driver needs these properties.

So with this fix, warning will not show up on systems where autocal is 
not enabled.

Thanks

Sowjanya

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 11830 bytes --]

On Wed, May 20, 2020 at 09:09:33AM -0700, Sowjanya Komatineni wrote:
> 
> On 5/20/20 4:26 AM, Ulf Hansson wrote:
> > On Wed, 20 May 2020 at 04:00, Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > 19.05.2020 23:44, Sowjanya Komatineni пишет:
> > > > On 5/19/20 12:07 PM, Sowjanya Komatineni wrote:
> > > > > On 5/19/20 11:41 AM, Sowjanya Komatineni wrote:
> > > > > > On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
> > > > > > > On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
> > > > > > > > 19.05.2020 19:24, Thierry Reding пишет:
> > > > > > > > > On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
> > > > > > > > > > 19.05.2020 10:28, Ulf Hansson пишет:
> > > > > > > > > > > On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com>
> > > > > > > > > > > wrote:
> > > > > > > > > > > > Several people asked me about the MMC warnings in the KMSG log and
> > > > > > > > > > > > I had to tell to ignore them because these warning are
> > > > > > > > > > > > irrelevant to
> > > > > > > > > > > > pre-Tegra210 SoCs.
> > > > > > > > > > > Why are the warnings irrelevant?
> > > > > > > > > > That's what the DT binding doc says [1].
> > > > > > > > > > 
> > > > > > > > > > [1]
> > > > > > > > > > https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > Although, looking at the driver's code and TRM docs, it seems
> > > > > > > > > > that all
> > > > > > > > > > those properties are really irrelevant only to the older Terga20
> > > > > > > > > > SoC. So
> > > > > > > > > > the binding doc is a bit misleading.
> > > > > > > > > > 
> > > > > > > > > > Nevertheless, the binding explicitly says that the properties are
> > > > > > > > > > optional, which is correct.
> > > > > > > > > Optional only means that drivers must not fail if these properties
> > > > > > > > > aren't found, it doesn't mean that the driver can't warn that they
> > > > > > > > > are missing.
> > > > > > > > > 
> > > > > > > > > Quite possibly the only reason why they were made optional is because
> > > > > > > > > they weren't part of the bindings since the beginning. Anything added
> > > > > > > > > to a binding after the first public release has to be optional by
> > > > > > > > > definition, otherwise DT ABI wouldn't be stable.
> > > > > > > > > 
> > > > > > > > > I think these warnings were added on purpose, though I also see that
> > > > > > > > > there are only values for these in device tree for Tegra186 and
> > > > > > > > > Tegra194
> > > > > > > > > but not Tegra210 where these should also be necessary.
> > > > > > > dt binding doc we have is common for MMC, SD and SDIO of all Tegras.
> > > > > > > Its not mandatory to have both 3v3 and 1v8 in device tree as based
> > > > > > > on signal mode.
> > > > > > > 
> > > > > > > As these driver strengths are SoC specific, they are part of Tegra
> > > > > > > SoC specific device tree where same values will be applicable to all
> > > > > > > Tegra SoC specific platforms.
> > > > > > > 
> > > > > > > Based on interface usage on platform, we use one or both of them
> > > > > > > like sdcard supports dual voltage and we use both 3V3 and 1V8 but if
> > > > > > > same interface is used for WIFI SD we use 1V8 only.
> > > > > > > 
> > > > > > > So made these dt properties as optional.
> > > > > > > 
> > > > > > > Other reason they are optional is, Tegra210 and prior has drive
> > > > > > > strength settings part of apb_misc and Tegra186 and later has driver
> > > > > > > strengths part of SDMMC controller. So,
> > > > > > > 
> > > > > > > - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths
> > > > > > > are applicable for Tegra210 and prior.
> > > > > > > - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are
> > > > > > > for T186 onwards for driver strengths
> > > > > > > 
> > > > > > > Looks like dt binding doc need fix to clearly document these based
> > > > > > > on SoC or agree with Yaml we can conditionally specify pinctrl or dt
> > > > > > > properties based on SoC dependent.
> > > > > > > 
> > > > > > > 
> > > > > > > > > Adding Sowjanya who wrote this code. Perhaps she can clarify why the
> > > > > > > > > warnings were added. If these values /should/ be there on a subset of
> > > > > > > > > Tegra, then I think we should keep them, or add them again, but
> > > > > > > > > perhaps
> > > > > > > > > add a better way of identifying when they are necessary and when
> > > > > > > > > it is
> > > > > > > > > safe to work without them.
> > > > > > > > > 
> > > > > > > > > That said, looking at those checks I wonder if they are perhaps just
> > > > > > > > > wrong. Or at the very least they seem redundant. It looks to me like
> > > > > > > > > they can just be:
> > > > > > > > > 
> > > > > > > > >      if (tegra_host->pinctrl_state_XYZ == NULL) {
> > > > > > > > >          ...
> > > > > > > > >      }
> > > > > > > > > 
> > > > > > > > > That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
> > > > > > > > > also obvious why we're warning about them on platforms where these
> > > > > > > > > properties don't exist in DT.
> > > > > > > As drive strengths are through dt properties for T186 and later and
> > > > > > > thru pinctrl for T210 and prior, driver first checks for dt autocal
> > > > > > > timeout pull-up/down properties and if they are not found, it then
> > > > > > > checks for presence of pinctrl_state_xyx_drv only when valid
> > > > > > > pinctrl_state_xyz is present.
> > > > > > > 
> > > > > > > Driver expects either pinctrl or dt properties and shows warning
> > > > > > > when neither of them are present as its mandatory to use fixed
> > > > > > > driver strengths when auto calibration fails.
> > > > > > > 
> > > > > > >      err = device_property_read_u32(host->mmc->parent,
> > > > > > >              "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
> > > > > > >              &autocal->pull_down_3v3_timeout);
> > > > > > >      if (err) {
> > > > > > >          if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
> > > > > > >              (tegra_host->pinctrl_state_3v3_drv == NULL))
> > > > > > >              pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
> > > > > > >                  mmc_hostname(host->mmc));
> > > > > > >          autocal->pull_down_3v3_timeout = 0;
> > > > > > >      }
> > > > > > > 
> > > > > > > > > So I think we either need to add those values where appropriate so
> > > > > > > > > that
> > > > > > > > > the warning doesn't show, or we need to narrow down where they are
> > > > > > > > > really needed and add a corresponding condition.
> > > > > > > > > 
> > > > > > > > > But again, perhaps Sowjanya can help clarify if these really are only
> > > > > > > > > needed on Tegra210 and later or if these also apply to older chips.
> > > > > > > > Either way will be cleaner to convert the DT binding to YAML rather
> > > > > > > > than
> > > > > > > > clutter the driver, IMO.
> > > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > Auto calibration is present from Tegra30 onward and looking into
> > > > > > change where autocalibration was added to sdhci driver somehow it was
> > > > > > enabled only for T30/T210/T186/T194.
> > > > > > 
> > > > > > tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration
> > > > > > was added to driver and I see this dt parse is being done
> > > > > > irrespective of NVQUIRK_HAS_PADCALIB quirk so even on platforms
> > > > > > without auto cal enabled in driver, these messages shows up.
> > > > > > 
> > > > > > This should be fixed in driver to allow
> > > > > > tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is
> > > > > > set to avoid dt parsing to happen on platforms that don't have auto
> > > > > > cal enabled.
> > > > > Warning on missing drive strengths when auto cal is enabled should be
> > > > > present as we should switch to fixed recommended drive strengths when
> > > > > auto cal fails.
> > > > > 
> > > > > So probably proper fix should be
> > > > > 
> > > > > - allow tegra_sdhci_parse_pad_autocal_dt() only when
> > > > > NVQUIRK_HAS_PADCALIB is set
> > > > > 
> > > > > - current driver sets NVQUIRK_HAS_PADCALIB for T30 as well so need to
> > > > > add pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" to Tegra30 device tree.
> > > > [Correction] T30 has same drive strengths to use irrespective of signal
> > > > voltage and it doesn't have pad control. So for T3- we can update device
> > > > tree to specify "default" pinctrl with drvup/dn settings.
> > > > > - Keep warning message of missing auto cal timeouts as its mandatory
> > > > > to use fixed recommended driver strengths when auto cal fails.
> > > > > 
> > > > Regarding warnings, I guess simpler and easy fix is to remove warning
> > > > message on missing 3v3/1v8 drive strengths as pinctrl/dt properties were
> > > > already added for T210/186/194 where we need and old device tree don't
> > > > have them but the case where auto cal can fail is very rare.
> > > > 
> > > > Otherwise should update driver to allow
> > > > tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set
> > > > and also within tegra_sdhci_parse_pad_autocal_dt() show warning of
> > > > missing 3v3/1v8 settings only when NVQUIRK_NEEDS_PAD_CONTROL is set.
> > > > 
> > > > Thierry, please suggest if you prefer to removing warnings or fix driver
> > > > to show warning based on PADCALIB and PAD_CONTROL quirks.
> > > The SDIO PINCTRL drive-strengths are usually a part of the board's
> > > default PINCTRL state, which is either preset by bootloader or by
> > > PINCTRL driver early at a boot time.
> > > 
> > > The SDIO drive-strengths values should be board-specific and not
> > > SoC-specific because they should depend on the electrical properties of
> > > the board, IIUC.
> 
> Drive strengths we program here when auto calibration fails are recommended
> values based on pre-SI circuit analysis and characterized across PVT.
> 
> So,  these fail safe values are same for all boards of specific SoC as all
> platform designs follow the design guidelines.
> 
> > > If the SDIO PINCTRL states are mandatory for the SDHCI nodes in the
> > > device-trees, then the DT binding is wrong since it says that all
> > > properties are optional. But I think that the current binding is okay,
> > > since today SDHCI PINCTRL drive-strengths are specified implicitly in
> > > the device-trees, and thus, there is no real need to emit the noisy
> > > warnings in this case.
> > For now I will keep $subject patch applied, but please tell me if I
> > should drop it so we can start over.
> > 
> > In any case, I would appreciate it if someone could have a stab at
> > converting sdhci and tegra DT bindings to yaml.
> > 
> > Kind regards
> > Uffe
> 
> HI Uffe,
> 
> Please drop $subject patch. Will send patch that allows parsing for these
> properties only for SoC where auto cal is enabled as that's where driver
> needs these properties.
> 
> So with this fix, warning will not show up on systems where autocal is not
> enabled.

Yes, I think that's a better option. Have we ensured that on all systems
where autocal is enabled these values are part of the device tree? Just
making sure that we're not going to have some generation still spit out
these warnings because we forgot to update the device tree.

For example I see that we set NVQUIRK_HAS_PADCALIB but I don't see these
properties being set in arch/arm/boot/dts/tegra30.dtsi. Can you add a
patch that also adds the properties for Tegra30?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 11800 bytes --]

On Wed, May 20, 2020 at 09:09:33AM -0700, Sowjanya Komatineni wrote:
> 
> On 5/20/20 4:26 AM, Ulf Hansson wrote:
> > On Wed, 20 May 2020 at 04:00, Dmitry Osipenko <digetx@gmail.com> wrote:
> > > 19.05.2020 23:44, Sowjanya Komatineni пишет:
> > > > On 5/19/20 12:07 PM, Sowjanya Komatineni wrote:
> > > > > On 5/19/20 11:41 AM, Sowjanya Komatineni wrote:
> > > > > > On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
> > > > > > > On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
> > > > > > > > 19.05.2020 19:24, Thierry Reding пишет:
> > > > > > > > > On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
> > > > > > > > > > 19.05.2020 10:28, Ulf Hansson пишет:
> > > > > > > > > > > On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com>
> > > > > > > > > > > wrote:
> > > > > > > > > > > > Several people asked me about the MMC warnings in the KMSG log and
> > > > > > > > > > > > I had to tell to ignore them because these warning are
> > > > > > > > > > > > irrelevant to
> > > > > > > > > > > > pre-Tegra210 SoCs.
> > > > > > > > > > > Why are the warnings irrelevant?
> > > > > > > > > > That's what the DT binding doc says [1].
> > > > > > > > > > 
> > > > > > > > > > [1]
> > > > > > > > > > https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > Although, looking at the driver's code and TRM docs, it seems
> > > > > > > > > > that all
> > > > > > > > > > those properties are really irrelevant only to the older Terga20
> > > > > > > > > > SoC. So
> > > > > > > > > > the binding doc is a bit misleading.
> > > > > > > > > > 
> > > > > > > > > > Nevertheless, the binding explicitly says that the properties are
> > > > > > > > > > optional, which is correct.
> > > > > > > > > Optional only means that drivers must not fail if these properties
> > > > > > > > > aren't found, it doesn't mean that the driver can't warn that they
> > > > > > > > > are missing.
> > > > > > > > > 
> > > > > > > > > Quite possibly the only reason why they were made optional is because
> > > > > > > > > they weren't part of the bindings since the beginning. Anything added
> > > > > > > > > to a binding after the first public release has to be optional by
> > > > > > > > > definition, otherwise DT ABI wouldn't be stable.
> > > > > > > > > 
> > > > > > > > > I think these warnings were added on purpose, though I also see that
> > > > > > > > > there are only values for these in device tree for Tegra186 and
> > > > > > > > > Tegra194
> > > > > > > > > but not Tegra210 where these should also be necessary.
> > > > > > > dt binding doc we have is common for MMC, SD and SDIO of all Tegras.
> > > > > > > Its not mandatory to have both 3v3 and 1v8 in device tree as based
> > > > > > > on signal mode.
> > > > > > > 
> > > > > > > As these driver strengths are SoC specific, they are part of Tegra
> > > > > > > SoC specific device tree where same values will be applicable to all
> > > > > > > Tegra SoC specific platforms.
> > > > > > > 
> > > > > > > Based on interface usage on platform, we use one or both of them
> > > > > > > like sdcard supports dual voltage and we use both 3V3 and 1V8 but if
> > > > > > > same interface is used for WIFI SD we use 1V8 only.
> > > > > > > 
> > > > > > > So made these dt properties as optional.
> > > > > > > 
> > > > > > > Other reason they are optional is, Tegra210 and prior has drive
> > > > > > > strength settings part of apb_misc and Tegra186 and later has driver
> > > > > > > strengths part of SDMMC controller. So,
> > > > > > > 
> > > > > > > - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths
> > > > > > > are applicable for Tegra210 and prior.
> > > > > > > - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are
> > > > > > > for T186 onwards for driver strengths
> > > > > > > 
> > > > > > > Looks like dt binding doc need fix to clearly document these based
> > > > > > > on SoC or agree with Yaml we can conditionally specify pinctrl or dt
> > > > > > > properties based on SoC dependent.
> > > > > > > 
> > > > > > > 
> > > > > > > > > Adding Sowjanya who wrote this code. Perhaps she can clarify why the
> > > > > > > > > warnings were added. If these values /should/ be there on a subset of
> > > > > > > > > Tegra, then I think we should keep them, or add them again, but
> > > > > > > > > perhaps
> > > > > > > > > add a better way of identifying when they are necessary and when
> > > > > > > > > it is
> > > > > > > > > safe to work without them.
> > > > > > > > > 
> > > > > > > > > That said, looking at those checks I wonder if they are perhaps just
> > > > > > > > > wrong. Or at the very least they seem redundant. It looks to me like
> > > > > > > > > they can just be:
> > > > > > > > > 
> > > > > > > > >      if (tegra_host->pinctrl_state_XYZ == NULL) {
> > > > > > > > >          ...
> > > > > > > > >      }
> > > > > > > > > 
> > > > > > > > > That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
> > > > > > > > > also obvious why we're warning about them on platforms where these
> > > > > > > > > properties don't exist in DT.
> > > > > > > As drive strengths are through dt properties for T186 and later and
> > > > > > > thru pinctrl for T210 and prior, driver first checks for dt autocal
> > > > > > > timeout pull-up/down properties and if they are not found, it then
> > > > > > > checks for presence of pinctrl_state_xyx_drv only when valid
> > > > > > > pinctrl_state_xyz is present.
> > > > > > > 
> > > > > > > Driver expects either pinctrl or dt properties and shows warning
> > > > > > > when neither of them are present as its mandatory to use fixed
> > > > > > > driver strengths when auto calibration fails.
> > > > > > > 
> > > > > > >      err = device_property_read_u32(host->mmc->parent,
> > > > > > >              "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
> > > > > > >              &autocal->pull_down_3v3_timeout);
> > > > > > >      if (err) {
> > > > > > >          if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
> > > > > > >              (tegra_host->pinctrl_state_3v3_drv == NULL))
> > > > > > >              pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
> > > > > > >                  mmc_hostname(host->mmc));
> > > > > > >          autocal->pull_down_3v3_timeout = 0;
> > > > > > >      }
> > > > > > > 
> > > > > > > > > So I think we either need to add those values where appropriate so
> > > > > > > > > that
> > > > > > > > > the warning doesn't show, or we need to narrow down where they are
> > > > > > > > > really needed and add a corresponding condition.
> > > > > > > > > 
> > > > > > > > > But again, perhaps Sowjanya can help clarify if these really are only
> > > > > > > > > needed on Tegra210 and later or if these also apply to older chips.
> > > > > > > > Either way will be cleaner to convert the DT binding to YAML rather
> > > > > > > > than
> > > > > > > > clutter the driver, IMO.
> > > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > Auto calibration is present from Tegra30 onward and looking into
> > > > > > change where autocalibration was added to sdhci driver somehow it was
> > > > > > enabled only for T30/T210/T186/T194.
> > > > > > 
> > > > > > tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration
> > > > > > was added to driver and I see this dt parse is being done
> > > > > > irrespective of NVQUIRK_HAS_PADCALIB quirk so even on platforms
> > > > > > without auto cal enabled in driver, these messages shows up.
> > > > > > 
> > > > > > This should be fixed in driver to allow
> > > > > > tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is
> > > > > > set to avoid dt parsing to happen on platforms that don't have auto
> > > > > > cal enabled.
> > > > > Warning on missing drive strengths when auto cal is enabled should be
> > > > > present as we should switch to fixed recommended drive strengths when
> > > > > auto cal fails.
> > > > > 
> > > > > So probably proper fix should be
> > > > > 
> > > > > - allow tegra_sdhci_parse_pad_autocal_dt() only when
> > > > > NVQUIRK_HAS_PADCALIB is set
> > > > > 
> > > > > - current driver sets NVQUIRK_HAS_PADCALIB for T30 as well so need to
> > > > > add pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" to Tegra30 device tree.
> > > > [Correction] T30 has same drive strengths to use irrespective of signal
> > > > voltage and it doesn't have pad control. So for T3- we can update device
> > > > tree to specify "default" pinctrl with drvup/dn settings.
> > > > > - Keep warning message of missing auto cal timeouts as its mandatory
> > > > > to use fixed recommended driver strengths when auto cal fails.
> > > > > 
> > > > Regarding warnings, I guess simpler and easy fix is to remove warning
> > > > message on missing 3v3/1v8 drive strengths as pinctrl/dt properties were
> > > > already added for T210/186/194 where we need and old device tree don't
> > > > have them but the case where auto cal can fail is very rare.
> > > > 
> > > > Otherwise should update driver to allow
> > > > tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set
> > > > and also within tegra_sdhci_parse_pad_autocal_dt() show warning of
> > > > missing 3v3/1v8 settings only when NVQUIRK_NEEDS_PAD_CONTROL is set.
> > > > 
> > > > Thierry, please suggest if you prefer to removing warnings or fix driver
> > > > to show warning based on PADCALIB and PAD_CONTROL quirks.
> > > The SDIO PINCTRL drive-strengths are usually a part of the board's
> > > default PINCTRL state, which is either preset by bootloader or by
> > > PINCTRL driver early at a boot time.
> > > 
> > > The SDIO drive-strengths values should be board-specific and not
> > > SoC-specific because they should depend on the electrical properties of
> > > the board, IIUC.
> 
> Drive strengths we program here when auto calibration fails are recommended
> values based on pre-SI circuit analysis and characterized across PVT.
> 
> So,  these fail safe values are same for all boards of specific SoC as all
> platform designs follow the design guidelines.
> 
> > > If the SDIO PINCTRL states are mandatory for the SDHCI nodes in the
> > > device-trees, then the DT binding is wrong since it says that all
> > > properties are optional. But I think that the current binding is okay,
> > > since today SDHCI PINCTRL drive-strengths are specified implicitly in
> > > the device-trees, and thus, there is no real need to emit the noisy
> > > warnings in this case.
> > For now I will keep $subject patch applied, but please tell me if I
> > should drop it so we can start over.
> > 
> > In any case, I would appreciate it if someone could have a stab at
> > converting sdhci and tegra DT bindings to yaml.
> > 
> > Kind regards
> > Uffe
> 
> HI Uffe,
> 
> Please drop $subject patch. Will send patch that allows parsing for these
> properties only for SoC where auto cal is enabled as that's where driver
> needs these properties.
> 
> So with this fix, warning will not show up on systems where autocal is not
> enabled.

Yes, I think that's a better option. Have we ensured that on all systems
where autocal is enabled these values are part of the device tree? Just
making sure that we're not going to have some generation still spit out
these warnings because we forgot to update the device tree.

For example I see that we set NVQUIRK_HAS_PADCALIB but I don't see these
properties being set in arch/arm/boot/dts/tegra30.dtsi. Can you add a
patch that also adds the properties for Tegra30?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Dmitry Osipenko]

22.05.2020 15:13, Thierry Reding пишет:
> On Wed, May 20, 2020 at 09:09:33AM -0700, Sowjanya Komatineni wrote:
>>
>> On 5/20/20 4:26 AM, Ulf Hansson wrote:
>>> On Wed, 20 May 2020 at 04:00, Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> 19.05.2020 23:44, Sowjanya Komatineni пишет:
>>>>> On 5/19/20 12:07 PM, Sowjanya Komatineni wrote:
>>>>>> On 5/19/20 11:41 AM, Sowjanya Komatineni wrote:
>>>>>>> On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
>>>>>>>> On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
>>>>>>>>> 19.05.2020 19:24, Thierry Reding пишет:
>>>>>>>>>> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
>>>>>>>>>>> 19.05.2020 10:28, Ulf Hansson пишет:
>>>>>>>>>>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>> Several people asked me about the MMC warnings in the KMSG log and
>>>>>>>>>>>>> I had to tell to ignore them because these warning are
>>>>>>>>>>>>> irrelevant to
>>>>>>>>>>>>> pre-Tegra210 SoCs.
>>>>>>>>>>>> Why are the warnings irrelevant?
>>>>>>>>>>> That's what the DT binding doc says [1].
>>>>>>>>>>>
>>>>>>>>>>> [1]
>>>>>>>>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Although, looking at the driver's code and TRM docs, it seems
>>>>>>>>>>> that all
>>>>>>>>>>> those properties are really irrelevant only to the older Terga20
>>>>>>>>>>> SoC. So
>>>>>>>>>>> the binding doc is a bit misleading.
>>>>>>>>>>>
>>>>>>>>>>> Nevertheless, the binding explicitly says that the properties are
>>>>>>>>>>> optional, which is correct.
>>>>>>>>>> Optional only means that drivers must not fail if these properties
>>>>>>>>>> aren't found, it doesn't mean that the driver can't warn that they
>>>>>>>>>> are missing.
>>>>>>>>>>
>>>>>>>>>> Quite possibly the only reason why they were made optional is because
>>>>>>>>>> they weren't part of the bindings since the beginning. Anything added
>>>>>>>>>> to a binding after the first public release has to be optional by
>>>>>>>>>> definition, otherwise DT ABI wouldn't be stable.
>>>>>>>>>>
>>>>>>>>>> I think these warnings were added on purpose, though I also see that
>>>>>>>>>> there are only values for these in device tree for Tegra186 and
>>>>>>>>>> Tegra194
>>>>>>>>>> but not Tegra210 where these should also be necessary.
>>>>>>>> dt binding doc we have is common for MMC, SD and SDIO of all Tegras.
>>>>>>>> Its not mandatory to have both 3v3 and 1v8 in device tree as based
>>>>>>>> on signal mode.
>>>>>>>>
>>>>>>>> As these driver strengths are SoC specific, they are part of Tegra
>>>>>>>> SoC specific device tree where same values will be applicable to all
>>>>>>>> Tegra SoC specific platforms.
>>>>>>>>
>>>>>>>> Based on interface usage on platform, we use one or both of them
>>>>>>>> like sdcard supports dual voltage and we use both 3V3 and 1V8 but if
>>>>>>>> same interface is used for WIFI SD we use 1V8 only.
>>>>>>>>
>>>>>>>> So made these dt properties as optional.
>>>>>>>>
>>>>>>>> Other reason they are optional is, Tegra210 and prior has drive
>>>>>>>> strength settings part of apb_misc and Tegra186 and later has driver
>>>>>>>> strengths part of SDMMC controller. So,
>>>>>>>>
>>>>>>>> - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths
>>>>>>>> are applicable for Tegra210 and prior.
>>>>>>>> - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are
>>>>>>>> for T186 onwards for driver strengths
>>>>>>>>
>>>>>>>> Looks like dt binding doc need fix to clearly document these based
>>>>>>>> on SoC or agree with Yaml we can conditionally specify pinctrl or dt
>>>>>>>> properties based on SoC dependent.
>>>>>>>>
>>>>>>>>
>>>>>>>>>> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
>>>>>>>>>> warnings were added. If these values /should/ be there on a subset of
>>>>>>>>>> Tegra, then I think we should keep them, or add them again, but
>>>>>>>>>> perhaps
>>>>>>>>>> add a better way of identifying when they are necessary and when
>>>>>>>>>> it is
>>>>>>>>>> safe to work without them.
>>>>>>>>>>
>>>>>>>>>> That said, looking at those checks I wonder if they are perhaps just
>>>>>>>>>> wrong. Or at the very least they seem redundant. It looks to me like
>>>>>>>>>> they can just be:
>>>>>>>>>>
>>>>>>>>>>      if (tegra_host->pinctrl_state_XYZ == NULL) {
>>>>>>>>>>          ...
>>>>>>>>>>      }
>>>>>>>>>>
>>>>>>>>>> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
>>>>>>>>>> also obvious why we're warning about them on platforms where these
>>>>>>>>>> properties don't exist in DT.
>>>>>>>> As drive strengths are through dt properties for T186 and later and
>>>>>>>> thru pinctrl for T210 and prior, driver first checks for dt autocal
>>>>>>>> timeout pull-up/down properties and if they are not found, it then
>>>>>>>> checks for presence of pinctrl_state_xyx_drv only when valid
>>>>>>>> pinctrl_state_xyz is present.
>>>>>>>>
>>>>>>>> Driver expects either pinctrl or dt properties and shows warning
>>>>>>>> when neither of them are present as its mandatory to use fixed
>>>>>>>> driver strengths when auto calibration fails.
>>>>>>>>
>>>>>>>>      err = device_property_read_u32(host->mmc->parent,
>>>>>>>>              "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
>>>>>>>>              &autocal->pull_down_3v3_timeout);
>>>>>>>>      if (err) {
>>>>>>>>          if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
>>>>>>>>              (tegra_host->pinctrl_state_3v3_drv == NULL))
>>>>>>>>              pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
>>>>>>>>                  mmc_hostname(host->mmc));
>>>>>>>>          autocal->pull_down_3v3_timeout = 0;
>>>>>>>>      }
>>>>>>>>
>>>>>>>>>> So I think we either need to add those values where appropriate so
>>>>>>>>>> that
>>>>>>>>>> the warning doesn't show, or we need to narrow down where they are
>>>>>>>>>> really needed and add a corresponding condition.
>>>>>>>>>>
>>>>>>>>>> But again, perhaps Sowjanya can help clarify if these really are only
>>>>>>>>>> needed on Tegra210 and later or if these also apply to older chips.
>>>>>>>>> Either way will be cleaner to convert the DT binding to YAML rather
>>>>>>>>> than
>>>>>>>>> clutter the driver, IMO.
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>> Auto calibration is present from Tegra30 onward and looking into
>>>>>>> change where autocalibration was added to sdhci driver somehow it was
>>>>>>> enabled only for T30/T210/T186/T194.
>>>>>>>
>>>>>>> tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration
>>>>>>> was added to driver and I see this dt parse is being done
>>>>>>> irrespective of NVQUIRK_HAS_PADCALIB quirk so even on platforms
>>>>>>> without auto cal enabled in driver, these messages shows up.
>>>>>>>
>>>>>>> This should be fixed in driver to allow
>>>>>>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is
>>>>>>> set to avoid dt parsing to happen on platforms that don't have auto
>>>>>>> cal enabled.
>>>>>> Warning on missing drive strengths when auto cal is enabled should be
>>>>>> present as we should switch to fixed recommended drive strengths when
>>>>>> auto cal fails.
>>>>>>
>>>>>> So probably proper fix should be
>>>>>>
>>>>>> - allow tegra_sdhci_parse_pad_autocal_dt() only when
>>>>>> NVQUIRK_HAS_PADCALIB is set
>>>>>>
>>>>>> - current driver sets NVQUIRK_HAS_PADCALIB for T30 as well so need to
>>>>>> add pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" to Tegra30 device tree.
>>>>> [Correction] T30 has same drive strengths to use irrespective of signal
>>>>> voltage and it doesn't have pad control. So for T3- we can update device
>>>>> tree to specify "default" pinctrl with drvup/dn settings.
>>>>>> - Keep warning message of missing auto cal timeouts as its mandatory
>>>>>> to use fixed recommended driver strengths when auto cal fails.
>>>>>>
>>>>> Regarding warnings, I guess simpler and easy fix is to remove warning
>>>>> message on missing 3v3/1v8 drive strengths as pinctrl/dt properties were
>>>>> already added for T210/186/194 where we need and old device tree don't
>>>>> have them but the case where auto cal can fail is very rare.
>>>>>
>>>>> Otherwise should update driver to allow
>>>>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set
>>>>> and also within tegra_sdhci_parse_pad_autocal_dt() show warning of
>>>>> missing 3v3/1v8 settings only when NVQUIRK_NEEDS_PAD_CONTROL is set.
>>>>>
>>>>> Thierry, please suggest if you prefer to removing warnings or fix driver
>>>>> to show warning based on PADCALIB and PAD_CONTROL quirks.
>>>> The SDIO PINCTRL drive-strengths are usually a part of the board's
>>>> default PINCTRL state, which is either preset by bootloader or by
>>>> PINCTRL driver early at a boot time.
>>>>
>>>> The SDIO drive-strengths values should be board-specific and not
>>>> SoC-specific because they should depend on the electrical properties of
>>>> the board, IIUC.
>>
>> Drive strengths we program here when auto calibration fails are recommended
>> values based on pre-SI circuit analysis and characterized across PVT.
>>
>> So,  these fail safe values are same for all boards of specific SoC as all
>> platform designs follow the design guidelines.
>>
>>>> If the SDIO PINCTRL states are mandatory for the SDHCI nodes in the
>>>> device-trees, then the DT binding is wrong since it says that all
>>>> properties are optional. But I think that the current binding is okay,
>>>> since today SDHCI PINCTRL drive-strengths are specified implicitly in
>>>> the device-trees, and thus, there is no real need to emit the noisy
>>>> warnings in this case.
>>> For now I will keep $subject patch applied, but please tell me if I
>>> should drop it so we can start over.
>>>
>>> In any case, I would appreciate it if someone could have a stab at
>>> converting sdhci and tegra DT bindings to yaml.
>>>
>>> Kind regards
>>> Uffe
>>
>> HI Uffe,
>>
>> Please drop $subject patch. Will send patch that allows parsing for these
>> properties only for SoC where auto cal is enabled as that's where driver
>> needs these properties.
>>
>> So with this fix, warning will not show up on systems where autocal is not
>> enabled.
> 
> Yes, I think that's a better option. Have we ensured that on all systems
> where autocal is enabled these values are part of the device tree? Just
> making sure that we're not going to have some generation still spit out
> these warnings because we forgot to update the device tree.
> 
> For example I see that we set NVQUIRK_HAS_PADCALIB but I don't see these
> properties being set in arch/arm/boot/dts/tegra30.dtsi. Can you add a
> patch that also adds the properties for Tegra30?

I don't see the warnings on T30 using Sowjanya's patch which checks for
NVQUIRK_NEEDS_PAD_CONTROL and not NVQUIRK_HAS_PADCALIB.

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Dmitry Osipenko]

22.05.2020 15:13, Thierry Reding пишет:
> On Wed, May 20, 2020 at 09:09:33AM -0700, Sowjanya Komatineni wrote:
>>
>> On 5/20/20 4:26 AM, Ulf Hansson wrote:
>>> On Wed, 20 May 2020 at 04:00, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>> 19.05.2020 23:44, Sowjanya Komatineni пишет:
>>>>> On 5/19/20 12:07 PM, Sowjanya Komatineni wrote:
>>>>>> On 5/19/20 11:41 AM, Sowjanya Komatineni wrote:
>>>>>>> On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
>>>>>>>> On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
>>>>>>>>> 19.05.2020 19:24, Thierry Reding пишет:
>>>>>>>>>> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
>>>>>>>>>>> 19.05.2020 10:28, Ulf Hansson пишет:
>>>>>>>>>>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>> Several people asked me about the MMC warnings in the KMSG log and
>>>>>>>>>>>>> I had to tell to ignore them because these warning are
>>>>>>>>>>>>> irrelevant to
>>>>>>>>>>>>> pre-Tegra210 SoCs.
>>>>>>>>>>>> Why are the warnings irrelevant?
>>>>>>>>>>> That's what the DT binding doc says [1].
>>>>>>>>>>>
>>>>>>>>>>> [1]
>>>>>>>>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Although, looking at the driver's code and TRM docs, it seems
>>>>>>>>>>> that all
>>>>>>>>>>> those properties are really irrelevant only to the older Terga20
>>>>>>>>>>> SoC. So
>>>>>>>>>>> the binding doc is a bit misleading.
>>>>>>>>>>>
>>>>>>>>>>> Nevertheless, the binding explicitly says that the properties are
>>>>>>>>>>> optional, which is correct.
>>>>>>>>>> Optional only means that drivers must not fail if these properties
>>>>>>>>>> aren't found, it doesn't mean that the driver can't warn that they
>>>>>>>>>> are missing.
>>>>>>>>>>
>>>>>>>>>> Quite possibly the only reason why they were made optional is because
>>>>>>>>>> they weren't part of the bindings since the beginning. Anything added
>>>>>>>>>> to a binding after the first public release has to be optional by
>>>>>>>>>> definition, otherwise DT ABI wouldn't be stable.
>>>>>>>>>>
>>>>>>>>>> I think these warnings were added on purpose, though I also see that
>>>>>>>>>> there are only values for these in device tree for Tegra186 and
>>>>>>>>>> Tegra194
>>>>>>>>>> but not Tegra210 where these should also be necessary.
>>>>>>>> dt binding doc we have is common for MMC, SD and SDIO of all Tegras.
>>>>>>>> Its not mandatory to have both 3v3 and 1v8 in device tree as based
>>>>>>>> on signal mode.
>>>>>>>>
>>>>>>>> As these driver strengths are SoC specific, they are part of Tegra
>>>>>>>> SoC specific device tree where same values will be applicable to all
>>>>>>>> Tegra SoC specific platforms.
>>>>>>>>
>>>>>>>> Based on interface usage on platform, we use one or both of them
>>>>>>>> like sdcard supports dual voltage and we use both 3V3 and 1V8 but if
>>>>>>>> same interface is used for WIFI SD we use 1V8 only.
>>>>>>>>
>>>>>>>> So made these dt properties as optional.
>>>>>>>>
>>>>>>>> Other reason they are optional is, Tegra210 and prior has drive
>>>>>>>> strength settings part of apb_misc and Tegra186 and later has driver
>>>>>>>> strengths part of SDMMC controller. So,
>>>>>>>>
>>>>>>>> - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths
>>>>>>>> are applicable for Tegra210 and prior.
>>>>>>>> - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are
>>>>>>>> for T186 onwards for driver strengths
>>>>>>>>
>>>>>>>> Looks like dt binding doc need fix to clearly document these based
>>>>>>>> on SoC or agree with Yaml we can conditionally specify pinctrl or dt
>>>>>>>> properties based on SoC dependent.
>>>>>>>>
>>>>>>>>
>>>>>>>>>> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
>>>>>>>>>> warnings were added. If these values /should/ be there on a subset of
>>>>>>>>>> Tegra, then I think we should keep them, or add them again, but
>>>>>>>>>> perhaps
>>>>>>>>>> add a better way of identifying when they are necessary and when
>>>>>>>>>> it is
>>>>>>>>>> safe to work without them.
>>>>>>>>>>
>>>>>>>>>> That said, looking at those checks I wonder if they are perhaps just
>>>>>>>>>> wrong. Or at the very least they seem redundant. It looks to me like
>>>>>>>>>> they can just be:
>>>>>>>>>>
>>>>>>>>>>      if (tegra_host->pinctrl_state_XYZ == NULL) {
>>>>>>>>>>          ...
>>>>>>>>>>      }
>>>>>>>>>>
>>>>>>>>>> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
>>>>>>>>>> also obvious why we're warning about them on platforms where these
>>>>>>>>>> properties don't exist in DT.
>>>>>>>> As drive strengths are through dt properties for T186 and later and
>>>>>>>> thru pinctrl for T210 and prior, driver first checks for dt autocal
>>>>>>>> timeout pull-up/down properties and if they are not found, it then
>>>>>>>> checks for presence of pinctrl_state_xyx_drv only when valid
>>>>>>>> pinctrl_state_xyz is present.
>>>>>>>>
>>>>>>>> Driver expects either pinctrl or dt properties and shows warning
>>>>>>>> when neither of them are present as its mandatory to use fixed
>>>>>>>> driver strengths when auto calibration fails.
>>>>>>>>
>>>>>>>>      err = device_property_read_u32(host->mmc->parent,
>>>>>>>>              "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
>>>>>>>>              &autocal->pull_down_3v3_timeout);
>>>>>>>>      if (err) {
>>>>>>>>          if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
>>>>>>>>              (tegra_host->pinctrl_state_3v3_drv == NULL))
>>>>>>>>              pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
>>>>>>>>                  mmc_hostname(host->mmc));
>>>>>>>>          autocal->pull_down_3v3_timeout = 0;
>>>>>>>>      }
>>>>>>>>
>>>>>>>>>> So I think we either need to add those values where appropriate so
>>>>>>>>>> that
>>>>>>>>>> the warning doesn't show, or we need to narrow down where they are
>>>>>>>>>> really needed and add a corresponding condition.
>>>>>>>>>>
>>>>>>>>>> But again, perhaps Sowjanya can help clarify if these really are only
>>>>>>>>>> needed on Tegra210 and later or if these also apply to older chips.
>>>>>>>>> Either way will be cleaner to convert the DT binding to YAML rather
>>>>>>>>> than
>>>>>>>>> clutter the driver, IMO.
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>> Auto calibration is present from Tegra30 onward and looking into
>>>>>>> change where autocalibration was added to sdhci driver somehow it was
>>>>>>> enabled only for T30/T210/T186/T194.
>>>>>>>
>>>>>>> tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration
>>>>>>> was added to driver and I see this dt parse is being done
>>>>>>> irrespective of NVQUIRK_HAS_PADCALIB quirk so even on platforms
>>>>>>> without auto cal enabled in driver, these messages shows up.
>>>>>>>
>>>>>>> This should be fixed in driver to allow
>>>>>>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is
>>>>>>> set to avoid dt parsing to happen on platforms that don't have auto
>>>>>>> cal enabled.
>>>>>> Warning on missing drive strengths when auto cal is enabled should be
>>>>>> present as we should switch to fixed recommended drive strengths when
>>>>>> auto cal fails.
>>>>>>
>>>>>> So probably proper fix should be
>>>>>>
>>>>>> - allow tegra_sdhci_parse_pad_autocal_dt() only when
>>>>>> NVQUIRK_HAS_PADCALIB is set
>>>>>>
>>>>>> - current driver sets NVQUIRK_HAS_PADCALIB for T30 as well so need to
>>>>>> add pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" to Tegra30 device tree.
>>>>> [Correction] T30 has same drive strengths to use irrespective of signal
>>>>> voltage and it doesn't have pad control. So for T3- we can update device
>>>>> tree to specify "default" pinctrl with drvup/dn settings.
>>>>>> - Keep warning message of missing auto cal timeouts as its mandatory
>>>>>> to use fixed recommended driver strengths when auto cal fails.
>>>>>>
>>>>> Regarding warnings, I guess simpler and easy fix is to remove warning
>>>>> message on missing 3v3/1v8 drive strengths as pinctrl/dt properties were
>>>>> already added for T210/186/194 where we need and old device tree don't
>>>>> have them but the case where auto cal can fail is very rare.
>>>>>
>>>>> Otherwise should update driver to allow
>>>>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set
>>>>> and also within tegra_sdhci_parse_pad_autocal_dt() show warning of
>>>>> missing 3v3/1v8 settings only when NVQUIRK_NEEDS_PAD_CONTROL is set.
>>>>>
>>>>> Thierry, please suggest if you prefer to removing warnings or fix driver
>>>>> to show warning based on PADCALIB and PAD_CONTROL quirks.
>>>> The SDIO PINCTRL drive-strengths are usually a part of the board's
>>>> default PINCTRL state, which is either preset by bootloader or by
>>>> PINCTRL driver early at a boot time.
>>>>
>>>> The SDIO drive-strengths values should be board-specific and not
>>>> SoC-specific because they should depend on the electrical properties of
>>>> the board, IIUC.
>>
>> Drive strengths we program here when auto calibration fails are recommended
>> values based on pre-SI circuit analysis and characterized across PVT.
>>
>> So,  these fail safe values are same for all boards of specific SoC as all
>> platform designs follow the design guidelines.
>>
>>>> If the SDIO PINCTRL states are mandatory for the SDHCI nodes in the
>>>> device-trees, then the DT binding is wrong since it says that all
>>>> properties are optional. But I think that the current binding is okay,
>>>> since today SDHCI PINCTRL drive-strengths are specified implicitly in
>>>> the device-trees, and thus, there is no real need to emit the noisy
>>>> warnings in this case.
>>> For now I will keep $subject patch applied, but please tell me if I
>>> should drop it so we can start over.
>>>
>>> In any case, I would appreciate it if someone could have a stab at
>>> converting sdhci and tegra DT bindings to yaml.
>>>
>>> Kind regards
>>> Uffe
>>
>> HI Uffe,
>>
>> Please drop $subject patch. Will send patch that allows parsing for these
>> properties only for SoC where auto cal is enabled as that's where driver
>> needs these properties.
>>
>> So with this fix, warning will not show up on systems where autocal is not
>> enabled.
> 
> Yes, I think that's a better option. Have we ensured that on all systems
> where autocal is enabled these values are part of the device tree? Just
> making sure that we're not going to have some generation still spit out
> these warnings because we forgot to update the device tree.
> 
> For example I see that we set NVQUIRK_HAS_PADCALIB but I don't see these
> properties being set in arch/arm/boot/dts/tegra30.dtsi. Can you add a
> patch that also adds the properties for Tegra30?

I don't see the warnings on T30 using Sowjanya's patch which checks for
NVQUIRK_NEEDS_PAD_CONTROL and not NVQUIRK_HAS_PADCALIB.

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 11824 bytes --]

On Fri, May 22, 2020 at 03:18:40PM +0300, Dmitry Osipenko wrote:
> 22.05.2020 15:13, Thierry Reding пишет:
> > On Wed, May 20, 2020 at 09:09:33AM -0700, Sowjanya Komatineni wrote:
> >>
> >> On 5/20/20 4:26 AM, Ulf Hansson wrote:
> >>> On Wed, 20 May 2020 at 04:00, Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>>> 19.05.2020 23:44, Sowjanya Komatineni пишет:
> >>>>> On 5/19/20 12:07 PM, Sowjanya Komatineni wrote:
> >>>>>> On 5/19/20 11:41 AM, Sowjanya Komatineni wrote:
> >>>>>>> On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
> >>>>>>>> On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
> >>>>>>>>> 19.05.2020 19:24, Thierry Reding пишет:
> >>>>>>>>>> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
> >>>>>>>>>>> 19.05.2020 10:28, Ulf Hansson пишет:
> >>>>>>>>>>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com>
> >>>>>>>>>>>> wrote:
> >>>>>>>>>>>>> Several people asked me about the MMC warnings in the KMSG log and
> >>>>>>>>>>>>> I had to tell to ignore them because these warning are
> >>>>>>>>>>>>> irrelevant to
> >>>>>>>>>>>>> pre-Tegra210 SoCs.
> >>>>>>>>>>>> Why are the warnings irrelevant?
> >>>>>>>>>>> That's what the DT binding doc says [1].
> >>>>>>>>>>>
> >>>>>>>>>>> [1]
> >>>>>>>>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Although, looking at the driver's code and TRM docs, it seems
> >>>>>>>>>>> that all
> >>>>>>>>>>> those properties are really irrelevant only to the older Terga20
> >>>>>>>>>>> SoC. So
> >>>>>>>>>>> the binding doc is a bit misleading.
> >>>>>>>>>>>
> >>>>>>>>>>> Nevertheless, the binding explicitly says that the properties are
> >>>>>>>>>>> optional, which is correct.
> >>>>>>>>>> Optional only means that drivers must not fail if these properties
> >>>>>>>>>> aren't found, it doesn't mean that the driver can't warn that they
> >>>>>>>>>> are missing.
> >>>>>>>>>>
> >>>>>>>>>> Quite possibly the only reason why they were made optional is because
> >>>>>>>>>> they weren't part of the bindings since the beginning. Anything added
> >>>>>>>>>> to a binding after the first public release has to be optional by
> >>>>>>>>>> definition, otherwise DT ABI wouldn't be stable.
> >>>>>>>>>>
> >>>>>>>>>> I think these warnings were added on purpose, though I also see that
> >>>>>>>>>> there are only values for these in device tree for Tegra186 and
> >>>>>>>>>> Tegra194
> >>>>>>>>>> but not Tegra210 where these should also be necessary.
> >>>>>>>> dt binding doc we have is common for MMC, SD and SDIO of all Tegras.
> >>>>>>>> Its not mandatory to have both 3v3 and 1v8 in device tree as based
> >>>>>>>> on signal mode.
> >>>>>>>>
> >>>>>>>> As these driver strengths are SoC specific, they are part of Tegra
> >>>>>>>> SoC specific device tree where same values will be applicable to all
> >>>>>>>> Tegra SoC specific platforms.
> >>>>>>>>
> >>>>>>>> Based on interface usage on platform, we use one or both of them
> >>>>>>>> like sdcard supports dual voltage and we use both 3V3 and 1V8 but if
> >>>>>>>> same interface is used for WIFI SD we use 1V8 only.
> >>>>>>>>
> >>>>>>>> So made these dt properties as optional.
> >>>>>>>>
> >>>>>>>> Other reason they are optional is, Tegra210 and prior has drive
> >>>>>>>> strength settings part of apb_misc and Tegra186 and later has driver
> >>>>>>>> strengths part of SDMMC controller. So,
> >>>>>>>>
> >>>>>>>> - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths
> >>>>>>>> are applicable for Tegra210 and prior.
> >>>>>>>> - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are
> >>>>>>>> for T186 onwards for driver strengths
> >>>>>>>>
> >>>>>>>> Looks like dt binding doc need fix to clearly document these based
> >>>>>>>> on SoC or agree with Yaml we can conditionally specify pinctrl or dt
> >>>>>>>> properties based on SoC dependent.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
> >>>>>>>>>> warnings were added. If these values /should/ be there on a subset of
> >>>>>>>>>> Tegra, then I think we should keep them, or add them again, but
> >>>>>>>>>> perhaps
> >>>>>>>>>> add a better way of identifying when they are necessary and when
> >>>>>>>>>> it is
> >>>>>>>>>> safe to work without them.
> >>>>>>>>>>
> >>>>>>>>>> That said, looking at those checks I wonder if they are perhaps just
> >>>>>>>>>> wrong. Or at the very least they seem redundant. It looks to me like
> >>>>>>>>>> they can just be:
> >>>>>>>>>>
> >>>>>>>>>>      if (tegra_host->pinctrl_state_XYZ == NULL) {
> >>>>>>>>>>          ...
> >>>>>>>>>>      }
> >>>>>>>>>>
> >>>>>>>>>> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
> >>>>>>>>>> also obvious why we're warning about them on platforms where these
> >>>>>>>>>> properties don't exist in DT.
> >>>>>>>> As drive strengths are through dt properties for T186 and later and
> >>>>>>>> thru pinctrl for T210 and prior, driver first checks for dt autocal
> >>>>>>>> timeout pull-up/down properties and if they are not found, it then
> >>>>>>>> checks for presence of pinctrl_state_xyx_drv only when valid
> >>>>>>>> pinctrl_state_xyz is present.
> >>>>>>>>
> >>>>>>>> Driver expects either pinctrl or dt properties and shows warning
> >>>>>>>> when neither of them are present as its mandatory to use fixed
> >>>>>>>> driver strengths when auto calibration fails.
> >>>>>>>>
> >>>>>>>>      err = device_property_read_u32(host->mmc->parent,
> >>>>>>>>              "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
> >>>>>>>>              &autocal->pull_down_3v3_timeout);
> >>>>>>>>      if (err) {
> >>>>>>>>          if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
> >>>>>>>>              (tegra_host->pinctrl_state_3v3_drv == NULL))
> >>>>>>>>              pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
> >>>>>>>>                  mmc_hostname(host->mmc));
> >>>>>>>>          autocal->pull_down_3v3_timeout = 0;
> >>>>>>>>      }
> >>>>>>>>
> >>>>>>>>>> So I think we either need to add those values where appropriate so
> >>>>>>>>>> that
> >>>>>>>>>> the warning doesn't show, or we need to narrow down where they are
> >>>>>>>>>> really needed and add a corresponding condition.
> >>>>>>>>>>
> >>>>>>>>>> But again, perhaps Sowjanya can help clarify if these really are only
> >>>>>>>>>> needed on Tegra210 and later or if these also apply to older chips.
> >>>>>>>>> Either way will be cleaner to convert the DT binding to YAML rather
> >>>>>>>>> than
> >>>>>>>>> clutter the driver, IMO.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>> Auto calibration is present from Tegra30 onward and looking into
> >>>>>>> change where autocalibration was added to sdhci driver somehow it was
> >>>>>>> enabled only for T30/T210/T186/T194.
> >>>>>>>
> >>>>>>> tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration
> >>>>>>> was added to driver and I see this dt parse is being done
> >>>>>>> irrespective of NVQUIRK_HAS_PADCALIB quirk so even on platforms
> >>>>>>> without auto cal enabled in driver, these messages shows up.
> >>>>>>>
> >>>>>>> This should be fixed in driver to allow
> >>>>>>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is
> >>>>>>> set to avoid dt parsing to happen on platforms that don't have auto
> >>>>>>> cal enabled.
> >>>>>> Warning on missing drive strengths when auto cal is enabled should be
> >>>>>> present as we should switch to fixed recommended drive strengths when
> >>>>>> auto cal fails.
> >>>>>>
> >>>>>> So probably proper fix should be
> >>>>>>
> >>>>>> - allow tegra_sdhci_parse_pad_autocal_dt() only when
> >>>>>> NVQUIRK_HAS_PADCALIB is set
> >>>>>>
> >>>>>> - current driver sets NVQUIRK_HAS_PADCALIB for T30 as well so need to
> >>>>>> add pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" to Tegra30 device tree.
> >>>>> [Correction] T30 has same drive strengths to use irrespective of signal
> >>>>> voltage and it doesn't have pad control. So for T3- we can update device
> >>>>> tree to specify "default" pinctrl with drvup/dn settings.
> >>>>>> - Keep warning message of missing auto cal timeouts as its mandatory
> >>>>>> to use fixed recommended driver strengths when auto cal fails.
> >>>>>>
> >>>>> Regarding warnings, I guess simpler and easy fix is to remove warning
> >>>>> message on missing 3v3/1v8 drive strengths as pinctrl/dt properties were
> >>>>> already added for T210/186/194 where we need and old device tree don't
> >>>>> have them but the case where auto cal can fail is very rare.
> >>>>>
> >>>>> Otherwise should update driver to allow
> >>>>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set
> >>>>> and also within tegra_sdhci_parse_pad_autocal_dt() show warning of
> >>>>> missing 3v3/1v8 settings only when NVQUIRK_NEEDS_PAD_CONTROL is set.
> >>>>>
> >>>>> Thierry, please suggest if you prefer to removing warnings or fix driver
> >>>>> to show warning based on PADCALIB and PAD_CONTROL quirks.
> >>>> The SDIO PINCTRL drive-strengths are usually a part of the board's
> >>>> default PINCTRL state, which is either preset by bootloader or by
> >>>> PINCTRL driver early at a boot time.
> >>>>
> >>>> The SDIO drive-strengths values should be board-specific and not
> >>>> SoC-specific because they should depend on the electrical properties of
> >>>> the board, IIUC.
> >>
> >> Drive strengths we program here when auto calibration fails are recommended
> >> values based on pre-SI circuit analysis and characterized across PVT.
> >>
> >> So,  these fail safe values are same for all boards of specific SoC as all
> >> platform designs follow the design guidelines.
> >>
> >>>> If the SDIO PINCTRL states are mandatory for the SDHCI nodes in the
> >>>> device-trees, then the DT binding is wrong since it says that all
> >>>> properties are optional. But I think that the current binding is okay,
> >>>> since today SDHCI PINCTRL drive-strengths are specified implicitly in
> >>>> the device-trees, and thus, there is no real need to emit the noisy
> >>>> warnings in this case.
> >>> For now I will keep $subject patch applied, but please tell me if I
> >>> should drop it so we can start over.
> >>>
> >>> In any case, I would appreciate it if someone could have a stab at
> >>> converting sdhci and tegra DT bindings to yaml.
> >>>
> >>> Kind regards
> >>> Uffe
> >>
> >> HI Uffe,
> >>
> >> Please drop $subject patch. Will send patch that allows parsing for these
> >> properties only for SoC where auto cal is enabled as that's where driver
> >> needs these properties.
> >>
> >> So with this fix, warning will not show up on systems where autocal is not
> >> enabled.
> > 
> > Yes, I think that's a better option. Have we ensured that on all systems
> > where autocal is enabled these values are part of the device tree? Just
> > making sure that we're not going to have some generation still spit out
> > these warnings because we forgot to update the device tree.
> > 
> > For example I see that we set NVQUIRK_HAS_PADCALIB but I don't see these
> > properties being set in arch/arm/boot/dts/tegra30.dtsi. Can you add a
> > patch that also adds the properties for Tegra30?
> 
> I don't see the warnings on T30 using Sowjanya's patch which checks for
> NVQUIRK_NEEDS_PAD_CONTROL and not NVQUIRK_HAS_PADCALIB.

Yeah, I noticed that too when looking at Sowjanya's patch. The fact that
we have two of these quirks is somewhat confusing to me. Perhaps we can
add a comment near their definition to clarify what their purpose is?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 11794 bytes --]

On Fri, May 22, 2020 at 03:18:40PM +0300, Dmitry Osipenko wrote:
> 22.05.2020 15:13, Thierry Reding пишет:
> > On Wed, May 20, 2020 at 09:09:33AM -0700, Sowjanya Komatineni wrote:
> >>
> >> On 5/20/20 4:26 AM, Ulf Hansson wrote:
> >>> On Wed, 20 May 2020 at 04:00, Dmitry Osipenko <digetx@gmail.com> wrote:
> >>>> 19.05.2020 23:44, Sowjanya Komatineni пишет:
> >>>>> On 5/19/20 12:07 PM, Sowjanya Komatineni wrote:
> >>>>>> On 5/19/20 11:41 AM, Sowjanya Komatineni wrote:
> >>>>>>> On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
> >>>>>>>> On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
> >>>>>>>>> 19.05.2020 19:24, Thierry Reding пишет:
> >>>>>>>>>> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
> >>>>>>>>>>> 19.05.2020 10:28, Ulf Hansson пишет:
> >>>>>>>>>>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com>
> >>>>>>>>>>>> wrote:
> >>>>>>>>>>>>> Several people asked me about the MMC warnings in the KMSG log and
> >>>>>>>>>>>>> I had to tell to ignore them because these warning are
> >>>>>>>>>>>>> irrelevant to
> >>>>>>>>>>>>> pre-Tegra210 SoCs.
> >>>>>>>>>>>> Why are the warnings irrelevant?
> >>>>>>>>>>> That's what the DT binding doc says [1].
> >>>>>>>>>>>
> >>>>>>>>>>> [1]
> >>>>>>>>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Although, looking at the driver's code and TRM docs, it seems
> >>>>>>>>>>> that all
> >>>>>>>>>>> those properties are really irrelevant only to the older Terga20
> >>>>>>>>>>> SoC. So
> >>>>>>>>>>> the binding doc is a bit misleading.
> >>>>>>>>>>>
> >>>>>>>>>>> Nevertheless, the binding explicitly says that the properties are
> >>>>>>>>>>> optional, which is correct.
> >>>>>>>>>> Optional only means that drivers must not fail if these properties
> >>>>>>>>>> aren't found, it doesn't mean that the driver can't warn that they
> >>>>>>>>>> are missing.
> >>>>>>>>>>
> >>>>>>>>>> Quite possibly the only reason why they were made optional is because
> >>>>>>>>>> they weren't part of the bindings since the beginning. Anything added
> >>>>>>>>>> to a binding after the first public release has to be optional by
> >>>>>>>>>> definition, otherwise DT ABI wouldn't be stable.
> >>>>>>>>>>
> >>>>>>>>>> I think these warnings were added on purpose, though I also see that
> >>>>>>>>>> there are only values for these in device tree for Tegra186 and
> >>>>>>>>>> Tegra194
> >>>>>>>>>> but not Tegra210 where these should also be necessary.
> >>>>>>>> dt binding doc we have is common for MMC, SD and SDIO of all Tegras.
> >>>>>>>> Its not mandatory to have both 3v3 and 1v8 in device tree as based
> >>>>>>>> on signal mode.
> >>>>>>>>
> >>>>>>>> As these driver strengths are SoC specific, they are part of Tegra
> >>>>>>>> SoC specific device tree where same values will be applicable to all
> >>>>>>>> Tegra SoC specific platforms.
> >>>>>>>>
> >>>>>>>> Based on interface usage on platform, we use one or both of them
> >>>>>>>> like sdcard supports dual voltage and we use both 3V3 and 1V8 but if
> >>>>>>>> same interface is used for WIFI SD we use 1V8 only.
> >>>>>>>>
> >>>>>>>> So made these dt properties as optional.
> >>>>>>>>
> >>>>>>>> Other reason they are optional is, Tegra210 and prior has drive
> >>>>>>>> strength settings part of apb_misc and Tegra186 and later has driver
> >>>>>>>> strengths part of SDMMC controller. So,
> >>>>>>>>
> >>>>>>>> - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths
> >>>>>>>> are applicable for Tegra210 and prior.
> >>>>>>>> - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are
> >>>>>>>> for T186 onwards for driver strengths
> >>>>>>>>
> >>>>>>>> Looks like dt binding doc need fix to clearly document these based
> >>>>>>>> on SoC or agree with Yaml we can conditionally specify pinctrl or dt
> >>>>>>>> properties based on SoC dependent.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
> >>>>>>>>>> warnings were added. If these values /should/ be there on a subset of
> >>>>>>>>>> Tegra, then I think we should keep them, or add them again, but
> >>>>>>>>>> perhaps
> >>>>>>>>>> add a better way of identifying when they are necessary and when
> >>>>>>>>>> it is
> >>>>>>>>>> safe to work without them.
> >>>>>>>>>>
> >>>>>>>>>> That said, looking at those checks I wonder if they are perhaps just
> >>>>>>>>>> wrong. Or at the very least they seem redundant. It looks to me like
> >>>>>>>>>> they can just be:
> >>>>>>>>>>
> >>>>>>>>>>      if (tegra_host->pinctrl_state_XYZ == NULL) {
> >>>>>>>>>>          ...
> >>>>>>>>>>      }
> >>>>>>>>>>
> >>>>>>>>>> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
> >>>>>>>>>> also obvious why we're warning about them on platforms where these
> >>>>>>>>>> properties don't exist in DT.
> >>>>>>>> As drive strengths are through dt properties for T186 and later and
> >>>>>>>> thru pinctrl for T210 and prior, driver first checks for dt autocal
> >>>>>>>> timeout pull-up/down properties and if they are not found, it then
> >>>>>>>> checks for presence of pinctrl_state_xyx_drv only when valid
> >>>>>>>> pinctrl_state_xyz is present.
> >>>>>>>>
> >>>>>>>> Driver expects either pinctrl or dt properties and shows warning
> >>>>>>>> when neither of them are present as its mandatory to use fixed
> >>>>>>>> driver strengths when auto calibration fails.
> >>>>>>>>
> >>>>>>>>      err = device_property_read_u32(host->mmc->parent,
> >>>>>>>>              "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
> >>>>>>>>              &autocal->pull_down_3v3_timeout);
> >>>>>>>>      if (err) {
> >>>>>>>>          if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
> >>>>>>>>              (tegra_host->pinctrl_state_3v3_drv == NULL))
> >>>>>>>>              pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
> >>>>>>>>                  mmc_hostname(host->mmc));
> >>>>>>>>          autocal->pull_down_3v3_timeout = 0;
> >>>>>>>>      }
> >>>>>>>>
> >>>>>>>>>> So I think we either need to add those values where appropriate so
> >>>>>>>>>> that
> >>>>>>>>>> the warning doesn't show, or we need to narrow down where they are
> >>>>>>>>>> really needed and add a corresponding condition.
> >>>>>>>>>>
> >>>>>>>>>> But again, perhaps Sowjanya can help clarify if these really are only
> >>>>>>>>>> needed on Tegra210 and later or if these also apply to older chips.
> >>>>>>>>> Either way will be cleaner to convert the DT binding to YAML rather
> >>>>>>>>> than
> >>>>>>>>> clutter the driver, IMO.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>> Auto calibration is present from Tegra30 onward and looking into
> >>>>>>> change where autocalibration was added to sdhci driver somehow it was
> >>>>>>> enabled only for T30/T210/T186/T194.
> >>>>>>>
> >>>>>>> tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration
> >>>>>>> was added to driver and I see this dt parse is being done
> >>>>>>> irrespective of NVQUIRK_HAS_PADCALIB quirk so even on platforms
> >>>>>>> without auto cal enabled in driver, these messages shows up.
> >>>>>>>
> >>>>>>> This should be fixed in driver to allow
> >>>>>>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is
> >>>>>>> set to avoid dt parsing to happen on platforms that don't have auto
> >>>>>>> cal enabled.
> >>>>>> Warning on missing drive strengths when auto cal is enabled should be
> >>>>>> present as we should switch to fixed recommended drive strengths when
> >>>>>> auto cal fails.
> >>>>>>
> >>>>>> So probably proper fix should be
> >>>>>>
> >>>>>> - allow tegra_sdhci_parse_pad_autocal_dt() only when
> >>>>>> NVQUIRK_HAS_PADCALIB is set
> >>>>>>
> >>>>>> - current driver sets NVQUIRK_HAS_PADCALIB for T30 as well so need to
> >>>>>> add pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" to Tegra30 device tree.
> >>>>> [Correction] T30 has same drive strengths to use irrespective of signal
> >>>>> voltage and it doesn't have pad control. So for T3- we can update device
> >>>>> tree to specify "default" pinctrl with drvup/dn settings.
> >>>>>> - Keep warning message of missing auto cal timeouts as its mandatory
> >>>>>> to use fixed recommended driver strengths when auto cal fails.
> >>>>>>
> >>>>> Regarding warnings, I guess simpler and easy fix is to remove warning
> >>>>> message on missing 3v3/1v8 drive strengths as pinctrl/dt properties were
> >>>>> already added for T210/186/194 where we need and old device tree don't
> >>>>> have them but the case where auto cal can fail is very rare.
> >>>>>
> >>>>> Otherwise should update driver to allow
> >>>>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set
> >>>>> and also within tegra_sdhci_parse_pad_autocal_dt() show warning of
> >>>>> missing 3v3/1v8 settings only when NVQUIRK_NEEDS_PAD_CONTROL is set.
> >>>>>
> >>>>> Thierry, please suggest if you prefer to removing warnings or fix driver
> >>>>> to show warning based on PADCALIB and PAD_CONTROL quirks.
> >>>> The SDIO PINCTRL drive-strengths are usually a part of the board's
> >>>> default PINCTRL state, which is either preset by bootloader or by
> >>>> PINCTRL driver early at a boot time.
> >>>>
> >>>> The SDIO drive-strengths values should be board-specific and not
> >>>> SoC-specific because they should depend on the electrical properties of
> >>>> the board, IIUC.
> >>
> >> Drive strengths we program here when auto calibration fails are recommended
> >> values based on pre-SI circuit analysis and characterized across PVT.
> >>
> >> So,  these fail safe values are same for all boards of specific SoC as all
> >> platform designs follow the design guidelines.
> >>
> >>>> If the SDIO PINCTRL states are mandatory for the SDHCI nodes in the
> >>>> device-trees, then the DT binding is wrong since it says that all
> >>>> properties are optional. But I think that the current binding is okay,
> >>>> since today SDHCI PINCTRL drive-strengths are specified implicitly in
> >>>> the device-trees, and thus, there is no real need to emit the noisy
> >>>> warnings in this case.
> >>> For now I will keep $subject patch applied, but please tell me if I
> >>> should drop it so we can start over.
> >>>
> >>> In any case, I would appreciate it if someone could have a stab at
> >>> converting sdhci and tegra DT bindings to yaml.
> >>>
> >>> Kind regards
> >>> Uffe
> >>
> >> HI Uffe,
> >>
> >> Please drop $subject patch. Will send patch that allows parsing for these
> >> properties only for SoC where auto cal is enabled as that's where driver
> >> needs these properties.
> >>
> >> So with this fix, warning will not show up on systems where autocal is not
> >> enabled.
> > 
> > Yes, I think that's a better option. Have we ensured that on all systems
> > where autocal is enabled these values are part of the device tree? Just
> > making sure that we're not going to have some generation still spit out
> > these warnings because we forgot to update the device tree.
> > 
> > For example I see that we set NVQUIRK_HAS_PADCALIB but I don't see these
> > properties being set in arch/arm/boot/dts/tegra30.dtsi. Can you add a
> > patch that also adds the properties for Tegra30?
> 
> I don't see the warnings on T30 using Sowjanya's patch which checks for
> NVQUIRK_NEEDS_PAD_CONTROL and not NVQUIRK_HAS_PADCALIB.

Yeah, I noticed that too when looking at Sowjanya's patch. The fact that
we have two of these quirks is somewhat confusing to me. Perhaps we can
add a comment near their definition to clarify what their purpose is?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Sowjanya Komatineni]

On 5/22/20 5:34 AM, Thierry Reding wrote:
> On Fri, May 22, 2020 at 03:18:40PM +0300, Dmitry Osipenko wrote:
>> 22.05.2020 15:13, Thierry Reding пишет:
>>> On Wed, May 20, 2020 at 09:09:33AM -0700, Sowjanya Komatineni wrote:
>>>> On 5/20/20 4:26 AM, Ulf Hansson wrote:
>>>>> On Wed, 20 May 2020 at 04:00, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>>> 19.05.2020 23:44, Sowjanya Komatineni пишет:
>>>>>>> On 5/19/20 12:07 PM, Sowjanya Komatineni wrote:
>>>>>>>> On 5/19/20 11:41 AM, Sowjanya Komatineni wrote:
>>>>>>>>> On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
>>>>>>>>>> On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
>>>>>>>>>>> 19.05.2020 19:24, Thierry Reding пишет:
>>>>>>>>>>>> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
>>>>>>>>>>>>> 19.05.2020 10:28, Ulf Hansson пишет:
>>>>>>>>>>>>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com>
>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>> Several people asked me about the MMC warnings in the KMSG log and
>>>>>>>>>>>>>>> I had to tell to ignore them because these warning are
>>>>>>>>>>>>>>> irrelevant to
>>>>>>>>>>>>>>> pre-Tegra210 SoCs.
>>>>>>>>>>>>>> Why are the warnings irrelevant?
>>>>>>>>>>>>> That's what the DT binding doc says [1].
>>>>>>>>>>>>>
>>>>>>>>>>>>> [1]
>>>>>>>>>>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Although, looking at the driver's code and TRM docs, it seems
>>>>>>>>>>>>> that all
>>>>>>>>>>>>> those properties are really irrelevant only to the older Terga20
>>>>>>>>>>>>> SoC. So
>>>>>>>>>>>>> the binding doc is a bit misleading.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Nevertheless, the binding explicitly says that the properties are
>>>>>>>>>>>>> optional, which is correct.
>>>>>>>>>>>> Optional only means that drivers must not fail if these properties
>>>>>>>>>>>> aren't found, it doesn't mean that the driver can't warn that they
>>>>>>>>>>>> are missing.
>>>>>>>>>>>>
>>>>>>>>>>>> Quite possibly the only reason why they were made optional is because
>>>>>>>>>>>> they weren't part of the bindings since the beginning. Anything added
>>>>>>>>>>>> to a binding after the first public release has to be optional by
>>>>>>>>>>>> definition, otherwise DT ABI wouldn't be stable.
>>>>>>>>>>>>
>>>>>>>>>>>> I think these warnings were added on purpose, though I also see that
>>>>>>>>>>>> there are only values for these in device tree for Tegra186 and
>>>>>>>>>>>> Tegra194
>>>>>>>>>>>> but not Tegra210 where these should also be necessary.
>>>>>>>>>> dt binding doc we have is common for MMC, SD and SDIO of all Tegras.
>>>>>>>>>> Its not mandatory to have both 3v3 and 1v8 in device tree as based
>>>>>>>>>> on signal mode.
>>>>>>>>>>
>>>>>>>>>> As these driver strengths are SoC specific, they are part of Tegra
>>>>>>>>>> SoC specific device tree where same values will be applicable to all
>>>>>>>>>> Tegra SoC specific platforms.
>>>>>>>>>>
>>>>>>>>>> Based on interface usage on platform, we use one or both of them
>>>>>>>>>> like sdcard supports dual voltage and we use both 3V3 and 1V8 but if
>>>>>>>>>> same interface is used for WIFI SD we use 1V8 only.
>>>>>>>>>>
>>>>>>>>>> So made these dt properties as optional.
>>>>>>>>>>
>>>>>>>>>> Other reason they are optional is, Tegra210 and prior has drive
>>>>>>>>>> strength settings part of apb_misc and Tegra186 and later has driver
>>>>>>>>>> strengths part of SDMMC controller. So,
>>>>>>>>>>
>>>>>>>>>> - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths
>>>>>>>>>> are applicable for Tegra210 and prior.
>>>>>>>>>> - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are
>>>>>>>>>> for T186 onwards for driver strengths
>>>>>>>>>>
>>>>>>>>>> Looks like dt binding doc need fix to clearly document these based
>>>>>>>>>> on SoC or agree with Yaml we can conditionally specify pinctrl or dt
>>>>>>>>>> properties based on SoC dependent.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
>>>>>>>>>>>> warnings were added. If these values /should/ be there on a subset of
>>>>>>>>>>>> Tegra, then I think we should keep them, or add them again, but
>>>>>>>>>>>> perhaps
>>>>>>>>>>>> add a better way of identifying when they are necessary and when
>>>>>>>>>>>> it is
>>>>>>>>>>>> safe to work without them.
>>>>>>>>>>>>
>>>>>>>>>>>> That said, looking at those checks I wonder if they are perhaps just
>>>>>>>>>>>> wrong. Or at the very least they seem redundant. It looks to me like
>>>>>>>>>>>> they can just be:
>>>>>>>>>>>>
>>>>>>>>>>>>       if (tegra_host->pinctrl_state_XYZ == NULL) {
>>>>>>>>>>>>           ...
>>>>>>>>>>>>       }
>>>>>>>>>>>>
>>>>>>>>>>>> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
>>>>>>>>>>>> also obvious why we're warning about them on platforms where these
>>>>>>>>>>>> properties don't exist in DT.
>>>>>>>>>> As drive strengths are through dt properties for T186 and later and
>>>>>>>>>> thru pinctrl for T210 and prior, driver first checks for dt autocal
>>>>>>>>>> timeout pull-up/down properties and if they are not found, it then
>>>>>>>>>> checks for presence of pinctrl_state_xyx_drv only when valid
>>>>>>>>>> pinctrl_state_xyz is present.
>>>>>>>>>>
>>>>>>>>>> Driver expects either pinctrl or dt properties and shows warning
>>>>>>>>>> when neither of them are present as its mandatory to use fixed
>>>>>>>>>> driver strengths when auto calibration fails.
>>>>>>>>>>
>>>>>>>>>>       err = device_property_read_u32(host->mmc->parent,
>>>>>>>>>>               "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
>>>>>>>>>>               &autocal->pull_down_3v3_timeout);
>>>>>>>>>>       if (err) {
>>>>>>>>>>           if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
>>>>>>>>>>               (tegra_host->pinctrl_state_3v3_drv == NULL))
>>>>>>>>>>               pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
>>>>>>>>>>                   mmc_hostname(host->mmc));
>>>>>>>>>>           autocal->pull_down_3v3_timeout = 0;
>>>>>>>>>>       }
>>>>>>>>>>
>>>>>>>>>>>> So I think we either need to add those values where appropriate so
>>>>>>>>>>>> that
>>>>>>>>>>>> the warning doesn't show, or we need to narrow down where they are
>>>>>>>>>>>> really needed and add a corresponding condition.
>>>>>>>>>>>>
>>>>>>>>>>>> But again, perhaps Sowjanya can help clarify if these really are only
>>>>>>>>>>>> needed on Tegra210 and later or if these also apply to older chips.
>>>>>>>>>>> Either way will be cleaner to convert the DT binding to YAML rather
>>>>>>>>>>> than
>>>>>>>>>>> clutter the driver, IMO.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>> Auto calibration is present from Tegra30 onward and looking into
>>>>>>>>> change where autocalibration was added to sdhci driver somehow it was
>>>>>>>>> enabled only for T30/T210/T186/T194.
>>>>>>>>>
>>>>>>>>> tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration
>>>>>>>>> was added to driver and I see this dt parse is being done
>>>>>>>>> irrespective of NVQUIRK_HAS_PADCALIB quirk so even on platforms
>>>>>>>>> without auto cal enabled in driver, these messages shows up.
>>>>>>>>>
>>>>>>>>> This should be fixed in driver to allow
>>>>>>>>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is
>>>>>>>>> set to avoid dt parsing to happen on platforms that don't have auto
>>>>>>>>> cal enabled.
>>>>>>>> Warning on missing drive strengths when auto cal is enabled should be
>>>>>>>> present as we should switch to fixed recommended drive strengths when
>>>>>>>> auto cal fails.
>>>>>>>>
>>>>>>>> So probably proper fix should be
>>>>>>>>
>>>>>>>> - allow tegra_sdhci_parse_pad_autocal_dt() only when
>>>>>>>> NVQUIRK_HAS_PADCALIB is set
>>>>>>>>
>>>>>>>> - current driver sets NVQUIRK_HAS_PADCALIB for T30 as well so need to
>>>>>>>> add pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" to Tegra30 device tree.
>>>>>>> [Correction] T30 has same drive strengths to use irrespective of signal
>>>>>>> voltage and it doesn't have pad control. So for T3- we can update device
>>>>>>> tree to specify "default" pinctrl with drvup/dn settings.
>>>>>>>> - Keep warning message of missing auto cal timeouts as its mandatory
>>>>>>>> to use fixed recommended driver strengths when auto cal fails.
>>>>>>>>
>>>>>>> Regarding warnings, I guess simpler and easy fix is to remove warning
>>>>>>> message on missing 3v3/1v8 drive strengths as pinctrl/dt properties were
>>>>>>> already added for T210/186/194 where we need and old device tree don't
>>>>>>> have them but the case where auto cal can fail is very rare.
>>>>>>>
>>>>>>> Otherwise should update driver to allow
>>>>>>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set
>>>>>>> and also within tegra_sdhci_parse_pad_autocal_dt() show warning of
>>>>>>> missing 3v3/1v8 settings only when NVQUIRK_NEEDS_PAD_CONTROL is set.
>>>>>>>
>>>>>>> Thierry, please suggest if you prefer to removing warnings or fix driver
>>>>>>> to show warning based on PADCALIB and PAD_CONTROL quirks.
>>>>>> The SDIO PINCTRL drive-strengths are usually a part of the board's
>>>>>> default PINCTRL state, which is either preset by bootloader or by
>>>>>> PINCTRL driver early at a boot time.
>>>>>>
>>>>>> The SDIO drive-strengths values should be board-specific and not
>>>>>> SoC-specific because they should depend on the electrical properties of
>>>>>> the board, IIUC.
>>>> Drive strengths we program here when auto calibration fails are recommended
>>>> values based on pre-SI circuit analysis and characterized across PVT.
>>>>
>>>> So,  these fail safe values are same for all boards of specific SoC as all
>>>> platform designs follow the design guidelines.
>>>>
>>>>>> If the SDIO PINCTRL states are mandatory for the SDHCI nodes in the
>>>>>> device-trees, then the DT binding is wrong since it says that all
>>>>>> properties are optional. But I think that the current binding is okay,
>>>>>> since today SDHCI PINCTRL drive-strengths are specified implicitly in
>>>>>> the device-trees, and thus, there is no real need to emit the noisy
>>>>>> warnings in this case.
>>>>> For now I will keep $subject patch applied, but please tell me if I
>>>>> should drop it so we can start over.
>>>>>
>>>>> In any case, I would appreciate it if someone could have a stab at
>>>>> converting sdhci and tegra DT bindings to yaml.
>>>>>
>>>>> Kind regards
>>>>> Uffe
>>>> HI Uffe,
>>>>
>>>> Please drop $subject patch. Will send patch that allows parsing for these
>>>> properties only for SoC where auto cal is enabled as that's where driver
>>>> needs these properties.
>>>>
>>>> So with this fix, warning will not show up on systems where autocal is not
>>>> enabled.
>>> Yes, I think that's a better option. Have we ensured that on all systems
>>> where autocal is enabled these values are part of the device tree? Just
>>> making sure that we're not going to have some generation still spit out
>>> these warnings because we forgot to update the device tree.
>>>
>>> For example I see that we set NVQUIRK_HAS_PADCALIB but I don't see these
>>> properties being set in arch/arm/boot/dts/tegra30.dtsi. Can you add a
>>> patch that also adds the properties for Tegra30?
>> I don't see the warnings on T30 using Sowjanya's patch which checks for
>> NVQUIRK_NEEDS_PAD_CONTROL and not NVQUIRK_HAS_PADCALIB.

Both of these quirks are different.

PADCALIB is for auto calibration support.

NEEDS_PAD_CONTROL is for SoC having separate 3V3 and 1V8 pads where they 
have pad state selection and also diff drive strengths apply based on 
3V3 and 1V8 which are used only when auto cal is not used/failed.

So, above warnings are applicable only for SoC having separate pad 
controls. Other SoC which don't have separate pads use default drive 
strengths.

> Yeah, I noticed that too when looking at Sowjanya's patch. The fact that
> we have two of these quirks is somewhat confusing to me. Perhaps we can
> add a comment near their definition to clarify what their purpose is?
>
> Thierry

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 14351 bytes --]

On Fri, May 22, 2020 at 08:22:47AM -0700, Sowjanya Komatineni wrote:
> 
> On 5/22/20 5:34 AM, Thierry Reding wrote:
> > On Fri, May 22, 2020 at 03:18:40PM +0300, Dmitry Osipenko wrote:
> > > 22.05.2020 15:13, Thierry Reding пишет:
> > > > On Wed, May 20, 2020 at 09:09:33AM -0700, Sowjanya Komatineni wrote:
> > > > > On 5/20/20 4:26 AM, Ulf Hansson wrote:
> > > > > > On Wed, 20 May 2020 at 04:00, Dmitry Osipenko <digetx@gmail.com> wrote:
> > > > > > > 19.05.2020 23:44, Sowjanya Komatineni пишет:
> > > > > > > > On 5/19/20 12:07 PM, Sowjanya Komatineni wrote:
> > > > > > > > > On 5/19/20 11:41 AM, Sowjanya Komatineni wrote:
> > > > > > > > > > On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
> > > > > > > > > > > On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
> > > > > > > > > > > > 19.05.2020 19:24, Thierry Reding пишет:
> > > > > > > > > > > > > On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
> > > > > > > > > > > > > > 19.05.2020 10:28, Ulf Hansson пишет:
> > > > > > > > > > > > > > > On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com>
> > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > Several people asked me about the MMC warnings in the KMSG log and
> > > > > > > > > > > > > > > > I had to tell to ignore them because these warning are
> > > > > > > > > > > > > > > > irrelevant to
> > > > > > > > > > > > > > > > pre-Tegra210 SoCs.
> > > > > > > > > > > > > > > Why are the warnings irrelevant?
> > > > > > > > > > > > > > That's what the DT binding doc says [1].
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > [1]
> > > > > > > > > > > > > > https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Although, looking at the driver's code and TRM docs, it seems
> > > > > > > > > > > > > > that all
> > > > > > > > > > > > > > those properties are really irrelevant only to the older Terga20
> > > > > > > > > > > > > > SoC. So
> > > > > > > > > > > > > > the binding doc is a bit misleading.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Nevertheless, the binding explicitly says that the properties are
> > > > > > > > > > > > > > optional, which is correct.
> > > > > > > > > > > > > Optional only means that drivers must not fail if these properties
> > > > > > > > > > > > > aren't found, it doesn't mean that the driver can't warn that they
> > > > > > > > > > > > > are missing.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Quite possibly the only reason why they were made optional is because
> > > > > > > > > > > > > they weren't part of the bindings since the beginning. Anything added
> > > > > > > > > > > > > to a binding after the first public release has to be optional by
> > > > > > > > > > > > > definition, otherwise DT ABI wouldn't be stable.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I think these warnings were added on purpose, though I also see that
> > > > > > > > > > > > > there are only values for these in device tree for Tegra186 and
> > > > > > > > > > > > > Tegra194
> > > > > > > > > > > > > but not Tegra210 where these should also be necessary.
> > > > > > > > > > > dt binding doc we have is common for MMC, SD and SDIO of all Tegras.
> > > > > > > > > > > Its not mandatory to have both 3v3 and 1v8 in device tree as based
> > > > > > > > > > > on signal mode.
> > > > > > > > > > > 
> > > > > > > > > > > As these driver strengths are SoC specific, they are part of Tegra
> > > > > > > > > > > SoC specific device tree where same values will be applicable to all
> > > > > > > > > > > Tegra SoC specific platforms.
> > > > > > > > > > > 
> > > > > > > > > > > Based on interface usage on platform, we use one or both of them
> > > > > > > > > > > like sdcard supports dual voltage and we use both 3V3 and 1V8 but if
> > > > > > > > > > > same interface is used for WIFI SD we use 1V8 only.
> > > > > > > > > > > 
> > > > > > > > > > > So made these dt properties as optional.
> > > > > > > > > > > 
> > > > > > > > > > > Other reason they are optional is, Tegra210 and prior has drive
> > > > > > > > > > > strength settings part of apb_misc and Tegra186 and later has driver
> > > > > > > > > > > strengths part of SDMMC controller. So,
> > > > > > > > > > > 
> > > > > > > > > > > - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths
> > > > > > > > > > > are applicable for Tegra210 and prior.
> > > > > > > > > > > - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are
> > > > > > > > > > > for T186 onwards for driver strengths
> > > > > > > > > > > 
> > > > > > > > > > > Looks like dt binding doc need fix to clearly document these based
> > > > > > > > > > > on SoC or agree with Yaml we can conditionally specify pinctrl or dt
> > > > > > > > > > > properties based on SoC dependent.
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > > > Adding Sowjanya who wrote this code. Perhaps she can clarify why the
> > > > > > > > > > > > > warnings were added. If these values /should/ be there on a subset of
> > > > > > > > > > > > > Tegra, then I think we should keep them, or add them again, but
> > > > > > > > > > > > > perhaps
> > > > > > > > > > > > > add a better way of identifying when they are necessary and when
> > > > > > > > > > > > > it is
> > > > > > > > > > > > > safe to work without them.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > That said, looking at those checks I wonder if they are perhaps just
> > > > > > > > > > > > > wrong. Or at the very least they seem redundant. It looks to me like
> > > > > > > > > > > > > they can just be:
> > > > > > > > > > > > > 
> > > > > > > > > > > > >       if (tegra_host->pinctrl_state_XYZ == NULL) {
> > > > > > > > > > > > >           ...
> > > > > > > > > > > > >       }
> > > > > > > > > > > > > 
> > > > > > > > > > > > > That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
> > > > > > > > > > > > > also obvious why we're warning about them on platforms where these
> > > > > > > > > > > > > properties don't exist in DT.
> > > > > > > > > > > As drive strengths are through dt properties for T186 and later and
> > > > > > > > > > > thru pinctrl for T210 and prior, driver first checks for dt autocal
> > > > > > > > > > > timeout pull-up/down properties and if they are not found, it then
> > > > > > > > > > > checks for presence of pinctrl_state_xyx_drv only when valid
> > > > > > > > > > > pinctrl_state_xyz is present.
> > > > > > > > > > > 
> > > > > > > > > > > Driver expects either pinctrl or dt properties and shows warning
> > > > > > > > > > > when neither of them are present as its mandatory to use fixed
> > > > > > > > > > > driver strengths when auto calibration fails.
> > > > > > > > > > > 
> > > > > > > > > > >       err = device_property_read_u32(host->mmc->parent,
> > > > > > > > > > >               "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
> > > > > > > > > > >               &autocal->pull_down_3v3_timeout);
> > > > > > > > > > >       if (err) {
> > > > > > > > > > >           if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
> > > > > > > > > > >               (tegra_host->pinctrl_state_3v3_drv == NULL))
> > > > > > > > > > >               pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
> > > > > > > > > > >                   mmc_hostname(host->mmc));
> > > > > > > > > > >           autocal->pull_down_3v3_timeout = 0;
> > > > > > > > > > >       }
> > > > > > > > > > > 
> > > > > > > > > > > > > So I think we either need to add those values where appropriate so
> > > > > > > > > > > > > that
> > > > > > > > > > > > > the warning doesn't show, or we need to narrow down where they are
> > > > > > > > > > > > > really needed and add a corresponding condition.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > But again, perhaps Sowjanya can help clarify if these really are only
> > > > > > > > > > > > > needed on Tegra210 and later or if these also apply to older chips.
> > > > > > > > > > > > Either way will be cleaner to convert the DT binding to YAML rather
> > > > > > > > > > > > than
> > > > > > > > > > > > clutter the driver, IMO.
> > > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > Auto calibration is present from Tegra30 onward and looking into
> > > > > > > > > > change where autocalibration was added to sdhci driver somehow it was
> > > > > > > > > > enabled only for T30/T210/T186/T194.
> > > > > > > > > > 
> > > > > > > > > > tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration
> > > > > > > > > > was added to driver and I see this dt parse is being done
> > > > > > > > > > irrespective of NVQUIRK_HAS_PADCALIB quirk so even on platforms
> > > > > > > > > > without auto cal enabled in driver, these messages shows up.
> > > > > > > > > > 
> > > > > > > > > > This should be fixed in driver to allow
> > > > > > > > > > tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is
> > > > > > > > > > set to avoid dt parsing to happen on platforms that don't have auto
> > > > > > > > > > cal enabled.
> > > > > > > > > Warning on missing drive strengths when auto cal is enabled should be
> > > > > > > > > present as we should switch to fixed recommended drive strengths when
> > > > > > > > > auto cal fails.
> > > > > > > > > 
> > > > > > > > > So probably proper fix should be
> > > > > > > > > 
> > > > > > > > > - allow tegra_sdhci_parse_pad_autocal_dt() only when
> > > > > > > > > NVQUIRK_HAS_PADCALIB is set
> > > > > > > > > 
> > > > > > > > > - current driver sets NVQUIRK_HAS_PADCALIB for T30 as well so need to
> > > > > > > > > add pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" to Tegra30 device tree.
> > > > > > > > [Correction] T30 has same drive strengths to use irrespective of signal
> > > > > > > > voltage and it doesn't have pad control. So for T3- we can update device
> > > > > > > > tree to specify "default" pinctrl with drvup/dn settings.
> > > > > > > > > - Keep warning message of missing auto cal timeouts as its mandatory
> > > > > > > > > to use fixed recommended driver strengths when auto cal fails.
> > > > > > > > > 
> > > > > > > > Regarding warnings, I guess simpler and easy fix is to remove warning
> > > > > > > > message on missing 3v3/1v8 drive strengths as pinctrl/dt properties were
> > > > > > > > already added for T210/186/194 where we need and old device tree don't
> > > > > > > > have them but the case where auto cal can fail is very rare.
> > > > > > > > 
> > > > > > > > Otherwise should update driver to allow
> > > > > > > > tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set
> > > > > > > > and also within tegra_sdhci_parse_pad_autocal_dt() show warning of
> > > > > > > > missing 3v3/1v8 settings only when NVQUIRK_NEEDS_PAD_CONTROL is set.
> > > > > > > > 
> > > > > > > > Thierry, please suggest if you prefer to removing warnings or fix driver
> > > > > > > > to show warning based on PADCALIB and PAD_CONTROL quirks.
> > > > > > > The SDIO PINCTRL drive-strengths are usually a part of the board's
> > > > > > > default PINCTRL state, which is either preset by bootloader or by
> > > > > > > PINCTRL driver early at a boot time.
> > > > > > > 
> > > > > > > The SDIO drive-strengths values should be board-specific and not
> > > > > > > SoC-specific because they should depend on the electrical properties of
> > > > > > > the board, IIUC.
> > > > > Drive strengths we program here when auto calibration fails are recommended
> > > > > values based on pre-SI circuit analysis and characterized across PVT.
> > > > > 
> > > > > So,  these fail safe values are same for all boards of specific SoC as all
> > > > > platform designs follow the design guidelines.
> > > > > 
> > > > > > > If the SDIO PINCTRL states are mandatory for the SDHCI nodes in the
> > > > > > > device-trees, then the DT binding is wrong since it says that all
> > > > > > > properties are optional. But I think that the current binding is okay,
> > > > > > > since today SDHCI PINCTRL drive-strengths are specified implicitly in
> > > > > > > the device-trees, and thus, there is no real need to emit the noisy
> > > > > > > warnings in this case.
> > > > > > For now I will keep $subject patch applied, but please tell me if I
> > > > > > should drop it so we can start over.
> > > > > > 
> > > > > > In any case, I would appreciate it if someone could have a stab at
> > > > > > converting sdhci and tegra DT bindings to yaml.
> > > > > > 
> > > > > > Kind regards
> > > > > > Uffe
> > > > > HI Uffe,
> > > > > 
> > > > > Please drop $subject patch. Will send patch that allows parsing for these
> > > > > properties only for SoC where auto cal is enabled as that's where driver
> > > > > needs these properties.
> > > > > 
> > > > > So with this fix, warning will not show up on systems where autocal is not
> > > > > enabled.
> > > > Yes, I think that's a better option. Have we ensured that on all systems
> > > > where autocal is enabled these values are part of the device tree? Just
> > > > making sure that we're not going to have some generation still spit out
> > > > these warnings because we forgot to update the device tree.
> > > > 
> > > > For example I see that we set NVQUIRK_HAS_PADCALIB but I don't see these
> > > > properties being set in arch/arm/boot/dts/tegra30.dtsi. Can you add a
> > > > patch that also adds the properties for Tegra30?
> > > I don't see the warnings on T30 using Sowjanya's patch which checks for
> > > NVQUIRK_NEEDS_PAD_CONTROL and not NVQUIRK_HAS_PADCALIB.
> 
> Both of these quirks are different.
> 
> PADCALIB is for auto calibration support.
> 
> NEEDS_PAD_CONTROL is for SoC having separate 3V3 and 1V8 pads where they
> have pad state selection and also diff drive strengths apply based on 3V3
> and 1V8 which are used only when auto cal is not used/failed.

Great, would you mind sending out a patch that describes their uses
somewhere above their definitions? It'd be good to have this documented
in the code in case this ever comes up again.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Sowjanya Komatineni]

On 5/22/20 8:26 AM, Thierry Reding wrote:
> On Fri, May 22, 2020 at 08:22:47AM -0700, Sowjanya Komatineni wrote:
>> On 5/22/20 5:34 AM, Thierry Reding wrote:
>>> On Fri, May 22, 2020 at 03:18:40PM +0300, Dmitry Osipenko wrote:
>>>> 22.05.2020 15:13, Thierry Reding пишет:
>>>>> On Wed, May 20, 2020 at 09:09:33AM -0700, Sowjanya Komatineni wrote:
>>>>>> On 5/20/20 4:26 AM, Ulf Hansson wrote:
>>>>>>> On Wed, 20 May 2020 at 04:00, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>>>>> 19.05.2020 23:44, Sowjanya Komatineni пишет:
>>>>>>>>> On 5/19/20 12:07 PM, Sowjanya Komatineni wrote:
>>>>>>>>>> On 5/19/20 11:41 AM, Sowjanya Komatineni wrote:
>>>>>>>>>>> On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
>>>>>>>>>>>> On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>> 19.05.2020 19:24, Thierry Reding пишет:
>>>>>>>>>>>>>> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>> 19.05.2020 10:28, Ulf Hansson пишет:
>>>>>>>>>>>>>>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com>
>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>> Several people asked me about the MMC warnings in the KMSG log and
>>>>>>>>>>>>>>>>> I had to tell to ignore them because these warning are
>>>>>>>>>>>>>>>>> irrelevant to
>>>>>>>>>>>>>>>>> pre-Tegra210 SoCs.
>>>>>>>>>>>>>>>> Why are the warnings irrelevant?
>>>>>>>>>>>>>>> That's what the DT binding doc says [1].
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Although, looking at the driver's code and TRM docs, it seems
>>>>>>>>>>>>>>> that all
>>>>>>>>>>>>>>> those properties are really irrelevant only to the older Terga20
>>>>>>>>>>>>>>> SoC. So
>>>>>>>>>>>>>>> the binding doc is a bit misleading.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Nevertheless, the binding explicitly says that the properties are
>>>>>>>>>>>>>>> optional, which is correct.
>>>>>>>>>>>>>> Optional only means that drivers must not fail if these properties
>>>>>>>>>>>>>> aren't found, it doesn't mean that the driver can't warn that they
>>>>>>>>>>>>>> are missing.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Quite possibly the only reason why they were made optional is because
>>>>>>>>>>>>>> they weren't part of the bindings since the beginning. Anything added
>>>>>>>>>>>>>> to a binding after the first public release has to be optional by
>>>>>>>>>>>>>> definition, otherwise DT ABI wouldn't be stable.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I think these warnings were added on purpose, though I also see that
>>>>>>>>>>>>>> there are only values for these in device tree for Tegra186 and
>>>>>>>>>>>>>> Tegra194
>>>>>>>>>>>>>> but not Tegra210 where these should also be necessary.
>>>>>>>>>>>> dt binding doc we have is common for MMC, SD and SDIO of all Tegras.
>>>>>>>>>>>> Its not mandatory to have both 3v3 and 1v8 in device tree as based
>>>>>>>>>>>> on signal mode.
>>>>>>>>>>>>
>>>>>>>>>>>> As these driver strengths are SoC specific, they are part of Tegra
>>>>>>>>>>>> SoC specific device tree where same values will be applicable to all
>>>>>>>>>>>> Tegra SoC specific platforms.
>>>>>>>>>>>>
>>>>>>>>>>>> Based on interface usage on platform, we use one or both of them
>>>>>>>>>>>> like sdcard supports dual voltage and we use both 3V3 and 1V8 but if
>>>>>>>>>>>> same interface is used for WIFI SD we use 1V8 only.
>>>>>>>>>>>>
>>>>>>>>>>>> So made these dt properties as optional.
>>>>>>>>>>>>
>>>>>>>>>>>> Other reason they are optional is, Tegra210 and prior has drive
>>>>>>>>>>>> strength settings part of apb_misc and Tegra186 and later has driver
>>>>>>>>>>>> strengths part of SDMMC controller. So,
>>>>>>>>>>>>
>>>>>>>>>>>> - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths
>>>>>>>>>>>> are applicable for Tegra210 and prior.
>>>>>>>>>>>> - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are
>>>>>>>>>>>> for T186 onwards for driver strengths
>>>>>>>>>>>>
>>>>>>>>>>>> Looks like dt binding doc need fix to clearly document these based
>>>>>>>>>>>> on SoC or agree with Yaml we can conditionally specify pinctrl or dt
>>>>>>>>>>>> properties based on SoC dependent.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>>> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
>>>>>>>>>>>>>> warnings were added. If these values /should/ be there on a subset of
>>>>>>>>>>>>>> Tegra, then I think we should keep them, or add them again, but
>>>>>>>>>>>>>> perhaps
>>>>>>>>>>>>>> add a better way of identifying when they are necessary and when
>>>>>>>>>>>>>> it is
>>>>>>>>>>>>>> safe to work without them.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> That said, looking at those checks I wonder if they are perhaps just
>>>>>>>>>>>>>> wrong. Or at the very least they seem redundant. It looks to me like
>>>>>>>>>>>>>> they can just be:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>        if (tegra_host->pinctrl_state_XYZ == NULL) {
>>>>>>>>>>>>>>            ...
>>>>>>>>>>>>>>        }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
>>>>>>>>>>>>>> also obvious why we're warning about them on platforms where these
>>>>>>>>>>>>>> properties don't exist in DT.
>>>>>>>>>>>> As drive strengths are through dt properties for T186 and later and
>>>>>>>>>>>> thru pinctrl for T210 and prior, driver first checks for dt autocal
>>>>>>>>>>>> timeout pull-up/down properties and if they are not found, it then
>>>>>>>>>>>> checks for presence of pinctrl_state_xyx_drv only when valid
>>>>>>>>>>>> pinctrl_state_xyz is present.
>>>>>>>>>>>>
>>>>>>>>>>>> Driver expects either pinctrl or dt properties and shows warning
>>>>>>>>>>>> when neither of them are present as its mandatory to use fixed
>>>>>>>>>>>> driver strengths when auto calibration fails.
>>>>>>>>>>>>
>>>>>>>>>>>>        err = device_property_read_u32(host->mmc->parent,
>>>>>>>>>>>>                "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
>>>>>>>>>>>>                &autocal->pull_down_3v3_timeout);
>>>>>>>>>>>>        if (err) {
>>>>>>>>>>>>            if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
>>>>>>>>>>>>                (tegra_host->pinctrl_state_3v3_drv == NULL))
>>>>>>>>>>>>                pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
>>>>>>>>>>>>                    mmc_hostname(host->mmc));
>>>>>>>>>>>>            autocal->pull_down_3v3_timeout = 0;
>>>>>>>>>>>>        }
>>>>>>>>>>>>
>>>>>>>>>>>>>> So I think we either need to add those values where appropriate so
>>>>>>>>>>>>>> that
>>>>>>>>>>>>>> the warning doesn't show, or we need to narrow down where they are
>>>>>>>>>>>>>> really needed and add a corresponding condition.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> But again, perhaps Sowjanya can help clarify if these really are only
>>>>>>>>>>>>>> needed on Tegra210 and later or if these also apply to older chips.
>>>>>>>>>>>>> Either way will be cleaner to convert the DT binding to YAML rather
>>>>>>>>>>>>> than
>>>>>>>>>>>>> clutter the driver, IMO.
>>>>>>>>>>>>>
>>>>>>>>>>> Auto calibration is present from Tegra30 onward and looking into
>>>>>>>>>>> change where autocalibration was added to sdhci driver somehow it was
>>>>>>>>>>> enabled only for T30/T210/T186/T194.
>>>>>>>>>>>
>>>>>>>>>>> tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration
>>>>>>>>>>> was added to driver and I see this dt parse is being done
>>>>>>>>>>> irrespective of NVQUIRK_HAS_PADCALIB quirk so even on platforms
>>>>>>>>>>> without auto cal enabled in driver, these messages shows up.
>>>>>>>>>>>
>>>>>>>>>>> This should be fixed in driver to allow
>>>>>>>>>>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is
>>>>>>>>>>> set to avoid dt parsing to happen on platforms that don't have auto
>>>>>>>>>>> cal enabled.
>>>>>>>>>> Warning on missing drive strengths when auto cal is enabled should be
>>>>>>>>>> present as we should switch to fixed recommended drive strengths when
>>>>>>>>>> auto cal fails.
>>>>>>>>>>
>>>>>>>>>> So probably proper fix should be
>>>>>>>>>>
>>>>>>>>>> - allow tegra_sdhci_parse_pad_autocal_dt() only when
>>>>>>>>>> NVQUIRK_HAS_PADCALIB is set
>>>>>>>>>>
>>>>>>>>>> - current driver sets NVQUIRK_HAS_PADCALIB for T30 as well so need to
>>>>>>>>>> add pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" to Tegra30 device tree.
>>>>>>>>> [Correction] T30 has same drive strengths to use irrespective of signal
>>>>>>>>> voltage and it doesn't have pad control. So for T3- we can update device
>>>>>>>>> tree to specify "default" pinctrl with drvup/dn settings.
>>>>>>>>>> - Keep warning message of missing auto cal timeouts as its mandatory
>>>>>>>>>> to use fixed recommended driver strengths when auto cal fails.
>>>>>>>>>>
>>>>>>>>> Regarding warnings, I guess simpler and easy fix is to remove warning
>>>>>>>>> message on missing 3v3/1v8 drive strengths as pinctrl/dt properties were
>>>>>>>>> already added for T210/186/194 where we need and old device tree don't
>>>>>>>>> have them but the case where auto cal can fail is very rare.
>>>>>>>>>
>>>>>>>>> Otherwise should update driver to allow
>>>>>>>>> tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set
>>>>>>>>> and also within tegra_sdhci_parse_pad_autocal_dt() show warning of
>>>>>>>>> missing 3v3/1v8 settings only when NVQUIRK_NEEDS_PAD_CONTROL is set.
>>>>>>>>>
>>>>>>>>> Thierry, please suggest if you prefer to removing warnings or fix driver
>>>>>>>>> to show warning based on PADCALIB and PAD_CONTROL quirks.
>>>>>>>> The SDIO PINCTRL drive-strengths are usually a part of the board's
>>>>>>>> default PINCTRL state, which is either preset by bootloader or by
>>>>>>>> PINCTRL driver early at a boot time.
>>>>>>>>
>>>>>>>> The SDIO drive-strengths values should be board-specific and not
>>>>>>>> SoC-specific because they should depend on the electrical properties of
>>>>>>>> the board, IIUC.
>>>>>> Drive strengths we program here when auto calibration fails are recommended
>>>>>> values based on pre-SI circuit analysis and characterized across PVT.
>>>>>>
>>>>>> So,  these fail safe values are same for all boards of specific SoC as all
>>>>>> platform designs follow the design guidelines.
>>>>>>
>>>>>>>> If the SDIO PINCTRL states are mandatory for the SDHCI nodes in the
>>>>>>>> device-trees, then the DT binding is wrong since it says that all
>>>>>>>> properties are optional. But I think that the current binding is okay,
>>>>>>>> since today SDHCI PINCTRL drive-strengths are specified implicitly in
>>>>>>>> the device-trees, and thus, there is no real need to emit the noisy
>>>>>>>> warnings in this case.
>>>>>>> For now I will keep $subject patch applied, but please tell me if I
>>>>>>> should drop it so we can start over.
>>>>>>>
>>>>>>> In any case, I would appreciate it if someone could have a stab at
>>>>>>> converting sdhci and tegra DT bindings to yaml.
>>>>>>>
>>>>>>> Kind regards
>>>>>>> Uffe
>>>>>> HI Uffe,
>>>>>>
>>>>>> Please drop $subject patch. Will send patch that allows parsing for these
>>>>>> properties only for SoC where auto cal is enabled as that's where driver
>>>>>> needs these properties.
>>>>>>
>>>>>> So with this fix, warning will not show up on systems where autocal is not
>>>>>> enabled.
>>>>> Yes, I think that's a better option. Have we ensured that on all systems
>>>>> where autocal is enabled these values are part of the device tree? Just
>>>>> making sure that we're not going to have some generation still spit out
>>>>> these warnings because we forgot to update the device tree.
>>>>>
>>>>> For example I see that we set NVQUIRK_HAS_PADCALIB but I don't see these
>>>>> properties being set in arch/arm/boot/dts/tegra30.dtsi. Can you add a
>>>>> patch that also adds the properties for Tegra30?
>>>> I don't see the warnings on T30 using Sowjanya's patch which checks for
>>>> NVQUIRK_NEEDS_PAD_CONTROL and not NVQUIRK_HAS_PADCALIB.
>> Both of these quirks are different.
>>
>> PADCALIB is for auto calibration support.
>>
>> NEEDS_PAD_CONTROL is for SoC having separate 3V3 and 1V8 pads where they
>> have pad state selection and also diff drive strengths apply based on 3V3
>> and 1V8 which are used only when auto cal is not used/failed.
> Great, would you mind sending out a patch that describes their uses
> somewhere above their definitions? It'd be good to have this documented
> in the code in case this ever comes up again.
>
> Thierry
OK, Will send

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Ulf Hansson]

On Fri, 22 May 2020 at 14:14, Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> On Wed, May 20, 2020 at 09:09:33AM -0700, Sowjanya Komatineni wrote:
> >
> > On 5/20/20 4:26 AM, Ulf Hansson wrote:
> > > On Wed, 20 May 2020 at 04:00, Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > > 19.05.2020 23:44, Sowjanya Komatineni пишет:
> > > > > On 5/19/20 12:07 PM, Sowjanya Komatineni wrote:
> > > > > > On 5/19/20 11:41 AM, Sowjanya Komatineni wrote:
> > > > > > > On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
> > > > > > > > On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
> > > > > > > > > 19.05.2020 19:24, Thierry Reding пишет:
> > > > > > > > > > On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
> > > > > > > > > > > 19.05.2020 10:28, Ulf Hansson пишет:
> > > > > > > > > > > > On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > Several people asked me about the MMC warnings in the KMSG log and
> > > > > > > > > > > > > I had to tell to ignore them because these warning are
> > > > > > > > > > > > > irrelevant to
> > > > > > > > > > > > > pre-Tegra210 SoCs.
> > > > > > > > > > > > Why are the warnings irrelevant?
> > > > > > > > > > > That's what the DT binding doc says [1].
> > > > > > > > > > >
> > > > > > > > > > > [1]
> > > > > > > > > > > https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Although, looking at the driver's code and TRM docs, it seems
> > > > > > > > > > > that all
> > > > > > > > > > > those properties are really irrelevant only to the older Terga20
> > > > > > > > > > > SoC. So
> > > > > > > > > > > the binding doc is a bit misleading.
> > > > > > > > > > >
> > > > > > > > > > > Nevertheless, the binding explicitly says that the properties are
> > > > > > > > > > > optional, which is correct.
> > > > > > > > > > Optional only means that drivers must not fail if these properties
> > > > > > > > > > aren't found, it doesn't mean that the driver can't warn that they
> > > > > > > > > > are missing.
> > > > > > > > > >
> > > > > > > > > > Quite possibly the only reason why they were made optional is because
> > > > > > > > > > they weren't part of the bindings since the beginning. Anything added
> > > > > > > > > > to a binding after the first public release has to be optional by
> > > > > > > > > > definition, otherwise DT ABI wouldn't be stable.
> > > > > > > > > >
> > > > > > > > > > I think these warnings were added on purpose, though I also see that
> > > > > > > > > > there are only values for these in device tree for Tegra186 and
> > > > > > > > > > Tegra194
> > > > > > > > > > but not Tegra210 where these should also be necessary.
> > > > > > > > dt binding doc we have is common for MMC, SD and SDIO of all Tegras.
> > > > > > > > Its not mandatory to have both 3v3 and 1v8 in device tree as based
> > > > > > > > on signal mode.
> > > > > > > >
> > > > > > > > As these driver strengths are SoC specific, they are part of Tegra
> > > > > > > > SoC specific device tree where same values will be applicable to all
> > > > > > > > Tegra SoC specific platforms.
> > > > > > > >
> > > > > > > > Based on interface usage on platform, we use one or both of them
> > > > > > > > like sdcard supports dual voltage and we use both 3V3 and 1V8 but if
> > > > > > > > same interface is used for WIFI SD we use 1V8 only.
> > > > > > > >
> > > > > > > > So made these dt properties as optional.
> > > > > > > >
> > > > > > > > Other reason they are optional is, Tegra210 and prior has drive
> > > > > > > > strength settings part of apb_misc and Tegra186 and later has driver
> > > > > > > > strengths part of SDMMC controller. So,
> > > > > > > >
> > > > > > > > - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths
> > > > > > > > are applicable for Tegra210 and prior.
> > > > > > > > - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are
> > > > > > > > for T186 onwards for driver strengths
> > > > > > > >
> > > > > > > > Looks like dt binding doc need fix to clearly document these based
> > > > > > > > on SoC or agree with Yaml we can conditionally specify pinctrl or dt
> > > > > > > > properties based on SoC dependent.
> > > > > > > >
> > > > > > > >
> > > > > > > > > > Adding Sowjanya who wrote this code. Perhaps she can clarify why the
> > > > > > > > > > warnings were added. If these values /should/ be there on a subset of
> > > > > > > > > > Tegra, then I think we should keep them, or add them again, but
> > > > > > > > > > perhaps
> > > > > > > > > > add a better way of identifying when they are necessary and when
> > > > > > > > > > it is
> > > > > > > > > > safe to work without them.
> > > > > > > > > >
> > > > > > > > > > That said, looking at those checks I wonder if they are perhaps just
> > > > > > > > > > wrong. Or at the very least they seem redundant. It looks to me like
> > > > > > > > > > they can just be:
> > > > > > > > > >
> > > > > > > > > >      if (tegra_host->pinctrl_state_XYZ == NULL) {
> > > > > > > > > >          ...
> > > > > > > > > >      }
> > > > > > > > > >
> > > > > > > > > > That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
> > > > > > > > > > also obvious why we're warning about them on platforms where these
> > > > > > > > > > properties don't exist in DT.
> > > > > > > > As drive strengths are through dt properties for T186 and later and
> > > > > > > > thru pinctrl for T210 and prior, driver first checks for dt autocal
> > > > > > > > timeout pull-up/down properties and if they are not found, it then
> > > > > > > > checks for presence of pinctrl_state_xyx_drv only when valid
> > > > > > > > pinctrl_state_xyz is present.
> > > > > > > >
> > > > > > > > Driver expects either pinctrl or dt properties and shows warning
> > > > > > > > when neither of them are present as its mandatory to use fixed
> > > > > > > > driver strengths when auto calibration fails.
> > > > > > > >
> > > > > > > >      err = device_property_read_u32(host->mmc->parent,
> > > > > > > >              "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
> > > > > > > >              &autocal->pull_down_3v3_timeout);
> > > > > > > >      if (err) {
> > > > > > > >          if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
> > > > > > > >              (tegra_host->pinctrl_state_3v3_drv == NULL))
> > > > > > > >              pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
> > > > > > > >                  mmc_hostname(host->mmc));
> > > > > > > >          autocal->pull_down_3v3_timeout = 0;
> > > > > > > >      }
> > > > > > > >
> > > > > > > > > > So I think we either need to add those values where appropriate so
> > > > > > > > > > that
> > > > > > > > > > the warning doesn't show, or we need to narrow down where they are
> > > > > > > > > > really needed and add a corresponding condition.
> > > > > > > > > >
> > > > > > > > > > But again, perhaps Sowjanya can help clarify if these really are only
> > > > > > > > > > needed on Tegra210 and later or if these also apply to older chips.
> > > > > > > > > Either way will be cleaner to convert the DT binding to YAML rather
> > > > > > > > > than
> > > > > > > > > clutter the driver, IMO.
> > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > Auto calibration is present from Tegra30 onward and looking into
> > > > > > > change where autocalibration was added to sdhci driver somehow it was
> > > > > > > enabled only for T30/T210/T186/T194.
> > > > > > >
> > > > > > > tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration
> > > > > > > was added to driver and I see this dt parse is being done
> > > > > > > irrespective of NVQUIRK_HAS_PADCALIB quirk so even on platforms
> > > > > > > without auto cal enabled in driver, these messages shows up.
> > > > > > >
> > > > > > > This should be fixed in driver to allow
> > > > > > > tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is
> > > > > > > set to avoid dt parsing to happen on platforms that don't have auto
> > > > > > > cal enabled.
> > > > > > Warning on missing drive strengths when auto cal is enabled should be
> > > > > > present as we should switch to fixed recommended drive strengths when
> > > > > > auto cal fails.
> > > > > >
> > > > > > So probably proper fix should be
> > > > > >
> > > > > > - allow tegra_sdhci_parse_pad_autocal_dt() only when
> > > > > > NVQUIRK_HAS_PADCALIB is set
> > > > > >
> > > > > > - current driver sets NVQUIRK_HAS_PADCALIB for T30 as well so need to
> > > > > > add pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" to Tegra30 device tree.
> > > > > [Correction] T30 has same drive strengths to use irrespective of signal
> > > > > voltage and it doesn't have pad control. So for T3- we can update device
> > > > > tree to specify "default" pinctrl with drvup/dn settings.
> > > > > > - Keep warning message of missing auto cal timeouts as its mandatory
> > > > > > to use fixed recommended driver strengths when auto cal fails.
> > > > > >
> > > > > Regarding warnings, I guess simpler and easy fix is to remove warning
> > > > > message on missing 3v3/1v8 drive strengths as pinctrl/dt properties were
> > > > > already added for T210/186/194 where we need and old device tree don't
> > > > > have them but the case where auto cal can fail is very rare.
> > > > >
> > > > > Otherwise should update driver to allow
> > > > > tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set
> > > > > and also within tegra_sdhci_parse_pad_autocal_dt() show warning of
> > > > > missing 3v3/1v8 settings only when NVQUIRK_NEEDS_PAD_CONTROL is set.
> > > > >
> > > > > Thierry, please suggest if you prefer to removing warnings or fix driver
> > > > > to show warning based on PADCALIB and PAD_CONTROL quirks.
> > > > The SDIO PINCTRL drive-strengths are usually a part of the board's
> > > > default PINCTRL state, which is either preset by bootloader or by
> > > > PINCTRL driver early at a boot time.
> > > >
> > > > The SDIO drive-strengths values should be board-specific and not
> > > > SoC-specific because they should depend on the electrical properties of
> > > > the board, IIUC.
> >
> > Drive strengths we program here when auto calibration fails are recommended
> > values based on pre-SI circuit analysis and characterized across PVT.
> >
> > So,  these fail safe values are same for all boards of specific SoC as all
> > platform designs follow the design guidelines.
> >
> > > > If the SDIO PINCTRL states are mandatory for the SDHCI nodes in the
> > > > device-trees, then the DT binding is wrong since it says that all
> > > > properties are optional. But I think that the current binding is okay,
> > > > since today SDHCI PINCTRL drive-strengths are specified implicitly in
> > > > the device-trees, and thus, there is no real need to emit the noisy
> > > > warnings in this case.
> > > For now I will keep $subject patch applied, but please tell me if I
> > > should drop it so we can start over.
> > >
> > > In any case, I would appreciate it if someone could have a stab at
> > > converting sdhci and tegra DT bindings to yaml.
> > >
> > > Kind regards
> > > Uffe
> >
> > HI Uffe,
> >
> > Please drop $subject patch. Will send patch that allows parsing for these
> > properties only for SoC where auto cal is enabled as that's where driver
> > needs these properties.
> >
> > So with this fix, warning will not show up on systems where autocal is not
> > enabled.
>
> Yes, I think that's a better option. Have we ensured that on all systems
> where autocal is enabled these values are part of the device tree? Just
> making sure that we're not going to have some generation still spit out
> these warnings because we forgot to update the device tree.
>
> For example I see that we set NVQUIRK_HAS_PADCALIB but I don't see these
> properties being set in arch/arm/boot/dts/tegra30.dtsi. Can you add a
> patch that also adds the properties for Tegra30?
>

Patch dropped, thanks!

Kind regards
Uffe

Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties

[Ulf Hansson]

On Fri, 22 May 2020 at 14:14, Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Wed, May 20, 2020 at 09:09:33AM -0700, Sowjanya Komatineni wrote:
> >
> > On 5/20/20 4:26 AM, Ulf Hansson wrote:
> > > On Wed, 20 May 2020 at 04:00, Dmitry Osipenko <digetx@gmail.com> wrote:
> > > > 19.05.2020 23:44, Sowjanya Komatineni пишет:
> > > > > On 5/19/20 12:07 PM, Sowjanya Komatineni wrote:
> > > > > > On 5/19/20 11:41 AM, Sowjanya Komatineni wrote:
> > > > > > > On 5/19/20 11:34 AM, Sowjanya Komatineni wrote:
> > > > > > > > On 5/19/20 9:33 AM, Dmitry Osipenko wrote:
> > > > > > > > > 19.05.2020 19:24, Thierry Reding пишет:
> > > > > > > > > > On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
> > > > > > > > > > > 19.05.2020 10:28, Ulf Hansson пишет:
> > > > > > > > > > > > On Sat, 16 May 2020 at 17:44, Dmitry Osipenko <digetx@gmail.com>
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > Several people asked me about the MMC warnings in the KMSG log and
> > > > > > > > > > > > > I had to tell to ignore them because these warning are
> > > > > > > > > > > > > irrelevant to
> > > > > > > > > > > > > pre-Tegra210 SoCs.
> > > > > > > > > > > > Why are the warnings irrelevant?
> > > > > > > > > > > That's what the DT binding doc says [1].
> > > > > > > > > > >
> > > > > > > > > > > [1]
> > > > > > > > > > > https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Although, looking at the driver's code and TRM docs, it seems
> > > > > > > > > > > that all
> > > > > > > > > > > those properties are really irrelevant only to the older Terga20
> > > > > > > > > > > SoC. So
> > > > > > > > > > > the binding doc is a bit misleading.
> > > > > > > > > > >
> > > > > > > > > > > Nevertheless, the binding explicitly says that the properties are
> > > > > > > > > > > optional, which is correct.
> > > > > > > > > > Optional only means that drivers must not fail if these properties
> > > > > > > > > > aren't found, it doesn't mean that the driver can't warn that they
> > > > > > > > > > are missing.
> > > > > > > > > >
> > > > > > > > > > Quite possibly the only reason why they were made optional is because
> > > > > > > > > > they weren't part of the bindings since the beginning. Anything added
> > > > > > > > > > to a binding after the first public release has to be optional by
> > > > > > > > > > definition, otherwise DT ABI wouldn't be stable.
> > > > > > > > > >
> > > > > > > > > > I think these warnings were added on purpose, though I also see that
> > > > > > > > > > there are only values for these in device tree for Tegra186 and
> > > > > > > > > > Tegra194
> > > > > > > > > > but not Tegra210 where these should also be necessary.
> > > > > > > > dt binding doc we have is common for MMC, SD and SDIO of all Tegras.
> > > > > > > > Its not mandatory to have both 3v3 and 1v8 in device tree as based
> > > > > > > > on signal mode.
> > > > > > > >
> > > > > > > > As these driver strengths are SoC specific, they are part of Tegra
> > > > > > > > SoC specific device tree where same values will be applicable to all
> > > > > > > > Tegra SoC specific platforms.
> > > > > > > >
> > > > > > > > Based on interface usage on platform, we use one or both of them
> > > > > > > > like sdcard supports dual voltage and we use both 3V3 and 1V8 but if
> > > > > > > > same interface is used for WIFI SD we use 1V8 only.
> > > > > > > >
> > > > > > > > So made these dt properties as optional.
> > > > > > > >
> > > > > > > > Other reason they are optional is, Tegra210 and prior has drive
> > > > > > > > strength settings part of apb_misc and Tegra186 and later has driver
> > > > > > > > strengths part of SDMMC controller. So,
> > > > > > > >
> > > > > > > > - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths
> > > > > > > > are applicable for Tegra210 and prior.
> > > > > > > > - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are
> > > > > > > > for T186 onwards for driver strengths
> > > > > > > >
> > > > > > > > Looks like dt binding doc need fix to clearly document these based
> > > > > > > > on SoC or agree with Yaml we can conditionally specify pinctrl or dt
> > > > > > > > properties based on SoC dependent.
> > > > > > > >
> > > > > > > >
> > > > > > > > > > Adding Sowjanya who wrote this code. Perhaps she can clarify why the
> > > > > > > > > > warnings were added. If these values /should/ be there on a subset of
> > > > > > > > > > Tegra, then I think we should keep them, or add them again, but
> > > > > > > > > > perhaps
> > > > > > > > > > add a better way of identifying when they are necessary and when
> > > > > > > > > > it is
> > > > > > > > > > safe to work without them.
> > > > > > > > > >
> > > > > > > > > > That said, looking at those checks I wonder if they are perhaps just
> > > > > > > > > > wrong. Or at the very least they seem redundant. It looks to me like
> > > > > > > > > > they can just be:
> > > > > > > > > >
> > > > > > > > > >      if (tegra_host->pinctrl_state_XYZ == NULL) {
> > > > > > > > > >          ...
> > > > > > > > > >      }
> > > > > > > > > >
> > > > > > > > > > That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
> > > > > > > > > > also obvious why we're warning about them on platforms where these
> > > > > > > > > > properties don't exist in DT.
> > > > > > > > As drive strengths are through dt properties for T186 and later and
> > > > > > > > thru pinctrl for T210 and prior, driver first checks for dt autocal
> > > > > > > > timeout pull-up/down properties and if they are not found, it then
> > > > > > > > checks for presence of pinctrl_state_xyx_drv only when valid
> > > > > > > > pinctrl_state_xyz is present.
> > > > > > > >
> > > > > > > > Driver expects either pinctrl or dt properties and shows warning
> > > > > > > > when neither of them are present as its mandatory to use fixed
> > > > > > > > driver strengths when auto calibration fails.
> > > > > > > >
> > > > > > > >      err = device_property_read_u32(host->mmc->parent,
> > > > > > > >              "nvidia,pad-autocal-pull-down-offset-3v3-timeout",
> > > > > > > >              &autocal->pull_down_3v3_timeout);
> > > > > > > >      if (err) {
> > > > > > > >          if (!IS_ERR(tegra_host->pinctrl_state_3v3) &&
> > > > > > > >              (tegra_host->pinctrl_state_3v3_drv == NULL))
> > > > > > > >              pr_warn("%s: Missing autocal timeout 3v3-pad drvs\n",
> > > > > > > >                  mmc_hostname(host->mmc));
> > > > > > > >          autocal->pull_down_3v3_timeout = 0;
> > > > > > > >      }
> > > > > > > >
> > > > > > > > > > So I think we either need to add those values where appropriate so
> > > > > > > > > > that
> > > > > > > > > > the warning doesn't show, or we need to narrow down where they are
> > > > > > > > > > really needed and add a corresponding condition.
> > > > > > > > > >
> > > > > > > > > > But again, perhaps Sowjanya can help clarify if these really are only
> > > > > > > > > > needed on Tegra210 and later or if these also apply to older chips.
> > > > > > > > > Either way will be cleaner to convert the DT binding to YAML rather
> > > > > > > > > than
> > > > > > > > > clutter the driver, IMO.
> > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > Auto calibration is present from Tegra30 onward and looking into
> > > > > > > change where autocalibration was added to sdhci driver somehow it was
> > > > > > > enabled only for T30/T210/T186/T194.
> > > > > > >
> > > > > > > tegra_sdhci_parse_pad_autocal_dt() was added when auto-calibration
> > > > > > > was added to driver and I see this dt parse is being done
> > > > > > > irrespective of NVQUIRK_HAS_PADCALIB quirk so even on platforms
> > > > > > > without auto cal enabled in driver, these messages shows up.
> > > > > > >
> > > > > > > This should be fixed in driver to allow
> > > > > > > tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is
> > > > > > > set to avoid dt parsing to happen on platforms that don't have auto
> > > > > > > cal enabled.
> > > > > > Warning on missing drive strengths when auto cal is enabled should be
> > > > > > present as we should switch to fixed recommended drive strengths when
> > > > > > auto cal fails.
> > > > > >
> > > > > > So probably proper fix should be
> > > > > >
> > > > > > - allow tegra_sdhci_parse_pad_autocal_dt() only when
> > > > > > NVQUIRK_HAS_PADCALIB is set
> > > > > >
> > > > > > - current driver sets NVQUIRK_HAS_PADCALIB for T30 as well so need to
> > > > > > add pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" to Tegra30 device tree.
> > > > > [Correction] T30 has same drive strengths to use irrespective of signal
> > > > > voltage and it doesn't have pad control. So for T3- we can update device
> > > > > tree to specify "default" pinctrl with drvup/dn settings.
> > > > > > - Keep warning message of missing auto cal timeouts as its mandatory
> > > > > > to use fixed recommended driver strengths when auto cal fails.
> > > > > >
> > > > > Regarding warnings, I guess simpler and easy fix is to remove warning
> > > > > message on missing 3v3/1v8 drive strengths as pinctrl/dt properties were
> > > > > already added for T210/186/194 where we need and old device tree don't
> > > > > have them but the case where auto cal can fail is very rare.
> > > > >
> > > > > Otherwise should update driver to allow
> > > > > tegra_sdhci_parse_pad_autocal_dt() only when NVQUIRK_HAS_PADCALIB is set
> > > > > and also within tegra_sdhci_parse_pad_autocal_dt() show warning of
> > > > > missing 3v3/1v8 settings only when NVQUIRK_NEEDS_PAD_CONTROL is set.
> > > > >
> > > > > Thierry, please suggest if you prefer to removing warnings or fix driver
> > > > > to show warning based on PADCALIB and PAD_CONTROL quirks.
> > > > The SDIO PINCTRL drive-strengths are usually a part of the board's
> > > > default PINCTRL state, which is either preset by bootloader or by
> > > > PINCTRL driver early at a boot time.
> > > >
> > > > The SDIO drive-strengths values should be board-specific and not
> > > > SoC-specific because they should depend on the electrical properties of
> > > > the board, IIUC.
> >
> > Drive strengths we program here when auto calibration fails are recommended
> > values based on pre-SI circuit analysis and characterized across PVT.
> >
> > So,  these fail safe values are same for all boards of specific SoC as all
> > platform designs follow the design guidelines.
> >
> > > > If the SDIO PINCTRL states are mandatory for the SDHCI nodes in the
> > > > device-trees, then the DT binding is wrong since it says that all
> > > > properties are optional. But I think that the current binding is okay,
> > > > since today SDHCI PINCTRL drive-strengths are specified implicitly in
> > > > the device-trees, and thus, there is no real need to emit the noisy
> > > > warnings in this case.
> > > For now I will keep $subject patch applied, but please tell me if I
> > > should drop it so we can start over.
> > >
> > > In any case, I would appreciate it if someone could have a stab at
> > > converting sdhci and tegra DT bindings to yaml.
> > >
> > > Kind regards
> > > Uffe
> >
> > HI Uffe,
> >
> > Please drop $subject patch. Will send patch that allows parsing for these
> > properties only for SoC where auto cal is enabled as that's where driver
> > needs these properties.
> >
> > So with this fix, warning will not show up on systems where autocal is not
> > enabled.
>
> Yes, I think that's a better option. Have we ensured that on all systems
> where autocal is enabled these values are part of the device tree? Just
> making sure that we're not going to have some generation still spit out
> these warnings because we forgot to update the device tree.
>
> For example I see that we set NVQUIRK_HAS_PADCALIB but I don't see these
> properties being set in arch/arm/boot/dts/tegra30.dtsi. Can you add a
> patch that also adds the properties for Tegra30?
>

Patch dropped, thanks!

Kind regards
Uffe