Re: [PATCH v4 1/4] mmc: dw_mmc: Don't try to enable the CD until we're sure we're not deferring

Re: [PATCH v4 1/4] mmc: dw_mmc: Don't try to enable the CD until we're sure we're not deferring

[Jaehoon Chung]

Hi, Heiko.


On 03/13/2015 09:10 PM, Heiko Stuebner wrote:
> Hi,
> 
> Am Freitag, 13. März 2015, 20:32:43 schrieb Jaehoon Chung:
>> Hi Doug,
>>
>> Will apply. Thanks!
> 
> just to make sure, you'll take patches 1-3 and I'll take the dts change from 
> patch 4, right?

Right. I will check on today and request pull to ulf on tomorrow.
Will CC'd your email when PR.

Best Regards,
Jaehoon Chung

> 
> 
> Thanks
> Heiko
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()

[Ulf Hansson]

On 11 March 2015 at 23:15, Doug Anderson <dianders@chromium.org> wrote:
> This adds logic to the MMC core to set VQMMC.  This is expected to be
> called by MMC drivers like dw_mmc as part of (or instead of) their
> start_signal_voltage_switch() callback.
>
> A few notes:
>
> * When setting the signal voltage to 3.3V we do our best to make VQMMC
>   and VMMC match.  It's been reported that this makes some old cards
>   happy since they were tested back in the day before UHS when VQMMC
>   and VMMC were provided by the same regulator.  A nice side effect of
>   this is that we don't end up on the hairy edge of VQMMC (2.7V),
>   which some EEs claim is a little too close to the minimum for
>   comfort.
>
> * When setting the signal voltage to 1.8V or 1.2V we aim for that
>   specific voltage instead of picking the lowest one in the range.
>
> * We very purposely don't print errors in mmc_regulator_set_vqmmc().
>   There are cases where the MMC core will try several different
>   voltages and we don't want to pollute the logs.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Reviewed-by: Andrew Bresticker <abrestic@chromium.org>
> ---
> Changes in v4: None
> Changes in v3: None
> Changes in v2:
> - Now use existing regulator_set_voltage_tol() function.
>
>  drivers/mmc/core/core.c  | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mmc/host.h |  7 +++++++
>  2 files changed, 58 insertions(+)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 23f10f7..1d3b84e 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1394,6 +1394,57 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc,
>  }
>  EXPORT_SYMBOL_GPL(mmc_regulator_set_ocr);
>
> +static int mmc_regulator_set_voltage_if_supported(struct regulator *regulator,
> +                                                 int new_uV, int tol_uV)
> +{
> +       /*
> +        * Check if supported first to avoid errors since we may try several
> +        * signal levels during power up and don't want to show errors.
> +        */
> +       if (!regulator_is_supported_voltage_tol(regulator, new_uV, tol_uV))
> +               return -EINVAL;
> +
> +       return regulator_set_voltage_tol(regulator, new_uV, tol_uV);
> +}
> +
> +/**
> + * mmc_regulator_set_vqmmc - Set VQMMC as per the ios
> + *
> + * For 3.3V signaling, we try to match VQMMC to VMMC as closely as possible.
> + * That will match the behavior of old boards where VQMMC and VMMC were supplied
> + * by the same supply.  The Bus Operating conditions for 3.3V signaling in the
> + * SD card spec also define VQMMC in terms of VMMC.
> + *
> + * For 1.2V and 1.8V signaling we'll try to get as close as possible to the
> + * requested voltage.  This is definitely a good idea for UHS where there's a
> + * separate regulator on the card that's trying to make 1.8V and it's best if
> + * we match.
> + *
> + * This function is expected to be used by a controller's
> + * start_signal_voltage_switch() function.
> + */
> +int mmc_regulator_set_vqmmc(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> +       /* If no vqmmc supply then we can't change the voltage */
> +       if (IS_ERR(mmc->supply.vqmmc))
> +               return -EINVAL;
> +
> +       switch (ios->signal_voltage) {
> +       case MMC_SIGNAL_VOLTAGE_120:
> +               return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
> +                       1200000, 100000);

Is 1V the lowest possible value? How did you get to that?

> +       case MMC_SIGNAL_VOLTAGE_180:
> +               return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
> +                       1800000, 100000);

Is 1V the lowest possible value? How did you get to that?

> +       case MMC_SIGNAL_VOLTAGE_330:
> +               return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
> +                       regulator_get_voltage(mmc->supply.vmmc), 300000);

Why 3V? Shouldn't it be 2.7V? How will else those SoC that for example
supports 2.9V only work?

> +       default:
> +               return -EINVAL;
> +       }
> +}
> +EXPORT_SYMBOL_GPL(mmc_regulator_set_vqmmc);
> +
>  #endif /* CONFIG_REGULATOR */
>
>  int mmc_regulator_get_supply(struct mmc_host *mmc)
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 0c8cbe5..edd7d59 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -416,6 +416,7 @@ int mmc_regulator_get_ocrmask(struct regulator *supply);
>  int mmc_regulator_set_ocr(struct mmc_host *mmc,
>                         struct regulator *supply,
>                         unsigned short vdd_bit);
> +int mmc_regulator_set_vqmmc(struct mmc_host *mmc, struct mmc_ios *ios);
>  #else
>  static inline int mmc_regulator_get_ocrmask(struct regulator *supply)
>  {
> @@ -428,6 +429,12 @@ static inline int mmc_regulator_set_ocr(struct mmc_host *mmc,
>  {
>         return 0;
>  }
> +
> +static inline int mmc_regulator_set_vqmmc(struct mmc_host *mmc,
> +                                         struct mmc_ios *ios)
> +{
> +       return -EINVAL;
> +}
>  #endif
>
>  int mmc_regulator_get_supply(struct mmc_host *mmc);

One more thought,s as for the vmmc regulator we have a
"regulator_enabled" member in the mmc_host. Should we add a similar
member for vqmmc? That would prevent host drivers from keeping track
of this state themselves.

Kind regards
Uffe

> --
> 2.2.0.rc0.207.ga3a616c
>
> --
> 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

Re: [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()

[Doug Anderson]

Ulf,

On Mon, Mar 16, 2015 at 7:05 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> +       switch (ios->signal_voltage) {
>> +       case MMC_SIGNAL_VOLTAGE_120:
>> +               return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
>> +                       1200000, 100000);
>
> Is 1V the lowest possible value? How did you get to that?

I think you've added a zero in your mind and not realized that I'm
calling regulator_set_voltage_tol() here and in other calls.  Please
read the above as:

* Try to set the voltage to exactly 1,200,000 uV (1.2V).
* If you can't get 1.2V exactly, a tolerance ("tol") of 100,000 uV
(.1V) is OK.
* In other words, 1.1V - 1.3V are OK, but aim for 1.2V


>> +       case MMC_SIGNAL_VOLTAGE_180:
>> +               return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
>> +                       1800000, 100000);
>
> Is 1V the lowest possible value? How did you get to that?

Again, check my zeros.  This should be 1.7 - 1.9V, aiming for 1.8V.


>> +       case MMC_SIGNAL_VOLTAGE_330:
>> +               return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
>> +                       regulator_get_voltage(mmc->supply.vmmc), 300000);
>
> Why 3V? Shouldn't it be 2.7V? How will else those SoC that for example
> supports 2.9V only work?

This will get us within .3V of whatever vmmc is.  If vmmc is 3.3V, it
will allow vqmmc of 3.0V - 3.6V.

This _seems_ sane to me and given any sane system design we should be
fine here, I think.  I can't see someone designing a system where
vqmmc was not within .3V of vmmc, can you?  If we think someone will
actually build a system where vmmc is 3.3V and vqmmc can't go higher
than 2.7V then we'll either need to increase the tolerance here or add
a new asymmetric system call like my original patches did.

>>  int mmc_regulator_get_supply(struct mmc_host *mmc);
>
> One more thought,s as for the vmmc regulator we have a
> "regulator_enabled" member in the mmc_host. Should we add a similar
> member for vqmmc? That would prevent host drivers from keeping track
> of this state themselves.

Yeah, that does sound nice.  Are you suggesting that I modify this
patch or submit a new one.  Let me know.


-Doug

Re: [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()

[Ulf Hansson]

On 16 March 2015 at 16:12, Doug Anderson <dianders@chromium.org> wrote:
> Ulf,
>
> On Mon, Mar 16, 2015 at 7:05 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> +       switch (ios->signal_voltage) {
>>> +       case MMC_SIGNAL_VOLTAGE_120:
>>> +               return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
>>> +                       1200000, 100000);
>>
>> Is 1V the lowest possible value? How did you get to that?
>
> I think you've added a zero in your mind and not realized that I'm
> calling regulator_set_voltage_tol() here and in other calls.  Please
> read the above as:

Hehe, you are absolutely right.

>
> * Try to set the voltage to exactly 1,200,000 uV (1.2V).
> * If you can't get 1.2V exactly, a tolerance ("tol") of 100,000 uV
> (.1V) is OK.
> * In other words, 1.1V - 1.3V are OK, but aim for 1.2V

So what happens in the case when 1.3V and 1.1V, but not 1.2V. Which
value will be used? Is that algorithm defined by the regulator core or
does it depend per regulator implementation?

>
>
>>> +       case MMC_SIGNAL_VOLTAGE_180:
>>> +               return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
>>> +                       1800000, 100000);
>>
>> Is 1V the lowest possible value? How did you get to that?
>
> Again, check my zeros.  This should be 1.7 - 1.9V, aiming for 1.8V.
>
>
>>> +       case MMC_SIGNAL_VOLTAGE_330:
>>> +               return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
>>> +                       regulator_get_voltage(mmc->supply.vmmc), 300000);
>>
>> Why 3V? Shouldn't it be 2.7V? How will else those SoC that for example
>> supports 2.9V only work?
>
> This will get us within .3V of whatever vmmc is.  If vmmc is 3.3V, it
> will allow vqmmc of 3.0V - 3.6V.
>
> This _seems_ sane to me and given any sane system design we should be
> fine here, I think.  I can't see someone designing a system where
> vqmmc was not within .3V of vmmc, can you?  If we think someone will
> actually build a system where vmmc is 3.3V and vqmmc can't go higher
> than 2.7V then we'll either need to increase the tolerance here or add
> a new asymmetric system call like my original patches did.

I know about SoC that supports 3.4V vmmc and 2.9V vqmmc.

What I think we need is the option to have a policy here. We need to
allow voltage levels stated by the spec and at the same time try chose
the one best suited. That's not being accomplished here.

Moreover, I wonder whether it's okay (from spec perspective) to have
vqmmc at a higher voltage level than vmmc. I don't think that's
allowed, but I might be wrong.

>
>>>  int mmc_regulator_get_supply(struct mmc_host *mmc);
>>
>> One more thought,s as for the vmmc regulator we have a
>> "regulator_enabled" member in the mmc_host. Should we add a similar
>> member for vqmmc? That would prevent host drivers from keeping track
>> of this state themselves.
>
> Yeah, that does sound nice.  Are you suggesting that I modify this
> patch or submit a new one.  Let me know.

Yes, please add the option as well. It's seems like it will belongs to
this code.

>
>
> -Doug

Kind regards
Uffe

Re: [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()

[Mark Brown]

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

On Tue, Mar 17, 2015 at 11:23:33AM +0100, Ulf Hansson wrote:
> On 16 March 2015 at 16:12, Doug Anderson <dianders@chromium.org> wrote:

> > * Try to set the voltage to exactly 1,200,000 uV (1.2V).
> > * If you can't get 1.2V exactly, a tolerance ("tol") of 100,000 uV
> > (.1V) is OK.
> > * In other words, 1.1V - 1.3V are OK, but aim for 1.2V

> So what happens in the case when 1.3V and 1.1V, but not 1.2V. Which
> value will be used? Is that algorithm defined by the regulator core or
> does it depend per regulator implementation?

It's done in the core.  It first tries to hit the target voltage to the
maximum (picking the lowest voltage in that range) then tries to pick
the lowest voltage to the target, though that's an implementation detail
and we really should be trying to get as close as possible to the
target.  We don't do that yet because it can be expensive to work out so
we do the current thing which is cheap and mostly good enough.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()

[Ulf Hansson]

On 17 March 2015 at 11:38, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Mar 17, 2015 at 11:23:33AM +0100, Ulf Hansson wrote:
>> On 16 March 2015 at 16:12, Doug Anderson <dianders@chromium.org> wrote:
>
>> > * Try to set the voltage to exactly 1,200,000 uV (1.2V).
>> > * If you can't get 1.2V exactly, a tolerance ("tol") of 100,000 uV
>> > (.1V) is OK.
>> > * In other words, 1.1V - 1.3V are OK, but aim for 1.2V
>
>> So what happens in the case when 1.3V and 1.1V, but not 1.2V. Which
>> value will be used? Is that algorithm defined by the regulator core or
>> does it depend per regulator implementation?
>
> It's done in the core.  It first tries to hit the target voltage to the
> maximum (picking the lowest voltage in that range) then tries to pick
> the lowest voltage to the target, though that's an implementation detail
> and we really should be trying to get as close as possible to the
> target.  We don't do that yet because it can be expensive to work out so
> we do the current thing which is cheap and mostly good enough.

Okay, so that seems to work well for our 1.1V->1.3V case.

Thanks!

Kind regards
Uffe

Re: [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()

[Doug Anderson]

Ulf,

On Tue, Mar 17, 2015 at 3:23 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> This will get us within .3V of whatever vmmc is.  If vmmc is 3.3V, it
>> will allow vqmmc of 3.0V - 3.6V.
>>
>> This _seems_ sane to me and given any sane system design we should be
>> fine here, I think.  I can't see someone designing a system where
>> vqmmc was not within .3V of vmmc, can you?  If we think someone will
>> actually build a system where vmmc is 3.3V and vqmmc can't go higher
>> than 2.7V then we'll either need to increase the tolerance here or add
>> a new asymmetric system call like my original patches did.
>
> I know about SoC that supports 3.4V vmmc and 2.9V vqmmc.
>
> What I think we need is the option to have a policy here. We need to
> allow voltage levels stated by the spec and at the same time try chose
> the one best suited. That's not being accomplished here.
>
> Moreover, I wonder whether it's okay (from spec perspective) to have
> vqmmc at a higher voltage level than vmmc. I don't think that's
> allowed, but I might be wrong.

OK, so sounds like I need to add a regulator_set_voltage_tol2()
function that takes in an upper tolerance and a lower.  We can use the
same rough implementation in the core we have today (if Mark is OK
with that) with regulator_set_voltage_tol() but just allow it to be
asymmetric.

From what I see in the spec for 3.3V cards are supposed to react to a
high signal that is .625 * VDD - VDD + .3


I might not be able to get to this till next week, though...

-Doug

Re: [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()

[Ulf Hansson]

On 19 March 2015 at 05:09, Doug Anderson <dianders@chromium.org> wrote:
> Ulf,
>
> On Tue, Mar 17, 2015 at 3:23 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> This will get us within .3V of whatever vmmc is.  If vmmc is 3.3V, it
>>> will allow vqmmc of 3.0V - 3.6V.
>>>
>>> This _seems_ sane to me and given any sane system design we should be
>>> fine here, I think.  I can't see someone designing a system where
>>> vqmmc was not within .3V of vmmc, can you?  If we think someone will
>>> actually build a system where vmmc is 3.3V and vqmmc can't go higher
>>> than 2.7V then we'll either need to increase the tolerance here or add
>>> a new asymmetric system call like my original patches did.
>>
>> I know about SoC that supports 3.4V vmmc and 2.9V vqmmc.
>>
>> What I think we need is the option to have a policy here. We need to
>> allow voltage levels stated by the spec and at the same time try chose
>> the one best suited. That's not being accomplished here.
>>
>> Moreover, I wonder whether it's okay (from spec perspective) to have
>> vqmmc at a higher voltage level than vmmc. I don't think that's
>> allowed, but I might be wrong.
>
> OK, so sounds like I need to add a regulator_set_voltage_tol2()
> function that takes in an upper tolerance and a lower.  We can use the
> same rough implementation in the core we have today (if Mark is OK
> with that) with regulator_set_voltage_tol() but just allow it to be
> asymmetric.

Agree. Moreover we need that API to also pick the closest value to
target, when trying the range "target->minimum". I also believe it
would be good to allow both upper and lower limits to be zero.

If Mark disagrees with this approach, we will have to deal with all
the magic inside the mmc core layer. Very much similar how
mmc_regulator_get_ocrmask() is implemented.

Kind regards
Uffe

>
> From what I see in the spec for 3.3V cards are supposed to react to a
> high signal that is .625 * VDD - VDD + .3
>
>
> I might not be able to get to this till next week, though...
>
> -Doug

Re: [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 771 bytes --]

On Thu, Mar 19, 2015 at 12:14:11PM +0100, Ulf Hansson wrote:

> Agree. Moreover we need that API to also pick the closest value to
> target, when trying the range "target->minimum". I also believe it

The implementation *should* do that anyway, it's just not trivial to
implement in an efficient fashion with the current information we have
from drivers.

> would be good to allow both upper and lower limits to be zero.

The lower limit can be zero already though it isn't clear to me that
this is useful.  Setting an upper limit of zero seems nonsensical, an
upper limit that is lower than the lower limit isn't terribly obvious
and removing the upper limit isn't safe - it means that we'll happily
oversupply things which is a road to physical damage.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 200 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()

[Ulf Hansson]

On 19 March 2015 at 12:36, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Mar 19, 2015 at 12:14:11PM +0100, Ulf Hansson wrote:
>
>> Agree. Moreover we need that API to also pick the closest value to
>> target, when trying the range "target->minimum". I also believe it
>
> The implementation *should* do that anyway, it's just not trivial to
> implement in an efficient fashion with the current information we have
> from drivers.

The APIs regulator_count_voltages() and regulator_list_voltage(), are
currently used from the mmc core to find out which voltages that is
supported (with 0.1V granularity). Then that information can be used
when trying to set a new voltage.

But I guess such a wrapper API is out of the question?

Anyway, I get the feeling that we will need to do the same for this case.

>
>> would be good to allow both upper and lower limits to be zero.
>
> The lower limit can be zero already though it isn't clear to me that
> this is useful.  Setting an upper limit of zero seems nonsensical, an
> upper limit that is lower than the lower limit isn't terribly obvious
> and removing the upper limit isn't safe - it means that we'll happily
> oversupply things which is a road to physical damage.

I am not sure I follow here. In the regulator_set_voltage_tol() you
can only specifiy one limit (tolerance?). What Dough proposed was to
add a new API which can have both a low tolerance value and a high
tolerance value.

Kind regards
Uffe

Re: [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1892 bytes --]

On Fri, Mar 20, 2015 at 11:55:50AM +0100, Ulf Hansson wrote:
> On 19 March 2015 at 12:36, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > The implementation *should* do that anyway, it's just not trivial to
> > implement in an efficient fashion with the current information we have
> > from drivers.

> The APIs regulator_count_voltages() and regulator_list_voltage(), are
> currently used from the mmc core to find out which voltages that is
> supported (with 0.1V granularity). Then that information can be used
> when trying to set a new voltage.

> But I guess such a wrapper API is out of the question?

> Anyway, I get the feeling that we will need to do the same for this case.

As previously discussed the problem is that there can be a *lot* of
voltages on a modern regulator with fine grained voltage steps and
tolerances are also used for things like cpufreq where we care about
performance.  We need something that doesn't require a linear scan of
possible values.

> >> would be good to allow both upper and lower limits to be zero.

> > The lower limit can be zero already though it isn't clear to me that
> > this is useful.  Setting an upper limit of zero seems nonsensical, an
> > upper limit that is lower than the lower limit isn't terribly obvious
> > and removing the upper limit isn't safe - it means that we'll happily
> > oversupply things which is a road to physical damage.

> I am not sure I follow here. In the regulator_set_voltage_tol() you
> can only specifiy one limit (tolerance?). What Dough proposed was to
> add a new API which can have both a low tolerance value and a high
> tolerance value.

Tolerances are not limits - when you say "limit" that sounds like a hard
value.  I can't see any reason why the code would prevent anyone setting
a tolerance of zero, though it should be rare that this is actually the
best thing to do.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 200 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()

[Doug Anderson]

Hi,

On Fri, Mar 20, 2015 at 4:28 AM, Mark Brown <broonie@kernel.org> wrote:
> As previously discussed the problem is that there can be a *lot* of
> voltages on a modern regulator with fine grained voltage steps and
> tolerances are also used for things like cpufreq where we care about
> performance.  We need something that doesn't require a linear scan of
> possible values.

Finally getting back to this (sorry for the delay!).  I tried
modifying my patches to keep using the simple implementation of
regulator_set_voltage_tol() but to take two tolerances (lower and
upper).  ...but when I thought about it I decided it wasn't enough.  I
think that doing a proper implementation of
regulator_set_voltage_tol() is going to be a requirement for getting
the MMC core changes posted.

Specifically, I think the right implementation for the MMC core's 3.3V
signaling is something like this:

  /*
   * Bus operating conditions say that card should accept input
   * between (0.625 * VDD) and (VDD + 0.3), so we'll use those
   * as tolerances.
   */
  return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
       vmmc_voltage, vmmc_voltage * 375 / 1000, 300000);

Ulf says he has a board where vmmc is 3.4V and the max vqmmc is 2.9V.
If we think about that board, we'll end up calling
regulator_set_voltage_tol() with a lower/upper tolerance of 1.275V and
.3V.

Extending the current simple implementation of
regulator_set_voltage_tol() to take an upper and lower, that will
translate to first trying to set the voltage to 3.4V - 3.7V, which
will fail.  We'll then try to set the voltage to 2.125V - 3.7V.
Presumably that will end up picking 2.125V, which is really non-ideal
compared to 2.9V and it seems likely to cause some cards to start
failing.

Mark: I know you said you were considering writing a better
regulator_set_voltage_tol() yourself, but I don't know if you've
already started work on it.

I'm expecting to maybe have time to take a crack at it in a few weeks
if you haven't already done it by then.

Thanks!

-Doug

Re: [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 398 bytes --]

On Tue, Apr 07, 2015 at 01:05:43PM -0700, Doug Anderson wrote:

> Mark: I know you said you were considering writing a better
> regulator_set_voltage_tol() yourself, but I don't know if you've
> already started work on it.

> I'm expecting to maybe have time to take a crack at it in a few weeks
> if you haven't already done it by then.

If there's nothing on the list I won't have done anything.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 200 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 1/4] mmc: dw_mmc: Don't try to enable the CD until we're sure we're not deferring

[Jaehoon Chung]

Hi, Doug.

This patch is not related with [patch 2/4~4/4].
"[PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()" is discussing..
So i think if you're ok, i will pick this one [PATCH v4 1/4]. how about?

Best Regards,
Jaehoon Chung

On 03/12/2015 07:15 AM, Doug Anderson wrote:
> If dw_mci_init_slot() returns that we got a probe deferral then it may
> leave slot->mmc as NULL.  That will cause dw_mci_enable_cd() to crash
> when it calls mmc_gpio_get_cd().
> 
> Fix this by moving the call of dw_mci_enable_cd() until we're sure
> that we're good.  Note that if we have more than one slot and one
> defers (but the others don't) things won't work so well.  ...but
> that's not a new thing and everyone has already agreed that multislot
> support ought to be removed from dw_mmc eventually anyway since it is
> unused, untested, and you can see several bugs like this by inspecting
> the code.
> 
> Fixes: bcafaf5470f0 ("mmc: dw_mmc: Only enable CD after setup and only if needed")
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Changes in v4:
> - Defer vs. card detect fix patch new for v4.
> 
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/mmc/host/dw_mmc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 47dfd0e..e2811cf 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -2863,9 +2863,6 @@ int dw_mci_probe(struct dw_mci *host)
>  			init_slots++;
>  	}
>  
> -	/* Now that slots are all setup, we can enable card detect */
> -	dw_mci_enable_cd(host);
> -
>  	if (init_slots) {
>  		dev_info(host->dev, "%d slots initialized\n", init_slots);
>  	} else {
> @@ -2874,6 +2871,9 @@ int dw_mci_probe(struct dw_mci *host)
>  		goto err_dmaunmap;
>  	}
>  
> +	/* Now that slots are all setup, we can enable card detect */
> +	dw_mci_enable_cd(host);
> +
>  	if (host->quirks & DW_MCI_QUIRK_IDMAC_DTO)
>  		dev_info(host->dev, "Internal DMAC interrupt fix enabled.\n");
>  
> 

Re: [PATCH v4 1/4] mmc: dw_mmc: Don't try to enable the CD until we're sure we're not deferring

[Doug Anderson]

Jaehoon,

On Thu, Mar 26, 2015 at 10:55 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> Hi, Doug.
>
> This patch is not related with [patch 2/4~4/4].
> "[PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()" is discussing..
> So i think if you're ok, i will pick this one [PATCH v4 1/4]. how about?

Please take it and send it to Ulf as soon as you can.  It is only
related in that the later patches in this series suddenly make the
rk3288-evb start deferring and that causes a crash without this patch.

Re: [PATCH v4 4/4] ARM: dts: Specify VMMC and VQMMC on rk3288-evb

[Heiko Stübner]

Am Donnerstag, 12. März 2015, 00:30:14 schrieb Heiko Stuebner:
> Am Mittwoch, 11. März 2015, 15:15:17 schrieb Doug Anderson:
> > Specifying these rails should eventually let us do UHS.
> > 
> > Signed-off-by: Doug Anderson <dianders@chromium.org>
> 
> This one looks good to me and I'll take it into my dts branch.
> 
> But due to the deferal issue fixed in patch1, I'll wait a bit for patch1
> hopefully getting applied, so that we have a "clean" behaviour when
> everything reaches linux-next.

looks like patch1 made it into the mmc tree [0], which was the prequisite for 
this one. Therefore applied to my dts branch.


Heiko


[0] 
https://git.linaro.org/people/ulf.hansson/mmc.git/commit/b793f658b194edfe5e1d86aaeace01a7b03c68f9