* [PATCH] mmc: dw_mmc: minor cleanup for dw_mci_adjust_fifoth
From: Shawn Lin @ 2016-09-21 2:40 UTC (permalink / raw)
To: Jaehoon Chung, Ulf Hansson
Cc: linux-mmc, linux-kernel, linux-rockchip, Shawn Lin
msize and rx_wmark are properly initialized, we dont't
need to assign them again.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/host/dw_mmc.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 22dacae..d838428 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -876,11 +876,8 @@ static void dw_mci_adjust_fifoth(struct dw_mci *host, struct mmc_data *data)
* MSIZE is '1',
* if blksz is not a multiple of the FIFO width
*/
- if (blksz % fifo_width) {
- msize = 0;
- rx_wmark = 1;
+ if (blksz % fifo_width)
goto done;
- }
do {
if (!((blksz_depth % mszs[idx]) ||
--
2.3.7
^ permalink raw reply related
* [PATCH 5/5] mmc: sdhci-of-arasan: add sdhci_arasan_voltage_switch for arasan,5.1
From: Shawn Lin @ 2016-09-21 1:45 UTC (permalink / raw)
To: Ulf Hansson
Cc: Adrian Hunter, Jaehoon Chung, linux-mmc, linux-kernel,
linux-rockchip, Shawn Lin
In-Reply-To: <1474422233-29355-1-git-send-email-shawn.lin@rock-chips.com>
Per the vendor's requirement, we shouldn't do any setting for
1.8V Signaling Enable, otherwise the interaction/behaviour between
phy and controller will be undefined. Mostly it works fine if we do
that, but we still see failures. Anyway, let's fix it to meet the
vendor's requirement. The error log looks like:
[ 93.405085] mmc1: unexpected status 0x800900 after switch
[ 93.408474] mmc1: switch to bus width 1 failed
[ 93.408482] mmc1: mmc_select_hs200 failed, error -110
[ 93.408492] mmc1: error -110 during resume (card was removed?)
[ 93.408705] PM: resume of devices complete after 213.453 msecs
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/host/sdhci-of-arasan.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index 33601a8..9162495 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -245,6 +245,28 @@ static void sdhci_arasan_hs400_enhanced_strobe(struct mmc_host *mmc,
writel(vendor, host->ioaddr + SDHCI_ARASAN_VENDOR_REGISTER);
}
+static int sdhci_arasan_voltage_switch(struct mmc_host *mmc,
+ struct mmc_ios *ios)
+{
+ switch (ios->signal_voltage) {
+ case MMC_SIGNAL_VOLTAGE_180:
+ /*
+ * Plese don't switch to 1V8 as arasan,5.1 doesn't
+ * actually refer to this setting to indicate the
+ * signal voltage and the state machine will be broken
+ * actually if we force to enable 1V8. That's something
+ * like broken quirk but we could work around here.
+ */
+ return 0;
+ case MMC_SIGNAL_VOLTAGE_330:
+ case MMC_SIGNAL_VOLTAGE_120:
+ /* We don't support 3V3 and 1V2 */
+ break;
+ }
+
+ return -EINVAL;
+}
+
static struct sdhci_ops sdhci_arasan_ops = {
.set_clock = sdhci_arasan_set_clock,
.get_max_clock = sdhci_pltfm_clk_get_max_clock,
@@ -636,6 +658,8 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
host->mmc_host_ops.hs400_enhanced_strobe =
sdhci_arasan_hs400_enhanced_strobe;
+ host->mmc_host_ops.start_signal_voltage_switch =
+ sdhci_arasan_voltage_switch;
}
ret = sdhci_add_host(host);
--
2.3.7
^ permalink raw reply related
* [PATCH 4/5] mmc: sdhci: Don't try to switch to unsupported voltage
From: Shawn Lin @ 2016-09-21 1:43 UTC (permalink / raw)
To: Ulf Hansson
Cc: Adrian Hunter, Jaehoon Chung, linux-mmc, linux-kernel,
linux-rockchip, Ziyuan Xu, Shawn Lin
In-Reply-To: <1474422233-29355-1-git-send-email-shawn.lin@rock-chips.com>
From: Ziyuan Xu <xzy.xu@rock-chips.com>
Sdhci shouldn't switch to the unsupported voltage if claiming
that it can not support the requested voltage. Let's fix it.
Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/host/sdhci.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 4805566..b1f1edd 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1845,7 +1845,8 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
switch (ios->signal_voltage) {
case MMC_SIGNAL_VOLTAGE_330:
- if (!(host->flags & SDHCI_SIGNALING_330))
+ if (!(host->flags & SDHCI_SIGNALING_330) ||
+ !(host->caps & SDHCI_CAN_VDD_330))
return -EINVAL;
/* Set 1.8V Signal Enable in the Host Control2 register to 0 */
ctrl &= ~SDHCI_CTRL_VDD_180;
@@ -1872,7 +1873,8 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
return -EAGAIN;
case MMC_SIGNAL_VOLTAGE_180:
- if (!(host->flags & SDHCI_SIGNALING_180))
+ if (!(host->flags & SDHCI_SIGNALING_180) ||
+ !(host->caps & SDHCI_CAN_VDD_180))
return -EINVAL;
if (!IS_ERR(mmc->supply.vqmmc)) {
ret = mmc_regulator_set_vqmmc(mmc, ios);
--
2.3.7
^ permalink raw reply related
* [PATCH 3/5] mmc: core: changes frequency to hs_max_dtr when selecting hs400es
From: Shawn Lin @ 2016-09-21 1:43 UTC (permalink / raw)
To: Ulf Hansson
Cc: Adrian Hunter, Jaehoon Chung, linux-mmc, linux-kernel,
linux-rockchip, Shawn Lin
In-Reply-To: <1474422233-29355-1-git-send-email-shawn.lin@rock-chips.com>
Per JESD84-B51 P69, Host need to change frequency to <=52MHz after
setting HS_TIMING to 0x1, and host may changes frequency to <= 200MHz
after setting HS_TIMING to 0x3. It seems there is no difference if
we don't change frequency to <= 52MHz as f_init is already less than
52MHz. But actually it does make difference. When doing compatibility
test we see failures for some eMMC devices without changing the
frequency to hs_max_dtr. And let's read the spec again, we could see
that "Host may changes frequency to 200MHz" implies that it's not
mandatory. But the "Host need to change frequency to <= 52MHz" implies
that we should do this.
Reported-by: Xiao Yao <xiaoyao@rock-chips.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/core/mmc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 3163bb9..989d37e 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1282,6 +1282,8 @@ static int mmc_select_hs400es(struct mmc_card *card)
if (err)
goto out_err;
+ mmc_set_clock(host, card->ext_csd.hs_max_dtr);
+
err = mmc_switch_status(card);
if (err)
goto out_err;
--
2.3.7
^ permalink raw reply related
* [PATCH 2/5] mmc: core: switch to 1V8 or 1V2 for hs400es mode
From: Shawn Lin @ 2016-09-21 1:43 UTC (permalink / raw)
To: Ulf Hansson
Cc: Adrian Hunter, Jaehoon Chung, linux-mmc, linux-kernel,
linux-rockchip, Shawn Lin, stable, 4.4#, +
In-Reply-To: <1474422233-29355-1-git-send-email-shawn.lin@rock-chips.com>
When introducing hs400es, I didn't notice that we haven't
switched voltage to 1V2 or 1V8 for it. That happens to work
as the first controller claiming to support hs400es, arasan(5.1),
which is designed to only support 1V8. So the voltage is fixed to 1V8.
But it actually is wrong, and will not fit for other host controllers.
Let's fix it.
Fixes: commit 81ac2af65793ecf ("mmc: core: implement enhanced strobe support")
Cc: <stable@vger.kernel.org> 4.4# +
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/core/mmc.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 3486bc7..3163bb9 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1263,6 +1263,16 @@ static int mmc_select_hs400es(struct mmc_card *card)
goto out_err;
}
+ if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200_1_2V)
+ err = __mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120);
+
+ if (err && card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200_1_8V)
+ err = __mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180);
+
+ /* If fails try again during next card power cycle */
+ if (err)
+ goto out_err;
+
err = mmc_select_bus_width(card);
if (err < 0)
goto out_err;
--
2.3.7
^ permalink raw reply related
* [PATCH 1/5] mmc: core: don't try to switch block size for dual rate mode
From: Shawn Lin @ 2016-09-21 1:43 UTC (permalink / raw)
To: Ulf Hansson
Cc: Adrian Hunter, Jaehoon Chung, linux-mmc, linux-kernel,
linux-rockchip, Ziyuan Xu, Shawn Lin
In-Reply-To: <1474422233-29355-1-git-send-email-shawn.lin@rock-chips.com>
From: Ziyuan Xu <xzy.xu@rock-chips.com>
Per spec, block size should always be 512 bytes for dual rate mode,
so any attempts to switch the block size under dual rate mode should
be neglected.
Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/core/core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index f0ed0af..2553d90 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2576,7 +2576,8 @@ int mmc_set_blocklen(struct mmc_card *card, unsigned int blocklen)
{
struct mmc_command cmd = {0};
- if (mmc_card_blockaddr(card) || mmc_card_ddr52(card))
+ if (mmc_card_blockaddr(card) || mmc_card_ddr52(card) ||
+ mmc_card_hs400(card) || mmc_card_hs400es(card))
return 0;
cmd.opcode = MMC_SET_BLOCKLEN;
--
2.3.7
^ permalink raw reply related
* [PATCH 0/5] Some fixes for mmc core and sdhci/arasan
From: Shawn Lin @ 2016-09-21 1:43 UTC (permalink / raw)
To: Ulf Hansson
Cc: Adrian Hunter, Jaehoon Chung, linux-mmc, linux-kernel,
linux-rockchip, Shawn Lin
patch 1 is gonna fix the longstanding issues which may fail the
mmc_test. Per spec, block size is always 512 bytes for dual rate mode.
Any attempt to change it will be treated as illegal one.So we
need to check hs400(es) and DDR50 as well.
patch 2 and 3 actually fix my previous introduction for hs400es.
That happened to work for the controller and devices I was using
for test at that time. I believe Doug and Jaehoon didn't see failure
when testing it as well. After several numbers of compatibility test
and stability test for mass production, we are sure that patch 3 is
needed.
patch 4 also fix a longtanding issue of sdhci which try to switch
voltage without checking the cap. We found this issue as arasan,5.1
doesn't support VDD_330 but we still find sdhci try to switch to
3v3.
patch 5 will nak my patch[0]. It more or less looks like a hack from
Jaehoon's point. I think I find the root cause after numerous repro and
debug work with my ASIC team guys.
Thanks for Ziyuan Xu and Xiao Yao who help me test my massive hack patch
and finally we found what the problem is.
[0]: https://patchwork.kernel.org/patch/9238663
Shawn Lin (3):
mmc: core: switch to 1V8 or 1V2 for hs400es mode
mmc: core: changes frequency to hs_max_dtr when selecting hs400es
mmc: sdhci-of-arasan: add sdhci_arasan_voltage_switch for arasan,5.1
Ziyuan Xu (2):
mmc: core: don't try to switch block size for dual rate mode
mmc: sdhci: Don't try to switch to unsupported voltage
drivers/mmc/core/core.c | 3 ++-
drivers/mmc/core/mmc.c | 12 ++++++++++++
drivers/mmc/host/sdhci-of-arasan.c | 24 ++++++++++++++++++++++++
drivers/mmc/host/sdhci.c | 6 ++++--
4 files changed, 42 insertions(+), 3 deletions(-)
--
2.3.7
^ permalink raw reply
* Re: [PATCH v2 2/4] mmc: dw_mmc: avoid race condition of cpu and IDMAC
From: Shawn Lin @ 2016-09-21 1:03 UTC (permalink / raw)
To: Jaehoon Chung
Cc: shawn.lin, Ulf Hansson, linux-mmc, linux-kernel, Doug Anderson,
Heiko Stuebner, linux-rockchip
In-Reply-To: <d3cd8993-7c5e-bdd4-5182-aec5fd0b040c@samsung.com>
在 2016/9/20 17:49, Jaehoon Chung 写道:
> Hi Shawn,
>
> On 09/20/2016 06:47 PM, Shawn Lin wrote:
>> Hi Jaehoon,
>>
>> Friendly ping... :)
>
> Thanks for reminding! :) I forgot your patch-set..sorry!
Ah, never mind, I just want to make sure if I still need
to update it.:)
>
> Best Regards,
> Jaehoon Chung
>>
>> On 2016/9/2 12:14, Shawn Lin wrote:
>>> We could see an obvious race condition by test that
>>> the former write operation by IDMAC aiming to clear
>>> OWN bit reach right after the later configuration of
>>> the same desc, which makes the IDMAC be in SUSPEND
>>> state as the OWN bit was cleared by the asynchronous
>>> write operation of IDMAC. The bug can be very easy
>>> reproduced on RK3288 or similar when we reduce the
>>> running rate of system buses and keep the CPU running
>>> faster. So as two separate masters, IDMAC and cpu
>>> write the same descriptor stored on the same address,
>>> and this should be protected by adding check of OWN
>>> bit before preparing new descriptors.
>>>
>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>
>>> ---
>>>
>>> Changes in v2:
>>> - fallback to PIO mode if failing to wait for OWN bit
>>> - cleanup polluted desc chain as the own bit error could
>>> occur on any place of the chain, so we need to clr the desc
>>> configured before that one. Use the exist function to reinit
>>> the desc chain. As this issue is really rare, so after applied,
>>> the fio/iozone stress test didn't show up performance regression.
>>>
>>> drivers/mmc/host/dw_mmc.c | 208 ++++++++++++++++++++++++++++------------------
>>> 1 file changed, 129 insertions(+), 79 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index 782b303..daa1c52 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -467,12 +467,87 @@ static void dw_mci_dmac_complete_dma(void *arg)
>>> }
>>> }
>>>
>>> -static inline void dw_mci_prepare_desc64(struct dw_mci *host,
>>> +static int dw_mci_idmac_init(struct dw_mci *host)
>>> +{
>>> + int i;
>>> +
>>> + if (host->dma_64bit_address == 1) {
>>> + struct idmac_desc_64addr *p;
>>> + /* Number of descriptors in the ring buffer */
>>> + host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc_64addr);
>>> +
>>> + /* Forward link the descriptor list */
>>> + for (i = 0, p = host->sg_cpu; i < host->ring_size - 1;
>>> + i++, p++) {
>>> + p->des6 = (host->sg_dma +
>>> + (sizeof(struct idmac_desc_64addr) *
>>> + (i + 1))) & 0xffffffff;
>>> +
>>> + p->des7 = (u64)(host->sg_dma +
>>> + (sizeof(struct idmac_desc_64addr) *
>>> + (i + 1))) >> 32;
>>> + /* Initialize reserved and buffer size fields to "0" */
>>> + p->des1 = 0;
>>> + p->des2 = 0;
>>> + p->des3 = 0;
>>> + }
>>> +
>>> + /* Set the last descriptor as the end-of-ring descriptor */
>>> + p->des6 = host->sg_dma & 0xffffffff;
>>> + p->des7 = (u64)host->sg_dma >> 32;
>>> + p->des0 = IDMAC_DES0_ER;
>>> +
>>> + } else {
>>> + struct idmac_desc *p;
>>> + /* Number of descriptors in the ring buffer */
>>> + host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc);
>>> +
>>> + /* Forward link the descriptor list */
>>> + for (i = 0, p = host->sg_cpu;
>>> + i < host->ring_size - 1;
>>> + i++, p++) {
>>> + p->des3 = cpu_to_le32(host->sg_dma +
>>> + (sizeof(struct idmac_desc) * (i + 1)));
>>> + p->des1 = 0;
>>> + }
>>> +
>>> + /* Set the last descriptor as the end-of-ring descriptor */
>>> + p->des3 = cpu_to_le32(host->sg_dma);
>>> + p->des0 = cpu_to_le32(IDMAC_DES0_ER);
>>> + }
>>> +
>>> + dw_mci_idmac_reset(host);
>>> +
>>> + if (host->dma_64bit_address == 1) {
>>> + /* Mask out interrupts - get Tx & Rx complete only */
>>> + mci_writel(host, IDSTS64, IDMAC_INT_CLR);
>>> + mci_writel(host, IDINTEN64, SDMMC_IDMAC_INT_NI |
>>> + SDMMC_IDMAC_INT_RI | SDMMC_IDMAC_INT_TI);
>>> +
>>> + /* Set the descriptor base address */
>>> + mci_writel(host, DBADDRL, host->sg_dma & 0xffffffff);
>>> + mci_writel(host, DBADDRU, (u64)host->sg_dma >> 32);
>>> +
>>> + } else {
>>> + /* Mask out interrupts - get Tx & Rx complete only */
>>> + mci_writel(host, IDSTS, IDMAC_INT_CLR);
>>> + mci_writel(host, IDINTEN, SDMMC_IDMAC_INT_NI |
>>> + SDMMC_IDMAC_INT_RI | SDMMC_IDMAC_INT_TI);
>>> +
>>> + /* Set the descriptor base address */
>>> + mci_writel(host, DBADDR, host->sg_dma);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static inline int dw_mci_prepare_desc64(struct dw_mci *host,
>>> struct mmc_data *data,
>>> unsigned int sg_len)
>>> {
>>> unsigned int desc_len;
>>> struct idmac_desc_64addr *desc_first, *desc_last, *desc;
>>> + unsigned long timeout;
>>> int i;
>>>
>>> desc_first = desc_last = desc = host->sg_cpu;
>>> @@ -489,6 +564,19 @@ static inline void dw_mci_prepare_desc64(struct dw_mci *host,
>>> length -= desc_len;
>>>
>>> /*
>>> + * Wait for the former clear OWN bit operation
>>> + * of IDMAC to make sure that this descriptor
>>> + * isn't still owned by IDMAC as IDMAC's write
>>> + * ops and CPU's read ops are asynchronous.
>>> + */
>>> + timeout = jiffies + msecs_to_jiffies(100);
>>> + while (readl(&desc->des0) & IDMAC_DES0_OWN) {
>>> + if (time_after(jiffies, timeout))
>>> + goto err_own_bit;
>>> + udelay(10);
>>> + }
>>> +
>>> + /*
>>> * Set the OWN bit and disable interrupts
>>> * for this descriptor
>>> */
>>> @@ -516,15 +604,24 @@ static inline void dw_mci_prepare_desc64(struct dw_mci *host,
>>> /* Set last descriptor */
>>> desc_last->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC);
>>> desc_last->des0 |= IDMAC_DES0_LD;
>>> +
>>> + return 0;
>>> +err_own_bit:
>>> + /* restore the descriptor chain as it's polluted */
>>> + dev_dbg(host->dev, "desciptor is still owned by IDMAC.\n");
>>> + memset(host->sg_cpu, 0, PAGE_SIZE);
>>> + dw_mci_idmac_init(host);
>>> + return -EINVAL;
>>> }
>>>
>>>
>>> -static inline void dw_mci_prepare_desc32(struct dw_mci *host,
>>> +static inline int dw_mci_prepare_desc32(struct dw_mci *host,
>>> struct mmc_data *data,
>>> unsigned int sg_len)
>>> {
>>> unsigned int desc_len;
>>> struct idmac_desc *desc_first, *desc_last, *desc;
>>> + unsigned long timeout;
>>> int i;
>>>
>>> desc_first = desc_last = desc = host->sg_cpu;
>>> @@ -541,6 +638,20 @@ static inline void dw_mci_prepare_desc32(struct dw_mci *host,
>>> length -= desc_len;
>>>
>>> /*
>>> + * Wait for the former clear OWN bit operation
>>> + * of IDMAC to make sure that this descriptor
>>> + * isn't still owned by IDMAC as IDMAC's write
>>> + * ops and CPU's read ops are asynchronous.
>>> + */
>>> + timeout = jiffies + msecs_to_jiffies(100);
>>> + while (readl(&desc->des0) &
>>> + cpu_to_le32(IDMAC_DES0_OWN)) {
>>> + if (time_after(jiffies, timeout))
>>> + goto err_own_bit;
>>> + udelay(10);
>>> + }
>>> +
>>> + /*
>>> * Set the OWN bit and disable interrupts
>>> * for this descriptor
>>> */
>>> @@ -569,16 +680,28 @@ static inline void dw_mci_prepare_desc32(struct dw_mci *host,
>>> desc_last->des0 &= cpu_to_le32(~(IDMAC_DES0_CH |
>>> IDMAC_DES0_DIC));
>>> desc_last->des0 |= cpu_to_le32(IDMAC_DES0_LD);
>>> +
>>> + return 0;
>>> +err_own_bit:
>>> + /* restore the descriptor chain as it's polluted */
>>> + dev_dbg(host->dev, "desciptor is still owned by IDMAC.\n");
>>> + memset(host->sg_cpu, 0, PAGE_SIZE);
>>> + dw_mci_idmac_init(host);
>>> + return -EINVAL;
>>> }
>>>
>>> static int dw_mci_idmac_start_dma(struct dw_mci *host, unsigned int sg_len)
>>> {
>>> u32 temp;
>>> + int ret;
>>>
>>> if (host->dma_64bit_address == 1)
>>> - dw_mci_prepare_desc64(host, host->data, sg_len);
>>> + ret = dw_mci_prepare_desc64(host, host->data, sg_len);
>>> else
>>> - dw_mci_prepare_desc32(host, host->data, sg_len);
>>> + ret = dw_mci_prepare_desc32(host, host->data, sg_len);
>>> +
>>> + if (ret)
>>> + goto out;
>>>
>>> /* drain writebuffer */
>>> wmb();
>>> @@ -603,81 +726,8 @@ static int dw_mci_idmac_start_dma(struct dw_mci *host, unsigned int sg_len)
>>> /* Start it running */
>>> mci_writel(host, PLDMND, 1);
>>>
>>> - return 0;
>>> -}
>>> -
>>> -static int dw_mci_idmac_init(struct dw_mci *host)
>>> -{
>>> - int i;
>>> -
>>> - if (host->dma_64bit_address == 1) {
>>> - struct idmac_desc_64addr *p;
>>> - /* Number of descriptors in the ring buffer */
>>> - host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc_64addr);
>>> -
>>> - /* Forward link the descriptor list */
>>> - for (i = 0, p = host->sg_cpu; i < host->ring_size - 1;
>>> - i++, p++) {
>>> - p->des6 = (host->sg_dma +
>>> - (sizeof(struct idmac_desc_64addr) *
>>> - (i + 1))) & 0xffffffff;
>>> -
>>> - p->des7 = (u64)(host->sg_dma +
>>> - (sizeof(struct idmac_desc_64addr) *
>>> - (i + 1))) >> 32;
>>> - /* Initialize reserved and buffer size fields to "0" */
>>> - p->des1 = 0;
>>> - p->des2 = 0;
>>> - p->des3 = 0;
>>> - }
>>> -
>>> - /* Set the last descriptor as the end-of-ring descriptor */
>>> - p->des6 = host->sg_dma & 0xffffffff;
>>> - p->des7 = (u64)host->sg_dma >> 32;
>>> - p->des0 = IDMAC_DES0_ER;
>>> -
>>> - } else {
>>> - struct idmac_desc *p;
>>> - /* Number of descriptors in the ring buffer */
>>> - host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc);
>>> -
>>> - /* Forward link the descriptor list */
>>> - for (i = 0, p = host->sg_cpu;
>>> - i < host->ring_size - 1;
>>> - i++, p++) {
>>> - p->des3 = cpu_to_le32(host->sg_dma +
>>> - (sizeof(struct idmac_desc) * (i + 1)));
>>> - p->des1 = 0;
>>> - }
>>> -
>>> - /* Set the last descriptor as the end-of-ring descriptor */
>>> - p->des3 = cpu_to_le32(host->sg_dma);
>>> - p->des0 = cpu_to_le32(IDMAC_DES0_ER);
>>> - }
>>> -
>>> - dw_mci_idmac_reset(host);
>>> -
>>> - if (host->dma_64bit_address == 1) {
>>> - /* Mask out interrupts - get Tx & Rx complete only */
>>> - mci_writel(host, IDSTS64, IDMAC_INT_CLR);
>>> - mci_writel(host, IDINTEN64, SDMMC_IDMAC_INT_NI |
>>> - SDMMC_IDMAC_INT_RI | SDMMC_IDMAC_INT_TI);
>>> -
>>> - /* Set the descriptor base address */
>>> - mci_writel(host, DBADDRL, host->sg_dma & 0xffffffff);
>>> - mci_writel(host, DBADDRU, (u64)host->sg_dma >> 32);
>>> -
>>> - } else {
>>> - /* Mask out interrupts - get Tx & Rx complete only */
>>> - mci_writel(host, IDSTS, IDMAC_INT_CLR);
>>> - mci_writel(host, IDINTEN, SDMMC_IDMAC_INT_NI |
>>> - SDMMC_IDMAC_INT_RI | SDMMC_IDMAC_INT_TI);
>>> -
>>> - /* Set the descriptor base address */
>>> - mci_writel(host, DBADDR, host->sg_dma);
>>> - }
>>> -
>>> - return 0;
>>> +out:
>>> + return ret;
>>> }
>>>
>>> static const struct dw_mci_dma_ops dw_mci_idmac_ops = {
>>>
>>
>>
>
>
>
>
--
Best Regards
Shawn Lin
^ permalink raw reply
* Re: [PATCH v9] mmc: OCTEON: Add host driver for OCTEON MMC controller.
From: Aaro Koskinen @ 2016-09-20 22:57 UTC (permalink / raw)
To: Steven J. Hill
Cc: linux-mips, linux-mmc, Ulf Hansson, Mark Rutland, David Daney
In-Reply-To: <836c0ca9-18f0-f6b5-bb79-8d0301d54154@cavium.com>
On Mon, Sep 19, 2016 at 03:24:30PM -0500, Steven J. Hill wrote:
> The OCTEON MMC controller is currently found on cn61XX and cn71XX
> devices. Device parameters are configured from device tree data.
> eMMC, MMC and SD devices are supported. Tested on Cavium CN7130.
>
> Signed-off-by: Steven J. Hill <steven.hill@cavium.com>
> Acked-by: David Daney <david.daney@cavium.com>
Acked-by: Aaro Koskinen <aaro.koskinen@iki.fi>
A.
^ permalink raw reply
* Re: xHCI problem? [was Re: Erratic USB device behavior and device loss]
From: Ritesh Raj Sarraf @ 2016-09-20 15:51 UTC (permalink / raw)
To: Alan Stern; +Cc: Ulf Hansson, Alex Dubov, USB list, linux-mmc
In-Reply-To: <Pine.LNX.4.44L0.1609201141591.1379-100000@iolanthe.rowland.org>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
On Tue, 2016-09-20 at 11:43 -0400, Alan Stern wrote:
> > Yes. But that'd also mean to write that value upon every suspend/resume
> cycle
> > because the rtsx usb driver still declares support for autosuspend.
> > Should that be dropped ?
>
> No, the value doesn't change across a suspend/resume cycle.
>
I just verified, and yes, you are right. The value doesn't change.
> > > I'm afraid that this won't prevent the device from disconnecting
> > > itself, though. This appears to be some sort of hardware bug that
> > > can't be fixed in software.
> >
> > And that'd mean that upon every reset, the driver will again enable
> autosuspend
> > for that driver.
>
> Yes, that's true. I'm curious to see if preventing autosuspends will
> get rid of the resets. My guess is that it won't.
No. We tried it in the beginning. And the resets were still seen.
Thanks.
- --
Ritesh Raj Sarraf
RESEARCHUT - http://www.researchut.com
"Necessity is the mother of invention."
-----BEGIN PGP SIGNATURE-----
iQIcBAEBCgAGBQJX4Vr/AAoJEKY6WKPy4XVpqSoP/jimRbblWfR56HM3RuK6jLTp
XHm2lJj6LopxC7BDPVF+SIMULlTyPh2hjbF2MVw9yNwsv1nruGQspzZ5VrBWlTsK
8Fs2sJif7Y8tWVFEMZczghrEoHN0KLe5vW4W6rX8xjjH5nL6ljtUeBDT6DyvD7yT
WymQWfObwp6VnjoR3nZ1SzB4DN/oGH10NaMjkk234mTkhU9Pl+UXFmesDdWn8Y64
3l5SpemMbNQaCaa/jyFQBJXu3+OTYVQafHjcl0bb3aRt4sHq5neS5zc/EIjz+Cpo
kqQwpQ6FslvSvamlwwB8mqDalPQZHeIvUNFMjlldpiAs8iCVeMHpolWI/CXCfo+1
BwVv8Kc1VnoMsjZ7uEUQJY9F1Q7YJ+4gFK6WSAhz7B9Na/0ztPJgq0tFYnVQgrwx
zUnLL7jPZZ4Wt8if9UayPtCUCdqHBSIfeoJ7+HMkC6FPt5GGCsrhtZX0u0Onop7F
Ka/VNgpMUNccgPvdqq3zYKyNIaAIUPf0jSyFbwxVXGbCLSZi8f4QmSw7k3BvkqNN
lR+pyqjKbImTpzqk0QT22SGT+4MeQgclbEUkpfA8PaPyb+9uLjgtZgp1ucTlMzOV
c3mXaTzRtSihagSW4hNyqYOINtBnvZp3n2fWDPgjJu+LWGOhpqY7P8mq8gFj77Fp
G49mKNuDiOkPWH3qHJgA
=bGWs
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: xHCI problem? [was Re: Erratic USB device behavior and device loss]
From: Alan Stern @ 2016-09-20 15:43 UTC (permalink / raw)
To: Ritesh Raj Sarraf; +Cc: Ulf Hansson, Alex Dubov, USB list, linux-mmc
In-Reply-To: <1474384626.21100.6.camel-7WuBAv+fczCJ8c2fQYRYNw@public.gmane.org>
On Tue, 20 Sep 2016, Ritesh Raj Sarraf wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
> Hello Alan,
>
> On Tue, 2016-09-20 at 10:16 -0400, Alan Stern wrote:
> > This is a lot better. No more I/O errors.
> >
> > We still have irregular suspends and resumes, but that's to be
> > expected. More worrying are the spontaneous disconnects. They don't
> > seem to be related to the suspend/resume activity.
> >
> > You can disable suspend for this device entirely by doing:
> >
> > echo on >/sys/bus/usb/devices/2-4/power/control
> >
>
> Yes. But that'd also mean to write that value upon every suspend/resume cycle
> because the rtsx usb driver still declares support for autosuspend.
> Should that be dropped ?
No, the value doesn't change across a suspend/resume cycle.
> > I'm afraid that this won't prevent the device from disconnecting
> > itself, though. This appears to be some sort of hardware bug that
> > can't be fixed in software.
>
> And that'd mean that upon every reset, the driver will again enable autosuspend
> for that driver.
Yes, that's true. I'm curious to see if preventing autosuspends will
get rid of the resets. My guess is that it won't.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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: xHCI problem? [was Re: Erratic USB device behavior and device loss]
From: Ritesh Raj Sarraf @ 2016-09-20 15:17 UTC (permalink / raw)
To: Alan Stern; +Cc: Ulf Hansson, Alex Dubov, USB list, linux-mmc
In-Reply-To: <Pine.LNX.4.44L0.1609201012290.1459-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Hello Alan,
On Tue, 2016-09-20 at 10:16 -0400, Alan Stern wrote:
> This is a lot better. No more I/O errors.
>
> We still have irregular suspends and resumes, but that's to be
> expected. More worrying are the spontaneous disconnects. They don't
> seem to be related to the suspend/resume activity.
>
> You can disable suspend for this device entirely by doing:
>
> echo on >/sys/bus/usb/devices/2-4/power/control
>
Yes. But that'd also mean to write that value upon every suspend/resume cycle
because the rtsx usb driver still declares support for autosuspend.
Should that be dropped ?
> I'm afraid that this won't prevent the device from disconnecting
> itself, though. This appears to be some sort of hardware bug that
> can't be fixed in software.
And that'd mean that upon every reset, the driver will again enable autosuspend
for that driver.
It is an upsetting state for this device but thank you, to all of you, for
helping debug this problem.
- --
RESEARCHUT - http://www.researchut.com
"Necessity is the mother of invention."
-----BEGIN PGP SIGNATURE-----
iQIcBAEBCgAGBQJX4VLyAAoJEKY6WKPy4XVprEgQAKdYcFIF1ICNGHkF+oyDe1VY
DqyxTa0vLPbXWsG6GaV4Jdwld+gVKiWIVNKxbLpboBHj/vj0bkzoPJ0z4xI+Oc8p
bu6T5Io6nAaegdGa6XvYIAXk1fIlXEehBFVM6OyzC1EUJAjgYhIDVlG8mqZKxqVp
GGsX1e5UFdS0vCqjYqSxI5IHrqsm1M4lXuwr8ia66qyuSfpg8trizLiWrdiEa7hv
hRtI81XxITvVJ4+2ernO6Y+RO/z6WQLs1SAhXvDEH3RlYh/RoEBpolqMIO8LVWMK
jd4GSsYmMKiG1eJJq3UYM+iPDANIIi4gdO0hf/24vNcsVa8eF5kcKT+bxufRaiB/
5TzZS0RARBdq1+N6bK/wF8lDL4bWy4Sl1mts/dXJaCOlOoLeQ/u/J55K58mDJb2O
gdvEvzD/S9NNeawL2ow4sxaM8EBpeyJtBTyVIbLJVFmetauVs+ClI0uLoNMW/N+u
qMDE8yhiey4ClXa0WmVZHN4qjfNAnW4OSMtrq8+TLa1yhVj/ONNBo8QkfjuKBHwE
ELDrX3/N9JsZo6ZX0CPNVhodvck8ZVKrk8w3jAlcqQ0FlJy5MMFoqGEvqJ1EIfNv
IQ5FKrI08RIA7yw6keTy3nybzyY3MhepLJWxiEVyKRCopiHk96DmEGJmOmor7VIQ
hjgbpCikJ7sXPcn0n97u
=DBja
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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: xHCI problem? [was Re: Erratic USB device behavior and device loss]
From: Alan Stern @ 2016-09-20 14:16 UTC (permalink / raw)
To: Ritesh Raj Sarraf; +Cc: Ulf Hansson, Alex Dubov, USB list, linux-mmc
In-Reply-To: <1474374977.12288.2.camel@researchut.com>
On Tue, 20 Sep 2016, Ritesh Raj Sarraf wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
> On Mon, 2016-09-19 at 13:48 -0400, Alan Stern wrote:
> >
> > This ought to help. Ritesh, please apply this patch on top of the
> > two earlier ones and let's see what happens.
> >
> > Alan Stern
> >
> >
>
> Please find the logs at the following links. On this boot, I did not see any
> kernel stack being printed.
>
> https://people.debian.org/~rrs/tmp/4.8.0-rc7ulf1alan2+.kern.log
> https://people.debian.org/~rrs/tmp/usb-4.8.0-rc7ulf1alan2+.log
This is a lot better. No more I/O errors.
We still have irregular suspends and resumes, but that's to be
expected. More worrying are the spontaneous disconnects. They don't
seem to be related to the suspend/resume activity.
You can disable suspend for this device entirely by doing:
echo on >/sys/bus/usb/devices/2-4/power/control
I'm afraid that this won't prevent the device from disconnecting
itself, though. This appears to be some sort of hardware bug that
can't be fixed in software.
Alan Stern
^ permalink raw reply
* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
From: Alan Stern @ 2016-09-20 14:12 UTC (permalink / raw)
To: Oliver Neukum
Cc: Ulf Hansson, Micky Ching, Wei WANG, Roger Tseng,
Ritesh Raj Sarraf, linux-mmc, USB list
In-Reply-To: <1474364420.4358.21.camel@suse.com>
On Tue, 20 Sep 2016, Oliver Neukum wrote:
> On Mon, 2016-09-19 at 14:02 -0400, Alan Stern wrote:
> > > We can for sure enable autosuspend for the sdmmc device, although as
> > > soon as an SD card will be detected the rtsx driver will increase
> > the
> > > runtime PM usage count. The count is decreased when the card is
> > > removed (or failed to be initialized), thus runtime suspend is
> > > prevented as long as there is a functional card inserted.
>
> Testing autosuspend with card readers on usb_storage I saw a uniform
> response of reporting a medium change event upon resume.
> I am afraid other kinds of readers are not better in that regard.
That shouldn't be an issue in this case, at least, not with the current
code. The sdmmc and memstick drivers block autosuspend if media is
present.
> > Which means that autosuspend matters only when a card isn't present,
> > and the host is polled every second or so to see whether a card has
> > been inserted.
> >
> > Under those circumstances you probably don't want to use
> > autosuspend.
> > That is, resuming before each poll and suspending afterward may use
> > less energy than staying at full power all the time.
>
> Is that based on concrete figures about power consumption?
No.
> And it seems to me that we need a way to indicate that the heuristics
> should not be used, but a device immediately suspended. The timer
> is sensible only if the next wakeup is unknown.
The driver can always turn off autosuspend if it wants to.
Alan Stern
^ permalink raw reply
* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
From: Alan Stern @ 2016-09-20 14:09 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Ritesh Raj Sarraf, USB list, Micky Ching, Roger Tseng,
Wei WANG
In-Reply-To: <CAPDyKFq2Y1vn1OjNJJg7hocbzFo-QUpezLSMWnF_1cSJ9Ot3NQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, 20 Sep 2016, Ulf Hansson wrote:
> >> I am wondering what you think would be a good autosuspend timeout in
> >> this case? It seems to me that the only thing the rtsx driver really
> >> care about is to tell the parent device that it needs to be runtime
> >> resumed during a certain timeframe, more or less it would like to
> >> inherit the parents settings.
> >>
> >> Other mmc hosts, not being usb-mmc devices, are using an autosuspend
> >> timeout of ~50-100ms but that doesn't seem like good value here,
> >> right?
> >
> > Well, if you decide to let the device go into runtime suspend between
> > polls then there's no reason to use autosuspend at all. Once a poll
> > has ended, you know there won't be any more activity until the next
> > poll.
>
> We could change that, as currently the approach in the mmc core isn't
> that sophisticated. I even think this has been discussed earlier for
> the very similar reasons regards polling card detect mode.
>
> I guess the main reason to why we yet have changed this, is because
> mmc host drivers are using an autosuspend timeout of ~50-100 ms, so in
> the end it haven't been such a big deal.
No, it isn't. Although at a polling interval of 1 second, it means
reducing the low-power time by as much as 10%.
> > On the other hand, if you decide to keep the device at full power all
> > the time during polling, then any autosuspend timeout larger than 1000
> > ms would do what you want.
> >
> > Mostly I'm concerned about how this will interact with the USB runtime
> > PM. The thing is, suspending the sdmmc device doesn't save any energy,
> > whereas suspending the USB device does.
>
> Yes, I agree.
>
> My concern is also 2s autosuspend timeout which is set for the usb
> device. Somehow I feel we need to be able "share" more information
> between a parent-child relationship, in this case between the sdmmc
> device and the usb device.
I agree, but it's not clear how this should be done. One easy solution
would be to turn off USB autosuspend and do all the runtime-PM
management in the sdmmc and memstick drivers.
> An observation I made, is when the sdmmc device gets runtime resumed
> (pm_runtime_get_sync()), the parent device (the usb device) may also
> become runtime resumed (unless it's already). In this sequence, but
> *only* when actually runtime resuming the usb device, the runtime PM
> core decides to update the last busy mark for the usb device. Should
> it really do that?
Yes, that's deliberate. The whole idea of autosuspend is to prevent
the device from being runtime-suspended too soon after it was used, and
the PM core considers runtime-resume to be a form of usage.
> Moreover, I am curious about the 2s usb timeout. Why isn't that chosen
> to something like ~100ms instead? Is there is a long latency to
> runtime resume the usb device or because we fear to wear out the HW,
> which may be powered on/off too frequently?
When I first implemented runtime PM for the USB stack, I had to choose
a autosuspend timeout. Not having any basis for such a choice, and
figuring that a suspend-resume cycle takes around 100 ms, I decided
that 2 seconds would be a reasonable value. But it's just a default;
drivers and userspace can change it whenever they want.
> If we assume that the usb device shouldn't be used with a timeout less
> than 2s, then I think we have two options:
>
> *) As the mmc polling timeout is 1s, there is really no point in
> trying to runtime suspend the usb device, it may just be left runtime
> resumed all the time. Wasting power, of course!
Or we can decrease the USB autosuspend delay to 100 ms.
> **) Add an interface to allow dynamically changes of the mmc polling
> timeout to better suit the current user.
Note that the block layer does its own polling for removable media, and
it already has a sysfs interface to control the polling interval (or
disable it entirely). But I don't know how the MMC stack interacts
with the block layer.
One awkward point is that the sdmmc and memstick drivers each do their
own polling. This is a waste. You can see it in the usbmon trace;
every second there are two query-response interactions. Even if
there's no good way to reduce the number, we should at least try to
synchronize the polls so that the device doesn't need to be resumed
twice every second.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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: xHCI problem? [was Re: Erratic USB device behavior and device loss]
From: Ritesh Raj Sarraf @ 2016-09-20 12:36 UTC (permalink / raw)
To: Alan Stern, Ulf Hansson, Alex Dubov; +Cc: USB list, linux-mmc
In-Reply-To: <Pine.LNX.4.44L0.1609191340320.1458-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
On Mon, 2016-09-19 at 13:48 -0400, Alan Stern wrote:
>
> This ought to help. Ritesh, please apply this patch on top of the
> two earlier ones and let's see what happens.
>
> Alan Stern
>
>
Please find the logs at the following links. On this boot, I did not see any
kernel stack being printed.
https://people.debian.org/~rrs/tmp/4.8.0-rc7ulf1alan2+.kern.log
https://people.debian.org/~rrs/tmp/usb-4.8.0-rc7ulf1alan2+.log
>
> Index: usb-4.x/drivers/memstick/host/rtsx_usb_ms.c
> ===================================================================
> --- usb-4.x.orig/drivers/memstick/host/rtsx_usb_ms.c
> +++ usb-4.x/drivers/memstick/host/rtsx_usb_ms.c
> @@ -681,6 +681,7 @@ static int rtsx_usb_detect_ms_card(void
> 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
> }
>
> poll_again:
> + pm_runtime_put(ms_dev(host));
> if (host->eject)
> break;
>
- --
Ritesh Raj Sarraf
RESEARCHUT - http://www.researchut.com
"Necessity is the mother of invention."
-----BEGIN PGP SIGNATURE-----
iQIcBAEBCgAGBQJX4S1BAAoJEKY6WKPy4XVprAUP/jPnuxUAZ+6qKiCVx6qB69d+
wHQDkFOxmlTwTh5GyMa+oxEqvi0shvOKZ/ef7Oz0NA9DSiLonFw4aqSzF7jBRbee
UTKIgnNxHmJC6pdMPXWo5HVLBn6qYtX4pJFX6g1MwmjEDa9pjYWK9p7QzHkrx1GB
Z3X7TcWYk3DJS04GbFO9pMDl0P1phLR2VtnfzQwqtgF/g2fy7USpft1bYIQLQzxb
oOSAEDnTCtpurdAfLWq8OVQbL3rrf+HD3InVtdCZa+lwNSNwNfUZWnKKkS1S1tq+
hgKxvGOTEGunhm6Px6iQUCE9yxsvfmDK2GBxc/a3Tqpcy5ndZv/5laKFhXTt27pa
OuGksYgHCf2vWGHFuYHJH6cQKxgdsnnE7yGwbC8zYnrCT9O3hcLPxbVbzJorWFU0
YMNKt7RYZXrNQss9J4ufkTSLvzbUqsiYJwWH27LbQ5zHC7b9/ebgnMW6JIb1x+2p
iuz6MERvyxVxorG3R260GWSz/5SM/VVnTqzlRUnMHcVAyUHNGPGoqLu5LkrmI2VT
Zwcikip9G3fE79786eKF50X7dp2kU2p+W2bBmcJEWpWV9Vz5PiQdibsiu3CQilKc
QGxrKLp0OSsUvtwb4ceD/RWu7F99F7VCu3f/ohYYS2iciux5sFky+27GfY0fEJ2u
ikPpuK6xNWWSDgaNVVHD
=Nq4a
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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: mmc: dw_mmc: log spamming
From: Tobias Jakobi @ 2016-09-20 12:09 UTC (permalink / raw)
To: Jaehoon Chung, sw0312.kim; +Cc: Ulf Hansson, linux-mmc@vger.kernel.org
In-Reply-To: <25a3a5a5-da21-dc71-45fd-d9b3f2a2b2bc@samsung.com>
Hey Jaehoon,
Jaehoon Chung wrote:
> Hi Tobias,
>
> On 09/19/2016 07:00 PM, Tobias Jakobi wrote:
>> Hey Jaehoon,
>>
>>
>> Jaehoon Chung wrote:
>>> Hi Tobias,
>>>
>>> On 09/19/2016 04:41 PM, Tobias Jakobi wrote:
>>>> Hello Jaehoon,
>>>>
>>>>
>>>> Jaehoon Chung wrote:
>>>>> Hi Tobias,
>>>>>
>>>>> CC'd mmc mailing.
>>>>>
>>>>> On 09/19/2016 10:03 AM, Seung-Woo Kim wrote:
>>>>>> Hello Jaehoon,
>>>>>>
>>>>>> On 2016년 09월 19일 09:32, Jaehoon Chung wrote:
>>>>>>> Hi Tobias,
>>>>>>>
>>>>>>> On 09/16/2016 02:29 AM, Tobias Jakobi wrote:
>>>>>>>> Hello everyone,
>>>>>>>>
>>>>>>>> I'm experiencing massive kernel log spamming by dw_mmc, the commit that
>>>>>>>> causes this is the following one.
>>>>>>>>
>>>>>>>> 65257a0deed5aee66b4e3708944f0be62a64cabc
>>>>>>>>
>>>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=65257a0deed5aee66b4e3708944f0be62a64cabc
>>>>>>>>
>>>>>>>> I've briefly checked the commit and I think the rationale behind
>>>>>>>> removing the check is incorrect. While MMC_CLKGATE was certainly
>>>>>>>> removed, runtime PM has "replaced" it (the commit
>>>>>>>> 9eadcc0581a8ccaf4c2378aa1c193fb164304f1d even mentions this).
>>>>>>>>
>>>>>>>> This is on an Exynos4412 board, kernel is 4.8-rc6. Nothing is connected
>>>>>>>> to the eMMC connector.
>>>>>>>
>>>>>>> Thanks for reporting this.
>>>>>>>
>>>>>>> Seung-Woo, Could you check your patch? I will also check this.
>>>>>
>>>>> Did you test after reverting this commit? or previous version is working fine?
>>>> yes, reverting the commit fixes the log spamming. I'm aware though that
>>>> the check is more of less broken (undefined behaviour because of
>>>> bit-shifting with large values), so I have also tried the following
>>>> thing: Just checking against 'div' instead of 'clock << div'. Anyway,
>>>> this also works for me.
>>>>
>>>>
>>>>> Which exynos4412 board do you use? I think it's related with "broken-cd".
>>>>> (If you can share which board and dts you are using, we can check in more detail.)
>>>> This is an Odroid-X2 board.
>>>>
>>>
>>> When i have checked on odroid-u3, it's reproduced very easy.
>> good to hear! I also worry that I'm the only one with these issues on
>> the X2. :)
>>
>> Anyway, do you seen any other message on the U3 that are related to the
>> eMMC. I'm asking because I have also experience some warning/error from
>> the regulator subsystem:
>>> Sep 15 17:19:56 chidori kernel: [ 4.976690] vddf_emmc_2.85V: voltage operation not allowed
>>> Sep 15 17:19:56 chidori kernel: [ 4.995417] mmc_host mmc1: Bus speed (slot 0) = 50000000Hz (slot req 400000Hz, actual 396825HZ div = 63)
>>
>> I'm sending you a patch shortly which fixes the warning, but I'm not
>> sure if this is the right approach. Maybe you can take a look?
>
> I didn't see this..what is vddf_emmc_2.85V? I can't find this anywhere.
sorry for the confusion, I have renamed some of the regulator labels in
my private tree.
vddf_emmc_2.85V is BUCK8, and called 'BUCK8_VDDQ_MMC4_2.8V' in the
vanilla kernel. It's the vqmmc regulator of the eMMC.
>>>>> As you mentioned, you didn't insert the eMMC card on board.
>>>>> Then it should be polling whether card is inserted/removed. (If broken-cd is set...)
>>>> I can check the callstack leading to dw_mci_setup_bus() I guess. Would
>>>> that should make it easier for you guys to understand where the issue
>>>> originates from?
>>>
>>> Not need to share the callstack. Because i understood what is problem.
>>> But checking 'div' instead of 'clock << div' should also have the unexpected behavior.
>> Why is that? If I understood the initial problem correctly, then
>> shifting with 'div' produces undefined behaviour since we don't know an
>> upper bound for it. That's not the case if we just compare old and new
>> divisor.
>
> This problem is because of "polling" method. ("broken-cd" property in device-tree)
> If can't find the eMMC card, mmc_rescan_try_freq() is running four times.
> Because it's looping with freqs array size in mmc_rescan().
> At that time, clock and current_speed is 400KHz..and if didn't find any cards, clock is set to 0.
> After HZ, mmc_rescan is running again..and repeat above behavior.
>
> I didn't consider about this case. (removable case and polling method.)
>
> I think we can fix the below code..Could you check the below?
I'll try to do a test this evening. I'll let you know the results then!
And thanks for looking into this!
With best wishes,
Tobias
> I have tested with exynos4412/exynos5422 boards.
> If there is no issue, i will send the patch. Or if there is a problem, let me know, plz.
>
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 22dacae..41306d1 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1112,11 +1112,12 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>
> div = (host->bus_hz != clock) ? DIV_ROUND_UP(div, 2) : 0;
>
> - dev_info(&slot->mmc->class_dev,
> - "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n",
> - slot->id, host->bus_hz, clock,
> - div ? ((host->bus_hz / div) >> 1) :
> - host->bus_hz, div);
> + if (clock != slot->__clk_old || force_clkinit)
> + dev_info(&slot->mmc->class_dev,
> + "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n",
> + slot->id, host->bus_hz, clock,
> + div ? ((host->bus_hz / div) >> 1) :
> + host->bus_hz, div);
>
> /* disable clock */
> mci_writel(host, CLKENA, 0);
> @@ -1139,6 +1140,9 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>
> /* inform CIU */
> mci_send_cmd(slot, sdmmc_cmd_bits, 0);
> +
> + /* keep the last clock value that requested from core */
> + slot->__clk_old = clock;
> }
>
> host->current_speed = clock;
> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> index 9e740bc..0f12e15 100644
> --- a/drivers/mmc/host/dw_mmc.h
> +++ b/drivers/mmc/host/dw_mmc.h
> @@ -249,6 +249,8 @@ extern int dw_mci_resume(struct dw_mci *host);
> * @queue_node: List node for placing this node in the @queue list of
> * &struct dw_mci.
> * @clock: Clock rate configured by set_ios(). Protected by host->lock.
> + * @__clk_old: The last clock value that requested from core.
> + * Keeping track of this helps us to avoid spamming the console.
> * @flags: Random state bits associated with the slot.
> * @id: Number of this slot.
> * @sdio_id: Number of this slot in the SDIO interrupt registers.
> @@ -263,6 +265,7 @@ struct dw_mci_slot {
> struct list_head queue_node;
>
> unsigned int clock;
> + unsigned int __clk_old;
>
> unsigned long flags;
> #define DW_MMC_CARD_PRESENT 0
>
>
> Best Regards,
> Jaehoon Chung
>
>>
>>
>>> If needs to fix, i want to go ahead the correct way at this time.
>>> Anyway, thanks for reporting this!
>>
>> Also thanks, and let me know if I can test anything else.
>>
>> With best wishes,
>> Tobias
>>
>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>>
>>>>
>>>>> I think it's not related with runtime PM.
>>>>>
>>>>> Best Regards,
>>>>> Jaehoon Chung
>>>>
>>>> WIth best wishes,
>>>> Tobias
>>>>
>>>>
>>>>
>>>>
>>>>>> Ok, I will check on Exynos4412 SpC boards.
>>>>>>
>>>>>> By the way, to check no condition case, when I posted after v2[1], I
>>>>>> checked with Exynos5422 and Exynos5433 SoC boards and they didn't show
>>>>>> duplicated log, so I agreed not to check condition for logging.
>>>>>>
>>>>>> [1] https://patchwork.kernel.org/patch/9182469/
>>>>>>
>>>>>> Best Regards,
>>>>>> - Seung-Woo Kim
>>>>>>
>>>>>>>
>>>>>>> Best Regards,
>>>>>>> Jaehoon Chung
>>>>>>>
>>>>>>>>
>>>>>>>> With best wishes,
>>>>>>>> Tobias
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>>
>
^ permalink raw reply
* RE: [PATCH v6 0/9] Replay Protected Memory Block (RPMB) subsystem
From: Winkler, Tomas @ 2016-09-20 11:58 UTC (permalink / raw)
To: Greg Kroah-Hartman, alan@linux.intel.com, Hunter, Adrian
Cc: Ulf Hansson, James Bottomley, Martin K . Petersen,
Vinayak Holikatti, Andy Lutomirski, Arve Hj?nnev?g,
Michael Ryleev, Joao Pinto, Christoph Hellwig, Yaniv Gardi,
Avri Altman, linux-kernel@vger.kernel.org,
linux-mmc@vger.kernel.org, linux-scsi@vger.kernel.org
In-Reply-To: <20160920111153.GA17297@kroah.com>
>
> On Mon, Sep 19, 2016 at 12:17:48PM +0000, Winkler, Tomas wrote:
> > \
> > > Subject: [PATCH v6 0/9] Replay Protected Memory Block (RPMB)
> > > subsystem
> > >
> > >
> > > Few storage technologies such is EMMC, UFS, and NVMe support RPMB
> > > hardware partition with common protocol and frame layout.
> > > The RPMB partition cannot be accessed via standard block layer, but
> > > by a set of specific commands: WRITE, READ, GET_WRITE_COUNTER, and
> > > PROGRAM_KEY.
> > > Such a partition provides authenticated and replay protected access,
> > > hence suitable as a secure storage.
> > >
> > > The RPMB layer aims to provide in-kernel API for Trusted Execution
> > > Environment (TEE) devices that are capable to securely compute block
> > > frame signature. In case a TEE device wish to store a replay
> > > protected data, it creates an RPMB frame with requested data and
> > > computes HMAC of the frame, then it requests the storage device via
> RPMB layer to store the data.
> > >
> > > The layer provides two APIs, for rpmb_req_cmd() for issuing one of
> > > RPMB specific commands and rpmb_seq_cmd() for issuing of raw RPMB
> > > protocol frames, which is close to the functionality provided by
> > > emmc multi ioctl interface.
> > >
> > > A TEE driver can claim the RPMB interface, for example, via
> > > class_interface_register ().
> > >
> > > A storage device registers its RPMB hardware (eMMC) partition or
> > > RPMB W- LUN (UFS) with the RPMB layer providing an implementation
> > > for
> > > rpmb_seq_cmd() handler. The interface enables sending sequence of
> > > RPMB standard frames.
> > >
> > > A parallel user space API is provided via /dev/rpmbX character
> > > device with two IOCTL commands.
> > > Simplified one, RPMB_IOC_REQ_CMD, were read result cycles is
> > > performed by the framework on behalf the user and second,
> > > RPMB_IOC_SEQ_CMD where the whole RPMB sequence, including
> > > RESULT_READ is supplied by the caller.
> > > The latter is intended for easier adjusting of the applications that
> > > use MMC_IOC_MULTI_CMD ioctl, such as
> > > https://android.googlesource.com/trusty/app/storage/
> > >
> > > There is a also sample tool under tools/rpmb/ directory that
> > > exercises these interfaces and a simulation device that implements the
> device part.
> > >
> > > The code is also available from:
> > >
> > > https://github.com/tomasbw/linux-mei.git rpmb
> > >
> >
> > Greg, can you please check if this series has addressed all your comments.
> > Are there are any more items that preventing it from merging?
>
> Ugh, my queue is huge right now, give me a week or so to dig out of it and
> review this...
>
> Oh wait, you have almost no reviews from anyone else! Why is it up to me to
> do all of this work? :)
There were reviews I've addressed those as well, this is already 6th version of this series.
> Please get acks from others, at the very least, get it reviewed by other Intel
> kernel developers that we know and trust. I'm amazed you haven't already
> done that!
Adrian Hunter who is relevant from Intel is on the list. Also I've addressed his comments in v5. Adding Alan as he expressed some interest this code lately.
The code was of course reviewed and tested internally.
But this is indeed a good place to ask you the guys that addressed me privately or on the mailing list for additional review and yours X-by?
Thanks
Tomas
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH v6 0/9] Replay Protected Memory Block (RPMB) subsystem
From: Greg Kroah-Hartman @ 2016-09-20 11:11 UTC (permalink / raw)
To: Winkler, Tomas
Cc: Ulf Hansson, Hunter, Adrian, James Bottomley, Martin K . Petersen,
Vinayak Holikatti, Andy Lutomirski, Arve Hj?nnev?g,
Michael Ryleev, Joao Pinto, Christoph Hellwig, Yaniv Gardi,
Avri Altman, linux-kernel@vger.kernel.org,
linux-mmc@vger.kernel.org, linux-scsi@vger.kernel.org
In-Reply-To: <5B8DA87D05A7694D9FA63FD143655C1B542EB180@hasmsx108.ger.corp.intel.com>
On Mon, Sep 19, 2016 at 12:17:48PM +0000, Winkler, Tomas wrote:
> \
> > Subject: [PATCH v6 0/9] Replay Protected Memory Block (RPMB) subsystem
> >
> >
> > Few storage technologies such is EMMC, UFS, and NVMe support RPMB
> > hardware partition with common protocol and frame layout.
> > The RPMB partition cannot be accessed via standard block layer, but by a set
> > of specific commands: WRITE, READ, GET_WRITE_COUNTER, and
> > PROGRAM_KEY.
> > Such a partition provides authenticated and replay protected access, hence
> > suitable as a secure storage.
> >
> > The RPMB layer aims to provide in-kernel API for Trusted Execution
> > Environment (TEE) devices that are capable to securely compute block frame
> > signature. In case a TEE device wish to store a replay protected data, it
> > creates an RPMB frame with requested data and computes HMAC of the
> > frame, then it requests the storage device via RPMB layer to store the data.
> >
> > The layer provides two APIs, for rpmb_req_cmd() for issuing one of RPMB
> > specific commands and rpmb_seq_cmd() for issuing of raw RPMB protocol
> > frames, which is close to the functionality provided by emmc multi ioctl
> > interface.
> >
> > A TEE driver can claim the RPMB interface, for example, via
> > class_interface_register ().
> >
> > A storage device registers its RPMB hardware (eMMC) partition or RPMB W-
> > LUN (UFS) with the RPMB layer providing an implementation for
> > rpmb_seq_cmd() handler. The interface enables sending sequence of RPMB
> > standard frames.
> >
> > A parallel user space API is provided via /dev/rpmbX character device with
> > two IOCTL commands.
> > Simplified one, RPMB_IOC_REQ_CMD, were read result cycles is performed
> > by the framework on behalf the user and second, RPMB_IOC_SEQ_CMD
> > where the whole RPMB sequence, including RESULT_READ is supplied by the
> > caller.
> > The latter is intended for easier adjusting of the applications that use
> > MMC_IOC_MULTI_CMD ioctl, such as
> > https://android.googlesource.com/trusty/app/storage/
> >
> > There is a also sample tool under tools/rpmb/ directory that exercises these
> > interfaces and a simulation device that implements the device part.
> >
> > The code is also available from:
> >
> > https://github.com/tomasbw/linux-mei.git rpmb
> >
>
> Greg, can you please check if this series has addressed all your comments.
> Are there are any more items that preventing it from merging?
Ugh, my queue is huge right now, give me a week or so to dig out of it
and review this...
Oh wait, you have almost no reviews from anyone else! Why is it up to
me to do all of this work? :)
Please get acks from others, at the very least, get it reviewed by other
Intel kernel developers that we know and trust. I'm amazed you haven't
already done that!
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v2 2/4] mmc: dw_mmc: avoid race condition of cpu and IDMAC
From: Jaehoon Chung @ 2016-09-20 9:49 UTC (permalink / raw)
To: Shawn Lin
Cc: Ulf Hansson, linux-mmc, linux-kernel, Doug Anderson,
Heiko Stuebner, linux-rockchip
In-Reply-To: <35afa945-1b94-f870-d52e-6ab7b1da4a67@rock-chips.com>
Hi Shawn,
On 09/20/2016 06:47 PM, Shawn Lin wrote:
> Hi Jaehoon,
>
> Friendly ping... :)
Thanks for reminding! :) I forgot your patch-set..sorry!
Best Regards,
Jaehoon Chung
>
> On 2016/9/2 12:14, Shawn Lin wrote:
>> We could see an obvious race condition by test that
>> the former write operation by IDMAC aiming to clear
>> OWN bit reach right after the later configuration of
>> the same desc, which makes the IDMAC be in SUSPEND
>> state as the OWN bit was cleared by the asynchronous
>> write operation of IDMAC. The bug can be very easy
>> reproduced on RK3288 or similar when we reduce the
>> running rate of system buses and keep the CPU running
>> faster. So as two separate masters, IDMAC and cpu
>> write the same descriptor stored on the same address,
>> and this should be protected by adding check of OWN
>> bit before preparing new descriptors.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> ---
>>
>> Changes in v2:
>> - fallback to PIO mode if failing to wait for OWN bit
>> - cleanup polluted desc chain as the own bit error could
>> occur on any place of the chain, so we need to clr the desc
>> configured before that one. Use the exist function to reinit
>> the desc chain. As this issue is really rare, so after applied,
>> the fio/iozone stress test didn't show up performance regression.
>>
>> drivers/mmc/host/dw_mmc.c | 208 ++++++++++++++++++++++++++++------------------
>> 1 file changed, 129 insertions(+), 79 deletions(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 782b303..daa1c52 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -467,12 +467,87 @@ static void dw_mci_dmac_complete_dma(void *arg)
>> }
>> }
>>
>> -static inline void dw_mci_prepare_desc64(struct dw_mci *host,
>> +static int dw_mci_idmac_init(struct dw_mci *host)
>> +{
>> + int i;
>> +
>> + if (host->dma_64bit_address == 1) {
>> + struct idmac_desc_64addr *p;
>> + /* Number of descriptors in the ring buffer */
>> + host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc_64addr);
>> +
>> + /* Forward link the descriptor list */
>> + for (i = 0, p = host->sg_cpu; i < host->ring_size - 1;
>> + i++, p++) {
>> + p->des6 = (host->sg_dma +
>> + (sizeof(struct idmac_desc_64addr) *
>> + (i + 1))) & 0xffffffff;
>> +
>> + p->des7 = (u64)(host->sg_dma +
>> + (sizeof(struct idmac_desc_64addr) *
>> + (i + 1))) >> 32;
>> + /* Initialize reserved and buffer size fields to "0" */
>> + p->des1 = 0;
>> + p->des2 = 0;
>> + p->des3 = 0;
>> + }
>> +
>> + /* Set the last descriptor as the end-of-ring descriptor */
>> + p->des6 = host->sg_dma & 0xffffffff;
>> + p->des7 = (u64)host->sg_dma >> 32;
>> + p->des0 = IDMAC_DES0_ER;
>> +
>> + } else {
>> + struct idmac_desc *p;
>> + /* Number of descriptors in the ring buffer */
>> + host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc);
>> +
>> + /* Forward link the descriptor list */
>> + for (i = 0, p = host->sg_cpu;
>> + i < host->ring_size - 1;
>> + i++, p++) {
>> + p->des3 = cpu_to_le32(host->sg_dma +
>> + (sizeof(struct idmac_desc) * (i + 1)));
>> + p->des1 = 0;
>> + }
>> +
>> + /* Set the last descriptor as the end-of-ring descriptor */
>> + p->des3 = cpu_to_le32(host->sg_dma);
>> + p->des0 = cpu_to_le32(IDMAC_DES0_ER);
>> + }
>> +
>> + dw_mci_idmac_reset(host);
>> +
>> + if (host->dma_64bit_address == 1) {
>> + /* Mask out interrupts - get Tx & Rx complete only */
>> + mci_writel(host, IDSTS64, IDMAC_INT_CLR);
>> + mci_writel(host, IDINTEN64, SDMMC_IDMAC_INT_NI |
>> + SDMMC_IDMAC_INT_RI | SDMMC_IDMAC_INT_TI);
>> +
>> + /* Set the descriptor base address */
>> + mci_writel(host, DBADDRL, host->sg_dma & 0xffffffff);
>> + mci_writel(host, DBADDRU, (u64)host->sg_dma >> 32);
>> +
>> + } else {
>> + /* Mask out interrupts - get Tx & Rx complete only */
>> + mci_writel(host, IDSTS, IDMAC_INT_CLR);
>> + mci_writel(host, IDINTEN, SDMMC_IDMAC_INT_NI |
>> + SDMMC_IDMAC_INT_RI | SDMMC_IDMAC_INT_TI);
>> +
>> + /* Set the descriptor base address */
>> + mci_writel(host, DBADDR, host->sg_dma);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static inline int dw_mci_prepare_desc64(struct dw_mci *host,
>> struct mmc_data *data,
>> unsigned int sg_len)
>> {
>> unsigned int desc_len;
>> struct idmac_desc_64addr *desc_first, *desc_last, *desc;
>> + unsigned long timeout;
>> int i;
>>
>> desc_first = desc_last = desc = host->sg_cpu;
>> @@ -489,6 +564,19 @@ static inline void dw_mci_prepare_desc64(struct dw_mci *host,
>> length -= desc_len;
>>
>> /*
>> + * Wait for the former clear OWN bit operation
>> + * of IDMAC to make sure that this descriptor
>> + * isn't still owned by IDMAC as IDMAC's write
>> + * ops and CPU's read ops are asynchronous.
>> + */
>> + timeout = jiffies + msecs_to_jiffies(100);
>> + while (readl(&desc->des0) & IDMAC_DES0_OWN) {
>> + if (time_after(jiffies, timeout))
>> + goto err_own_bit;
>> + udelay(10);
>> + }
>> +
>> + /*
>> * Set the OWN bit and disable interrupts
>> * for this descriptor
>> */
>> @@ -516,15 +604,24 @@ static inline void dw_mci_prepare_desc64(struct dw_mci *host,
>> /* Set last descriptor */
>> desc_last->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC);
>> desc_last->des0 |= IDMAC_DES0_LD;
>> +
>> + return 0;
>> +err_own_bit:
>> + /* restore the descriptor chain as it's polluted */
>> + dev_dbg(host->dev, "desciptor is still owned by IDMAC.\n");
>> + memset(host->sg_cpu, 0, PAGE_SIZE);
>> + dw_mci_idmac_init(host);
>> + return -EINVAL;
>> }
>>
>>
>> -static inline void dw_mci_prepare_desc32(struct dw_mci *host,
>> +static inline int dw_mci_prepare_desc32(struct dw_mci *host,
>> struct mmc_data *data,
>> unsigned int sg_len)
>> {
>> unsigned int desc_len;
>> struct idmac_desc *desc_first, *desc_last, *desc;
>> + unsigned long timeout;
>> int i;
>>
>> desc_first = desc_last = desc = host->sg_cpu;
>> @@ -541,6 +638,20 @@ static inline void dw_mci_prepare_desc32(struct dw_mci *host,
>> length -= desc_len;
>>
>> /*
>> + * Wait for the former clear OWN bit operation
>> + * of IDMAC to make sure that this descriptor
>> + * isn't still owned by IDMAC as IDMAC's write
>> + * ops and CPU's read ops are asynchronous.
>> + */
>> + timeout = jiffies + msecs_to_jiffies(100);
>> + while (readl(&desc->des0) &
>> + cpu_to_le32(IDMAC_DES0_OWN)) {
>> + if (time_after(jiffies, timeout))
>> + goto err_own_bit;
>> + udelay(10);
>> + }
>> +
>> + /*
>> * Set the OWN bit and disable interrupts
>> * for this descriptor
>> */
>> @@ -569,16 +680,28 @@ static inline void dw_mci_prepare_desc32(struct dw_mci *host,
>> desc_last->des0 &= cpu_to_le32(~(IDMAC_DES0_CH |
>> IDMAC_DES0_DIC));
>> desc_last->des0 |= cpu_to_le32(IDMAC_DES0_LD);
>> +
>> + return 0;
>> +err_own_bit:
>> + /* restore the descriptor chain as it's polluted */
>> + dev_dbg(host->dev, "desciptor is still owned by IDMAC.\n");
>> + memset(host->sg_cpu, 0, PAGE_SIZE);
>> + dw_mci_idmac_init(host);
>> + return -EINVAL;
>> }
>>
>> static int dw_mci_idmac_start_dma(struct dw_mci *host, unsigned int sg_len)
>> {
>> u32 temp;
>> + int ret;
>>
>> if (host->dma_64bit_address == 1)
>> - dw_mci_prepare_desc64(host, host->data, sg_len);
>> + ret = dw_mci_prepare_desc64(host, host->data, sg_len);
>> else
>> - dw_mci_prepare_desc32(host, host->data, sg_len);
>> + ret = dw_mci_prepare_desc32(host, host->data, sg_len);
>> +
>> + if (ret)
>> + goto out;
>>
>> /* drain writebuffer */
>> wmb();
>> @@ -603,81 +726,8 @@ static int dw_mci_idmac_start_dma(struct dw_mci *host, unsigned int sg_len)
>> /* Start it running */
>> mci_writel(host, PLDMND, 1);
>>
>> - return 0;
>> -}
>> -
>> -static int dw_mci_idmac_init(struct dw_mci *host)
>> -{
>> - int i;
>> -
>> - if (host->dma_64bit_address == 1) {
>> - struct idmac_desc_64addr *p;
>> - /* Number of descriptors in the ring buffer */
>> - host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc_64addr);
>> -
>> - /* Forward link the descriptor list */
>> - for (i = 0, p = host->sg_cpu; i < host->ring_size - 1;
>> - i++, p++) {
>> - p->des6 = (host->sg_dma +
>> - (sizeof(struct idmac_desc_64addr) *
>> - (i + 1))) & 0xffffffff;
>> -
>> - p->des7 = (u64)(host->sg_dma +
>> - (sizeof(struct idmac_desc_64addr) *
>> - (i + 1))) >> 32;
>> - /* Initialize reserved and buffer size fields to "0" */
>> - p->des1 = 0;
>> - p->des2 = 0;
>> - p->des3 = 0;
>> - }
>> -
>> - /* Set the last descriptor as the end-of-ring descriptor */
>> - p->des6 = host->sg_dma & 0xffffffff;
>> - p->des7 = (u64)host->sg_dma >> 32;
>> - p->des0 = IDMAC_DES0_ER;
>> -
>> - } else {
>> - struct idmac_desc *p;
>> - /* Number of descriptors in the ring buffer */
>> - host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc);
>> -
>> - /* Forward link the descriptor list */
>> - for (i = 0, p = host->sg_cpu;
>> - i < host->ring_size - 1;
>> - i++, p++) {
>> - p->des3 = cpu_to_le32(host->sg_dma +
>> - (sizeof(struct idmac_desc) * (i + 1)));
>> - p->des1 = 0;
>> - }
>> -
>> - /* Set the last descriptor as the end-of-ring descriptor */
>> - p->des3 = cpu_to_le32(host->sg_dma);
>> - p->des0 = cpu_to_le32(IDMAC_DES0_ER);
>> - }
>> -
>> - dw_mci_idmac_reset(host);
>> -
>> - if (host->dma_64bit_address == 1) {
>> - /* Mask out interrupts - get Tx & Rx complete only */
>> - mci_writel(host, IDSTS64, IDMAC_INT_CLR);
>> - mci_writel(host, IDINTEN64, SDMMC_IDMAC_INT_NI |
>> - SDMMC_IDMAC_INT_RI | SDMMC_IDMAC_INT_TI);
>> -
>> - /* Set the descriptor base address */
>> - mci_writel(host, DBADDRL, host->sg_dma & 0xffffffff);
>> - mci_writel(host, DBADDRU, (u64)host->sg_dma >> 32);
>> -
>> - } else {
>> - /* Mask out interrupts - get Tx & Rx complete only */
>> - mci_writel(host, IDSTS, IDMAC_INT_CLR);
>> - mci_writel(host, IDINTEN, SDMMC_IDMAC_INT_NI |
>> - SDMMC_IDMAC_INT_RI | SDMMC_IDMAC_INT_TI);
>> -
>> - /* Set the descriptor base address */
>> - mci_writel(host, DBADDR, host->sg_dma);
>> - }
>> -
>> - return 0;
>> +out:
>> + return ret;
>> }
>>
>> static const struct dw_mci_dma_ops dw_mci_idmac_ops = {
>>
>
>
^ permalink raw reply
* Re: [PATCH v2 2/4] mmc: dw_mmc: avoid race condition of cpu and IDMAC
From: Shawn Lin @ 2016-09-20 9:47 UTC (permalink / raw)
To: Jaehoon Chung
Cc: shawn.lin, Ulf Hansson, linux-mmc, linux-kernel, Doug Anderson,
Heiko Stuebner, linux-rockchip
In-Reply-To: <1472789679-15121-2-git-send-email-shawn.lin@rock-chips.com>
Hi Jaehoon,
Friendly ping... :)
On 2016/9/2 12:14, Shawn Lin wrote:
> We could see an obvious race condition by test that
> the former write operation by IDMAC aiming to clear
> OWN bit reach right after the later configuration of
> the same desc, which makes the IDMAC be in SUSPEND
> state as the OWN bit was cleared by the asynchronous
> write operation of IDMAC. The bug can be very easy
> reproduced on RK3288 or similar when we reduce the
> running rate of system buses and keep the CPU running
> faster. So as two separate masters, IDMAC and cpu
> write the same descriptor stored on the same address,
> and this should be protected by adding check of OWN
> bit before preparing new descriptors.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>
> ---
>
> Changes in v2:
> - fallback to PIO mode if failing to wait for OWN bit
> - cleanup polluted desc chain as the own bit error could
> occur on any place of the chain, so we need to clr the desc
> configured before that one. Use the exist function to reinit
> the desc chain. As this issue is really rare, so after applied,
> the fio/iozone stress test didn't show up performance regression.
>
> drivers/mmc/host/dw_mmc.c | 208 ++++++++++++++++++++++++++++------------------
> 1 file changed, 129 insertions(+), 79 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 782b303..daa1c52 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -467,12 +467,87 @@ static void dw_mci_dmac_complete_dma(void *arg)
> }
> }
>
> -static inline void dw_mci_prepare_desc64(struct dw_mci *host,
> +static int dw_mci_idmac_init(struct dw_mci *host)
> +{
> + int i;
> +
> + if (host->dma_64bit_address == 1) {
> + struct idmac_desc_64addr *p;
> + /* Number of descriptors in the ring buffer */
> + host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc_64addr);
> +
> + /* Forward link the descriptor list */
> + for (i = 0, p = host->sg_cpu; i < host->ring_size - 1;
> + i++, p++) {
> + p->des6 = (host->sg_dma +
> + (sizeof(struct idmac_desc_64addr) *
> + (i + 1))) & 0xffffffff;
> +
> + p->des7 = (u64)(host->sg_dma +
> + (sizeof(struct idmac_desc_64addr) *
> + (i + 1))) >> 32;
> + /* Initialize reserved and buffer size fields to "0" */
> + p->des1 = 0;
> + p->des2 = 0;
> + p->des3 = 0;
> + }
> +
> + /* Set the last descriptor as the end-of-ring descriptor */
> + p->des6 = host->sg_dma & 0xffffffff;
> + p->des7 = (u64)host->sg_dma >> 32;
> + p->des0 = IDMAC_DES0_ER;
> +
> + } else {
> + struct idmac_desc *p;
> + /* Number of descriptors in the ring buffer */
> + host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc);
> +
> + /* Forward link the descriptor list */
> + for (i = 0, p = host->sg_cpu;
> + i < host->ring_size - 1;
> + i++, p++) {
> + p->des3 = cpu_to_le32(host->sg_dma +
> + (sizeof(struct idmac_desc) * (i + 1)));
> + p->des1 = 0;
> + }
> +
> + /* Set the last descriptor as the end-of-ring descriptor */
> + p->des3 = cpu_to_le32(host->sg_dma);
> + p->des0 = cpu_to_le32(IDMAC_DES0_ER);
> + }
> +
> + dw_mci_idmac_reset(host);
> +
> + if (host->dma_64bit_address == 1) {
> + /* Mask out interrupts - get Tx & Rx complete only */
> + mci_writel(host, IDSTS64, IDMAC_INT_CLR);
> + mci_writel(host, IDINTEN64, SDMMC_IDMAC_INT_NI |
> + SDMMC_IDMAC_INT_RI | SDMMC_IDMAC_INT_TI);
> +
> + /* Set the descriptor base address */
> + mci_writel(host, DBADDRL, host->sg_dma & 0xffffffff);
> + mci_writel(host, DBADDRU, (u64)host->sg_dma >> 32);
> +
> + } else {
> + /* Mask out interrupts - get Tx & Rx complete only */
> + mci_writel(host, IDSTS, IDMAC_INT_CLR);
> + mci_writel(host, IDINTEN, SDMMC_IDMAC_INT_NI |
> + SDMMC_IDMAC_INT_RI | SDMMC_IDMAC_INT_TI);
> +
> + /* Set the descriptor base address */
> + mci_writel(host, DBADDR, host->sg_dma);
> + }
> +
> + return 0;
> +}
> +
> +static inline int dw_mci_prepare_desc64(struct dw_mci *host,
> struct mmc_data *data,
> unsigned int sg_len)
> {
> unsigned int desc_len;
> struct idmac_desc_64addr *desc_first, *desc_last, *desc;
> + unsigned long timeout;
> int i;
>
> desc_first = desc_last = desc = host->sg_cpu;
> @@ -489,6 +564,19 @@ static inline void dw_mci_prepare_desc64(struct dw_mci *host,
> length -= desc_len;
>
> /*
> + * Wait for the former clear OWN bit operation
> + * of IDMAC to make sure that this descriptor
> + * isn't still owned by IDMAC as IDMAC's write
> + * ops and CPU's read ops are asynchronous.
> + */
> + timeout = jiffies + msecs_to_jiffies(100);
> + while (readl(&desc->des0) & IDMAC_DES0_OWN) {
> + if (time_after(jiffies, timeout))
> + goto err_own_bit;
> + udelay(10);
> + }
> +
> + /*
> * Set the OWN bit and disable interrupts
> * for this descriptor
> */
> @@ -516,15 +604,24 @@ static inline void dw_mci_prepare_desc64(struct dw_mci *host,
> /* Set last descriptor */
> desc_last->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC);
> desc_last->des0 |= IDMAC_DES0_LD;
> +
> + return 0;
> +err_own_bit:
> + /* restore the descriptor chain as it's polluted */
> + dev_dbg(host->dev, "desciptor is still owned by IDMAC.\n");
> + memset(host->sg_cpu, 0, PAGE_SIZE);
> + dw_mci_idmac_init(host);
> + return -EINVAL;
> }
>
>
> -static inline void dw_mci_prepare_desc32(struct dw_mci *host,
> +static inline int dw_mci_prepare_desc32(struct dw_mci *host,
> struct mmc_data *data,
> unsigned int sg_len)
> {
> unsigned int desc_len;
> struct idmac_desc *desc_first, *desc_last, *desc;
> + unsigned long timeout;
> int i;
>
> desc_first = desc_last = desc = host->sg_cpu;
> @@ -541,6 +638,20 @@ static inline void dw_mci_prepare_desc32(struct dw_mci *host,
> length -= desc_len;
>
> /*
> + * Wait for the former clear OWN bit operation
> + * of IDMAC to make sure that this descriptor
> + * isn't still owned by IDMAC as IDMAC's write
> + * ops and CPU's read ops are asynchronous.
> + */
> + timeout = jiffies + msecs_to_jiffies(100);
> + while (readl(&desc->des0) &
> + cpu_to_le32(IDMAC_DES0_OWN)) {
> + if (time_after(jiffies, timeout))
> + goto err_own_bit;
> + udelay(10);
> + }
> +
> + /*
> * Set the OWN bit and disable interrupts
> * for this descriptor
> */
> @@ -569,16 +680,28 @@ static inline void dw_mci_prepare_desc32(struct dw_mci *host,
> desc_last->des0 &= cpu_to_le32(~(IDMAC_DES0_CH |
> IDMAC_DES0_DIC));
> desc_last->des0 |= cpu_to_le32(IDMAC_DES0_LD);
> +
> + return 0;
> +err_own_bit:
> + /* restore the descriptor chain as it's polluted */
> + dev_dbg(host->dev, "desciptor is still owned by IDMAC.\n");
> + memset(host->sg_cpu, 0, PAGE_SIZE);
> + dw_mci_idmac_init(host);
> + return -EINVAL;
> }
>
> static int dw_mci_idmac_start_dma(struct dw_mci *host, unsigned int sg_len)
> {
> u32 temp;
> + int ret;
>
> if (host->dma_64bit_address == 1)
> - dw_mci_prepare_desc64(host, host->data, sg_len);
> + ret = dw_mci_prepare_desc64(host, host->data, sg_len);
> else
> - dw_mci_prepare_desc32(host, host->data, sg_len);
> + ret = dw_mci_prepare_desc32(host, host->data, sg_len);
> +
> + if (ret)
> + goto out;
>
> /* drain writebuffer */
> wmb();
> @@ -603,81 +726,8 @@ static int dw_mci_idmac_start_dma(struct dw_mci *host, unsigned int sg_len)
> /* Start it running */
> mci_writel(host, PLDMND, 1);
>
> - return 0;
> -}
> -
> -static int dw_mci_idmac_init(struct dw_mci *host)
> -{
> - int i;
> -
> - if (host->dma_64bit_address == 1) {
> - struct idmac_desc_64addr *p;
> - /* Number of descriptors in the ring buffer */
> - host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc_64addr);
> -
> - /* Forward link the descriptor list */
> - for (i = 0, p = host->sg_cpu; i < host->ring_size - 1;
> - i++, p++) {
> - p->des6 = (host->sg_dma +
> - (sizeof(struct idmac_desc_64addr) *
> - (i + 1))) & 0xffffffff;
> -
> - p->des7 = (u64)(host->sg_dma +
> - (sizeof(struct idmac_desc_64addr) *
> - (i + 1))) >> 32;
> - /* Initialize reserved and buffer size fields to "0" */
> - p->des1 = 0;
> - p->des2 = 0;
> - p->des3 = 0;
> - }
> -
> - /* Set the last descriptor as the end-of-ring descriptor */
> - p->des6 = host->sg_dma & 0xffffffff;
> - p->des7 = (u64)host->sg_dma >> 32;
> - p->des0 = IDMAC_DES0_ER;
> -
> - } else {
> - struct idmac_desc *p;
> - /* Number of descriptors in the ring buffer */
> - host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc);
> -
> - /* Forward link the descriptor list */
> - for (i = 0, p = host->sg_cpu;
> - i < host->ring_size - 1;
> - i++, p++) {
> - p->des3 = cpu_to_le32(host->sg_dma +
> - (sizeof(struct idmac_desc) * (i + 1)));
> - p->des1 = 0;
> - }
> -
> - /* Set the last descriptor as the end-of-ring descriptor */
> - p->des3 = cpu_to_le32(host->sg_dma);
> - p->des0 = cpu_to_le32(IDMAC_DES0_ER);
> - }
> -
> - dw_mci_idmac_reset(host);
> -
> - if (host->dma_64bit_address == 1) {
> - /* Mask out interrupts - get Tx & Rx complete only */
> - mci_writel(host, IDSTS64, IDMAC_INT_CLR);
> - mci_writel(host, IDINTEN64, SDMMC_IDMAC_INT_NI |
> - SDMMC_IDMAC_INT_RI | SDMMC_IDMAC_INT_TI);
> -
> - /* Set the descriptor base address */
> - mci_writel(host, DBADDRL, host->sg_dma & 0xffffffff);
> - mci_writel(host, DBADDRU, (u64)host->sg_dma >> 32);
> -
> - } else {
> - /* Mask out interrupts - get Tx & Rx complete only */
> - mci_writel(host, IDSTS, IDMAC_INT_CLR);
> - mci_writel(host, IDINTEN, SDMMC_IDMAC_INT_NI |
> - SDMMC_IDMAC_INT_RI | SDMMC_IDMAC_INT_TI);
> -
> - /* Set the descriptor base address */
> - mci_writel(host, DBADDR, host->sg_dma);
> - }
> -
> - return 0;
> +out:
> + return ret;
> }
>
> static const struct dw_mci_dma_ops dw_mci_idmac_ops = {
>
--
Best Regards
Shawn Lin
^ permalink raw reply
* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
From: Oliver Neukum @ 2016-09-20 9:40 UTC (permalink / raw)
To: Alan Stern
Cc: Ulf Hansson, Micky Ching, Wei WANG, Roger Tseng,
Ritesh Raj Sarraf, linux-mmc, USB list
In-Reply-To: <Pine.LNX.4.44L0.1609191348440.1458-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
On Mon, 2016-09-19 at 14:02 -0400, Alan Stern wrote:
> > We can for sure enable autosuspend for the sdmmc device, although as
> > soon as an SD card will be detected the rtsx driver will increase
> the
> > runtime PM usage count. The count is decreased when the card is
> > removed (or failed to be initialized), thus runtime suspend is
> > prevented as long as there is a functional card inserted.
Testing autosuspend with card readers on usb_storage I saw a uniform
response of reporting a medium change event upon resume.
I am afraid other kinds of readers are not better in that regard.
>
> Which means that autosuspend matters only when a card isn't present,
> and the host is polled every second or so to see whether a card has
> been inserted.
>
> Under those circumstances you probably don't want to use
> autosuspend.
> That is, resuming before each poll and suspending afterward may use
> less energy than staying at full power all the time.
Is that based on concrete figures about power consumption?
And it seems to me that we need a way to indicate that the heuristics
should not be used, but a device immediately suspended. The timer
is sensible only if the next wakeup is unknown.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 v3 4/6] mmc: tmio: add eMMC support
From: Jaehoon Chung @ 2016-09-20 9:35 UTC (permalink / raw)
To: Wolfram Sang, linux-mmc; +Cc: linux-renesas-soc, Simon Horman, Dirk Behme
In-Reply-To: <20160919205750.4766-5-wsa+renesas@sang-engineering.com>
On 09/20/2016 05:57 AM, Wolfram Sang wrote:
> We need to add R1 without CRC support, refactor the bus width routine a
> little and extend a quirk check. To support "non-removable;" we need a
> workaround which will be hopefully removed when reworking PM soon.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> drivers/mmc/host/tmio_mmc.h | 3 +++
> drivers/mmc/host/tmio_mmc_pio.c | 38 ++++++++++++++++++++++++++------------
> 2 files changed, 29 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index 4b501f2d529f6e..637581faf756b1 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -79,6 +79,9 @@
> #define CLK_CTL_DIV_MASK 0xff
> #define CLK_CTL_SCLKEN BIT(8)
>
> +#define CARD_OPT_WIDTH8 BIT(13)
> +#define CARD_OPT_WIDTH BIT(15)
Just confusing whether CARD_OPT_WIDTH is 4bit or 1bit?
> +
> #define TMIO_BBS 512 /* Boot block size */
>
> /* Definitions for values the CTRL_SDIO_STATUS register can take. */
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index 46b5a456243b84..a0f05eb4f34490 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -340,7 +340,9 @@ static int tmio_mmc_start_command(struct tmio_mmc_host *host, struct mmc_command
>
> switch (mmc_resp_type(cmd)) {
> case MMC_RSP_NONE: c |= RESP_NONE; break;
> - case MMC_RSP_R1: c |= RESP_R1; break;
> + case MMC_RSP_R1:
> + case MMC_RSP_R1_NO_CRC:
> + c |= RESP_R1; break;
Just wonder..It there case that hit "case MMC_RSP_R1_NO_CRC" ?
> case MMC_RSP_R1B: c |= RESP_R1B; break;
> case MMC_RSP_R2: c |= RESP_R2; break;
> case MMC_RSP_R3: c |= RESP_R3; break;
> @@ -737,12 +739,13 @@ static int tmio_mmc_start_data(struct tmio_mmc_host *host,
> pr_debug("setup data transfer: blocksize %08x nr_blocks %d\n",
> data->blksz, data->blocks);
>
> - /* Some hardware cannot perform 2 byte requests in 4 bit mode */
> - if (host->mmc->ios.bus_width == MMC_BUS_WIDTH_4) {
> + /* Some hardware cannot perform 2 byte requests in 4/8 bit mode */
> + if (host->mmc->ios.bus_width == MMC_BUS_WIDTH_4 ||
> + host->mmc->ios.bus_width == MMC_BUS_WIDTH_8) {
> int blksz_2bytes = pdata->flags & TMIO_MMC_BLKSZ_2BYTES;
>
> if (data->blksz < 2 || (data->blksz < 4 && !blksz_2bytes)) {
> - pr_err("%s: %d byte block unsupported in 4 bit mode\n",
> + pr_err("%s: %d byte block unsupported in 4/8 bit mode\n",
> mmc_hostname(host->mmc), data->blksz);
> return -EINVAL;
> }
> @@ -922,14 +925,16 @@ static void tmio_mmc_power_off(struct tmio_mmc_host *host)
> static void tmio_mmc_set_bus_width(struct tmio_mmc_host *host,
> unsigned char bus_width)
> {
> - switch (bus_width) {
> - case MMC_BUS_WIDTH_1:
> - sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x80e0);
> - break;
> - case MMC_BUS_WIDTH_4:
> - sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, 0x00e0);
> - break;
> - }
> + u16 reg = sd_ctrl_read16(host, CTL_SD_MEM_CARD_OPT)
> + & ~(CARD_OPT_WIDTH | CARD_OPT_WIDTH8);
> +
> + /* reg now applies to MMC_BUS_WIDTH_4 */
> + if (bus_width == MMC_BUS_WIDTH_1)
> + reg |= CARD_OPT_WIDTH;
> + else if (bus_width == MMC_BUS_WIDTH_8)
> + reg |= CARD_OPT_WIDTH8;
> +
> + sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, reg);
> }
>
> /* Set MMC clock / power.
> @@ -1149,6 +1154,15 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host,
> !mmc_card_is_removable(mmc) ||
> mmc->slot.cd_irq >= 0);
>
> + /*
> + * On Gen2+, eMMC with NONREMOVABLE currently fails because native
> + * hotplug gets disabled. It seems RuntimePM related yet we need further
> + * research. Since we are planning a PM overhaul anyway, let's enforce
> + * for now the device being active by enabling native hotplug always.
> + */
> + if (pdata->flags & TMIO_MMC_MIN_RCAR2)
> + _host->native_hotplug = true;
> +
> if (tmio_mmc_clk_enable(_host) < 0) {
> mmc->f_max = pdata->hclk;
> mmc->f_min = mmc->f_max / 512;
>
^ permalink raw reply
* [PATCH v2] mmc: card: do away with indirection pointer
From: Linus Walleij @ 2016-09-20 9:34 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson; +Cc: Linus Walleij
We have enough vtables in the kernel as it is, we don't need
this one to create even more artificial separation of concerns.
As is proved by the Makefile:
obj-$(CONFIG_MMC_BLOCK) += mmc_block.o
mmc_block-objs := block.o queue.o
block.c and queue.c are baked into the same mmc_block.o object.
So why would one of these objects access a function in the
other object by dereferencing a pointer?
Create a new block.h header file for the single shared function
from block to queue and remove the function pointer and just
call the queue request function.
Apart from making the code more readable, this also makes link
optimizations possible and probably speeds up the call as well.
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Actually commit the new block.h file too, mea culpa.
---
drivers/mmc/card/block.c | 3 +--
drivers/mmc/card/block.h | 1 +
drivers/mmc/card/queue.c | 4 +++-
drivers/mmc/card/queue.h | 2 --
4 files changed, 5 insertions(+), 5 deletions(-)
create mode 100644 drivers/mmc/card/block.h
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 2206d4477dbb..15acf96147f3 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -2144,7 +2144,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
return 0;
}
-static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
+int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
{
int ret;
struct mmc_blk_data *md = mq->data;
@@ -2265,7 +2265,6 @@ again:
if (ret)
goto err_putdisk;
- md->queue.issue_fn = mmc_blk_issue_rq;
md->queue.data = md;
md->disk->major = MMC_BLOCK_MAJOR;
diff --git a/drivers/mmc/card/block.h b/drivers/mmc/card/block.h
new file mode 100644
index 000000000000..cdabb2ee74be
--- /dev/null
+++ b/drivers/mmc/card/block.h
@@ -0,0 +1 @@
+int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req);
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index 708057261b38..8037f73a109a 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -19,7 +19,9 @@
#include <linux/mmc/card.h>
#include <linux/mmc/host.h>
+
#include "queue.h"
+#include "block.h"
#define MMC_QUEUE_BOUNCESZ 65536
@@ -68,7 +70,7 @@ static int mmc_queue_thread(void *d)
bool req_is_special = mmc_req_is_special(req);
set_current_state(TASK_RUNNING);
- mq->issue_fn(mq, req);
+ mmc_blk_issue_rq(mq, req);
cond_resched();
if (mq->flags & MMC_QUEUE_NEW_REQUEST) {
mq->flags &= ~MMC_QUEUE_NEW_REQUEST;
diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
index fee5e1271465..3c15a75bae86 100644
--- a/drivers/mmc/card/queue.h
+++ b/drivers/mmc/card/queue.h
@@ -57,8 +57,6 @@ struct mmc_queue {
unsigned int flags;
#define MMC_QUEUE_SUSPENDED (1 << 0)
#define MMC_QUEUE_NEW_REQUEST (1 << 1)
-
- int (*issue_fn)(struct mmc_queue *, struct request *);
void *data;
struct request_queue *queue;
struct mmc_queue_req mqrq[2];
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
From: Ulf Hansson @ 2016-09-20 9:34 UTC (permalink / raw)
To: Alan Stern
Cc: linux-mmc, Ritesh Raj Sarraf, USB list, Micky Ching, Roger Tseng,
Wei WANG
In-Reply-To: <Pine.LNX.4.44L0.1609191348440.1458-100000@iolanthe.rowland.org>
On 19 September 2016 at 20:02, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 19 Sep 2016, Ulf Hansson wrote:
>
>> On 18 September 2016 at 04:30, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Sat, 17 Sep 2016, Ulf Hansson wrote:
>> >
>> >> Each access of the parent device (usb device) needs to be done in runtime
>> >> resumed state. Currently this isn't case while changing the leds, so let's
>> >> add pm_runtime_get_sync() and pm_runtime_put() around these calls.
>> >>
>> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> >> ---
>> >>
>> >> While discussing an issue[1] related to runtime PM, I found out that this
>> >> minor change at least improves the behavior that has been observed.
>> >>
>> >> [1]
>> >> http://www.spinics.net/lists/linux-usb/msg144634.html
>> >>
>> >> ---
>> >> 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 6c71fc9..a59c7fa 100644
>> >> --- a/drivers/mmc/host/rtsx_usb_sdmmc.c
>> >> +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
>> >> @@ -1314,6 +1314,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)
>> >> @@ -1322,6 +1323,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
>> >
>> > The missing aspect here is that this won't stop the parent USB device
>> > from going into autosuspend every 2 seconds and then resuming shortly
>> > afterward. There are two ways of preventing this:
>> >
>> > Call usb_mark_last_busy() at appropriate places.
>> >
>> > Enable autosuspend for the sdmmc device.
>> >
>> > The second approach would also prevent the sdmmc device from going into
>> > autosuspend as soon as the LED update is finished. Maybe that's okay,
>> > but if going into suspend is a lightweight procedure then you may want
>> > to prevent it.
>> >
>>
>> We can for sure enable autosuspend for the sdmmc device, although as
>> soon as an SD card will be detected the rtsx driver will increase the
>> runtime PM usage count. The count is decreased when the card is
>> removed (or failed to be initialized), thus runtime suspend is
>> prevented as long as there is a functional card inserted.
>
> Which means that autosuspend matters only when a card isn't present,
> and the host is polled every second or so to see whether a card has
> been inserted.
>
> Under those circumstances you probably don't want to use autosuspend.
> That is, resuming before each poll and suspending afterward may use
> less energy than staying at full power all the time.
>
>> I am wondering what you think would be a good autosuspend timeout in
>> this case? It seems to me that the only thing the rtsx driver really
>> care about is to tell the parent device that it needs to be runtime
>> resumed during a certain timeframe, more or less it would like to
>> inherit the parents settings.
>>
>> Other mmc hosts, not being usb-mmc devices, are using an autosuspend
>> timeout of ~50-100ms but that doesn't seem like good value here,
>> right?
>
> Well, if you decide to let the device go into runtime suspend between
> polls then there's no reason to use autosuspend at all. Once a poll
> has ended, you know there won't be any more activity until the next
> poll.
We could change that, as currently the approach in the mmc core isn't
that sophisticated. I even think this has been discussed earlier for
the very similar reasons regards polling card detect mode.
I guess the main reason to why we yet have changed this, is because
mmc host drivers are using an autosuspend timeout of ~50-100 ms, so in
the end it haven't been such a big deal.
>
> On the other hand, if you decide to keep the device at full power all
> the time during polling, then any autosuspend timeout larger than 1000
> ms would do what you want.
>
> Mostly I'm concerned about how this will interact with the USB runtime
> PM. The thing is, suspending the sdmmc device doesn't save any energy,
> whereas suspending the USB device does.
Yes, I agree.
My concern is also 2s autosuspend timeout which is set for the usb
device. Somehow I feel we need to be able "share" more information
between a parent-child relationship, in this case between the sdmmc
device and the usb device.
An observation I made, is when the sdmmc device gets runtime resumed
(pm_runtime_get_sync()), the parent device (the usb device) may also
become runtime resumed (unless it's already). In this sequence, but
*only* when actually runtime resuming the usb device, the runtime PM
core decides to update the last busy mark for the usb device. Should
it really do that?
Moreover, I am curious about the 2s usb timeout. Why isn't that chosen
to something like ~100ms instead? Is there is a long latency to
runtime resume the usb device or because we fear to wear out the HW,
which may be powered on/off too frequently?
If we assume that the usb device shouldn't be used with a timeout less
than 2s, then I think we have two options:
*) As the mmc polling timeout is 1s, there is really no point in
trying to runtime suspend the usb device, it may just be left runtime
resumed all the time. Wasting power, of course!
**) Add an interface to allow dynamically changes of the mmc polling
timeout to better suit the current user.
[...]
Kind regards
Uffe
^ 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