* [PATCH 1/2] mmc: sdhci-esdhc: clean up register definitions
From: Yangbo Lu @ 2016-11-25 4:00 UTC (permalink / raw)
To: linux-mmc, ulf.hansson; +Cc: Xiaobo Xie, Yangbo Lu
The eSDHC register definitions in header file were messy and confusing.
This patch is to clean up these definitions.
Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
drivers/mmc/host/sdhci-esdhc.h | 39 ++++++++++++++++++++-------------------
1 file changed, 20 insertions(+), 19 deletions(-)
diff --git a/drivers/mmc/host/sdhci-esdhc.h b/drivers/mmc/host/sdhci-esdhc.h
index de132e2..8cd8449 100644
--- a/drivers/mmc/host/sdhci-esdhc.h
+++ b/drivers/mmc/host/sdhci-esdhc.h
@@ -24,30 +24,31 @@
SDHCI_QUIRK_PIO_NEEDS_DELAY | \
SDHCI_QUIRK_NO_HISPD_BIT)
-#define ESDHC_PROCTL 0x28
-
-#define ESDHC_SYSTEM_CONTROL 0x2c
-#define ESDHC_CLOCK_MASK 0x0000fff0
-#define ESDHC_PREDIV_SHIFT 8
-#define ESDHC_DIVIDER_SHIFT 4
-#define ESDHC_CLOCK_PEREN 0x00000004
-#define ESDHC_CLOCK_HCKEN 0x00000002
-#define ESDHC_CLOCK_IPGEN 0x00000001
-
/* pltfm-specific */
#define ESDHC_HOST_CONTROL_LE 0x20
/*
- * P2020 interpretation of the SDHCI_HOST_CONTROL register
+ * eSDHC register definition
*/
-#define ESDHC_CTRL_4BITBUS (0x1 << 1)
-#define ESDHC_CTRL_8BITBUS (0x2 << 1)
-#define ESDHC_CTRL_BUSWIDTH_MASK (0x3 << 1)
-
-/* OF-specific */
-#define ESDHC_DMA_SYSCTL 0x40c
-#define ESDHC_DMA_SNOOP 0x00000040
-#define ESDHC_HOST_CONTROL_RES 0x01
+/* Protocol Control Register */
+#define ESDHC_PROCTL 0x28
+#define ESDHC_CTRL_4BITBUS (0x1 << 1)
+#define ESDHC_CTRL_8BITBUS (0x2 << 1)
+#define ESDHC_CTRL_BUSWIDTH_MASK (0x3 << 1)
+#define ESDHC_HOST_CONTROL_RES 0x01
+
+/* System Control Register */
+#define ESDHC_SYSTEM_CONTROL 0x2c
+#define ESDHC_CLOCK_MASK 0x0000fff0
+#define ESDHC_PREDIV_SHIFT 8
+#define ESDHC_DIVIDER_SHIFT 4
+#define ESDHC_CLOCK_PEREN 0x00000004
+#define ESDHC_CLOCK_HCKEN 0x00000002
+#define ESDHC_CLOCK_IPGEN 0x00000001
+
+/* Control Register for DMA transfer */
+#define ESDHC_DMA_SYSCTL 0x40c
+#define ESDHC_DMA_SNOOP 0x00000040
#endif /* _DRIVERS_MMC_SDHCI_ESDHC_H */
--
2.1.0.27.g96db324
^ permalink raw reply related
* [PATCH 2/2] mmc: sdhci-of-esdhc: avoid clock glitch when frequency is changing
From: Yangbo Lu @ 2016-11-25 4:00 UTC (permalink / raw)
To: linux-mmc, ulf.hansson; +Cc: Xiaobo Xie, Yangbo Lu
In-Reply-To: <1480046451-29492-1-git-send-email-yangbo.lu@nxp.com>
The eSDHC_PRSSTAT[SDSTB] bit indicates whether the internal card clock is
stable. This bit is for the host driver to poll clock status when changing
the clock frequency. It is recommended to clear eSDHC_SYSCTL[SDCLKEN]
to remove glitch on the card clock when the frequency is changing. This
patch is to disable SDCLKEN bit before changing frequency and enable it
after SDSTB bit is set.
Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
drivers/mmc/host/sdhci-esdhc.h | 5 +++++
drivers/mmc/host/sdhci-of-esdhc.c | 21 ++++++++++++++++++---
2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/host/sdhci-esdhc.h b/drivers/mmc/host/sdhci-esdhc.h
index 8cd8449..ece8b37 100644
--- a/drivers/mmc/host/sdhci-esdhc.h
+++ b/drivers/mmc/host/sdhci-esdhc.h
@@ -31,6 +31,10 @@
* eSDHC register definition
*/
+/* Present State Register */
+#define ESDHC_PRSSTAT 0x24
+#define ESDHC_CLOCK_STABLE 0x00000008
+
/* Protocol Control Register */
#define ESDHC_PROCTL 0x28
#define ESDHC_CTRL_4BITBUS (0x1 << 1)
@@ -43,6 +47,7 @@
#define ESDHC_CLOCK_MASK 0x0000fff0
#define ESDHC_PREDIV_SHIFT 8
#define ESDHC_DIVIDER_SHIFT 4
+#define ESDHC_CLOCK_SDCLKEN 0x00000008
#define ESDHC_CLOCK_PEREN 0x00000004
#define ESDHC_CLOCK_HCKEN 0x00000002
#define ESDHC_CLOCK_IPGEN 0x00000001
diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index 9a6eb44..0849885 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -431,6 +431,7 @@ static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int clock)
struct sdhci_esdhc *esdhc = sdhci_pltfm_priv(pltfm_host);
int pre_div = 1;
int div = 1;
+ u32 timeout;
u32 temp;
host->mmc->actual_clock = 0;
@@ -451,8 +452,8 @@ static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int clock)
}
temp = sdhci_readl(host, ESDHC_SYSTEM_CONTROL);
- temp &= ~(ESDHC_CLOCK_IPGEN | ESDHC_CLOCK_HCKEN | ESDHC_CLOCK_PEREN
- | ESDHC_CLOCK_MASK);
+ temp &= ~(ESDHC_CLOCK_SDCLKEN | ESDHC_CLOCK_IPGEN | ESDHC_CLOCK_HCKEN |
+ ESDHC_CLOCK_PEREN | ESDHC_CLOCK_MASK);
sdhci_writel(host, temp, ESDHC_SYSTEM_CONTROL);
while (host->max_clk / pre_div / 16 > clock && pre_div < 256)
@@ -472,7 +473,21 @@ static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int clock)
| (div << ESDHC_DIVIDER_SHIFT)
| (pre_div << ESDHC_PREDIV_SHIFT));
sdhci_writel(host, temp, ESDHC_SYSTEM_CONTROL);
- mdelay(1);
+
+ /* Wait max 20 ms */
+ timeout = 20;
+ while (!(sdhci_readl(host, ESDHC_PRSSTAT) & ESDHC_CLOCK_STABLE)) {
+ if (timeout == 0) {
+ pr_err("%s: Internal clock never stabilised.\n",
+ mmc_hostname(host->mmc));
+ return;
+ }
+ timeout--;
+ mdelay(1);
+ }
+
+ temp |= ESDHC_CLOCK_SDCLKEN;
+ sdhci_writel(host, temp, ESDHC_SYSTEM_CONTROL);
}
static void esdhc_pltfm_set_bus_width(struct sdhci_host *host, int width)
--
2.1.0.27.g96db324
^ permalink raw reply related
* Re: [PATCH v2] mmc: dw_mmc: add missing codes for runtime resume
From: Jaehoon Chung @ 2016-11-25 3:49 UTC (permalink / raw)
To: Joonyoung Shim, linux-mmc; +Cc: ulf.hansson, shawn.lin
In-Reply-To: <1480045635-32042-1-git-send-email-jy0922.shim@samsung.com>
On 11/25/2016 12:47 PM, Joonyoung Shim wrote:
> The commit 64997de4fd17 ("mmc: dw_mmc: remove system PM callback") is
> missing to call dw_mci_ctrl_reset(). This adds to call
> dw_mci_ctrl_reset() and to handle error of clocks.
>
> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
Applied on my repository, instead of v1. Thanks for noticing.
If someone find the issue, let me know before PR to Ulf.
Best Regards,
Jaehoon Chung
> ---
> v2: fix error handling of biu_clk.
>
> drivers/mmc/host/dw_mmc.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index d426fa41bcce..5b50b2cdb008 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -3301,7 +3301,13 @@ int dw_mci_runtime_resume(struct device *dev)
>
> ret = clk_prepare_enable(host->ciu_clk);
> if (ret)
> - return ret;
> + goto err;
> +
> + if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS)) {
> + clk_disable_unprepare(host->ciu_clk);
> + ret = -ENODEV;
> + goto err;
> + }
>
> if (host->use_dma && host->dma_ops->init)
> host->dma_ops->init(host);
> @@ -3336,6 +3342,14 @@ int dw_mci_runtime_resume(struct device *dev)
> /* Now that slots are all setup, we can enable card detect */
> dw_mci_enable_cd(host);
>
> + return 0;
> +
> +err:
> + if (host->cur_slot &&
> + (mmc_can_gpio_cd(host->cur_slot->mmc) ||
> + !mmc_card_is_removable(host->cur_slot->mmc)))
> + clk_disable_unprepare(host->biu_clk);
> +
> return ret;
> }
> EXPORT_SYMBOL(dw_mci_runtime_resume);
>
^ permalink raw reply
* [PATCH v2] mmc: dw_mmc: add missing codes for runtime resume
From: Joonyoung Shim @ 2016-11-25 3:47 UTC (permalink / raw)
To: linux-mmc; +Cc: jh80.chung, ulf.hansson, shawn.lin, jy0922.shim
The commit 64997de4fd17 ("mmc: dw_mmc: remove system PM callback") is
missing to call dw_mci_ctrl_reset(). This adds to call
dw_mci_ctrl_reset() and to handle error of clocks.
Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
---
v2: fix error handling of biu_clk.
drivers/mmc/host/dw_mmc.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index d426fa41bcce..5b50b2cdb008 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -3301,7 +3301,13 @@ int dw_mci_runtime_resume(struct device *dev)
ret = clk_prepare_enable(host->ciu_clk);
if (ret)
- return ret;
+ goto err;
+
+ if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS)) {
+ clk_disable_unprepare(host->ciu_clk);
+ ret = -ENODEV;
+ goto err;
+ }
if (host->use_dma && host->dma_ops->init)
host->dma_ops->init(host);
@@ -3336,6 +3342,14 @@ int dw_mci_runtime_resume(struct device *dev)
/* Now that slots are all setup, we can enable card detect */
dw_mci_enable_cd(host);
+ return 0;
+
+err:
+ if (host->cur_slot &&
+ (mmc_can_gpio_cd(host->cur_slot->mmc) ||
+ !mmc_card_is_removable(host->cur_slot->mmc)))
+ clk_disable_unprepare(host->biu_clk);
+
return ret;
}
EXPORT_SYMBOL(dw_mci_runtime_resume);
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] mmc: dw_mmc: add missing codes for runtime resume
From: Joonyoung Shim @ 2016-11-25 2:55 UTC (permalink / raw)
To: Jaehoon Chung, linux-mmc
Cc: ulf.hansson, shawn.lin,
jy0922.shim@samsung.com >> Joonyoung Shim
In-Reply-To: <4478096a-4d57-f739-d746-17c506548981@samsung.com>
Hi jaehoon,
On 11/25/2016 11:37 AM, Jaehoon Chung wrote:
> Hi Joonyoung,
>
> On 11/23/2016 06:33 PM, Joonyoung Shim wrote:
>> The commit 64997de4fd17 ("mmc: dw_mmc: remove system PM callback") is
>> missing to call dw_mci_ctrl_reset(). This adds to call
>> dw_mci_ctrl_reset() and to handle error of clocks.
>>
>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>
> Applied on my dwmmc repository. Thanks!
>
Thanks, but i found one more issue.
> Best Regards,
> Jaehoon Chung
>
>> ---
>> drivers/mmc/host/dw_mmc.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index d426fa41bcce..25e21a20ee04 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -3303,6 +3303,17 @@ int dw_mci_runtime_resume(struct device *dev)
>> if (ret)
>> return ret;
Here, error handling about biu_clk should be consider.
>>
>> + if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS)) {
>> + clk_disable_unprepare(host->ciu_clk);
>> +
>> + if (host->cur_slot &&
>> + (mmc_can_gpio_cd(host->cur_slot->mmc) ||
>> + !mmc_card_is_removable(host->cur_slot->mmc)))
>> + clk_disable_unprepare(host->biu_clk);
>> +
>> + return -ENODEV;
>> + }
>> +
>> if (host->use_dma && host->dma_ops->init)
>> host->dma_ops->init(host);
>>
>>
I will send patch v2, could you review and apply patch v2 instead of v1?
Thanks.
^ permalink raw reply
* Re: [PATCH] mmc: dw_mmc: add missing codes for runtime resume
From: Jaehoon Chung @ 2016-11-25 2:37 UTC (permalink / raw)
To: Joonyoung Shim, linux-mmc; +Cc: ulf.hansson, shawn.lin
In-Reply-To: <1479893632-1742-1-git-send-email-jy0922.shim@samsung.com>
Hi Joonyoung,
On 11/23/2016 06:33 PM, Joonyoung Shim wrote:
> The commit 64997de4fd17 ("mmc: dw_mmc: remove system PM callback") is
> missing to call dw_mci_ctrl_reset(). This adds to call
> dw_mci_ctrl_reset() and to handle error of clocks.
>
> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
Applied on my dwmmc repository. Thanks!
Best Regards,
Jaehoon Chung
> ---
> drivers/mmc/host/dw_mmc.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index d426fa41bcce..25e21a20ee04 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -3303,6 +3303,17 @@ int dw_mci_runtime_resume(struct device *dev)
> if (ret)
> return ret;
>
> + if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS)) {
> + clk_disable_unprepare(host->ciu_clk);
> +
> + if (host->cur_slot &&
> + (mmc_can_gpio_cd(host->cur_slot->mmc) ||
> + !mmc_card_is_removable(host->cur_slot->mmc)))
> + clk_disable_unprepare(host->biu_clk);
> +
> + return -ENODEV;
> + }
> +
> if (host->use_dma && host->dma_ops->init)
> host->dma_ops->init(host);
>
>
^ permalink raw reply
* Re: [PATCH] mmc: dw_mmc: add missing codes for runtime resume
From: Jaehoon Chung @ 2016-11-25 2:37 UTC (permalink / raw)
To: Joonyoung Shim, linux-mmc; +Cc: ulf.hansson, shawn.lin
In-Reply-To: <1479893632-1742-1-git-send-email-jy0922.shim@samsung.com>
Hi Joonyoung,
On 11/23/2016 06:33 PM, Joonyoung Shim wrote:
> The commit 64997de4fd17 ("mmc: dw_mmc: remove system PM callback") is
> missing to call dw_mci_ctrl_reset(). This adds to call
> dw_mci_ctrl_reset() and to handle error of clocks.
>
> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
Applied on my dwmmc repository. Thanks!
Best Regards,
Jaehoon Chung
> ---
> drivers/mmc/host/dw_mmc.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index d426fa41bcce..25e21a20ee04 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -3303,6 +3303,17 @@ int dw_mci_runtime_resume(struct device *dev)
> if (ret)
> return ret;
>
> + if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS)) {
> + clk_disable_unprepare(host->ciu_clk);
> +
> + if (host->cur_slot &&
> + (mmc_can_gpio_cd(host->cur_slot->mmc) ||
> + !mmc_card_is_removable(host->cur_slot->mmc)))
> + clk_disable_unprepare(host->biu_clk);
> +
> + return -ENODEV;
> + }
> +
> if (host->use_dma && host->dma_ops->init)
> host->dma_ops->init(host);
>
>
^ permalink raw reply
* Re: [PATCH v2] mmc: block: delete packed command support
From: Fengguang Wu @ 2016-11-25 1:43 UTC (permalink / raw)
To: Linus Walleij
Cc: kbuild-all@01.org, linux-mmc@vger.kernel.org, Ulf Hansson,
Chunyan Zhang, Baolin Wang, Namjae Jeon, Maya Erez
In-Reply-To: <CACRpkdYaCs4f-aaCmGu-RPQJe4HMKbhE10vcekxNpKHVm=6YYg@mail.gmail.com>
Hi Linus,
On Thu, Nov 24, 2016 at 10:53:55AM +0100, Linus Walleij wrote:
>On Thu, Nov 24, 2016 at 7:35 AM, kbuild test robot <lkp@intel.com> wrote:
>
>> Hi Linus,
>>
>> [auto build test WARNING on ulf.hansson-mmc/next]
That line indicates the patch is applied to 'ulf.hansson-mmc/next',
which is exactly the tree/branch you mentioned below. :)
>> [cannot apply to linus/master linux/master v4.9-rc6 next-20161123]
>> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
>This is indeed a false build error.
>
>Patches sent to linux-mmc@vger.kernel.org is probably better tested agains
>the "next" branch on Ulf's MMC tree at:
>https://git.kernel.org/cgit/linux/kernel/git/ulfh/mmc.git/
>
>Yours,
>Linus Walleij
^ permalink raw reply
* Re: [PATCH mmc/next] mmc: sh_mmcif: Document r8a73a4, r8a7779 and sh73a0 DT bindings
From: Sergei Shtylyov @ 2016-11-24 18:50 UTC (permalink / raw)
To: Simon Horman, Ulf Hansson
Cc: linux-mmc, devicetree, Magnus Damm, linux-renesas-soc
In-Reply-To: <1480011435-22125-1-git-send-email-horms+renesas@verge.net.au>
Hello.
On 11/24/2016 09:17 PM, Simon Horman wrote:
> Simply document new compatibility strings as the driver is already
> activated using a fallback compatibility string.
>
> These compat strings are in keeping with those for all other
> Renesas ARM based SoCs with sh_mmcif enabled in mainline.
>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
> I plan to follow-up with patches to use these new compat strings
> to bring the DT files of the SoCs in question in-line with those
> for other Renesas ARM based SoCs with sh_mmcif enabled in mainline.
> ---
> Documentation/devicetree/bindings/mmc/renesas,mmcif.txt | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt b/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
> index ff611fa66871..e4ba92aa035e 100644
> --- a/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
> +++ b/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
> @@ -8,11 +8,14 @@ Required properties:
>
> - compatible: should be "renesas,mmcif-<soctype>", "renesas,sh-mmcif" as a
> fallback. Examples with <soctype> are:
> + - "renesas,mmcif-r8a73a4" for the MMCIF found in r8a73a4 SoCs
> - "renesas,mmcif-r8a7740" for the MMCIF found in r8a7740 SoCs
> + - "renesas,mmcif-r8a7778" for the MMCIF found in r8a7778 SoCs
7779 in the subject, 7778 here.
[...]
MBR, Sergei
^ permalink raw reply
* [PATCH mmc/next] mmc: sh_mmcif: Document r8a73a4, r8a7779 and sh73a0 DT bindings
From: Simon Horman @ 2016-11-24 18:17 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, devicetree, Magnus Damm, linux-renesas-soc,
Simon Horman
Simply document new compatibility strings as the driver is already
activated using a fallback compatibility string.
These compat strings are in keeping with those for all other
Renesas ARM based SoCs with sh_mmcif enabled in mainline.
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
I plan to follow-up with patches to use these new compat strings
to bring the DT files of the SoCs in question in-line with those
for other Renesas ARM based SoCs with sh_mmcif enabled in mainline.
---
Documentation/devicetree/bindings/mmc/renesas,mmcif.txt | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt b/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
index ff611fa66871..e4ba92aa035e 100644
--- a/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
+++ b/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
@@ -8,11 +8,14 @@ Required properties:
- compatible: should be "renesas,mmcif-<soctype>", "renesas,sh-mmcif" as a
fallback. Examples with <soctype> are:
+ - "renesas,mmcif-r8a73a4" for the MMCIF found in r8a73a4 SoCs
- "renesas,mmcif-r8a7740" for the MMCIF found in r8a7740 SoCs
+ - "renesas,mmcif-r8a7778" for the MMCIF found in r8a7778 SoCs
- "renesas,mmcif-r8a7790" for the MMCIF found in r8a7790 SoCs
- "renesas,mmcif-r8a7791" for the MMCIF found in r8a7791 SoCs
- "renesas,mmcif-r8a7793" for the MMCIF found in r8a7793 SoCs
- "renesas,mmcif-r8a7794" for the MMCIF found in r8a7794 SoCs
+ - "renesas,mmcif-sh73a0" for the MMCIF found in sh73a0 SoCs
- clocks: reference to the functional clock
--
2.7.0.rc3.207.g0ac5344
^ permalink raw reply related
* Re: [PATCH] mmc: sdhci-msm: Add sdhci_reset() implementation
From: Ritesh Harjani @ 2016-11-24 16:06 UTC (permalink / raw)
To: Georgi Djakov, adrian.hunter, ulf.hansson
Cc: linux-mmc, linux-kernel, linux-arm-msm
In-Reply-To: <20161122155005.16910-1-georgi.djakov@linaro.org>
Hi Georgi,
I collected some info on this problem. May be below info might help you.
I think "Reset 0x1" problem is occurring because of below call stack.
SDHCI_RESET_ALL to SDHCI_SOFTWARE_RESET register will anyway trigger the
sdhci_msm_pwr_irq.
But I think the problem is that the above occurs in spinlock context
and because of only one core the IRQ will never be serviced, hence you
were seeing (Reset 0x1) error.
[ 12.583245] systemd-journald[1236]: Received SIGTERM from PID 1
(systemd-shutdow).
[ 12.673684] EXT4-fs (mmcblk0p10): re-mounted. Opts: data=ordered
[ 12.678224] EXT4-fs (mmcblk0p10): re-mounted. Opts: data=ordered
[ 13.698330] mmc0: Reset 0x1 never completed.
[ 13.698353] sdhci: =========== REGISTER DUMP (mmc0)===========
[ 13.701659] sdhci: Sys addr: 0x00000000 | Version: 0x00002e02
[ 13.707301] sdhci: Blk size: 0x00004000 | Blk cnt: 0x00000000
[ 13.713117] sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
[ 13.718933] sdhci: Present: 0x01f80000 | Host ctl: 0x00000000
[ 13.724750] sdhci: Power: 0x00000000 | Blk gap: 0x00000000
[ 13.730564] sdhci: Wake-up: 0x00000000 | Clock: 0x00000003
[ 13.736381] sdhci: Timeout: 0x00000000 | Int stat: 0x00000000
[ 13.742197] sdhci: Int enab: 0x00000000 | Sig enab: 0x00000000
[ 13.748012] sdhci: AC12 err: 0x00000000 | Slot int: 0x00000000
[ 13.753830] sdhci: Caps: 0x322dc8b2 | Caps_1: 0x00008007
[ 13.759644] sdhci: Cmd: 0x00000000 | Max curr: 0x00000000
[ 13.765461] sdhci: Host ctl2: 0x00000000
[ 13.771275] sdhci: ADMA Err: 0x00000000 | ADMA Ptr: 0x0000000000000000
[ 13.775357] sdhci: ===========================================
[ 13.781698] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted
4.9.0-rc5-00222-g59ac3c0-dirty #9
[ 13.787514] Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
[ 13.796104] Call trace:
[ 13.802878] [<ffff0000080882c4>] dump_backtrace+0x0/0x1a8
[ 13.805049] [<ffff000008088480>] show_stack+0x14/0x1c
[ 13.810602] [<ffff000008366274>] dump_stack+0x8c/0xb0
[ 13.815640] [<ffff00000870eab0>] sdhci_reset+0xd8/0x114
[ 13.820670] [<ffff00000870ee30>] sdhci_do_reset+0x48/0x7c
[ 13.825706] [<ffff00000870ef20>] sdhci_init+0xbc/0x110
[ 13.831260] [<ffff000008710c44>] sdhci_set_ios+0x68/0x59c
[ 13.836299] [<ffff0000086f95f4>] mmc_set_initial_state+0xc0/0xcc
[ 13.841767] [<ffff0000086f983c>] mmc_power_off.part.22+0x28/0x40
[ 13.847841] [<ffff0000086f9b5c>] mmc_power_off+0x14/0x1c
[ 13.853832] [<ffff0000086fc754>] _mmc_suspend+0x1e4/0x260
[ 13.859127] [<ffff0000086fe104>] mmc_shutdown+0x2c/0x60
[ 13.864421] [<ffff0000086faa00>] mmc_bus_shutdown+0x40/0x74
[ 13.869458] [<ffff0000084f9280>] device_shutdown+0xf0/0x1a8
[ 13.875013] [<ffff0000080db908>] kernel_restart_prepare+0x34/0x3c
[ 13.880568] [<ffff0000080db9e4>] kernel_restart+0x14/0x74
[ 13.886817] [<ffff0000080dbd2c>] SyS_reboot+0x178/0x244
[ 13.892198] [<ffff000008082ef0>] el0_svc_naked+0x24/0x28
[ 13.897300] mmc0: sdhci_msm_pwr_irq:
[ 13.902799] mmc0: sdhci_msm_voltage_switch: irq_status 9
[ 13.906355] mmc0: sdhci_msm_voltage_switch: irq_status 9, irq_ack 5
To prove above I tried this and the problem goes away. But I still dont
think that the below approach is correct. As it will still trigger a
pwr_irq as well.
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 62aedf1..01e611c 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -174,6 +174,8 @@ void sdhci_reset(struct sdhci_host *host, u8 mask)
/* Reset-all turns off SD Bus Power */
if (host->quirks2 & SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON)
sdhci_runtime_pm_bus_off(host);
+ if (host->ops->voltage_switch)
+ host->ops->voltage_switch(host);
}
/* Wait max 100 ms */
On 11/22/2016 9:20 PM, Georgi Djakov wrote:
> On apq8016, apq8084 and apq8074 platforms, when we want to do a
> software reset, we need to poke some additional vendor specific
> registers. If we don't do so, the following error message appears:
>
> mmc0: Reset 0x1 never completed.
> sdhci: =========== REGISTER DUMP (mmc0)===========
> sdhci: Sys addr: 0x00000000 | Version: 0x00002e02
> sdhci: Blk size: 0x00004000 | Blk cnt: 0x00000000
> sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
> sdhci: Present: 0x01f80000 | Host ctl: 0x00000000
> sdhci: Power: 0x00000000 | Blk gap: 0x00000000
> sdhci: Wake-up: 0x00000000 | Clock: 0x00000003
> sdhci: Timeout: 0x00000000 | Int stat: 0x00000000
> sdhci: Int enab: 0x00000000 | Sig enab: 0x00000000
> sdhci: AC12 err: 0x00000000 | Slot int: 0x00000000
> sdhci: Caps: 0x322dc8b2 | Caps_1: 0x00008007
> sdhci: Cmd: 0x00000000 | Max curr: 0x00000000
> sdhci: Host ctl2: 0x00000000
> sdhci: ADMA Err: 0x00000000 | ADMA Ptr: 0x0000000000000000
> sdhci: ===========================================
>
> Fix it by implementing the custom sdhci_reset() function,
> which does what is needed.
>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
> drivers/mmc/host/sdhci-msm.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 8ef44a2a2fd9..87a124a37408 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -505,6 +505,23 @@ static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
> return IRQ_HANDLED;
> }
>
> +void sdhci_msm_reset(struct sdhci_host *host, u8 mask)
> +{
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +
> + if (mask & SDHCI_RESET_ALL) {
> + u32 val = readl_relaxed(msm_host->core_mem + CORE_POWER);
> +
> + val |= CORE_SW_RST;
> + writel_relaxed(val, msm_host->core_mem + CORE_POWER);
Not required as sdhci_reset register should anyway reset and trigger a
pwr_irq. Because you are not servicing that IRQ the problem is present.
> +
> + sdhci_msm_voltage_switch(host);
> + }
> +
> + sdhci_reset(host, mask);
This I think is sufficient.
> +}
> +
> static const struct of_device_id sdhci_msm_dt_match[] = {
> { .compatible = "qcom,sdhci-msm-v4" },
> {},
> @@ -514,7 +531,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
>
> static const struct sdhci_ops sdhci_msm_ops = {
> .platform_execute_tuning = sdhci_msm_execute_tuning,
> - .reset = sdhci_reset,
> + .reset = sdhci_msm_reset,
> .set_clock = sdhci_set_clock,
> .set_bus_width = sdhci_set_bus_width,
> .set_uhs_signaling = sdhci_msm_set_uhs_signaling,
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related
* Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC
From: Ziji Hu @ 2016-11-24 15:37 UTC (permalink / raw)
To: Ulf Hansson
Cc: Gregory CLEMENT, Adrian Hunter, linux-mmc@vger.kernel.org,
Jason Cooper, Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
devicetree@vger.kernel.org, Thomas Petazzoni,
linux-arm-kernel@lists.infradead.org, Jimmy Xu, Jisheng Zhang,
Nadav Haklai, Ryan Gao, Doug Jones, Victor Gu, Wei(SOCP) Liu,
Wilson Ding
In-Reply-To: <CAPDyKFrskyZvMwhmj8ioyaowU8GL5=d-6ipGQcmmig_a9E9AQQ@mail.gmail.com>
Hi Ulf,
On 2016/11/24 22:33, Ulf Hansson wrote:
> [...]
>
>>>
>>>>
>>>> +
>>>> +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
>>>> +{
>>>> + int err;
>>>> + u8 *ext_csd = NULL;
>>>> +
>>>> + err = mmc_get_ext_csd(card, &ext_csd);
>>>> + kfree(ext_csd);
>>>
>>> Why do you read the ext csd here?
>>>
>> I would like to simply introduce the PHY setting of our SDHC.
>> The target of the PHY setting is to achieve a perfect sampling
>> point for transfers, during card initialization.
>
> Okay, so the phy is involved when running the tuning sequence.
>
Actually, all the transfers pass our host PHY.
>>
>> For HS200/HS400/SDR104 whose SDCLK is more than 50MHz, SDHC HW
>> will search for this sampling point with DLL's help.
>
> Apologize for my ignorance, but what is a "DLL" in this case?
>
DLL is Delay-locked Loop. It is a HW module similar to PLL.
>>
>> For other speed mode whose SDLCK is less than or equals to 50MHz,
>> SW has to scan the PHY delay line to find out this perfect sampling
>> point. Our driver sends a command to verify a sampling point
>> in current environment.
>
> Ahh, okay! I guess the important part here is to not only send a
> command, but also to make sure data becomes transferred on the DAT
> lines, as to confirm your tuning sequence!?
Yes.
It is the best if the test command can transfer on DAT lines.
>
> In cases of HS200/HS400/SDR104 you should be able to use the
> mmc_send_tuning() API, don't you think?
For HS200/HS400/SDR104, we finally call sdhci_execute_tuning() to
execute tuning. Those test commands are not used.
In HS200/HS400/SDR104, HW will provide our host driver with suitable
tuning step. Our host driver set the tuning step in SDHCI register and
then start standard tuning sequence. The tuning step value provided
by our host HW will enhance tuning.
>
> For the other cases (lower speed modes) which cards doesn't support
> the tuning command, perhaps you can just assume the PHY scan succeeded
> and then allow to core to continue with the card initialization
> sequence? Or do you foresee any issues with that? My point is that, if
> it will fail - it will fail anyway.
Usually, our host driver will always successfully scan and select a
perfect sampling point.
If driver cannot find any suitable sampling point, it is likely that
transfers will also fail after init. But usually it is a issue, caused by
incorrect setting on boards/SOC/other PHY parameters, especially in development.
We will fix the issue and then scan will succeed in final product.
>
>>
>> As result, our SDHC driver has to implement the functionality to
>> send commands and check the results, in host layer.
>> If directly calling mmc_wait_for_cmd() is improper, could you please
>> give us some suggestions?
>>
>> For eMMC, CMD8 is used to test current sampling point set in PHY.
>
> Try to use mmc_send_tuning().
>
Could you please tell me the requirement of "op_code" parameter in
mmc_send_tuning()?
According to mmc_send_tuning(),it seems that tuning command(CMD19/CMD21)
is required. Thus device will not response mmc_send_tuning() if current
speed mode doesn't support tuning command.
Please correct me if I am wrong.
>>
>>>> +
>>>> + return err;
>>>> +}
>>>> +
>>>> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
>>>> +{
>>>> + struct mmc_command cmd = {0};
>>>> + int err;
>>>> +
>>>> + cmd.opcode = SD_IO_RW_DIRECT;
>>>> + cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
>>>> +
>>>> + err = mmc_wait_for_cmd(card->host, &cmd, 0);
>>>> + if (err)
>>>> + return err;
>>>> +
>>>> + if (cmd.resp[0] & R5_ERROR)
>>>> + return -EIO;
>>>> + if (cmd.resp[0] & R5_FUNCTION_NUMBER)
>>>> + return -EINVAL;
>>>> + if (cmd.resp[0] & R5_OUT_OF_RANGE)
>>>> + return -ERANGE;
>>>> + return 0;
>>>
>>> No thanks! MMC/SD/SDIO protocol code belongs in the core.
>>>
>> For SDIO, SD_IO_RW_DIRECT command is sent to test current sampling point
>> in PHY.
>> Please help provide some suggestion to implement the command transfer.
>
> Again, I think mmc_send_tuning() should be possible for you to use.
>
> [...]
>
>>>> + if (mmc->card)
>>>> + card = mmc->card;
>>>> + else
>>>> + /*
>>>> + * Only valid during initialization
>>>> + * before mmc->card is set
>>>> + */
>>>> + card = priv->card_candidate;
>>>> + if (unlikely(!card)) {
>>>> + dev_warn(mmc_dev(mmc), "card is not present\n");
>>>> + return -EINVAL;
>>>> + }
>>>
>>> That your host need to hold a copy of the card pointer, tells me that
>>> something is not really correct.
>>>
>>> I might be wrong, if this turns out to be a special case, but I doubt
>>> it. Although, if it *is* a special such case, we shall most likely try
>>> to extend the the mmc core layer instead of adding all these hacks in
>>> your host driver.
>>>
>> This card pointer copies the temporary structure mmc_card
>> used in mmc_init_card(), mmc_sd_init_card() and mmc_sdio_init_card().
>> Since we call mmc_wait_for_cmd() to send test commands, we need a copy
>> of that temporary mmc_card here in our host driver.
>
> I see, thanks for clarifying.
>
>>
>> During PHY setting in card initialization, mmc_host->card is not updated
>> yet with that temporary mmc_card. Thus we are not able to directly use
>> mmc_host->card. Instead, this card pointer is introduced to enable
>> mmc_wait_for_cmd().
>>
>> If we can improve our host driver to send test commands without mmc_card,
>> this card pointer can be removed.
>> Could you please share your opinion please?
>
> The mmc_send_tuning() API takes the mmc_host as parameter. If you
> convert to that, perhaps you would be able to remove the need to hold
> the card pointer.
>
> BTW, the reason why mmc_send_tuning() doesn't take the card as a
> parameter, is exactly those you just described above.
>
Got it.
Thanks a lot for the information.
Thank you for the great help.
Best regards,
Hu Ziji
> [...]
>
> Kind regards
> Uffe
>
^ permalink raw reply
* Re: [PATCH v2] mmc: block: delete packed command support
From: Linus Walleij @ 2016-11-24 15:22 UTC (permalink / raw)
To: Jaehoon Chung
Cc: linux-mmc@vger.kernel.org, Ulf Hansson, Chunyan Zhang,
Baolin Wang, Namjae Jeon, Maya Erez
In-Reply-To: <bffa21b9-6c14-345b-bf90-f12ffe56b542@samsung.com>
On Thu, Nov 24, 2016 at 11:23 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> Anyway, until today, i have checked the I/O performance with Exynos3/4/5 boards which i have.
> It seems that there is no I/O performance benefit about packed command at this time.
Hm so is that with a separate tree where the packed command is enabled
and with an eMMC 4.5 that has packed command support?
If you put a print in some packed command code, can you see it getting
executed?
I think we may want packed command support but we need to fix it
on top of blk-mq as well as we need to fix command queueing.
Yours,
Linus Walleij
^ permalink raw reply
* [GIT PULL] MMC fixes for v.4.9 rc7
From: Ulf Hansson @ 2016-11-24 15:04 UTC (permalink / raw)
To: Linus, linux-mmc, linux-kernel@vger.kernel.org
Cc: Jaehoon Chung, Adrian Hunter
Hi Linus,
Here are some mmc fixes intended for v4.9 rc7. They are based on v4.9-rc5.
Details are as usual found in the signed tag. Please pull this in!
Kind regards
Ulf Hansson
The following changes since commit a25f0944ba9b1d8a6813fd6f1a86f1bd59ac25a6:
Linux 4.9-rc5 (2016-11-13 10:32:32 -0800)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git tags/mmc-v4.9-rc5
for you to fetch changes up to 647f80a1f233bb66fc58fb25664d029e0f12f3ae:
mmc: dw_mmc: fix the error handling for dma operation (2016-11-21
11:08:28 +0100)
----------------------------------------------------------------
MMC host:
- sdhci-of-esdhc: Fix card detection
- dw_mmc: Fix DMA error path
----------------------------------------------------------------
Jaehoon Chung (1):
mmc: dw_mmc: fix the error handling for dma operation
Michael Walle (1):
mmc: sdhci-of-esdhc: fixup PRESENT_STATE read
drivers/mmc/host/dw_mmc.c | 1 +
drivers/mmc/host/sdhci-of-esdhc.c | 14 ++++++++++++++
drivers/mmc/host/sdhci.h | 1 +
3 files changed, 16 insertions(+)
^ permalink raw reply
* Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality
From: Ziji Hu @ 2016-11-24 15:00 UTC (permalink / raw)
To: Ulf Hansson, Adrian Hunter
Cc: Jimmy Xu, Andrew Lunn, Romain Perier, Hanna Hawa, Nadav Haklai,
Zhen Huang, Victor Gu, Doug Jones, Jisheng Zhang, Yehuda Yitschak,
Wei(SOCP) Liu, Kostya Porotchkin, Sebastian Hesselbarth,
devicetree@vger.kernel.org, Jason Cooper, Rob Herring, Ryan Gao,
Gregory CLEMENT, Marcin Wojtas,
linux-arm-kernel@lists.infradead.org, Thomas Petazzoni, linux-mmc
In-Reply-To: <CAPDyKFr9uEjVQmTNP0KK8Zj9mxCW3i564E=47vTK0RLvXCjw3Q@mail.gmail.com>
Hi Ulf,
On 2016/11/24 21:34, Ulf Hansson wrote:
> On 24 November 2016 at 13:41, Ziji Hu <huziji@marvell.com> wrote:
>> Hi Ulf,
>>
>> On 2016/11/24 18:43, Ulf Hansson wrote:
>>> On 31 October 2016 at 12:09, Gregory CLEMENT
>>> <gregory.clement@free-electrons.com> wrote:
>>>> From: Ziji Hu <huziji@marvell.com>
>>>>
>> <snip>
>>>> +static int xenon_emmc_signal_voltage_switch(struct mmc_host *mmc,
>>>> + struct mmc_ios *ios)
>>>> +{
>>>> + unsigned char voltage = ios->signal_voltage;
>>>> +
>>>> + if ((voltage == MMC_SIGNAL_VOLTAGE_330) ||
>>>> + (voltage == MMC_SIGNAL_VOLTAGE_180))
>>>> + return __emmc_signal_voltage_switch(mmc, voltage);
>>>> +
>>>> + dev_err(mmc_dev(mmc), "Unsupported signal voltage: %d\n",
>>>> + voltage);
>>>> + return -EINVAL;
>>>
>>> This wrapper function seems unnessarry. It only adds a dev_err(), so
>>> then might as well do that in __emmc_signal_voltage_switch().
>>>
>> Sure. Will merge it back to __emmc_signal_voltage_switch().
>>
>>>> +}
>>>> +
>>>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
>>>> + struct mmc_ios *ios)
>>>> +{
>>>> + struct sdhci_host *host = mmc_priv(mmc);
>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>> +
>>>> + /*
>>>> + * Before SD/SDIO set signal voltage, SD bus clock should be
>>>> + * disabled. However, sdhci_set_clock will also disable the Internal
>>>> + * clock in mmc_set_signal_voltage().
>>>
>>> If that's the case then that is wrong in the generic sdhci code.
>>> What's the reason why it can't be fixed there instead of having this
>>> workaround?
>>>
>> In my very own opinion, SD Spec doesn't specify whether SDCLK should be
>> enabled or not during power setting.
>> Enabling SDCLK might be a special condition only required by our SDHC.
>> I try to avoid breaking other vendors' SDHC functionality
>> if their SDHCs require SDCLK disabled.
>> Thus I prefer to keep it inside our SDHC driver.
>
> I let Adrian comment on this.
>
> For sure we should avoid breaking other sdhci variant, but on the
> other hand *if* the generic code is wrong we should fix it!
>
Of course.
>>
>>>> + * If Internal clock is disabled, the 3.3V/1.8V bit can not be updated.
>>>> + * Thus here manually enable internal clock.
>>>> + *
>>>> + * After switch completes, it is unnecessary to disable internal clock,
>>>> + * since keeping internal clock active obeys SD spec.
>>>> + */
>>>> + enable_xenon_internal_clk(host);
>>>> +
>>>> + if (priv->emmc_slot)
>>>> + return xenon_emmc_signal_voltage_switch(mmc, ios);
>>>> +
>>>> + return sdhci_start_signal_voltage_switch(mmc, ios);
>>>> +}
>>>> +
>>>> +/*
>>>> + * After determining which slot is used for SDIO,
>>>> + * some additional task is required.
>>>> + */
>>>> +static void xenon_init_card(struct mmc_host *mmc, struct mmc_card *card)
>>>> +{
>>>> + struct sdhci_host *host = mmc_priv(mmc);
>>>> + u32 reg;
>>>> + u8 slot_idx;
>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>> +
>>>> + /* Link the card for delay adjustment */
>>>> + priv->card_candidate = card;
>>>> + /* Set tuning functionality of this slot */
>>>> + xenon_slot_tuning_setup(host);
>>>
>>> This looks weird. I assume this can be done as a part of the regular
>>> tuning seqeunce!?
>>>
>> It is our SDHC specific preparation prior to tuning, rather than a
>> standard step in spec.
>> Thus I leave it inside our driver.
>
> My point is that this isn't the purpose of ->init_card(). thus you are
> abusing it.
>
> Try to make it work in another way, please. I think you can.
>
Got it.
I will move it to our host specific probe function.
>>
>>>> +
>>>> + slot_idx = priv->slot_idx;
>>>> + if (!mmc_card_sdio(card)) {
>>>> + /* Clear SDIO Card Inserted indication */
>>>
>>> Why do you need this?
>>>
>>> If you need to reset this, I think it's better to do it from
>>> ->set_ios() at MMC_POWER_OFF.
>>>
>> This field indicates SDIO card and controls async interrupt feature
>> of SDIO in our SDHC.
>> This async interrupt feature is enabled when SDIO card is inserted.
>> It should be disabled if SD card is inserted instead.
>
> What do you mean by SDIO async interupts? Are you talking about SDIO
> irqs on DAT1 line?
>
> Those is supposed to be enabled when someone explicitly requests them,
> not when the card is inserted.
> In other words when an SDIO func driver have called sdio_claim_irq().
>
> Moreover, we have ->enable_sdio_irq() ops that deals with this.
>
Yes. I mean the SDIO irqs on DAT1 line in async mode.
This field enables our host to recognize the async SDIO irq from SDIO device.
It controls our host side behavior, other than the SDIO device.
I think ->enable_sdio_irq() is a more reasonable place to put this workraound.
I will export sdhci_enable_sdio_irq() and implement out host own
enable_sdio_irq() calling sdhci_enable_sdio)irq() plus this workaround.
Does it sound reasonable to you?
> [...]
>
>>>> +
>>>> + /*
>>>> + * Xenon Specific property:
>>>> + * emmc: explicitly indicate whether this slot is for eMMC
>>>> + * slotno: the index of slot. Refer to SDHC_SYS_CFG_INFO register
>>>> + * tun-count: the interval between re-tuning
>>>> + * PHY type: "sdhc phy", "emmc phy 5.0" or "emmc phy 5.1"
>>>> + */
>>>> + if (of_property_read_bool(np, "marvell,xenon-emmc"))
>>>> + priv->emmc_slot = true;
>>>
>>> So, you need this because of the eMMC voltage switch behaviour, right?
>>>
>>> Then I would rather like to describe this a generic DT bindings for
>>> the eMMC voltage level support. There have acutally been some earlier
>>> discussions for this, but we haven't yet made some changes.
>>>
>>> I think what is missing is a mmc-ddr-3_3v DT binding, which when set,
>>> allows the host driver to accept I/O voltage switches to 3.3V. If not
>>> supported the ->start_signal_voltage_switch() ops may return -EINVAL.
>>> This would inform the mmc core to move on to the next supported
>>> voltage level. There might be some minor additional changes to the mmc
>>> card initialization sequence, but those should be simple.
>>>
>>> I can help out to look into this, unless you want to do it yourself of course!?
>>>
>> Yes. One of the reasons is to provide eMMC specific voltage setting.
>> But in my very own opinion, it should be irrelevant to voltage level.
>> The eMMC voltage setting on our SDHC is different from SD/SDIO voltage switch.
>> It will become more complex with different SOC implementation details.
>
> Got it. Although I think we can cope with that fine just by using the
> different SD/eMMC speed modes settings defined in DT (or from the
> SDHCI caps register)
>
In my very opinion, I'm not sure if there is any corner case that driver cannot
determine the eMMC card type from DT and SDHC caps.
>> Unfortunately, MMC driver cannot determine the card type yet when eMMC voltage
>> setting should be executed.
>> Thus an flag is required here to tell driver to execute eMMC voltage setting.
>>
>> Besides, additional eMMC specific settings might be implemented in future, besides
>> voltage setting. Most of them should be completed before MMC driver recognizes the
>> card type. Thus I have to keep this flag to indicate current SDHC is for eMMC.
>
> I doubt you will need a generic "eMMC" flag, but let's see when we go forward.
>
> Currently it's clear you don't need such a flag, so I will submit a
> change adding a DT binding for "mmc-ddr-3_3v" then we can take it from
> there, to see if it suits your needs.
>
Actually, our eMMC is usually fixed as 1.8V.
The pair "no-sd" + "no-sdio" can provide the similar information.
But I'm not sure if it is proper to use those two property in such a way.
Thank you.
Best regards
Hu Ziji
> [...]
>
> Kind regards
> Uffe
>
^ permalink raw reply
* Re: [PATCH mmc/next] mmc: sh_mobile_sdhi: remove support for sh7372
From: Wolfram Sang @ 2016-11-24 14:42 UTC (permalink / raw)
To: Simon Horman
Cc: Wolfram Sang, Ulf Hansson, Magnus Damm, linux-mmc,
linux-renesas-soc
In-Reply-To: <1479984534-25468-1-git-send-email-horms+renesas@verge.net.au>
[-- Attachment #1: Type: text/plain, Size: 592 bytes --]
On Thu, Nov 24, 2016 at 11:48:54AM +0100, Simon Horman wrote:
> Remove documentation of support for the SH7372 (SH-Mobile AP4) from the MMC
> driver. The driver itself appears to have no SH7372 specific code.
>
> Commit edf4100906044225 ("ARM: shmobile: sh7372 dtsi: Remove Legacy file")
> removes this SoC from the kernel in v4.1.
>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
Yes, the compatible entry was removed with ab22f516715086 ("mmc:
sh_mobile_sdhi: remove obsolete support for sh7372").
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC
From: Ulf Hansson @ 2016-11-24 14:33 UTC (permalink / raw)
To: Ziji Hu
Cc: Gregory CLEMENT, Adrian Hunter,
linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jason Cooper,
Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Thomas Petazzoni,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Jimmy Xu, Jisheng Zhang, Nadav Haklai, Ryan Gao, Doug Jones,
Victor Gu, Wei(SOCP) Liu, Wilson Ding
In-Reply-To: <3cd05a26-d340-476e-bab1-8be9d5446f47-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
[...]
>>
>>>
>>> +
>>> +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
>>> +{
>>> + int err;
>>> + u8 *ext_csd = NULL;
>>> +
>>> + err = mmc_get_ext_csd(card, &ext_csd);
>>> + kfree(ext_csd);
>>
>> Why do you read the ext csd here?
>>
> I would like to simply introduce the PHY setting of our SDHC.
> The target of the PHY setting is to achieve a perfect sampling
> point for transfers, during card initialization.
Okay, so the phy is involved when running the tuning sequence.
>
> For HS200/HS400/SDR104 whose SDCLK is more than 50MHz, SDHC HW
> will search for this sampling point with DLL's help.
Apologize for my ignorance, but what is a "DLL" in this case?
>
> For other speed mode whose SDLCK is less than or equals to 50MHz,
> SW has to scan the PHY delay line to find out this perfect sampling
> point. Our driver sends a command to verify a sampling point
> in current environment.
Ahh, okay! I guess the important part here is to not only send a
command, but also to make sure data becomes transferred on the DAT
lines, as to confirm your tuning sequence!?
In cases of HS200/HS400/SDR104 you should be able to use the
mmc_send_tuning() API, don't you think?
For the other cases (lower speed modes) which cards doesn't support
the tuning command, perhaps you can just assume the PHY scan succeeded
and then allow to core to continue with the card initialization
sequence? Or do you foresee any issues with that? My point is that, if
it will fail - it will fail anyway.
>
> As result, our SDHC driver has to implement the functionality to
> send commands and check the results, in host layer.
> If directly calling mmc_wait_for_cmd() is improper, could you please
> give us some suggestions?
>
> For eMMC, CMD8 is used to test current sampling point set in PHY.
Try to use mmc_send_tuning().
>
>>> +
>>> + return err;
>>> +}
>>> +
>>> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
>>> +{
>>> + struct mmc_command cmd = {0};
>>> + int err;
>>> +
>>> + cmd.opcode = SD_IO_RW_DIRECT;
>>> + cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
>>> +
>>> + err = mmc_wait_for_cmd(card->host, &cmd, 0);
>>> + if (err)
>>> + return err;
>>> +
>>> + if (cmd.resp[0] & R5_ERROR)
>>> + return -EIO;
>>> + if (cmd.resp[0] & R5_FUNCTION_NUMBER)
>>> + return -EINVAL;
>>> + if (cmd.resp[0] & R5_OUT_OF_RANGE)
>>> + return -ERANGE;
>>> + return 0;
>>
>> No thanks! MMC/SD/SDIO protocol code belongs in the core.
>>
> For SDIO, SD_IO_RW_DIRECT command is sent to test current sampling point
> in PHY.
> Please help provide some suggestion to implement the command transfer.
Again, I think mmc_send_tuning() should be possible for you to use.
[...]
>>> + if (mmc->card)
>>> + card = mmc->card;
>>> + else
>>> + /*
>>> + * Only valid during initialization
>>> + * before mmc->card is set
>>> + */
>>> + card = priv->card_candidate;
>>> + if (unlikely(!card)) {
>>> + dev_warn(mmc_dev(mmc), "card is not present\n");
>>> + return -EINVAL;
>>> + }
>>
>> That your host need to hold a copy of the card pointer, tells me that
>> something is not really correct.
>>
>> I might be wrong, if this turns out to be a special case, but I doubt
>> it. Although, if it *is* a special such case, we shall most likely try
>> to extend the the mmc core layer instead of adding all these hacks in
>> your host driver.
>>
> This card pointer copies the temporary structure mmc_card
> used in mmc_init_card(), mmc_sd_init_card() and mmc_sdio_init_card().
> Since we call mmc_wait_for_cmd() to send test commands, we need a copy
> of that temporary mmc_card here in our host driver.
I see, thanks for clarifying.
>
> During PHY setting in card initialization, mmc_host->card is not updated
> yet with that temporary mmc_card. Thus we are not able to directly use
> mmc_host->card. Instead, this card pointer is introduced to enable
> mmc_wait_for_cmd().
>
> If we can improve our host driver to send test commands without mmc_card,
> this card pointer can be removed.
> Could you please share your opinion please?
The mmc_send_tuning() API takes the mmc_host as parameter. If you
convert to that, perhaps you would be able to remove the need to hold
the card pointer.
BTW, the reason why mmc_send_tuning() doesn't take the card as a
parameter, is exactly those you just described above.
[...]
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] mmc: android-goldfish: drop duplicate header scatterlist.h
From: Geliang Tang @ 2016-11-24 13:58 UTC (permalink / raw)
To: Ulf Hansson; +Cc: Geliang Tang, linux-mmc, linux-kernel
In-Reply-To: <15299de49216a2360976ca37ff774cae9d27d88b.1479991297.git.geliangtang@gmail.com>
Drop duplicate header scatterlist.h from android-goldfish.c.
Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
drivers/mmc/host/android-goldfish.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/mmc/host/android-goldfish.c b/drivers/mmc/host/android-goldfish.c
index dca5518..5de7450 100644
--- a/drivers/mmc/host/android-goldfish.c
+++ b/drivers/mmc/host/android-goldfish.c
@@ -42,7 +42,6 @@
#include <linux/spinlock.h>
#include <linux/timer.h>
#include <linux/clk.h>
-#include <linux/scatterlist.h>
#include <asm/io.h>
#include <asm/irq.h>
--
2.9.3
^ permalink raw reply related
* Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC
From: Ziji Hu @ 2016-11-24 13:34 UTC (permalink / raw)
To: Ulf Hansson, Gregory CLEMENT
Cc: Adrian Hunter, linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Jason Cooper, Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Thomas Petazzoni,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Jimmy Xu, Jisheng Zhang, Nadav Haklai, Ryan Gao, Doug Jones,
Victor Gu, Wei(SOCP) Liu, Wilson Ding, Romain Perier
In-Reply-To: <CAPDyKFpkcoVMKbVOwjX1WDyNgc1vvUX60D6XRX6=YHGvkvHvnA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi Ulf,
Thanks a lot for the review.
On 2016/11/24 19:37, Ulf Hansson wrote:
> On 31 October 2016 at 12:09, Gregory CLEMENT
> <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> From: Ziji Hu <huziji-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>>
>> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
>> Three types of PHYs are supported.
>>
>> Add support to multiple types of PHYs init and configuration.
>> Add register definitions of PHYs.
>>
>> Signed-off-by: Hu Ziji <huziji-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>> Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>> ---
>> MAINTAINERS | 2 +-
>> drivers/mmc/host/Makefile | 2 +-
>> drivers/mmc/host/sdhci-xenon-phy.c | 1181 +++++++++++++++++++++++++++++-
>> drivers/mmc/host/sdhci-xenon-phy.h | 157 ++++-
>> drivers/mmc/host/sdhci-xenon.c | 4 +-
>> drivers/mmc/host/sdhci-xenon.h | 17 +-
>> 6 files changed, 1361 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c
>> create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h
>
> Can you please consider to split this up somehow!? It would make it
> easier to review...
>
Sure. I will try to split them into smaller pieces.
> Anyway, allow me to provide some initial feedback, particularly around
> those things that Adrian and you requested for my input.
>
> [...]
>
>>
>> +
>> +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
>> +{
>> + int err;
>> + u8 *ext_csd = NULL;
>> +
>> + err = mmc_get_ext_csd(card, &ext_csd);
>> + kfree(ext_csd);
>
> Why do you read the ext csd here?
>
I would like to simply introduce the PHY setting of our SDHC.
The target of the PHY setting is to achieve a perfect sampling
point for transfers, during card initialization.
For HS200/HS400/SDR104 whose SDCLK is more than 50MHz, SDHC HW
will search for this sampling point with DLL's help.
For other speed mode whose SDLCK is less than or equals to 50MHz,
SW has to scan the PHY delay line to find out this perfect sampling
point. Our driver sends a command to verify a sampling point
in current environment.
As result, our SDHC driver has to implement the functionality to
send commands and check the results, in host layer.
If directly calling mmc_wait_for_cmd() is improper, could you please
give us some suggestions?
For eMMC, CMD8 is used to test current sampling point set in PHY.
>> +
>> + return err;
>> +}
>> +
>> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
>> +{
>> + struct mmc_command cmd = {0};
>> + int err;
>> +
>> + cmd.opcode = SD_IO_RW_DIRECT;
>> + cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
>> +
>> + err = mmc_wait_for_cmd(card->host, &cmd, 0);
>> + if (err)
>> + return err;
>> +
>> + if (cmd.resp[0] & R5_ERROR)
>> + return -EIO;
>> + if (cmd.resp[0] & R5_FUNCTION_NUMBER)
>> + return -EINVAL;
>> + if (cmd.resp[0] & R5_OUT_OF_RANGE)
>> + return -ERANGE;
>> + return 0;
>
> No thanks! MMC/SD/SDIO protocol code belongs in the core.
>
For SDIO, SD_IO_RW_DIRECT command is sent to test current sampling point
in PHY.
Please help provide some suggestion to implement the command transfer.
>> +}
>> +
>> +static int __xenon_sd_delay_adj_test(struct mmc_card *card)
>> +{
>> + struct mmc_command cmd = {0};
>> + int err;
>> +
>> + cmd.opcode = MMC_SEND_STATUS;
>> + cmd.arg = card->rca << 16;
>> + cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>> +
>> + err = mmc_wait_for_cmd(card->host, &cmd, 0);
>> + return err;
>
> No thanks! MMC/SD/SDIO protocol code belongs in the core.
>
>> +}
>> +
>
> [...]
>
>> +int xenon_phy_adj(struct sdhci_host *host, struct mmc_ios *ios)
>> +{
>> + struct mmc_host *mmc = host->mmc;
>> + struct mmc_card *card;
>> + int ret = 0;
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> + if (!host->clock) {
>> + priv->clock = 0;
>> + return 0;
>> + }
>> +
>> + /*
>> + * The timing, frequency or bus width is changed,
>> + * better to set eMMC PHY based on current setting
>> + * and adjust Xenon SDHC delay.
>> + */
>> + if ((host->clock == priv->clock) &&
>> + (ios->bus_width == priv->bus_width) &&
>> + (ios->timing == priv->timing))
>> + return 0;
>> +
>> + xenon_phy_set(host, ios->timing);
>> +
>> + /* Update the record */
>> + priv->bus_width = ios->bus_width;
>> + /* Temp stage from HS200 to HS400 */
>> + if (((priv->timing == MMC_TIMING_MMC_HS200) &&
>> + (ios->timing == MMC_TIMING_MMC_HS)) ||
>> + ((ios->timing == MMC_TIMING_MMC_HS) &&
>> + (priv->clock > host->clock))) {
>> + priv->timing = ios->timing;
>> + priv->clock = host->clock;
>> + return 0;
>> + }
>> + /*
>> + * Skip temp stages from HS400 t0 HS200:
>> + * from 200MHz to 52MHz in HS400
>> + * from HS400 to HS DDR in 52MHz
>> + * from HS DDR to HS in 52MHz
>> + * from HS to HS200 in 52MHz
>> + */
>> + if (((priv->timing == MMC_TIMING_MMC_HS400) &&
>> + ((host->clock == MMC_HIGH_52_MAX_DTR) ||
>> + (ios->timing == MMC_TIMING_MMC_DDR52))) ||
>> + ((priv->timing == MMC_TIMING_MMC_DDR52) &&
>> + (ios->timing == MMC_TIMING_MMC_HS)) ||
>> + ((ios->timing == MMC_TIMING_MMC_HS200) &&
>> + (ios->clock == MMC_HIGH_52_MAX_DTR))) {
>> + priv->timing = ios->timing;
>> + priv->clock = host->clock;
>> + return 0;
>> + }
>> + priv->timing = ios->timing;
>> + priv->clock = host->clock;
>> +
>> + /* Legacy mode is a special case */
>> + if (ios->timing == MMC_TIMING_LEGACY)
>> + return 0;
>> +
>> + if (mmc->card)
>> + card = mmc->card;
>> + else
>> + /*
>> + * Only valid during initialization
>> + * before mmc->card is set
>> + */
>> + card = priv->card_candidate;
>> + if (unlikely(!card)) {
>> + dev_warn(mmc_dev(mmc), "card is not present\n");
>> + return -EINVAL;
>> + }
>
> That your host need to hold a copy of the card pointer, tells me that
> something is not really correct.
>
> I might be wrong, if this turns out to be a special case, but I doubt
> it. Although, if it *is* a special such case, we shall most likely try
> to extend the the mmc core layer instead of adding all these hacks in
> your host driver.
>
This card pointer copies the temporary structure mmc_card
used in mmc_init_card(), mmc_sd_init_card() and mmc_sdio_init_card().
Since we call mmc_wait_for_cmd() to send test commands, we need a copy
of that temporary mmc_card here in our host driver.
During PHY setting in card initialization, mmc_host->card is not updated
yet with that temporary mmc_card. Thus we are not able to directly use
mmc_host->card. Instead, this card pointer is introduced to enable
mmc_wait_for_cmd().
If we can improve our host driver to send test commands without mmc_card,
this card pointer can be removed.
Could you please share your opinion please?
> [...]
>
> Another suggestion of a general improvement; could you perhaps try to
> add some brief information about what goes on in function headers.
> Perhaps that could help to more easily understand things.
>
Sorry about any inconvenience. Most of the functions here are our host specific.
It is really difficult to understand them without proper comment.
I will add more information.
Thank you.
Best regards,
Hu Ziji
> Kind regards
> Uffe
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality
From: Ulf Hansson @ 2016-11-24 13:34 UTC (permalink / raw)
To: Ziji Hu, Adrian Hunter
Cc: Gregory CLEMENT, linux-mmc@vger.kernel.org, Jason Cooper,
Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
devicetree@vger.kernel.org, Thomas Petazzoni,
linux-arm-kernel@lists.infradead.org, Jimmy Xu, Jisheng Zhang,
Nadav Haklai, Ryan Gao, Doug Jones, Victor Gu, Wei(SOCP) Liu,
Wilson Ding, Romain Perier
In-Reply-To: <dd230463-04f6-df31-7056-1a185eb6cfc7@marvell.com>
On 24 November 2016 at 13:41, Ziji Hu <huziji@marvell.com> wrote:
> Hi Ulf,
>
> On 2016/11/24 18:43, Ulf Hansson wrote:
>> On 31 October 2016 at 12:09, Gregory CLEMENT
>> <gregory.clement@free-electrons.com> wrote:
>>> From: Ziji Hu <huziji@marvell.com>
>>>
> <snip>
>>> +static int xenon_emmc_signal_voltage_switch(struct mmc_host *mmc,
>>> + struct mmc_ios *ios)
>>> +{
>>> + unsigned char voltage = ios->signal_voltage;
>>> +
>>> + if ((voltage == MMC_SIGNAL_VOLTAGE_330) ||
>>> + (voltage == MMC_SIGNAL_VOLTAGE_180))
>>> + return __emmc_signal_voltage_switch(mmc, voltage);
>>> +
>>> + dev_err(mmc_dev(mmc), "Unsupported signal voltage: %d\n",
>>> + voltage);
>>> + return -EINVAL;
>>
>> This wrapper function seems unnessarry. It only adds a dev_err(), so
>> then might as well do that in __emmc_signal_voltage_switch().
>>
> Sure. Will merge it back to __emmc_signal_voltage_switch().
>
>>> +}
>>> +
>>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
>>> + struct mmc_ios *ios)
>>> +{
>>> + struct sdhci_host *host = mmc_priv(mmc);
>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>> +
>>> + /*
>>> + * Before SD/SDIO set signal voltage, SD bus clock should be
>>> + * disabled. However, sdhci_set_clock will also disable the Internal
>>> + * clock in mmc_set_signal_voltage().
>>
>> If that's the case then that is wrong in the generic sdhci code.
>> What's the reason why it can't be fixed there instead of having this
>> workaround?
>>
> In my very own opinion, SD Spec doesn't specify whether SDCLK should be
> enabled or not during power setting.
> Enabling SDCLK might be a special condition only required by our SDHC.
> I try to avoid breaking other vendors' SDHC functionality
> if their SDHCs require SDCLK disabled.
> Thus I prefer to keep it inside our SDHC driver.
I let Adrian comment on this.
For sure we should avoid breaking other sdhci variant, but on the
other hand *if* the generic code is wrong we should fix it!
>
>>> + * If Internal clock is disabled, the 3.3V/1.8V bit can not be updated.
>>> + * Thus here manually enable internal clock.
>>> + *
>>> + * After switch completes, it is unnecessary to disable internal clock,
>>> + * since keeping internal clock active obeys SD spec.
>>> + */
>>> + enable_xenon_internal_clk(host);
>>> +
>>> + if (priv->emmc_slot)
>>> + return xenon_emmc_signal_voltage_switch(mmc, ios);
>>> +
>>> + return sdhci_start_signal_voltage_switch(mmc, ios);
>>> +}
>>> +
>>> +/*
>>> + * After determining which slot is used for SDIO,
>>> + * some additional task is required.
>>> + */
>>> +static void xenon_init_card(struct mmc_host *mmc, struct mmc_card *card)
>>> +{
>>> + struct sdhci_host *host = mmc_priv(mmc);
>>> + u32 reg;
>>> + u8 slot_idx;
>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>> +
>>> + /* Link the card for delay adjustment */
>>> + priv->card_candidate = card;
>>> + /* Set tuning functionality of this slot */
>>> + xenon_slot_tuning_setup(host);
>>
>> This looks weird. I assume this can be done as a part of the regular
>> tuning seqeunce!?
>>
> It is our SDHC specific preparation prior to tuning, rather than a
> standard step in spec.
> Thus I leave it inside our driver.
My point is that this isn't the purpose of ->init_card(). thus you are
abusing it.
Try to make it work in another way, please. I think you can.
>
>>> +
>>> + slot_idx = priv->slot_idx;
>>> + if (!mmc_card_sdio(card)) {
>>> + /* Clear SDIO Card Inserted indication */
>>
>> Why do you need this?
>>
>> If you need to reset this, I think it's better to do it from
>> ->set_ios() at MMC_POWER_OFF.
>>
> This field indicates SDIO card and controls async interrupt feature
> of SDIO in our SDHC.
> This async interrupt feature is enabled when SDIO card is inserted.
> It should be disabled if SD card is inserted instead.
What do you mean by SDIO async interupts? Are you talking about SDIO
irqs on DAT1 line?
Those is supposed to be enabled when someone explicitly requests them,
not when the card is inserted.
In other words when an SDIO func driver have called sdio_claim_irq().
Moreover, we have ->enable_sdio_irq() ops that deals with this.
[...]
>>> +
>>> + /*
>>> + * Xenon Specific property:
>>> + * emmc: explicitly indicate whether this slot is for eMMC
>>> + * slotno: the index of slot. Refer to SDHC_SYS_CFG_INFO register
>>> + * tun-count: the interval between re-tuning
>>> + * PHY type: "sdhc phy", "emmc phy 5.0" or "emmc phy 5.1"
>>> + */
>>> + if (of_property_read_bool(np, "marvell,xenon-emmc"))
>>> + priv->emmc_slot = true;
>>
>> So, you need this because of the eMMC voltage switch behaviour, right?
>>
>> Then I would rather like to describe this a generic DT bindings for
>> the eMMC voltage level support. There have acutally been some earlier
>> discussions for this, but we haven't yet made some changes.
>>
>> I think what is missing is a mmc-ddr-3_3v DT binding, which when set,
>> allows the host driver to accept I/O voltage switches to 3.3V. If not
>> supported the ->start_signal_voltage_switch() ops may return -EINVAL.
>> This would inform the mmc core to move on to the next supported
>> voltage level. There might be some minor additional changes to the mmc
>> card initialization sequence, but those should be simple.
>>
>> I can help out to look into this, unless you want to do it yourself of course!?
>>
> Yes. One of the reasons is to provide eMMC specific voltage setting.
> But in my very own opinion, it should be irrelevant to voltage level.
> The eMMC voltage setting on our SDHC is different from SD/SDIO voltage switch.
> It will become more complex with different SOC implementation details.
Got it. Although I think we can cope with that fine just by using the
different SD/eMMC speed modes settings defined in DT (or from the
SDHCI caps register)
> Unfortunately, MMC driver cannot determine the card type yet when eMMC voltage
> setting should be executed.
> Thus an flag is required here to tell driver to execute eMMC voltage setting.
>
> Besides, additional eMMC specific settings might be implemented in future, besides
> voltage setting. Most of them should be completed before MMC driver recognizes the
> card type. Thus I have to keep this flag to indicate current SDHC is for eMMC.
I doubt you will need a generic "eMMC" flag, but let's see when we go forward.
Currently it's clear you don't need such a flag, so I will submit a
change adding a DT binding for "mmc-ddr-3_3v" then we can take it from
there, to see if it suits your needs.
[...]
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality
From: Ziji Hu @ 2016-11-24 12:41 UTC (permalink / raw)
To: Ulf Hansson, Gregory CLEMENT
Cc: Adrian Hunter, linux-mmc@vger.kernel.org, Jason Cooper,
Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
devicetree@vger.kernel.org, Thomas Petazzoni,
linux-arm-kernel@lists.infradead.org, Jimmy Xu, Jisheng Zhang,
Nadav Haklai, Ryan Gao, Doug Jones, Victor Gu, Wei(SOCP) Liu,
Wilson Ding, Romain Perier
In-Reply-To: <CAPDyKFpqPSqiEi=0UW5LoZmy+y-KHm9nbcGKBSy3RzchdLU9cA@mail.gmail.com>
Hi Ulf,
On 2016/11/24 18:43, Ulf Hansson wrote:
> On 31 October 2016 at 12:09, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>> From: Ziji Hu <huziji@marvell.com>
>>
<snip>
>> +static int xenon_emmc_signal_voltage_switch(struct mmc_host *mmc,
>> + struct mmc_ios *ios)
>> +{
>> + unsigned char voltage = ios->signal_voltage;
>> +
>> + if ((voltage == MMC_SIGNAL_VOLTAGE_330) ||
>> + (voltage == MMC_SIGNAL_VOLTAGE_180))
>> + return __emmc_signal_voltage_switch(mmc, voltage);
>> +
>> + dev_err(mmc_dev(mmc), "Unsupported signal voltage: %d\n",
>> + voltage);
>> + return -EINVAL;
>
> This wrapper function seems unnessarry. It only adds a dev_err(), so
> then might as well do that in __emmc_signal_voltage_switch().
>
Sure. Will merge it back to __emmc_signal_voltage_switch().
>> +}
>> +
>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
>> + struct mmc_ios *ios)
>> +{
>> + struct sdhci_host *host = mmc_priv(mmc);
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> + /*
>> + * Before SD/SDIO set signal voltage, SD bus clock should be
>> + * disabled. However, sdhci_set_clock will also disable the Internal
>> + * clock in mmc_set_signal_voltage().
>
> If that's the case then that is wrong in the generic sdhci code.
> What's the reason why it can't be fixed there instead of having this
> workaround?
>
In my very own opinion, SD Spec doesn't specify whether SDCLK should be
enabled or not during power setting.
Enabling SDCLK might be a special condition only required by our SDHC.
I try to avoid breaking other vendors' SDHC functionality
if their SDHCs require SDCLK disabled.
Thus I prefer to keep it inside our SDHC driver.
>> + * If Internal clock is disabled, the 3.3V/1.8V bit can not be updated.
>> + * Thus here manually enable internal clock.
>> + *
>> + * After switch completes, it is unnecessary to disable internal clock,
>> + * since keeping internal clock active obeys SD spec.
>> + */
>> + enable_xenon_internal_clk(host);
>> +
>> + if (priv->emmc_slot)
>> + return xenon_emmc_signal_voltage_switch(mmc, ios);
>> +
>> + return sdhci_start_signal_voltage_switch(mmc, ios);
>> +}
>> +
>> +/*
>> + * After determining which slot is used for SDIO,
>> + * some additional task is required.
>> + */
>> +static void xenon_init_card(struct mmc_host *mmc, struct mmc_card *card)
>> +{
>> + struct sdhci_host *host = mmc_priv(mmc);
>> + u32 reg;
>> + u8 slot_idx;
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> + /* Link the card for delay adjustment */
>> + priv->card_candidate = card;
>> + /* Set tuning functionality of this slot */
>> + xenon_slot_tuning_setup(host);
>
> This looks weird. I assume this can be done as a part of the regular
> tuning seqeunce!?
>
It is our SDHC specific preparation prior to tuning, rather than a
standard step in spec.
Thus I leave it inside our driver.
>> +
>> + slot_idx = priv->slot_idx;
>> + if (!mmc_card_sdio(card)) {
>> + /* Clear SDIO Card Inserted indication */
>
> Why do you need this?
>
> If you need to reset this, I think it's better to do it from
> ->set_ios() at MMC_POWER_OFF.
>
This field indicates SDIO card and controls async interrupt feature
of SDIO in our SDHC.
This async interrupt feature is enabled when SDIO card is inserted.
It should be disabled if SD card is inserted instead.
>> + reg = sdhci_readl(host, SDHC_SYS_CFG_INFO);
>> + reg &= ~(1 << (slot_idx + SLOT_TYPE_SDIO_SHIFT));
>> + sdhci_writel(host, reg, SDHC_SYS_CFG_INFO);
>> +
>> + if (mmc_card_mmc(card)) {
>> + mmc->caps |= MMC_CAP_NONREMOVABLE;
>> + if (!(host->quirks2 & SDHCI_QUIRK2_NO_1_8_V))
>> + mmc->caps |= MMC_CAP_1_8V_DDR;
>> + /*
>> + * Force to clear BUS_TEST to
>> + * skip bus_test_pre and bus_test_post
>> + */
>> + mmc->caps &= ~MMC_CAP_BUS_WIDTH_TEST;
>> + mmc->caps2 |= MMC_CAP2_HC_ERASE_SZ |
>> + MMC_CAP2_PACKED_CMD;
>> + if (mmc->caps & MMC_CAP_8_BIT_DATA)
>> + mmc->caps2 |= MMC_CAP2_HS400_1_8V;
>
> Most of this can be specified as DT configurations. Please use that instead.
>
> More importantly, please don't use the ->init_card() ops to assign
> host caps. If not DT, please do it from ->probe().
>
Sure. Will try to use DT instead.
>> + }
>> + } else {
>> + /*
>> + * Set SDIO Card Inserted indication
>> + * to inform that the current slot is for SDIO
>> + */
>> + reg = sdhci_readl(host, SDHC_SYS_CFG_INFO);
>> + reg |= (1 << (slot_idx + SLOT_TYPE_SDIO_SHIFT));
>> + sdhci_writel(host, reg, SDHC_SYS_CFG_INFO);
>
> So this makes sence to have in the ->init_card() ops. The rest above, not.
>
>> + }
>> +}
>> +
>> +static int xenon_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> +{
>> + struct sdhci_host *host = mmc_priv(mmc);
>> +
>> + if (host->timing == MMC_TIMING_UHS_DDR50)
>> + return 0;
>> +
>> + return sdhci_execute_tuning(mmc, opcode);
>> +}
>> +
>> +static void xenon_replace_mmc_host_ops(struct sdhci_host *host)
>> +{
>> + host->mmc_host_ops.set_ios = xenon_set_ios;
>> + host->mmc_host_ops.start_signal_voltage_switch =
>> + xenon_start_signal_voltage_switch;
>> + host->mmc_host_ops.init_card = xenon_init_card;
>> + host->mmc_host_ops.execute_tuning = xenon_execute_tuning;
>> +}
>> +
>> +static int xenon_probe_dt(struct platform_device *pdev)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + struct sdhci_host *host = platform_get_drvdata(pdev);
>> + struct mmc_host *mmc = host->mmc;
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> + int err;
>> + u32 slot_idx, nr_slot;
>> + u32 tuning_count;
>> + u32 reg;
>> +
>> + /* Standard MMC property */
>> + err = mmc_of_parse(mmc);
>> + if (err)
>> + return err;
>> +
>> + /* Standard SDHCI property */
>> + sdhci_get_of_property(pdev);
>> +
>> + /*
>> + * Xenon Specific property:
>> + * emmc: explicitly indicate whether this slot is for eMMC
>> + * slotno: the index of slot. Refer to SDHC_SYS_CFG_INFO register
>> + * tun-count: the interval between re-tuning
>> + * PHY type: "sdhc phy", "emmc phy 5.0" or "emmc phy 5.1"
>> + */
>> + if (of_property_read_bool(np, "marvell,xenon-emmc"))
>> + priv->emmc_slot = true;
>
> So, you need this because of the eMMC voltage switch behaviour, right?
>
> Then I would rather like to describe this a generic DT bindings for
> the eMMC voltage level support. There have acutally been some earlier
> discussions for this, but we haven't yet made some changes.
>
> I think what is missing is a mmc-ddr-3_3v DT binding, which when set,
> allows the host driver to accept I/O voltage switches to 3.3V. If not
> supported the ->start_signal_voltage_switch() ops may return -EINVAL.
> This would inform the mmc core to move on to the next supported
> voltage level. There might be some minor additional changes to the mmc
> card initialization sequence, but those should be simple.
>
> I can help out to look into this, unless you want to do it yourself of course!?
>
Yes. One of the reasons is to provide eMMC specific voltage setting.
But in my very own opinion, it should be irrelevant to voltage level.
The eMMC voltage setting on our SDHC is different from SD/SDIO voltage switch.
It will become more complex with different SOC implementation details.
Unfortunately, MMC driver cannot determine the card type yet when eMMC voltage
setting should be executed.
Thus an flag is required here to tell driver to execute eMMC voltage setting.
Besides, additional eMMC specific settings might be implemented in future, besides
voltage setting. Most of them should be completed before MMC driver recognizes the
card type. Thus I have to keep this flag to indicate current SDHC is for eMMC.
>> + else
>> + priv->emmc_slot = false;
>> +
>> + if (!of_property_read_u32(np, "marvell,xenon-slotno", &slot_idx)) {
>> + nr_slot = sdhci_readl(host, SDHC_SYS_CFG_INFO);
>> + nr_slot &= NR_SUPPORTED_SLOT_MASK;
>> + if (unlikely(slot_idx > nr_slot)) {
>> + dev_err(mmc_dev(mmc), "Slot Index %d exceeds Number of slots %d\n",
>> + slot_idx, nr_slot);
>> + return -EINVAL;
>> + }
>> + } else {
>> + priv->slot_idx = 0x0;
>> + }
>> +
>> + if (!of_property_read_u32(np, "marvell,xenon-tun-count",
>> + &tuning_count)) {
>> + if (unlikely(tuning_count >= TMR_RETUN_NO_PRESENT)) {
>> + dev_err(mmc_dev(mmc), "Wrong Re-tuning Count. Set default value %d\n",
>> + DEF_TUNING_COUNT);
>> + tuning_count = DEF_TUNING_COUNT;
>> + }
>> + } else {
>> + priv->tuning_count = DEF_TUNING_COUNT;
>> + }
>
> To make the code a bit easier...
>
> Maybe set "priv->tuning_count = DEF_TUNING_COUNT" before the "if", and
> instead have the of_property_read_u32() to update the value when set.
>
Yes. You are correct.
>> +
>> + if (of_property_read_bool(np, "marvell,xenon-mask-conflict-err")) {
>> + reg = sdhci_readl(host, SDHC_SYS_EXT_OP_CTRL);
>> + reg |= MASK_CMD_CONFLICT_ERROR;
>> + sdhci_writel(host, reg, SDHC_SYS_EXT_OP_CTRL);
>> + }
>> +
>> + return err;
>> +}
>> +
>> +static int xenon_slot_probe(struct sdhci_host *host)
>> +{
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> + u8 slot_idx = priv->slot_idx;
>> +
>> + /* Enable slot */
>> + xenon_enable_slot(host, slot_idx);
>> +
>> + /* Enable ACG */
>> + xenon_set_acg(host, true);
>> +
>> + /* Enable Parallel Transfer Mode */
>> + xenon_enable_slot_parallel_tran(host, slot_idx);
>> +
>> + priv->timing = MMC_TIMING_FAKE;
>> + priv->clock = 0;
>
> What are these used for?
>
During card initialization, our SDHC PHY setting depends on current
timing and SDCLK frequency.
priv->timing and priv->clock will be used in PHY setting later.
It can be considered as a clean-up.
Anyway, it does look ugly. I will improve them after our PHY setting
passes your review.
>> +
>> + return 0;
>> +}
>> +
>> +static void xenon_slot_remove(struct sdhci_host *host)
>> +{
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> + u8 slot_idx = priv->slot_idx;
>> +
>> + /* disable slot */
>> + xenon_disable_slot(host, slot_idx);
>> +}
>> +
>> +static int sdhci_xenon_probe(struct platform_device *pdev)
>> +{
>> + struct sdhci_pltfm_host *pltfm_host;
>> + struct sdhci_host *host;
>> + struct clk *clk, *axi_clk;
>> + struct sdhci_xenon_priv *priv;
>> + int err;
>> +
>> + host = sdhci_pltfm_init(pdev, &sdhci_xenon_pdata,
>> + sizeof(struct sdhci_xenon_priv));
>> + if (IS_ERR(host))
>> + return PTR_ERR(host);
>> +
>> + pltfm_host = sdhci_priv(host);
>> + priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> + xenon_set_acg(host, false);
>> +
>> + /*
>> + * Link Xenon specific mmc_host_ops function,
>> + * to replace standard ones in sdhci_ops.
>> + */
>> + xenon_replace_mmc_host_ops(host);
>> +
>> + clk = devm_clk_get(&pdev->dev, "core");
>> + if (IS_ERR(clk)) {
>> + dev_err(&pdev->dev, "Failed to setup input clk.\n");
>> + err = PTR_ERR(clk);
>> + goto free_pltfm;
>> + }
>> + clk_prepare_enable(clk);
>
> Check error code.
>
>> + pltfm_host->clk = clk;
>
> Why not assign pltfm_host->clk immedately when doing devm_clk_get(),
> that would make this a bit cleaner, right?
>
Yes, of course.
>> +
>> + /*
>> + * Some SOCs require additional clock to
>> + * manage AXI bus clock.
>> + * It is optional.
>> + */
>> + axi_clk = devm_clk_get(&pdev->dev, "axi");
>> + if (!IS_ERR(axi_clk)) {
>> + clk_prepare_enable(axi_clk);
>> + priv->axi_clk = axi_clk;
>> + }
>
> Same comments as for the above core clock.
>
OK.
>> +
>> + err = xenon_probe_dt(pdev);
>> + if (err)
>> + goto err_clk;
>> +
>> + err = xenon_slot_probe(host);
>> + if (err)
>> + goto err_clk;
>> +
>> + err = sdhci_add_host(host);
>> + if (err)
>> + goto remove_slot;
>> +
>> + return 0;
>> +
>> +remove_slot:
>> + xenon_slot_remove(host);
>> +err_clk:
>> + clk_disable_unprepare(pltfm_host->clk);
>> + if (!IS_ERR(axi_clk))
>> + clk_disable_unprepare(axi_clk);
>> +free_pltfm:
>> + sdhci_pltfm_free(pdev);
>> + return err;
>> +}
>> +
>> +static int sdhci_xenon_remove(struct platform_device *pdev)
>> +{
>> + struct sdhci_host *host = platform_get_drvdata(pdev);
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> + int dead = (readl(host->ioaddr + SDHCI_INT_STATUS) == 0xFFFFFFFF);
>> +
>> + xenon_slot_remove(host);
>> +
>> + sdhci_remove_host(host, dead);
>> +
>> + clk_disable_unprepare(pltfm_host->clk);
>> + clk_disable_unprepare(priv->axi_clk);
>> +
>> + sdhci_pltfm_free(pdev);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id sdhci_xenon_dt_ids[] = {
>> + { .compatible = "marvell,xenon-sdhci",},
>> + { .compatible = "marvell,armada-3700-sdhci",},
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, sdhci_xenon_dt_ids);
>> +
>> +static struct platform_driver sdhci_xenon_driver = {
>> + .driver = {
>> + .name = "xenon-sdhci",
>> + .of_match_table = sdhci_xenon_dt_ids,
>> + .pm = &sdhci_pltfm_pmops,
>> + },
>> + .probe = sdhci_xenon_probe,
>> + .remove = sdhci_xenon_remove,
>> +};
>> +
>> +module_platform_driver(sdhci_xenon_driver);
>> +
>> +MODULE_DESCRIPTION("SDHCI platform driver for Marvell Xenon SDHC");
>> +MODULE_AUTHOR("Hu Ziji <huziji@marvell.com>");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/mmc/host/sdhci-xenon.h b/drivers/mmc/host/sdhci-xenon.h
>> new file mode 100644
>> index 000000000000..4601d0a4b22f
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci-xenon.h
>
> I don't think you need a specific header for this, let's instead just
> put everthing in the c-file.
>
Some definitions inside this file will also be referred in PHY setting in
sdhci-xenon-phy.c.
Thus I put all the definitions together into a header file.
>> @@ -0,0 +1,142 @@
>> +/*
>> + * Copyright (C) 2016 Marvell, All Rights Reserved.
>> + *
>> + * Author: Hu Ziji <huziji@marvell.com>
>> + * Date: 2016-8-24
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation version 2.
>> + */
>> +#ifndef SDHCI_XENON_H_
>> +#define SDHCI_XENON_H_
>> +
>> +#include <linux/clk.h>
>> +#include <linux/mmc/card.h>
>> +#include <linux/of.h>
>> +#include "sdhci.h"
>> +
>> +/* Register Offset of SD Host Controller SOCP self-defined register */
>> +#define SDHC_SYS_CFG_INFO 0x0104
>> +#define SLOT_TYPE_SDIO_SHIFT 24
>> +#define SLOT_TYPE_EMMC_MASK 0xFF
>> +#define SLOT_TYPE_EMMC_SHIFT 16
>> +#define SLOT_TYPE_SD_SDIO_MMC_MASK 0xFF
>> +#define SLOT_TYPE_SD_SDIO_MMC_SHIFT 8
>> +#define NR_SUPPORTED_SLOT_MASK 0x7
>> +
>> +#define SDHC_SYS_OP_CTRL 0x0108
>> +#define AUTO_CLKGATE_DISABLE_MASK BIT(20)
>> +#define SDCLK_IDLEOFF_ENABLE_SHIFT 8
>> +#define SLOT_ENABLE_SHIFT 0
>> +
>> +#define SDHC_SYS_EXT_OP_CTRL 0x010C
>> +#define MASK_CMD_CONFLICT_ERROR BIT(8)
>> +
>> +#define SDHC_SLOT_OP_STATUS_CTRL 0x0128
>> +#define DELAY_90_DEGREE_MASK_EMMC5 BIT(7)
>> +#define DELAY_90_DEGREE_SHIFT_EMMC5 7
>> +#define EMMC_5_0_PHY_FIXED_DELAY_MASK 0x7F
>> +#define EMMC_PHY_FIXED_DELAY_MASK 0xFF
>> +#define EMMC_PHY_FIXED_DELAY_WINDOW_MIN (EMMC_PHY_FIXED_DELAY_MASK >> 3)
>> +#define SDH_PHY_FIXED_DELAY_MASK 0x1FF
>> +#define SDH_PHY_FIXED_DELAY_WINDOW_MIN (SDH_PHY_FIXED_DELAY_MASK >> 4)
>> +
>> +#define TUN_CONSECUTIVE_TIMES_SHIFT 16
>> +#define TUN_CONSECUTIVE_TIMES_MASK 0x7
>> +#define TUN_CONSECUTIVE_TIMES 0x4
>> +#define TUNING_STEP_SHIFT 12
>> +#define TUNING_STEP_MASK 0xF
>> +#define TUNING_STEP_DIVIDER BIT(6)
>> +
>> +#define FORCE_SEL_INVERSE_CLK_SHIFT 11
>> +
>> +#define SDHC_SLOT_EMMC_CTRL 0x0130
>> +#define ENABLE_DATA_STROBE BIT(24)
>> +#define SET_EMMC_RSTN BIT(16)
>> +#define DISABLE_RD_DATA_CRC BIT(14)
>> +#define DISABLE_CRC_STAT_TOKEN BIT(13)
>> +#define EMMC_VCCQ_MASK 0x3
>> +#define EMMC_VCCQ_1_8V 0x1
>> +#define EMMC_VCCQ_3_3V 0x3
>> +
>> +#define SDHC_SLOT_RETUNING_REQ_CTRL 0x0144
>> +/* retuning compatible */
>> +#define RETUNING_COMPATIBLE 0x1
>> +
>> +#define SDHC_SLOT_EXT_PRESENT_STATE 0x014C
>> +#define LOCK_STATE 0x1
>> +
>> +#define SDHC_SLOT_DLL_CUR_DLY_VAL 0x0150
>> +
>> +/* Tuning Parameter */
>> +#define TMR_RETUN_NO_PRESENT 0xF
>> +#define DEF_TUNING_COUNT 0x9
>> +
>> +#define MMC_TIMING_FAKE 0xFF
>> +
>> +#define DEFAULT_SDCLK_FREQ (400000)
>> +
>> +/* Xenon specific Mode Select value */
>> +#define XENON_SDHCI_CTRL_HS200 0x5
>> +#define XENON_SDHCI_CTRL_HS400 0x6
>
> For all defines above:
>
> All these defines needs some *SDHCI* prefix. Can you please update that.
Sure. Will add prefix for all of them.
>
>> +
>> +struct sdhci_xenon_priv {
>> + /*
>> + * The bus_width, timing, and clock fields in below
>> + * record the current setting of Xenon SDHC.
>> + * Driver will call a Sampling Fixed Delay Adjustment
>> + * if any setting is changed.
>> + */
>> + unsigned char bus_width;
>> + unsigned char timing;
>
> These two are not used. Please remove.
>
The above two variables will be used in PHY setting
in sdhci-xenon-phy.c.
Could you please help review them in next patch?
>> + unsigned char tuning_count;
>> + unsigned int clock;
>
> "clock" isn't used, please remove.
>
It will be accessed in PHY setting in sdhci-xenon-phy.c.
Could you please help review it in next patch?
>> + struct clk *axi_clk;
>> +
>> + /* Slot idx */
>> + u8 slot_idx;
>> + /* Whether this slot is for eMMC */
>> + bool emmc_slot;
>> +
>> + /*
>> + * When initializing card, Xenon has to determine card type and
>> + * adjust Sampling Fixed delay for the speed mode in which
>> + * DLL tuning is not support.
>> + * However, at that time, card structure is not linked to mmc_host.
>> + * Thus a card pointer is added here to provide
>> + * the delay adjustment function with the card structure
>> + * of the card during initialization.
>> + *
>> + * It is only valid during initialization after it is updated in
>> + * xenon_init_card().
>> + * Do not access this variable in normal transfers after
>> + * initialization completes.
>> + */
>> + struct mmc_card *card_candidate;
>
> Not activley used in this change, please remove and let's discuss it
> in the next step.
>
This varible will be accessed in PHY setting in sdhci-xenon-phy.c.
I would like to discuss about it in PHY file. Could you please help
review it in next patch?
Thank you.
Best regards,
Hu Ziji
>> +};
>> +
>> +static inline int enable_xenon_internal_clk(struct sdhci_host *host)
>> +{
>> + u32 reg;
>> + u8 timeout;
>> +
>> + reg = sdhci_readl(host, SDHCI_CLOCK_CONTROL);
>> + reg |= SDHCI_CLOCK_INT_EN;
>> + sdhci_writel(host, reg, SDHCI_CLOCK_CONTROL);
>> + /* Wait max 20 ms */
>> + timeout = 20;
>> + while (!((reg = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
>> + & SDHCI_CLOCK_INT_STABLE)) {
>> + if (timeout == 0) {
>> + pr_err("%s: Internal clock never stabilised.\n",
>> + mmc_hostname(host->mmc));
>> + return -ETIMEDOUT;
>> + }
>> + timeout--;
>> + mdelay(1);
>> + }
>> +
>> + return 0;
>> +}
>> +#endif
>> --
>> git-series 0.8.10
>
> Kind regards
> Uffe
>
^ permalink raw reply
* Re: [GIT PULL] DWMMC controller for next
From: Jaehoon Chung @ 2016-11-24 12:30 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc@vger.kernel.org
In-Reply-To: <19761c56-36a6-3a05-1299-d30a163c1968@samsung.com>
On 11/24/2016 08:50 PM, Jaehoon Chung wrote:
> On 11/24/2016 08:43 PM, Ulf Hansson wrote:
>> On 24 November 2016 at 12:17, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>> On 11/23/2016 09:31 PM, Jaehoon Chung wrote:
>>>> On 11/23/2016 08:50 PM, Jaehoon Chung wrote:
>>>>> On 11/23/2016 08:47 PM, Ulf Hansson wrote:
>>>>>> On 22 November 2016 at 17:13, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>>>> On 22 November 2016 at 10:59, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>>>>>>> Dear Ulf,
>>>>>>>>
>>>>>>>> Could you pull these patches on your next branch?
>>>>>>>> If there is a problem, let me know, plz.
>>>>>>>>
>>>>>>>> The following changes since commit e2081b37d910c5e6c6925d9b4d0a7a6283f84ec5:
>>>>>>>>
>>>>>>>> Merge branch 'fixes' into next (2016-11-21 11:09:18 +0100)
>>>>>>>>
>>>>>>>> are available in the git repository at:
>>>>>>>>
>>>>>>>> https://github.com/jh80chung/dw-mmc.git for-ulf
>>>>>>>>
>>>>>>>> for you to fetch changes up to e25fd245b557482a8e0f7ab87841085f30706f3a:
>>>>>>>>
>>>>>>>> Documentation: synopsys-dw-mshc: remove the unused properties (2016-11-22 10:34:05 +0900)
>>>>>>>>
>>>>>>>> ----------------------------------------------------------------
>>>>>>>> Colin Ian King (1):
>>>>>>>> mmc: dw_mmc: fix spelling mistake in dev_dbg message
>>>>>>>>
>>>>>>>> Jaehoon Chung (9):
>>>>>>>> mmc: dw_mmc: display the real register value on debugfs
>>>>>>>> mmc: dw_mmc: fix the debug message for checking card's present
>>>>>>>> mmc: dw_mmc: change the DW_MCI_FREQ_MIN from 400K to 100K
>>>>>>>> mmc: dw_mmc: use the hold register when send stop command
>>>>>>>> mmc: dw_mmc: call the dw_mci_prep_stop_abort() by default
>>>>>>>> mmc: dw_mmc: use the cookie's enum values for post/pre_req()
>>>>>>>> mmc: dw_mmc: remove the unnecessary mmc_data structure
>>>>>>>> mmc: dw_mmc: The "clock-freq-min-max" property was deprecated
>>>>>>>> Documentation: synopsys-dw-mshc: remove the unused properties
>>>>>>>>
>>>>>>>> Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt | 8 ++------
>>>>>>>> drivers/mmc/host/dw_mmc.c | 92 ++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------
>>>>>>>> include/linux/mmc/dw_mmc.h | 6 ++++++
>>>>>>>> 3 files changed, 52 insertions(+), 54 deletions(-)
>>>>>>>>
>>>>>>>> Best Regards,
>>>>>>>> Jaehoon Chung
>>>>>>>
>>>>>>> Dear Jaehoon, thanks for the pull request. I have add queued it all up
>>>>>>> for my next branch!
>>>>>>>
>>>>>>>
>>>>>>> Kind regards
>>>>>>> Uffe
>>>>>>
>>>>>> Jaehoon,
>>>>>>
>>>>>> It seems like some of these patches breaks Exynos 5250-arndale. Can
>>>>>> you please have a look and see what you think?
>>>>>> https://kernelci.org/boot/all/job/ulfh/kernel/v4.9-rc5-78-g52eb5d4531e4/
>>>>>
>>>>> I will check and reply. Thanks for noticing.
>>>
>>> Ulf,
>>>
>>> It's strange..My patches didn't have any dependency...
>>> There is no any log for dwmmc..and even it doesn't initialize dwmmc controller.
>>>
>>> Did you think that this failure is my patches problem? What is your thinking?
>>
>> Well, I don't know but indeed it seems a bit odd. Maybe there are some
>> interdependency that changes the behaviour for something outside
>> dw_mmc, which triggers the problems!?
>>
>> It could of course also be completely unrelated.
>>
>> I do have the Arndale board available. I will try it out within a
>> couple of days, to see if I can find something.
>
> If there is effect..it seems that only below patch for clock..
>
> commit 38332b0fd6c06fcecc64bc238bbb7cf80ed6bc7b
> Author: Jaehoon Chung <jh80.chung@samsung.com>
> Date: Thu Nov 17 16:40:35 2016 +0900
>
> mmc: dw_mmc: change the DW_MCI_FREQ_MIN from 400K to 100K
>
> If there is no property "clock-freq-min-max", mmc->f_min should be set
> to 400K by default. But Some SoC can be used 100K.
> When 100K is used, MMC core will try to check from 400K to 100K.
>
> otherwise timing?
>
> I wonder when the below patches is applied, what happen..
>
> https://patchwork.kernel.org/patch/9445233/
> https://patchwork.kernel.org/patch/9445209/
> https://patchwork.kernel.org/patch/9445211/
>
> I will also check this continuously..to find before PR for next.
Dear Ulf,
It seems that related to the below patch.
https://lkml.org/lkml/2016/9/15/170
commit 212ca7c839abbec1d5ef4f29206b66b42192db22
Refs: v4.9-rc1-29-g212ca7c
Author: Viresh Kumar <viresh.kumar@linaro.org>
AuthorDate: Thu Oct 27 15:50:18 2016 +0530
Commit: Lee Jones <lee.jones@linaro.org>
CommitDate: Mon Nov 21 13:00:28 2016 +0000
Applied on v4.9-rc1..but i don't know why occurred this error.
Could you rebase your branch on v4.9-rc5 again?
Best Regards,
Jaehoon Chung
>
> Best Regards,
> Jaehoon Chung
>
>>
>> [...]
>>
>> Kind regards
>> Uffe
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
^ permalink raw reply
* [PATCH 1/1] mmc: mmc: Relax checking for switch errors after HS200 switch
From: Adrian Hunter @ 2016-11-24 12:02 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Jaehoon Chung, Chaotian Jing, Stephen Boyd,
Michael Walle, Yong Mao, Shawn Lin
In-Reply-To: <1479988969-1747-1-git-send-email-adrian.hunter@intel.com>
The JEDEC specification indicates CMD13 can be used after a HS200 switch
to check for errors. However in practice some boards experience CRC errors
in the CMD13 response. Consequently, for HS200, CRC errors are not a
reliable way to know the switch failed. If there really is a problem, we
would expect tuning will fail and the result ends up the same. So change
the error condition to ignore CRC errors in that case.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
drivers/mmc/core/mmc.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 3268fcd3378d..34d30e2a09ff 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1223,7 +1223,12 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
mmc_set_timing(host, MMC_TIMING_MMC_HS200);
err = mmc_switch_status(card);
- if (err)
+ /*
+ * For HS200, CRC errors are not a reliable way to know the switch
+ * failed. If there really is a problem, we would expect tuning will
+ * fail and the result ends up the same.
+ */
+ if (err && err != -EILSEQ)
goto out_err;
mmc_set_bus_speed(card);
@@ -1387,6 +1392,14 @@ static int mmc_select_hs200(struct mmc_card *card)
err = mmc_switch_status(card);
/*
+ * For HS200, CRC errors are not a reliable way to know the
+ * switch failed. If there really is a problem, we would expect
+ * tuning will fail and the result ends up the same.
+ */
+ if (err == -EILSEQ)
+ err = 0;
+
+ /*
* mmc_select_timing() assumes timing has not changed if
* it is a switch error.
*/
--
1.9.1
^ permalink raw reply related
* [PATCH 0/1] mmc: mmc: Relax checking for switch errors after HS200 switch
From: Adrian Hunter @ 2016-11-24 12:02 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Jaehoon Chung, Chaotian Jing, Stephen Boyd,
Michael Walle, Yong Mao, Shawn Lin
Hi
Here is a patch for an issue we discussed briefly here:
https://marc.info/?l=linux-mmc&m=147937852408104
Adrian Hunter (1):
mmc: mmc: Relax checking for switch errors after HS200 switch
drivers/mmc/core/mmc.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
Regards
Adrian
^ permalink raw reply
* Re: [GIT PULL] DWMMC controller for next
From: Jaehoon Chung @ 2016-11-24 11:50 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc@vger.kernel.org
In-Reply-To: <CAPDyKFqUjLqYT5nS7hsAULK6iDtvcv8a1Cqao_VCtTXm5X82cA@mail.gmail.com>
On 11/24/2016 08:43 PM, Ulf Hansson wrote:
> On 24 November 2016 at 12:17, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> On 11/23/2016 09:31 PM, Jaehoon Chung wrote:
>>> On 11/23/2016 08:50 PM, Jaehoon Chung wrote:
>>>> On 11/23/2016 08:47 PM, Ulf Hansson wrote:
>>>>> On 22 November 2016 at 17:13, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>>> On 22 November 2016 at 10:59, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>>>>>> Dear Ulf,
>>>>>>>
>>>>>>> Could you pull these patches on your next branch?
>>>>>>> If there is a problem, let me know, plz.
>>>>>>>
>>>>>>> The following changes since commit e2081b37d910c5e6c6925d9b4d0a7a6283f84ec5:
>>>>>>>
>>>>>>> Merge branch 'fixes' into next (2016-11-21 11:09:18 +0100)
>>>>>>>
>>>>>>> are available in the git repository at:
>>>>>>>
>>>>>>> https://github.com/jh80chung/dw-mmc.git for-ulf
>>>>>>>
>>>>>>> for you to fetch changes up to e25fd245b557482a8e0f7ab87841085f30706f3a:
>>>>>>>
>>>>>>> Documentation: synopsys-dw-mshc: remove the unused properties (2016-11-22 10:34:05 +0900)
>>>>>>>
>>>>>>> ----------------------------------------------------------------
>>>>>>> Colin Ian King (1):
>>>>>>> mmc: dw_mmc: fix spelling mistake in dev_dbg message
>>>>>>>
>>>>>>> Jaehoon Chung (9):
>>>>>>> mmc: dw_mmc: display the real register value on debugfs
>>>>>>> mmc: dw_mmc: fix the debug message for checking card's present
>>>>>>> mmc: dw_mmc: change the DW_MCI_FREQ_MIN from 400K to 100K
>>>>>>> mmc: dw_mmc: use the hold register when send stop command
>>>>>>> mmc: dw_mmc: call the dw_mci_prep_stop_abort() by default
>>>>>>> mmc: dw_mmc: use the cookie's enum values for post/pre_req()
>>>>>>> mmc: dw_mmc: remove the unnecessary mmc_data structure
>>>>>>> mmc: dw_mmc: The "clock-freq-min-max" property was deprecated
>>>>>>> Documentation: synopsys-dw-mshc: remove the unused properties
>>>>>>>
>>>>>>> Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt | 8 ++------
>>>>>>> drivers/mmc/host/dw_mmc.c | 92 ++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------
>>>>>>> include/linux/mmc/dw_mmc.h | 6 ++++++
>>>>>>> 3 files changed, 52 insertions(+), 54 deletions(-)
>>>>>>>
>>>>>>> Best Regards,
>>>>>>> Jaehoon Chung
>>>>>>
>>>>>> Dear Jaehoon, thanks for the pull request. I have add queued it all up
>>>>>> for my next branch!
>>>>>>
>>>>>>
>>>>>> Kind regards
>>>>>> Uffe
>>>>>
>>>>> Jaehoon,
>>>>>
>>>>> It seems like some of these patches breaks Exynos 5250-arndale. Can
>>>>> you please have a look and see what you think?
>>>>> https://kernelci.org/boot/all/job/ulfh/kernel/v4.9-rc5-78-g52eb5d4531e4/
>>>>
>>>> I will check and reply. Thanks for noticing.
>>
>> Ulf,
>>
>> It's strange..My patches didn't have any dependency...
>> There is no any log for dwmmc..and even it doesn't initialize dwmmc controller.
>>
>> Did you think that this failure is my patches problem? What is your thinking?
>
> Well, I don't know but indeed it seems a bit odd. Maybe there are some
> interdependency that changes the behaviour for something outside
> dw_mmc, which triggers the problems!?
>
> It could of course also be completely unrelated.
>
> I do have the Arndale board available. I will try it out within a
> couple of days, to see if I can find something.
If there is effect..it seems that only below patch for clock..
commit 38332b0fd6c06fcecc64bc238bbb7cf80ed6bc7b
Author: Jaehoon Chung <jh80.chung@samsung.com>
Date: Thu Nov 17 16:40:35 2016 +0900
mmc: dw_mmc: change the DW_MCI_FREQ_MIN from 400K to 100K
If there is no property "clock-freq-min-max", mmc->f_min should be set
to 400K by default. But Some SoC can be used 100K.
When 100K is used, MMC core will try to check from 400K to 100K.
otherwise timing?
I wonder when the below patches is applied, what happen..
https://patchwork.kernel.org/patch/9445233/
https://patchwork.kernel.org/patch/9445209/
https://patchwork.kernel.org/patch/9445211/
I will also check this continuously..to find before PR for next.
Best Regards,
Jaehoon Chung
>
> [...]
>
> Kind regards
> Uffe
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox