Re: [PATCH] mmc: don't request CD IRQ until mmc_start_host()
On 09/18/2014 08:25 AM, Adrian Hunter wrote:
On 09/17/2014 10:57 PM, Stephen Warren wrote:
On 09/17/2014 01:55 PM, Ulf Hansson wrote:
On 12 September 2014 19:18, Stephen Warren swar...@wwwdotorg.org wrote:
From: Stephen Warren swar...@nvidia.com
As soon as the CD IRQ is requested, it can trigger, since it's an
externally controlled event. If it does, delayed_work host-detect will
be scheduled.
Many host controller probe()s are roughly structured as:
*_probe() {
host = sdhci_pltfm_init();
mmc_of_parse(host-mmc);
rc = sdhci_add_host(host);
if (rc) {
sdhci_pltfm_free();
return rc;
}
In 3.17, CD IRQs can are enabled quite early via *_probe() -
mmc_of_parse() - mmc_gpio_request_cd() - mmc_gpiod_request_cd_irq().
Note that in linux-next, mmc_of_parse() calls mmc_gpio*d*_request_cd()
rather than mmc_gpio_request_cd(), and mmc_gpio*d*_request_cd() doesn't
call mmc_gpiod_request_cd_irq(). However, this issue still exists for
any other direct users of mmc_gpio_request_cd().
sdhci_add_host() may fail part way through (e.g. due to deferred
probe for a vmmc regulator), and sdhci_pltfm_free() does nothing to
unrequest the CD IRQ nor cancel the delayed_work. sdhci_pltfm_free() is
coded to assume that if sdhci_add_host() failed, then the delayed_work
cannot (or should not) have been triggered.
This can lead to the following with CONFIG_DEBUG_OBJECTS_* enabled, when
kfree(host) is eventually called inside sdhci_pltfm_free():
WARNING: CPU: 2 PID: 6 at lib/debugobjects.c:263
debug_print_object+0x8c/0xb4()
ODEBUG: free active (active state 0) object type: timer_list hint:
delayed_work_timer_fn+0x0/0x18
The object being complained about is host-detect.
There's no need to request the CD IRQ so early; mmc_start_host() already
requests it, and I *assume* that mmc_start_host() is called somehow for
all host controllers. For SDHCI hosts at least, the typical call path
that does this is: *_probe() - sdhci_add_host() - mmc_add_host() -
mmc_start_host(). So, remove the call to mmc_gpiod_request_cd_irq() from
mmc_gpio_request_cd(). This matches mmc_gpio*d*_request_cd(), which
already doesn't call mmc_gpiod_request_cd_irq().
This solves the problem (eliminates the kernel error message above),
since it guarantees that the IRQ can't trigger before mmc_start_host()
is called.
The critical point here is that once sdhci_add_host() calls
mmc_add_host() - mmc_start_host(), sdhci_add_host() is coded not to
fail. In other words, if there's a chance that mmc_start_host() may have
been called, and CD IRQs triggered, and the delayed_work scheduled,
sdhci_add_host() won't fail, and so cleanup is no longer via
sdhci_pltfm_free() (which doesn't free the IRQ or cancel the work queue)
but instead must be via sdhci_remove_host(), which calls mmc_remove_host()
- mmc_stop_host(), which does free the IRQ and cancel the work queue.
This fixes what I might conclude to be a mistake in commit 740a221ef0e5
(mmc: slot-gpio: Add GPIO descriptor based CD GPIO API), which added the
call from mmc_start_host() to mmc_gpiod_request_cd_irq(), but also added
incorrectly added a call from mmc_gpio_request_cd() to
mmc_gpiod_request_cd_irq().
That comment is wrong. mmc_gpio_request_cd() has always set up the irq.
CC: Russell King li...@arm.linux.org.uk
Cc: Adrian Hunter adrian.hun...@intel.com
Cc: Alexandre Courbot acour...@nvidia.com
Cc: Linus Walleij linus.wall...@linaro.org
Signed-off-by: Stephen Warren swar...@nvidia.com
Hi Stephen,
Thanks for looking into this. It seems like this issue has been
present for quite a while.
I believe your patch should have a stable tag for 3.15+ as well,
unless you object I will add it.
Yes, that probably makes sense, thanks.
Doesn't this patch break the drivers that call mmc_gpio_request_cd() after
mmc_add_host() like mmc_spi.c or sdhci-sirf.c or tmio_mmc_pio.c ?
Ulf, this should be reverted.
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: rtsx: add card power off during probe
On Wed, 2014-09-17 at 21:29 +0200, Ulf Hansson wrote:
On 17 September 2014 11:11, micky micky_ch...@realsil.com.cn wrote:
On 09/17/2014 02:01 AM, Ulf Hansson wrote:
On 12 September 2014 03:39, micky_ch...@realsil.com.cn wrote:
From: Roger Tseng rogera...@realtek.com
Some platform have both UEFI driver and MFD/mmc driver, if entering
linux while card in the slot, the card power is already on, and rtsx-mmc
driver have no chance to make card power off. This will lead UHSI card
failed to enter UHSI mode.
It is hard to control the UEFI driver leaving state, so we power off the
card power during probe.
Signed-off-by: Roger Tseng rogera...@realtek.com
Signed-off-by: Micky Ching micky_ch...@realsil.com.cn
---
drivers/mmc/host/rtsx_pci_sdmmc.c |7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c
b/drivers/mmc/host/rtsx_pci_sdmmc.c
index dfde4a2..57b0796 100644
--- a/drivers/mmc/host/rtsx_pci_sdmmc.c
+++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
@@ -1341,8 +1341,13 @@ static int rtsx_pci_sdmmc_drv_probe(struct
platform_device *pdev)
host-pcr = pcr;
host-mmc = mmc;
host-pdev = pdev;
- host-power_state = SDMMC_POWER_OFF;
INIT_WORK(host-work, sd_request);
+ sd_power_off(host);
+ /*
+* ref: SD spec 3.01: 6.4.1.2 Power On or Power Cycle
+*/
+ usleep_range(1000, 2000);
+
This won't work in cases were you power off eMMC cards, unless you can
do a full power cycle - cut both VCC and VCCQ. Can you?
Hi Uffe,
VCCQ will poweroff at the same time.
In that case, don't forget to enable MMC_CAP2_FULL_PWR_CYCLE.
if MMC_CAP2_NO_PRESCAN_POWERUP enable, will call mmc_power_off() at start,
then it will check ios.power_mode, but the state is MMC_POWER_OFF and just
return.
Uhh, that's right! So, I wonder why we invokes mmc_power_off() from
that path at all.
Hmm, I think we should change the behavior in mmc_start_host(), like below:
1) Add a MMC_POWER_UNDEFINED state which is what the power state
should be assigned to at allocation.
2 ) From mmc_start_host(), invoke mmc_power_off() when
MMC_CAP2_NO_PRESCAN_POWERUP and MMC_CAP2_FULL_PWR_CYCLE is set.
Would that work?
Yes. I have confirmed this by following changes. The MMC_POWER_UNDEFINED
designation in mmc_start_host() will eventually cause a power-off
operation.
But I wonder if we need to additionally check MMC_CAP2_FULL_PWR_CYCLE
before calling mmc_power_off()?
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index d03a080fb9cd..3457b0f74b71 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2489,7 +2489,9 @@ void mmc_start_host(struct mmc_host *host)
{
host-f_init = max(freqs[0], host-f_min);
host-rescan_disable = 0;
- if (host-caps2 MMC_CAP2_NO_PRESCAN_POWERUP)
+ host-ios.power_mode = MMC_POWER_UNDEFINED;
+ if (host-caps2 (MMC_CAP2_NO_PRESCAN_POWERUP |
+ MMC_CAP2_FULL_PWR_CYCLE))
mmc_power_off(host);
else
mmc_power_up(host, host-ocr_avail);
diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c
b/drivers/mmc/host/rtsx_pci_sdmmc.c
index dfde4a210238..d49460b5ff07 100644
--- a/drivers/mmc/host/rtsx_pci_sdmmc.c
+++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
@@ -1292,6 +1292,7 @@ static void realtek_init_host(struct
realtek_pci_sdmmc *host)
mmc-caps = MMC_CAP_4_BIT_DATA | MMC_CAP_SD_HIGHSPEED |
MMC_CAP_MMC_HIGHSPEED | MMC_CAP_BUS_WIDTH_TEST |
MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25;
+ mmc-caps2 = MMC_CAP2_NO_PRESCAN_POWERUP |
MMC_CAP2_FULL_PWR_CYCLE;
mmc-max_current_330 = 400;
mmc-max_current_180 = 800;
mmc-ops = realtek_pci_sdmmc_ops;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 7960424d0bc0..b3bfa609816a 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -42,6 +42,7 @@ struct mmc_ios {
#define MMC_POWER_OFF 0
#define MMC_POWER_UP 1
#define MMC_POWER_ON 2
+#define MMC_POWER_UNDEFINED3
unsigned char bus_width; /* data bus width */
Kind regards
Uffe
Best Regards.
micky.
There are also another option you might want to use,
MMC_CAP2_NO_PRESCAN_POWERUP. But again, it must only be used for those
hosts that you are able to do a full power cycle for.
Kind regards
Uffe
platform_set_drvdata(pdev, host);
pcr-slots[RTSX_SD_CARD].p_dev = pdev;
pcr-slots[RTSX_SD_CARD].card_event = rtsx_pci_sdmmc_card_event;
--
1.7.9.5
.
--Please consider the environment before printing this e-mail.
--
Best regards,
Roger Tseng
Re: [PATCH 6/7] mmc: sdhci-esdhc-imx: add imx6sx support
On Thu, Sep 18, 2014 at 12:29:52AM +0200, Ulf Hansson wrote:
On 3 September 2014 14:05, Dong Aisheng b29...@freescale.com wrote:
The imx6sx usdhc is derived from imx6sl, the difference is minor.
imx6sx have the errata ESDHC_FLAG_ERR004536 fixed.
So introduce a new compatible string for imx6sx to distinguish them.
Signed-off-by: Dong Aisheng b29...@freescale.com
Hi Dong,
This one has checkpatch errors due to missing DT documentation.
The original binding doc is writing in the format of:
Required properties:
-- compatible : Should be fsl,chip-esdhc
It just provides a rule and does not provide the specific compatible string.
So i did not update the doc before.
But i think it's better to fix it to avoid future warning again.
Can you help add below patch before this commit or do you need me
to resend the patch series again with this patch added?
From 467b84e5ffcba543b9ac88913b1d2dc1159dfa72 Mon Sep 17 00:00:00 2001
From: Dong Aisheng b29...@freescale.com
Date: Thu, 18 Sep 2014 13:11:03 +0800
Subject: [PATCH 6/8] mmc: sdhci-esdhc-imx: using specific compatible string
in binding doc
Using specific compatible string in binding doc to make the binding
more clear.
It's also used to avoid checkpatch warning in the future like follows:
0005-mmc-sdhci-do-not-enable-card-cd-wakeup-for-gpio-case.patch has no obvious
style problems and is ready for submission.
WARNING: DT compatible string fsl,imx6sx-usdhc appears un-documented -- check
./Documentation/devicetree/bindings/
+ { .compatible = fsl,imx6sx-usdhc, .data = usdhc_imx6sx_data, },
total: 0 errors, 1 warnings, 18 lines checked
Signed-off-by: Dong Aisheng b29...@freescale.com
---
.../devicetree/bindings/mmc/fsl-imx-esdhc.txt |9 -
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
index 9046ba06..c415d34 100644
--- a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
+++ b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
@@ -7,7 +7,14 @@ This file documents differences between the core properties
described
by mmc.txt and the properties used by the sdhci-esdhc-imx driver.
Required properties:
-- compatible : Should be fsl,chip-esdhc
+- compatible : Should be fsl,chip-esdhc, the supported chips include
+ fsl,imx25-esdhc
+ fsl,imx35-esdhc
+ fsl,imx51-esdhc
+ fsl,imx53-esdhc
+ fsl,imx6q-usdhc
+ fsl,imx6sl-usdhc
+ fsl,imx6sx-usdhc
Optional properties:
- fsl,cd-controller : Indicate to use controller internal card detection
--
1.7.8
Regards
Dong Aisheng
Kind regards
Uffe
---
drivers/mmc/host/sdhci-esdhc-imx.c |6 ++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
b/drivers/mmc/host/sdhci-esdhc-imx.c
index dc0e384..87179c4 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -150,6 +150,11 @@ static struct esdhc_soc_data usdhc_imx6sl_data = {
| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_ERR004536,
};
+static struct esdhc_soc_data usdhc_imx6sx_data = {
+ .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
+ | ESDHC_FLAG_HAVE_CAP1,
+};
+
struct pltfm_imx_data {
u32 scratchpad;
struct pinctrl *pinctrl;
@@ -190,6 +195,7 @@ static const struct of_device_id imx_esdhc_dt_ids[] = {
{ .compatible = fsl,imx35-esdhc, .data = esdhc_imx35_data, },
{ .compatible = fsl,imx51-esdhc, .data = esdhc_imx51_data, },
{ .compatible = fsl,imx53-esdhc, .data = esdhc_imx53_data, },
+ { .compatible = fsl,imx6sx-usdhc, .data = usdhc_imx6sx_data, },
{ .compatible = fsl,imx6sl-usdhc, .data = usdhc_imx6sl_data, },
{ .compatible = fsl,imx6q-usdhc, .data = usdhc_imx6q_data, },
{ /* sentinel */ }
--
1.7.8
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: dw_mmc-exynos: fixed wrong sample-clock selection
Hi, Alim.
On 09/17/2014 09:49 PM, Alim Akhtar wrote:
Hi Jaehoon
On Wed, Sep 17, 2014 at 8:06 AM, Jaehoon Chung jh80.ch...@samsung.com wrote:
Hi, Alim.
On 09/17/2014 07:27 AM, Alim Akhtar wrote:
Hi Jaehoon,
On Mon, Sep 15, 2014 at 3:56 PM, Jaehoon Chung jh80.ch...@samsung.com
wrote:
Almost all case is selected to 0.
(It's not correct sample-clock value.)
Since it set to wrong value, HS200 mode don't work fine.
Can you please explain what problem you are facing here?
When i try to use HS200 with some eMMC card, Some card is working fine and
other isn't working fine.
(DWMMC is occured the DCRC error.)
DCRC error might be occurred when the wrong sample-clock is set.
So i tried to see what value is selected to sample-clock.
(Selected value - Working fine card : 0, Not working fine card : 0)
When i skip to 0, it's selected to other value for best sample-clock.
(Selected value - Working fine card : 4, Not working fine card : 6)
After working fine card is selected to 6, it's working fine.
I didn't know which value is set for other card. But in my case, best
sample-clock is selected to 0.
I am not if this is only reason for the failure on you board.
When I tested this patch on my exynos5800 based Pi board, HS200 does
not work at all. Even the eMMC card detection does not happening,
forget about getting a DCRC error.
Can you confirm what base kernel you are testing on? FYI, I am testing
on Ulf's -next branch, which has two recently merged patches specially
for HS200 on dw_mmc controller.
In case you are using the same branch as mine, then some more debug
log will be good for more analysis of the problem.
Thanks for pointing out. Well, i didn't test with other boards.
So your testing is helpful to me. I think it's not my board's problem.
Do you ensure that sample-clock is correct value?
I tested with 3.16-kerenl. Which value is selected at exynos5800 board?
I will test with kernel on Ulf's repository.
Best Regards,
Jaehoon Chung
I think it has the some problem for selecting the sample-clock.
Best Regards,
Jaehoon Chung
It is not clear from your patch description.
If we want to select the correct value, it has to check from 1 to 7.(skip
0)
Signed-off-by: Jaehoon Chung jh80.ch...@samsung.com
---
drivers/mmc/host/dw_mmc-exynos.c |9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/dw_mmc-exynos.c
b/drivers/mmc/host/dw_mmc-exynos.c
index 0fbc53a..5294035 100644
--- a/drivers/mmc/host/dw_mmc-exynos.c
+++ b/drivers/mmc/host/dw_mmc-exynos.c
@@ -25,6 +25,7 @@
#define NUM_PINS(x)(x + 2)
#define SDMMC_CLKSEL 0x09C
+#define SDMMC_CLKSEL_CCLK_SAMPLE_MASK 0x7
#define SDMMC_CLKSEL_CCLK_SAMPLE(x)(((x) 7) 0)
#define SDMMC_CLKSEL_CCLK_DRIVE(x) (((x) 7) 16)
#define SDMMC_CLKSEL_CCLK_DIVIDER(x) (((x) 7) 24)
@@ -330,6 +331,12 @@ static int dw_mci_exynos_execute_tuning(struct
dw_mci_slot *slot, u32 opcode,
if (!blk_test)
return -ENOMEM;
+ /*
+* In order to check all selclk_sample clock,
+* it needs to reset to 0.
+*/
+ dw_mci_exynos_set_clksmpl(host, 0);
+
start_smpl = dw_mci_exynos_get_clksmpl(host);
do {
@@ -372,7 +379,7 @@ static int dw_mci_exynos_execute_tuning(struct
dw_mci_slot *slot, u32 opcode,
Tuning error: cmd.error:%d,
data.error:%d\n,
cmd.error, data.error);
}
- } while (start_smpl != smpl);
+ } while (start_smpl SDMMC_CLKSEL_CCLK_SAMPLE_MASK);
found = dw_mci_exynos_get_best_clksmpl(candiates);
if (found = 0)
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: don't request CD IRQ until mmc_start_host()
On 09/17/2014 11:25 PM, Adrian Hunter wrote:
On 09/17/2014 10:57 PM, Stephen Warren wrote:
On 09/17/2014 01:55 PM, Ulf Hansson wrote:
On 12 September 2014 19:18, Stephen Warren swar...@wwwdotorg.org wrote:
From: Stephen Warren swar...@nvidia.com
As soon as the CD IRQ is requested, it can trigger, since it's an
externally controlled event. If it does, delayed_work host-detect will
be scheduled.
Many host controller probe()s are roughly structured as:
*_probe() {
host = sdhci_pltfm_init();
mmc_of_parse(host-mmc);
rc = sdhci_add_host(host);
if (rc) {
sdhci_pltfm_free();
return rc;
}
In 3.17, CD IRQs can are enabled quite early via *_probe() -
mmc_of_parse() - mmc_gpio_request_cd() - mmc_gpiod_request_cd_irq().
Note that in linux-next, mmc_of_parse() calls mmc_gpio*d*_request_cd()
rather than mmc_gpio_request_cd(), and mmc_gpio*d*_request_cd() doesn't
call mmc_gpiod_request_cd_irq(). However, this issue still exists for
any other direct users of mmc_gpio_request_cd().
sdhci_add_host() may fail part way through (e.g. due to deferred
probe for a vmmc regulator), and sdhci_pltfm_free() does nothing to
unrequest the CD IRQ nor cancel the delayed_work. sdhci_pltfm_free() is
coded to assume that if sdhci_add_host() failed, then the delayed_work
cannot (or should not) have been triggered.
This can lead to the following with CONFIG_DEBUG_OBJECTS_* enabled, when
kfree(host) is eventually called inside sdhci_pltfm_free():
WARNING: CPU: 2 PID: 6 at lib/debugobjects.c:263
debug_print_object+0x8c/0xb4()
ODEBUG: free active (active state 0) object type: timer_list hint:
delayed_work_timer_fn+0x0/0x18
The object being complained about is host-detect.
There's no need to request the CD IRQ so early; mmc_start_host() already
requests it, and I *assume* that mmc_start_host() is called somehow for
all host controllers. For SDHCI hosts at least, the typical call path
that does this is: *_probe() - sdhci_add_host() - mmc_add_host() -
mmc_start_host(). So, remove the call to mmc_gpiod_request_cd_irq() from
mmc_gpio_request_cd(). This matches mmc_gpio*d*_request_cd(), which
already doesn't call mmc_gpiod_request_cd_irq().
This solves the problem (eliminates the kernel error message above),
since it guarantees that the IRQ can't trigger before mmc_start_host()
is called.
The critical point here is that once sdhci_add_host() calls
mmc_add_host() - mmc_start_host(), sdhci_add_host() is coded not to
fail. In other words, if there's a chance that mmc_start_host() may have
been called, and CD IRQs triggered, and the delayed_work scheduled,
sdhci_add_host() won't fail, and so cleanup is no longer via
sdhci_pltfm_free() (which doesn't free the IRQ or cancel the work queue)
but instead must be via sdhci_remove_host(), which calls mmc_remove_host()
- mmc_stop_host(), which does free the IRQ and cancel the work queue.
This fixes what I might conclude to be a mistake in commit 740a221ef0e5
(mmc: slot-gpio: Add GPIO descriptor based CD GPIO API), which added the
call from mmc_start_host() to mmc_gpiod_request_cd_irq(), but also added
incorrectly added a call from mmc_gpio_request_cd() to
mmc_gpiod_request_cd_irq().
CC: Russell King li...@arm.linux.org.uk
Cc: Adrian Hunter adrian.hun...@intel.com
Cc: Alexandre Courbot acour...@nvidia.com
Cc: Linus Walleij linus.wall...@linaro.org
Signed-off-by: Stephen Warren swar...@nvidia.com
Hi Stephen,
Thanks for looking into this. It seems like this issue has been
present for quite a while.
I believe your patch should have a stable tag for 3.15+ as well,
unless you object I will add it.
Yes, that probably makes sense, thanks.
Doesn't this patch break the drivers that call mmc_gpio_request_cd() after
mmc_add_host() like mmc_spi.c or sdhci-sirf.c or tmio_mmc_pio.c ?
Oh, if there are drivers that do that, this patch might cause an issue.
But why are they doing that? Shouldn't all the drivers set up the same
kinds of resources in the same order and way?
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: don't request CD IRQ until mmc_start_host()
On 09/18/2014 12:49 AM, Adrian Hunter wrote:
On 09/18/2014 08:25 AM, Adrian Hunter wrote:
On 09/17/2014 10:57 PM, Stephen Warren wrote:
On 09/17/2014 01:55 PM, Ulf Hansson wrote:
On 12 September 2014 19:18, Stephen Warren swar...@wwwdotorg.org wrote:
From: Stephen Warren swar...@nvidia.com
As soon as the CD IRQ is requested, it can trigger, since it's an
externally controlled event. If it does, delayed_work host-detect will
be scheduled.
Many host controller probe()s are roughly structured as:
*_probe() {
host = sdhci_pltfm_init();
mmc_of_parse(host-mmc);
rc = sdhci_add_host(host);
if (rc) {
sdhci_pltfm_free();
return rc;
}
In 3.17, CD IRQs can are enabled quite early via *_probe() -
mmc_of_parse() - mmc_gpio_request_cd() - mmc_gpiod_request_cd_irq().
Note that in linux-next, mmc_of_parse() calls mmc_gpio*d*_request_cd()
rather than mmc_gpio_request_cd(), and mmc_gpio*d*_request_cd() doesn't
call mmc_gpiod_request_cd_irq(). However, this issue still exists for
any other direct users of mmc_gpio_request_cd().
sdhci_add_host() may fail part way through (e.g. due to deferred
probe for a vmmc regulator), and sdhci_pltfm_free() does nothing to
unrequest the CD IRQ nor cancel the delayed_work. sdhci_pltfm_free() is
coded to assume that if sdhci_add_host() failed, then the delayed_work
cannot (or should not) have been triggered.
This can lead to the following with CONFIG_DEBUG_OBJECTS_* enabled, when
kfree(host) is eventually called inside sdhci_pltfm_free():
WARNING: CPU: 2 PID: 6 at lib/debugobjects.c:263
debug_print_object+0x8c/0xb4()
ODEBUG: free active (active state 0) object type: timer_list hint:
delayed_work_timer_fn+0x0/0x18
The object being complained about is host-detect.
There's no need to request the CD IRQ so early; mmc_start_host() already
requests it, and I *assume* that mmc_start_host() is called somehow for
all host controllers. For SDHCI hosts at least, the typical call path
that does this is: *_probe() - sdhci_add_host() - mmc_add_host() -
mmc_start_host(). So, remove the call to mmc_gpiod_request_cd_irq() from
mmc_gpio_request_cd(). This matches mmc_gpio*d*_request_cd(), which
already doesn't call mmc_gpiod_request_cd_irq().
This solves the problem (eliminates the kernel error message above),
since it guarantees that the IRQ can't trigger before mmc_start_host()
is called.
The critical point here is that once sdhci_add_host() calls
mmc_add_host() - mmc_start_host(), sdhci_add_host() is coded not to
fail. In other words, if there's a chance that mmc_start_host() may have
been called, and CD IRQs triggered, and the delayed_work scheduled,
sdhci_add_host() won't fail, and so cleanup is no longer via
sdhci_pltfm_free() (which doesn't free the IRQ or cancel the work queue)
but instead must be via sdhci_remove_host(), which calls mmc_remove_host()
- mmc_stop_host(), which does free the IRQ and cancel the work queue.
This fixes what I might conclude to be a mistake in commit 740a221ef0e5
(mmc: slot-gpio: Add GPIO descriptor based CD GPIO API), which added the
call from mmc_start_host() to mmc_gpiod_request_cd_irq(), but also added
incorrectly added a call from mmc_gpio_request_cd() to
mmc_gpiod_request_cd_irq().
That comment is wrong. mmc_gpio_request_cd() has always set up the irq.
Uggh, yes. I did misinterpret your patch again, so that one paragraph is
just wrong.
Aside from that though, I do think my patch is a step in the correct
direction. It just needs some thought how to avoid the other issue you
mentioned - that some drivers rely on calling mmc_gpio_request_cd()
after the call to mmc_start().
Perhaps the logic should not be to remove mmc_gpio_request_cd()'s call
to mmc_gpiod_request_cd_irq(), but rather to make it conditional upon
mmc_start_host() having already been called; I assume that state that
can easily be checked to determine that.
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: don't request CD IRQ until mmc_start_host()
On Thu, Sep 18, 2014 at 10:39:38AM -0600, Stephen Warren wrote:
On 09/17/2014 11:25 PM, Adrian Hunter wrote:
On 09/17/2014 10:57 PM, Stephen Warren wrote:
On 09/17/2014 01:55 PM, Ulf Hansson wrote:
On 12 September 2014 19:18, Stephen Warren swar...@wwwdotorg.org wrote:
From: Stephen Warren swar...@nvidia.com
As soon as the CD IRQ is requested, it can trigger, since it's an
externally controlled event. If it does, delayed_work host-detect will
be scheduled.
Many host controller probe()s are roughly structured as:
*_probe() {
host = sdhci_pltfm_init();
mmc_of_parse(host-mmc);
rc = sdhci_add_host(host);
if (rc) {
sdhci_pltfm_free();
return rc;
}
In 3.17, CD IRQs can are enabled quite early via *_probe() -
mmc_of_parse() - mmc_gpio_request_cd() - mmc_gpiod_request_cd_irq().
Note that in linux-next, mmc_of_parse() calls mmc_gpio*d*_request_cd()
rather than mmc_gpio_request_cd(), and mmc_gpio*d*_request_cd() doesn't
call mmc_gpiod_request_cd_irq(). However, this issue still exists for
any other direct users of mmc_gpio_request_cd().
sdhci_add_host() may fail part way through (e.g. due to deferred
probe for a vmmc regulator), and sdhci_pltfm_free() does nothing to
unrequest the CD IRQ nor cancel the delayed_work. sdhci_pltfm_free() is
coded to assume that if sdhci_add_host() failed, then the delayed_work
cannot (or should not) have been triggered.
This can lead to the following with CONFIG_DEBUG_OBJECTS_* enabled, when
kfree(host) is eventually called inside sdhci_pltfm_free():
WARNING: CPU: 2 PID: 6 at lib/debugobjects.c:263
debug_print_object+0x8c/0xb4()
ODEBUG: free active (active state 0) object type: timer_list hint:
delayed_work_timer_fn+0x0/0x18
The object being complained about is host-detect.
There's no need to request the CD IRQ so early; mmc_start_host() already
requests it, and I *assume* that mmc_start_host() is called somehow for
all host controllers. For SDHCI hosts at least, the typical call path
that does this is: *_probe() - sdhci_add_host() - mmc_add_host() -
mmc_start_host(). So, remove the call to mmc_gpiod_request_cd_irq() from
mmc_gpio_request_cd(). This matches mmc_gpio*d*_request_cd(), which
already doesn't call mmc_gpiod_request_cd_irq().
This solves the problem (eliminates the kernel error message above),
since it guarantees that the IRQ can't trigger before mmc_start_host()
is called.
The critical point here is that once sdhci_add_host() calls
mmc_add_host() - mmc_start_host(), sdhci_add_host() is coded not to
fail. In other words, if there's a chance that mmc_start_host() may have
been called, and CD IRQs triggered, and the delayed_work scheduled,
sdhci_add_host() won't fail, and so cleanup is no longer via
sdhci_pltfm_free() (which doesn't free the IRQ or cancel the work queue)
but instead must be via sdhci_remove_host(), which calls mmc_remove_host()
- mmc_stop_host(), which does free the IRQ and cancel the work queue.
This fixes what I might conclude to be a mistake in commit 740a221ef0e5
(mmc: slot-gpio: Add GPIO descriptor based CD GPIO API), which added the
call from mmc_start_host() to mmc_gpiod_request_cd_irq(), but also added
incorrectly added a call from mmc_gpio_request_cd() to
mmc_gpiod_request_cd_irq().
CC: Russell King li...@arm.linux.org.uk
Cc: Adrian Hunter adrian.hun...@intel.com
Cc: Alexandre Courbot acour...@nvidia.com
Cc: Linus Walleij linus.wall...@linaro.org
Signed-off-by: Stephen Warren swar...@nvidia.com
Hi Stephen,
Thanks for looking into this. It seems like this issue has been
present for quite a while.
I believe your patch should have a stable tag for 3.15+ as well,
unless you object I will add it.
Yes, that probably makes sense, thanks.
Doesn't this patch break the drivers that call mmc_gpio_request_cd() after
mmc_add_host() like mmc_spi.c or sdhci-sirf.c or tmio_mmc_pio.c ?
Oh, if there are drivers that do that, this patch might cause an issue.
But why are they doing that? Shouldn't all the drivers set up the same
kinds of resources in the same order and way?
The way this /should/ work is that:
+ mmc_alloc_host() (and corresponding derivatives) should initialise
everything into a safe state.
+ mmc_add_host() (and corresponding derivatives) publishes the host,
and enables card discovery etc.
Host drivers should not do anything after mmc_add_host(). Yes, there's
buggy host drivers (particularly the sdhci crap - and even after my
mega patch set, the most friendly and positive term I have to describe
sdhci _is_ crap) which oops the kernel if you (eg) receive a card
detect IRQ between those two calls, but that's really because the
host driver _is_ crap and not following proper driver initialisation
rules.
Someone /really/ needs to sort out MMC and stop this kind of driver
variability poliferating. All drivers should be doing the same thing:
- allocate the host
- map the resources
- claim interrupts etc
[PATCH v4] tmio_mmc_pio: prevent endless loop in tmio_mmc_set_clock()
I spent a couple of days with the driver just hanging due to me forgetting to
specify the external crystal frequency, so that clk_get_rate() returned 0 and
thus the loop in tmio_mmc_set_clock() never ended. I don't think that's an
acceptable behavior, so I suggest that the minimum frequency is checked for 0
in tmio_mmc_host_probe().
Signed-off-by: Sergei Shtylyov sergei.shtyl...@cogentembedded.com
Acked-by: Ian Molton ian.mol...@codethink.co.uk
Cc: sta...@vger.kernel.org
---
The patch is against Ulf Hansson's 'mmc.git' repo's 'next' branch.
Changes in version 4:
- rebased to the 'next' branch, resolving reject;
- added Ian Molton's ACK.
Changes in version 3:
- added Cc: stable.
Changes in version 2:
- fixed grammar in the changelog.
drivers/mmc/host/tmio_mmc_pio.c |9 +
1 file changed, 9 insertions(+)
Index: mmc/drivers/mmc/host/tmio_mmc_pio.c
===
--- mmc.orig/drivers/mmc/host/tmio_mmc_pio.c
+++ mmc/drivers/mmc/host/tmio_mmc_pio.c
@@ -1091,6 +1091,15 @@ int tmio_mmc_host_probe(struct tmio_mmc_
}
/*
+* Check the sanity of mmc-f_min to prevent tmio_mmc_set_clock() from
+* looping forever...
+*/
+ if (mmc-f_min == 0) {
+ ret = -EINVAL;
+ goto host_free;
+ }
+
+ /*
* While using internal tmio hardware logic for card detection, we need
* to ensure it stays powered for it to work.
*/
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: rtsx: add card power off during probe
[...] In that case, don't forget to enable MMC_CAP2_FULL_PWR_CYCLE. if MMC_CAP2_NO_PRESCAN_POWERUP enable, will call mmc_power_off() at start, then it will check ios.power_mode, but the state is MMC_POWER_OFF and just return. Uhh, that's right! So, I wonder why we invokes mmc_power_off() from that path at all. Hmm, I think we should change the behavior in mmc_start_host(), like below: 1) Add a MMC_POWER_UNDEFINED state which is what the power state should be assigned to at allocation. 2 ) From mmc_start_host(), invoke mmc_power_off() when MMC_CAP2_NO_PRESCAN_POWERUP and MMC_CAP2_FULL_PWR_CYCLE is set. Would that work? Yes. I have confirmed this by following changes. The MMC_POWER_UNDEFINED designation in mmc_start_host() will eventually cause a power-off operation. But I wonder if we need to additionally check MMC_CAP2_FULL_PWR_CYCLE before calling mmc_power_off()? The intent from my side was to keep the current behaviour for those that already used MMC_CAP2_NO_PRESCAN_POWERUP, but it's s not a big deal. So, let's try your proposal, thus don't check MMC_CAP2_FULL_PWR_CYCLE. Can you repost new version of your patches and please split them up on core and host separately. Kind regards Uffe -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: don't request CD IRQ until mmc_start_host()
On 18 September 2014 08:49, Adrian Hunter adrian.hun...@intel.com wrote:
On 09/18/2014 08:25 AM, Adrian Hunter wrote:
On 09/17/2014 10:57 PM, Stephen Warren wrote:
On 09/17/2014 01:55 PM, Ulf Hansson wrote:
On 12 September 2014 19:18, Stephen Warren swar...@wwwdotorg.org wrote:
From: Stephen Warren swar...@nvidia.com
As soon as the CD IRQ is requested, it can trigger, since it's an
externally controlled event. If it does, delayed_work host-detect will
be scheduled.
Many host controller probe()s are roughly structured as:
*_probe() {
host = sdhci_pltfm_init();
mmc_of_parse(host-mmc);
rc = sdhci_add_host(host);
if (rc) {
sdhci_pltfm_free();
return rc;
}
In 3.17, CD IRQs can are enabled quite early via *_probe() -
mmc_of_parse() - mmc_gpio_request_cd() - mmc_gpiod_request_cd_irq().
Note that in linux-next, mmc_of_parse() calls mmc_gpio*d*_request_cd()
rather than mmc_gpio_request_cd(), and mmc_gpio*d*_request_cd() doesn't
call mmc_gpiod_request_cd_irq(). However, this issue still exists for
any other direct users of mmc_gpio_request_cd().
sdhci_add_host() may fail part way through (e.g. due to deferred
probe for a vmmc regulator), and sdhci_pltfm_free() does nothing to
unrequest the CD IRQ nor cancel the delayed_work. sdhci_pltfm_free() is
coded to assume that if sdhci_add_host() failed, then the delayed_work
cannot (or should not) have been triggered.
This can lead to the following with CONFIG_DEBUG_OBJECTS_* enabled, when
kfree(host) is eventually called inside sdhci_pltfm_free():
WARNING: CPU: 2 PID: 6 at lib/debugobjects.c:263
debug_print_object+0x8c/0xb4()
ODEBUG: free active (active state 0) object type: timer_list hint:
delayed_work_timer_fn+0x0/0x18
The object being complained about is host-detect.
There's no need to request the CD IRQ so early; mmc_start_host() already
requests it, and I *assume* that mmc_start_host() is called somehow for
all host controllers. For SDHCI hosts at least, the typical call path
that does this is: *_probe() - sdhci_add_host() - mmc_add_host() -
mmc_start_host(). So, remove the call to mmc_gpiod_request_cd_irq() from
mmc_gpio_request_cd(). This matches mmc_gpio*d*_request_cd(), which
already doesn't call mmc_gpiod_request_cd_irq().
This solves the problem (eliminates the kernel error message above),
since it guarantees that the IRQ can't trigger before mmc_start_host()
is called.
The critical point here is that once sdhci_add_host() calls
mmc_add_host() - mmc_start_host(), sdhci_add_host() is coded not to
fail. In other words, if there's a chance that mmc_start_host() may have
been called, and CD IRQs triggered, and the delayed_work scheduled,
sdhci_add_host() won't fail, and so cleanup is no longer via
sdhci_pltfm_free() (which doesn't free the IRQ or cancel the work queue)
but instead must be via sdhci_remove_host(), which calls mmc_remove_host()
- mmc_stop_host(), which does free the IRQ and cancel the work queue.
This fixes what I might conclude to be a mistake in commit 740a221ef0e5
(mmc: slot-gpio: Add GPIO descriptor based CD GPIO API), which added the
call from mmc_start_host() to mmc_gpiod_request_cd_irq(), but also added
incorrectly added a call from mmc_gpio_request_cd() to
mmc_gpiod_request_cd_irq().
That comment is wrong. mmc_gpio_request_cd() has always set up the irq.
CC: Russell King li...@arm.linux.org.uk
Cc: Adrian Hunter adrian.hun...@intel.com
Cc: Alexandre Courbot acour...@nvidia.com
Cc: Linus Walleij linus.wall...@linaro.org
Signed-off-by: Stephen Warren swar...@nvidia.com
Hi Stephen,
Thanks for looking into this. It seems like this issue has been
present for quite a while.
I believe your patch should have a stable tag for 3.15+ as well,
unless you object I will add it.
Yes, that probably makes sense, thanks.
Doesn't this patch break the drivers that call mmc_gpio_request_cd() after
mmc_add_host() like mmc_spi.c or sdhci-sirf.c or tmio_mmc_pio.c ?
Ulf, this should be reverted.
Okay, I have dropped it from my next branch now.
It seems like we need to walk through each an every driver's
behaviour, according to what Russell/Stephen also pointed out.
Kind regards
Uffe
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] mmc: tmio: enable odd number size access
Hi Ulf
ping ?
From: Kuninori Morimoto kuninori.morimoto...@renesas.com
Current tmio is using sd_ctrl_read16/write16_rep()
for data transfer.
It works if transfer size was even number,
but, last 1 byte will be ignored if
transfer size was odd number.
This patch adds new tmio_mmc_transfer_data()
and solve this issue.
Tested-by: Shinobu Uehara shinobu.uehara...@renesas.com
Signed-off-by: Kuninori Morimoto kuninori.morimoto...@renesas.com
---
v2 - v3
- remove cast
- remove extra variable
drivers/mmc/host/tmio_mmc_pio.c | 39
+++
1 file changed, 35 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index ff5ff0f..692e578 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -376,6 +376,40 @@ static int tmio_mmc_start_command(struct tmio_mmc_host
*host, struct mmc_command
return 0;
}
+static void tmio_mmc_transfer_data(struct tmio_mmc_host *host,
+unsigned short *buf,
+unsigned int count)
+{
+ int is_read = host-data-flags MMC_DATA_READ;
+ u8 *buf8;
+
+ /*
+ * Transfer the data
+ */
+ if (is_read)
+ sd_ctrl_read16_rep(host, CTL_SD_DATA_PORT, buf, count 1);
+ else
+ sd_ctrl_write16_rep(host, CTL_SD_DATA_PORT, buf, count 1);
+
+ /* if count was even number */
+ if (!(count 0x1))
+ return;
+
+ /* if count was odd number */
+ buf8 = (u8 *)(buf + (count 1));
+
+ /*
+ * FIXME
+ *
+ * driver and this function are assuming that
+ * it is used as little endian
+ */
+ if (is_read)
+ *buf8 = sd_ctrl_read16(host, CTL_SD_DATA_PORT) 0xff;
+ else
+ sd_ctrl_write16(host, CTL_SD_DATA_PORT, *buf8);
+}
+
/*
* This chip always returns (at least?) as much data as you ask for.
* I'm unsure what happens if you ask for less than a block. This should be
@@ -408,10 +442,7 @@ static void tmio_mmc_pio_irq(struct tmio_mmc_host *host)
count, host-sg_off, data-flags);
/* Transfer the data */
- if (data-flags MMC_DATA_READ)
- sd_ctrl_read16_rep(host, CTL_SD_DATA_PORT, buf, count 1);
- else
- sd_ctrl_write16_rep(host, CTL_SD_DATA_PORT, buf, count 1);
+ tmio_mmc_transfer_data(host, buf, count);
host-sg_off += count;
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
