Initial signal voltage ?

in mmc_power_up() we have:

        /* Try to set signal voltage to 3.3V but fall back to 1.8v or 1.2v */
        if (__mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_330) == 0)
                dev_err(mmc_dev(host), "Initial signal voltage of 3.3v\n");
        else if (__mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180) == 0)
                dev_err(mmc_dev(host), "Initial signal voltage of 1.8v\n");
        else if (__mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120) == 0)
                dev_err(mmc_dev(host), "Initial signal voltage of 1.2v\n");

Here one jut brutally tries to set the initial power to 3.3 but nowhere in general code there is a check if the controller can handle this w.r.t HOST CAPS.

Where should this check be performed, I am guessing either here or in sdhci? This is all new to me so I am just throwing this out there

Jocke--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines · Joakim Tjernlund

in mmc_power_up() we have:

        /* Try to set signal voltage to 3.3V but fall back to 1.8v or 1.2v */
        if (__mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_330) == 0)
                dev_err(mmc_dev(host), "Initial signal voltage of 3.3v\n");
        else if (__mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180) == 0)
                dev_err(mmc_dev(host), "Initial signal voltage of 1.8v\n");
        else if (__mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120) == 0)
                dev_err(mmc_dev(host), "Initial signal voltage of 1.2v\n");

Quoted 3 lines

Here one jut brutally tries to set the initial power to 3.3 but nowhere in general code there is a check if the controller can handle this w.r.t HOST CAPS.

You are right, it's brutal. :-)

Perhaps the host driver should return -EINVAL when a voltage level can't be reached. The above code should not print messages as errors, at least as long some voltage level can be used.

Quoted 2 lines

Where should this check be performed, I am guessing either here or in sdhci? This is all new to me so I am just throwing this out there

Do you want to send a patch?

Quoted 21 lines · Ulf Hansson

Quoted 9 lines · Joakim Tjernlund

in mmc_power_up() we have:

        /* Try to set signal voltage to 3.3V but fall back to 1.8v or 1.2v */
        if (__mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_330) == 0)
                dev_err(mmc_dev(host), "Initial signal voltage of 3.3v\n");
        else if (__mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180) == 0)
                dev_err(mmc_dev(host), "Initial signal voltage of 1.8v\n");
        else if (__mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120) == 0)
                dev_err(mmc_dev(host), "Initial signal voltage of 1.2v\n");

Quoted 3 lines

Here one jut brutally tries to set the initial power to 3.3 but nowhere in general code there is a check if the controller can handle this w.r.t HOST CAPS.

You are right, it's brutal. :-)

Perhaps the host driver should return -EINVAL when a voltage level can't be reached. The above code should not print messages as errors, at least as long some voltage level can be used.

Quoted 4 lines

Quoted 2 lines

Where should this check be performed, I am guessing either here or in sdhci? This is all new to me so I am just throwing this out there

Do you want to send a patch?

No, you didn't answer the question

Quoted 25 lines · Joakim Tjernlund

Quoted 24 lines · Ulf Hansson

Quoted 9 lines · Joakim Tjernlund

in mmc_power_up() we have:

        /* Try to set signal voltage to 3.3V but fall back to 1.8v or 1.2v */
        if (__mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_330) == 0)
                dev_err(mmc_dev(host), "Initial signal voltage of 3.3v\n");
        else if (__mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180) == 0)
                dev_err(mmc_dev(host), "Initial signal voltage of 1.8v\n");
        else if (__mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120) == 0)
                dev_err(mmc_dev(host), "Initial signal voltage of 1.2v\n");

Quoted 3 lines

Here one jut brutally tries to set the initial power to 3.3 but nowhere in general code there is a check if the controller can handle this w.r.t HOST CAPS.

You are right, it's brutal. :-)

Perhaps the host driver should return -EINVAL when a voltage level can't be reached. The above code should not print messages as errors, at least as long some voltage level can be used.

Quoted 2 lines

Where should this check be performed, I am guessing either here or in sdhci? This is all new to me so I am just throwing this out there

Quoted 3 lines

Quoted 1 line

Do you want to send a patch?

No, you didn't answer the question

Sorry for being a bit vague. I looked a bit more into this.

Currently drivers return error codes according to when they can't set the requested voltage level, so that's all fine - right?

Moreover, the above code snippet is modified by you (or someone else) locally, since in the upstream version of the code, the prints are

done a debug level. That's also okay, I think.
Perhaps changing the last print to warn level instead, since reaching that point would mean all attempts have failed.

Quoted 3 lines

Currently drivers return error codes according to when they can't set the requested voltage level, so that's all fine - right?

Yes, they should. Hovever it seems like my fsl controller cannot. There is VS18,VS30 and VS33 bits in hostcap register but theses are always set :(

Quoted 4 lines

Moreover, the above code snippet is modified by you (or someone else) locally, since in the upstream version of the code, the prints are

ahh, yes a did that while debugging.

Quoted 3 lines

done a debug level. That's also okay, I think.
Perhaps changing the last print to warn level instead, since reaching that point would mean all attempts have failed.

Since not all controllers can tell what VS is you have to specify that, in my case I do it in my device tree as there is a way to do that already.

So I think mmc_power_up() should mask away VS not supported and then try the remaining. The exact way to do this escapes me as I am not familiar with this area yet.

[...]

Quoted 4 lines

Quoted 3 lines

Currently drivers return error codes according to when they can't set the requested voltage level, so that's all fine - right?

Quoted 3 lines

Yes, they should. Hovever it seems like my fsl controller cannot. There is VS18,VS30 and VS33 bits in hostcap register but theses are always set :(

If there are set in the *hostcap* register, doesn't that indicates that they are supported? Not what's currently used!?

What controller/driver are you using here?

Quoted 9 lines

Quoted 2 lines

Moreover, the above code snippet is modified by you (or someone else) locally, since in the upstream version of the code, the prints are

ahh, yes a did that while debugging.

Quoted 3 lines

done a debug level. That's also okay, I think.
Perhaps changing the last print to warn level instead, since reaching that point would mean all attempts have failed.

Quoted 5 lines

Since not all controllers can tell what VS is you have to specify that, in my case I do it in my device tree as there is a way to do that already.

So I think mmc_power_up() should mask away VS not supported and then try the remaining. The exact way to do this escapes me as I am not familiar with this area yet.

Okay, I think we could potentially look at the host caps and figure out what IO voltage level the host supports before trying to set them.

Although it does complicates things, so before going there I just want to be sure that we really have an issue here.

I haven't yet really understood why your driver is unable to return error codes when failing to set an unsupported voltage level, you need to convince me on that.

None the less, the info is there in the host caps so why not use it? If caps say 1.8V only why insist on trying 3/3.3 Volt?