* Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality
From: Adrian Hunter @ 2016-10-17 8:14 UTC (permalink / raw)
To: Ziji Hu, 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, Liuliu Zhao, Peng Zhu, Yu Cao
In-Reply-To: <6bed26dc-f4c1-7f0d-36b6-d7d5fcbf0da9@marvell.com>
On 13/10/16 08:38, Ziji Hu wrote:
> 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.
Ok, so maybe just add some comments and more explanation of how it works.
>
>>>
>>>>> + /* 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.
OK, so SDHCI should also not allow auto-cmd12 if there is no stop command
i.e. data->stop is null.
>
>>>
>>> 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: [PATCH v2] mmc: sdhci: cast unsigned int to unsigned long long to avoid unexpeted error
From: Adrian Hunter @ 2016-10-17 8:13 UTC (permalink / raw)
To: Haibo Chen, ulf.hansson, rmk+kernel; +Cc: stable, linux-mmc
In-Reply-To: <1476692317-26845-1-git-send-email-haibo.chen@nxp.com>
On 17/10/16 11:18, Haibo Chen wrote:
> Potentially overflowing expression 1000000 * data->timeout_clks with
> type unsigned int is evaluated using 32-bit arithmetic, and then used
> in a context that expects an expression of type unsigned long long.
>
> To avoid overflow, cast 1000000U to type unsigned long long.
> Special thanks to Coverity.
>
> Fixes: 7f05538af71c ("mmc: sdhci: fix data timeout (part 2)")
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> Cc: stable@vger.kernel.org # v3.15+
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
> drivers/mmc/host/sdhci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 223a91e..71654b9 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -687,7 +687,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
> * host->clock is in Hz. target_timeout is in us.
> * Hence, us = 1000000 * cycles / Hz. Round up.
> */
> - val = 1000000 * data->timeout_clks;
> + val = 1000000ULL * data->timeout_clks;
> if (do_div(val, host->clock))
> target_timeout++;
> target_timeout += val;
>
^ permalink raw reply
* Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC
From: Adrian Hunter @ 2016-10-17 7:55 UTC (permalink / raw)
To: Ziji Hu, 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, Liuliu Zhao, Peng Zhu, Yu Cao
In-Reply-To: <bcf4abca-9f2e-6d1b-6690-d672766e18cb@marvell.com>
On 12/10/16 15:17, Ziji Hu wrote:
> 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.
Currently, re-tuning is automatically enabled whenever tuning is executed,
and then re-tuning will be done periodically or after CRC errors. The
function to disable re-tuning mmc_retune_disable() is not exported, however
if you have you are determining the sampling point your own way, you could
simply not implement ->execute_tuning() and then there would be no tuning
and no re-tuning.
>
> 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: [RFC v2 2/2] sdhci: Prevent SD from doing high-speed timing when broken-highspeed property is set
From: Adrian Hunter @ 2016-10-17 6:56 UTC (permalink / raw)
To: Zach Brown, ulf.hansson
Cc: robh+dt, mark.rutland, linux-mmc, devicetree, linux-kernel
In-Reply-To: <1476297656-7019-3-git-send-email-zach.brown@ni.com>
On 12/10/16 21:40, Zach Brown wrote:
> 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>
Assuming the DT property is accepted:
Acked-by: Adrian Hunter <adrian.hunter@intel.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) &&
>
^ permalink raw reply
* Re: [PATCH 2/2] mmc: sdhci-iproc: support standard byte register accesses
From: Adrian Hunter @ 2016-10-17 6:36 UTC (permalink / raw)
To: Scott Branden, Ulf Hansson, Rob Herring, Mark Rutland, Ray Jui,
Scott Branden
Cc: BCM Kernel Feedback, linux-mmc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Srinath Mannam
In-Reply-To: <1476297352-7812-3-git-send-email-scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
On 12/10/16 21:35, Scott Branden wrote:
> 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-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Scott Branden <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Acked-by: Adrian Hunter <adrian.hunter-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
--
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] mmc: sdhci: cast unsigned int to unsigned long long to avoid unexpeted error
From: Bough Chen @ 2016-10-14 9:53 UTC (permalink / raw)
To: Adrian Hunter, ulf.hansson@linaro.org; +Cc: linux-mmc@vger.kernel.org
In-Reply-To: <cd546cd4-e64f-1b86-1cae-08b89dabc410@intel.com>
> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> owner@vger.kernel.org] On Behalf Of Adrian Hunter
> Sent: Friday, October 14, 2016 5:12 PM
> To: Bough Chen <haibo.chen@nxp.com>; ulf.hansson@linaro.org
> Cc: linux-mmc@vger.kernel.org
> Subject: Re: [PATCH] mmc: sdhci: cast unsigned int to unsigned long long to
> avoid unexpeted error
>
> On 13/09/16 11:36, Adrian Hunter wrote:
> > On 13/09/16 10:06, Haibo Chen wrote:
> >> Potentially overflowing expression 1000000 * data->timeout_clks with
> >> type unsigned int is evaluated using 32-bit arithmetic, and then used
> >> in a context that expects an expression of type unsigned long long.
> >>
> >> To avoid overflow, cast 1000000U to type unsigned long long.
> >> Special thanks to coverity <http://www.coverity.com>
> >
> > Let's leave out the web link - it is enough to say it was found with Coverity.
> >
> >>
> >> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
>
> Do you plan to re-submit this?
>
Yes, I will re-submit this soon.
> >
> > Needs a fixes tag and cc stable.
> >
> >> ---
> >> drivers/mmc/host/sdhci.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >> index 4805566..0e9f495 100644
> >> --- a/drivers/mmc/host/sdhci.c
> >> +++ b/drivers/mmc/host/sdhci.c
> >> @@ -687,7 +687,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host,
> struct mmc_command *cmd)
> >> * host->clock is in Hz. target_timeout is in us.
> >> * Hence, us = 1000000 * cycles / Hz. Round up.
> >> */
> >> - val = 1000000 * data->timeout_clks;
> >> + val = (unsigned long long)1000000 * data->timeout_clks;
> >
> > Please use 1000000ULL instead of a cast.
> >
> >> if (do_div(val, host->clock))
> >> target_timeout++;
> >> target_timeout += val;
> >>
> >
> >
>
> --
> 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: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
From: Chris Brandt @ 2016-10-14 13:18 UTC (permalink / raw)
To: Ulf Hansson, Lee Jones
Cc: Wolfram Sang, Sergei Shtylyov, Geert Uytterhoeven, Simon Horman,
linux-mmc, Linux-Renesas
In-Reply-To: <CAPDyKFoeRggX-YMy2L+o6CBtwa46rT2YyJiXdPK_c3a1R1Nvgg@mail.gmail.com>
On 9/22/2016, Ulf Hansson wrote:
> To: Chris Brandt <Chris.Brandt@renesas.com>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>; Sergei Shtylyov
> <sergei.shtylyov@cogentembedded.com>; Geert Uytterhoeven <geert@linux-
> m68k.org>; Simon Horman <horms+renesas@verge.net.au>; linux-mmc <linux-
> mmc@vger.kernel.org>; Linux-Renesas <linux-renesas-soc@vger.kernel.org>;
> Lee Jones <lee@kernel.org>
> Subject: Re: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
>
> + Lee
>
> On 12 September 2016 at 16:15, Chris Brandt <chris.brandt@renesas.com>
> wrote:
> > For the r7s72100 SOC, the DATA_PORT register was changed to 32-bits wide.
> > Therefore a new flag has been created that will allow 32-bit
> > reads/writes to the DATA_PORT register instead of 16-bit (because
> > 16-bits accesses are not supported).
> >
> > Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> > ---
> > v3:
> > * changed loops to memcpy
> > v2:
> > * changed 'data * 0xFF' to 'data & 0xFF'
> > * added 'const' for sd_ctrl_write32_rep
> > ---
> > drivers/mmc/host/tmio_mmc.h | 12 ++++++++++++
> > drivers/mmc/host/tmio_mmc_pio.c | 30 ++++++++++++++++++++++++++++++
> > include/linux/mfd/tmio.h | 5 +++++
>
> This header file needs to be split up. The mmc specific bits, should be
> moved to a local header file under driver/mmc/host/*.
>
> Sure, we don't need to do that as part of $subject patch, but then I need
> an ack from Lee Jones, the mfd maintainer. I have added him on cc.
Is this still waiting on an ACK from Lee Jones because it touches the tmio.h file under include/linux/mfd/ ?
> > 3 files changed, 47 insertions(+)
> >
> > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> > index ecb99fc..a99e634 100644
> > --- a/drivers/mmc/host/tmio_mmc.h
> > +++ b/drivers/mmc/host/tmio_mmc.h
> > @@ -237,6 +237,12 @@ static inline u32
> sd_ctrl_read16_and_16_as_32(struct tmio_mmc_host *host, int ad
> > readw(host->ctl + ((addr + 2) << host->bus_shift)) <<
> > 16; }
> >
> > +static inline void sd_ctrl_read32_rep(struct tmio_mmc_host *host, int
> addr,
> > + u32 *buf, int count)
> > +{
> > + readsl(host->ctl + (addr << host->bus_shift), buf, count); }
> > +
> > static inline void sd_ctrl_write16(struct tmio_mmc_host *host, int
> > addr, u16 val) {
> > /* If there is a hook and it returns non-zero then there @@
> > -259,4 +265,10 @@ static inline void sd_ctrl_write32_as_16_and_16(struct
> tmio_mmc_host *host, int
> > writew(val >> 16, host->ctl + ((addr + 2) <<
> > host->bus_shift)); }
> >
> > +static inline void sd_ctrl_write32_rep(struct tmio_mmc_host *host, int
> addr,
> > + const u32 *buf, int count) {
> > + writesl(host->ctl + (addr << host->bus_shift), buf, count); }
> > +
> > #endif
> > diff --git a/drivers/mmc/host/tmio_mmc_pio.c
> > b/drivers/mmc/host/tmio_mmc_pio.c index 017a4dc..59fac7d 100644
> > --- a/drivers/mmc/host/tmio_mmc_pio.c
> > +++ b/drivers/mmc/host/tmio_mmc_pio.c
> > @@ -443,6 +443,36 @@ static void tmio_mmc_transfer_data(struct
> tmio_mmc_host *host,
> > /*
> > * Transfer the data
> > */
> > + if (host->pdata->flags & TMIO_MMC_32BIT_DATA_PORT) {
> > + u8 data[4] = { };
> > +
> > + if (is_read)
> > + sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, (u32
> *)buf,
> > + count >> 2);
> > + else
> > + sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, (u32
> *)buf,
> > + count >> 2);
> > +
> > + /* if count was multiple of 4 */
> > + if (!(count & 0x3))
> > + return;
> > +
> > + buf8 = (u8 *)(buf + (count >> 2));
> > + count %= 4;
> > +
> > + if (is_read) {
> > + sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT,
> > + (u32 *)data, 1);
> > + memcpy(buf8, data, count);
> > + } else {
> > + memcpy(data, buf8, count);
> > + sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT,
> > + (u32 *)data, 1);
> > + }
> > +
> > + return;
> > + }
> > +
> > if (is_read)
> > sd_ctrl_read16_rep(host, CTL_SD_DATA_PORT, buf, count >>
> 1);
> > else
> > diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h index
> > 3b95dc7..0dbcb7e 100644
> > --- a/include/linux/mfd/tmio.h
> > +++ b/include/linux/mfd/tmio.h
> > @@ -100,6 +100,11 @@
> > #define TMIO_MMC_SDIO_STATUS_QUIRK (1 << 8)
> >
> > /*
> > + * Some controllers have a 32-bit wide data port register */
> > +#define TMIO_MMC_32BIT_DATA_PORT (1 << 9)
> > +
> > +/*
> > * Some controllers allows to set SDx actual clock
> > */
> > #define TMIO_MMC_CLK_ACTUAL (1 << 10)
> > --
> > 2.9.2
> >
> >
>
> Kind regards
> Uffe
^ permalink raw reply
* RE: [PATCH v3 0/3] mmc: sh_mobile_sdhi: Add r7s72100 support
From: Chris Brandt @ 2016-10-14 13:14 UTC (permalink / raw)
To: Ulf Hansson, Wolfram Sang, Sergei Shtylyov, Geert Uytterhoeven
Cc: Simon Horman, linux-mmc@vger.kernel.org,
linux-renesas-soc@vger.kernel.org
In-Reply-To: <20160912141507.6837-1-chris.brandt@renesas.com>
Hello.
What ever happened to this series of patches?
They didn't seem too outrageous, but they never got applied.
Thank you.
Chris
On 9/12/2016, Chris Brandt wrote:
> To: Ulf Hansson <ulf.hansson@linaro.org>; Wolfram Sang <wsa+renesas@sang-
> engineering.com>; Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>;
> Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Simon Horman <horms+renesas@verge.net.au>; linux-mmc@vger.kernel.org;
> linux-renesas-soc@vger.kernel.org; Chris Brandt <Chris.Brandt@renesas.com>
> Subject: [PATCH v3 0/3] mmc: sh_mobile_sdhi: Add r7s72100 support
>
> For the most part, the SDHI controller in the RZ/A1 (r7s72100) is the same
> as other Renesas SoC...except for the fact that the data port register was
> widened to 32-bits and the 16-bit accesses in the current tmio driver
> aren't going to cut it.
>
> Also, the tmio driver allows you to pass in what voltages you support via
> the ocr_mask. For the RZ/A1, it's just simple 3.3v.
> So, I added the ability to pass that when using DT.
>
> Chris Brandt (3):
> mmc: sh_mobile_sdhi: add ocr_mask option
> mmc: tmio-mmc: add support for 32bit data port
> mmc: sh_mobile_sdhi: Add r7s72100 support
>
> Documentation/devicetree/bindings/mmc/tmio_mmc.txt | 1 +
> drivers/mmc/host/sh_mobile_sdhi.c | 9 +++++++
> drivers/mmc/host/tmio_mmc.h | 12 +++++++++
> drivers/mmc/host/tmio_mmc_pio.c | 30
> ++++++++++++++++++++++
> include/linux/mfd/tmio.h | 5 ++++
> 5 files changed, 57 insertions(+)
>
> --
> 2.9.2
>
^ permalink raw reply
* Re: Detected aborted journal error in Kernel 3.12
From: Tony Lindgren @ 2016-10-14 11:12 UTC (permalink / raw)
To: Matthijs van Duin; +Cc: Ankur Tank, linux-omap, linux-mmc
In-Reply-To: <CAALWOA_RHBW8gYq=HncNgw8k2yKipieRgmCaNyz=kvLF4RSAwg@mail.gmail.com>
* Matthijs van Duin <matthijsvanduin@gmail.com> [161013 13:25]:
> It would indeed be very useful to have a reproducible case on an
> uptodate kernel, but it is definitely not specific to his hardware.
> Someone on the beagleboard forum also hit this issue, but sporadically:
>
> https://groups.google.com/d/msg/beagleboard/5ZwTUQwGSx4/r15mENdfAwAJ
>
> I'll quote my reply to it:
>
> On 4 October 2016 at 21:00, Matthijs van Duin <matthijsvanduin@gmail.com> wrote:
> > I've also run into this once during an apt-get upgrade, leaving the system
> > in a pretty hosed state. I managed to recover it but it required a lot of
> > effort. Although evidently rare, this is clearly a very serious issue.
> >
> > The direct cause is edma_prep_slave_sg() failing to allocate memory for the
> > struct edma_desc. I don't know whether the kernel is genuinely out of memory
> > or if it simply cannot free it up immediately (the allocation is done with
> > GPF_ATOMIC), and whether this is because it is filled with a backlog of
> > writes or whether a leak of some sort is going on.
> >
> > Instead of deferring until memory is available, or even just proceeding
> > without DMA, the omap_hsmmc driver immediately fails the request with an
> > error, thus pretty much guaranteeing loss of data or even filesystem
> > corruption. I personally think this is completely unacceptable behaviour of
> > a block driver.
Yes I can see that being a problem. The fact that omap_hsmmc does
not fail back to PIO mode is just strange. We've had the omap1
drivers/mmc/host/omap.c work with and without DMA no problem,
no idea why this would not be required also for omap_hsmmc too.
> > I've been meaning to persue this matter on the linux-mmc and/or linux-omap
> > lists, but since it was a single isolated incident and I have lots of other
> > stuff to do I haven't been able to find the time and motivation yet.
>
> If Ankur has a reproducible case then I think it's time to get
> motivated. Ankur, can you please try 4.7 or 4.8 ?
Yes please. And also let's set up a new bug in bugzilla.kernel.org
for this.
Regards,
Tony
^ permalink raw reply
* Re: [PATCH] mmc: sdhci: cast unsigned int to unsigned long long to avoid unexpeted error
From: Adrian Hunter @ 2016-10-14 9:12 UTC (permalink / raw)
To: Haibo Chen, ulf.hansson; +Cc: linux-mmc
In-Reply-To: <3603c63f-f007-12cc-677b-f39924628335@intel.com>
On 13/09/16 11:36, Adrian Hunter wrote:
> On 13/09/16 10:06, Haibo Chen wrote:
>> Potentially overflowing expression 1000000 * data->timeout_clks with
>> type unsigned int is evaluated using 32-bit arithmetic, and then used
>> in a context that expects an expression of type unsigned long long.
>>
>> To avoid overflow, cast 1000000U to type unsigned long long.
>> Special thanks to coverity <http://www.coverity.com>
>
> Let's leave out the web link - it is enough to say it was found with Coverity.
>
>>
>> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
Do you plan to re-submit this?
>
> Needs a fixes tag and cc stable.
>
>> ---
>> drivers/mmc/host/sdhci.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 4805566..0e9f495 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -687,7 +687,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
>> * host->clock is in Hz. target_timeout is in us.
>> * Hence, us = 1000000 * cycles / Hz. Round up.
>> */
>> - val = 1000000 * data->timeout_clks;
>> + val = (unsigned long long)1000000 * data->timeout_clks;
>
> Please use 1000000ULL instead of a cast.
>
>> if (do_div(val, host->clock))
>> target_timeout++;
>> target_timeout += val;
>>
>
>
^ permalink raw reply
* Re: Detected aborted journal error in Kernel 3.12
From: Matthijs van Duin @ 2016-10-13 20:24 UTC (permalink / raw)
To: Tony Lindgren, Ankur Tank; +Cc: linux-omap, linux-mmc
In-Reply-To: <20160919181541.45ahl6ktojawwtui@atomide.com>
On 19 September 2016 at 20:15, Tony Lindgren <tony@atomide.com> wrote:
> Well in that case you should be able to test it easily with v4.7
> too :) If you have it reproducable with a beaglebone, please provide
> some script to reproduce. Otherwise chances are this is something
> specific to your hardware and we have no way of helping you out.
It would indeed be very useful to have a reproducible case on an
uptodate kernel, but it is definitely not specific to his hardware.
Someone on the beagleboard forum also hit this issue, but sporadically:
https://groups.google.com/d/msg/beagleboard/5ZwTUQwGSx4/r15mENdfAwAJ
I'll quote my reply to it:
On 4 October 2016 at 21:00, Matthijs van Duin <matthijsvanduin@gmail.com> wrote:
> I've also run into this once during an apt-get upgrade, leaving the system
> in a pretty hosed state. I managed to recover it but it required a lot of
> effort. Although evidently rare, this is clearly a very serious issue.
>
> The direct cause is edma_prep_slave_sg() failing to allocate memory for the
> struct edma_desc. I don't know whether the kernel is genuinely out of memory
> or if it simply cannot free it up immediately (the allocation is done with
> GPF_ATOMIC), and whether this is because it is filled with a backlog of
> writes or whether a leak of some sort is going on.
>
> Instead of deferring until memory is available, or even just proceeding
> without DMA, the omap_hsmmc driver immediately fails the request with an
> error, thus pretty much guaranteeing loss of data or even filesystem
> corruption. I personally think this is completely unacceptable behaviour of
> a block driver.
>
> I've been meaning to persue this matter on the linux-mmc and/or linux-omap
> lists, but since it was a single isolated incident and I have lots of other
> stuff to do I haven't been able to find the time and motivation yet.
If Ankur has a reproducible case then I think it's time to get
motivated. Ankur, can you please try 4.7 or 4.8 ?
For those who might be interested, I've included an abbreviated kernel
log from my crash below, containing the first allocation failure and
some fragments of all later failures (since it was mostly more of the
same) until the journal was aborted.
[1388370.068396] mmcqd/1: page allocation failure: order:4, mode:0x204020
[1388370.068423] CPU: 0 PID: 67 Comm: mmcqd/1 Not tainted 4.2.0-bone2-dd1 #3
[1388370.068430] Hardware name: Generic AM33XX (Flattened Device Tree)
[1388370.068483] [<c0012e91>] (unwind_backtrace) from [<c00110d1>] (show_stack+0x11/0x14)
[1388370.068507] [<c00110d1>] (show_stack) from [<c00bd2c9>] (warn_alloc_failed+0x99/0xcc)
[1388370.068524] [<c00bd2c9>] (warn_alloc_failed) from [<c00bf3cf>] (__alloc_pages_nodemask+0x4db/0x5f4)
[1388370.068544] [<c00bf3cf>] (__alloc_pages_nodemask) from [<c00e6b75>] (cache_alloc_refill+0x1f5/0x3c4)
[1388370.068558] [<c00e6b75>] (cache_alloc_refill) from [<c00e6ec5>] (__kmalloc+0xb5/0xcc)
[1388370.068575] [<c00e6ec5>] (__kmalloc) from [<c0224083>] (edma_prep_slave_sg+0x77/0x224)
[1388370.068599] [<c0224083>] (edma_prep_slave_sg) from [<c0296097>] (omap_hsmmc_request+0x30f/0x3c8)
[1388370.068629] [<c0296097>] (omap_hsmmc_request) from [<c02833df>] (mmc_start_request+0xcb/0x1b0)
[1388370.068642] [<c02833df>] (mmc_start_request) from [<c0283a37>] (mmc_start_req+0x1e3/0x2ac)
[1388370.068654] [<c0283a37>] (mmc_start_req) from [<c028f435>] (mmc_blk_issue_rw_rq+0x8d/0x77c)
[1388370.068667] [<c028f435>] (mmc_blk_issue_rw_rq) from [<c028fc4f>] (mmc_blk_issue_rq+0x12b/0x34c)
[1388370.068678] [<c028fc4f>] (mmc_blk_issue_rq) from [<c029041f>] (mmc_queue_thread+0x6b/0x104)
[1388370.068701] [<c029041f>] (mmc_queue_thread) from [<c003d74b>] (kthread+0x9b/0xb0)
[1388370.068717] [<c003d74b>] (kthread) from [<c000e601>] (ret_from_fork+0x11/0x30)
[1388370.068723] Mem-Info:
[1388370.068742] active_anon:16913 inactive_anon:3153 isolated_anon:0
active_file:57786 inactive_file:27763 isolated_file:0
unevictable:0 dirty:186 writeback:5367 unstable:0
slab_reclaimable:16912 slab_unreclaimable:1471
mapped:16290 shmem:3216 pagetables:378 bounce:0
free:2918 free_pcp:93 free_cma:0
[1388370.068781] Normal free:11672kB min:2860kB low:3572kB high:4288kB active_anon:67652kB inactive_anon:12612kB active_file:231144kB inactive_file:111052kB unevictable:0kB isolated(anon):0kB isolated(file):0kB present:524288kB managed:512036kB mlocked:0kB dirty:744kB writeback:21468kB mapped:65160kB shmem:12864kB slab_reclaimable:67648kB slab_unreclaimable:5884kB kernel_stack:704kB pagetables:1512kB unstable:0kB bounce:0kB free_pcp:372kB local_pcp:372kB free_cma:0kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no
[1388370.068790] lowmem_reserve[]: 0 0
[1388370.068800] Normal: 1546*4kB (UE) 488*8kB (UE) 95*16kB (UE) 2*32kB (U) 0*64kB 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB 0*8192kB = 11672kB
[1388370.068835] 88765 total pagecache pages
[1388370.068847] 0 pages in swap cache
[1388370.068854] Swap cache stats: add 0, delete 0, find 0/0
[1388370.068858] Free swap = 0kB
[1388370.068862] Total swap = 0kB
[1388370.068867] 131072 pages RAM
[1388370.068872] 0 pages HighMem/MovableOnly
[1388370.068877] 4294964215 pages reserved
[1388370.068882] 6144 pages cma reserved
[1388370.068899] edma-dma-engine edma-dma-engine.0: edma_prep_slave_sg: Failed to allocate a descriptor
[1388370.078545] omap_hsmmc 481d8000.mmc: prep_slave_sg() failed
[1388370.084583] omap_hsmmc 481d8000.mmc: MMC start dma failure
[1388370.100106] mmcblk0: unknown error -1 sending read/write command, card status 0x900
[1388370.108621] blk_update_request: I/O error, dev mmcblk0, sector 5965856
[1388370.116079] blk_update_request: I/O error, dev mmcblk0, sector 5965864
[1388370.123424] blk_update_request: I/O error, dev mmcblk0, sector 5965872
[1388370.130744] blk_update_request: I/O error, dev mmcblk0, sector 5965880
[1388370.138080] blk_update_request: I/O error, dev mmcblk0, sector 5965888
[1388370.145453] blk_update_request: I/O error, dev mmcblk0, sector 5965896
[1388370.152789] blk_update_request: I/O error, dev mmcblk0, sector 5965904
[1388370.160148] blk_update_request: I/O error, dev mmcblk0, sector 5965912
[1388370.167483] blk_update_request: I/O error, dev mmcblk0, sector 5965920
[1388370.174811] blk_update_request: I/O error, dev mmcblk0, sector 5965928
[1388370.250530] EXT4-fs warning (device mmcblk0p1): ext4_end_bio:332: I/O error -5 writing to inode 52 (offset 20480 size 8368128 starting block 745988)
[1388370.250544] Buffer I/O error on device mmcblk0p1, logical block 745476
[1388370.257602] Buffer I/O error on device mmcblk0p1, logical block 745477
[1388370.264680] Buffer I/O error on device mmcblk0p1, logical block 745478
[1388370.271721] Buffer I/O error on device mmcblk0p1, logical block 745479
[1388370.278748] Buffer I/O error on device mmcblk0p1, logical block 745480
[1388370.285786] Buffer I/O error on device mmcblk0p1, logical block 745481
[1388370.292841] Buffer I/O error on device mmcblk0p1, logical block 745482
[1388370.299868] Buffer I/O error on device mmcblk0p1, logical block 745483
[1388370.306906] Buffer I/O error on device mmcblk0p1, logical block 745484
[1388370.313983] Buffer I/O error on device mmcblk0p1, logical block 745485
[1388370.385585] EXT4-fs warning (device mmcblk0p1): ext4_end_bio:332: I/O error -5 writing to inode 52 (offset 20480 size 8368128 starting block 746244)
[1388370.442558] EXT4-fs warning (device mmcblk0p1): ext4_end_bio:332: I/O error -5 writing to inode 52 (offset 20480 size 8368128 starting block 746500)
[1388370.492046] EXT4-fs warning (device mmcblk0p1): ext4_end_bio:332: I/O error -5 writing to inode 52 (offset 20480 size 8368128 starting block 746756)
[1388370.533845] EXT4-fs warning (device mmcblk0p1): ext4_end_bio:332: I/O error -5 writing to inode 52 (offset 20480 size 8368128 starting block 747012)
[1388370.568064] EXT4-fs warning (device mmcblk0p1): ext4_end_bio:332: I/O error -5 writing to inode 52 (offset 20480 size 8368128 starting block 747268)
[1388370.594972] EXT4-fs warning (device mmcblk0p1): ext4_end_bio:332: I/O error -5 writing to inode 52 (offset 20480 size 8368128 starting block 747524)
[1388370.613990] EXT4-fs warning (device mmcblk0p1): ext4_end_bio:332: I/O error -5 writing to inode 52 (offset 20480 size 8368128 starting block 747776)
[1388370.625719] EXT4-fs warning (device mmcblk0p1): ext4_end_bio:332: I/O error -5 writing to inode 52 (offset 8388608 size 8388608 starting block 748032)
[1388370.629917] EXT4-fs warning (device mmcblk0p1): ext4_end_bio:332: I/O error -5 writing to inode 52 (offset 8388608 size 8388608 starting block 748288)
[1404756.401859] mmcqd/1: page allocation failure: order:3, mode:0x204020
...
[1404756.402206] active_anon:20306 inactive_anon:3152 isolated_anon:0
active_file:42883 inactive_file:40510 isolated_file:0
unevictable:0 dirty:15 writeback:712 unstable:0
slab_reclaimable:17377 slab_unreclaimable:1433
mapped:10552 shmem:3218 pagetables:394 bounce:0
free:1161 free_pcp:116 free_cma:5
[1404756.402245] Normal free:4644kB min:2860kB low:3572kB high:4288kB active_anon:81224kB inactive_anon:12608kB active_file:171532kB inactive_file:162040kB unevictable:0kB isolated(anon):0kB isolated(file):0kB present:524288kB managed:512036kB mlocked:0kB dirty:60kB writeback:2848kB mapped:42208kB shmem:12872kB slab_reclaimable:69508kB slab_unreclaimable:5732kB kernel_stack:736kB pagetables:1576kB unstable:0kB bounce:0kB free_pcp:464kB local_pcp:464kB free_cma:20kB writeback_tmp:0kB pages_scanned:44 all_unreclaimable? no
[1404756.402254] lowmem_reserve[]: 0 0
[1404756.402264] Normal: 1095*4kB (UEMC) 21*8kB (UMC) 6*16kB (UM) 0*32kB 0*64kB 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB 0*8192kB = 4644kB
[1404756.402296] 86611 total pagecache pages
...
[1404756.402339] 4294964215 pages reserved
[1404756.402344] 6144 pages cma reserved
...
[1404756.460563] blk_update_request: 2546 callbacks suppressed
[1404756.460573] blk_update_request: I/O error, dev mmcblk0, sector 6277632
...
[1404756.547542] buffer_io_error: 2546 callbacks suppressed
[1404756.547551] Buffer I/O error on device mmcblk0p1, logical block 784448
[1404756.554588] Buffer I/O error on device mmcblk0p1, logical block 784449
[1404756.561625] Buffer I/O error on device mmcblk0p1, logical block 784450
[1404756.568663] Buffer I/O error on device mmcblk0p1, logical block 784451
[1404756.575689] Buffer I/O error on device mmcblk0p1, logical block 784452
[1404756.582767] Buffer I/O error on device mmcblk0p1, logical block 784453
[1404756.589822] Buffer I/O error on device mmcblk0p1, logical block 784454
[1404756.596861] Buffer I/O error on device mmcblk0p1, logical block 784455
[1404756.603888] Buffer I/O error on device mmcblk0p1, logical block 784456
[1404756.610923] Buffer I/O error on device mmcblk0p1, logical block 784457
[1404756.627519] EXT4-fs warning (device mmcblk0p1): ext4_end_bio:332: I/O error -5 writing to inode 38392 (offset 0 size 2916352 starting block 785216)
[1404756.630152] EXT4-fs warning (device mmcblk0p1): ext4_end_bio:332: I/O error -5 writing to inode 38392 (offset 0 size 2916352 starting block 785416)
[1404845.787954] mmcqd/1: page allocation failure: order:4, mode:0x204020
...
[1404845.788299] active_anon:20955 inactive_anon:3152 isolated_anon:0
active_file:41364 inactive_file:40956 isolated_file:0
unevictable:0 dirty:7360 writeback:1024 unstable:0
slab_reclaimable:16950 slab_unreclaimable:1441
mapped:10343 shmem:3218 pagetables:397 bounce:0
free:1976 free_pcp:140 free_cma:2
[1404845.788338] Normal free:7904kB min:2860kB low:3572kB high:4288kB active_anon:83820kB inactive_anon:12608kB active_file:165456kB inactive_file:163824kB unevictable:0kB isolated(anon):0kB isolated(file):0kB present:524288kB managed:512036kB mlocked:0kB dirty:29440kB writeback:4096kB mapped:41372kB shmem:12872kB slab_reclaimable:67800kB slab_unreclaimable:5764kB kernel_stack:736kB pagetables:1588kB unstable:0kB bounce:0kB free_pcp:560kB local_pcp:560kB free_cma:8kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no
[1404845.788348] lowmem_reserve[]: 0 0
[1404845.788357] Normal: 1628*4kB (UEMC) 122*8kB (UEM) 14*16kB (UM) 6*32kB (U) 0*64kB 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB 0*8192kB = 7904kB
[1404845.788391] 85540 total pagecache pages
...
[1404847.076047] mmcqd/1: page allocation failure: order:4, mode:0x204020
...
[1404847.076394] active_anon:20955 inactive_anon:3152 isolated_anon:0
active_file:41397 inactive_file:41282 isolated_file:0
unevictable:0 dirty:7316 writeback:1024 unstable:0
slab_reclaimable:16935 slab_unreclaimable:1441
mapped:10349 shmem:3218 pagetables:397 bounce:0
free:1738 free_pcp:35 free_cma:7
[1404847.076433] Normal free:6952kB min:2860kB low:3572kB high:4288kB active_anon:83820kB inactive_anon:12608kB active_file:165588kB inactive_file:165128kB unevictable:0kB isolated(anon):0kB isolated(file):0kB present:524288kB managed:512036kB mlocked:0kB dirty:29264kB writeback:4096kB mapped:41396kB shmem:12872kB slab_reclaimable:67740kB slab_unreclaimable:5764kB kernel_stack:736kB pagetables:1588kB unstable:0kB bounce:0kB free_pcp:140kB local_pcp:140kB free_cma:28kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no
[1404847.076442] lowmem_reserve[]: 0 0
[1404847.076452] Normal: 1328*4kB (UEMC) 141*8kB (UEM) 20*16kB (UM) 6*32kB (U) 0*64kB 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB 0*8192kB = 6952kB
[1404847.076486] 85897 total pagecache pages
...
...
...
...
[1405129.992113] mmcqd/1: page allocation failure: order:4, mode:0x204020
...
[1405129.992456] active_anon:19748 inactive_anon:3152 isolated_anon:0
active_file:40227 inactive_file:49987 isolated_file:0
unevictable:0 dirty:520 writeback:1627 unstable:0
slab_reclaimable:11113 slab_unreclaimable:1456
mapped:8668 shmem:3218 pagetables:385 bounce:0
free:1133 free_pcp:155 free_cma:22
[1405129.992495] Normal free:4532kB min:2860kB low:3572kB high:4288kB active_anon:78992kB inactive_anon:12608kB active_file:160908kB inactive_file:199948kB unevictable:0kB isolated(anon):0kB isolated(file):0kB present:524288kB managed:512036kB mlocked:0kB dirty:2080kB writeback:6508kB mapped:34672kB shmem:12872kB slab_reclaimable:44452kB slab_unreclaimable:5824kB kernel_stack:728kB pagetables:1540kB unstable:0kB bounce:0kB free_pcp:620kB local_pcp:620kB free_cma:88kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no
[1405129.992504] lowmem_reserve[]: 0 0
[1405129.992514] Normal: 1009*4kB (UEMC) 46*8kB (EMC) 6*16kB (E) 1*32kB (M) 0*64kB 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB 0*8192kB = 4532kB
[1405129.992548] 93432 total pagecache pages
...
[1405264.702468] mmcqd/1: page allocation failure: order:3, mode:0x204020
...
[1405264.702813] active_anon:18935 inactive_anon:3151 isolated_anon:0
active_file:41990 inactive_file:48264 isolated_file:0
unevictable:0 dirty:450 writeback:0 unstable:0
slab_reclaimable:10709 slab_unreclaimable:1473
mapped:9511 shmem:3219 pagetables:376 bounce:0
free:2373 free_pcp:88 free_cma:73
[1405264.702854] Normal free:9492kB min:2860kB low:3572kB high:4288kB active_anon:75740kB inactive_anon:12604kB active_file:167960kB inactive_file:193056kB unevictable:0kB isolated(anon):0kB isolated(file):0kB present:524288kB managed:512036kB mlocked:0kB dirty:1800kB writeback:0kB mapped:38044kB shmem:12876kB slab_reclaimable:42836kB slab_unreclaimable:5892kB kernel_stack:728kB pagetables:1504kB unstable:0kB bounce:0kB free_pcp:352kB local_pcp:352kB free_cma:292kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no
[1405264.702862] lowmem_reserve[]: 0 0
[1405264.702872] Normal: 1519*4kB (UEMC) 421*8kB (UEMC) 3*16kB (E) 0*32kB 0*64kB 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB 0*8192kB = 9492kB
[1405264.702905] 93474 total pagecache pages
...
[1405264.851288] Aborting journal on device mmcblk0p1-8.
[1405264.916239] EXT4-fs error (device mmcblk0p1): ext4_journal_check_start:56: Detected aborted journal
[1405264.925972] EXT4-fs (mmcblk0p1): Remounting filesystem read-only
[1405264.933063] EXT4-fs error (device mmcblk0p1): ext4_journal_check_start:56: Detected aborted journal
--
Matthijs
^ permalink raw reply
* [PATCH 5/6] mmc: rtsx_usb_sdmmc: Enable runtime PM autosuspend
From: Ulf Hansson @ 2016-10-13 11:37 UTC (permalink / raw)
To: linux-mmc, linux-kernel, Ulf Hansson
Cc: Ritesh Raj Sarraf, Alan Stern, Oliver Neukum, USB list,
Roger Tseng, Wei WANG
In-Reply-To: <1476358660-8338-1-git-send-email-ulf.hansson@linaro.org>
Enable runtime PM autosuspend for the rtsx_usb_sdmmc driver to avoid the
device being runtime suspended and runtime resumed between each request.
Let's use a default timeout of 50ms, to be consistent with other mmc hosts.
Cc: Ritesh Raj Sarraf <rrs@researchut.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/mmc/host/rtsx_usb_sdmmc.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c
index 6e9c0f8..dc1abd1 100644
--- a/drivers/mmc/host/rtsx_usb_sdmmc.c
+++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
@@ -1374,6 +1374,8 @@ static int rtsx_usb_sdmmc_drv_probe(struct platform_device *pdev)
mutex_init(&host->host_mutex);
rtsx_usb_init_host(host);
+ pm_runtime_use_autosuspend(&pdev->dev);
+ pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
pm_runtime_enable(&pdev->dev);
#ifdef RTSX_USB_USE_LEDS_CLASS
@@ -1428,6 +1430,7 @@ static int rtsx_usb_sdmmc_drv_remove(struct platform_device *pdev)
mmc_free_host(mmc);
pm_runtime_disable(&pdev->dev);
+ pm_runtime_dont_use_autosuspend(&pdev->dev);
platform_set_drvdata(pdev, NULL);
dev_dbg(&(pdev->dev),
--
1.9.1
^ permalink raw reply related
* [PATCH 1/6] mmc: rtsx_usb_sdmmc: Avoid keeping the device runtime resumed when unused
From: Ulf Hansson @ 2016-10-13 11:37 UTC (permalink / raw)
To: linux-mmc, linux-kernel, Ulf Hansson
Cc: Ritesh Raj Sarraf, Alan Stern, Oliver Neukum, USB list,
Roger Tseng, Wei WANG, stable
In-Reply-To: <1476358660-8338-1-git-send-email-ulf.hansson@linaro.org>
The rtsx_usb_sdmmc driver may bail out in its ->set_ios() callback when no
SD card is inserted. This is wrong, as it could cause the device to remain
runtime resumed when it's unused. Fix this behaviour.
Tested-by: Ritesh Raj Sarraf <rrs@researchut.com>
Cc: <stable@vger.kernel.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/mmc/host/rtsx_usb_sdmmc.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c
index 4106295..e0b8590 100644
--- a/drivers/mmc/host/rtsx_usb_sdmmc.c
+++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
@@ -1138,11 +1138,6 @@ static void sdmmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
dev_dbg(sdmmc_dev(host), "%s\n", __func__);
mutex_lock(&ucr->dev_mutex);
- if (rtsx_usb_card_exclusive_check(ucr, RTSX_USB_SD_CARD)) {
- mutex_unlock(&ucr->dev_mutex);
- return;
- }
-
sd_set_power_mode(host, ios->power_mode);
sd_set_bus_width(host, ios->bus_width);
sd_set_timing(host, ios->timing, &host->ddr_mode);
--
1.9.1
^ permalink raw reply related
* [PATCH 6/6] mmc: core: Don't power off the card when starting the host
From: Ulf Hansson @ 2016-10-13 11:37 UTC (permalink / raw)
To: linux-mmc, linux-kernel, Ulf Hansson
Cc: Ritesh Raj Sarraf, Alan Stern, Oliver Neukum, USB list,
Roger Tseng, Wei WANG
In-Reply-To: <1476358660-8338-1-git-send-email-ulf.hansson@linaro.org>
The MMC_CAP2_NO_PRESCAN_POWERUP was invented to avoid running the power up
sequence, mmc_power_up(), during ->probe() of the mmc host driver, but
instead defer this to the mmc detect work. This is especially useful for
those hosts that suffers from a long initialization time, as this time
would otherwise add up to the total boot time.
However, due to the introduction of runtime PM of mmc host devices in the
mmc core, this behaviour changed a bit. More precisely, it caused the mmc
core to runtime resume the host device during ->probe() of the host driver.
In cases like the rtsx_usb_sdmmc, runtime resuming the device may be costly
and thus affecting the total boot time.
To improve this behaviour when using MMC_CAP2_NO_PRESCAN_POWERUP, let's
postpone also calling mmc_power_off() when starting the host. This change
allows the mmc core to avoid runtime resuming the device, as it don't need
to claim the host for that execution path.
Cc: Ritesh Raj Sarraf <rrs@researchut.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/mmc/core/core.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 2553d90..2ad7291 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2824,12 +2824,11 @@ void mmc_start_host(struct mmc_host *host)
host->rescan_disable = 0;
host->ios.power_mode = MMC_POWER_UNDEFINED;
- mmc_claim_host(host);
- if (host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP)
- mmc_power_off(host);
- else
+ if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP)) {
+ mmc_claim_host(host);
mmc_power_up(host, host->ocr_avail);
- mmc_release_host(host);
+ mmc_release_host(host);
+ }
mmc_gpiod_request_cd_irq(host);
_mmc_detect_change(host, 0, false);
--
1.9.1
^ permalink raw reply related
* [PATCH 4/6] memstick: rtsx_usb_ms: Manage runtime PM when accessing the device
From: Ulf Hansson @ 2016-10-13 11:37 UTC (permalink / raw)
To: linux-mmc, linux-kernel, Ulf Hansson
Cc: Ritesh Raj Sarraf, Alan Stern, Oliver Neukum, USB list,
Roger Tseng, Wei WANG, stable
In-Reply-To: <1476358660-8338-1-git-send-email-ulf.hansson@linaro.org>
Accesses to the rtsx usb device, which is the parent of the rtsx memstick
device, must not be done unless it's runtime resumed. This is currently not
the case and it could trigger various errors.
Fix this by properly deal with runtime PM in this regards. This means
making sure the device is runtime resumed, when serving requests via the
->request() callback or changing settings via the ->set_param() callbacks.
Cc: <stable@vger.kernel.org>
Cc: Ritesh Raj Sarraf <rrs@researchut.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/memstick/host/rtsx_usb_ms.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/memstick/host/rtsx_usb_ms.c b/drivers/memstick/host/rtsx_usb_ms.c
index 1b99489..2e3cf01 100644
--- a/drivers/memstick/host/rtsx_usb_ms.c
+++ b/drivers/memstick/host/rtsx_usb_ms.c
@@ -524,6 +524,7 @@ static void rtsx_usb_ms_handle_req(struct work_struct *work)
int rc;
if (!host->req) {
+ pm_runtime_get_sync(ms_dev(host));
do {
rc = memstick_next_req(msh, &host->req);
dev_dbg(ms_dev(host), "next req %d\n", rc);
@@ -544,6 +545,7 @@ static void rtsx_usb_ms_handle_req(struct work_struct *work)
host->req->error);
}
} while (!rc);
+ pm_runtime_put(ms_dev(host));
}
}
@@ -570,6 +572,7 @@ static int rtsx_usb_ms_set_param(struct memstick_host *msh,
dev_dbg(ms_dev(host), "%s: param = %d, value = %d\n",
__func__, param, value);
+ pm_runtime_get_sync(ms_dev(host));
mutex_lock(&ucr->dev_mutex);
err = rtsx_usb_card_exclusive_check(ucr, RTSX_USB_MS_CARD);
@@ -635,6 +638,7 @@ static int rtsx_usb_ms_set_param(struct memstick_host *msh,
}
out:
mutex_unlock(&ucr->dev_mutex);
+ pm_runtime_put(ms_dev(host));
/* power-on delay */
if (param == MEMSTICK_POWER && value == MEMSTICK_POWER_ON)
--
1.9.1
^ permalink raw reply related
* [PATCH 3/6] memstick: rtsx_usb_ms: Runtime resume the device when polling for cards
From: Ulf Hansson @ 2016-10-13 11:37 UTC (permalink / raw)
To: linux-mmc, linux-kernel, Ulf Hansson
Cc: Ritesh Raj Sarraf, Alan Stern, Oliver Neukum, USB list,
Roger Tseng, Wei WANG, stable
In-Reply-To: <1476358660-8338-1-git-send-email-ulf.hansson@linaro.org>
From: Alan Stern <stern@rowland.harvard.edu>
Accesses to the rtsx usb device, which is the parent of the rtsx memstick
device, must not be done unless it's runtime resumed.
Therefore when the rtsx_usb_ms driver polls for inserted memstick cards,
let's add pm_runtime_get|put*() to make sure accesses is done when the
rtsx usb device is runtime resumed.
Reported-by: Ritesh Raj Sarraf <rrs@researchut.com>
Tested-by: Ritesh Raj Sarraf <rrs@researchut.com>
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Cc: <stable@vger.kernel.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/memstick/host/rtsx_usb_ms.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/memstick/host/rtsx_usb_ms.c b/drivers/memstick/host/rtsx_usb_ms.c
index d34bc35..1b99489 100644
--- a/drivers/memstick/host/rtsx_usb_ms.c
+++ b/drivers/memstick/host/rtsx_usb_ms.c
@@ -681,6 +681,7 @@ static int rtsx_usb_detect_ms_card(void *__host)
int err;
for (;;) {
+ pm_runtime_get_sync(ms_dev(host));
mutex_lock(&ucr->dev_mutex);
/* Check pending MS card changes */
@@ -703,6 +704,7 @@ static int rtsx_usb_detect_ms_card(void *__host)
}
poll_again:
+ pm_runtime_put(ms_dev(host));
if (host->eject)
break;
--
1.9.1
^ permalink raw reply related
* [PATCH 2/6] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing the led
From: Ulf Hansson @ 2016-10-13 11:37 UTC (permalink / raw)
To: linux-mmc, linux-kernel, Ulf Hansson
Cc: Ritesh Raj Sarraf, Alan Stern, Oliver Neukum, USB list,
Roger Tseng, Wei WANG, stable
In-Reply-To: <1476358660-8338-1-git-send-email-ulf.hansson@linaro.org>
Accesses of the rtsx sdmmc's parent device, which is the rtsx usb device,
must be done when it's runtime resumed. Currently this isn't case when
changing the led, so let's fix this by adding a pm_runtime_get_sync() and
a pm_runtime_put() around those operations.
Reported-by: Ritesh Raj Sarraf <rrs@researchut.com>
Tested-by: Ritesh Raj Sarraf <rrs@researchut.com>
Cc: <stable@vger.kernel.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/mmc/host/rtsx_usb_sdmmc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c
index e0b8590..6e9c0f8 100644
--- a/drivers/mmc/host/rtsx_usb_sdmmc.c
+++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
@@ -1309,6 +1309,7 @@ static void rtsx_usb_update_led(struct work_struct *work)
container_of(work, struct rtsx_usb_sdmmc, led_work);
struct rtsx_ucr *ucr = host->ucr;
+ pm_runtime_get_sync(sdmmc_dev(host));
mutex_lock(&ucr->dev_mutex);
if (host->led.brightness == LED_OFF)
@@ -1317,6 +1318,7 @@ static void rtsx_usb_update_led(struct work_struct *work)
rtsx_usb_turn_on_led(ucr);
mutex_unlock(&ucr->dev_mutex);
+ pm_runtime_put(sdmmc_dev(host));
}
#endif
--
1.9.1
^ permalink raw reply related
* [PATCH 0/6] mmc/memstick: rtsx_usb: Fix runtime PM issues
From: Ulf Hansson @ 2016-10-13 11:37 UTC (permalink / raw)
To: linux-mmc, linux-kernel, Ulf Hansson
Cc: Ritesh Raj Sarraf, Alan Stern, Oliver Neukum, USB list,
Roger Tseng, Wei WANG
The rtsx_usb_sdmmc (mmc/sd) and rtsx_usb_ms (memstick) devices are interfacing
an rtsx_usb device (managed by an mfd driver) while communicating with the
cards. The mmc/sd and memstick devices are children of the usb device.
Issues has been reported [1] for rtsx, which have been investigated, discussed,
fixed, tested, etc, particularly when using mmc/sd cards. During the
investigation, some changes was proposed and tested successfully. In this
series I have picked up these changes and created some proper patches with
change-logs.
It turned out that most of the problems was related to the runtime PM
deployment in the memstick and the mmc/sd driver, which are fixed in this
series.
A couple of more issues were also discussed [2], which needs to be fixed as
well. Although they are out of scope for this series, so we will have to look
into those at a later point. Here's a brief summary of these leftovers:
*)
It seems reasonable to turn off autosuspend for usb devices and instead rely on
the usb device's children to deal with autosuspend. The reason is simply that
the children has better knowledge of when using autosuspend makes sense.
**)
Polling card detect mode is used both for mmc/sd and memstick. Because of the
lack of synchronization between the polling attempts for these subsystems, the
usb device may be powered on for unnecessary long intervals, thus we are
wasting power.
Besides the above changes, I folded in a change in the mmc core which improves
the behaviour for mmc hosts using MMC_CAP2_NO_PRESCAN_POWERUP, which is the
case for the rtsx_usb_sdmmc driver. This change should improve the boot time
with ~100ms per rtsx_usb_sdmmc device.
Finally, as I couldn't find an active maintainer for the memstick
subsystem/driver and because the author of the drivers can be reached, I
volunteer to take this all through my mmc tree. Please tell me if that is
problem to any of you.
[1]
https://www.spinics.net/lists/linux-usb/msg146998.html
[2]
https://www.spinics.net/lists/linux-mmc/msg39235.html
Alan Stern (1):
memstick: rtsx_usb_ms: Runtime resume the device when polling for
cards
Ulf Hansson (5):
mmc: rtsx_usb_sdmmc: Avoid keeping the device runtime resumed when
unused
mmc: rtsx_usb_sdmmc: Handle runtime PM while changing the led
memstick: rtsx_usb_ms: Manage runtime PM when accessing the device
mmc: rtsx_usb_sdmmc: Enable runtime PM autosuspend
mmc: core: Don't power off the card when starting the host
drivers/memstick/host/rtsx_usb_ms.c | 6 ++++++
drivers/mmc/core/core.c | 9 ++++-----
drivers/mmc/host/rtsx_usb_sdmmc.c | 10 +++++-----
3 files changed, 15 insertions(+), 10 deletions(-)
--
1.9.1
^ permalink raw reply
* Re: [PATCH 1/3] mmc: replacing hardcoded value for runtime PM suspend
From: Shawn Lin @ 2016-10-13 8:30 UTC (permalink / raw)
To: Ulf Hansson, Alex Lemberg
Cc: shawn.lin, linux-mmc@vger.kernel.org, Avi Shchislowski
In-Reply-To: <CAPDyKFo=xUkGvqghr3zFB_anvzVQDJgkjFziveceQ+jpjjxdbQ@mail.gmail.com>
在 2016/10/13 15:36, Ulf Hansson 写道:
> On 4 September 2016 at 15:30, Alex Lemberg <Alex.Lemberg@sandisk.com> wrote:
>> Hi Shawn,
>>
>> On 9/2/16, 10:39 AM, "Shawn Lin" <shawn.lin@rock-chips.com> wrote:
>>
>>> Hi
>>>
>>> On 2016/9/1 22:24, alex lemberg wrote:
>>>> Add MMC_AUTOSUSPEND_DELAY_MS define
>>>
>>> I'm thinking should we expose it to userspace
>>> via sysfs, since I don't know why it should be 3ms, not 4ms, 5ms, etc.
>>
>> Agree, we can consider exposing it to the user space.
>> The exposure to the user space is less critical to the current
>> patchset, but we can do it as a “follow-up change” once its approved.
>
> No need for that, is already available for user-space. :-)
>
> Each device with runtime PM autosuspend enabled, allows userspace to
> change the delay via the "autosuspend_delay_ms" sysfs file.
Indeed， I just check it when enabling runtime PM for dw_mmc and could
able to change the value of autosuspend_delay_ms.
Thanks for sharing this.
>
> 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
>
--
Best Regards
Shawn Lin
^ permalink raw reply
* Re: [PATCH 1/3] mmc: replacing hardcoded value for runtime PM suspend
From: Ulf Hansson @ 2016-10-13 7:36 UTC (permalink / raw)
To: alex.lemberg; +Cc: linux-mmc, Avi Shchislowski
In-Reply-To: <1472706726-893-1-git-send-email-alex.lemberg@sandisk.com>
On 1 September 2016 at 07:12, alex.lemberg <alex.lemberg@sandisk.com> wrote:
> Add MMC_AUTOSUSPEND_DELAY_MS define
>
> Signed-off-by: alex.lemberg <alex.lemberg@sandisk.com>
> ---
> drivers/mmc/card/block.c | 3 ++-
> include/linux/mmc/host.h | 2 ++
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 48a5dd7..f4386ed 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -2610,7 +2610,8 @@ static int mmc_blk_probe(struct mmc_card *card)
> goto out;
> }
>
> - pm_runtime_set_autosuspend_delay(&card->dev, 3000);
> + pm_runtime_set_autosuspend_delay(&card->dev,
> + MMC_RUNTIME_SUSPEND_DELAY_MS);
> pm_runtime_use_autosuspend(&card->dev);
>
> /*
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index aa4bfbf..8cb8218 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -22,6 +22,8 @@
> #include <linux/mmc/mmc.h>
> #include <linux/mmc/pm.h>
>
> +#define MMC_RUNTIME_SUSPEND_DELAY_MS 3000
> +
> struct mmc_ios {
> unsigned int clock; /* clock rate */
> unsigned short vdd;
> --
> 1.9.1
>
Actually, I don't mind having hardcoded values like this one directly
in the code. Especially since it's used only at one single place and
because it's not a magic number.
Although, if really feel strong about this, I can apply it.
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH 1/3] mmc: replacing hardcoded value for runtime PM suspend
From: Ulf Hansson @ 2016-10-13 7:36 UTC (permalink / raw)
To: Alex Lemberg; +Cc: Shawn Lin, linux-mmc@vger.kernel.org, Avi Shchislowski
In-Reply-To: <2D82096E-231C-43BA-801D-381F8191F26C@sandisk.com>
On 4 September 2016 at 15:30, Alex Lemberg <Alex.Lemberg@sandisk.com> wrote:
> Hi Shawn,
>
> On 9/2/16, 10:39 AM, "Shawn Lin" <shawn.lin@rock-chips.com> wrote:
>
>>Hi
>>
>>On 2016/9/1 22:24, alex lemberg wrote:
>>> Add MMC_AUTOSUSPEND_DELAY_MS define
>>
>>I'm thinking should we expose it to userspace
>>via sysfs, since I don't know why it should be 3ms, not 4ms, 5ms, etc.
>
> Agree, we can consider exposing it to the user space.
> The exposure to the user space is less critical to the current
> patchset, but we can do it as a “follow-up change” once its approved.
No need for that, is already available for user-space. :-)
Each device with runtime PM autosuspend enabled, allows userspace to
change the delay via the "autosuspend_delay_ms" sysfs file.
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH 2/3] mmc: Adding AUTO_BKOPS_EN bit set for Auto BKOPS support
From: Ulf Hansson @ 2016-10-13 7:36 UTC (permalink / raw)
To: alex lemberg; +Cc: linux-mmc, Avi Shchislowski
In-Reply-To: <1472739890-3384-3-git-send-email-alex.lemberg@sandisk.com>
On 1 September 2016 at 16:24, alex lemberg <alex.lemberg@sandisk.com> wrote:
> Signed-off-by: alex lemberg <alex.lemberg@sandisk.com>
> ---
> drivers/mmc/core/mmc.c | 6 ++++++
> include/linux/mmc/card.h | 1 +
> include/linux/mmc/mmc.h | 1 +
> 3 files changed, 8 insertions(+)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index f2d185c..e2e987f 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -531,6 +531,12 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
> if (!card->ext_csd.man_bkops_en)
> pr_debug("%s: MAN_BKOPS_EN bit is not set\n",
> mmc_hostname(card->host));
> + card->ext_csd.auto_bkops_en =
> + (ext_csd[EXT_CSD_BKOPS_EN] &
> + EXT_CSD_AUTO_BKOPS_MASK);
> + if (!card->ext_csd.auto_bkops_en)
> + pr_debug("%s: AUTO_BKOPS_EN bit is not set\n",
> + mmc_hostname(card->host));
According to discussions here [1], I am wondering whether we actually
should inverse the logic for when printing the debug message.
Isn't it more relevant to know when the bits *are* set than rather
when they aren't?
> }
>
> /* check whether the eMMC card supports HPI */
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 73fad83..aaedb68 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -84,6 +84,7 @@ struct mmc_ext_csd {
> unsigned int hpi_cmd; /* cmd used as HPI */
> bool bkops; /* background support bit */
> bool man_bkops_en; /* manual bkops enable bit */
> + bool auto_bkops_en; /* auto bkops enable bit */
> unsigned int data_sector_size; /* 512 bytes or 4KB */
> unsigned int data_tag_unit_size; /* DATA TAG UNIT size */
> unsigned int boot_ro_lock; /* ro lock support */
> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
> index c376209..0fe3908 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -436,6 +436,7 @@ struct _mmc_csd {
> * BKOPS modes
> */
> #define EXT_CSD_MANUAL_BKOPS_MASK 0x01
> +#define EXT_CSD_AUTO_BKOPS_MASK 0x02
>
> /*
> * MMC_SWITCH access modes
> --
> 1.9.1
>
Kind regards
Uffe
[1]
http://www.spinics.net/lists/linux-mmc/msg39553.html
^ permalink raw reply
* Re: [PATCH 3/3] mmc: Checking BKOPS status prior to Suspend
From: Ulf Hansson @ 2016-10-13 7:36 UTC (permalink / raw)
To: alex lemberg; +Cc: linux-mmc, Avi Shchislowski
In-Reply-To: <1472739890-3384-4-git-send-email-alex.lemberg@sandisk.com>
On 1 September 2016 at 16:24, alex lemberg <alex.lemberg@sandisk.com> wrote:
> Rescheduling Suspend in case of BKOPS Level >= 1
> in order to let eMMC device to complete its internal GC.
> Applicable for Runtime Suspend Only.
>
> Signed-off-by: alex lemberg <alex.lemberg@sandisk.com>
> ---
> drivers/mmc/core/mmc.c | 30 +++++++++++++++++++++++-------
> include/linux/mmc/mmc.h | 1 +
> 2 files changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index e2e987f..c4c6326 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1904,7 +1904,8 @@ static void mmc_detect(struct mmc_host *host)
> }
> }
>
> -static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
> +static int _mmc_suspend(struct mmc_host *host, bool is_suspend,
> + bool is_runtime_pm)
> {
> int err = 0;
> unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT :
> @@ -1918,10 +1919,25 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
> if (mmc_card_suspended(host->card))
> goto out;
>
> - if (mmc_card_doing_bkops(host->card)) {
> - err = mmc_stop_bkops(host->card);
> - if (err)
> + if (mmc_card_doing_bkops(host->card) ||
> + host->card->ext_csd.auto_bkops_en) {
> + err = mmc_read_bkops_status(host->card);
> + if (err) {
> + pr_err("%s: error %d reading BKOPS Status\n",
> + mmc_hostname(host), err);
> + goto out;
> + }
I would appreciate some simple comment of what you want to do here below.
> + if (is_runtime_pm && host->card->ext_csd.raw_bkops_status >=
> + EXT_CSD_BKOPS_LEVEL_1) {
> + pm_schedule_suspend(&host->card->dev,
> + MMC_RUNTIME_SUSPEND_DELAY_MS);
> goto out;
If I understand correctly, you would like to abort the runtime suspend
and allow the background operations to complete. That seems
reasonable, although in such case you need to return -EBUSY from this
function.
Moreover, perhaps we should discuss at what BKOPS_LEVEL* we should
allow to abort.
> + }
> + if (mmc_card_doing_bkops(host->card)) {
> + err = mmc_stop_bkops(host->card);
> + if (err)
> + goto out;
> + }
> }
>
> err = mmc_flush_cache(host->card);
> @@ -1952,7 +1968,7 @@ static int mmc_suspend(struct mmc_host *host)
> {
> int err;
>
> - err = _mmc_suspend(host, true);
> + err = _mmc_suspend(host, true, false);
> if (!err) {
> pm_runtime_disable(&host->card->dev);
> pm_runtime_set_suspended(&host->card->dev);
> @@ -2002,7 +2018,7 @@ static int mmc_shutdown(struct mmc_host *host)
> err = _mmc_resume(host);
>
> if (!err)
> - err = _mmc_suspend(host, false);
> + err = _mmc_suspend(host, false, false);
>
> return err;
> }
> @@ -2026,7 +2042,7 @@ static int mmc_runtime_suspend(struct mmc_host *host)
> if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
> return 0;
>
> - err = _mmc_suspend(host, true);
> + err = _mmc_suspend(host, true, true);
> if (err)
> pr_err("%s: error %d doing aggressive suspend\n",
> mmc_hostname(host), err);
> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
> index 0fe3908..0f08d5c 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -430,6 +430,7 @@ struct _mmc_csd {
> /*
> * BKOPS status level
> */
> +#define EXT_CSD_BKOPS_LEVEL_1 0x1
> #define EXT_CSD_BKOPS_LEVEL_2 0x2
>
> /*
> --
> 1.9.1
>
Another idea I had, was to actually make use of that we know the card
will be inactive. So instead of checking if there is an ongoing BKOPS
(mmc_card_doing_bkops()), in case of the manual BKOPS, we could
potentially check the needed BKOPS LEVEL and schedule a BKOPS to
start. In other words, do manual BKOPS when the card is idle.
Of course, one need to be able to interrupt/cancel the BKOPS if there
is a new request that needs to be managed. I am not sure how we can
achieve that, but I hope you get the idea.
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH v2] sdhci-esdhc-imx: Correct two register accesses
From: Ulf Hansson @ 2016-10-13 7:01 UTC (permalink / raw)
To: Aaron Brice
Cc: Adrian Hunter, Aisheng Dong, linux-mmc,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Dave Russell
In-Reply-To: <1476124792-18441-1-git-send-email-aaron.brice@datasoft.com>
On 10 October 2016 at 20:39, Aaron Brice <aaron.brice@datasoft.com> wrote:
>
> - The DMA error interrupt bit is in a different position as
> compared to the sdhci standard. This is accounted for in
> many cases, but not handled in the case of clearing the
> INT_STATUS register by writing a 1 to that location.
> - The HOST_CONTROL register is very different as compared to
> the sdhci standard. This is accounted for in the write
> case, but not when read back out (which it is in the sdhci
> code).
>
> Signed-off-by: Dave Russell <david.russell@datasoft.com>
> Signed-off-by: Aaron Brice <aaron.brice@datasoft.com>
> Acked-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
Thanks, applied for fixes!
Kind regards
Uffe
>
> v1 -> v2:
> - rename long_val to val
>
> drivers/mmc/host/sdhci-esdhc-imx.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 1f54fd8..7123ef9 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -346,7 +346,8 @@ static void esdhc_writel_le(struct sdhci_host *host, u32 val, int reg)
> struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
> u32 data;
>
> - if (unlikely(reg == SDHCI_INT_ENABLE || reg == SDHCI_SIGNAL_ENABLE)) {
> + if (unlikely(reg == SDHCI_INT_ENABLE || reg == SDHCI_SIGNAL_ENABLE ||
> + reg == SDHCI_INT_STATUS)) {
> if ((val & SDHCI_INT_CARD_INT) && !esdhc_is_usdhc(imx_data)) {
> /*
> * Clear and then set D3CD bit to avoid missing the
> @@ -555,6 +556,25 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
> esdhc_clrset_le(host, 0xffff, val, reg);
> }
>
> +static u8 esdhc_readb_le(struct sdhci_host *host, int reg)
> +{
> + u8 ret;
> + u32 val;
> +
> + switch (reg) {
> + case SDHCI_HOST_CONTROL:
> + val = readl(host->ioaddr + reg);
> +
> + ret = val & SDHCI_CTRL_LED;
> + ret |= (val >> 5) & SDHCI_CTRL_DMA_MASK;
> + ret |= (val & ESDHC_CTRL_4BITBUS);
> + ret |= (val & ESDHC_CTRL_8BITBUS) << 3;
> + return ret;
> + }
> +
> + return readb(host->ioaddr + reg);
> +}
> +
> static void esdhc_writeb_le(struct sdhci_host *host, u8 val, int reg)
> {
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -947,6 +967,7 @@ static void esdhc_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
> static struct sdhci_ops sdhci_esdhc_ops = {
> .read_l = esdhc_readl_le,
> .read_w = esdhc_readw_le,
> + .read_b = esdhc_readb_le,
> .write_l = esdhc_writel_le,
> .write_w = esdhc_writew_le,
> .write_b = esdhc_writeb_le,
> --
> 2.7.4
>
^ permalink raw reply
* 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
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