Re: Initial signal voltage ?

[Joakim Tjernlund <joakim.tjernlund@transmode.se>]

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