* [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()
2015-03-11 22:15 [PATCH v4 1/4] mmc: dw_mmc: Don't try to enable the CD until we're sure we're not deferring Doug Anderson
@ 2015-03-11 22:15 ` Doug Anderson
2015-03-16 14:05 ` Ulf Hansson
2015-03-11 22:15 ` [PATCH v4 3/4] mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch Doug Anderson
` (2 subsequent siblings)
3 siblings, 1 reply; 20+ messages in thread
From: Doug Anderson @ 2015-03-11 22:15 UTC (permalink / raw)
To: Ulf Hansson, Heiko Stuebner, Jaehoon Chung, Seungwon Jeon
Cc: Mark Brown, Alexandru Stan, Alim Akhtar, Sonny Rao,
Andrew Bresticker, Addy Ke, javier.martinez, linux-rockchip,
linux-arm-kernel, Doug Anderson, chris, johan.rudholm,
adrian.hunter, tim.kryger, andrew_gabbasov, s.hauer, linux-mmc,
linux-kernel
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);
+ case MMC_SIGNAL_VOLTAGE_180:
+ return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
+ 1800000, 100000);
+ case MMC_SIGNAL_VOLTAGE_330:
+ return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc,
+ regulator_get_voltage(mmc->supply.vmmc), 300000);
+ 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);
--
2.2.0.rc0.207.ga3a616c
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()
2015-03-11 22:15 ` [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc() Doug Anderson
@ 2015-03-16 14:05 ` Ulf Hansson
2015-03-16 15:12 ` Doug Anderson
0 siblings, 1 reply; 20+ messages in thread
From: Ulf Hansson @ 2015-03-16 14:05 UTC (permalink / raw)
To: Doug Anderson
Cc: Heiko Stuebner, Jaehoon Chung, Seungwon Jeon, Mark Brown,
Alexandru Stan, Alim Akhtar, Sonny Rao, Andrew Bresticker,
Addy Ke, Javier Martinez Canillas, open list:ARM/Rockchip SoC...,
linux-arm-kernel, Chris Ball, Johan Rudholm, Adrian Hunter,
Tim Kryger, Andrew Gabbasov, Sascha Hauer, linux-mmc,
linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()
2015-03-16 14:05 ` Ulf Hansson
@ 2015-03-16 15:12 ` Doug Anderson
2015-03-17 10:23 ` Ulf Hansson
0 siblings, 1 reply; 20+ messages in thread
From: Doug Anderson @ 2015-03-16 15:12 UTC (permalink / raw)
To: Ulf Hansson
Cc: Heiko Stuebner, Jaehoon Chung, Seungwon Jeon, Mark Brown,
Alexandru Stan, Alim Akhtar, Sonny Rao, Andrew Bresticker,
Addy Ke, Javier Martinez Canillas, open list:ARM/Rockchip SoC...,
linux-arm-kernel, Chris Ball, Johan Rudholm, Adrian Hunter,
Tim Kryger, Andrew Gabbasov, Sascha Hauer, linux-mmc,
linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()
2015-03-16 15:12 ` Doug Anderson
@ 2015-03-17 10:23 ` Ulf Hansson
2015-03-17 10:38 ` Mark Brown
2015-03-19 4:09 ` Doug Anderson
0 siblings, 2 replies; 20+ messages in thread
From: Ulf Hansson @ 2015-03-17 10:23 UTC (permalink / raw)
To: Doug Anderson, Mark Brown
Cc: Heiko Stuebner, Jaehoon Chung, Seungwon Jeon, Alexandru Stan,
Alim Akhtar, Sonny Rao, Andrew Bresticker, Addy Ke,
Javier Martinez Canillas, open list:ARM/Rockchip SoC...,
linux-arm-kernel, Chris Ball, Johan Rudholm, Adrian Hunter,
Tim Kryger, Andrew Gabbasov, Sascha Hauer, linux-mmc,
linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()
2015-03-17 10:23 ` Ulf Hansson
@ 2015-03-17 10:38 ` Mark Brown
2015-03-17 11:28 ` Ulf Hansson
2015-03-19 4:09 ` Doug Anderson
1 sibling, 1 reply; 20+ messages in thread
From: Mark Brown @ 2015-03-17 10:38 UTC (permalink / raw)
To: Ulf Hansson
Cc: Doug Anderson, Heiko Stuebner, Jaehoon Chung, Seungwon Jeon,
Alexandru Stan, Alim Akhtar, Sonny Rao, Andrew Bresticker,
Addy Ke, Javier Martinez Canillas, open list:ARM/Rockchip SoC...,
linux-arm-kernel, Chris Ball, Johan Rudholm, Adrian Hunter,
Tim Kryger, Andrew Gabbasov, Sascha Hauer, linux-mmc,
linux-kernel@vger.kernel.org
[-- 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 --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()
2015-03-17 10:38 ` Mark Brown
@ 2015-03-17 11:28 ` Ulf Hansson
0 siblings, 0 replies; 20+ messages in thread
From: Ulf Hansson @ 2015-03-17 11:28 UTC (permalink / raw)
To: Mark Brown
Cc: Doug Anderson, Heiko Stuebner, Jaehoon Chung, Seungwon Jeon,
Alexandru Stan, Alim Akhtar, Sonny Rao, Andrew Bresticker,
Addy Ke, Javier Martinez Canillas, open list:ARM/Rockchip SoC...,
linux-arm-kernel, Chris Ball, Johan Rudholm, Adrian Hunter,
Tim Kryger, Andrew Gabbasov, Sascha Hauer, linux-mmc,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()
2015-03-17 10:23 ` Ulf Hansson
2015-03-17 10:38 ` Mark Brown
@ 2015-03-19 4:09 ` Doug Anderson
2015-03-19 11:14 ` Ulf Hansson
1 sibling, 1 reply; 20+ messages in thread
From: Doug Anderson @ 2015-03-19 4:09 UTC (permalink / raw)
To: Ulf Hansson
Cc: Mark Brown, Heiko Stuebner, Jaehoon Chung, Seungwon Jeon,
Alexandru Stan, Alim Akhtar, Sonny Rao, Andrew Bresticker,
Addy Ke, Javier Martinez Canillas, open list:ARM/Rockchip SoC...,
linux-arm-kernel, Chris Ball, Johan Rudholm, Adrian Hunter,
Tim Kryger, Andrew Gabbasov, Sascha Hauer, linux-mmc,
linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()
2015-03-19 4:09 ` Doug Anderson
@ 2015-03-19 11:14 ` Ulf Hansson
[not found] ` <CAPDyKFrkznbW7k_=BDo99jJhWzDbUgbeU+MXWB_E_UWXh=56Eg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 20+ messages in thread
From: Ulf Hansson @ 2015-03-19 11:14 UTC (permalink / raw)
To: Doug Anderson
Cc: Mark Brown, Heiko Stuebner, Jaehoon Chung, Seungwon Jeon,
Alexandru Stan, Alim Akhtar, Sonny Rao, Andrew Bresticker,
Addy Ke, Javier Martinez Canillas, open list:ARM/Rockchip SoC...,
linux-arm-kernel, Chris Ball, Johan Rudholm, Adrian Hunter,
Tim Kryger, Andrew Gabbasov, Sascha Hauer, linux-mmc,
linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 3/4] mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch
2015-03-11 22:15 [PATCH v4 1/4] mmc: dw_mmc: Don't try to enable the CD until we're sure we're not deferring Doug Anderson
2015-03-11 22:15 ` [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc() Doug Anderson
@ 2015-03-11 22:15 ` Doug Anderson
2015-03-13 11:32 ` [PATCH v4 1/4] mmc: dw_mmc: Don't try to enable the CD until we're sure we're not deferring Jaehoon Chung
2015-03-27 5:55 ` Jaehoon Chung
3 siblings, 0 replies; 20+ messages in thread
From: Doug Anderson @ 2015-03-11 22:15 UTC (permalink / raw)
To: Ulf Hansson, Heiko Stuebner, Jaehoon Chung, Seungwon Jeon
Cc: Mark Brown, Alexandru Stan, Alim Akhtar, Sonny Rao,
Andrew Bresticker, Addy Ke, javier.martinez, linux-rockchip,
linux-arm-kernel, Doug Anderson, chris, linux-mmc, linux-kernel
We've introduced a new helper in the MMC core:
mmc_regulator_set_vqmmc(). Let's use this in dw_mmc. Using this new
helper has some advantages:
1. We get the mmc_regulator_set_vqmmc() behavior of trying to match
VQMMC and VMMC when the signal voltage is 3.3V. This ensures max
compatibility.
2. We get rid of a few more warnings when probing unsupported
voltages.
3. We get rid of some non-dw_mmc specific code in dw_mmc.
Signed-off-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Andrew Bresticker <abrestic@chromium.org>
---
Changes in v4: None
Changes in v3:
- Continue to check for errors if VQMMC is not an error as per Andrew.
Changes in v2: None
drivers/mmc/host/dw_mmc.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index e2811cf..615d1d9 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1214,7 +1214,6 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
struct dw_mci *host = slot->host;
u32 uhs;
u32 v18 = SDMMC_UHS_18V << slot->id;
- int min_uv, max_uv;
int ret;
/*
@@ -1223,22 +1222,18 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
* does no harm but you need to set the regulator directly. Try both.
*/
uhs = mci_readl(host, UHS_REG);
- if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
- min_uv = 2700000;
- max_uv = 3600000;
+ if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330)
uhs &= ~v18;
- } else {
- min_uv = 1700000;
- max_uv = 1950000;
+ else
uhs |= v18;
- }
+
if (!IS_ERR(mmc->supply.vqmmc)) {
- ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, max_uv);
+ ret = mmc_regulator_set_vqmmc(mmc, ios);
if (ret) {
dev_dbg(&mmc->class_dev,
- "Regulator set error %d: %d - %d\n",
- ret, min_uv, max_uv);
+ "Regulator set error %d - %s V\n",
+ ret, uhs & v18 ? "1.8" : "3.3");
return ret;
}
}
--
2.2.0.rc0.207.ga3a616c
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/4] mmc: dw_mmc: Don't try to enable the CD until we're sure we're not deferring
2015-03-11 22:15 [PATCH v4 1/4] mmc: dw_mmc: Don't try to enable the CD until we're sure we're not deferring Doug Anderson
2015-03-11 22:15 ` [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc() Doug Anderson
2015-03-11 22:15 ` [PATCH v4 3/4] mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch Doug Anderson
@ 2015-03-13 11:32 ` Jaehoon Chung
2015-03-13 12:10 ` Heiko Stuebner
2015-03-27 5:55 ` Jaehoon Chung
3 siblings, 1 reply; 20+ messages in thread
From: Jaehoon Chung @ 2015-03-13 11:32 UTC (permalink / raw)
To: Doug Anderson, Ulf Hansson, Heiko Stuebner, Jaehoon Chung, Seungwon Jeon
Cc: Mark Brown, Alexandru Stan, Alim Akhtar, Sonny Rao,
Andrew Bresticker, Addy Ke, javier.martinez, linux-rockchip,
linux-arm-kernel, chris, linux-mmc, linux-kernel
Hi Doug,
Will apply. Thanks!
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");
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/4] mmc: dw_mmc: Don't try to enable the CD until we're sure we're not deferring
2015-03-13 11:32 ` [PATCH v4 1/4] mmc: dw_mmc: Don't try to enable the CD until we're sure we're not deferring Jaehoon Chung
@ 2015-03-13 12:10 ` Heiko Stuebner
2015-03-16 2:09 ` Jaehoon Chung
0 siblings, 1 reply; 20+ messages in thread
From: Heiko Stuebner @ 2015-03-13 12:10 UTC (permalink / raw)
To: Jaehoon Chung
Cc: Ulf Hansson, Alexandru Stan, linux-rockchip, Seungwon Jeon,
linux-kernel, linux-mmc, Doug Anderson, chris, Andrew Bresticker,
Mark Brown, Addy Ke, Alim Akhtar, Sonny Rao, javier.martinez,
linux-arm-kernel
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?
Thanks
Heiko
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/4] mmc: dw_mmc: Don't try to enable the CD until we're sure we're not deferring
2015-03-13 12:10 ` Heiko Stuebner
@ 2015-03-16 2:09 ` Jaehoon Chung
0 siblings, 0 replies; 20+ messages in thread
From: Jaehoon Chung @ 2015-03-16 2:09 UTC (permalink / raw)
To: Heiko Stuebner, Jaehoon Chung
Cc: Ulf Hansson, Alexandru Stan, linux-rockchip, Seungwon Jeon,
linux-kernel, linux-mmc, Doug Anderson, chris, Andrew Bresticker,
Mark Brown, Addy Ke, Alim Akhtar, Sonny Rao, javier.martinez,
linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/4] mmc: dw_mmc: Don't try to enable the CD until we're sure we're not deferring
2015-03-11 22:15 [PATCH v4 1/4] mmc: dw_mmc: Don't try to enable the CD until we're sure we're not deferring Doug Anderson
` (2 preceding siblings ...)
2015-03-13 11:32 ` [PATCH v4 1/4] mmc: dw_mmc: Don't try to enable the CD until we're sure we're not deferring Jaehoon Chung
@ 2015-03-27 5:55 ` Jaehoon Chung
2015-03-27 15:46 ` Doug Anderson
3 siblings, 1 reply; 20+ messages in thread
From: Jaehoon Chung @ 2015-03-27 5:55 UTC (permalink / raw)
To: Doug Anderson, Ulf Hansson, Heiko Stuebner, Seungwon Jeon
Cc: Mark Brown, Alexandru Stan, Alim Akhtar, Sonny Rao,
Andrew Bresticker, Addy Ke, javier.martinez, linux-rockchip,
linux-arm-kernel, chris, linux-mmc, linux-kernel
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");
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/4] mmc: dw_mmc: Don't try to enable the CD until we're sure we're not deferring
2015-03-27 5:55 ` Jaehoon Chung
@ 2015-03-27 15:46 ` Doug Anderson
0 siblings, 0 replies; 20+ messages in thread
From: Doug Anderson @ 2015-03-27 15:46 UTC (permalink / raw)
To: Jaehoon Chung
Cc: Ulf Hansson, Heiko Stuebner, Seungwon Jeon, Mark Brown,
Alexandru Stan, Alim Akhtar, Sonny Rao, Andrew Bresticker,
Addy Ke, Javier Martinez Canillas, open list:ARM/Rockchip SoC...,
linux-arm-kernel, Chris Ball, linux-mmc, linux-kernel
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.
^ permalink raw reply [flat|nested] 20+ messages in thread