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

From: Sowjanya Komatineni
Date: Tue May 19 2020 - 14:34:20 EST



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