mmc: sdhci: SDHCI controllers also need power

PATCH New Andy Shevchenko 4 participants 18 messages Mar 15, 2017 – Apr 23, 2017 MMC, SD and SDIO

lore

Share & tools

lore

Andy Shevchenko

author

2017-03-15 18:25 UTC #0 lore raw

PATCH 1/1 mmc: sdhci: SDHCI controllers also need power

On some systems SDHCI controllers may be powered off and it's required to bring them on before accessing.

SDHCI generic driver currently lacks any mean of doing it. Call the same board_power_mmc_init() at sdhci_init() stage.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/mmc/sdhci.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/mmc/sdhci.c +2 -0

index 93cefd89cd..54a7e379ff 100644
@@ -469,6 +469,8 @@ static int sdhci_init(struct mmc *mmc)
 {
 	struct sdhci_host *host = mmc->priv;
 
+	board_mmc_power_init();
+
 	sdhci_reset(host, SDHCI_RESET_ALL);
 
 	if ((host->quirks & SDHCI_QUIRK_32BIT_DMA_ADDR) && !aligned_buffer) {

Signature · 1 line

2.11.0

Andy Shevchenko

author

2017-03-20 12:51 UTC #2 lore raw

PATCH 1/1 mmc: sdhci: SDHCI controllers also need power

Quoted 25 lines · Simon Glass

Hi Andy,
Quoted 21 lines · Andy Shevchenko
On some systems SDHCI controllers may be powered off and it's required
to bring them on before accessing.
SDHCI generic driver currently lacks any mean of doing it. Call the same
board_power_mmc_init() at sdhci_init() stage.
Signed-off-by Andy Shevchenko <[email protected]>
---
drivers/mmc/sdhci.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/mmc/sdhci.c +0 -0
index 93cefd89cd..54a7e379ff 100644
@@ -469,6 +469,8 @@ static int sdhci_init(struct mmc *mmc)
  {
         struct sdhci_host *host = mmc->priv;
 

Quoted 3 lines

drivers/mmc/sdhci.c static int sdhci_init(struct mmc *mmc) · line 472
Quoted 1 line
drivers/mmc/sdhci.c +1 -0
+       board_mmc_power_init();
You should be using driver model for this (CONFIG_DM_MMC*).

I didn't get this part. It's used by the driver (tangier_sdhci) as far as I understand.

Quoted 2 lines

So either
get the power supply from the device tree and enable it,

Any example?

Quoted 2 lines

or do this in
the board code.

How? It's already board code that powers on the controller. If you look at mmc_init() it does this. SDHCI on the other hand doesn't which is for my opinion is a bug. Otherwise why is the difference between initialization sequence of MMC and SHDCI controllers?

Quoted 12 lines

Quoted 9 lines
drivers/mmc/sdhci.c +1 -0
+
         sdhci_reset(host, SDHCI_RESET_ALL);
 
if ((host->quirks & SDHCI_QUIRK_32BIT_DMA_ADDR) && !aligned_buffer) {
--
2.11.0
Regards,
Simon

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Simon Glass

2017-04-01 04:24 UTC #3 lore raw

PATCH 1/1 mmc: sdhci: SDHCI controllers also need power

Hi Andy,

Quoted 30 lines · Andy Shevchenko

Quoted 25 lines · Simon Glass
Hi Andy,
Quoted 21 lines · Andy Shevchenko
On some systems SDHCI controllers may be powered off and it's required
to bring them on before accessing.
SDHCI generic driver currently lacks any mean of doing it. Call the same
board_power_mmc_init() at sdhci_init() stage.
Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/mmc/sdhci.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/mmc/sdhci.c +0 -0
index 93cefd89cd..54a7e379ff 100644
@@ -469,6 +469,8 @@ static int sdhci_init(struct mmc *mmc)
 {
        struct sdhci_host *host = mmc->priv;
 
Quoted 3 lines
drivers/mmc/sdhci.c static int sdhci_init(struct mmc *mmc) · line 472
Quoted 1 line
drivers/mmc/sdhci.c +1 -0
+       board_mmc_power_init();
You should be using driver model for this (CONFIG_DM_MMC*).

Quoted 7 lines

I didn't get this part. It's used by the driver (tangier_sdhci) as far as I understand.
Quoted 2 lines
So either
get the power supply from the device tree and enable it,
Any example?

Well in mmc_power_init() there is:

ret = device_get_supply_regulator(mmc->dev, "vmmc-supply",
&vmmc_supply);

or say you have a node like this (from firefly-rk3288):

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>;
};

Then you can use regulator_get_by_platname("vcc_sd"...

Quoted 8 lines

Quoted 3 lines
or do this in
the board code.
How? It's already board code that powers on the controller. If you look at mmc_init() it does this. SDHCI on the other hand doesn't which is for my opinion is a bug. Otherwise why is the difference between initialization sequence of MMC and SHDCI controllers?

There should not really be a different I think, except that with driver model we want to use drivers for power rather than hard-coding things in custom code.

Quoted 9 lines

Quoted 9 lines
Quoted 9 lines
drivers/mmc/sdhci.c +1 -0
+
        sdhci_reset(host, SDHCI_RESET_ALL);
 
        if ((host->quirks & SDHCI_QUIRK_32BIT_DMA_ADDR) &&
!aligned_buffer) {
--
2.11.0

Regards,
Simon

Andy Shevchenko

author

2017-04-01 13:11 UTC #4 lore raw

PATCH 1/1 mmc: sdhci: SDHCI controllers also need power

Quoted 65 lines · Simon Glass

Hi Andy,
Quoted 33 lines · Andy Shevchenko
Quoted 28 lines · Simon Glass
Hi Andy,
Quoted 23 lines · Andy Shevchenko
On some systems SDHCI controllers may be powered off and it's required
to bring them on before accessing.
SDHCI generic driver currently lacks any mean of doing it. Call the
same
board_power_mmc_init() at sdhci_init() stage.
Signed-off-by Andy Shevchenko <[email protected]
m>
---
drivers/mmc/sdhci.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/mmc/sdhci.c +0 -0
index 93cefd89cd..54a7e379ff 100644
@@ -469,6 +469,8 @@ static int sdhci_init(struct mmc *mmc)
  {
         struct sdhci_host *host = mmc->priv;
 
Quoted 3 lines
drivers/mmc/sdhci.c static int sdhci_init(struct mmc *mmc) · line 472
Quoted 1 line
drivers/mmc/sdhci.c +1 -0
+       board_mmc_power_init();
You should be using driver model for this (CONFIG_DM_MMC*).
Quoted 8 lines
I didn't get this part. It's used by the driver (tangier_sdhci) as far
as I understand.
Quoted 2 lines
So either
get the power supply from the device tree and enable it,
Any example?
Well in mmc_power_init() there is:
ret = device_get_supply_regulator(mmc->dev, "vmmc-supply", &vmmc_supply);
or say you have a node like this (from firefly-rk3288):
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>;
};

Quoted 1 line

Then you can use regulator_get_by_platname("vcc_sd"...

Oh, we are talking about host controller's power management which is done using PMU (power management unit) inside SoC. It's *not* a power regulator.

Above is clearly about card power management, which we also have (in case of Wi-Fi), but it's not applicable for eMMC soldered on the module.

Quoted 10 lines

Quoted 9 lines
Quoted 2 lines
or do this in
the board code.
How? It's already board code that powers on the controller. If you look
at mmc_init() it does this. SDHCI on the other hand doesn't which is for
my opinion is a bug. Otherwise why is the difference between initialization sequence of MMC and SHDCI controllers?

Quoted 3 lines

There should not really be a different I think, except that with driver model we want to use drivers for power rather than hard-coding things in custom code.

I totally agree with this, though since we have no clear PCI implementation on that board (*) we can't have good described PCI power management for it.

(*) It's called "fake PCI" meaning it mimics PCI programming interface while being not 100% compatible with PCI specification on hardware and firmware levels.

So, for now I have been seeing no alternatives than my initial approach, though I'm all ears for better solution.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Simon Glass

2017-04-06 03:44 UTC #5 lore raw

PATCH 1/1 mmc: sdhci: SDHCI controllers also need power

Hi Andy,

Quoted 66 lines · Andy Shevchenko

Quoted 65 lines · Simon Glass
Hi Andy,
Quoted 33 lines · Andy Shevchenko
Quoted 28 lines · Simon Glass
Hi Andy,
Quoted 23 lines · Andy Shevchenko
On some systems SDHCI controllers may be powered off and it's required
to bring them on before accessing.
SDHCI generic driver currently lacks any mean of doing it. Call the
same
board_power_mmc_init() at sdhci_init() stage.
Signed-off-by Andy Shevchenko <[email protected]
m>
---
 drivers/mmc/sdhci.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/mmc/sdhci.c +0 -0
index 93cefd89cd..54a7e379ff 100644
@@ -469,6 +469,8 @@ static int sdhci_init(struct mmc *mmc)
 {
        struct sdhci_host *host = mmc->priv;
 
Quoted 3 lines
drivers/mmc/sdhci.c static int sdhci_init(struct mmc *mmc) · line 472
Quoted 1 line
drivers/mmc/sdhci.c +1 -0
+       board_mmc_power_init();
You should be using driver model for this (CONFIG_DM_MMC*).
Quoted 8 lines
I didn't get this part. It's used by the driver (tangier_sdhci) as far
as I understand.
Quoted 2 lines
So either
get the power supply from the device tree and enable it,
Any example?
Well in mmc_power_init() there is:
ret = device_get_supply_regulator(mmc->dev, "vmmc-supply",
&vmmc_supply);
or say you have a node like this (from firefly-rk3288):
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>;
};

Quoted 8 lines

Quoted 1 line
Then you can use regulator_get_by_platname("vcc_sd"...
Oh, we are talking about host controller's power management which is done using PMU (power management unit) inside SoC. It's *not* a power regulator.
Above is clearly about card power management, which we also have (in case of Wi-Fi), but it's not applicable for eMMC soldered on the module.

Still if the eMMC is soldered on, it needs power, right? What is the distinction?

In any case we cannot call board code from the driver with DM - it's just not how things work. So can you init it in your board_init() code perhaps, if you can't use a power driver?

Quoted 19 lines

Quoted 11 lines
Quoted 10 lines
Quoted 3 lines
or do this in
the board code.
How? It's already board code that powers on the controller. If you look
at mmc_init() it does this. SDHCI on the other hand doesn't which is for
my opinion is a bug. Otherwise why is the difference between initialization sequence of MMC and SHDCI controllers?
Quoted 3 lines
There should not really be a different I think, except that with driver model we want to use drivers for power rather than hard-coding things in custom code.
I totally agree with this, though since we have no clear PCI implementation on that board (*) we can't have good described PCI power management for it.

Quoted 6 lines

(*) It's called "fake PCI" meaning it mimics PCI programming interface while being not 100% compatible with PCI specification on hardware and firmware levels.
So, for now I have been seeing no alternatives than my initial approach, though I'm all ears for better solution.

Well you can create a regulator driver which has a single regulator to handle whatever needs doing to enable MMC power.

Regards,
Simon

Andy Shevchenko

2017-04-06 08:51 UTC #6 lore raw

PATCH 1/1 mmc: sdhci: SDHCI controllers also need power

Quoted 21 lines · Simon Glass

Quoted 19 lines · Andy Shevchenko
Quoted 6 lines · Simon Glass
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Simon Glass
On 15 March 2017 at 12:25, Andy Shevchenko
<[email protected]> wrote:
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
+ board_mmc_power_init();
You should be using driver model for this (CONFIG_DM_MMC*).
I didn't get this part. It's used by the driver (tangier_sdhci) as far
as I understand.
Oh, we are talking about host controller's power management which is done using PMU (power management unit) inside SoC. It's *not* a power regulator.

Quoted 5 lines

Quoted 2 lines
Above is clearly about card power management, which we also have (in case of Wi-Fi), but it's not applicable for eMMC soldered on the module.
Still if the eMMC is soldered on, it needs power, right? What is the distinction?

It's irrelevant to this patch and discussion.

Quoted 3 lines

In any case we cannot call board code from the driver with DM - it's just not how things work. So can you init it in your board_init() code perhaps, if you can't use a power driver?

I didn't get this either.

It means that PMU driver should *not* go with DM model then or what?

Quoted 22 lines

Quoted 22 lines
Quoted 10 lines
Quoted 9 lines
Quoted 2 lines
or do this in
the board code.
How? It's already board code that powers on the controller. If you look
at mmc_init() it does this. SDHCI on the other hand doesn't which is for
my opinion is a bug. Otherwise why is the difference between initialization sequence of MMC and SHDCI controllers?
Quoted 3 lines
There should not really be a different I think, except that with driver model we want to use drivers for power rather than hard-coding things in custom code.
I totally agree with this, though since we have no clear PCI implementation on that board (*) we can't have good described PCI power management for it.
(*) It's called "fake PCI" meaning it mimics PCI programming interface while being not 100% compatible with PCI specification on hardware and firmware levels.

Quoted 6 lines

Quoted 2 lines
So, for now I have been seeing no alternatives than my initial approach, though I'm all ears for better solution.
Well you can create a regulator driver which has a single regulator to handle whatever needs doing to enable MMC power.

No. It looks like you are mixing two power controls: card itself and host controller. They are using quite different mechanisms to be powered on.
We are talking here about *host* controller power flow.

And still there is no clarification why MMC flow calls board code and on the other hand you made an objectiion to do the same for SDHCI.

I still do not see better solution as mine initial one, otherwise above question should be clarified first.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Jaehoon Chung

2017-04-06 09:24 UTC #7 lore raw

PATCH 1/1 mmc: sdhci: SDHCI controllers also need power

Quoted 72 lines · Andy Shevchenko

Quoted 21 lines · Simon Glass
Quoted 19 lines · Andy Shevchenko
Quoted 6 lines · Simon Glass
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Simon Glass
On 15 March 2017 at 12:25, Andy Shevchenko
<[email protected]> wrote:
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
+ board_mmc_power_init();
You should be using driver model for this (CONFIG_DM_MMC*).
I didn't get this part. It's used by the driver (tangier_sdhci) as far
as I understand.
Oh, we are talking about host controller's power management which is done using PMU (power management unit) inside SoC. It's *not* a power regulator.
Quoted 5 lines
Quoted 2 lines
Above is clearly about card power management, which we also have (in case of Wi-Fi), but it's not applicable for eMMC soldered on the module.
Still if the eMMC is soldered on, it needs power, right? What is the distinction?
It's irrelevant to this patch and discussion.
Quoted 3 lines
In any case we cannot call board code from the driver with DM - it's just not how things work. So can you init it in your board_init() code perhaps, if you can't use a power driver?
I didn't get this either.
It means that PMU driver should *not* go with DM model then or what?
Quoted 22 lines
Quoted 22 lines
Quoted 10 lines
Quoted 9 lines
Quoted 2 lines
or do this in
the board code.
How? It's already board code that powers on the controller. If you look
at mmc_init() it does this. SDHCI on the other hand doesn't which is for
my opinion is a bug. Otherwise why is the difference between initialization sequence of MMC and SHDCI controllers?
Quoted 3 lines
There should not really be a different I think, except that with driver model we want to use drivers for power rather than hard-coding things in custom code.
I totally agree with this, though since we have no clear PCI implementation on that board (*) we can't have good described PCI power management for it.
(*) It's called "fake PCI" meaning it mimics PCI programming interface while being not 100% compatible with PCI specification on hardware and firmware levels.
Quoted 6 lines
Quoted 2 lines
So, for now I have been seeing no alternatives than my initial approach, though I'm all ears for better solution.
Well you can create a regulator driver which has a single regulator to handle whatever needs doing to enable MMC power.
No. It looks like you are mixing two power controls: card itself and host controller. They are using quite different mechanisms to be powered on.
We are talking here about *host* controller power flow.

Quoted 5 lines

And still there is no clarification why MMC flow calls board code and on the other hand you made an objectiion to do the same for SDHCI.
I still do not see better solution as mine initial one, otherwise above question should be clarified first.

how about mmc_power_init() is called in mmc_probe()?

Quoted 1 line

Andy Shevchenko

author

2017-04-06 09:46 UTC #8 lore raw

PATCH 1/1 mmc: sdhci: SDHCI controllers also need power

Quoted 87 lines · Jaehoon Chung

Quoted 86 lines · Andy Shevchenko
Quoted 22 lines · Simon Glass
Quoted 20 lines · Andy Shevchenko
Quoted 5 lines · Simon Glass
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Simon Glass
On 15 March 2017 at 12:25, Andy Shevchenko
<[email protected]> wrote:
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
+ board_mmc_power_init();
You should be using driver model for this
(CONFIG_DM_MMC*).
I didn't get this part. It's used by the driver
(tangier_sdhci) as
far
as I understand.
Oh, we are talking about host controller's power management which is
done using PMU (power management unit) inside SoC. It's *not* a power
regulator.
Quoted 8 lines
Quoted 4 lines
Above is clearly about card power management, which we also have (in
case of Wi-Fi), but it's not applicable for eMMC soldered on the module.
Still if the eMMC is soldered on, it needs power, right? What is the
distinction?
It's irrelevant to this patch and discussion.
Quoted 5 lines
In any case we cannot call board code from the driver with DM - it's
just not how things work. So can you init it in your board_init() code
perhaps, if you can't use a power driver?
I didn't get this either.
It means that PMU driver should *not* go with DM model then or what?
Quoted 29 lines
Quoted 29 lines
Quoted 12 lines
Quoted 11 lines
Quoted 2 lines
or do this in
the board code.
How? It's already board code that powers on the controller. If you
look
at mmc_init() it does this. SDHCI on the other hand doesn't which is
for
my opinion is a bug. Otherwise why is the difference between initialization sequence of MMC and SHDCI controllers?
Quoted 5 lines
There should not really be a different I think, except that with
driver model we want to use drivers for power rather than hard-coding
things in custom code.
I totally agree with this, though since we have no clear PCI implementation on that board (*) we can't have good described PCI power
management for it.
(*) It's called "fake PCI" meaning it mimics PCI programming interface
while being not 100% compatible with PCI specification on hardware and
firmware levels.
Quoted 6 lines
Quoted 3 lines
So, for now I have been seeing no alternatives than my initial approach,
though I'm all ears for better solution.
Well you can create a regulator driver which has a single regulator to
handle whatever needs doing to enable MMC power.
No. It looks like you are mixing two power controls: card itself and host controller. They are using quite different mechanisms to be powered on.
We are talking here about *host* controller power flow.

Quoted 8 lines

Quoted 6 lines
And still there is no clarification why MMC flow calls board code and
on the other hand you made an objectiion to do the same for SDHCI.
I still do not see better solution as mine initial one, otherwise above question should be clarified first.
how about mmc_power_init() is called in mmc_probe()?

Yes, that's what I'm referring to. But the driver is pure SDHCI, it doesn't call mmc_probe() IIRC.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Jaehoon Chung

2017-04-06 10:50 UTC #9 lore raw

PATCH 1/1 mmc: sdhci: SDHCI controllers also need power

Quoted 92 lines · Andy Shevchenko

Quoted 91 lines · Jaehoon Chung
Quoted 90 lines · Andy Shevchenko
Quoted 22 lines · Simon Glass
Quoted 20 lines · Andy Shevchenko
Quoted 5 lines · Simon Glass
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Simon Glass
On 15 March 2017 at 12:25, Andy Shevchenko
<[email protected]> wrote:
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
+ board_mmc_power_init();
You should be using driver model for this
(CONFIG_DM_MMC*).
I didn't get this part. It's used by the driver
(tangier_sdhci) as
far
as I understand.
Oh, we are talking about host controller's power management which is
done using PMU (power management unit) inside SoC. It's *not* a power
regulator.
Quoted 8 lines
Quoted 4 lines
Above is clearly about card power management, which we also have (in
case of Wi-Fi), but it's not applicable for eMMC soldered on the module.
Still if the eMMC is soldered on, it needs power, right? What is the
distinction?
It's irrelevant to this patch and discussion.
Quoted 5 lines
In any case we cannot call board code from the driver with DM - it's
just not how things work. So can you init it in your board_init() code
perhaps, if you can't use a power driver?
I didn't get this either.
It means that PMU driver should *not* go with DM model then or what?
Quoted 29 lines
Quoted 29 lines
Quoted 12 lines
Quoted 11 lines
Quoted 2 lines
or do this in
the board code.
How? It's already board code that powers on the controller. If you
look
at mmc_init() it does this. SDHCI on the other hand doesn't which is
for
my opinion is a bug. Otherwise why is the difference between initialization sequence of MMC and SHDCI controllers?
Quoted 5 lines
There should not really be a different I think, except that with
driver model we want to use drivers for power rather than hard-coding
things in custom code.
I totally agree with this, though since we have no clear PCI implementation on that board (*) we can't have good described PCI power
management for it.
(*) It's called "fake PCI" meaning it mimics PCI programming interface
while being not 100% compatible with PCI specification on hardware and
firmware levels.
Quoted 6 lines
Quoted 3 lines
So, for now I have been seeing no alternatives than my initial approach,
though I'm all ears for better solution.
Well you can create a regulator driver which has a single regulator to
handle whatever needs doing to enable MMC power.
No. It looks like you are mixing two power controls: card itself and host controller. They are using quite different mechanisms to be powered on.
We are talking here about *host* controller power flow.
And still there is no clarification why MMC flow calls board code and
on the other hand you made an objectiion to do the same for SDHCI.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
I still do not see better solution as mine initial one, otherwise above question should be clarified first.
how about mmc_power_init() is called in mmc_probe()?
Yes, that's what I'm referring to. But the driver is pure SDHCI, it doesn't call mmc_probe() IIRC.

After converting to DM, it might have the dependent to probing sequence. I'm not sure that u-boot has the priority for probing. maybe not...

hmm..need to consider this patch..but i will think about more generic solution..

Best Regards,
Jaehoon Chung

Quoted 1 line

Andy Shevchenko

2017-04-06 10:58 UTC #10 lore raw

PATCH 1/1 mmc: sdhci: SDHCI controllers also need power

Quoted 10 lines · Jaehoon Chung

Quoted 9 lines · Andy Shevchenko
Quoted 7 lines · Jaehoon Chung
Quoted 4 lines · Andy Shevchenko
Quoted 2 lines · Simon Glass
On 1 April 2017 at 07:11, Andy Shevchenko
<[email protected]> wrote:
how about mmc_power_init() is called in mmc_probe()?

Quoted 7 lines

Quoted 2 lines
Yes, that's what I'm referring to. But the driver is pure SDHCI, it doesn't call mmc_probe() IIRC.
After converting to DM, it might have the dependent to probing sequence. I'm not sure that u-boot has the priority for probing. maybe not...
hmm..need to consider this patch..but i will think about more generic solution..

It would be nice to have a generic solution indeed.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Jaehoon Chung

2017-04-07 10:05 UTC #11 lore raw

PATCH 1/1 mmc: sdhci: SDHCI controllers also need power

Hi Andy,

Quoted 14 lines · Andy Shevchenko

Quoted 13 lines · Jaehoon Chung
Quoted 11 lines · Andy Shevchenko
Quoted 7 lines · Jaehoon Chung
Quoted 4 lines · Andy Shevchenko
Quoted 2 lines · Simon Glass
On 1 April 2017 at 07:11, Andy Shevchenko
<[email protected]> wrote:
how about mmc_power_init() is called in mmc_probe()?
Yes, that's what I'm referring to. But the driver is pure SDHCI, it doesn't call mmc_probe() IIRC.

Quoted 6 lines

Quoted 4 lines
After converting to DM, it might have the dependent to probing sequence. I'm not sure that u-boot has the priority for probing. maybe not...
hmm..need to consider this patch..but i will think about more generic solution..
It would be nice to have a generic solution indeed.

Just thinking about below..?

vcc_sd: sdmmc-regulator {
	...
	regulator-boot-on;
or
	regulator-always-on;
	...

};

It should be always enabled..

Best Regards,
Jaehoon Chung

Quoted 1 line

Andy Shevchenko

author

2017-04-18 14:29 UTC #12 lore raw

PATCH 1/1 mmc: sdhci: SDHCI controllers also need power

Quoted 38 lines · Jaehoon Chung

Hi Andy,
Quoted 21 lines · Andy Shevchenko
Quoted 19 lines · Jaehoon Chung
Quoted 5 lines · Andy Shevchenko
Quoted 4 lines · Jaehoon Chung
Quoted 3 lines · Andy Shevchenko
On Thu, Apr 6, 2017 at 6:44 AM, Simon Glass <[email protected] g>
wrote:
Quoted 7 lines
Quoted 3 lines
Quoted 2 lines
Quoted 2 lines
On 1 April 2017 at 07:11, Andy Shevchenko
<[email protected]> wrote:
how about mmc_power_init() is called in mmc_probe()?
Yes, that's what I'm referring to. But the driver is pure SDHCI, it
doesn't call mmc_probe() IIRC.
After converting to DM, it might have the dependent to probing sequence.
I'm not sure that u-boot has the priority for probing. maybe not...
Quoted 4 lines
Quoted 2 lines
hmm..need to consider this patch..but i will think about more generic solution..
It would be nice to have a generic solution indeed.
Just thinking about below..?
vcc_sd: sdmmc-regulator {
	...
	regulator-boot-on;
or
	regulator-always-on;
	...

Quoted 3 lines

};
It should be always enabled..

Sorry, but no. It's not a regulator.

If you would like to know details, the 2 bits in PMU registers basically represent clock gate and reset signal per IP which PMU controls.

P.S. Hardware might have a common regulator per power island which is automatically latches the power down if all devices on the island are on D3hot. But it's not controlled by software.

So, please consider my initial approach.

This patch holds Edison enabling...

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Simon Glass

2017-04-18 14:33 UTC #13 lore raw

PATCH 1/1 mmc: sdhci: SDHCI controllers also need power

Hi Andy,

Quoted 43 lines · Andy Shevchenko

Quoted 38 lines · Jaehoon Chung
Hi Andy,
Quoted 21 lines · Andy Shevchenko
Quoted 19 lines · Jaehoon Chung
Quoted 5 lines · Andy Shevchenko
Quoted 4 lines · Jaehoon Chung
Quoted 3 lines · Andy Shevchenko
On Thu, Apr 6, 2017 at 6:44 AM, Simon Glass <[email protected] g>
wrote:
Quoted 7 lines
Quoted 3 lines
Quoted 2 lines
Quoted 2 lines
On 1 April 2017 at 07:11, Andy Shevchenko
<[email protected]> wrote:
how about mmc_power_init() is called in mmc_probe()?
Yes, that's what I'm referring to. But the driver is pure SDHCI, it
doesn't call mmc_probe() IIRC.
After converting to DM, it might have the dependent to probing sequence.
I'm not sure that u-boot has the priority for probing. maybe not...
Quoted 4 lines
Quoted 2 lines
hmm..need to consider this patch..but i will think about more generic solution..
It would be nice to have a generic solution indeed.
Just thinking about below..?
vcc_sd: sdmmc-regulator {
      ...
      regulator-boot-on;
or
      regulator-always-on;
      ...
Quoted 3 lines
};
It should be always enabled..

Quoted 8 lines

Sorry, but no. It's not a regulator.
If you would like to know details, the 2 bits in PMU registers basically represent clock gate and reset signal per IP which PMU controls.
P.S. Hardware might have a common regulator per power island which is automatically latches the power down if all devices on the island are on D3hot. But it's not controlled by software.

You have a few options:

- Add a regulator/pmic driver for the PMU
- Add a reset driver to handle the reset and perhaps a clock driver to handle the clock gate, then handle this in your driver

You can subclass sdhci.c and adjust it as you need it.

Quoted 2 lines

So, please consider my initial approach.

We should use DM rather than custom hooks. If this doesn't make sense please let me know how I can help expound on it.

Quoted 2 lines

This patch holds Edison enabling...

Then let's get it figured out!

Quoted 4 lines

--
Andy Shevchenko <[email protected]> Intel Finland Oy

Regards,
Simon

Andy Shevchenko

author

2017-04-18 14:45 UTC #14 lore raw

PATCH 1/1 mmc: sdhci: SDHCI controllers also need power

Quoted 64 lines · Simon Glass

Hi Andy,
Quoted 54 lines · Andy Shevchenko
Quoted 43 lines · Jaehoon Chung
Hi Andy,
Quoted 26 lines · Andy Shevchenko
Quoted 23 lines · Jaehoon Chung
Quoted 9 lines · Andy Shevchenko
Quoted 8 lines · Jaehoon Chung
Quoted 6 lines · Andy Shevchenko
On Thu, Apr 6, 2017 at 6:44 AM, Simon Glass <sjg@chromiu m.or
g>
Quoted 2 lines · wrote:
On 1 April 2017 at 07:11, Andy Shevchenko
<[email protected]> wrote:
Quoted 7 lines
Quoted 1 line
how about mmc_power_init() is called in mmc_probe()?
Yes, that's what I'm referring to. But the driver is pure SDHCI,
it
doesn't call mmc_probe() IIRC.
After converting to DM, it might have the dependent to probing sequence.
I'm not sure that u-boot has the priority for probing. maybe not...
Quoted 4 lines
Quoted 2 lines
hmm..need to consider this patch..but i will think about more generic solution..
It would be nice to have a generic solution indeed.
Just thinking about below..?
vcc_sd: sdmmc-regulator {
      ...
      regulator-boot-on;
or
      regulator-always-on;
      ...
Quoted 3 lines
};
It should be always enabled..
Sorry, but no. It's not a regulator.
If you would like to know details, the 2 bits in PMU registers basically
represent clock gate and reset signal per IP which PMU controls.
Quoted 5 lines
P.S. Hardware might have a common regulator per power island which is
automatically latches the power down if all devices on the island are on
D3hot. But it's not controlled by software.

Quoted 3 lines

You have a few options:
- Add a regulator/pmic driver for the PMU

I dunno how many times should I repeat that it is *not* a PMIC at all!

PMIC is a separate *external* IC which is connected to Atom SoC. And it has nothing to do with PMU (on software level).

Quoted 2 lines

- Add a reset driver to handle the reset and perhaps a clock driver to handle the clock gate, then handle this in your driver

No, I disclosed details just for your understanding that it's not a regulator. On the other hand it's 1:1 mapping to D0/D3hot in PCI, and bits can't be switched separately by specification.

TBH I even don't know which one is which.

Quoted 6 lines

You can subclass sdhci.c and adjust it as you need it.
Quoted 2 lines
So, please consider my initial approach.
We should use DM rather than custom hooks.

Can anyone answer to a simple question why MMC code *has* been calling such hook and you strongly object to do the same / similar for SDHCI?

Quoted 1 line

If this doesn't make sense

It does not.

Quoted 1 line

please let me know how I can help expound on it.

Please, elaborate how pure SDHCI drivers are so different to MMC in init stage and why, but please don't offer regulators.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Simon Glass

2017-04-19 00:12 UTC #15 lore raw

PATCH 1/1 mmc: sdhci: SDHCI controllers also need power

Hi Andy,

Quoted 65 lines · Andy Shevchenko

Quoted 64 lines · Simon Glass
Hi Andy,
Quoted 59 lines · Andy Shevchenko
Quoted 43 lines · Jaehoon Chung
Hi Andy,
Quoted 26 lines · Andy Shevchenko
Quoted 23 lines · Jaehoon Chung
Quoted 9 lines · Andy Shevchenko
Quoted 8 lines · Jaehoon Chung
Quoted 6 lines · Andy Shevchenko
On Thu, Apr 6, 2017 at 6:44 AM, Simon Glass <sjg@chromiu m.or
g>
Quoted 2 lines · wrote:
On 1 April 2017 at 07:11, Andy Shevchenko
<[email protected]> wrote:
Quoted 7 lines
Quoted 1 line
how about mmc_power_init() is called in mmc_probe()?
Yes, that's what I'm referring to. But the driver is pure SDHCI,
it
doesn't call mmc_probe() IIRC.
After converting to DM, it might have the dependent to probing sequence.
I'm not sure that u-boot has the priority for probing. maybe not...
Quoted 4 lines
Quoted 2 lines
hmm..need to consider this patch..but i will think about more generic solution..
It would be nice to have a generic solution indeed.
Just thinking about below..?
vcc_sd: sdmmc-regulator {
      ...
      regulator-boot-on;
or
      regulator-always-on;
      ...
Quoted 3 lines
};
It should be always enabled..
Sorry, but no. It's not a regulator.
If you would like to know details, the 2 bits in PMU registers basically
represent clock gate and reset signal per IP which PMU controls.
P.S. Hardware might have a common regulator per power island which is
automatically latches the power down if all devices on the island are on
D3hot. But it's not controlled by software.

Quoted 8 lines

Quoted 3 lines
You have a few options:
- Add a regulator/pmic driver for the PMU
I dunno how many times should I repeat that it is *not* a PMIC at all!
PMIC is a separate *external* IC which is connected to Atom SoC. And it has nothing to do with PMU (on software level).

That doesn't really matter though. The point is how it is modeled in U-Boot.

Quoted 13 lines

Quoted 3 lines
- Add a reset driver to handle the reset and perhaps a clock driver to handle the clock gate, then handle this in your driver
No, I disclosed details just for your understanding that it's not a regulator. On the other hand it's 1:1 mapping to D0/D3hot in PCI, and bits can't be switched separately by specification.
TBH I even don't know which one is which.
Quoted 3 lines
You can subclass sdhci.c and adjust it as you need it.
Quoted 1 line

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
So, please consider my initial approach.
We should use DM rather than custom hooks.
Can anyone answer to a simple question why MMC code *has* been calling such hook and you strongly object to do the same / similar for SDHCI?

Can you point me to the mmc function you are referring to?

Quoted 9 lines

Quoted 2 lines
If this doesn't make sense
It does not.
Quoted 1 line
please let me know how I can help expound on it.
Please, elaborate how pure SDHCI drivers are so different to MMC in init stage and why, but please don't offer regulators.

It's just that we cannot call a board hook function from DM. That's the way things used to work, but with DM we need to have things in the driver.

I'm sorry if you're finding this frustrating, but I do want to understand this. While it seems like a minor point it actually is a key design feature of DM.

Regards,
Simon

Andy Shevchenko

author

2017-04-19 11:50 UTC #16 lore raw

PATCH 1/1 mmc: sdhci: SDHCI controllers also need power

Quoted 82 lines · Simon Glass

Hi Andy,
Quoted 78 lines · Andy Shevchenko
Quoted 70 lines · Simon Glass
Hi Andy,
Quoted 58 lines · Andy Shevchenko
Quoted 47 lines · Jaehoon Chung
Hi Andy,
Quoted 29 lines · Andy Shevchenko
On Thu, Apr 6, 2017 at 1:50 PM, Jaehoon Chung <jh80.chung@sa msun
g.co
Quoted 25 lines · m> wrote:
Quoted 10 lines · Andy Shevchenko
Quoted 9 lines · Jaehoon Chung
Quoted 7 lines · Andy Shevchenko
On Thu, Apr 6, 2017 at 6:44 AM, Simon Glass <sjg@chr omiu
m.or
g>
Quoted 2 lines · wrote:
On 1 April 2017 at 07:11, Andy Shevchenko
<[email protected]> wrote:
Quoted 6 lines
Quoted 1 line
how about mmc_power_init() is called in mmc_probe()?
Yes, that's what I'm referring to. But the driver is pure
SDHCI,
it
doesn't call mmc_probe() IIRC.
After converting to DM, it might have the dependent to probing
sequence.
I'm not sure that u-boot has the priority for probing. maybe
not...
Quoted 5 lines
Quoted 3 lines
hmm..need to consider this patch..but i will think about more
generic solution..
It would be nice to have a generic solution indeed.
Just thinking about below..?
vcc_sd: sdmmc-regulator {
      ...
      regulator-boot-on;
or
      regulator-always-on;
      ...
Quoted 3 lines
};
It should be always enabled..
Sorry, but no. It's not a regulator.
If you would like to know details, the 2 bits in PMU registers basically
represent clock gate and reset signal per IP which PMU controls.
Quoted 7 lines
P.S. Hardware might have a common regulator per power island which
is
automatically latches the power down if all devices on the island
are on
D3hot. But it's not controlled by software.
Quoted 3 lines
You have a few options:
- Add a regulator/pmic driver for the PMU
I dunno how many times should I repeat that it is *not* a PMIC at all!

Quoted 6 lines

Quoted 3 lines
PMIC is a separate *external* IC which is connected to Atom SoC. And it
has nothing to do with PMU (on software level).
That doesn't really matter though. The point is how it is modeled in U-Boot.

Hardware matters. Software (drivers) represents whatever hardware design is underneath. This is how Linux kernel at least being designed. Does U- Boot follow the same paradigm?

Quoted 17 lines

Quoted 17 lines
Quoted 4 lines
- Add a reset driver to handle the reset and perhaps a clock driver to
handle the clock gate, then handle this in your driver
No, I disclosed details just for your understanding that it's not a regulator. On the other hand it's 1:1 mapping to D0/D3hot in PCI, and
bits can't be switched separately by specification.
TBH I even don't know which one is which.
Quoted 5 lines
You can subclass sdhci.c and adjust it as you need it.
Quoted 2 lines
So, please consider my initial approach.

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
We should use DM rather than custom hooks.
Can anyone answer to a simple question why MMC code *has* been calling
such hook and you strongly object to do the same / similar for SDHCI?
Can you point me to the mmc function you are referring to?

drivers/mmc/mmc.c:

-> (mmc-uclass.c) mmc_blk_probe()
-> mmc_init()

    -> mmc_start_init()
      -> mmc_power_init()
       -> board_mmc_power_init()

Quoted 11 lines

Quoted 9 lines
Quoted 1 line
If this doesn't make sense
It does not.
Quoted 1 line
please let me know how I can help expound on it.
Please, elaborate how pure SDHCI drivers are so different to MMC in init
stage and why, but please don't offer regulators.
It's just that we cannot call a board hook function from DM.

World is not ideal, and for me is clear that DM is not ideal either.

Quoted 7 lines

That's
the way things used to work, but with DM we need to have things in the driver.
I'm sorry if you're finding this frustrating, but I do want to understand this. While it seems like a minor point it actually is a key design feature of DM.

I understand your point. And I am all ears to implement the best of possible solutions (with current U-Boot design), OTOH I don't like any idea of faking in software something that is not present on real platform (like doing weird PMIC or regulators for PMU which is not either of them).

Please, understand my point as well.

The other option (not a good one from user experience) is to disable SD card slot in U-Boot completely.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Simon Glass

2017-04-23 01:18 UTC #17 lore raw

PATCH 1/1 mmc: sdhci: SDHCI controllers also need power

Hi Andy,

Quoted 87 lines · Andy Shevchenko

Quoted 86 lines · Simon Glass
Hi Andy,
Quoted 81 lines · Andy Shevchenko
Quoted 70 lines · Simon Glass
Hi Andy,
Quoted 58 lines · Andy Shevchenko
Quoted 47 lines · Jaehoon Chung
Hi Andy,
Quoted 29 lines · Andy Shevchenko
On Thu, Apr 6, 2017 at 1:50 PM, Jaehoon Chung <jh80.chung@sa msun
g.co
Quoted 25 lines · m> wrote:
Quoted 10 lines · Andy Shevchenko
Quoted 9 lines · Jaehoon Chung
Quoted 7 lines · Andy Shevchenko
On Thu, Apr 6, 2017 at 6:44 AM, Simon Glass <sjg@chr omiu
m.or
g>
Quoted 2 lines · wrote:
On 1 April 2017 at 07:11, Andy Shevchenko
<[email protected]> wrote:
Quoted 6 lines
Quoted 1 line
how about mmc_power_init() is called in mmc_probe()?
Yes, that's what I'm referring to. But the driver is pure
SDHCI,
it
doesn't call mmc_probe() IIRC.
After converting to DM, it might have the dependent to probing
sequence.
I'm not sure that u-boot has the priority for probing. maybe
not...
Quoted 5 lines
Quoted 3 lines
hmm..need to consider this patch..but i will think about more
generic solution..
It would be nice to have a generic solution indeed.
Just thinking about below..?
vcc_sd: sdmmc-regulator {
      ...
      regulator-boot-on;
or
      regulator-always-on;
      ...
Quoted 3 lines
};
It should be always enabled..
Sorry, but no. It's not a regulator.
If you would like to know details, the 2 bits in PMU registers basically
represent clock gate and reset signal per IP which PMU controls.
Quoted 7 lines
P.S. Hardware might have a common regulator per power island which
is
automatically latches the power down if all devices on the island
are on
D3hot. But it's not controlled by software.
Quoted 3 lines
You have a few options:
- Add a regulator/pmic driver for the PMU
I dunno how many times should I repeat that it is *not* a PMIC at all!
PMIC is a separate *external* IC which is connected to Atom SoC. And it
has nothing to do with PMU (on software level).

Quoted 6 lines

Quoted 2 lines
That doesn't really matter though. The point is how it is modeled in U-Boot.
Hardware matters. Software (drivers) represents whatever hardware design is underneath. This is how Linux kernel at least being designed. Does U- Boot follow the same paradigm?

I'm not sure what you are getting at. Is there a call to board_mmc_power_init() in the middle of the Linux MMC stack? It breaks the driver model. While U-Boot's driver model is perhaps a bit stricter than Linux, on this point they surely agree.

Quoted 27 lines

Quoted 18 lines
Quoted 18 lines
Quoted 5 lines
- Add a reset driver to handle the reset and perhaps a clock driver to
handle the clock gate, then handle this in your driver
No, I disclosed details just for your understanding that it's not a regulator. On the other hand it's 1:1 mapping to D0/D3hot in PCI, and
bits can't be switched separately by specification.
TBH I even don't know which one is which.
Quoted 5 lines
You can subclass sdhci.c and adjust it as you need it.
Quoted 2 lines
So, please consider my initial approach.
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
We should use DM rather than custom hooks.
Can anyone answer to a simple question why MMC code *has* been calling
such hook and you strongly object to do the same / similar for SDHCI?
Can you point me to the mmc function you are referring to?

Quoted 7 lines

drivers/mmc/mmc.c:
-> (mmc-uclass.c) mmc_blk_probe()
-> mmc_init()
    -> mmc_start_init()
      -> mmc_power_init()
       -> board_mmc_power_init()

If you look at mmc_power_init() you can see it already has the code to enable a regulator. This is what you should be using. I've just sent a patch to clarify that:

http://patchwork.ozlabs.org/patch/753851/

An alternative if you like is to enable the power in your board code before you even get to your MMC driver.

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
If this doesn't make sense
It does not.
Quoted 1 line
please let me know how I can help expound on it.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Please, elaborate how pure SDHCI drivers are so different to MMC in init
stage and why, but please don't offer regulators.
It's just that we cannot call a board hook function from DM.
World is not ideal, and for me is clear that DM is not ideal either.

Ideal in what sense?

Quoted 9 lines

Quoted 8 lines
That's
the way things used to work, but with DM we need to have things in the driver.
I'm sorry if you're finding this frustrating, but I do want to understand this. While it seems like a minor point it actually is a key design feature of DM.

Quoted 5 lines

I understand your point. And I am all ears to implement the best of possible solutions (with current U-Boot design), OTOH I don't like any idea of faking in software something that is not present on real platform (like doing weird PMIC or regulators for PMU which is not either of them).

What exactly is it, then? My understanding is that it is a bit in the register that controls power and reset for the MMC controller. Is that right? In that case, with driver model, we would use a regulator and a reset. If you really don't want to do that, then you can put the code in your board init e.g. in board_early_init_r().

Quoted 5 lines

Please, understand my point as well.
The other option (not a good one from user experience) is to disable SD card slot in U-Boot completely.

You could start with that, and then enable it later once you have the rest of the support in if you like.

I'm sorry that I being so inflexible here. I can tell that you are frustrated. But so far I cannot see a compelling reason to break the driver model approach here. While some boards have challenges I have not seen any big issues with dealing with power and reset with REGULATOR and RESET drivers.

Regards,
Simon