Re: [PATCH 1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators
Hello Yuvaraj,
On Sat, Jun 28, 2014 at 12:47 AM, Doug Anderson diand...@chromium.org wrote:
On Fri, Jun 27, 2014 at 3:59 AM, Yuvaraj Kumar yuvaraj...@gmail.com wrote:
mmc_regulator_set_ocr() is failing as its a fixed-regulator.
Can you explain more about what's failing? It sure looks like mmc
core is supposed to handle this given comments below
/*
* If we're using a fixed/static regulator, don't call
* regulator_set_voltage; it would fail.
*/
tps65090 driver does not register through fixed regulator framework.It
uses normal regulator framework and supports only
enable/disable/is_enabled callbacks.Also it lacks certain callbacks
for list_voltage, get_voltage ,set_voltage etc.
[2.306476] dwmmc_exynos 1222.mmc: Failed getting OCR mask: -22
[2.393403] dwmmc_exynos 1222.mmc: could not set regulator OCR (-22)
For the above reason,regulator framework treats fet4 as unused
regulator and disables the vmmc regulator.
Ah. Perhaps tps65090 needs to be fixed then... I'm not sure any
other drivers cared before so maybe that's why it was never caught?
I've been looking at this and as Doug and you said it seems that
nobody cared and since the tps65090 fets are actually just load
switches the driver only implementes the .enable and .disable
functions handlers. Also since their output voltage is just equal to
their input supply, that information was not present neither in the
driver nor in the Device Tree.
But when fet4 is used as the vmmc-supply phandle,
mmc_regulator_get_supply() calls mmc_regulator_get_ocrmask() [0] which
expects to fetch the regulator voltage count and current voltage as
you had explained so the function was failing.
Now I don't think that the driver needs to implement the
{get,set,list}_voltage callbacks. If a tps65090 regulator has both
regulator-min-microvolt and regulator-max-microvolt properties from
the generic regulator DT binding, and the value is the same, it can be
assumed that the regulator is fixed and the fixed_uV field can be set
to the output voltage and n_voltages to 1. That's everything that the
regulator core needs from a fixed regulator in order to make
regulator_get_voltage() to succeed:
static int _regulator_get_voltage(struct regulator_dev *rdev)
{
...
if (rdev-desc-ops-get_voltage_sel) {
sel = rdev-desc-ops-get_voltage_sel(rdev);
if (sel 0)
return sel;
ret = rdev-desc-ops-list_voltage(rdev, sel);
} else if (rdev-desc-ops-get_voltage) {
ret = rdev-desc-ops-get_voltage(rdev);
} else if (rdev-desc-ops-list_voltage) {
ret = rdev-desc-ops-list_voltage(rdev, 0);
} else if (rdev-desc-fixed_uV (rdev-desc-n_voltages == 1)) {
ret = rdev-desc-fixed_uV;
} else {
return -EINVAL;
}
...
}
What do you think about patch [1]?
So, with that patch mmc_regulator_get_supply() does not fail anymore
and also does not break DT backward compatibility since it only has
effect on the regulators that define their min and max voltage and not
in the ones that don't like is the case in old DTB.
Now, it is an RFC since I don't know if this is the correct solution.
Even though I've seen that min == max seems to imply that the
regulator is fixed, I wonder if is better to have an explicit generic
DT property (regulator-fixed-microvolt?) that is used to set fixed_uV
in of_get_regulation_constraints(). Since this seems to be a general
problem and not only related to the tps65090 device or the Peach
Pit/Pi use case. If you agree that it's a good solution then I can
post it as a proper patch.
I've also pushed this patch and the ones adding the needed DTS bits
for Peach Pit and Pi DTS to my personal repository so you can test it.
The branch is:
http://cgit.collabora.com/git/user/javier/linux.git/log/?h=tps65090-fixes
You also need to replace regulator_enable(mmc-supply.vmmc) and
regulator_disable(mmc-supply.vmmc) by mmc_regulator_set_ocr(mmc,
mmc-supply.vmmc, ios-vdd) and mmc_regulator_set_ocr(mmc,
mmc-supply.vmmc, 0) respectively in your original patch.
Best regards,
Javier
[0]: http://lxr.free-electrons.com/source/drivers/mmc/core/core.c#L1215
[1]:
From 3d2e6079cc8c372da946d430d43d36af99e7a582 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas javier.marti...@collabora.co.uk
Date: Tue, 22 Jul 2014 19:16:47 +0200
Subject: [RFC] regulator: tps65090: Allow regulators voltage to be
queried
The tps65090 device has some regulators with a fixed output
voltage and others with an adjustable output voltage. But the
regulators with adjustable output just provide the voltage of
their input supply so they really are fixed regulators within
a supported voltage range that depends on their input voltage.
That is why the driver only provides an .enable and .disable
function handlers and not a .{get,set,list}_voltage handlers.
But there is code in the kernel that expects to query the
output voltage for all regulators. For example the function
mmc_regulator_get_ocrmask() is executed
Re: [PATCH 1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators
On 06/27/2014 01:18 AM, Doug Anderson wrote:
Yuvaraj,
On Thu, Jun 26, 2014 at 4:21 AM, Yuvaraj Kumar yuvaraj...@gmail.com wrote:
Doug
On Tue, Jun 24, 2014 at 11:30 PM, Doug Anderson diand...@chromium.org
wrote:
Yuvaraj,
On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D yuvaraj...@gmail.com
wrote:
This patch makes use of mmc_regulator_get_supply() to handle
the vmmc and vqmmc regulators.Also it moves the code handling
the these regulators to dw_mci_set_ios().It turned on the vmmc
and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off
during MMC_POWER_OFF.
Signed-off-by: Yuvaraj Kumar C D yuvaraj...@samsung.com
---
drivers/mmc/host/dw_mmc.c | 71
++---
drivers/mmc/host/dw_mmc.h |2 ++
2 files changed, 36 insertions(+), 37 deletions(-)
Perhaps you could CC me on the whole series for the next version since
I was involved in privately reviewing previous versions?
It was just accidental missing you in the CC .Surely i will add you in
CC for next versions.
Overall caveat for my review is that I'm nowhere near an SD/MMC expert.
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 1ac227c..f5cabce 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc,
struct mmc_ios *ios)
struct dw_mci_slot *slot = mmc_priv(mmc);
const struct dw_mci_drv_data *drv_data = slot-host-drv_data;
u32 regs;
+ int ret;
switch (ios-bus_width) {
case MMC_BUS_WIDTH_4:
@@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc,
struct mmc_ios *ios)
switch (ios-power_mode) {
case MMC_POWER_UP:
+ if ((!IS_ERR(mmc-supply.vmmc))
+ !test_bit(DW_MMC_CARD_POWERED,
slot-flags)) {
+ ret = regulator_enable(mmc-supply.vmmc);
+ if (!ret)
+ set_bit(DW_MMC_CARD_POWERED, slot-flags);
+ }
As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at
different times.
As you can see people's have different opinion on this.When i had a
look at the other drivers in the subsystem which does in the same flow
as above.However i will change in the next version.
Given my self proclaimed lack of SD/MMC knowledge, if others have a
good reason for doing them separate then you should do it that way.
So far I haven't heard that reason but I certainly could be wrong.
At first time, i had believed nothing problem that it turns on vmmc and vqmmc
at different time.
It could have the potential problem.
Best Regards,
Jaehoon Chung
@@ -225,6 +225,8 @@ struct dw_mci_slot {
unsigned long flags;
#define DW_MMC_CARD_PRESENT0
#define DW_MMC_CARD_NEED_INIT 1
+#define DW_MMC_CARD_POWERED2
+#define DW_MMC_IO_POWERED 3
I don't really think you should have two bits here. From my
understanding of SD cards there should be very little reason to have
vqmmc and vmmc not powered at the same time.
I think if i can use mmc_regulator_set_ocr(), we don't need additional
flag.But for tps65090 mmc_regulator_get_ocr() and
mmc_regulator_set_ocr() is failing as its a fixed-regulator.
Can you explain more about what's failing? It sure looks like mmc
core is supposed to handle this given comments below
/*
* If we're using a fixed/static regulator, don't call
* regulator_set_voltage; it would fail.
*/
voltage = regulator_get_voltage(supply);
if (!regulator_can_change_voltage(supply))
min_uV = max_uV = voltage;
-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators
Jaehoon,
On Mon, Jun 30, 2014 at 5:13 AM, Jaehoon Chung jh80.ch...@samsung.com wrote:
On 06/27/2014 01:18 AM, Doug Anderson wrote:
Yuvaraj,
On Thu, Jun 26, 2014 at 4:21 AM, Yuvaraj Kumar yuvaraj...@gmail.com wrote:
Doug
On Tue, Jun 24, 2014 at 11:30 PM, Doug Anderson diand...@chromium.org
wrote:
Yuvaraj,
On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D yuvaraj...@gmail.com
wrote:
This patch makes use of mmc_regulator_get_supply() to handle
the vmmc and vqmmc regulators.Also it moves the code handling
the these regulators to dw_mci_set_ios().It turned on the vmmc
and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off
during MMC_POWER_OFF.
Signed-off-by: Yuvaraj Kumar C D yuvaraj...@samsung.com
---
drivers/mmc/host/dw_mmc.c | 71
++---
drivers/mmc/host/dw_mmc.h |2 ++
2 files changed, 36 insertions(+), 37 deletions(-)
Perhaps you could CC me on the whole series for the next version since
I was involved in privately reviewing previous versions?
It was just accidental missing you in the CC .Surely i will add you in
CC for next versions.
Overall caveat for my review is that I'm nowhere near an SD/MMC expert.
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 1ac227c..f5cabce 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc,
struct mmc_ios *ios)
struct dw_mci_slot *slot = mmc_priv(mmc);
const struct dw_mci_drv_data *drv_data = slot-host-drv_data;
u32 regs;
+ int ret;
switch (ios-bus_width) {
case MMC_BUS_WIDTH_4:
@@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc,
struct mmc_ios *ios)
switch (ios-power_mode) {
case MMC_POWER_UP:
+ if ((!IS_ERR(mmc-supply.vmmc))
+ !test_bit(DW_MMC_CARD_POWERED,
slot-flags)) {
+ ret = regulator_enable(mmc-supply.vmmc);
+ if (!ret)
+ set_bit(DW_MMC_CARD_POWERED,
slot-flags);
+ }
As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at
different times.
As you can see people's have different opinion on this.When i had a
look at the other drivers in the subsystem which does in the same flow
as above.However i will change in the next version.
Given my self proclaimed lack of SD/MMC knowledge, if others have a
good reason for doing them separate then you should do it that way.
So far I haven't heard that reason but I certainly could be wrong.
At first time, i had believed nothing problem that it turns on vmmc and vqmmc
at different time.
It could have the potential problem.
OK. As I said I'm nowhere near an expert on SD/MMC, so if there's
something out there that says that turning vmmc on before vqmmc is the
right thing to do then I guess that's the answer. I would still be
very curious to get more details on what the potential problem is.
Could you provide a reference to documentation that says vmmc should
be on before vqmmc or describe a situation where it's important?
Thanks!
-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators
Hi Doug.
On 07/01/2014 12:06 AM, Doug Anderson wrote:
Jaehoon,
On Mon, Jun 30, 2014 at 5:13 AM, Jaehoon Chung jh80.ch...@samsung.com wrote:
On 06/27/2014 01:18 AM, Doug Anderson wrote:
Yuvaraj,
On Thu, Jun 26, 2014 at 4:21 AM, Yuvaraj Kumar yuvaraj...@gmail.com wrote:
Doug
On Tue, Jun 24, 2014 at 11:30 PM, Doug Anderson diand...@chromium.org
wrote:
Yuvaraj,
On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D yuvaraj...@gmail.com
wrote:
This patch makes use of mmc_regulator_get_supply() to handle
the vmmc and vqmmc regulators.Also it moves the code handling
the these regulators to dw_mci_set_ios().It turned on the vmmc
and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off
during MMC_POWER_OFF.
Signed-off-by: Yuvaraj Kumar C D yuvaraj...@samsung.com
---
drivers/mmc/host/dw_mmc.c | 71
++---
drivers/mmc/host/dw_mmc.h |2 ++
2 files changed, 36 insertions(+), 37 deletions(-)
Perhaps you could CC me on the whole series for the next version since
I was involved in privately reviewing previous versions?
It was just accidental missing you in the CC .Surely i will add you in
CC for next versions.
Overall caveat for my review is that I'm nowhere near an SD/MMC expert.
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 1ac227c..f5cabce 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc,
struct mmc_ios *ios)
struct dw_mci_slot *slot = mmc_priv(mmc);
const struct dw_mci_drv_data *drv_data = slot-host-drv_data;
u32 regs;
+ int ret;
switch (ios-bus_width) {
case MMC_BUS_WIDTH_4:
@@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc,
struct mmc_ios *ios)
switch (ios-power_mode) {
case MMC_POWER_UP:
+ if ((!IS_ERR(mmc-supply.vmmc))
+ !test_bit(DW_MMC_CARD_POWERED,
slot-flags)) {
+ ret = regulator_enable(mmc-supply.vmmc);
+ if (!ret)
+ set_bit(DW_MMC_CARD_POWERED,
slot-flags);
+ }
As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at
different times.
As you can see people's have different opinion on this.When i had a
look at the other drivers in the subsystem which does in the same flow
as above.However i will change in the next version.
Given my self proclaimed lack of SD/MMC knowledge, if others have a
good reason for doing them separate then you should do it that way.
So far I haven't heard that reason but I certainly could be wrong.
At first time, i had believed nothing problem that it turns on vmmc and
vqmmc at different time.
It could have the potential problem.
OK. As I said I'm nowhere near an expert on SD/MMC, so if there's
something out there that says that turning vmmc on before vqmmc is the
right thing to do then I guess that's the answer. I would still be
very curious to get more details on what the potential problem is.
Could you provide a reference to documentation that says vmmc should
be on before vqmmc or describe a situation where it's important?
Actually i didn't have any documentation.
According to eMMC spec, vmmc and vqmmc should be used with same power supply.
In this case, it will turn on vmmc and vqmmc at same time.
If vqmmc is used with the I/O power supply, the sequence isn't important,
but vmmc and vqmmc need to wait until stabling the power.
I didn't know Potential problem is what problem.
(Potential problem is mentioned from our H/W team member. H/W mis-operating?)
I didn't have the expert H/W knowledge.
Thanks!
Bset Regards,
Jaehoon Chung
Thanks!
-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators
On Thu, Jun 26, 2014 at 9:48 PM, Doug Anderson diand...@chromium.org wrote:
Yuvaraj,
On Thu, Jun 26, 2014 at 4:21 AM, Yuvaraj Kumar yuvaraj...@gmail.com wrote:
Doug
On Tue, Jun 24, 2014 at 11:30 PM, Doug Anderson diand...@chromium.org
wrote:
Yuvaraj,
On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D yuvaraj...@gmail.com
wrote:
This patch makes use of mmc_regulator_get_supply() to handle
the vmmc and vqmmc regulators.Also it moves the code handling
the these regulators to dw_mci_set_ios().It turned on the vmmc
and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off
during MMC_POWER_OFF.
Signed-off-by: Yuvaraj Kumar C D yuvaraj...@samsung.com
---
drivers/mmc/host/dw_mmc.c | 71
++---
drivers/mmc/host/dw_mmc.h |2 ++
2 files changed, 36 insertions(+), 37 deletions(-)
Perhaps you could CC me on the whole series for the next version since
I was involved in privately reviewing previous versions?
It was just accidental missing you in the CC .Surely i will add you in
CC for next versions.
Overall caveat for my review is that I'm nowhere near an SD/MMC expert.
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 1ac227c..f5cabce 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc,
struct mmc_ios *ios)
struct dw_mci_slot *slot = mmc_priv(mmc);
const struct dw_mci_drv_data *drv_data = slot-host-drv_data;
u32 regs;
+ int ret;
switch (ios-bus_width) {
case MMC_BUS_WIDTH_4:
@@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc,
struct mmc_ios *ios)
switch (ios-power_mode) {
case MMC_POWER_UP:
+ if ((!IS_ERR(mmc-supply.vmmc))
+ !test_bit(DW_MMC_CARD_POWERED,
slot-flags)) {
+ ret = regulator_enable(mmc-supply.vmmc);
+ if (!ret)
+ set_bit(DW_MMC_CARD_POWERED, slot-flags);
+ }
As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at
different times.
As you can see people's have different opinion on this.When i had a
look at the other drivers in the subsystem which does in the same flow
as above.However i will change in the next version.
Given my self proclaimed lack of SD/MMC knowledge, if others have a
good reason for doing them separate then you should do it that way.
So far I haven't heard that reason but I certainly could be wrong.
@@ -225,6 +225,8 @@ struct dw_mci_slot {
unsigned long flags;
#define DW_MMC_CARD_PRESENT0
#define DW_MMC_CARD_NEED_INIT 1
+#define DW_MMC_CARD_POWERED2
+#define DW_MMC_IO_POWERED 3
I don't really think you should have two bits here. From my
understanding of SD cards there should be very little reason to have
vqmmc and vmmc not powered at the same time.
I think if i can use mmc_regulator_set_ocr(), we don't need additional
flag.But for tps65090 mmc_regulator_get_ocr() and
mmc_regulator_set_ocr() is failing as its a fixed-regulator.
Can you explain more about what's failing? It sure looks like mmc
core is supposed to handle this given comments below
/*
* If we're using a fixed/static regulator, don't call
* regulator_set_voltage; it would fail.
*/
tps65090 driver does not register through fixed regulator framework.It
uses normal regulator framework and supports only
enable/disable/is_enabled callbacks.Also it lacks certain callbacks
for list_voltage, get_voltage ,set_voltage etc.
[2.306476] dwmmc_exynos 1222.mmc: Failed getting OCR mask: -22
[2.393403] dwmmc_exynos 1222.mmc: could not set regulator OCR (-22)
For the above reason,regulator framework treats fet4 as unused
regulator and disables the vmmc regulator.
voltage = regulator_get_voltage(supply);
if (!regulator_can_change_voltage(supply))
min_uV = max_uV = voltage;
-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators
On Fri, June 27, 2014, Doug Anderson wrote:
Seungwon,
On Thu, Jun 26, 2014 at 3:30 AM, Seungwon Jeon tgih@samsung.com wrote:
Hi Doug,
On Thu, June 26, 2014, Doug Anderson wrote:
Seungwon,
On Wed, Jun 25, 2014 at 4:18 AM, Seungwon Jeon tgih@samsung.com
wrote:
+ case MMC_POWER_ON:
+ if (!IS_ERR(mmc-supply.vqmmc)
+ !test_bit(DW_MMC_IO_POWERED,
slot-flags)) {
You can use regulator_is_enabled() instead of flag bit,
DW_MMC_IO_POWERED.
I'd be a little worried about regulator_is_enabled() since regulators
are reference counted. What if someone else is sharing this
regulator? The regulator might happen to be enabled when you check it
but unless you add your own dw_mmc reference count they might turn it
off.
Cool, that's a possibility. Some assumption may need.
If mmc's core can guarantee its balance, I think we don't need to consider
some flag.
I notice that the mmc core seems to keep a flag itself for vdd (the
mmc-regulator_enabled flag). That would imply that the core thought
it was important to have the extra flag and that we should keep our
own flag for vqmmc.
Ok, Good.
Thanks,
Seungwon Jeon
-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators
Yuvaraj, On Fri, Jun 27, 2014 at 3:59 AM, Yuvaraj Kumar yuvaraj...@gmail.com wrote: mmc_regulator_set_ocr() is failing as its a fixed-regulator. Can you explain more about what's failing? It sure looks like mmc core is supposed to handle this given comments below /* * If we're using a fixed/static regulator, don't call * regulator_set_voltage; it would fail. */ tps65090 driver does not register through fixed regulator framework.It uses normal regulator framework and supports only enable/disable/is_enabled callbacks.Also it lacks certain callbacks for list_voltage, get_voltage ,set_voltage etc. [2.306476] dwmmc_exynos 1222.mmc: Failed getting OCR mask: -22 [2.393403] dwmmc_exynos 1222.mmc: could not set regulator OCR (-22) For the above reason,regulator framework treats fet4 as unused regulator and disables the vmmc regulator. Ah. Perhaps tps65090 needs to be fixed then... I'm not sure any other drivers cared before so maybe that's why it was never caught? -Doug -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators
Hi Doug,
On Thu, June 26, 2014, Doug Anderson wrote:
Seungwon,
On Wed, Jun 25, 2014 at 4:18 AM, Seungwon Jeon tgih@samsung.com wrote:
+ case MMC_POWER_ON:
+ if (!IS_ERR(mmc-supply.vqmmc)
+ !test_bit(DW_MMC_IO_POWERED,
slot-flags)) {
You can use regulator_is_enabled() instead of flag bit, DW_MMC_IO_POWERED.
I'd be a little worried about regulator_is_enabled() since regulators
are reference counted. What if someone else is sharing this
regulator? The regulator might happen to be enabled when you check it
but unless you add your own dw_mmc reference count they might turn it
off.
Cool, that's a possibility. Some assumption may need.
If mmc's core can guarantee its balance, I think we don't need to consider some
flag.
Thanks,
Seungwon Jeon
Important thing is that if powering vmmc failed at MMC_POWER_UP, vqmmc
should not be powered.
Good point.
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators
Doug
On Tue, Jun 24, 2014 at 11:30 PM, Doug Anderson diand...@chromium.org wrote:
Yuvaraj,
On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D yuvaraj...@gmail.com
wrote:
This patch makes use of mmc_regulator_get_supply() to handle
the vmmc and vqmmc regulators.Also it moves the code handling
the these regulators to dw_mci_set_ios().It turned on the vmmc
and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off
during MMC_POWER_OFF.
Signed-off-by: Yuvaraj Kumar C D yuvaraj...@samsung.com
---
drivers/mmc/host/dw_mmc.c | 71
++---
drivers/mmc/host/dw_mmc.h |2 ++
2 files changed, 36 insertions(+), 37 deletions(-)
Perhaps you could CC me on the whole series for the next version since
I was involved in privately reviewing previous versions?
It was just accidental missing you in the CC .Surely i will add you in
CC for next versions.
Overall caveat for my review is that I'm nowhere near an SD/MMC expert.
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 1ac227c..f5cabce 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct
mmc_ios *ios)
struct dw_mci_slot *slot = mmc_priv(mmc);
const struct dw_mci_drv_data *drv_data = slot-host-drv_data;
u32 regs;
+ int ret;
switch (ios-bus_width) {
case MMC_BUS_WIDTH_4:
@@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc,
struct mmc_ios *ios)
switch (ios-power_mode) {
case MMC_POWER_UP:
+ if ((!IS_ERR(mmc-supply.vmmc))
+ !test_bit(DW_MMC_CARD_POWERED,
slot-flags)) {
+ ret = regulator_enable(mmc-supply.vmmc);
+ if (!ret)
+ set_bit(DW_MMC_CARD_POWERED, slot-flags);
+ }
As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at
different times.
As you can see people's have different opinion on this.When i had a
look at the other drivers in the subsystem which does in the same flow
as above.However i will change in the next version.
Also: if you fail to turn on either of the regulators it feels like
you should print a pretty nasty error message since your device will
almost certainly not work.
Yes. I will add a error message.
set_bit(DW_MMC_CARD_NEED_INIT, slot-flags);
regs = mci_readl(slot-host, PWREN);
regs |= (1 slot-id);
mci_writel(slot-host, PWREN, regs);
break;
case MMC_POWER_OFF:
+ if (!IS_ERR(mmc-supply.vqmmc)
+ test_bit(DW_MMC_IO_POWERED, slot-flags)) {
+ ret = regulator_disable(mmc-supply.vqmmc);
+ if (!ret)
+ clear_bit(DW_MMC_IO_POWERED, slot-flags);
+ }
+ if (!IS_ERR(mmc-supply.vmmc)
+ test_bit(DW_MMC_CARD_POWERED, slot-flags))
{
+ ret = regulator_disable(mmc-supply.vmmc);
+ if (!ret)
+ clear_bit(DW_MMC_CARD_POWERED, slot-flags);
+ }
regs = mci_readl(slot-host, PWREN);
regs = ~(1 slot-id);
mci_writel(slot-host, PWREN, regs);
break;
+ case MMC_POWER_ON:
+ if (!IS_ERR(mmc-supply.vqmmc)
+ !test_bit(DW_MMC_IO_POWERED, slot-flags)) {
+ ret = regulator_enable(mmc-supply.vqmmc);
+ if (!ret)
+ set_bit(DW_MMC_IO_POWERED, slot-flags);
+ }
default:
break;
}
@@ -2067,7 +2093,13 @@ static int dw_mci_init_slot(struct dw_mci *host,
unsigned int id)
mmc-f_max = freq[1];
}
- mmc-ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
+ /*if there are external regulators, get them*/
+ ret = mmc_regulator_get_supply(mmc);
+ if (ret == -EPROBE_DEFER)
+ goto err_setup_bus;
+
+ if (!mmc-ocr_avail)
+ mmc-ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
if (host-pdata-caps)
mmc-caps = host-pdata-caps;
@@ -2133,7 +2165,7 @@ static int dw_mci_init_slot(struct dw_mci *host,
unsigned int id)
err_setup_bus:
mmc_free_host(mmc);
- return -EINVAL;
+ return ret;
}
static void dw_mci_cleanup_slot(struct dw_mci_slot *slot, unsigned int id)
@@ -2375,24 +2407,6 @@ int dw_mci_probe(struct dw_mci *host)
}
}
- host-vmmc = devm_regulator_get_optional(host-dev, vmmc);
- if (IS_ERR(host-vmmc)) {
- ret = PTR_ERR(host-vmmc);
- if (ret ==
Re: [PATCH 1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators
Yuvaraj,
On Thu, Jun 26, 2014 at 4:21 AM, Yuvaraj Kumar yuvaraj...@gmail.com wrote:
Doug
On Tue, Jun 24, 2014 at 11:30 PM, Doug Anderson diand...@chromium.org wrote:
Yuvaraj,
On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D yuvaraj...@gmail.com
wrote:
This patch makes use of mmc_regulator_get_supply() to handle
the vmmc and vqmmc regulators.Also it moves the code handling
the these regulators to dw_mci_set_ios().It turned on the vmmc
and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off
during MMC_POWER_OFF.
Signed-off-by: Yuvaraj Kumar C D yuvaraj...@samsung.com
---
drivers/mmc/host/dw_mmc.c | 71
++---
drivers/mmc/host/dw_mmc.h |2 ++
2 files changed, 36 insertions(+), 37 deletions(-)
Perhaps you could CC me on the whole series for the next version since
I was involved in privately reviewing previous versions?
It was just accidental missing you in the CC .Surely i will add you in
CC for next versions.
Overall caveat for my review is that I'm nowhere near an SD/MMC expert.
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 1ac227c..f5cabce 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct
mmc_ios *ios)
struct dw_mci_slot *slot = mmc_priv(mmc);
const struct dw_mci_drv_data *drv_data = slot-host-drv_data;
u32 regs;
+ int ret;
switch (ios-bus_width) {
case MMC_BUS_WIDTH_4:
@@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc,
struct mmc_ios *ios)
switch (ios-power_mode) {
case MMC_POWER_UP:
+ if ((!IS_ERR(mmc-supply.vmmc))
+ !test_bit(DW_MMC_CARD_POWERED,
slot-flags)) {
+ ret = regulator_enable(mmc-supply.vmmc);
+ if (!ret)
+ set_bit(DW_MMC_CARD_POWERED, slot-flags);
+ }
As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at
different times.
As you can see people's have different opinion on this.When i had a
look at the other drivers in the subsystem which does in the same flow
as above.However i will change in the next version.
Given my self proclaimed lack of SD/MMC knowledge, if others have a
good reason for doing them separate then you should do it that way.
So far I haven't heard that reason but I certainly could be wrong.
@@ -225,6 +225,8 @@ struct dw_mci_slot {
unsigned long flags;
#define DW_MMC_CARD_PRESENT0
#define DW_MMC_CARD_NEED_INIT 1
+#define DW_MMC_CARD_POWERED2
+#define DW_MMC_IO_POWERED 3
I don't really think you should have two bits here. From my
understanding of SD cards there should be very little reason to have
vqmmc and vmmc not powered at the same time.
I think if i can use mmc_regulator_set_ocr(), we don't need additional
flag.But for tps65090 mmc_regulator_get_ocr() and
mmc_regulator_set_ocr() is failing as its a fixed-regulator.
Can you explain more about what's failing? It sure looks like mmc
core is supposed to handle this given comments below
/*
* If we're using a fixed/static regulator, don't call
* regulator_set_voltage; it would fail.
*/
voltage = regulator_get_voltage(supply);
if (!regulator_can_change_voltage(supply))
min_uV = max_uV = voltage;
-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators
Seungwon,
On Thu, Jun 26, 2014 at 3:30 AM, Seungwon Jeon tgih@samsung.com wrote:
Hi Doug,
On Thu, June 26, 2014, Doug Anderson wrote:
Seungwon,
On Wed, Jun 25, 2014 at 4:18 AM, Seungwon Jeon tgih@samsung.com wrote:
+ case MMC_POWER_ON:
+ if (!IS_ERR(mmc-supply.vqmmc)
+ !test_bit(DW_MMC_IO_POWERED,
slot-flags)) {
You can use regulator_is_enabled() instead of flag bit, DW_MMC_IO_POWERED.
I'd be a little worried about regulator_is_enabled() since regulators
are reference counted. What if someone else is sharing this
regulator? The regulator might happen to be enabled when you check it
but unless you add your own dw_mmc reference count they might turn it
off.
Cool, that's a possibility. Some assumption may need.
If mmc's core can guarantee its balance, I think we don't need to consider
some flag.
I notice that the mmc core seems to keep a flag itself for vdd (the
mmc-regulator_enabled flag). That would imply that the core thought
it was important to have the extra flag and that we should keep our
own flag for vqmmc.
-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators
Hi Yuvaraj.
This patch looks like similar Jaehoon's.
Is it resending?
Anyway, I added some comments below.
On Wed, June 25, 2014, Jaehoon Chung wrote:
On 06/25/2014 03:00 AM, Doug Anderson wrote:
Yuvaraj,
On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D yuvaraj...@gmail.com
wrote:
This patch makes use of mmc_regulator_get_supply() to handle
the vmmc and vqmmc regulators.Also it moves the code handling
the these regulators to dw_mci_set_ios().It turned on the vmmc
and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off
during MMC_POWER_OFF.
Signed-off-by: Yuvaraj Kumar C D yuvaraj...@samsung.com
---
drivers/mmc/host/dw_mmc.c | 71
++---
drivers/mmc/host/dw_mmc.h |2 ++
2 files changed, 36 insertions(+), 37 deletions(-)
Perhaps you could CC me on the whole series for the next version since
I was involved in privately reviewing previous versions?
Overall caveat for my review is that I'm nowhere near an SD/MMC expert.
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 1ac227c..f5cabce 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc,
struct mmc_ios *ios)
struct dw_mci_slot *slot = mmc_priv(mmc);
const struct dw_mci_drv_data *drv_data = slot-host-drv_data;
u32 regs;
+ int ret;
switch (ios-bus_width) {
case MMC_BUS_WIDTH_4:
@@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc,
struct mmc_ios *ios)
switch (ios-power_mode) {
case MMC_POWER_UP:
+ if ((!IS_ERR(mmc-supply.vmmc))
+ !test_bit(DW_MMC_CARD_POWERED,
slot-flags)) {
+ ret = regulator_enable(mmc-supply.vmmc);
+ if (!ret)
+ set_bit(DW_MMC_CARD_POWERED, slot-flags);
MMC_CARD_POWERED? I didn't know why it needs.
Also, ios-vdd is missed. Use mmc_regulator_set_ocr() instead of
regulator_enable() for vmmc.
And with mmc_regulator_set_ocr(), we don't need to check additional flag for
enable/disable.
It does that already.
+ }
As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at
different times.
In my case, in order to prevent the H/W mis-operation, it turns on vqmmc and
vmmc at different times.
Also: if you fail to turn on either of the regulators it feels like
you should print a pretty nasty error message since your device will
almost certainly not work.
set_bit(DW_MMC_CARD_NEED_INIT, slot-flags);
regs = mci_readl(slot-host, PWREN);
regs |= (1 slot-id);
mci_writel(slot-host, PWREN, regs);
break;
case MMC_POWER_OFF:
+ if (!IS_ERR(mmc-supply.vqmmc)
+ test_bit(DW_MMC_IO_POWERED, slot-flags))
{
+ ret = regulator_disable(mmc-supply.vqmmc);
+ if (!ret)
+ clear_bit(DW_MMC_IO_POWERED, slot-flags);
+ }
+ if (!IS_ERR(mmc-supply.vmmc)
+ test_bit(DW_MMC_CARD_POWERED,
slot-flags)) {
+ ret = regulator_disable(mmc-supply.vmmc);
+ if (!ret)
+ clear_bit(DW_MMC_CARD_POWERED,
slot-flags);
+ }
regs = mci_readl(slot-host, PWREN);
regs = ~(1 slot-id);
mci_writel(slot-host, PWREN, regs);
break;
+ case MMC_POWER_ON:
+ if (!IS_ERR(mmc-supply.vqmmc)
+ !test_bit(DW_MMC_IO_POWERED,
slot-flags)) {
You can use regulator_is_enabled() instead of flag bit, DW_MMC_IO_POWERED.
Important thing is that if powering vmmc failed at MMC_POWER_UP, vqmmc should
not be powered.
Please consider Doug's mentions below.
+ ret = regulator_enable(mmc-supply.vqmmc);
+ if (!ret)
+ set_bit(DW_MMC_IO_POWERED, slot-flags);
+ }
default:
break;
}
@@ -2067,7 +2093,13 @@ static int dw_mci_init_slot(struct dw_mci *host,
unsigned int id)
mmc-f_max = freq[1];
}
- mmc-ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
+ /*if there are external regulators, get them*/
+ ret = mmc_regulator_get_supply(mmc);
+ if (ret == -EPROBE_DEFER)
+ goto err_setup_bus;
+
+ if (!mmc-ocr_avail)
+ mmc-ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
if (host-pdata-caps)
mmc-caps = host-pdata-caps;
@@ -2133,7 +2165,7 @@ static int
Re: [PATCH 1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators
Seungwon,
On Wed, Jun 25, 2014 at 4:18 AM, Seungwon Jeon tgih@samsung.com wrote:
+ case MMC_POWER_ON:
+ if (!IS_ERR(mmc-supply.vqmmc)
+ !test_bit(DW_MMC_IO_POWERED,
slot-flags)) {
You can use regulator_is_enabled() instead of flag bit, DW_MMC_IO_POWERED.
I'd be a little worried about regulator_is_enabled() since regulators
are reference counted. What if someone else is sharing this
regulator? The regulator might happen to be enabled when you check it
but unless you add your own dw_mmc reference count they might turn it
off.
Important thing is that if powering vmmc failed at MMC_POWER_UP, vqmmc should
not be powered.
Good point.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators
Yuvaraj,
On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D yuvaraj...@gmail.com wrote:
This patch makes use of mmc_regulator_get_supply() to handle
the vmmc and vqmmc regulators.Also it moves the code handling
the these regulators to dw_mci_set_ios().It turned on the vmmc
and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off
during MMC_POWER_OFF.
Signed-off-by: Yuvaraj Kumar C D yuvaraj...@samsung.com
---
drivers/mmc/host/dw_mmc.c | 71
++---
drivers/mmc/host/dw_mmc.h |2 ++
2 files changed, 36 insertions(+), 37 deletions(-)
Perhaps you could CC me on the whole series for the next version since
I was involved in privately reviewing previous versions?
Overall caveat for my review is that I'm nowhere near an SD/MMC expert.
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 1ac227c..f5cabce 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct
mmc_ios *ios)
struct dw_mci_slot *slot = mmc_priv(mmc);
const struct dw_mci_drv_data *drv_data = slot-host-drv_data;
u32 regs;
+ int ret;
switch (ios-bus_width) {
case MMC_BUS_WIDTH_4:
@@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct
mmc_ios *ios)
switch (ios-power_mode) {
case MMC_POWER_UP:
+ if ((!IS_ERR(mmc-supply.vmmc))
+ !test_bit(DW_MMC_CARD_POWERED, slot-flags))
{
+ ret = regulator_enable(mmc-supply.vmmc);
+ if (!ret)
+ set_bit(DW_MMC_CARD_POWERED, slot-flags);
+ }
As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at
different times.
Also: if you fail to turn on either of the regulators it feels like
you should print a pretty nasty error message since your device will
almost certainly not work.
set_bit(DW_MMC_CARD_NEED_INIT, slot-flags);
regs = mci_readl(slot-host, PWREN);
regs |= (1 slot-id);
mci_writel(slot-host, PWREN, regs);
break;
case MMC_POWER_OFF:
+ if (!IS_ERR(mmc-supply.vqmmc)
+ test_bit(DW_MMC_IO_POWERED, slot-flags)) {
+ ret = regulator_disable(mmc-supply.vqmmc);
+ if (!ret)
+ clear_bit(DW_MMC_IO_POWERED, slot-flags);
+ }
+ if (!IS_ERR(mmc-supply.vmmc)
+ test_bit(DW_MMC_CARD_POWERED, slot-flags)) {
+ ret = regulator_disable(mmc-supply.vmmc);
+ if (!ret)
+ clear_bit(DW_MMC_CARD_POWERED, slot-flags);
+ }
regs = mci_readl(slot-host, PWREN);
regs = ~(1 slot-id);
mci_writel(slot-host, PWREN, regs);
break;
+ case MMC_POWER_ON:
+ if (!IS_ERR(mmc-supply.vqmmc)
+ !test_bit(DW_MMC_IO_POWERED, slot-flags)) {
+ ret = regulator_enable(mmc-supply.vqmmc);
+ if (!ret)
+ set_bit(DW_MMC_IO_POWERED, slot-flags);
+ }
default:
break;
}
@@ -2067,7 +2093,13 @@ static int dw_mci_init_slot(struct dw_mci *host,
unsigned int id)
mmc-f_max = freq[1];
}
- mmc-ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
+ /*if there are external regulators, get them*/
+ ret = mmc_regulator_get_supply(mmc);
+ if (ret == -EPROBE_DEFER)
+ goto err_setup_bus;
+
+ if (!mmc-ocr_avail)
+ mmc-ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
if (host-pdata-caps)
mmc-caps = host-pdata-caps;
@@ -2133,7 +2165,7 @@ static int dw_mci_init_slot(struct dw_mci *host,
unsigned int id)
err_setup_bus:
mmc_free_host(mmc);
- return -EINVAL;
+ return ret;
}
static void dw_mci_cleanup_slot(struct dw_mci_slot *slot, unsigned int id)
@@ -2375,24 +2407,6 @@ int dw_mci_probe(struct dw_mci *host)
}
}
- host-vmmc = devm_regulator_get_optional(host-dev, vmmc);
- if (IS_ERR(host-vmmc)) {
- ret = PTR_ERR(host-vmmc);
- if (ret == -EPROBE_DEFER)
- goto err_clk_ciu;
-
- dev_info(host-dev, no vmmc regulator found: %d\n, ret);
- host-vmmc = NULL;
- } else {
- ret = regulator_enable(host-vmmc);
- if (ret) {
- if (ret != -EPROBE_DEFER)
- dev_err(host-dev,
-
Re: [PATCH 1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators
On 06/25/2014 03:00 AM, Doug Anderson wrote:
Yuvaraj,
On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D yuvaraj...@gmail.com
wrote:
This patch makes use of mmc_regulator_get_supply() to handle
the vmmc and vqmmc regulators.Also it moves the code handling
the these regulators to dw_mci_set_ios().It turned on the vmmc
and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off
during MMC_POWER_OFF.
Signed-off-by: Yuvaraj Kumar C D yuvaraj...@samsung.com
---
drivers/mmc/host/dw_mmc.c | 71
++---
drivers/mmc/host/dw_mmc.h |2 ++
2 files changed, 36 insertions(+), 37 deletions(-)
Perhaps you could CC me on the whole series for the next version since
I was involved in privately reviewing previous versions?
Overall caveat for my review is that I'm nowhere near an SD/MMC expert.
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 1ac227c..f5cabce 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct
mmc_ios *ios)
struct dw_mci_slot *slot = mmc_priv(mmc);
const struct dw_mci_drv_data *drv_data = slot-host-drv_data;
u32 regs;
+ int ret;
switch (ios-bus_width) {
case MMC_BUS_WIDTH_4:
@@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc,
struct mmc_ios *ios)
switch (ios-power_mode) {
case MMC_POWER_UP:
+ if ((!IS_ERR(mmc-supply.vmmc))
+ !test_bit(DW_MMC_CARD_POWERED,
slot-flags)) {
+ ret = regulator_enable(mmc-supply.vmmc);
+ if (!ret)
+ set_bit(DW_MMC_CARD_POWERED, slot-flags);
MMC_CARD_POWERED? I didn't know why it needs.
+ }
As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at
different times.
In my case, in order to prevent the H/W mis-operation, it turns on vqmmc and
vmmc at different times.
Also: if you fail to turn on either of the regulators it feels like
you should print a pretty nasty error message since your device will
almost certainly not work.
set_bit(DW_MMC_CARD_NEED_INIT, slot-flags);
regs = mci_readl(slot-host, PWREN);
regs |= (1 slot-id);
mci_writel(slot-host, PWREN, regs);
break;
case MMC_POWER_OFF:
+ if (!IS_ERR(mmc-supply.vqmmc)
+ test_bit(DW_MMC_IO_POWERED, slot-flags)) {
+ ret = regulator_disable(mmc-supply.vqmmc);
+ if (!ret)
+ clear_bit(DW_MMC_IO_POWERED, slot-flags);
+ }
+ if (!IS_ERR(mmc-supply.vmmc)
+ test_bit(DW_MMC_CARD_POWERED, slot-flags))
{
+ ret = regulator_disable(mmc-supply.vmmc);
+ if (!ret)
+ clear_bit(DW_MMC_CARD_POWERED, slot-flags);
+ }
regs = mci_readl(slot-host, PWREN);
regs = ~(1 slot-id);
mci_writel(slot-host, PWREN, regs);
break;
+ case MMC_POWER_ON:
+ if (!IS_ERR(mmc-supply.vqmmc)
+ !test_bit(DW_MMC_IO_POWERED, slot-flags)) {
+ ret = regulator_enable(mmc-supply.vqmmc);
+ if (!ret)
+ set_bit(DW_MMC_IO_POWERED, slot-flags);
+ }
default:
break;
}
@@ -2067,7 +2093,13 @@ static int dw_mci_init_slot(struct dw_mci *host,
unsigned int id)
mmc-f_max = freq[1];
}
- mmc-ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
+ /*if there are external regulators, get them*/
+ ret = mmc_regulator_get_supply(mmc);
+ if (ret == -EPROBE_DEFER)
+ goto err_setup_bus;
+
+ if (!mmc-ocr_avail)
+ mmc-ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
if (host-pdata-caps)
mmc-caps = host-pdata-caps;
@@ -2133,7 +2165,7 @@ static int dw_mci_init_slot(struct dw_mci *host,
unsigned int id)
err_setup_bus:
mmc_free_host(mmc);
- return -EINVAL;
+ return ret;
}
static void dw_mci_cleanup_slot(struct dw_mci_slot *slot, unsigned int id)
@@ -2375,24 +2407,6 @@ int dw_mci_probe(struct dw_mci *host)
}
}
- host-vmmc = devm_regulator_get_optional(host-dev, vmmc);
- if (IS_ERR(host-vmmc)) {
- ret = PTR_ERR(host-vmmc);
- if (ret == -EPROBE_DEFER)
- goto err_clk_ciu;
-
- dev_info(host-dev, no vmmc regulator found: %d\n, ret);
- host-vmmc = NULL;
- } else {
-
Re: [PATCH 1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators
Jaehoon,
On Tue, Jun 24, 2014 at 8:18 PM, Jaehoon Chung jh80.ch...@samsung.com wrote:
On 06/25/2014 03:00 AM, Doug Anderson wrote:
Yuvaraj,
On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D yuvaraj...@gmail.com
wrote:
This patch makes use of mmc_regulator_get_supply() to handle
the vmmc and vqmmc regulators.Also it moves the code handling
the these regulators to dw_mci_set_ios().It turned on the vmmc
and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off
during MMC_POWER_OFF.
Signed-off-by: Yuvaraj Kumar C D yuvaraj...@samsung.com
---
drivers/mmc/host/dw_mmc.c | 71
++---
drivers/mmc/host/dw_mmc.h |2 ++
2 files changed, 36 insertions(+), 37 deletions(-)
Perhaps you could CC me on the whole series for the next version since
I was involved in privately reviewing previous versions?
Overall caveat for my review is that I'm nowhere near an SD/MMC expert.
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 1ac227c..f5cabce 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct
mmc_ios *ios)
struct dw_mci_slot *slot = mmc_priv(mmc);
const struct dw_mci_drv_data *drv_data = slot-host-drv_data;
u32 regs;
+ int ret;
switch (ios-bus_width) {
case MMC_BUS_WIDTH_4:
@@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc,
struct mmc_ios *ios)
switch (ios-power_mode) {
case MMC_POWER_UP:
+ if ((!IS_ERR(mmc-supply.vmmc))
+ !test_bit(DW_MMC_CARD_POWERED,
slot-flags)) {
+ ret = regulator_enable(mmc-supply.vmmc);
+ if (!ret)
+ set_bit(DW_MMC_CARD_POWERED, slot-flags);
MMC_CARD_POWERED? I didn't know why it needs.
I think the idea was to make sure that regulator enables/disables were
balanced. ...but if the mmc core does this for us then we probably
don't need to worry.
+ }
As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at
different times.
In my case, in order to prevent the H/W mis-operation, it turns on vqmmc and
vmmc at different times.
OK! Can you explain more? What instance is one powered but not the other?
I know that we talked to an SD Card manufacturer who told us that
powering their IO lines without their main power rail was a bad thing
(and we in fact saw this causing problems on SD cards). I would guess
that means that you're either powering vmmc before vqmmc or that vqmmc
somehow doesn't directly enable pullups for IO lines.
If you need vmmc before vqmmc then I'm curious about why...
-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators
This patch makes use of mmc_regulator_get_supply() to handle
the vmmc and vqmmc regulators.Also it moves the code handling
the these regulators to dw_mci_set_ios().It turned on the vmmc
and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off
during MMC_POWER_OFF.
Signed-off-by: Yuvaraj Kumar C D yuvaraj...@samsung.com
---
drivers/mmc/host/dw_mmc.c | 71 ++---
drivers/mmc/host/dw_mmc.h |2 ++
2 files changed, 36 insertions(+), 37 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 1ac227c..f5cabce 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct
mmc_ios *ios)
struct dw_mci_slot *slot = mmc_priv(mmc);
const struct dw_mci_drv_data *drv_data = slot-host-drv_data;
u32 regs;
+ int ret;
switch (ios-bus_width) {
case MMC_BUS_WIDTH_4:
@@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct
mmc_ios *ios)
switch (ios-power_mode) {
case MMC_POWER_UP:
+ if ((!IS_ERR(mmc-supply.vmmc))
+ !test_bit(DW_MMC_CARD_POWERED, slot-flags)) {
+ ret = regulator_enable(mmc-supply.vmmc);
+ if (!ret)
+ set_bit(DW_MMC_CARD_POWERED, slot-flags);
+ }
set_bit(DW_MMC_CARD_NEED_INIT, slot-flags);
regs = mci_readl(slot-host, PWREN);
regs |= (1 slot-id);
mci_writel(slot-host, PWREN, regs);
break;
case MMC_POWER_OFF:
+ if (!IS_ERR(mmc-supply.vqmmc)
+ test_bit(DW_MMC_IO_POWERED, slot-flags)) {
+ ret = regulator_disable(mmc-supply.vqmmc);
+ if (!ret)
+ clear_bit(DW_MMC_IO_POWERED, slot-flags);
+ }
+ if (!IS_ERR(mmc-supply.vmmc)
+ test_bit(DW_MMC_CARD_POWERED, slot-flags)) {
+ ret = regulator_disable(mmc-supply.vmmc);
+ if (!ret)
+ clear_bit(DW_MMC_CARD_POWERED, slot-flags);
+ }
regs = mci_readl(slot-host, PWREN);
regs = ~(1 slot-id);
mci_writel(slot-host, PWREN, regs);
break;
+ case MMC_POWER_ON:
+ if (!IS_ERR(mmc-supply.vqmmc)
+ !test_bit(DW_MMC_IO_POWERED, slot-flags)) {
+ ret = regulator_enable(mmc-supply.vqmmc);
+ if (!ret)
+ set_bit(DW_MMC_IO_POWERED, slot-flags);
+ }
default:
break;
}
@@ -2067,7 +2093,13 @@ static int dw_mci_init_slot(struct dw_mci *host,
unsigned int id)
mmc-f_max = freq[1];
}
- mmc-ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
+ /*if there are external regulators, get them*/
+ ret = mmc_regulator_get_supply(mmc);
+ if (ret == -EPROBE_DEFER)
+ goto err_setup_bus;
+
+ if (!mmc-ocr_avail)
+ mmc-ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
if (host-pdata-caps)
mmc-caps = host-pdata-caps;
@@ -2133,7 +2165,7 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned
int id)
err_setup_bus:
mmc_free_host(mmc);
- return -EINVAL;
+ return ret;
}
static void dw_mci_cleanup_slot(struct dw_mci_slot *slot, unsigned int id)
@@ -2375,24 +2407,6 @@ int dw_mci_probe(struct dw_mci *host)
}
}
- host-vmmc = devm_regulator_get_optional(host-dev, vmmc);
- if (IS_ERR(host-vmmc)) {
- ret = PTR_ERR(host-vmmc);
- if (ret == -EPROBE_DEFER)
- goto err_clk_ciu;
-
- dev_info(host-dev, no vmmc regulator found: %d\n, ret);
- host-vmmc = NULL;
- } else {
- ret = regulator_enable(host-vmmc);
- if (ret) {
- if (ret != -EPROBE_DEFER)
- dev_err(host-dev,
- regulator_enable fail: %d\n, ret);
- goto err_clk_ciu;
- }
- }
-
host-quirks = host-pdata-quirks;
spin_lock_init(host-lock);
@@ -2536,8 +2550,6 @@ err_workqueue:
err_dmaunmap:
if (host-use_dma host-dma_ops-exit)
host-dma_ops-exit(host);
- if (host-vmmc)
- regulator_disable(host-vmmc);
err_clk_ciu:
if (!IS_ERR(host-ciu_clk))
@@ -2573,9 +2585,6 @@ void dw_mci_remove(struct dw_mci *host)
if (host-use_dma host-dma_ops-exit)
host-dma_ops-exit(host);
- if (host-vmmc)
- regulator_disable(host-vmmc);
-
