* [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
` (3 subsequent siblings)
4 siblings, 1 reply; 23+ messages in thread
From: Doug Anderson @ 2015-03-11 22:15 UTC (permalink / raw)
To: linux-arm-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] 23+ messages in thread* [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; 23+ messages in thread
From: Ulf Hansson @ 2015-03-16 14:05 UTC (permalink / raw)
To: linux-arm-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 at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 23+ messages in thread* [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; 23+ messages in thread
From: Doug Anderson @ 2015-03-16 15:12 UTC (permalink / raw)
To: linux-arm-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] 23+ messages in thread* [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; 23+ messages in thread
From: Ulf Hansson @ 2015-03-17 10:23 UTC (permalink / raw)
To: linux-arm-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] 23+ messages in thread* [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; 23+ messages in thread
From: Mark Brown @ 2015-03-17 10:38 UTC (permalink / raw)
To: linux-arm-kernel
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150317/16382021/attachment.sig>
^ permalink raw reply [flat|nested] 23+ messages in thread* [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; 23+ messages in thread
From: Ulf Hansson @ 2015-03-17 11:28 UTC (permalink / raw)
To: linux-arm-kernel
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] 23+ messages in thread
* [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; 23+ messages in thread
From: Doug Anderson @ 2015-03-19 4:09 UTC (permalink / raw)
To: linux-arm-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] 23+ messages in thread
* [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
2015-03-19 11:36 ` Mark Brown
0 siblings, 1 reply; 23+ messages in thread
From: Ulf Hansson @ 2015-03-19 11:14 UTC (permalink / raw)
To: linux-arm-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] 23+ messages in thread
* [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()
2015-03-19 11:14 ` Ulf Hansson
@ 2015-03-19 11:36 ` Mark Brown
2015-03-20 10:55 ` Ulf Hansson
0 siblings, 1 reply; 23+ messages in thread
From: Mark Brown @ 2015-03-19 11:36 UTC (permalink / raw)
To: linux-arm-kernel
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150319/7de1b59e/attachment-0001.sig>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()
2015-03-19 11:36 ` Mark Brown
@ 2015-03-20 10:55 ` Ulf Hansson
2015-03-20 11:28 ` Mark Brown
0 siblings, 1 reply; 23+ messages in thread
From: Ulf Hansson @ 2015-03-20 10:55 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()
2015-03-20 10:55 ` Ulf Hansson
@ 2015-03-20 11:28 ` Mark Brown
2015-04-07 20:05 ` Doug Anderson
0 siblings, 1 reply; 23+ messages in thread
From: Mark Brown @ 2015-03-20 11:28 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Mar 20, 2015 at 11:55:50AM +0100, Ulf Hansson wrote:
> On 19 March 2015 at 12:36, Mark Brown <broonie@kernel.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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150320/295ed9f1/attachment.sig>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v4 2/4] mmc: core: Add mmc_regulator_set_vqmmc()
2015-03-20 11:28 ` Mark Brown
@ 2015-04-07 20:05 ` Doug Anderson
2015-04-08 11:28 ` Mark Brown
0 siblings, 1 reply; 23+ messages in thread
From: Doug Anderson @ 2015-04-07 20:05 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ 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-11 22:15 ` [PATCH v4 4/4] ARM: dts: Specify VMMC and VQMMC on rk3288-evb Doug Anderson
` (2 subsequent siblings)
4 siblings, 0 replies; 23+ messages in thread
From: Doug Anderson @ 2015-03-11 22:15 UTC (permalink / raw)
To: linux-arm-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] 23+ messages in thread* [PATCH v4 4/4] ARM: dts: Specify VMMC and VQMMC on rk3288-evb
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-11 22:15 ` Doug Anderson
2015-03-11 23:30 ` Heiko Stuebner
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
4 siblings, 1 reply; 23+ messages in thread
From: Doug Anderson @ 2015-03-11 22:15 UTC (permalink / raw)
To: linux-arm-kernel
Specifying these rails should eventually let us do UHS.
Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in v4:
- Add vcc_sd regulator which is present on EVB 2.0 boards
Changes in v3: None
Changes in v2:
- Fix subject line
arch/arm/boot/dts/rk3288-evb.dtsi | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/arch/arm/boot/dts/rk3288-evb.dtsi b/arch/arm/boot/dts/rk3288-evb.dtsi
index 5e895a5..6d4ae9f 100644
--- a/arch/arm/boot/dts/rk3288-evb.dtsi
+++ b/arch/arm/boot/dts/rk3288-evb.dtsi
@@ -103,6 +103,23 @@
regulator-always-on;
regulator-boot-on;
};
+
+ /*
+ * NOTE: vcc_sd isn't hooked up on v1.0 boards where power comes from
+ * vcc_io directly. Those boards won't be able to power cycle SD cards
+ * but it shouldn't hurt to toggle this pin there anyway.
+ */
+ vcc_sd: sdmmc-regulator {
+ compatible = "regulator-fixed";
+ gpio = <&gpio7 11 GPIO_ACTIVE_LOW>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&sdmmc_pwr>;
+ regulator-name = "vcc_sd";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ startup-delay-us = <100000>;
+ vin-supply = <&vcc_io>;
+ };
};
&emmc {
@@ -132,6 +149,8 @@
pinctrl-names = "default";
pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
status = "okay";
+ vmmc-supply = <&vcc_sd>;
+ vqmmc-supply = <&vccio_sd>;
};
&i2c0 {
@@ -223,6 +242,10 @@
sdmmc_cmd: sdmmc-cmd {
rockchip,pins = <6 21 RK_FUNC_1 &pcfg_pull_up_drv_8ma>;
};
+
+ sdmmc_pwr: sdmmc-pwr {
+ rockchip,pins = <7 11 RK_FUNC_GPIO &pcfg_pull_none>;
+ };
};
usb {
--
2.2.0.rc0.207.ga3a616c
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v4 4/4] ARM: dts: Specify VMMC and VQMMC on rk3288-evb
2015-03-11 22:15 ` [PATCH v4 4/4] ARM: dts: Specify VMMC and VQMMC on rk3288-evb Doug Anderson
@ 2015-03-11 23:30 ` Heiko Stuebner
2015-04-07 21:37 ` Heiko Stübner
0 siblings, 1 reply; 23+ messages in thread
From: Heiko Stuebner @ 2015-03-11 23:30 UTC (permalink / raw)
To: linux-arm-kernel
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.
> ---
> Changes in v4:
> - Add vcc_sd regulator which is present on EVB 2.0 boards
>
> Changes in v3: None
> Changes in v2:
> - Fix subject line
>
> arch/arm/boot/dts/rk3288-evb.dtsi | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/arch/arm/boot/dts/rk3288-evb.dtsi
> b/arch/arm/boot/dts/rk3288-evb.dtsi index 5e895a5..6d4ae9f 100644
> --- a/arch/arm/boot/dts/rk3288-evb.dtsi
> +++ b/arch/arm/boot/dts/rk3288-evb.dtsi
> @@ -103,6 +103,23 @@
> regulator-always-on;
> regulator-boot-on;
> };
> +
> + /*
> + * NOTE: vcc_sd isn't hooked up on v1.0 boards where power comes from
> + * vcc_io directly. Those boards won't be able to power cycle SD cards
> + * but it shouldn't hurt to toggle this pin there anyway.
> + */
> + vcc_sd: sdmmc-regulator {
> + compatible = "regulator-fixed";
> + gpio = <&gpio7 11 GPIO_ACTIVE_LOW>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&sdmmc_pwr>;
> + regulator-name = "vcc_sd";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + startup-delay-us = <100000>;
> + vin-supply = <&vcc_io>;
> + };
> };
>
> &emmc {
> @@ -132,6 +149,8 @@
> pinctrl-names = "default";
> pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
> status = "okay";
> + vmmc-supply = <&vcc_sd>;
> + vqmmc-supply = <&vccio_sd>;
> };
>
> &i2c0 {
> @@ -223,6 +242,10 @@
> sdmmc_cmd: sdmmc-cmd {
> rockchip,pins = <6 21 RK_FUNC_1 &pcfg_pull_up_drv_8ma>;
> };
> +
> + sdmmc_pwr: sdmmc-pwr {
> + rockchip,pins = <7 11 RK_FUNC_GPIO &pcfg_pull_none>;
> + };
> };
>
> usb {
^ permalink raw reply [flat|nested] 23+ messages in thread
* [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-11 22:15 ` [PATCH v4 4/4] ARM: dts: Specify VMMC and VQMMC on rk3288-evb Doug Anderson
@ 2015-03-13 11:32 ` Jaehoon Chung
2015-03-13 12:10 ` Heiko Stuebner
2015-03-27 5:55 ` Jaehoon Chung
4 siblings, 1 reply; 23+ messages in thread
From: Jaehoon Chung @ 2015-03-13 11:32 UTC (permalink / raw)
To: linux-arm-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] 23+ messages in thread* [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; 23+ messages in thread
From: Heiko Stuebner @ 2015-03-13 12:10 UTC (permalink / raw)
To: 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] 23+ messages in thread
* [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; 23+ messages in thread
From: Jaehoon Chung @ 2015-03-16 2:09 UTC (permalink / raw)
To: 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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [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
` (3 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
4 siblings, 1 reply; 23+ messages in thread
From: Jaehoon Chung @ 2015-03-27 5:55 UTC (permalink / raw)
To: linux-arm-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] 23+ messages in thread* [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; 23+ messages in thread
From: Doug Anderson @ 2015-03-27 15:46 UTC (permalink / raw)
To: linux-arm-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] 23+ messages in thread