[2/2] mmc: dw_mmc: add resets support to dw_mmc

Message ID 1457254062-22677-2-git-send-email-guodong.xu@linaro.org
State New
Headers show

Commit Message

Guodong Xu March 6, 2016, 8:47 a.m.
mmc registers may in abnormal state if mmc is used in bootloader,
eg. to support booting from eMMC. So we need reset mmc registers
when kernel boots up, instead of assuming mmc is in clean state.

With this patch, user can add a 'resets' property into dw_mmc dts
node. When driver parse_dt and probe, it calls reset API to
deassert the 'reset' of dw_mmc host controller. When probe error or
remove, it calls reset API to assert it.

Please also refer to Documentation/devicetree/bindings/reset/reset.txt

Signed-off-by: Guodong Xu <guodong.xu@linaro.org>

Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>

---
 drivers/mmc/host/dw_mmc.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

zhangfei March 29, 2016, 8:23 a.m. | #1
On 03/29/2016 10:22 AM, Shawn Lin wrote:

>>

>>

>>

>>         +               else if (IS_ERR(host->pdata)) {

>>                                  dev_err(host->dev, "platform data not

>>         available\n");

>>                                  return -EINVAL;

>>                          }

>>         @@ -3012,6 +3022,9 @@ int dw_mci_probe(struct dw_mci *host)

>>                          }

>>                  }

>>

>>         +       if (host->pdata->rstc != NULL)

>>         +               reset_control_deassert(host->pdata->rstc);

>>         +

>>

>>

>>     sorry, I can't follow your intention here. Shouldn't it be something

>>     like "assert mmc -> may need delay -> deassert mmc". As your current

>>     code, nothing happend right?

Should be abstracted in reset driver.

>>

>>

>> The chip exits from bootloader with this bit asserted. And when entering

>> kernel, we only need to deassert.

>>

>> In my current code, the driver deassert mmc in _probe(), and assert mmc

>> in _remove().

>

> I catch your point. From the previous discussion, we add it to make sure

> dw_mmc in good state after leaving bootloader to kernel. But My real

> question is that you can assert it in  bootloader, so you can also

> dessert it in bootloaer to make sure dw_mmc work fine when probing

> in kernel. In that way, we don't need this patch?


uefi does not have exit point, and kernel may not assume mmc controller 
state is always correct when boot.
If Uefi need copy Image from mmc, mmc controller is in working state.
When jump to kernel, uefi mmc driver can not recover itself.
If kernel assume mmc controller state is clean, mmc will be in abnormal 
state.
Some controller will clear itself when set clock, however, hip660 does 
not, it need special register to access.


>

> More to think, Is it ok to match the behaviour of bootloader stage?

> My bootloader doesn't assert the reset pin of dw_mmc, so it seams if

> I want to fix you issue on kernel stage, I need a new round of

> assert->delay->deassert.


The process like delay (if required) should be abstracted in reset driver.
reset framework just export reset_control_assert/reset_control_deassert API.
Unfortunately not find clear description in Documentation/.
Suppose deassert is like start, while assert is like stop.

drivers/reset/core.c
reset_control_deassert - deasserts the reset line
reset_control_assert - asserts the reset line

More example:
drivers/mmc/host/sdhci-st.c
drivers/mmc/host/sunxi-mmc.c
drivers/usb/host/ohci-platform.c
drivers/i2c/busses/i2c-mv64xxx.c

Thanks

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
zhangfei March 30, 2016, 1:18 a.m. | #2
On 03/30/2016 08:46 AM, Shawn Lin wrote:
> 在 2016/3/29 16:23, zhangfei 写道:


>>> More to think, Is it ok to match the behaviour of bootloader stage?

>>> My bootloader doesn't assert the reset pin of dw_mmc, so it seams if

>>> I want to fix you issue on kernel stage, I need a new round of

>>> assert->delay->deassert.

>>

>> The process like delay (if required) should be abstracted in reset

>> driver.

>> reset framework just export reset_control_assert/reset_control_deassert

>> API.

>> Unfortunately not find clear description in Documentation/.

>> Suppose deassert is like start, while assert is like stop.

>>

>

> yes, no docs except dt-bindings..... So seems the usage of these two

> APIs depends on the implementation of reset controller driver

>

>> drivers/reset/core.c

>> reset_control_deassert - deasserts the reset line

>> reset_control_assert - asserts the reset line

>>

>> More example:

>> drivers/mmc/host/sdhci-st.c

>> drivers/mmc/host/sunxi-mmc.c

>> drivers/usb/host/ohci-platform.c

>> drivers/i2c/busses/i2c-mv64xxx.c

>

> But I'm afraid I have to nack here....

>

> Looking at these files:

> drivers/dma/stm32-dma.c

> drivers/net/ethernet/mediatek/mtk_eth_soc.c

> drivers/devfreq/tegra-devfreq.c

> drivers/crypto/rockchip/rk3288_crypto.c

> drivers/thermal/rockchip_thermal.c

> drivers/thermal/tegra_soctherm.c

> drivers/i2c/busses/i2c-tegra.c

> ....

>

> they just do the way like: assert->[delay](maybe abstracted)->deassert

> I think these drivers are vendor specific, so they can do

> the reset operations in consistent with the implementation of

> their platforms' reset controller drivers.


Yes, have checked drivers/i2c/busses/i2c-tegra.c
         reset_control_assert(i2c_dev->rst);
         udelay(2);
         reset_control_deassert(i2c_dev->rst);

This usage looks strange, reset does not mean gpio reset, it maybe 
register accessing.
I think all these three operation should be abstracted to deassert 
function, while cover the details for sharing.

However, not doc describing the assert/deassert behavior so causing 
confusion.

>

> But, dw_mmc is shared by many Socs. So It's not good to do it in

> dw_mci_probe, otherwise you force my reset controller driver to be

> implemented in the same way as yours..... Right?

>

> How about move it to your drv_data->init callback?

Yes, we can.
But firstly we should consider put in common file for sharing, otherwise 
there maybe many assert/deassert in dw_mmc-xx.c.

Guodong may send another version and wait for Jaehoon's decision.

Thanks
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 242f9a0..281ea9c 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2878,6 +2878,14 @@  static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
 	if (!pdata)
 		return ERR_PTR(-ENOMEM);
 
+	/* find reset controller when exist */
+	pdata->rstc = devm_reset_control_get_optional(dev, NULL);
+	if (IS_ERR(pdata->rstc)) {
+		if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
+			return ERR_PTR(-EPROBE_DEFER);
+		pdata->rstc = NULL;
+	}
+
 	/* find out number of slots supported */
 	of_property_read_u32(np, "num-slots", &pdata->num_slots);
 
@@ -2949,7 +2957,9 @@  int dw_mci_probe(struct dw_mci *host)
 
 	if (!host->pdata) {
 		host->pdata = dw_mci_parse_dt(host);
-		if (IS_ERR(host->pdata)) {
+		if (PTR_ERR(host->pdata) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		else if (IS_ERR(host->pdata)) {
 			dev_err(host->dev, "platform data not available\n");
 			return -EINVAL;
 		}
@@ -3012,6 +3022,9 @@  int dw_mci_probe(struct dw_mci *host)
 		}
 	}
 
+	if (host->pdata->rstc != NULL)
+		reset_control_deassert(host->pdata->rstc);
+
 	setup_timer(&host->cmd11_timer,
 		    dw_mci_cmd11_timer, (unsigned long)host);
 
@@ -3164,6 +3177,9 @@  err_dmaunmap:
 	if (host->use_dma && host->dma_ops->exit)
 		host->dma_ops->exit(host);
 
+	if (host->pdata->rstc != NULL)
+		reset_control_assert(host->pdata->rstc);
+
 err_clk_ciu:
 	if (!IS_ERR(host->ciu_clk))
 		clk_disable_unprepare(host->ciu_clk);
@@ -3196,11 +3212,15 @@  void dw_mci_remove(struct dw_mci *host)
 	if (host->use_dma && host->dma_ops->exit)
 		host->dma_ops->exit(host);
 
+	if (host->pdata->rstc != NULL)
+		reset_control_assert(host->pdata->rstc);
+
 	if (!IS_ERR(host->ciu_clk))
 		clk_disable_unprepare(host->ciu_clk);
 
 	if (!IS_ERR(host->biu_clk))
 		clk_disable_unprepare(host->biu_clk);
+
 }
 EXPORT_SYMBOL(dw_mci_remove);