* Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality
From: Ziji Hu @ 2016-10-13 5:38 UTC (permalink / raw)
To: Adrian Hunter, Gregory CLEMENT, Ulf Hansson, linux-mmc
Cc: Hilbert Zhang, Andrew Lunn, Romain Perier, Liuliu Zhao, Peng Zhu,
Nadav Haklai, Jack(SH) Zhu, Victor Gu, Doug Jones, Jisheng Zhang,
Yehuda Yitschak, Marcin Wojtas, Xueping Liu, Shiwu Zhang, Yu Cao,
Sebastian Hesselbarth, devicetree, Jason Cooper,
Kostya Porotchkin, Rob Herring, Ryan Gao, Wei(SOCP) Liu,
linux-arm-kernel, Thomas Petazzoni
In-Reply-To: <7939a923-eb97-ca44-ac4c-f0a3cd4f45bc@intel.com>
Hi Adrian,
On 2016/10/12 21:07, Adrian Hunter wrote:
> On 12/10/16 14:58, Ziji Hu wrote:
>> Hi Adrian,
>>
>> Thank you very much for your review.
>> I will firstly fix the typo.
>>
>> On 2016/10/11 20:37, Adrian Hunter wrote:
<snip>
>>>> +
>>>> +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 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->card_candidate) {
>>>
>>> mmc_power_up() calls __mmc_set_signal_voltage() calls
>>> host->ops->start_signal_voltage_switch so priv->card_candidate could be an
>>> invalid reference to an old card.
>>>
>>> So that's not going to work if the card changes - not only for removable
>>> cards but even for eMMC if init fails and retries.
>>>
>> As you point out, this piece of code have defects, even though it actually works on Marvell multiple platforms, unless eMMC card is removable.
>>
>> I can add a property to explicitly indicate eMMC type in DTS.
>> Then card_candidate access can be removed here.
>> Does it sounds more reasonable to you?
>
> Sure
>
>>
>>>> + if (mmc_card_mmc(priv->card_candidate))
>>>> + return xenon_emmc_signal_voltage_switch(mmc, ios);
>>>
>>> So if all you need to know is whether it is a eMMC, why can't DT tell you?
>>>
>> I can add an eMMC type property in DTS, to remove the card_candidate access here.
>>
>>>> + }
>>>> +
>>>> + 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;
>>>
>>> You really need a better way to get the card. I suggest you take up the
>>> issue with Ulf. One possibility is to have mmc core set host->card = card
>>> much earlier.
>>>
>> Could you please tell me if any issue related to card_candidate still exists, after the card_candidate is removed from xenon_start_signal_voltage_switch() in above?
>> It seems that when init_card is called, the structure card has already been updated and stable in MMC/SD/SDIO initialization sequence.
>> May I keep it here?
>
> It works by accident rather than by design. We can do better.
>
Could you please tell me some details which are satisfied about card_candidate?
I must admit that card_candidate in xenon_start_signal_voltage_switch() is imperfect.
But card_candidate in init_card() and later in set_ios() work by design, rather than by accident. We did a lot of tests on several platforms.
The structure mmc_card passed in here is a stable one. Thus in my very own opinion, it is safe and stable to use mmc_card here.
card_candidate is used only in card initialization. It is not active in later transfers after initialization is done.
It will always updated with mmc_card in next card initialization.
>>
>>>> + /* Set tuning functionality of this slot */
>>>> + xenon_slot_tuning_setup(host);
>>>> +
>>>> + slot_idx = priv->slot_idx;
>>>> + if (!mmc_card_sdio(card)) {
>>>> + /* Re-enable the Auto-CMD12 cap flag. */
>>>> + host->quirks |= SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
>>>> + host->flags |= SDHCI_AUTO_CMD12;
>>>> +
>>>> + /* Clear SDIO Card Inserted indication */
>>>> + 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;
>>>> + }
>>>> + } else {
>>>> + /*
>>>> + * Delete the Auto-CMD12 cap flag.
>>>> + * Otherwise, when sending multi-block CMD53,
>>>> + * Driver will set Transfer Mode Register to enable Auto CMD12.
>>>> + * However, SDIO device cannot recognize CMD12.
>>>> + * Thus SDHC will time-out for waiting for CMD12 response.
>>>> + */
>>>> + host->quirks &= ~SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
>>>> + host->flags &= ~SDHCI_AUTO_CMD12;
>>>
>>> sdhci_set_transfer_mode() won't enable auto-CMD12 for CMD53 anyway, so is
>>> this needed?
>>>
>> In Xenon driver, Auto-CMD12 flag is set to enable full support to Auto-CMD feature, both Auto-CMD12 and Auto-CMD23.
>> As a result, when Xenon SDHC slot can both support SD and SDIO, Auto-CMD12 is disabled when SDIO card is inserted, and renabled when SD is inserted.
>>
>> I recheck the sdhci code to set Auto-CMD bit in Transfer Mode register, in sdhci_set_transfer_mode():
>> if (mmc_op_multi(cmd->opcode) || data->blocks > 1)
>> As you can see, as long as it is CMD18/CMD25 OR there are multiple data blocks, Auto-CMD field will be set.
>> CMD53 doesn't have CMD23. Thus Auto-CMD12 is selected since Auto-CMD12 flag is set.
>> Thus I have to clear Auto-CMD12 to avoid issuing Auto-CMD12 in SDIO transfer.
>
>
> The code is:
>
> if (mmc_op_multi(cmd->opcode) || data->blocks > 1) {
> mode = SDHCI_TRNS_BLK_CNT_EN | SDHCI_TRNS_MULTI;
> /*
> * If we are sending CMD23, CMD12 never gets sent
> * on successful completion (so no Auto-CMD12).
> */
> if (sdhci_auto_cmd12(host, cmd->mrq) &&
> (cmd->opcode != SD_IO_RW_EXTENDED))
> mode |= SDHCI_TRNS_AUTO_CMD12;
> else if (cmd->mrq->sbc && (host->flags & SDHCI_AUTO_CMD23)) {
> mode |= SDHCI_TRNS_AUTO_CMD23;
> sdhci_writel(host, cmd->mrq->sbc->arg, SDHCI_ARGUMENT2);
> }
> }
>
> You can see the check for SD_IO_RW_EXTENDED which is CMD53.
>
Sorry. I didn't notice CMD53 check was added.
I introduced this Auto-CMD12 hack since kernel 3.8. It seems that this check is not added in kernel 3.8.
Thanks for the information. I will remove the Auto-CMD12 hack.
>>
>> I just meet a similar issue in RPMB.
>> When Auto-CMD12 flag is set, eMMC RPMB access will trigger Auto-CMD12, since CMD25 is in use.
>> It will cause RPMB access failed.
>
> Can you explain more about the RPMB issue. Doesn't it use CMD23, so CMD12
> wouldn't be used - auto or manually.
>
RPMB go through the MMC ioctl routine.
Unlike normal data transfer, MMC ioctl for RPMB explicitly issues CMD23. When CMD25 is issued, there is neither data->sbc nor Auto-CMD23.
As a result, sdhci driver will automatically enable Auto-CMD12 for RPMB CMD25 if Auto-CMD12 flag is set.
>>
>> One possible solution is to drop Auto-CMD12 support and use Auto-CMD23 only, in Xenon driver.
>> May I know you opinion, please?
>
> I don't use auto-CMD12 because I don't know if it provides any benefit and
> sdhci does not seem to have implemented Auto CMD12 Error Recovery, although
> I have never looked at it closely.
>
Actually, Auto-CMD23 is always used on our Xenon. Auto-CMD12 is not used at all.
But since this driver is a general one for all Marvell products, Auto-CMD12 is also supported in case that Auto-CMD23 is not available.
^ permalink raw reply
* Re: [RFC v2 1/2] sdhci: Add device tree property broken-highspeed
From: Jaehoon Chung @ 2016-10-13 2:27 UTC (permalink / raw)
To: Zach Brown, ulf.hansson
Cc: adrian.hunter, robh+dt, mark.rutland, linux-mmc, devicetree,
linux-kernel
In-Reply-To: <1476297656-7019-2-git-send-email-zach.brown@ni.com>
On 10/13/2016 03:40 AM, Zach Brown wrote:
> Certain board configurations can make highspeed malfuction due to timing
> issues. In these cases a way is needed to force the controller and card
> into standard speed even if they otherwise appear to be capable of
> highspeed.
I'm not sure "broken-hishspeed" property is relevant to only sdhci driver? otherwise on card side..?
Subject looks like only sdhci driver..And your RFC patch should be supported only SDHCI driver.
But your [patch 1/2] can cause the confusing in future..because there are just comment as "controller".
Then developer who refer to mmc.txt will add the property "broken-highspeed".
I'm not sure what is correct way..but to prevent confusing..you mentioned only "sdhci" driver or supporting other controller.
Best Regards,
Jaehoon Chung
>
> The broken-highspeed property will let the sdhci driver know that
> highspeed will not work.
>
> Signed-off-by: Zach Brown <zach.brown@ni.com>
> ---
> Documentation/devicetree/bindings/mmc/mmc.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
> index 8a37782..a2b298c 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
> @@ -52,6 +52,8 @@ Optional properties:
> - no-sdio: controller is limited to send sdio cmd during initialization
> - no-sd: controller is limited to send sd cmd during initialization
> - no-mmc: controller is limited to send mmc cmd during initialization
> +- broken-highspeed: Highspeed is broken, even if the controller and card
> + themselves claim they support highspeed.
>
> *NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line
> polarity properties, we have to fix the meaning of the "normal" and "inverted"
>
^ permalink raw reply
* [RFC v2 2/2] sdhci: Prevent SD from doing high-speed timing when broken-highspeed property is set
From: Zach Brown @ 2016-10-12 18:40 UTC (permalink / raw)
To: ulf.hansson
Cc: adrian.hunter, robh+dt, mark.rutland, linux-mmc, devicetree,
linux-kernel, zach.brown
In-Reply-To: <1476297656-7019-1-git-send-email-zach.brown@ni.com>
When the broken-highspeed property is set the sdhci driver will not
go into highspeed mode even if the controller and card appear to
otherwise support highspeed mode.
This is useful in cases where the controller and card support highspeed,
but the board configuration or some other issue make highspeed
impossible. For example, we send the SDIO lines through a fpga so we
need the data to change on the falling edge of the clock or there will
be issues with hold time.
Signed-off-by: Zach Brown <zach.brown@ni.com>
---
drivers/mmc/host/sdhci.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 4805566..17e6c50 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -22,6 +22,7 @@
#include <linux/scatterlist.h>
#include <linux/regulator/consumer.h>
#include <linux/pm_runtime.h>
+#include <linux/of.h>
#include <linux/leds.h>
@@ -3274,7 +3275,8 @@ int sdhci_setup_host(struct sdhci_host *host)
if (host->quirks2 & SDHCI_QUIRK2_HOST_NO_CMD23)
mmc->caps &= ~MMC_CAP_CMD23;
- if (host->caps & SDHCI_CAN_DO_HISPD)
+ if ((host->caps & SDHCI_CAN_DO_HISPD) &&
+ !(of_property_read_bool(mmc_dev(mmc)->of_node, "broken-highspeed")))
mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
--
2.7.4
^ permalink raw reply related
* [RFC v2 0/2] Add device tree property and driver behavior for supporting sdhci configurations with broken highspeed.
From: Zach Brown @ 2016-10-12 18:40 UTC (permalink / raw)
To: ulf.hansson
Cc: adrian.hunter, robh+dt, mark.rutland, linux-mmc, devicetree,
linux-kernel, zach.brown
Some board configurations can not support sd highspeed mode due to the distance
between the card slot and the controller. The card and controller report that
they are capable of highspeed however, so we need a mechanism for specifying
that the setup is incapable of supporting highspeed mode.
The first patch adds documentation about a new devicetree property
sd-broken-highspeed.
The second patch keeps the sd controller and card from going into highspeed
mode when the property is set.
v2:
* changed sd-broken-highspeed to broken-highspeed
* Removed new quirk, instead read of property when would've checked quirk
Zach Brown (2):
sdhci: Add device tree property broken-highspeed
sdhci: Prevent SD from doing high-speed timing when broken-highspeed
property is set
Documentation/devicetree/bindings/mmc/mmc.txt | 2 ++
drivers/mmc/host/sdhci.c | 4 +++-
2 files changed, 5 insertions(+), 1 deletion(-)
--
2.7.4
^ permalink raw reply
* [RFC v2 1/2] sdhci: Add device tree property broken-highspeed
From: Zach Brown @ 2016-10-12 18:40 UTC (permalink / raw)
To: ulf.hansson-QSEj5FYQhm4dnm+yROfE0A
Cc: adrian.hunter-ral2JQCrhuEAvxtiuMwx3w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-mmc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, zach.brown-acOepvfBmUk
In-Reply-To: <1476297656-7019-1-git-send-email-zach.brown-acOepvfBmUk@public.gmane.org>
Certain board configurations can make highspeed malfuction due to timing
issues. In these cases a way is needed to force the controller and card
into standard speed even if they otherwise appear to be capable of
highspeed.
The broken-highspeed property will let the sdhci driver know that
highspeed will not work.
Signed-off-by: Zach Brown <zach.brown-acOepvfBmUk@public.gmane.org>
---
Documentation/devicetree/bindings/mmc/mmc.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
index 8a37782..a2b298c 100644
--- a/Documentation/devicetree/bindings/mmc/mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc.txt
@@ -52,6 +52,8 @@ Optional properties:
- no-sdio: controller is limited to send sdio cmd during initialization
- no-sd: controller is limited to send sd cmd during initialization
- no-mmc: controller is limited to send mmc cmd during initialization
+- broken-highspeed: Highspeed is broken, even if the controller and card
+ themselves claim they support highspeed.
*NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line
polarity properties, we have to fix the meaning of the "normal" and "inverted"
--
2.7.4
--
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 related
* [PATCH 2/2] mmc: sdhci-iproc: support standard byte register accesses
From: Scott Branden @ 2016-10-12 18:35 UTC (permalink / raw)
To: Ulf Hansson, Rob Herring, Mark Rutland, Ray Jui, Scott Branden,
Adrian Hunter
Cc: BCM Kernel Feedback, linux-mmc, devicetree, linux-arm-kernel,
linux-kernel, Scott Branden, Srinath Mannam
In-Reply-To: <1476297352-7812-1-git-send-email-scott.branden@broadcom.com>
Add bytewise register accesses support for newer versions of IPROC
SDHCI controllers.
Previous sdhci-iproc versions of SDIO controllers
(such as Raspberry Pi and Cygnus) only allowed for 32-bit register
accesses.
Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
drivers/mmc/host/sdhci-iproc.c | 35 +++++++++++++++++++++++++++++++++--
1 file changed, 33 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
index 7262466..d7046d6 100644
--- a/drivers/mmc/host/sdhci-iproc.c
+++ b/drivers/mmc/host/sdhci-iproc.c
@@ -143,6 +143,14 @@ static void sdhci_iproc_writeb(struct sdhci_host *host, u8 val, int reg)
}
static const struct sdhci_ops sdhci_iproc_ops = {
+ .set_clock = sdhci_set_clock,
+ .get_max_clock = sdhci_pltfm_clk_get_max_clock,
+ .set_bus_width = sdhci_set_bus_width,
+ .reset = sdhci_reset,
+ .set_uhs_signaling = sdhci_set_uhs_signaling,
+};
+
+static const struct sdhci_ops sdhci_iproc_32only_ops = {
.read_l = sdhci_iproc_readl,
.read_w = sdhci_iproc_readw,
.read_b = sdhci_iproc_readb,
@@ -156,6 +164,28 @@ static const struct sdhci_ops sdhci_iproc_ops = {
.set_uhs_signaling = sdhci_set_uhs_signaling,
};
+static const struct sdhci_pltfm_data sdhci_iproc_cygnus_pltfm_data = {
+ .quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK,
+ .quirks2 = SDHCI_QUIRK2_ACMD23_BROKEN,
+ .ops = &sdhci_iproc_32only_ops,
+};
+
+static const struct sdhci_iproc_data iproc_cygnus_data = {
+ .pdata = &sdhci_iproc_cygnus_pltfm_data,
+ .caps = ((0x1 << SDHCI_MAX_BLOCK_SHIFT)
+ & SDHCI_MAX_BLOCK_MASK) |
+ SDHCI_CAN_VDD_330 |
+ SDHCI_CAN_VDD_180 |
+ SDHCI_CAN_DO_SUSPEND |
+ SDHCI_CAN_DO_HISPD |
+ SDHCI_CAN_DO_ADMA2 |
+ SDHCI_CAN_DO_SDMA,
+ .caps1 = SDHCI_DRIVER_TYPE_C |
+ SDHCI_DRIVER_TYPE_D |
+ SDHCI_SUPPORT_DDR50,
+ .mmc_caps = MMC_CAP_1_8V_DDR,
+};
+
static const struct sdhci_pltfm_data sdhci_iproc_pltfm_data = {
.quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK,
.quirks2 = SDHCI_QUIRK2_ACMD23_BROKEN,
@@ -182,7 +212,7 @@ static const struct sdhci_pltfm_data sdhci_bcm2835_pltfm_data = {
.quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION |
SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK |
SDHCI_QUIRK_MISSING_CAPS,
- .ops = &sdhci_iproc_ops,
+ .ops = &sdhci_iproc_32only_ops,
};
static const struct sdhci_iproc_data bcm2835_data = {
@@ -194,7 +224,8 @@ static const struct sdhci_iproc_data bcm2835_data = {
static const struct of_device_id sdhci_iproc_of_match[] = {
{ .compatible = "brcm,bcm2835-sdhci", .data = &bcm2835_data },
- { .compatible = "brcm,sdhci-iproc-cygnus", .data = &iproc_data },
+ { .compatible = "brcm,sdhci-iproc-cygnus", .data = &iproc_cygnus_data},
+ { .compatible = "brcm,sdhci-iproc", .data = &iproc_data },
{ }
};
MODULE_DEVICE_TABLE(of, sdhci_iproc_of_match);
--
2.5.0
^ permalink raw reply related
* [PATCH 1/2] mmc: sdhci-iproc: Add brcm,sdhci-iproc compat string in bindings document
From: Scott Branden @ 2016-10-12 18:35 UTC (permalink / raw)
To: Ulf Hansson, Rob Herring, Mark Rutland, Ray Jui, Scott Branden,
Adrian Hunter
Cc: BCM Kernel Feedback, linux-mmc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Scott Branden, Anup Patel
In-Reply-To: <1476297352-7812-1-git-send-email-scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Adds brcm,sdhci-iproc compat string to DT bindings document for
the iProc SDHCI driver.
Signed-off-by: Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Signed-off-by: Scott Branden <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
---
Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt b/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
index be56d2b..aa58b94 100644
--- a/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
+++ b/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
@@ -7,6 +7,7 @@ Required properties:
- compatible : Should be one of the following
"brcm,bcm2835-sdhci"
"brcm,sdhci-iproc-cygnus"
+ "brcm,sdhci-iproc"
- clocks : The clock feeding the SDHCI controller.
--
2.5.0
--
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 related
* [PATCH 0/2] mmc: sdhci-iproc: Add byte register access support
From: Scott Branden @ 2016-10-12 18:35 UTC (permalink / raw)
To: Ulf Hansson, Rob Herring, Mark Rutland, Ray Jui, Scott Branden,
Adrian Hunter
Cc: BCM Kernel Feedback, linux-mmc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Scott Branden
Add brcm,sdhci-iproc compat string and code for support of newer versions of
sdhci-iproc controller that allow byte-wise register accesses.
Scott Branden (2):
mmc: sdhci-iproc: Add brcm,sdhci-iproc compat string in bindings
document
mmc: sdhci-iproc: support standard byte register accesses
.../devicetree/bindings/mmc/brcm,sdhci-iproc.txt | 1 +
drivers/mmc/host/sdhci-iproc.c | 35 ++++++++++++++++++++--
2 files changed, 34 insertions(+), 2 deletions(-)
--
2.5.0
--
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: Adrian Hunter @ 2016-10-12 13:07 UTC (permalink / raw)
To: Ziji Hu, Gregory CLEMENT, Ulf Hansson, linux-mmc
Cc: Hilbert Zhang, Andrew Lunn, Romain Perier, Liuliu Zhao, Peng Zhu,
Nadav Haklai, Jack(SH) Zhu, Victor Gu, Doug Jones, Jisheng Zhang,
Yehuda Yitschak, Marcin Wojtas, Xueping Liu, Shiwu Zhang, Yu Cao,
Sebastian Hesselbarth, devicetree, Jason Cooper, Hanna Hawa,
Kostya Porotchkin, Rob Herring, Ryan Gao, Wei(SOCP) Liu,
linux-arm-kernel, Thomas Petazzoni <thomas>
In-Reply-To: <4009a66b-1e09-0780-b7c1-128ff1401b5f@marvell.com>
On 12/10/16 14:58, Ziji Hu wrote:
> Hi Adrian,
>
> Thank you very much for your review.
> I will firstly fix the typo.
>
> On 2016/10/11 20:37, Adrian Hunter wrote:
>> On 07/10/16 18:22, Gregory CLEMENT wrote:
>>> From: Ziji Hu <huziji@marvell.com>
>>>
>>> Add Xenon eMMC/SD/SDIO host controller core functionality.
>>> Add Xenon specific intialization process.
>>> Add Xenon specific mmc_host_ops APIs.
>>> Add Xenon specific register definitions.
>>>
>>> Add CONFIG_MMC_SDHCI_XENON support in drivers/mmc/host/Kconfig.
>>>
>>> Marvell Xenon SDHC conforms to SD Physical Layer Specification
>>> Version 3.01 and is designed according to the guidelines provided
>>> in the SD Host Controller Standard Specification Version 3.00.
>>>
>>> Signed-off-by: Hu Ziji <huziji@marvell.com>
>>> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>>
>> I looked at a couple of things but you need to sort out the issues with
>> card_candidate before going further.
>>
> Understood.
> I will improve the card_candidate. Please help check the details in below.
>
>>> ---
> <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;
>>> +}
>>> +
>>> +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 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->card_candidate) {
>>
>> mmc_power_up() calls __mmc_set_signal_voltage() calls
>> host->ops->start_signal_voltage_switch so priv->card_candidate could be an
>> invalid reference to an old card.
>>
>> So that's not going to work if the card changes - not only for removable
>> cards but even for eMMC if init fails and retries.
>>
> As you point out, this piece of code have defects, even though it actually works on Marvell multiple platforms, unless eMMC card is removable.
>
> I can add a property to explicitly indicate eMMC type in DTS.
> Then card_candidate access can be removed here.
> Does it sounds more reasonable to you?
Sure
>
>>> + if (mmc_card_mmc(priv->card_candidate))
>>> + return xenon_emmc_signal_voltage_switch(mmc, ios);
>>
>> So if all you need to know is whether it is a eMMC, why can't DT tell you?
>>
> I can add an eMMC type property in DTS, to remove the card_candidate access here.
>
>>> + }
>>> +
>>> + 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;
>>
>> You really need a better way to get the card. I suggest you take up the
>> issue with Ulf. One possibility is to have mmc core set host->card = card
>> much earlier.
>>
> Could you please tell me if any issue related to card_candidate still exists, after the card_candidate is removed from xenon_start_signal_voltage_switch() in above?
> It seems that when init_card is called, the structure card has already been updated and stable in MMC/SD/SDIO initialization sequence.
> May I keep it here?
It works by accident rather than by design. We can do better.
>
>>> + /* Set tuning functionality of this slot */
>>> + xenon_slot_tuning_setup(host);
>>> +
>>> + slot_idx = priv->slot_idx;
>>> + if (!mmc_card_sdio(card)) {
>>> + /* Re-enable the Auto-CMD12 cap flag. */
>>> + host->quirks |= SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
>>> + host->flags |= SDHCI_AUTO_CMD12;
>>> +
>>> + /* Clear SDIO Card Inserted indication */
>>> + 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;
>>> + }
>>> + } else {
>>> + /*
>>> + * Delete the Auto-CMD12 cap flag.
>>> + * Otherwise, when sending multi-block CMD53,
>>> + * Driver will set Transfer Mode Register to enable Auto CMD12.
>>> + * However, SDIO device cannot recognize CMD12.
>>> + * Thus SDHC will time-out for waiting for CMD12 response.
>>> + */
>>> + host->quirks &= ~SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
>>> + host->flags &= ~SDHCI_AUTO_CMD12;
>>
>> sdhci_set_transfer_mode() won't enable auto-CMD12 for CMD53 anyway, so is
>> this needed?
>>
> In Xenon driver, Auto-CMD12 flag is set to enable full support to Auto-CMD feature, both Auto-CMD12 and Auto-CMD23.
> As a result, when Xenon SDHC slot can both support SD and SDIO, Auto-CMD12 is disabled when SDIO card is inserted, and renabled when SD is inserted.
>
> I recheck the sdhci code to set Auto-CMD bit in Transfer Mode register, in sdhci_set_transfer_mode():
> if (mmc_op_multi(cmd->opcode) || data->blocks > 1)
> As you can see, as long as it is CMD18/CMD25 OR there are multiple data blocks, Auto-CMD field will be set.
> CMD53 doesn't have CMD23. Thus Auto-CMD12 is selected since Auto-CMD12 flag is set.
> Thus I have to clear Auto-CMD12 to avoid issuing Auto-CMD12 in SDIO transfer.
The code is:
if (mmc_op_multi(cmd->opcode) || data->blocks > 1) {
mode = SDHCI_TRNS_BLK_CNT_EN | SDHCI_TRNS_MULTI;
/*
* If we are sending CMD23, CMD12 never gets sent
* on successful completion (so no Auto-CMD12).
*/
if (sdhci_auto_cmd12(host, cmd->mrq) &&
(cmd->opcode != SD_IO_RW_EXTENDED))
mode |= SDHCI_TRNS_AUTO_CMD12;
else if (cmd->mrq->sbc && (host->flags & SDHCI_AUTO_CMD23)) {
mode |= SDHCI_TRNS_AUTO_CMD23;
sdhci_writel(host, cmd->mrq->sbc->arg, SDHCI_ARGUMENT2);
}
}
You can see the check for SD_IO_RW_EXTENDED which is CMD53.
>
> I just meet a similar issue in RPMB.
> When Auto-CMD12 flag is set, eMMC RPMB access will trigger Auto-CMD12, since CMD25 is in use.
> It will cause RPMB access failed.
Can you explain more about the RPMB issue. Doesn't it use CMD23, so CMD12
wouldn't be used - auto or manually.
>
> One possible solution is to drop Auto-CMD12 support and use Auto-CMD23 only, in Xenon driver.
> May I know you opinion, please?
I don't use auto-CMD12 because I don't know if it provides any benefit and
sdhci does not seem to have implemented Auto CMD12 Error Recovery, although
I have never looked at it closely.
^ permalink raw reply
* Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC
From: Ziji Hu @ 2016-10-12 12:17 UTC (permalink / raw)
To: Adrian Hunter, Gregory CLEMENT, Ulf Hansson, linux-mmc
Cc: Hilbert Zhang, Andrew Lunn, Romain Perier, Liuliu Zhao, Peng Zhu,
Nadav Haklai, Jack(SH) Zhu, Victor Gu, Doug Jones, Jisheng Zhang,
Yehuda Yitschak, Marcin Wojtas, Xueping Liu, Shiwu Zhang, Yu Cao,
Sebastian Hesselbarth, devicetree, Jason Cooper,
Kostya Porotchkin, Rob Herring, Ryan Gao, Wei(SOCP) Liu,
linux-arm-kernel, Thomas Petazzoni
In-Reply-To: <bfe30317-bb3b-b3dc-e9df-87b6ba100307@intel.com>
Hi Adrian,
On 2016/10/11 20:39, Adrian Hunter wrote:
> On 07/10/16 18:22, Gregory CLEMENT wrote:
>> From: Ziji Hu <huziji@marvell.com>
>>
>> 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@marvell.com>
>> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> ---
>> MAINTAINERS | 1 +-
>> drivers/mmc/host/Makefile | 2 +-
>> drivers/mmc/host/sdhci-xenon-phy.c | 1141 +++++++++++++++++++++++++++++-
>> 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, 1321 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c
>> create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 859420e5dfd3..b5673c2ee5f2 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7583,6 +7583,7 @@ M: Ziji Hu <huziji@marvell.com>
>> L: linux-mmc@vger.kernel.org
>> S: Supported
>> F: drivers/mmc/host/sdhci-xenon.*
>> +F: drivers/mmc/host/sdhci-xenon-phy.*
>> F: Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt
>>
>> MATROX FRAMEBUFFER DRIVER
>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>> index 75eaf743486c..4f2854556ff7 100644
>> --- a/drivers/mmc/host/Makefile
>> +++ b/drivers/mmc/host/Makefile
>> @@ -82,4 +82,4 @@ ifeq ($(CONFIG_CB710_DEBUG),y)
>> endif
>>
>> obj-$(CONFIG_MMC_SDHCI_XENON) += sdhci-xenon-driver.o
>> -sdhci-xenon-driver-y += sdhci-xenon.o
>> +sdhci-xenon-driver-y += sdhci-xenon.o sdhci-xenon-phy.o
>> diff --git a/drivers/mmc/host/sdhci-xenon-phy.c b/drivers/mmc/host/sdhci-xenon-phy.c
>> new file mode 100644
>> index 000000000000..4eb8fea1bec9
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci-xenon-phy.c
>
> <SNIP>
>
>> +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);
>> +
>> + 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;
>> +}
>> +
>> +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;
>> +}
>> +
>> +static int xenon_delay_adj_test(struct mmc_card *card)
>> +{
>> + WARN_ON(!card);
>> + WARN_ON(!card->host);
>> +
>> + if (mmc_card_mmc(card))
>> + return __xenon_emmc_delay_adj_test(card);
>> + else if (mmc_card_sd(card))
>> + return __xenon_sd_delay_adj_test(card);
>> + else if (mmc_card_sdio(card))
>> + return __xenon_sdio_delay_adj_test(card);
>> + else
>> + return -EINVAL;
>> +}
>
> So you are issuing commands from the ->set_ios() callback. I would want to
> get Ulf's OK for that before going further.
>
Yes, you are correct.
In some speed mode, Xenon SDHC has to send a series of transfers to search for a perfect sampling point in PHY delay line.
It is like tuning process.
> One thing: you will need to ensure you don't trigger get HS400 re-tuning
> because it will call back into ->set_ios().
>
Could you please make the term "HS400 re-tuning" more detailed?
In current MMC driver, "HS400 re-tuning" will go back to HS200, execute HS200 tuning and come back to HS400.
I'm sure our Xenon SDHC will not execute it.
However, in coming eMMC 5.2, there is a real HS400 re-tuning, in which tuning can be directly executed in HS400 mode.
Our Xenon SDHC will neither trigger this HS400 re-tuning.
But since so far there is no such feature in MMC driver, I cannot give you a 100% guarantee now.
> And you have the problem that you need to get a reference to the card before
> the card device has been added. As I wrote in response to the previous
> patch, you should get Ulf's help with that too.
>
Sure.
I will get card_candidate solved at first.
Thank you again for your review and help.
Thank you.
Best regards,
Hu Ziji
>
^ permalink raw reply
* Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality
From: Ziji Hu @ 2016-10-12 11:58 UTC (permalink / raw)
To: Adrian Hunter, Gregory CLEMENT, Ulf Hansson, linux-mmc
Cc: Jason Cooper, Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
devicetree, Thomas Petazzoni, linux-arm-kernel, Jack(SH) Zhu,
Jimmy Xu, Jisheng Zhang, Nadav Haklai, Ryan Gao, Doug Jones,
Shiwu Zhang, Victor Gu, Wei(SOCP) Liu, Wilson Ding, Xueping Liu,
Hilbert Zhang, Keji Zhang, Liuliu Zhao, Peng
In-Reply-To: <1b015f16-aa37-591c-3299-9431f6e82cce@intel.com>
Hi Adrian,
Thank you very much for your review.
I will firstly fix the typo.
On 2016/10/11 20:37, Adrian Hunter wrote:
> On 07/10/16 18:22, Gregory CLEMENT wrote:
>> From: Ziji Hu <huziji@marvell.com>
>>
>> Add Xenon eMMC/SD/SDIO host controller core functionality.
>> Add Xenon specific intialization process.
>> Add Xenon specific mmc_host_ops APIs.
>> Add Xenon specific register definitions.
>>
>> Add CONFIG_MMC_SDHCI_XENON support in drivers/mmc/host/Kconfig.
>>
>> Marvell Xenon SDHC conforms to SD Physical Layer Specification
>> Version 3.01 and is designed according to the guidelines provided
>> in the SD Host Controller Standard Specification Version 3.00.
>>
>> Signed-off-by: Hu Ziji <huziji@marvell.com>
>> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>
> I looked at a couple of things but you need to sort out the issues with
> card_candidate before going further.
>
Understood.
I will improve the card_candidate. Please help check the details in below.
>> ---
<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;
>> +}
>> +
>> +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 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->card_candidate) {
>
> mmc_power_up() calls __mmc_set_signal_voltage() calls
> host->ops->start_signal_voltage_switch so priv->card_candidate could be an
> invalid reference to an old card.
>
> So that's not going to work if the card changes - not only for removable
> cards but even for eMMC if init fails and retries.
>
As you point out, this piece of code have defects, even though it actually works on Marvell multiple platforms, unless eMMC card is removable.
I can add a property to explicitly indicate eMMC type in DTS.
Then card_candidate access can be removed here.
Does it sounds more reasonable to you?
>> + if (mmc_card_mmc(priv->card_candidate))
>> + return xenon_emmc_signal_voltage_switch(mmc, ios);
>
> So if all you need to know is whether it is a eMMC, why can't DT tell you?
>
I can add an eMMC type property in DTS, to remove the card_candidate access here.
>> + }
>> +
>> + 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;
>
> You really need a better way to get the card. I suggest you take up the
> issue with Ulf. One possibility is to have mmc core set host->card = card
> much earlier.
>
Could you please tell me if any issue related to card_candidate still exists, after the card_candidate is removed from xenon_start_signal_voltage_switch() in above?
It seems that when init_card is called, the structure card has already been updated and stable in MMC/SD/SDIO initialization sequence.
May I keep it here?
>> + /* Set tuning functionality of this slot */
>> + xenon_slot_tuning_setup(host);
>> +
>> + slot_idx = priv->slot_idx;
>> + if (!mmc_card_sdio(card)) {
>> + /* Re-enable the Auto-CMD12 cap flag. */
>> + host->quirks |= SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
>> + host->flags |= SDHCI_AUTO_CMD12;
>> +
>> + /* Clear SDIO Card Inserted indication */
>> + 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;
>> + }
>> + } else {
>> + /*
>> + * Delete the Auto-CMD12 cap flag.
>> + * Otherwise, when sending multi-block CMD53,
>> + * Driver will set Transfer Mode Register to enable Auto CMD12.
>> + * However, SDIO device cannot recognize CMD12.
>> + * Thus SDHC will time-out for waiting for CMD12 response.
>> + */
>> + host->quirks &= ~SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
>> + host->flags &= ~SDHCI_AUTO_CMD12;
>
> sdhci_set_transfer_mode() won't enable auto-CMD12 for CMD53 anyway, so is
> this needed?
>
In Xenon driver, Auto-CMD12 flag is set to enable full support to Auto-CMD feature, both Auto-CMD12 and Auto-CMD23.
As a result, when Xenon SDHC slot can both support SD and SDIO, Auto-CMD12 is disabled when SDIO card is inserted, and renabled when SD is inserted.
I recheck the sdhci code to set Auto-CMD bit in Transfer Mode register, in sdhci_set_transfer_mode():
if (mmc_op_multi(cmd->opcode) || data->blocks > 1)
As you can see, as long as it is CMD18/CMD25 OR there are multiple data blocks, Auto-CMD field will be set.
CMD53 doesn't have CMD23. Thus Auto-CMD12 is selected since Auto-CMD12 flag is set.
Thus I have to clear Auto-CMD12 to avoid issuing Auto-CMD12 in SDIO transfer.
I just meet a similar issue in RPMB.
When Auto-CMD12 flag is set, eMMC RPMB access will trigger Auto-CMD12, since CMD25 is in use.
It will cause RPMB access failed.
One possible solution is to drop Auto-CMD12 support and use Auto-CMD23 only, in Xenon driver.
May I know you opinion, please?
>> +
>> + /*
>> + * 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);
>> + }
>> +}
>> +
>> +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:
>> + * slotno: the index of slot. Refer to SDHC_SYS_CFG_INFO register
>> + * tuning-count: the interval between re-tuning
>> + * PHY type: "sdhc phy", "emmc phy 5.0" or "emmc phy 5.1"
>> + */
>> + if (!of_property_read_u32(np, "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, "xenon,tuning-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;
>> + }
>> +
>> + if (of_property_read_bool(np, "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;
>> +
>> + 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);
>> + pltfm_host->clk = clk;
>> +
>> + /*
>> + * 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;
>> + }
>> +
>> + 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,sdhci-xenon",},
>> + { .compatible = "marvell,armada-3700-sdhci",},
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, sdhci_xenon_dt_ids);
>> +
>> +static struct platform_driver sdhci_xenon_driver = {
>> + .driver = {
>> + .name = "sdhci-xenon",
>> + .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..c2370493fbe8
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci-xenon.h
>> @@ -0,0 +1,134 @@
>> +/*
>> + * 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
>> +
>> +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;
>> + unsigned char tuning_count;
>> + unsigned int clock;
>> + struct clk *axi_clk;
>> +
>> + /* Slot idx */
>> + u8 slot_idx;
>> +
>> + /*
>> + * When initializing card, Xenon has to determine card type and
>> + * adjust Sampling Fixed delay.
>> + * 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
>> + */
>> + struct mmc_card *card_candidate;
>> +};
>> +
>> +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
>>
>
^ permalink raw reply
* Re: Enabling MMC BKOPs in kernel based on host caps
From: Jaehoon Chung @ 2016-10-12 11:30 UTC (permalink / raw)
To: Ulf Hansson, Adrian Hunter
Cc: Ravikumar, Shawn Lin, Ravikumar Kattekola, linux-mmc,
Alex Lemberg
In-Reply-To: <CAPDyKFqCg6kuTHhuMNmtZcL6+8wOTky+_8SsvedcPbPcaaoohA@mail.gmail.com>
On 10/12/2016 04:58 PM, Ulf Hansson wrote:
> On 12 October 2016 at 09:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 05/10/16 14:55, Ravikumar wrote:
>>> Hi Shawn,
>>>
>>>
>>> On Wednesday 05 October 2016 03:07 PM, Shawn Lin wrote:
>>>> Hi Ravikumar,
>>>>
>>>> + Alex,
>>>>
>>>> 在 2016/10/3 18:43, Ravikumar Kattekola 写道:
>>>>> Hi all,
>>>>> I’ve seen an eMMC failure due to pending background operations on a
>>>>> certain OMAP device since bkops enable bit was not set.
>>
>> It is not clear what you are talking about. BKOPS_EN is
>> one-time-programmable so you can just set when the device is provisioned.
>>
>> However BKOPS_EN does not really enable background operations. It is an
>> indication from the host to the card about whether the card may delay
>> maintenance operations. The definition is:
>>
>> "This field allows the host to indicate to the device if it is expected to
>> periodically manually start background operations by writing to the
>> BKOPS_START field."
>>
>> Currently the kernel does not do periodic background operations, so having
>> the kernel set BKOPS_EN would not make sense - even if it wasn't
>> one-time-programmable.
>
> Agree!
>
> However, in cases when the BKOPS_EN bit has been set (in one way or
> the other), this do becomes a problem as we currently only supports
> urgent background operations.
> Perhaps we should print a warning when BKOPS_EN is set during card
> initialization, as this isn't currently supported.
When i had added the bkops features, i remembered that discussed about this.
(set BKOPS_EN from kernel..or not..)
When enable the bkops_en by default, there should be a problem...
(I'm not sure..but at that time..Chris had wanted to remove the enabling bkops by default.)
For just understanding..i added the changelog for my bkops patch.
https://patchwork.kernel.org/patch/1466501/
>
>>
>>>>> Further investigation showed me that someone already posted patch to
>>>>> enable Background operations in kernel based on a host capability check
>>>>> (Caps2 & BK_OPS_EN)
>>>>> but was turned down quoting that it should be enabled from user space
>>>>> using mmc-utils.
>>>>>
>>>>> Enabling this would add one additional check for exception event in the
>>>>> response R1 or R1B (only on hosts that explicitly set BK_OPS_EN in caps2).
>>>>> But not enabling this could lead to a system failure especially when the
>>>>> Filesystem is on eMMC and the card stops responding due to pending
>>>>> critical bkops.
>>
>> Yes the kernel only does urgent background operations if BKOPS_EN is set.
>> You seem to be suggesting that the cards are violating the specification by
>> delaying maintenance operations even though they are not allowed to because
>> BKOPS_EN is not set. Is that the case?
>>
>
> If this is the case, the solution would probably be to actually set
> the BKOPS_EN bit from the kernel, perhaps via using a card quirk.
>
> Although, to do that, we first need to add proper background
> operations support. I guess that is what Alex intends to do here [1].
>
> [...]
>
> [1]
> http://www.spinics.net/lists/linux-mmc/msg38952.html
>
> 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
* Re: Enabling MMC BKOPs in kernel based on host caps
From: Ulf Hansson @ 2016-10-12 7:58 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ravikumar, Shawn Lin, Ravikumar Kattekola, linux-mmc,
Alex Lemberg
In-Reply-To: <4cf40ed6-85c1-a879-ef7a-ff9d5c9b24fe@intel.com>
On 12 October 2016 at 09:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 05/10/16 14:55, Ravikumar wrote:
>> Hi Shawn,
>>
>>
>> On Wednesday 05 October 2016 03:07 PM, Shawn Lin wrote:
>>> Hi Ravikumar,
>>>
>>> + Alex,
>>>
>>> 在 2016/10/3 18:43, Ravikumar Kattekola 写道:
>>>> Hi all,
>>>> I’ve seen an eMMC failure due to pending background operations on a
>>>> certain OMAP device since bkops enable bit was not set.
>
> It is not clear what you are talking about. BKOPS_EN is
> one-time-programmable so you can just set when the device is provisioned.
>
> However BKOPS_EN does not really enable background operations. It is an
> indication from the host to the card about whether the card may delay
> maintenance operations. The definition is:
>
> "This field allows the host to indicate to the device if it is expected to
> periodically manually start background operations by writing to the
> BKOPS_START field."
>
> Currently the kernel does not do periodic background operations, so having
> the kernel set BKOPS_EN would not make sense - even if it wasn't
> one-time-programmable.
Agree!
However, in cases when the BKOPS_EN bit has been set (in one way or
the other), this do becomes a problem as we currently only supports
urgent background operations.
Perhaps we should print a warning when BKOPS_EN is set during card
initialization, as this isn't currently supported.
>
>>>> Further investigation showed me that someone already posted patch to
>>>> enable Background operations in kernel based on a host capability check
>>>> (Caps2 & BK_OPS_EN)
>>>> but was turned down quoting that it should be enabled from user space
>>>> using mmc-utils.
>>>>
>>>> Enabling this would add one additional check for exception event in the
>>>> response R1 or R1B (only on hosts that explicitly set BK_OPS_EN in caps2).
>>>> But not enabling this could lead to a system failure especially when the
>>>> Filesystem is on eMMC and the card stops responding due to pending
>>>> critical bkops.
>
> Yes the kernel only does urgent background operations if BKOPS_EN is set.
> You seem to be suggesting that the cards are violating the specification by
> delaying maintenance operations even though they are not allowed to because
> BKOPS_EN is not set. Is that the case?
>
If this is the case, the solution would probably be to actually set
the BKOPS_EN bit from the kernel, perhaps via using a card quirk.
Although, to do that, we first need to add proper background
operations support. I guess that is what Alex intends to do here [1].
[...]
[1]
http://www.spinics.net/lists/linux-mmc/msg38952.html
Kind regards
Uffe
^ permalink raw reply
* Re: Enabling MMC BKOPs in kernel based on host caps
From: Adrian Hunter @ 2016-10-12 7:09 UTC (permalink / raw)
To: Ravikumar, Shawn Lin, Ravikumar Kattekola, linux-mmc; +Cc: Alex Lemberg
In-Reply-To: <dfcdad0d-0780-0641-cb0a-46557334fa16@ti.com>
On 05/10/16 14:55, Ravikumar wrote:
> Hi Shawn,
>
>
> On Wednesday 05 October 2016 03:07 PM, Shawn Lin wrote:
>> Hi Ravikumar,
>>
>> + Alex,
>>
>> 在 2016/10/3 18:43, Ravikumar Kattekola 写道:
>>> Hi all,
>>> I’ve seen an eMMC failure due to pending background operations on a
>>> certain OMAP device since bkops enable bit was not set.
It is not clear what you are talking about. BKOPS_EN is
one-time-programmable so you can just set when the device is provisioned.
However BKOPS_EN does not really enable background operations. It is an
indication from the host to the card about whether the card may delay
maintenance operations. The definition is:
"This field allows the host to indicate to the device if it is expected to
periodically manually start background operations by writing to the
BKOPS_START field."
Currently the kernel does not do periodic background operations, so having
the kernel set BKOPS_EN would not make sense - even if it wasn't
one-time-programmable.
>>> Further investigation showed me that someone already posted patch to
>>> enable Background operations in kernel based on a host capability check
>>> (Caps2 & BK_OPS_EN)
>>> but was turned down quoting that it should be enabled from user space
>>> using mmc-utils.
>>>
>>> Enabling this would add one additional check for exception event in the
>>> response R1 or R1B (only on hosts that explicitly set BK_OPS_EN in caps2).
>>> But not enabling this could lead to a system failure especially when the
>>> Filesystem is on eMMC and the card stops responding due to pending
>>> critical bkops.
Yes the kernel only does urgent background operations if BKOPS_EN is set.
You seem to be suggesting that the cards are violating the specification by
delaying maintenance operations even though they are not allowed to because
BKOPS_EN is not set. Is that the case?
>>>
>>> I would like to ask for expert opinion on ‘why is it a bad idea to enable
>>> bkops in kernel?’
It is one-time-programmable so the kernel should not have to program it,
because it should have already been programmed when the device was provisioned.
>>
>> Some discussion about the similar topic could be found here:
>> https://patchwork.kernel.org/patch/9157121/
>>
>>
>>> It’s a one time programmable bit but if it helps in keeping system
>>> functional why not do it?
>>
>> Actually BKOPS_EN is not OTP bit.. Quoted from Ulf "I don't have any
>> issue to allow all non-OTP registers bits to be written." So I guess
>> you could do this, although it needs more discussion there.
> In the spec for 4.5, jesd84_B45 it does say not whether the ENABLE bit is OTP.
That is not entirely accurate. Table 75 clearly lists it as R/W meaning
one-time-programmable and readable.
> But in 5.1 spec, jesd84-b51, it says MANUAL_EN is R/W which means OTP and
> readable.
> As I read form mmc-utils -help
> " mmc bkops enable <device>
> Enable the eMMC BKOPS feature on <device>.
> NOTE! This is a one-time programmable (unreversible) change "
>>
>> But it's persistent EXT_CSD register and we get used to control it from
>> userspace, which is the policy we have been sticking to when writing to
>> persistent EXT_CSD registers. I guess that is nothing about "right and
>> wrong", just a rule for us in case someone wants to set the persistent
>> bit in kernel but setting other persistent bits from user-space, which
>> is prone to mess up the mmc core. Or, someone will sent mail to the list
>> asking "why is it a good idea to enable bkops in kernel" ? :)
> So there's no functional problem/reason that stops us from enabling BKOPS
> (Manual) in
> kernel except for consistency with other persistent registers.
> Since not enabling BKOPS could lead to a functional failure / non-responsive
> system
> at a later point of time I guess this could be exempted.
> what do you think?
>
> As user I would choose functional safety and reliability over performance.
> Hence it would make sense to have the bkops (at least manual) be enabled by
> default,
> especially in Automotive applications.
>
>>
>>> I haven’t measured the performance impact but I don’t see a reason for
>>> major drop because the frequency of critical bkops events would be less.
>>>
>>> Regards,
>>> RK
>>> --
>>> 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
>>>
>>
>>
> Thanks for your response.
>
> Regards,
> RK
>
> --
> 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 v2 6/9] mmc: dw_mmc-exynos: deploy runtime PM facilities
From: Shawn Lin @ 2016-10-12 2:55 UTC (permalink / raw)
To: Jaehoon Chung, Ulf Hansson
Cc: linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Shawn Lin,
linux-mmc-u79uwXL29TY76Z2rM5mHXA, Doug Anderson
In-Reply-To: <1476240643-5701-1-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Let's migrate it to use runtime PM and remove the system
PM callback from this driver. With this patch, it could
handle system PM properly and could also use runtime PM
if we enable it.
Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
Changes in v2:
- minor fix since I change the argument for dw_mci_runtime_*
drivers/mmc/host/dw_mmc-exynos.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
index 7ab3d74..e2439a1 100644
--- a/drivers/mmc/host/dw_mmc-exynos.c
+++ b/drivers/mmc/host/dw_mmc-exynos.c
@@ -17,6 +17,7 @@
#include <linux/mmc/mmc.h>
#include <linux/of.h>
#include <linux/of_gpio.h>
+#include <linux/pm_runtime.h>
#include <linux/slab.h>
#include "dw_mmc.h"
@@ -161,20 +162,13 @@ static void dw_mci_exynos_set_clksel_timing(struct dw_mci *host, u32 timing)
set_bit(DW_MMC_CARD_NO_USE_HOLD, &host->cur_slot->flags);
}
-#ifdef CONFIG_PM_SLEEP
-static int dw_mci_exynos_suspend(struct device *dev)
-{
- struct dw_mci *host = dev_get_drvdata(dev);
-
- return dw_mci_suspend(host);
-}
-
-static int dw_mci_exynos_resume(struct device *dev)
+#ifdef CONFIG_PM
+static int dw_mci_exynos_runtime_resume(struct device *dev)
{
struct dw_mci *host = dev_get_drvdata(dev);
dw_mci_exynos_config_smu(host);
- return dw_mci_resume(host);
+ return dw_mci_runtime_resume(dev);
}
/**
@@ -211,10 +205,8 @@ static int dw_mci_exynos_resume_noirq(struct device *dev)
return 0;
}
#else
-#define dw_mci_exynos_suspend NULL
-#define dw_mci_exynos_resume NULL
#define dw_mci_exynos_resume_noirq NULL
-#endif /* CONFIG_PM_SLEEP */
+#endif /* CONFIG_PM */
static void dw_mci_exynos_config_hs400(struct dw_mci *host, u32 timing)
{
@@ -531,7 +523,11 @@ static int dw_mci_exynos_probe(struct platform_device *pdev)
}
static const struct dev_pm_ops dw_mci_exynos_pmops = {
- SET_SYSTEM_SLEEP_PM_OPS(dw_mci_exynos_suspend, dw_mci_exynos_resume)
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
+ SET_RUNTIME_PM_OPS(dw_mci_runtime_suspend,
+ dw_mci_exynos_runtime_resume,
+ NULL)
.resume_noirq = dw_mci_exynos_resume_noirq,
.thaw_noirq = dw_mci_exynos_resume_noirq,
.restore_noirq = dw_mci_exynos_resume_noirq,
--
2.3.7
^ permalink raw reply related
* [PATCH v2 5/9] mmc: dw_mmc-k3: deploy runtime PM facilities
From: Shawn Lin @ 2016-10-12 2:54 UTC (permalink / raw)
To: Jaehoon Chung, Ulf Hansson
Cc: linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Shawn Lin,
linux-mmc-u79uwXL29TY76Z2rM5mHXA, Doug Anderson
In-Reply-To: <1476240643-5701-1-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Let's migrate it to use runtime PM and remove the system
PM callback from this driver. With this patch, it could
handle system PM properly and could also use runtime PM
if we enable it.
Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
Changes in v2:
- use dw_mci_runtime_* directly
drivers/mmc/host/dw_mmc-k3.c | 39 +++++++++------------------------------
1 file changed, 9 insertions(+), 30 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c
index 6247894..9821e6b 100644
--- a/drivers/mmc/host/dw_mmc-k3.c
+++ b/drivers/mmc/host/dw_mmc-k3.c
@@ -15,6 +15,7 @@
#include <linux/module.h>
#include <linux/of_address.h>
#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
@@ -162,35 +163,13 @@ static int dw_mci_k3_probe(struct platform_device *pdev)
return dw_mci_pltfm_register(pdev, drv_data);
}
-#ifdef CONFIG_PM_SLEEP
-static int dw_mci_k3_suspend(struct device *dev)
-{
- struct dw_mci *host = dev_get_drvdata(dev);
- int ret;
-
- ret = dw_mci_suspend(host);
- if (!ret)
- clk_disable_unprepare(host->ciu_clk);
-
- return ret;
-}
-
-static int dw_mci_k3_resume(struct device *dev)
-{
- struct dw_mci *host = dev_get_drvdata(dev);
- int ret;
-
- ret = clk_prepare_enable(host->ciu_clk);
- if (ret) {
- dev_err(host->dev, "failed to enable ciu clock\n");
- return ret;
- }
-
- return dw_mci_resume(host);
-}
-#endif /* CONFIG_PM_SLEEP */
-
-static SIMPLE_DEV_PM_OPS(dw_mci_k3_pmops, dw_mci_k3_suspend, dw_mci_k3_resume);
+static const struct dev_pm_ops dw_mci_k3_dev_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
+ SET_RUNTIME_PM_OPS(dw_mci_runtime_suspend,
+ dw_mci_runtime_resume,
+ NULL)
+};
static struct platform_driver dw_mci_k3_pltfm_driver = {
.probe = dw_mci_k3_probe,
@@ -198,7 +177,7 @@ static struct platform_driver dw_mci_k3_pltfm_driver = {
.driver = {
.name = "dwmmc_k3",
.of_match_table = dw_mci_k3_match,
- .pm = &dw_mci_k3_pmops,
+ .pm = &dw_mci_k3_dev_pm_ops,
},
};
--
2.3.7
^ permalink raw reply related
* [PATCH v2 9/9] mmc: dw_mmc: remove system PM callback
From: Shawn Lin @ 2016-10-12 2:56 UTC (permalink / raw)
To: Jaehoon Chung, Ulf Hansson
Cc: linux-mmc, Doug Anderson, linux-rockchip, Shawn Lin
In-Reply-To: <1476240643-5701-1-git-send-email-shawn.lin@rock-chips.com>
Now there are no variant drivers using dw_mci_suspend
and dw_mci_resume, so let's remove it.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
Changes in v2: None
drivers/mmc/host/dw_mmc.c | 81 ++++++++++++++++-------------------------------
drivers/mmc/host/dw_mmc.h | 2 --
2 files changed, 27 insertions(+), 56 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 297b6c2..1c9ee36 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -3267,27 +3267,41 @@ EXPORT_SYMBOL(dw_mci_remove);
#ifdef CONFIG_PM
-/*
- * TODO: we should probably disable the clock to the card in the suspend path.
- */
-int dw_mci_suspend(struct dw_mci *host)
+int dw_mci_runtime_suspend(struct device *dev)
{
+ struct dw_mci *host = dev_get_drvdata(dev);
+
if (host->use_dma && host->dma_ops->exit)
host->dma_ops->exit(host);
+ 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 0;
}
-EXPORT_SYMBOL(dw_mci_suspend);
+EXPORT_SYMBOL(dw_mci_runtime_suspend);
-int dw_mci_resume(struct dw_mci *host)
+int dw_mci_runtime_resume(struct device *dev)
{
- int i, ret;
+ int i, ret = 0;
+ struct dw_mci *host = dev_get_drvdata(dev);
- if (!dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS)) {
- ret = -ENODEV;
- return ret;
+ if (host->cur_slot &&
+ (mmc_can_gpio_cd(host->cur_slot->mmc) ||
+ !mmc_card_is_removable(host->cur_slot->mmc))) {
+ ret = clk_prepare_enable(host->biu_clk);
+ if (ret)
+ return ret;
}
+ ret = clk_prepare_enable(host->ciu_clk);
+ if (ret)
+ return ret;
+
if (host->use_dma && host->dma_ops->init)
host->dma_ops->init(host);
@@ -3295,8 +3309,8 @@ int dw_mci_resume(struct dw_mci *host)
* Restore the initial value at FIFOTH register
* And Invalidate the prev_blksz with zero
*/
- mci_writel(host, FIFOTH, host->fifoth_val);
- host->prev_blksz = 0;
+ mci_writel(host, FIFOTH, host->fifoth_val);
+ host->prev_blksz = 0;
/* Put in max timeout */
mci_writel(host, TMOUT, 0xFFFFFFFF);
@@ -3321,48 +3335,7 @@ int dw_mci_resume(struct dw_mci *host)
/* Now that slots are all setup, we can enable card detect */
dw_mci_enable_cd(host);
- return 0;
-}
-EXPORT_SYMBOL(dw_mci_resume);
-
-int dw_mci_runtime_suspend(struct device *dev)
-{
- int err = 0;
- struct dw_mci *host = dev_get_drvdata(dev);
-
- err = dw_mci_suspend(host);
- if (err)
- return err;
-
- 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 err;
-}
-EXPORT_SYMBOL(dw_mci_runtime_suspend);
-
-int dw_mci_runtime_resume(struct device *dev)
-{
- int ret = 0;
- struct dw_mci *host = dev_get_drvdata(dev);
-
- if (host->cur_slot &&
- (mmc_can_gpio_cd(host->cur_slot->mmc) ||
- !mmc_card_is_removable(host->cur_slot->mmc))) {
- ret = clk_prepare_enable(host->biu_clk);
- if (ret)
- return ret;
- }
-
- ret = clk_prepare_enable(host->ciu_clk);
- if (ret)
- return ret;
-
- return dw_mci_resume(host);
+ return ret;
}
EXPORT_SYMBOL(dw_mci_runtime_resume);
#endif /* CONFIG_PM */
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index d14a391..4a6ae750 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -235,8 +235,6 @@
extern int dw_mci_probe(struct dw_mci *host);
extern void dw_mci_remove(struct dw_mci *host);
#ifdef CONFIG_PM
-extern int dw_mci_suspend(struct dw_mci *host);
-extern int dw_mci_resume(struct dw_mci *host);
extern int dw_mci_runtime_suspend(struct device *device);
extern int dw_mci_runtime_resume(struct device *device);
#endif
--
2.3.7
^ permalink raw reply related
* [PATCH v2 8/9] mmc: dw_mmc-pltfm: deploy runtime PM facilities
From: Shawn Lin @ 2016-10-12 2:56 UTC (permalink / raw)
To: Jaehoon Chung, Ulf Hansson
Cc: linux-mmc, Doug Anderson, linux-rockchip, Shawn Lin
In-Reply-To: <1476240643-5701-1-git-send-email-shawn.lin@rock-chips.com>
Let's migrate it to use runtime PM and remove the system
PM callback from this driver. With this patch, it could
handle system PM properly and could also use runtime PM
if we enable it.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
Changes in v2:
- use dw_mci_runtime_* directly
drivers/mmc/host/dw_mmc-pltfm.c | 28 ++++++++--------------------
1 file changed, 8 insertions(+), 20 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c
index c0bb0c7..b486fef 100644
--- a/drivers/mmc/host/dw_mmc-pltfm.c
+++ b/drivers/mmc/host/dw_mmc-pltfm.c
@@ -16,6 +16,7 @@
#include <linux/io.h>
#include <linux/irq.h>
#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
#include <linux/slab.h>
#include <linux/mmc/host.h>
#include <linux/mmc/mmc.h>
@@ -57,26 +58,13 @@ int dw_mci_pltfm_register(struct platform_device *pdev,
}
EXPORT_SYMBOL_GPL(dw_mci_pltfm_register);
-#ifdef CONFIG_PM_SLEEP
-/*
- * TODO: we should probably disable the clock to the card in the suspend path.
- */
-static int dw_mci_pltfm_suspend(struct device *dev)
-{
- struct dw_mci *host = dev_get_drvdata(dev);
-
- return dw_mci_suspend(host);
-}
-
-static int dw_mci_pltfm_resume(struct device *dev)
-{
- struct dw_mci *host = dev_get_drvdata(dev);
-
- return dw_mci_resume(host);
-}
-#endif /* CONFIG_PM_SLEEP */
-
-SIMPLE_DEV_PM_OPS(dw_mci_pltfm_pmops, dw_mci_pltfm_suspend, dw_mci_pltfm_resume);
+const struct dev_pm_ops dw_mci_pltfm_pmops = {
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
+ SET_RUNTIME_PM_OPS(dw_mci_runtime_suspend,
+ dw_mci_runtime_resume,
+ NULL)
+};
EXPORT_SYMBOL_GPL(dw_mci_pltfm_pmops);
static const struct of_device_id dw_mci_pltfm_match[] = {
--
2.3.7
^ permalink raw reply related
* [PATCH v2 7/9] mmc: dw_mmc-pci: deploy runtime PM facilities
From: Shawn Lin @ 2016-10-12 2:56 UTC (permalink / raw)
To: Jaehoon Chung, Ulf Hansson
Cc: linux-mmc, Doug Anderson, linux-rockchip, Shawn Lin
In-Reply-To: <1476240643-5701-1-git-send-email-shawn.lin@rock-chips.com>
Let's migrate it to use runtime PM and remove the system
PM callback from this driver. With this patch, it could
handle system PM properly and could also use runtime PM
if we enable it.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
Changes in v2:
- use dw_mci_runtime_* directly
drivers/mmc/host/dw_mmc-pci.c | 29 +++++++++--------------------
1 file changed, 9 insertions(+), 20 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc-pci.c b/drivers/mmc/host/dw_mmc-pci.c
index 4c69fbd..ab82796 100644
--- a/drivers/mmc/host/dw_mmc-pci.c
+++ b/drivers/mmc/host/dw_mmc-pci.c
@@ -14,6 +14,7 @@
#include <linux/io.h>
#include <linux/irq.h>
#include <linux/pci.h>
+#include <linux/pm_runtime.h>
#include <linux/slab.h>
#include <linux/mmc/host.h>
#include <linux/mmc/mmc.h>
@@ -79,25 +80,13 @@ static void dw_mci_pci_remove(struct pci_dev *pdev)
dw_mci_remove(host);
}
-#ifdef CONFIG_PM_SLEEP
-static int dw_mci_pci_suspend(struct device *dev)
-{
- struct pci_dev *pdev = to_pci_dev(dev);
- struct dw_mci *host = pci_get_drvdata(pdev);
-
- return dw_mci_suspend(host);
-}
-
-static int dw_mci_pci_resume(struct device *dev)
-{
- struct pci_dev *pdev = to_pci_dev(dev);
- struct dw_mci *host = pci_get_drvdata(pdev);
-
- return dw_mci_resume(host);
-}
-#endif /* CONFIG_PM_SLEEP */
-
-static SIMPLE_DEV_PM_OPS(dw_mci_pci_pmops, dw_mci_pci_suspend, dw_mci_pci_resume);
+static const struct dev_pm_ops dw_mci_pci_dev_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
+ SET_RUNTIME_PM_OPS(dw_mci_runtime_suspend,
+ dw_mci_runtime_resume,
+ NULL)
+};
static const struct pci_device_id dw_mci_pci_id[] = {
{ PCI_DEVICE(SYNOPSYS_DW_MCI_VENDOR_ID, SYNOPSYS_DW_MCI_DEVICE_ID) },
@@ -111,7 +100,7 @@ static struct pci_driver dw_mci_pci_driver = {
.probe = dw_mci_pci_probe,
.remove = dw_mci_pci_remove,
.driver = {
- .pm = &dw_mci_pci_pmops
+ .pm = &dw_mci_pci_dev_pm_ops,
},
};
--
2.3.7
^ permalink raw reply related
* [PATCH v2 4/9] mmc: dw_mmc: disable biu clk if possible
From: Shawn Lin @ 2016-10-12 2:50 UTC (permalink / raw)
To: Jaehoon Chung, Ulf Hansson
Cc: linux-mmc, Doug Anderson, linux-rockchip, Shawn Lin
In-Reply-To: <1476240643-5701-1-git-send-email-shawn.lin@rock-chips.com>
We could disable biu clk if gpio card detect available,
or it is a non-removable device.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
Changes in v2: None
drivers/mmc/host/dw_mmc-rockchip.c | 1 +
drivers/mmc/host/dw_mmc.c | 13 +++++++++++++
2 files changed, 14 insertions(+)
diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
index 0a05ad3..9a46e46 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -13,6 +13,7 @@
#include <linux/mmc/host.h>
#include <linux/mmc/dw_mmc.h>
#include <linux/of_address.h>
+#include <linux/mmc/slot-gpio.h>
#include <linux/pm_runtime.h>
#include <linux/slab.h>
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 67fa088..297b6c2 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -3336,6 +3336,11 @@ int dw_mci_runtime_suspend(struct device *dev)
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 err;
}
EXPORT_SYMBOL(dw_mci_runtime_suspend);
@@ -3345,6 +3350,14 @@ int dw_mci_runtime_resume(struct device *dev)
int ret = 0;
struct dw_mci *host = dev_get_drvdata(dev);
+ if (host->cur_slot &&
+ (mmc_can_gpio_cd(host->cur_slot->mmc) ||
+ !mmc_card_is_removable(host->cur_slot->mmc))) {
+ ret = clk_prepare_enable(host->biu_clk);
+ if (ret)
+ return ret;
+ }
+
ret = clk_prepare_enable(host->ciu_clk);
if (ret)
return ret;
--
2.3.7
^ permalink raw reply related
* [PATCH v2 3/9] mmc: core: expose the capability of gpio card detect
From: Shawn Lin @ 2016-10-12 2:50 UTC (permalink / raw)
To: Jaehoon Chung, Ulf Hansson
Cc: linux-mmc, Doug Anderson, linux-rockchip, Shawn Lin
In-Reply-To: <1476240643-5701-1-git-send-email-shawn.lin@rock-chips.com>
Add new helper API mmc_can_gpio_cd for slot-gpio to make
host drivers know whether it supports gpio card detect.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
Changes in v2: None
drivers/mmc/core/slot-gpio.c | 8 ++++++++
include/linux/mmc/slot-gpio.h | 1 +
2 files changed, 9 insertions(+)
diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index 27117ba..babe591 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -258,6 +258,14 @@ int mmc_gpiod_request_cd(struct mmc_host *host, const char *con_id,
}
EXPORT_SYMBOL(mmc_gpiod_request_cd);
+bool mmc_can_gpio_cd(struct mmc_host *host)
+{
+ struct mmc_gpio *ctx = host->slot.handler_priv;
+
+ return ctx->cd_gpio ? true : false;
+}
+EXPORT_SYMBOL(mmc_can_gpio_cd);
+
/**
* mmc_gpiod_request_ro - request a gpio descriptor for write protection
* @host: mmc host
diff --git a/include/linux/mmc/slot-gpio.h b/include/linux/mmc/slot-gpio.h
index 3945a8c..a7972cd 100644
--- a/include/linux/mmc/slot-gpio.h
+++ b/include/linux/mmc/slot-gpio.h
@@ -29,5 +29,6 @@ int mmc_gpiod_request_ro(struct mmc_host *host, const char *con_id,
void mmc_gpio_set_cd_isr(struct mmc_host *host,
irqreturn_t (*isr)(int irq, void *dev_id));
void mmc_gpiod_request_cd_irq(struct mmc_host *host);
+bool mmc_can_gpio_cd(struct mmc_host *host);
#endif
--
2.3.7
^ permalink raw reply related
* [PATCH v2 2/9] mmc: dw_mmc-rockchip: add runtime PM support
From: Shawn Lin @ 2016-10-12 2:50 UTC (permalink / raw)
To: Jaehoon Chung, Ulf Hansson
Cc: linux-mmc, Doug Anderson, linux-rockchip, Shawn Lin
In-Reply-To: <1476240643-5701-1-git-send-email-shawn.lin@rock-chips.com>
This patch adds runtime PM support for dw_mmc-rockchip.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
Changes in v2:
- use dw_mci_runtime_* directly
drivers/mmc/host/dw_mmc-rockchip.c | 41 +++++++++++++++++++++++++++++++++++---
1 file changed, 38 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
index 25eae35..0a05ad3 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -13,6 +13,7 @@
#include <linux/mmc/host.h>
#include <linux/mmc/dw_mmc.h>
#include <linux/of_address.h>
+#include <linux/pm_runtime.h>
#include <linux/slab.h>
#include "dw_mmc.h"
@@ -325,6 +326,7 @@ static int dw_mci_rockchip_probe(struct platform_device *pdev)
{
const struct dw_mci_drv_data *drv_data;
const struct of_device_id *match;
+ int ret;
if (!pdev->dev.of_node)
return -ENODEV;
@@ -332,16 +334,49 @@ static int dw_mci_rockchip_probe(struct platform_device *pdev)
match = of_match_node(dw_mci_rockchip_match, pdev->dev.of_node);
drv_data = match->data;
- return dw_mci_pltfm_register(pdev, drv_data);
+ pm_runtime_get_noresume(&pdev->dev);
+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+ pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
+ pm_runtime_use_autosuspend(&pdev->dev);
+
+ ret = dw_mci_pltfm_register(pdev, drv_data);
+ if (ret) {
+ pm_runtime_disable(&pdev->dev);
+ pm_runtime_set_suspended(&pdev->dev);
+ pm_runtime_put_noidle(&pdev->dev);
+ return ret;
+ }
+
+ pm_runtime_put_autosuspend(&pdev->dev);
+
+ return 0;
}
+static int dw_mci_rockchip_remove(struct platform_device *pdev)
+{
+ pm_runtime_get_sync(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);
+ pm_runtime_put_noidle(&pdev->dev);
+
+ return dw_mci_pltfm_remove(pdev);
+}
+
+static const struct dev_pm_ops dw_mci_rockchip_dev_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
+ SET_RUNTIME_PM_OPS(dw_mci_runtime_suspend,
+ dw_mci_runtime_resume,
+ NULL)
+};
+
static struct platform_driver dw_mci_rockchip_pltfm_driver = {
.probe = dw_mci_rockchip_probe,
- .remove = dw_mci_pltfm_remove,
+ .remove = dw_mci_rockchip_remove,
.driver = {
.name = "dwmmc_rockchip",
.of_match_table = dw_mci_rockchip_match,
- .pm = &dw_mci_pltfm_pmops,
+ .pm = &dw_mci_rockchip_dev_pm_ops,
},
};
--
2.3.7
^ permalink raw reply related
* [PATCH v2 1/9] mmc: dw_mmc: add runtime PM callback
From: Shawn Lin @ 2016-10-12 2:50 UTC (permalink / raw)
To: Jaehoon Chung, Ulf Hansson
Cc: linux-mmc, Doug Anderson, linux-rockchip, Shawn Lin
In-Reply-To: <1476240643-5701-1-git-send-email-shawn.lin@rock-chips.com>
This patch add dw_mci_runtime_suspend/resume interfaces
and expose it to dw_mci variant driver to support runtime
PM.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
Changes in v2:
- use struct device as argument for runtime callback
drivers/mmc/host/dw_mmc.c | 32 ++++++++++++++++++++++++++++++--
drivers/mmc/host/dw_mmc.h | 4 +++-
2 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 4fcbc40..67fa088 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -3266,7 +3266,7 @@ EXPORT_SYMBOL(dw_mci_remove);
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM
/*
* TODO: we should probably disable the clock to the card in the suspend path.
*/
@@ -3324,7 +3324,35 @@ int dw_mci_resume(struct dw_mci *host)
return 0;
}
EXPORT_SYMBOL(dw_mci_resume);
-#endif /* CONFIG_PM_SLEEP */
+
+int dw_mci_runtime_suspend(struct device *dev)
+{
+ int err = 0;
+ struct dw_mci *host = dev_get_drvdata(dev);
+
+ err = dw_mci_suspend(host);
+ if (err)
+ return err;
+
+ clk_disable_unprepare(host->ciu_clk);
+
+ return err;
+}
+EXPORT_SYMBOL(dw_mci_runtime_suspend);
+
+int dw_mci_runtime_resume(struct device *dev)
+{
+ int ret = 0;
+ struct dw_mci *host = dev_get_drvdata(dev);
+
+ ret = clk_prepare_enable(host->ciu_clk);
+ if (ret)
+ return ret;
+
+ return dw_mci_resume(host);
+}
+EXPORT_SYMBOL(dw_mci_runtime_resume);
+#endif /* CONFIG_PM */
static int __init dw_mci_init(void)
{
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index e8cd2de..d14a391 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -234,9 +234,11 @@
extern int dw_mci_probe(struct dw_mci *host);
extern void dw_mci_remove(struct dw_mci *host);
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM
extern int dw_mci_suspend(struct dw_mci *host);
extern int dw_mci_resume(struct dw_mci *host);
+extern int dw_mci_runtime_suspend(struct device *device);
+extern int dw_mci_runtime_resume(struct device *device);
#endif
/**
--
2.3.7
^ permalink raw reply related
* [PATCH v2 0/9] Init runtime PM support for dw_mmc
From: Shawn Lin @ 2016-10-12 2:50 UTC (permalink / raw)
To: Jaehoon Chung, Ulf Hansson
Cc: linux-mmc, Doug Anderson, linux-rockchip, Shawn Lin
Hi Jaehoon and Ulf,
This patch is gonna support runtime PM for dw_mmc.
It could support to disable ciu_clk by default and disable
biu_clk if the devices are non-removeable, or removeable
with gpio-base card detect.
Then I remove the system PM since the runtime PM actually
does the same thing as it. So I help migrate the dw_mmc variant
drivers to use runtime PM pairs and pm_runtime_force_*. Note
that I only enable runtime PM for dw_mmc-rockchip as I will
leave the decision to the owners of the corresponding drivers.
I just tested it on my RK3288 platform with linux-next to make
the runtime PM and system PM work fine for my emmc, sd card and
sdio. But I don't have hardware to help test other variant drivers.
But in theory it should work fine as I mentioned that the runtime
PM does the same thing as system PM except for disabling ciu_clk
aggressively which should not be related to the variant hosts.
As you could see that I just extend the slot-gpio a bit, so the
ideal way is Ulf could pick them up with Jaehoon's ack. :)
Changes in v2:
- use struct device as argument for runtime callback
- use dw_mci_runtime_* directly
- use dw_mci_runtime_* directly
- minor fix since I change the argument for dw_mci_runtime_*
- use dw_mci_runtime_* directly
- use dw_mci_runtime_* directly
Shawn Lin (9):
mmc: dw_mmc: add runtime PM callback
mmc: dw_mmc-rockchip: add runtime PM support
mmc: core: expose the capability of gpio card detect
mmc: dw_mmc: disable biu clk if possible
mmc: dw_mmc-k3: deploy runtime PM facilities
mmc: dw_mmc-exynos: deploy runtime PM facilities
mmc: dw_mmc-pci: deploy runtime PM facilities
mmc: dw_mmc-pltfm: deploy runtime PM facilities
mmc: dw_mmc: remove system PM callback
drivers/mmc/core/slot-gpio.c | 8 +++++++
drivers/mmc/host/dw_mmc-exynos.c | 24 +++++++++-----------
drivers/mmc/host/dw_mmc-k3.c | 39 ++++++++------------------------
drivers/mmc/host/dw_mmc-pci.c | 29 ++++++++----------------
drivers/mmc/host/dw_mmc-pltfm.c | 28 +++++++----------------
drivers/mmc/host/dw_mmc-rockchip.c | 42 +++++++++++++++++++++++++++++++---
drivers/mmc/host/dw_mmc.c | 46 +++++++++++++++++++++++++-------------
drivers/mmc/host/dw_mmc.h | 6 ++---
include/linux/mmc/slot-gpio.h | 1 +
9 files changed, 117 insertions(+), 106 deletions(-)
--
2.3.7
^ permalink raw reply
* Re: [PATCH v3] mmc: sunxi: Handle the 'New Timing' mode
From: Maxime Ripard @ 2016-10-11 14:55 UTC (permalink / raw)
To: Jean-Francois Moine
Cc: Ulf Hansson, Chen-Yu Tsai, Michael Turquette,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-mmc-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20160830193202.4baaefd508a5d20d48b2cbed-GANU6spQydw@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2252 bytes --]
On Tue, Aug 30, 2016 at 07:32:02PM +0200, Jean-Francois Moine wrote:
> On Tue, 30 Aug 2016 18:26:13 +0200
> Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>
> > > There are 2 flags saying that the new timing is used:
> > > - the bit 'mode select' in the clock register, and
> > > - the bit 'new timing' in the MMC register.
> > > Both bits must be set/reset at the same time, otherwise the device
> > > does not work (tested with wifi and eMMC in H3 and A83T boards).
> > > So, some synchronization must exist.
> > >
> > > The previous versions was using a DT property for the MMC and a flag
> > > in the clock driver. This did work with a correct configuration
> > > on both sides, but experiment showed that it was easy to do an error.
> >
> > I still believe that we will need a property, at least to identify on
> > which we can try the new mode, and on which clocks it's irrelevant (at
> > least for the A33 and A83T).
>
> As told above, setting the new mode on side (clock or MMC) and not on
> the other one prevents the devices to work. Then, it is safer to have
> the new mode capability flag only once for both sides.
>
> Now, as the clocks are defined by memory tables and not by the DT, it
> seems natural to have the flag on the clock side.
You can look at it from two sides.
Either you try to switch to the new mode all the time, or you try to
do it only for MMC controllers (and their associated clocks) that
support it.
In the former case, you'll need to ignore a failure in the switch
(mostly if the clock doesn't support it) only for the MMC controllers
that do not have that mode. For the controllers that support it, you
cannot ignore that error anymore. So you have to have some way to
identify in which case you are.
And you need to have that in the former case too, so there's really no
way you can go without such a flag.
> > However, I also believe we should make that mode switching explicit
> > through a function call, instead of relying on some side effect (of
> > some non-upstream code).
>
> Do you mean a direct call from the MMC driver to the clock driver?
Yes.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ 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