Initial signal voltage ?

Initial signal voltage ?

[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");

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

Re: Initial signal voltage ?

[Ulf Hansson]

On 10 August 2015 at 18:50, Joakim Tjernlund
<joakim.tjernlund@transmode.se> wrote:
> 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.

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.

> 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?

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

Kind regards
Uffe

Re: Initial signal voltage ?

[Joakim Tjernlund]

On Tue, 2015-08-25 at 14:05 +0200, Ulf Hansson wrote:
> On 10 August 2015 at 18:50, Joakim Tjernlund
> <joakim.tjernlund@transmode.se> wrote:
> > 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.
> 
> 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.
> 
> > 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

Re: Initial signal voltage ?

[Ulf Hansson]

On 25 August 2015 at 14:20, Joakim Tjernlund
<joakim.tjernlund@transmode.se> wrote:
> On Tue, 2015-08-25 at 14:05 +0200, Ulf Hansson wrote:
>> On 10 August 2015 at 18:50, Joakim Tjernlund
>> <joakim.tjernlund@transmode.se> wrote:
>> > 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.
>>
>> 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.
>>
>> > 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

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.

Kind regards
Uffe

Re: Initial signal voltage ?

[Joakim Tjernlund]

On Thu, 2015-08-27 at 16:01 +0200, Ulf Hansson wrote:
> On 25 August 2015 at 14:20, Joakim Tjernlund
> <joakim.tjernlund@transmode.se> wrote:
> > On Tue, 2015-08-25 at 14:05 +0200, Ulf Hansson wrote:
> > > On 10 August 2015 at 18:50, Joakim Tjernlund
> > > <joakim.tjernlund@transmode.se> wrote:
> > > > 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.
> > > 
> > > 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.
> > > 
> > > > 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
> 
> 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?

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 :(

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

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

 Jocke

Re: Initial signal voltage ?

[Ulf Hansson]

[...]

>>
>> 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 :(

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?

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

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.

Kind regards
Uffe

Re: Initial signal voltage ?

[Joakim Tjernlund]

On Thu, 2015-08-27 at 16:44 +0200, Ulf Hansson wrote:
> [...]
> 
> > > 
> > > 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 :(
> 
> If there are set in the *hostcap* register, doesn't that indicates
> that they are supported? Not what's currently used!?

hmm, this only shows that I am a newbie in this area :)
What register/bits normally holds this info?
> 
> What controller/driver are you using here?

esdhc,v3.0/sdhci-of-esdhc


> > > 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.
> > 
> > > 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.
> 
> 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?

 Jocke

Re: Initial signal voltage ?

[Fabio Estevam]

On Thu, Aug 27, 2015 at 2:16 PM, Joakim Tjernlund
<joakim.tjernlund@transmode.se> wrote:

>
> 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?

Yes, it seems this is similar to the issue I tried to solve before:
http://www.spinics.net/lists/linux-mmc/msg32285.html