If mmc controler is accessed in bootloader, it's required to execute disable boot before resetting the controller & enabling the clock in kernel driver.
Signed-off-by: Haojian Zhuang haojian.zhuang@linaro.org --- .../devicetree/bindings/mmc/k3-dw-mshc.txt | 7 +++++ drivers/mmc/host/dw_mmc-k3.c | 33 ++++++++++++++++++++++ drivers/mmc/host/dw_mmc.h | 1 + 3 files changed, 41 insertions(+)
diff --git a/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt index 3b35449..cf12cfa 100644 --- a/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt +++ b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt @@ -14,6 +14,13 @@ Required Properties: * compatible: should be one of the following. - "hisilicon,hi4511-dw-mshc": for controllers with hi4511 specific extensions.
+Optional Properties: + +* "hisilicon,disable-boot" + - Disable boot before reseting mmc controller and enabling clocks. It's + required on Hisilicon Hi6220 SoC if the mmc controller is accessed in + bootloader. + Example:
/* for Hi3620 */ diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c index 83a9f49..3042a7c 100644 --- a/drivers/mmc/host/dw_mmc-k3.c +++ b/drivers/mmc/host/dw_mmc-k3.c @@ -11,6 +11,7 @@ #include <linux/module.h> #include <linux/platform_device.h> #include <linux/clk.h> +#include <linux/delay.h> #include <linux/mmc/host.h> #include <linux/mmc/dw_mmc.h> #include <linux/of_address.h> @@ -29,8 +30,40 @@ static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios *ios) host->bus_hz = clk_get_rate(host->ciu_clk); }
+static void disable_boot(struct dw_mci *host) +{ + int timeout = 0; + unsigned int data; + + mci_writel(host, CTRL, SDMMC_CTRL_FIFO_RESET); + mci_writel(host, CMD, SDMMC_CMD_START | SDMMC_CMD_DISABLE_BOOT); + + while (1) { + data = mci_readl(host, CMD) & 0x80000000; + if (data == 0) + break; + if (timeout < 2000) { + timeout++; + mdelay(1); + } else { + dev_warn(host->dev, "failed to stop MMC\n"); + break; + } + } +} + +static int dw_mci_k3_parse_dt(struct dw_mci *host) +{ + struct device_node *np = host->dev->of_node; + if (of_find_property(np, "hisilicon,disable-boot", NULL)) { + disable_boot(host); + } + return 0; +} + static const struct dw_mci_drv_data k3_drv_data = { .set_ios = dw_mci_k3_set_ios, + .parse_dt = dw_mci_k3_parse_dt, };
static const struct of_device_id dw_mci_k3_match[] = { diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h index 01b99e8..8c5977b 100644 --- a/drivers/mmc/host/dw_mmc.h +++ b/drivers/mmc/host/dw_mmc.h @@ -115,6 +115,7 @@ #define SDMMC_CMD_START BIT(31) #define SDMMC_CMD_USE_HOLD_REG BIT(29) #define SDMMC_CMD_VOLT_SWITCH BIT(28) +#define SDMMC_CMD_DISABLE_BOOT BIT(26) #define SDMMC_CMD_CCS_EXP BIT(23) #define SDMMC_CMD_CEATA_RD BIT(22) #define SDMMC_CMD_UPD_CLK BIT(21)
Add "hisilicon,disable-boot" property in mmc0. It's required before initialize mmc in kernel since the mmc controller is accessed in bootloader. This property could help to clean the setting of mmc controller in bootloader.
Signed-off-by: Haojian Zhuang haojian.zhuang@linaro.org --- arch/arm64/boot/dts/hi6220.dtsi | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/hi6220.dtsi b/arch/arm64/boot/dts/hi6220.dtsi index 00ff9b7..092664f 100644 --- a/arch/arm64/boot/dts/hi6220.dtsi +++ b/arch/arm64/boot/dts/hi6220.dtsi @@ -1001,6 +1001,7 @@ clocks = <&clock_sys HI6220_MMC0_CIUCLK>, <&clock_sys HI6220_MMC0_CLK>; clock-names = "ciu", "biu"; vmmc-supply = <&ldo19>; + hisilicon,disable-boot; };
dwmmc_1: dwmmc1@f723e000 {
On 17 March 2015 at 09:52, Haojian Zhuang haojian.zhuang@linaro.org wrote:
Add "hisilicon,disable-boot" property in mmc0. It's required before initialize mmc in kernel since the mmc controller is accessed in bootloader. This property could help to clean the setting of mmc controller in bootloader.
Signed-off-by: Haojian Zhuang haojian.zhuang@linaro.org
I've been using this patch for a few days now in 4.0rc and it works wonders, so:
Tested-by: Koen Kooi <koen.kooi@linaro.org.
I'm not convinced yet-another-DT-property is the right way for mainline acceptence, but it works.
arch/arm64/boot/dts/hi6220.dtsi | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/hi6220.dtsi b/arch/arm64/boot/dts/hi6220.dtsi index 00ff9b7..092664f 100644 --- a/arch/arm64/boot/dts/hi6220.dtsi +++ b/arch/arm64/boot/dts/hi6220.dtsi @@ -1001,6 +1001,7 @@ clocks = <&clock_sys HI6220_MMC0_CIUCLK>, <&clock_sys HI6220_MMC0_CLK>; clock-names = "ciu", "biu"; vmmc-supply = <&ldo19>;
hisilicon,disable-boot; }; dwmmc_1: dwmmc1@f723e000 {-- 1.9.1
On Tue, Mar 17, 2015 at 4:04 AM, Koen Kooi koen.kooi@linaro.org wrote:
On 17 March 2015 at 09:52, Haojian Zhuang haojian.zhuang@linaro.org wrote:
Add "hisilicon,disable-boot" property in mmc0. It's required before initialize mmc in kernel since the mmc controller is accessed in bootloader. This property could help to clean the setting of mmc controller in bootloader.
Signed-off-by: Haojian Zhuang haojian.zhuang@linaro.org
I've been using this patch for a few days now in 4.0rc and it works wonders, so:
Tested-by: Koen Kooi <koen.kooi@linaro.org.
I'm not convinced yet-another-DT-property is the right way for mainline acceptence, but it works.
Certainly not. Why does this need to be conditional? The kernel needs to handle any initial state. What would happen with kexec of a 2nd kernel for example?
Rob
On 17 March 2015 at 09:52, Haojian Zhuang haojian.zhuang@linaro.org wrote:
If mmc controler is accessed in bootloader, it's required to execute disable boot before resetting the controller & enabling the clock in kernel driver.
Signed-off-by: Haojian Zhuang haojian.zhuang@linaro.org
I've been using this patch for a few days now in 4.0rc and it works wonders, so:
Tested-by: Koen Kooi <koen.kooi@linaro.org.
I'm not convinced yet-another-DT-property is the right way for mainline acceptence, but it works.
.../devicetree/bindings/mmc/k3-dw-mshc.txt | 7 +++++ drivers/mmc/host/dw_mmc-k3.c | 33 ++++++++++++++++++++++ drivers/mmc/host/dw_mmc.h | 1 + 3 files changed, 41 insertions(+)
diff --git a/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt index 3b35449..cf12cfa 100644 --- a/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt +++ b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt @@ -14,6 +14,13 @@ Required Properties:
- compatible: should be one of the following.
- "hisilicon,hi4511-dw-mshc": for controllers with hi4511 specific extensions.
+Optional Properties:
+* "hisilicon,disable-boot"
- Disable boot before reseting mmc controller and enabling clocks. It's
- required on Hisilicon Hi6220 SoC if the mmc controller is accessed in
- bootloader.
Example:
/* for Hi3620 */diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c index 83a9f49..3042a7c 100644 --- a/drivers/mmc/host/dw_mmc-k3.c +++ b/drivers/mmc/host/dw_mmc-k3.c @@ -11,6 +11,7 @@ #include <linux/module.h> #include <linux/platform_device.h> #include <linux/clk.h> +#include <linux/delay.h> #include <linux/mmc/host.h> #include <linux/mmc/dw_mmc.h> #include <linux/of_address.h> @@ -29,8 +30,40 @@ static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios *ios) host->bus_hz = clk_get_rate(host->ciu_clk); }
+static void disable_boot(struct dw_mci *host) +{
int timeout = 0;unsigned int data;mci_writel(host, CTRL, SDMMC_CTRL_FIFO_RESET);mci_writel(host, CMD, SDMMC_CMD_START | SDMMC_CMD_DISABLE_BOOT);while (1) {data = mci_readl(host, CMD) & 0x80000000;if (data == 0)break;if (timeout < 2000) {timeout++;mdelay(1);} else {dev_warn(host->dev, "failed to stop MMC\n");break;}}+}
+static int dw_mci_k3_parse_dt(struct dw_mci *host) +{
struct device_node *np = host->dev->of_node;if (of_find_property(np, "hisilicon,disable-boot", NULL)) {disable_boot(host);}return 0;+}
static const struct dw_mci_drv_data k3_drv_data = { .set_ios = dw_mci_k3_set_ios,
.parse_dt = dw_mci_k3_parse_dt,};
static const struct of_device_id dw_mci_k3_match[] = { diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h index 01b99e8..8c5977b 100644 --- a/drivers/mmc/host/dw_mmc.h +++ b/drivers/mmc/host/dw_mmc.h @@ -115,6 +115,7 @@ #define SDMMC_CMD_START BIT(31) #define SDMMC_CMD_USE_HOLD_REG BIT(29) #define SDMMC_CMD_VOLT_SWITCH BIT(28) +#define SDMMC_CMD_DISABLE_BOOT BIT(26) #define SDMMC_CMD_CCS_EXP BIT(23) #define SDMMC_CMD_CEATA_RD BIT(22)
#define SDMMC_CMD_UPD_CLK BIT(21)
1.9.1
On Tue, Mar 17, 2015 at 09:04:43AM +0000, Haojian Zhuang wrote:
+static void disable_boot(struct dw_mci *host) +{
- int timeout = 0;
- unsigned int data;
- mci_writel(host, CTRL, SDMMC_CTRL_FIFO_RESET);
- mci_writel(host, CMD, SDMMC_CMD_START | SDMMC_CMD_DISABLE_BOOT);
- while (1) {
data = mci_readl(host, CMD) & 0x80000000;if (data == 0)break;
This looks like something generic which other systems might be able to use and not specific to the HiSilicon SoC - does this belong in either the Designware code or the subsystem code?
On 17 March 2015 at 17:59, Mark Brown broonie@kernel.org wrote:
On Tue, Mar 17, 2015 at 09:04:43AM +0000, Haojian Zhuang wrote:
+static void disable_boot(struct dw_mci *host) +{
int timeout = 0;unsigned int data;mci_writel(host, CTRL, SDMMC_CTRL_FIFO_RESET);mci_writel(host, CMD, SDMMC_CMD_START | SDMMC_CMD_DISABLE_BOOT);while (1) {data = mci_readl(host, CMD) & 0x80000000;if (data == 0)break;This looks like something generic which other systems might be able to use and not specific to the HiSilicon SoC - does this belong in either the Designware code or the subsystem code?
But I have neither Designware spec nor designware code. I only got this code from Hisilicon.
Could any guy help to check whether this piece of code is in original designware code?
Best Regards Haojian
Hi Haojian,
On 17 March 2015 at 13:36, Haojian Zhuang haojian.zhuang@linaro.org wrote:
On 17 March 2015 at 17:59, Mark Brown broonie@kernel.org wrote:
On Tue, Mar 17, 2015 at 09:04:43AM +0000, Haojian Zhuang wrote:
+static void disable_boot(struct dw_mci *host) +{
int timeout = 0;unsigned int data;mci_writel(host, CTRL, SDMMC_CTRL_FIFO_RESET);mci_writel(host, CMD, SDMMC_CMD_START | SDMMC_CMD_DISABLE_BOOT);while (1) {data = mci_readl(host, CMD) & 0x80000000;if (data == 0)break;This looks like something generic which other systems might be able to use and not specific to the HiSilicon SoC - does this belong in either the Designware code or the subsystem code?
But I have neither Designware spec nor designware code. I only got this code from Hisilicon.
Could any guy help to check whether this piece of code is in original designware code?
At this 96boards list we can test patches on hikey and provide general mainlining advice. I think the right people to discuss the clean way to implement this are the linux-mmc@vger.kernel.org mailing list, where chances are some people with access to designware docs are able to answer.
For Ulf, since Mark clipped most of the patch, the full patch discussed is at:
https://lists.96boards.org/pipermail/dev/2015-March/000092.html
Riku
On Tue, Mar 17, 2015 at 08:59:10AM +0000, Haojian Zhuang wrote:
If mmc controler is accessed in bootloader, it's required to execute disable boot before resetting the controller & enabling the clock in kernel driver.
Signed-off-by: Haojian Zhuang haojian.zhuang@linaro.org
.../devicetree/bindings/mmc/k3-dw-mshc.txt | 7 +++++ drivers/mmc/host/dw_mmc-k3.c | 33 ++++++++++++++++++++++ drivers/mmc/host/dw_mmc.h | 1 + 3 files changed, 41 insertions(+)
diff --git a/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt index 3b35449..cf12cfa 100644 --- a/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt +++ b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt @@ -14,6 +14,13 @@ Required Properties:
- compatible: should be one of the following.
- "hisilicon,hi4511-dw-mshc": for controllers with hi4511 specific extensions.
+Optional Properties:
+* "hisilicon,disable-boot"
- Disable boot before reseting mmc controller and enabling clocks. It's
- required on Hisilicon Hi6220 SoC if the mmc controller is accessed in
- bootloader.
I don't follow. Why do we need another property?
What exactly does this cause to happen at the MMC controller prior to the reset, why exactly is this necessary, and why can we not always do this regardless?
That patch looks like we're poking the device prior to resetting it, which seems completely backwards.
Also, the description above seems to rely on the kernel behaviour w.r.t. the reset and clocking, which isn't great.
Mark.
Example: /* for Hi3620 */ diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c index 83a9f49..3042a7c 100644 --- a/drivers/mmc/host/dw_mmc-k3.c +++ b/drivers/mmc/host/dw_mmc-k3.c @@ -11,6 +11,7 @@ #include <linux/module.h> #include <linux/platform_device.h> #include <linux/clk.h> +#include <linux/delay.h> #include <linux/mmc/host.h> #include <linux/mmc/dw_mmc.h> #include <linux/of_address.h> @@ -29,8 +30,40 @@ static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios *ios) host->bus_hz = clk_get_rate(host->ciu_clk); } +static void disable_boot(struct dw_mci *host) +{
- int timeout = 0;
- unsigned int data;
- mci_writel(host, CTRL, SDMMC_CTRL_FIFO_RESET);
- mci_writel(host, CMD, SDMMC_CMD_START | SDMMC_CMD_DISABLE_BOOT);
- while (1) {
data = mci_readl(host, CMD) & 0x80000000;if (data == 0)break;if (timeout < 2000) {timeout++;mdelay(1);} else {dev_warn(host->dev, "failed to stop MMC\n");break;}- }
+}
+static int dw_mci_k3_parse_dt(struct dw_mci *host) +{
- struct device_node *np = host->dev->of_node;
- if (of_find_property(np, "hisilicon,disable-boot", NULL)) {
disable_boot(host);- }
- return 0;
+}
static const struct dw_mci_drv_data k3_drv_data = { .set_ios = dw_mci_k3_set_ios,
- .parse_dt = dw_mci_k3_parse_dt,
}; static const struct of_device_id dw_mci_k3_match[] = { diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h index 01b99e8..8c5977b 100644 --- a/drivers/mmc/host/dw_mmc.h +++ b/drivers/mmc/host/dw_mmc.h @@ -115,6 +115,7 @@ #define SDMMC_CMD_START BIT(31) #define SDMMC_CMD_USE_HOLD_REG BIT(29) #define SDMMC_CMD_VOLT_SWITCH BIT(28) +#define SDMMC_CMD_DISABLE_BOOT BIT(26) #define SDMMC_CMD_CCS_EXP BIT(23) #define SDMMC_CMD_CEATA_RD BIT(22)
#define SDMMC_CMD_UPD_CLK BIT(21)
1.9.1
Dev mailing list Dev@lists.96boards.org https://lists.96boards.org/mailman/listinfo/dev
On 03/17/2015 09:34 AM, Mark Rutland wrote:
On Tue, Mar 17, 2015 at 08:59:10AM +0000, Haojian Zhuang wrote:
If mmc controler is accessed in bootloader, it's required to execute disable boot before resetting the controller & enabling the clock in kernel driver.
Signed-off-by: Haojian Zhuang haojian.zhuang@linaro.org
.../devicetree/bindings/mmc/k3-dw-mshc.txt | 7 +++++ drivers/mmc/host/dw_mmc-k3.c | 33 ++++++++++++++++++++++ drivers/mmc/host/dw_mmc.h | 1 + 3 files changed, 41 insertions(+)
diff --git a/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt index 3b35449..cf12cfa 100644 --- a/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt +++ b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt @@ -14,6 +14,13 @@ Required Properties:
- compatible: should be one of the following.
- "hisilicon,hi4511-dw-mshc": for controllers with hi4511 specific extensions.
+Optional Properties:
+* "hisilicon,disable-boot"
- Disable boot before reseting mmc controller and enabling clocks. It's
- required on Hisilicon Hi6220 SoC if the mmc controller is accessed in
- bootloader.
I don't follow. Why do we need another property?
What exactly does this cause to happen at the MMC controller prior to the reset, why exactly is this necessary,
are you asking for documentation about the controller?
and why can we not always do this regardless?
+1
That patch looks like we're poking the device prior to resetting it, which seems completely backwards.
yes, some background info can be found here https://github.com/96boards/bugs/issues/19
it seems that UEFI accesses during boot might have left the device in an unsuitable state for the kernel. this commit aims at correcting that.
Also, the description above seems to rely on the kernel behaviour w.r.t. the reset and clocking, which isn't great.
Mark.
Example: /* for Hi3620 */ diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c index 83a9f49..3042a7c 100644 --- a/drivers/mmc/host/dw_mmc-k3.c +++ b/drivers/mmc/host/dw_mmc-k3.c @@ -11,6 +11,7 @@ #include <linux/module.h> #include <linux/platform_device.h> #include <linux/clk.h> +#include <linux/delay.h> #include <linux/mmc/host.h> #include <linux/mmc/dw_mmc.h> #include <linux/of_address.h> @@ -29,8 +30,40 @@ static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios *ios) host->bus_hz = clk_get_rate(host->ciu_clk); } +static void disable_boot(struct dw_mci *host) +{
- int timeout = 0;
- unsigned int data;
- mci_writel(host, CTRL, SDMMC_CTRL_FIFO_RESET);
- mci_writel(host, CMD, SDMMC_CMD_START | SDMMC_CMD_DISABLE_BOOT);
- while (1) {
data = mci_readl(host, CMD) & 0x80000000;if (data == 0)break;if (timeout < 2000) {timeout++;mdelay(1);} else {dev_warn(host->dev, "failed to stop MMC\n");break;}- }
+}
+static int dw_mci_k3_parse_dt(struct dw_mci *host) +{
- struct device_node *np = host->dev->of_node;
- if (of_find_property(np, "hisilicon,disable-boot", NULL)) {
disable_boot(host);- }
- return 0;
+}
static const struct dw_mci_drv_data k3_drv_data = { .set_ios = dw_mci_k3_set_ios,
- .parse_dt = dw_mci_k3_parse_dt,
}; static const struct of_device_id dw_mci_k3_match[] = { diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h index 01b99e8..8c5977b 100644 --- a/drivers/mmc/host/dw_mmc.h +++ b/drivers/mmc/host/dw_mmc.h @@ -115,6 +115,7 @@ #define SDMMC_CMD_START BIT(31) #define SDMMC_CMD_USE_HOLD_REG BIT(29) #define SDMMC_CMD_VOLT_SWITCH BIT(28) +#define SDMMC_CMD_DISABLE_BOOT BIT(26) #define SDMMC_CMD_CCS_EXP BIT(23) #define SDMMC_CMD_CEATA_RD BIT(22)
#define SDMMC_CMD_UPD_CLK BIT(21)
1.9.1
Dev mailing list Dev@lists.96boards.org https://lists.96boards.org/mailman/listinfo/dev
Dev mailing list Dev@lists.96boards.org https://lists.96boards.org/mailman/listinfo/dev
On Tue, Mar 17, 2015 at 02:27:25PM +0000, Jorge Ramirez-Ortiz wrote:
On 03/17/2015 09:34 AM, Mark Rutland wrote:
That patch looks like we're poking the device prior to resetting it, which seems completely backwards.
yes, some background info can be found here https://github.com/96boards/bugs/issues/19
it seems that UEFI accesses during boot might have left the device in an unsuitable state for the kernel. this commit aims at correcting that.
Or to put it another way it's trying to do a more thorough and careful job of resetting the hardware than we're doing at the minute.
On Tue, Mar 17, 2015 at 02:20:48PM +0000, Jorge Ramirez-Ortiz wrote:
On 03/17/2015 09:34 AM, Mark Rutland wrote:
On Tue, Mar 17, 2015 at 08:59:10AM +0000, Haojian Zhuang wrote:
If mmc controler is accessed in bootloader, it's required to execute disable boot before resetting the controller & enabling the clock in kernel driver.
Signed-off-by: Haojian Zhuang haojian.zhuang@linaro.org
.../devicetree/bindings/mmc/k3-dw-mshc.txt | 7 +++++ drivers/mmc/host/dw_mmc-k3.c | 33 ++++++++++++++++++++++ drivers/mmc/host/dw_mmc.h | 1 + 3 files changed, 41 insertions(+)
diff --git a/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt index 3b35449..cf12cfa 100644 --- a/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt +++ b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt @@ -14,6 +14,13 @@ Required Properties:
- compatible: should be one of the following.
- "hisilicon,hi4511-dw-mshc": for controllers with hi4511 specific extensions.
+Optional Properties:
+* "hisilicon,disable-boot"
- Disable boot before reseting mmc controller and enabling clocks. It's
- required on Hisilicon Hi6220 SoC if the mmc controller is accessed in
- bootloader.
I don't follow. Why do we need another property?
What exactly does this cause to happen at the MMC controller prior to the reset, why exactly is this necessary,
are you asking for documentation about the controller?
I'm asking for the semantics associated with this property rather than for a full TRM.
Realistically what I'd like to know is why this is necessary prior to the reset of the controller mentioned in the property description. From the sounds of things we're not actually resetting the controller.
I also suspect that "disable boot" is a misnomer both in the proeprty and code.
and why can we not always do this regardless?
+1
That patch looks like we're poking the device prior to resetting it, which seems completely backwards.
yes, some background info can be found here https://github.com/96boards/bugs/issues/19
it seems that UEFI accesses during boot might have left the device in an unsuitable state for the kernel. this commit aims at correcting that.
It would be nice to know what that state is. I've been bitten by loaders leaving devices on with DMA active, and it would be nice to know that something like that isn't the case here -- it can be tricky to detect until it's too late.
I was under the impression that UEFI drivers were meant to leave devices in some quiescent state. Sepending on the precise state the HW is left it, this may still be a UEFI bug regardless of whether it's possible for the kernel to fix things up, and if that's the case we should fix the UEFI port regardless of whether we also modify the kernel to handle that state.
Mark.
On Tue, Mar 17, 2015 at 03:08:52PM +0000, Mark Rutland wrote:
it seems that UEFI accesses during boot might have left the device in an unsuitable state for the kernel. this commit aims at correcting that.
It would be nice to know what that state is. I've been bitten by loaders leaving devices on with DMA active, and it would be nice to know that something like that isn't the case here -- it can be tricky to detect until it's too late.
I was under the impression that UEFI drivers were meant to leave devices in some quiescent state. Sepending on the precise state the HW is left it, this may still be a UEFI bug regardless of whether it's possible for the kernel to fix things up, and if that's the case we should fix the UEFI port regardless of whether we also modify the kernel to handle that state.
Indeed - why is this not handled as an ExitBootServices hook in UEFI?
/ Leif
On 17 March 2015 at 16:08, Mark Rutland mark.rutland@arm.com wrote:
On Tue, Mar 17, 2015 at 02:20:48PM +0000, Jorge Ramirez-Ortiz wrote:
On 03/17/2015 09:34 AM, Mark Rutland wrote:
On Tue, Mar 17, 2015 at 08:59:10AM +0000, Haojian Zhuang wrote:
If mmc controler is accessed in bootloader, it's required to execute disable boot before resetting the controller & enabling the clock in kernel driver.
Signed-off-by: Haojian Zhuang haojian.zhuang@linaro.org
.../devicetree/bindings/mmc/k3-dw-mshc.txt | 7 +++++ drivers/mmc/host/dw_mmc-k3.c | 33 ++++++++++++++++++++++ drivers/mmc/host/dw_mmc.h | 1 + 3 files changed, 41 insertions(+)
diff --git a/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt index 3b35449..cf12cfa 100644 --- a/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt +++ b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt @@ -14,6 +14,13 @@ Required Properties:
- compatible: should be one of the following.
- "hisilicon,hi4511-dw-mshc": for controllers with hi4511 specific extensions.
+Optional Properties:
+* "hisilicon,disable-boot"
- Disable boot before reseting mmc controller and enabling clocks. It's
- required on Hisilicon Hi6220 SoC if the mmc controller is accessed in
- bootloader.
I don't follow. Why do we need another property?
What exactly does this cause to happen at the MMC controller prior to the reset, why exactly is this necessary,
are you asking for documentation about the controller?
I'm asking for the semantics associated with this property rather than for a full TRM.
Realistically what I'd like to know is why this is necessary prior to the reset of the controller mentioned in the property description. From the sounds of things we're not actually resetting the controller.
I also suspect that "disable boot" is a misnomer both in the proeprty and code.
and why can we not always do this regardless?
+1
That patch looks like we're poking the device prior to resetting it, which seems completely backwards.
yes, some background info can be found here https://github.com/96boards/bugs/issues/19
it seems that UEFI accesses during boot might have left the device in an unsuitable state for the kernel. this commit aims at correcting that.
It would be nice to know what that state is.
There's code elsewhere in the hikey kernel to poke at dwmmc reset: https://github.com/96boards/linux/commit/6d790cc68afd5f65eb6ab4f20e635158d3c...
That writel() is needed to make the disable-boot patches work.
-
Haojian Zhuang -
Jorge Ramirez-Ortiz -
Koen Kooi -
Leif Lindholm -
Mark Brown -
Mark Rutland -
Riku Voipio -
Rob Herring