* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
From: Alan Stern @ 2016-09-22 13:56 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Ritesh Raj Sarraf, USB list, Roger Tseng, Wei WANG
In-Reply-To: <CAPDyKFozTL9h3HXoimHc4X3jeWQtJaedrfExVq1A5g7-JzcNLg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thu, 22 Sep 2016, Ulf Hansson wrote:
> >> > > 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.
> >>
> >> I understand it's deliberate, but I was more question whether we
> >> actually should have the runtime PM core to behaves like this.
> >>
> >> I don't think its behaviour is consistent, as in such case all calls
> >> to __pm_runtime_resume() should trigger the runtime PM core to update
> >> the last busy mark.
> >
> > Not a bad idea...
>
> Yes, it is. :-)
>
> Although, I am still concerned about he inconsistent behaviour.
>
> >
> >> Don't get me wrong, I am *not* suggesting we should do that change, as
> >> it would mean the last busy mark would be updated way too often.
> >
> > The updates aren't very expensive. Just one atomic write. It probably
> > takes less time than acquiring the runtime-PM spinlock.
> >
> >> Instead, perhaps it's better to leave the responsibility of updating
> >> the last busy mark to the runtime PM users solely.
> >
> > Maybe, but I think doing it once in the core, like this, can remove the
> > need for a lot of function calls in drivers.
>
> Unfortunate not. Most updates of the last busy mark happens when a
> device is no longer required to be runtime resumed. As when a driver
> has completed to serve a request and is about to call pm_runtime_put()
> (or similar API).
>
> So, I still believe doing it in the runtime PM core is just a waste.
>
> I think it's better to leave the update to the users entirely, it
> would become consistent but also more flexible, as one could easily
> think of situations where you may in some cases want to update the
> last busy mark and in some other not.
You can try making this change if you want. I'd be afraid of
regressions.
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: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
From: Oliver Neukum @ 2016-09-22 13:24 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.1609211031420.1996-100000@iolanthe.rowland.org>
On Wed, 2016-09-21 at 10:35 -0400, Alan Stern wrote:
> On Wed, 21 Sep 2016, Oliver Neukum wrote:
> > Yes, but this is not the point. A heuristic with a timeout makes
> > sense only if the uses are unpredictable. If you know with a high
> > degree of probability when the next activity comes, you ought to either
> > suspend now or not all until the next activity.
> >
> > Likewise the heuristic is appropriate for leaf nodes. You get nothing
> > from a delay on inner nodes.
>
> Almost true, but not quite. When an inner node has more than one leaf
> beneath it, enabling an autosuspend delay for the inner node can make
> sense -- particularly if the leaf activities are uncorrelated.
Well, it is true that an inner node is likelier to be woken up
depending on the number of children. That is a reason to have a longer
timeout for an inner node. But it should start when the first node goes
idle. It makes no sense to start yet another timeout when the last node
goes idle.
In terms of mathematics I think we would need to multiply the timeout
with the square root of busy children and restart it whenever a child
goes to idle.
But it seems to me that this is impractical.
So I would suggest that we are missing an API for drivers to tell the
core that they become idle for a known period of time and to propagate
that immediately up if that is the last leaf to become idle.
> > Any storage (generic sense) device
> > is an inner node. It should suspend immediately after the block
> > device which is the leaf node.
>
> Yes. In this case, however, the USB device has two platform devices
> beneath it: one for SDMMC and one for MemoryStick cards.
Indeed, we can hope that the power efficient work queue used will
join the polling of both devices. Ideally we could model the mutual
exclusion.
Regards
Oliver
^ permalink raw reply
* Re: [GIT PULL] Update dw-mmc controller
From: Ulf Hansson @ 2016-09-22 11:51 UTC (permalink / raw)
To: Jaehoon Chung; +Cc: linux-mmc@vger.kernel.org, Shawn Lin
In-Reply-To: <772c300d-7d81-d933-f9c5-401cb3a3afcb@samsung.com>
On 22 September 2016 at 12:57, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> Dear Ulf,
>
> Could you pull these patches into your mmc-next?
>
> The following changes since commit 6739115d4d319ba83d52f89e66b6a86cc021a2b5:
>
> mmc: core: don't try to switch block size for dual rate mode (2016-09-22 10:17:54 +0200)
>
> are available in the git repository at:
>
> https://github.com/jh80chung/dw-mmc.git for-ulf
>
> for you to fetch changes up to 2d25506534aec8a2e0980bf04b3e72b5b3c9f830:
>
> mmc: dw_mmc: remove the deprecated "supports-highspeed" property (2016-09-22 19:53:38 +0900)
>
> ----------------------------------------------------------------
> Jaehoon Chung (1):
> mmc: dw_mmc: remove the deprecated "supports-highspeed" property
>
> Shawn Lin (5):
> mmc: dw_mmc: split out preparation of desc for IDMAC32 and IDMAC64
> mmc: dw_mmc: avoid race condition of cpu and IDMAC
> mmc: dw_mmc: fix misleading error print if failing to do DMA transfer
> mmc: dw_mmc: use macro to define ring buffer size
> mmc: dw_mmc: minor cleanup for dw_mci_adjust_fifoth
>
> drivers/mmc/host/dw_mmc.c | 346 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------------------------------
> 1 file changed, 202 insertions(+), 144 deletions(-)
>
>
> Best Regards,
> Jaehoon Chung
Dear Jaehoon, thanks for the pull request. I have pulled it in to my
next branch.
Kind regards
Ulf Hansson
^ permalink raw reply
* [GIT PULL] Update dw-mmc controller
From: Jaehoon Chung @ 2016-09-22 10:57 UTC (permalink / raw)
To: linux-mmc@vger.kernel.org; +Cc: Ulf Hansson, Shawn Lin
Dear Ulf,
Could you pull these patches into your mmc-next?
The following changes since commit 6739115d4d319ba83d52f89e66b6a86cc021a2b5:
mmc: core: don't try to switch block size for dual rate mode (2016-09-22 10:17:54 +0200)
are available in the git repository at:
https://github.com/jh80chung/dw-mmc.git for-ulf
for you to fetch changes up to 2d25506534aec8a2e0980bf04b3e72b5b3c9f830:
mmc: dw_mmc: remove the deprecated "supports-highspeed" property (2016-09-22 19:53:38 +0900)
----------------------------------------------------------------
Jaehoon Chung (1):
mmc: dw_mmc: remove the deprecated "supports-highspeed" property
Shawn Lin (5):
mmc: dw_mmc: split out preparation of desc for IDMAC32 and IDMAC64
mmc: dw_mmc: avoid race condition of cpu and IDMAC
mmc: dw_mmc: fix misleading error print if failing to do DMA transfer
mmc: dw_mmc: use macro to define ring buffer size
mmc: dw_mmc: minor cleanup for dw_mci_adjust_fifoth
drivers/mmc/host/dw_mmc.c | 346 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------------------------------
1 file changed, 202 insertions(+), 144 deletions(-)
Best Regards,
Jaehoon Chung
^ permalink raw reply
* Re: [PATCH 3/5] mmc: core: changes frequency to hs_max_dtr when selecting hs400es
From: Ulf Hansson @ 2016-09-22 10:21 UTC (permalink / raw)
To: Shawn Lin
Cc: Adrian Hunter, Jaehoon Chung, linux-mmc,
linux-kernel@vger.kernel.org, open list:ARM/Rockchip SoC...
In-Reply-To: <62a8d622-39d2-db7d-042d-5425a3efba58@rock-chips.com>
On 22 September 2016 at 12:06, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> Hi ulf,
>
> 在 2016/9/22 17:38, Ulf Hansson 写道:
>>
>> On 21 September 2016 at 03:43, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>>
>>> 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.
>>
>>
>> I don't get this. Are you saying that f_init > 52 MHz? That should not
>> be impossible, right!?
>
>
> nope, I was saying that the spec implies we to set clock after
> setting HS_TIMING to 0x1 when doing hs400es selection.
>
> I thought there is no difference because the spec says "Host need to
> change frequency to <= 52MHz", and the f_init(<=400k) is <= 52MHz,
> right? So I didn't set clock to hs_max_dtr. But I think I misunderstood
> the spec, so this patch will fix this.
Okay, I see what you mean now!
In other words:
The card expects the clock rate to increase from the current used
f_init (which is <= 400KHz), but still being <= 52MHz, when you have
set HS_TIMING to 0x1.
Okay, we can do that change! Could you try to improve the change log a
little bit or you want me to help?
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
From: Ulf Hansson @ 2016-09-22 10:13 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.1609211038270.1996-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
On 21 September 2016 at 16:45, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Wed, 21 Sep 2016, Ulf Hansson wrote:
>
>> > > 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.
>>
>> Hmm, this could be a very good option. In the end the sdmmc/memstick
>> drivers knows best about which autosuspend timeout to use.
>
> I tend to agree, and so does Oliver.
Okay.
>
>> > > 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.
>>
>> I understand it's deliberate, but I was more question whether we
>> actually should have the runtime PM core to behaves like this.
>>
>> I don't think its behaviour is consistent, as in such case all calls
>> to __pm_runtime_resume() should trigger the runtime PM core to update
>> the last busy mark.
>
> Not a bad idea...
Yes, it is. :-)
Although, I am still concerned about he inconsistent behaviour.
>
>> Don't get me wrong, I am *not* suggesting we should do that change, as
>> it would mean the last busy mark would be updated way too often.
>
> The updates aren't very expensive. Just one atomic write. It probably
> takes less time than acquiring the runtime-PM spinlock.
>
>> Instead, perhaps it's better to leave the responsibility of updating
>> the last busy mark to the runtime PM users solely.
>
> Maybe, but I think doing it once in the core, like this, can remove the
> need for a lot of function calls in drivers.
Unfortunate not. Most updates of the last busy mark happens when a
device is no longer required to be runtime resumed. As when a driver
has completed to serve a request and is about to call pm_runtime_put()
(or similar API).
So, I still believe doing it in the runtime PM core is just a waste.
I think it's better to leave the update to the users entirely, it
would become consistent but also more flexible, as one could easily
think of situations where you may in some cases want to update the
last busy mark and in some other not.
>
>> > > 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.
>>
>> Yes, something like that makes sense to me.
>>
>> Unless we decide to turn off autosuspend completely for the usb host
>> as you suggested above. Then it would really become clear that the
>> sdmmc/memstick drivers gets the responsible for the autosuspend, which
>> certainly makes most sense.
>
> Yes.
Okay.
I will submit a series with the relevant changes we have come up with
during the discussions. I will keep you posted.
>
>> > > **) 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.
>>
>> Yes, you are right. I just haven't been able to prioritize doing that
>> change for MMC. Another thing added on my mmc TODO list. :-)
>
> To tell the truth, I'm not sure how you would synchronize the polling
> activities in the sdmmc and memstick drivers. Move most of it into the
> upper MFD driver?
I don't know, you may be right! I will have to think about it.
>
> One point worth mentioning is that if you already know an SDMMC card is
> present then there's no reason to poll for a MemoryStick card, and vice
> versa.
Yes, indeed. Although you still need to poll, as otherwise you
wouldn't detect if it gets removed.
Kind regards
Uffe
--
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 3/5] mmc: core: changes frequency to hs_max_dtr when selecting hs400es
From: Shawn Lin @ 2016-09-22 10:06 UTC (permalink / raw)
To: Ulf Hansson
Cc: shawn.lin, Adrian Hunter, Jaehoon Chung, linux-mmc,
linux-kernel@vger.kernel.org, open list:ARM/Rockchip SoC...
In-Reply-To: <CAPDyKFoHxSpFurgV9swN3MCTWUinPFNUHSzxgyKj+K3iqkGSbA@mail.gmail.com>
Hi ulf,
在 2016/9/22 17:38, Ulf Hansson 写道:
> On 21 September 2016 at 03:43, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> 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.
>
> I don't get this. Are you saying that f_init > 52 MHz? That should not
> be impossible, right!?
nope, I was saying that the spec implies we to set clock after
setting HS_TIMING to 0x1 when doing hs400es selection.
I thought there is no difference because the spec says "Host need to
change frequency to <= 52MHz", and the f_init(<=400k) is <= 52MHz,
right? So I didn't set clock to hs_max_dtr. But I think I misunderstood
the spec, so this patch will fix this.
>
> So either the core has changed the clock rate by mistake at some other
> execution path, or the host driver didn't set the correct clock rate
> the first time when invoked via mmc_power_up()?
>
> Kind regards
> Uffe
>
>>
>> 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
>>
>>
>
>
>
--
Best Regards
Shawn Lin
^ permalink raw reply
* Re: [PATCH 1/5] mmc: core: don't try to switch block size for dual rate mode
From: Ulf Hansson @ 2016-09-22 9:40 UTC (permalink / raw)
To: Shawn Lin
Cc: Adrian Hunter, Jaehoon Chung, linux-mmc,
linux-kernel@vger.kernel.org, open list:ARM/Rockchip SoC...,
Ziyuan Xu
In-Reply-To: <1474422233-29355-2-git-send-email-shawn.lin@rock-chips.com>
On 21 September 2016 at 03:43, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> 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>
Thanks, applied for next!
Kind regards
Uffe
>
> ---
>
> 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
* Re: [RESEND PATCH v7 2/2] sdhci-of-arasan: Set controller to test mode when xlnx,fails-without-test-cd is present
From: Ulf Hansson @ 2016-09-22 9:40 UTC (permalink / raw)
To: Zach Brown
Cc: Adrian Hunter, Rob Herring, Mark Rutland, linux-mmc,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Michal Simek, Sören Brinkmann,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
In-Reply-To: <1474038102-20937-2-git-send-email-zach.brown-acOepvfBmUk@public.gmane.org>
On 16 September 2016 at 17:01, Zach Brown <zach.brown-acOepvfBmUk@public.gmane.org> wrote:
> The sdhci controller on xilinx zynq devices will not function unless
> the CD bit is provided. http://www.xilinx.com/support/answers/61064.html
> In cases where it is impossible to provide the CD bit in hardware,
> setting the controller to test mode and then setting inserted to true
> will get the controller to function without the CD bit.
>
> When the device has the property xlnx,fails-without-test-cd the driver
> changes the controller to test mode and sets test inserted to true to
> make the controller function.
>
> Signed-off-by: Zach Brown <zach.brown-acOepvfBmUk@public.gmane.org>
Thanks, applied for next! I took the liberty to shrink the commit msg
header bit and to add "mmc: " prefix.
Kind regards
Uffe
> ---
> drivers/mmc/host/sdhci-of-arasan.c | 27 ++++++++++++++++++++++++++-
> drivers/mmc/host/sdhci.h | 2 ++
> 2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 33601a8..da8e40a 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -26,6 +26,7 @@
> #include <linux/phy/phy.h>
> #include <linux/regmap.h>
> #include "sdhci-pltfm.h"
> +#include <linux/of.h>
>
> #define SDHCI_ARASAN_CLK_CTRL_OFFSET 0x2c
> #define SDHCI_ARASAN_VENDOR_REGISTER 0x78
> @@ -98,6 +99,10 @@ struct sdhci_arasan_data {
>
> struct regmap *soc_ctl_base;
> const struct sdhci_arasan_soc_ctl_map *soc_ctl_map;
> + unsigned int quirks; /* Arasan deviations from spec */
> +
> +/* Controller does not have CD wired and will not function normally without */
> +#define SDHCI_ARASAN_QUIRK_FORCE_CDTEST BIT(0)
> };
>
> static const struct sdhci_arasan_soc_ctl_map rk3399_soc_ctl_map = {
> @@ -245,12 +250,27 @@ static void sdhci_arasan_hs400_enhanced_strobe(struct mmc_host *mmc,
> writel(vendor, host->ioaddr + SDHCI_ARASAN_VENDOR_REGISTER);
> }
>
> +void sdhci_arasan_reset(struct sdhci_host *host, u8 mask)
> +{
> + u8 ctrl;
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
> +
> + sdhci_reset(host, mask);
> +
> + if (sdhci_arasan->quirks & SDHCI_ARASAN_QUIRK_FORCE_CDTEST) {
> + ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> + ctrl |= SDHCI_CTRL_CDTEST_INS | SDHCI_CTRL_CDTEST_EN;
> + sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> + }
> +}
> +
> static struct sdhci_ops sdhci_arasan_ops = {
> .set_clock = sdhci_arasan_set_clock,
> .get_max_clock = sdhci_pltfm_clk_get_max_clock,
> .get_timeout_clock = sdhci_arasan_get_timeout_clock,
> .set_bus_width = sdhci_set_bus_width,
> - .reset = sdhci_reset,
> + .reset = sdhci_arasan_reset,
> .set_uhs_signaling = sdhci_set_uhs_signaling,
> };
>
> @@ -545,6 +565,7 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
> struct sdhci_host *host;
> struct sdhci_pltfm_host *pltfm_host;
> struct sdhci_arasan_data *sdhci_arasan;
> + struct device_node *np = pdev->dev.of_node;
>
> host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata,
> sizeof(*sdhci_arasan));
> @@ -599,6 +620,10 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
> }
>
> sdhci_get_of_property(pdev);
> +
> + if (of_property_read_bool(np, "xlnx,fails-without-test-cd"))
> + sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_FORCE_CDTEST;
> +
> pltfm_host->clk = clk_xin;
>
> if (of_device_is_compatible(pdev->dev.of_node,
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index a2bc9e1..c722cd2 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -84,6 +84,8 @@
> #define SDHCI_CTRL_ADMA32 0x10
> #define SDHCI_CTRL_ADMA64 0x18
> #define SDHCI_CTRL_8BITBUS 0x20
> +#define SDHCI_CTRL_CDTEST_INS 0x40
> +#define SDHCI_CTRL_CDTEST_EN 0x80
>
> #define SDHCI_POWER_CONTROL 0x29
> #define SDHCI_POWER_ON 0x01
> --
> 2.7.4
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RESEND PATCH v7 1/2] sdhci-of-arasan: Add device tree parameter xlnx,fails-without-test-cd bit
From: Ulf Hansson @ 2016-09-22 9:40 UTC (permalink / raw)
To: Zach Brown
Cc: Adrian Hunter, Rob Herring, Mark Rutland, linux-mmc,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
Michal Simek, Sören Brinkmann,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <1474038102-20937-1-git-send-email-zach.brown@ni.com>
On 16 September 2016 at 17:01, Zach Brown <zach.brown@ni.com> wrote:
> The sdhci controller on xilinx zynq devices will not function unless
> the CD bit is provided. http://www.xilinx.com/support/answers/61064.html
> In cases where it is impossible to provide the CD bit in hardware,
> setting the controller to test mode and then setting inserted to true
> will get the controller to function without the CD bit.
>
> The device property "xlnx,fails-without-test-cd" will let the arasan
> driver know the controller does not have the CD line wired and that the
> controller does not function without it.
>
> Signed-off-by: Zach Brown <zach.brown@ni.com>
Thanks, applied for next! I took the liberty to shrink the commit msg
header bit and to add "dt: " prefix.
Kind regards
Uffe
> ---
> v2:
> * improved commit messages
> * removed fake-cd device property
> * removed fake-cd quirk
> * use broken-cd device property
> * documented new usage of broken-cd
> v3:
> * removed new usage of broken-cd
> * created fails-without-test-cd device property
> * created arasan controller specific quirk
> v4:
> * changed fails-without-test-cd to xlnx-fails-without-test-cd
> * removed extra blank line
> v5:
> * Fixed style mistake
> * Changed (1 << 0 ) to BIT(0)
> v6:
> * Fixed style mistakes
> * Condensed unnecessarily long variable names
> * Removed line wraps that were no longer necessary.
> * Rebased changes off Ulf's mmc tree's next branch.
> v7:
> * Removed erroneous re-creation of Optional Properties section
> * Changed xlnx-fails-without-test-cd to xlnx,fails-without-test-cd
> * Changed of_get_property to of_property_read_bool
>
>
> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> index 3404afa..49df630 100644
> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> @@ -36,6 +36,9 @@ Optional Properties:
> - #clock-cells: If specified this should be the value <0>. With this property
> in place we will export a clock representing the Card Clock. This clock
> is expected to be consumed by our PHY. You must also specify
> + - xlnx,fails-without-test-cd: when present, the controller doesn't work when
> + the CD line is not connected properly, and the line is not connected
> + properly. Test mode can be used to force the controller to function.
>
> Example:
> sdhci@e0100000 {
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH v3 0/6] tmio: add support for eMMC with 8 bit bus width
From: Ulf Hansson @ 2016-09-22 9:40 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-mmc, Linux-Renesas, Simon Horman, Dirk Behme
In-Reply-To: <20160919205750.4766-1-wsa+renesas@sang-engineering.com>
On 19 September 2016 at 22:57, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> This series enables SDHI instances on R-Car Gen3 to access eMMC with 8 bit bus
> width. I think the patch descriptions speak for themselves.
>
> I decided to not protect this new feature with a flag because it needs
> specifically to be enabled by setting the bus width to 8. No legacy platform
> does that.
>
> Note that I decided to use the pattern that pinctrl-0 is 3.3v and
> pinctrl-1 is 1.8v, although the eMMC is fixed at 1.8v. I tried a few ways to
> only use pinctrl-0 being 1.8v here, but they all ended up to be confusing for
> users IMO, so I sticked to the most consistent solution after all.
>
> Changes since V2:
>
> * correct node name for sd2_uhs in both devicetrees
>
> Changes since V1:
>
> * merged the two distinct series (drivers + DTS) into one: patches 1-4 are for
> Ulf, I will ping Simon for patches 5+6 when the time is ready.
>
> * DTS now contains 'non-removable' and we have a software workaround for now.
>
> These patches are based on top of Simon's sdr104-v7 patches but they apply to
> current mmc/next as well. A branch can be found here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/sdhi-8bit-emmc
>
> Please review, comment, apply...
>
> Wolfram
>
> Wolfram Sang (6):
> mmc: add define for R1 response without CRC
> mmc: rtsx_pci: use new macro for R1 without CRC
> mmc: rtsx_usb: use new macro for R1 without CRC
> mmc: tmio: add eMMC support
> arm64: dts: r8a7795: salvator: enable on-board eMMC
> arm64: dts: r8a7796: salvator: enable on board eMMC
>
> arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts | 43 +++++++++++++++++++++
> arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts | 44 +++++++++++++++++++++-
> drivers/mmc/host/rtsx_pci_sdmmc.c | 2 +-
> drivers/mmc/host/rtsx_usb_sdmmc.c | 2 +-
> drivers/mmc/host/tmio_mmc.h | 3 ++
> drivers/mmc/host/tmio_mmc_pio.c | 38 +++++++++++++------
> include/linux/mmc/core.h | 3 ++
> 7 files changed, 120 insertions(+), 15 deletions(-)
>
Thanks, applied patch 1->4 for next. I assume Simon will pick up the
DT changes, sooner or later.
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH v2] mmc: card: do away with indirection pointer
From: Ulf Hansson @ 2016-09-22 9:40 UTC (permalink / raw)
To: Linus Walleij; +Cc: linux-mmc
In-Reply-To: <1474364078-15769-1-git-send-email-linus.walleij@linaro.org>
On 20 September 2016 at 11:34, Linus Walleij <linus.walleij@linaro.org> wrote:
> 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
Thanks, applied for next!
Kind regards
Uffe
>
> 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
* Re: [PATCH] mmc: dw_mmc: fix the spamming log message
From: Ulf Hansson @ 2016-09-22 9:39 UTC (permalink / raw)
To: Jaehoon Chung; +Cc: linux-mmc
In-Reply-To: <1474521120-9797-1-git-send-email-jh80.chung@samsung.com>
On 22 September 2016 at 07:12, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> When there is no Card which is set to "broken-cd", it's displayed a clock
> information continuously. Because it's polling for detecting card.
> This patch is fixed this problem.
>
> Fixed 'commit 65257a0deed5
> ("mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus()")'
>
> Reported-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
Thanks, applied for fixes!
I added a formal fixes tag from above.
Fixes: 65257a0deed5 ("mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus()")
Kind regards
Uffe
> ---
> drivers/mmc/host/dw_mmc.c | 14 +++++++++-----
> drivers/mmc/host/dw_mmc.h | 3 +++
> 2 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 32380d5..767af20 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 was 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..e8cd2de 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 was 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
> --
> 1.9.1
>
^ permalink raw reply
* Re: [PATCH 2/5] mmc: core: switch to 1V8 or 1V2 for hs400es mode
From: Ulf Hansson @ 2016-09-22 9:39 UTC (permalink / raw)
To: Shawn Lin
Cc: Adrian Hunter, Jaehoon Chung, linux-mmc,
linux-kernel@vger.kernel.org, open list:ARM/Rockchip SoC...,
# 4.0+, 4.4#, +
In-Reply-To: <1474422233-29355-3-git-send-email-shawn.lin@rock-chips.com>
On 21 September 2016 at 03:43, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> 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)
/s/EXT_CSD_CARD_TYPE_HS200_1_2V/EXT_CSD_CARD_TYPE_HS400_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)
/s/EXT_CSD_CARD_TYPE_HS200_1_8V/EXT_CSD_CARD_TYPE_HS400_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
>
>
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH 3/5] mmc: core: changes frequency to hs_max_dtr when selecting hs400es
From: Ulf Hansson @ 2016-09-22 9:38 UTC (permalink / raw)
To: Shawn Lin
Cc: Adrian Hunter, Jaehoon Chung, linux-mmc,
linux-kernel@vger.kernel.org, open list:ARM/Rockchip SoC...
In-Reply-To: <1474422233-29355-4-git-send-email-shawn.lin@rock-chips.com>
On 21 September 2016 at 03:43, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> 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.
I don't get this. Are you saying that f_init > 52 MHz? That should not
be impossible, right!?
So either the core has changed the clock rate by mistake at some other
execution path, or the host driver didn't set the correct clock rate
the first time when invoked via mmc_power_up()?
Kind regards
Uffe
>
> 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
* Re: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
From: Ulf Hansson @ 2016-09-22 8:13 UTC (permalink / raw)
To: Chris Brandt
Cc: Wolfram Sang, Sergei Shtylyov, Geert Uytterhoeven, Simon Horman,
linux-mmc, Linux-Renesas, Lee Jones
In-Reply-To: <20160912141507.6837-3-chris.brandt@renesas.com>
+ Lee
On 12 September 2016 at 16:15, Chris Brandt <chris.brandt@renesas.com> wrote:
> For the r7s72100 SOC, the DATA_PORT register was changed to 32-bits wide.
> Therefore a new flag has been created that will allow 32-bit reads/writes
> to the DATA_PORT register instead of 16-bit (because 16-bits accesses are
> not supported).
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
> v3:
> * changed loops to memcpy
> v2:
> * changed 'data * 0xFF' to 'data & 0xFF'
> * added 'const' for sd_ctrl_write32_rep
> ---
> drivers/mmc/host/tmio_mmc.h | 12 ++++++++++++
> drivers/mmc/host/tmio_mmc_pio.c | 30 ++++++++++++++++++++++++++++++
> include/linux/mfd/tmio.h | 5 +++++
This header file needs to be split up. The mmc specific bits, should
be moved to a local header file under driver/mmc/host/*.
Sure, we don't need to do that as part of $subject patch, but then I
need an ack from Lee Jones, the mfd maintainer. I have added him on
cc.
> 3 files changed, 47 insertions(+)
>
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index ecb99fc..a99e634 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -237,6 +237,12 @@ static inline u32 sd_ctrl_read16_and_16_as_32(struct tmio_mmc_host *host, int ad
> readw(host->ctl + ((addr + 2) << host->bus_shift)) << 16;
> }
>
> +static inline void sd_ctrl_read32_rep(struct tmio_mmc_host *host, int addr,
> + u32 *buf, int count)
> +{
> + readsl(host->ctl + (addr << host->bus_shift), buf, count);
> +}
> +
> static inline void sd_ctrl_write16(struct tmio_mmc_host *host, int addr, u16 val)
> {
> /* If there is a hook and it returns non-zero then there
> @@ -259,4 +265,10 @@ static inline void sd_ctrl_write32_as_16_and_16(struct tmio_mmc_host *host, int
> writew(val >> 16, host->ctl + ((addr + 2) << host->bus_shift));
> }
>
> +static inline void sd_ctrl_write32_rep(struct tmio_mmc_host *host, int addr,
> + const u32 *buf, int count)
> +{
> + writesl(host->ctl + (addr << host->bus_shift), buf, count);
> +}
> +
> #endif
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index 017a4dc..59fac7d 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -443,6 +443,36 @@ static void tmio_mmc_transfer_data(struct tmio_mmc_host *host,
> /*
> * Transfer the data
> */
> + if (host->pdata->flags & TMIO_MMC_32BIT_DATA_PORT) {
> + u8 data[4] = { };
> +
> + if (is_read)
> + sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, (u32 *)buf,
> + count >> 2);
> + else
> + sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, (u32 *)buf,
> + count >> 2);
> +
> + /* if count was multiple of 4 */
> + if (!(count & 0x3))
> + return;
> +
> + buf8 = (u8 *)(buf + (count >> 2));
> + count %= 4;
> +
> + if (is_read) {
> + sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT,
> + (u32 *)data, 1);
> + memcpy(buf8, data, count);
> + } else {
> + memcpy(data, buf8, count);
> + sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT,
> + (u32 *)data, 1);
> + }
> +
> + return;
> + }
> +
> if (is_read)
> sd_ctrl_read16_rep(host, CTL_SD_DATA_PORT, buf, count >> 1);
> else
> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
> index 3b95dc7..0dbcb7e 100644
> --- a/include/linux/mfd/tmio.h
> +++ b/include/linux/mfd/tmio.h
> @@ -100,6 +100,11 @@
> #define TMIO_MMC_SDIO_STATUS_QUIRK (1 << 8)
>
> /*
> + * Some controllers have a 32-bit wide data port register
> + */
> +#define TMIO_MMC_32BIT_DATA_PORT (1 << 9)
> +
> +/*
> * Some controllers allows to set SDx actual clock
> */
> #define TMIO_MMC_CLK_ACTUAL (1 << 10)
> --
> 2.9.2
>
>
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH v3 1/2] Documentation: mmc: sdhci-of-arasan: add description of power domain
From: Ulf Hansson @ 2016-09-22 7:14 UTC (permalink / raw)
To: Ziyuan Xu
Cc: Heiko Stübner, Rob Herring, devicetree@vger.kernel.org,
open list:ARM/Rockchip SoC..., linux-mmc, Shawn Lin,
Doug Anderson, zhangqing
In-Reply-To: <1ec66aea-10b4-4208-369e-9ef5feb800df@rock-chips.com>
On 20 September 2016 at 04:29, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
> hi Ulf,
>
>
> On 2016年09月17日 16:49, Ulf Hansson wrote:
>>
>> On 17 September 2016 at 04:32, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>>>
>>> Add power domain as a optional property for sdhci-of-arasan, which can
>>> be turned off in the so-called unused condition, such as suspend and
>>> remove. Aim to lower power requirements.
>>
>> I assume runtime suspend make sense as well?
>
>
> Sorry to reply this mail after a long time.
>
> On rk3399 platform, there is a eMMC-phy which is in charge of eMMC's logic
> interface. We should guarantee that phy is ready before cmd/data transfer.
> I mean that I need to consider about the resume latency, so that I can land
> RPM for arasan-of-sdhci.
> I intend to use auto-suspend for arasan-of-sdhci.
>
Okay!
>>
>> Moreover, perhaps you could share a little information here about what
>> needs to be done at the driver level which controls the mmc IP. More
>> precisely, what do you need to do at runtime_suspend, suspend,
>> runtime_resume, and resume?
>
>
> Invoke sdhci_runtime_suspend_host, disable relevant clock in
> rumtime_suspend, deprocessing in runtime_resume.
> Is that ok?
Okay!
[...]
Thanks for sharing some more information, could you please re-spin
with an updated change log?
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH] mmc: dw_mmc: fix the spamming log message
From: Jaehoon Chung @ 2016-09-22 5:14 UTC (permalink / raw)
To: linux-mmc; +Cc: ulf.hansson
In-Reply-To: <1474521120-9797-1-git-send-email-jh80.chung@samsung.com>
Dear Ulf,
On 09/22/2016 02:12 PM, Jaehoon Chung wrote:
> When there is no Card which is set to "broken-cd", it's displayed a clock
> information continuously. Because it's polling for detecting card.
> This patch is fixed this problem.
Could you pick this on your fixes branch? (Is it too late?)
Best Regards,
Jaehoon Chung
>
> Fixed 'commit 65257a0deed5
> ("mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus()")'
>
> Reported-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
> ---
> drivers/mmc/host/dw_mmc.c | 14 +++++++++-----
> drivers/mmc/host/dw_mmc.h | 3 +++
> 2 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 32380d5..767af20 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 was 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..e8cd2de 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 was 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
>
^ permalink raw reply
* [PATCH] mmc: dw_mmc: fix the spamming log message
From: Jaehoon Chung @ 2016-09-22 5:12 UTC (permalink / raw)
To: linux-mmc; +Cc: ulf.hansson, Jaehoon Chung
When there is no Card which is set to "broken-cd", it's displayed a clock
information continuously. Because it's polling for detecting card.
This patch is fixed this problem.
Fixed 'commit 65257a0deed5
("mmc: dw_mmc: remove UBSAN warning in dw_mci_setup_bus()")'
Reported-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
---
drivers/mmc/host/dw_mmc.c | 14 +++++++++-----
drivers/mmc/host/dw_mmc.h | 3 +++
2 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 32380d5..767af20 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 was 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..e8cd2de 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 was 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
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] mmc: dw_mmc: minor cleanup for dw_mci_adjust_fifoth
From: Jaehoon Chung @ 2016-09-22 4:27 UTC (permalink / raw)
To: Shawn Lin, Ulf Hansson; +Cc: linux-mmc, linux-kernel, linux-rockchip
In-Reply-To: <1474425625-24178-1-git-send-email-shawn.lin@rock-chips.com>
On 09/21/2016 11:40 AM, Shawn Lin wrote:
> msize and rx_wmark are properly initialized, we dont't
> need to assign them again.
Applied on my tree. Thanks!
Best Regards,
Jaehoon Chung
>
> 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]) ||
>
^ permalink raw reply
* Re: [PATCH v2 4/4] mmc: dw_mmc: use macro to define ring buffer size
From: Jaehoon Chung @ 2016-09-22 4:27 UTC (permalink / raw)
To: Shawn Lin
Cc: Ulf Hansson, linux-mmc, linux-kernel, Doug Anderson,
Heiko Stuebner, linux-rockchip
In-Reply-To: <1472789679-15121-4-git-send-email-shawn.lin@rock-chips.com>
On 09/02/2016 01:14 PM, Shawn Lin wrote:
> It's very prone to make mistake as we might forget
> to replace all PAGE_SIZEs with new values if we try
> to modify the ring buffer size for whatever reasons.
> Let's use a macro to define it.
Applied on my tree. Thanks!
Best Regards,
Jaehoon Chung
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
> Changes in v2: None
>
> drivers/mmc/host/dw_mmc.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 833b6ad..6e5926b 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -61,6 +61,8 @@
> SDMMC_IDMAC_INT_FBE | SDMMC_IDMAC_INT_RI | \
> SDMMC_IDMAC_INT_TI)
>
> +#define DESC_RING_BUF_SZ PAGE_SIZE
> +
> struct idmac_desc_64addr {
> u32 des0; /* Control Descriptor */
>
> @@ -474,7 +476,8 @@ static int dw_mci_idmac_init(struct dw_mci *host)
> 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);
> + host->ring_size =
> + DESC_RING_BUF_SZ / sizeof(struct idmac_desc_64addr);
>
> /* Forward link the descriptor list */
> for (i = 0, p = host->sg_cpu; i < host->ring_size - 1;
> @@ -500,7 +503,8 @@ static int dw_mci_idmac_init(struct dw_mci *host)
> } else {
> struct idmac_desc *p;
> /* Number of descriptors in the ring buffer */
> - host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc);
> + host->ring_size =
> + DESC_RING_BUF_SZ / sizeof(struct idmac_desc);
>
> /* Forward link the descriptor list */
> for (i = 0, p = host->sg_cpu;
> @@ -609,7 +613,7 @@ static inline int dw_mci_prepare_desc64(struct dw_mci *host,
> 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);
> + memset(host->sg_cpu, 0, DESC_RING_BUF_SZ);
> dw_mci_idmac_init(host);
> return -EINVAL;
> }
> @@ -685,7 +689,7 @@ static inline int dw_mci_prepare_desc32(struct dw_mci *host,
> 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);
> + memset(host->sg_cpu, 0, DESC_RING_BUF_SZ);
> dw_mci_idmac_init(host);
> return -EINVAL;
> }
> @@ -2750,7 +2754,8 @@ static void dw_mci_init_dma(struct dw_mci *host)
> }
>
> /* Alloc memory for sg translation */
> - host->sg_cpu = dmam_alloc_coherent(host->dev, PAGE_SIZE,
> + host->sg_cpu = dmam_alloc_coherent(host->dev,
> + DESC_RING_BUF_SZ,
> &host->sg_dma, GFP_KERNEL);
> if (!host->sg_cpu) {
> dev_err(host->dev,
>
^ permalink raw reply
* Re: [PATCH v2 3/4] mmc: dw_mmc: fix misleading error print if failing to do DMA transfer
From: Jaehoon Chung @ 2016-09-22 4:26 UTC (permalink / raw)
To: Shawn Lin
Cc: Ulf Hansson, linux-mmc, linux-kernel, Doug Anderson,
Heiko Stuebner, linux-rockchip
In-Reply-To: <1472789679-15121-3-git-send-email-shawn.lin@rock-chips.com>
On 09/02/2016 01:14 PM, Shawn Lin wrote:
> The original log didn't figure out that we could still
> finish this transfer by PIO mode even if failing to use
> DMA. And it should be kept for debug level instead of
> error one.
Applied on my tree. Thanks!
Best Regards,
Jaehoon Chung
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
> Changes in v2: None
>
> drivers/mmc/host/dw_mmc.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index daa1c52..833b6ad 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1057,8 +1057,10 @@ static int dw_mci_submit_data_dma(struct dw_mci *host, struct mmc_data *data)
> spin_unlock_irqrestore(&host->irq_lock, irqflags);
>
> if (host->dma_ops->start(host, sg_len)) {
> - /* We can't do DMA */
> - dev_err(host->dev, "%s: failed to start DMA.\n", __func__);
> + /* We can't do DMA, try PIO for this one */
> + dev_dbg(host->dev,
> + "%s: fall back to PIO mode for current transfer\n",
> + __func__);
> return -ENODEV;
> }
>
>
^ permalink raw reply
* Re: [PATCH v2 2/4] mmc: dw_mmc: avoid race condition of cpu and IDMAC
From: Jaehoon Chung @ 2016-09-22 4:26 UTC (permalink / raw)
To: Shawn Lin
Cc: 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>
On 09/02/2016 01:14 PM, 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.
Applied on my tree. Thanks!
Best Regards,
Jaehoon Chung
>
> 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 1/4] mmc: dw_mmc: split out preparation of desc for IDMAC32 and IDMAC64
From: Jaehoon Chung @ 2016-09-22 4:25 UTC (permalink / raw)
To: Shawn Lin
Cc: Ulf Hansson, linux-mmc, linux-kernel, Doug Anderson,
Heiko Stuebner, linux-rockchip
In-Reply-To: <1472789679-15121-1-git-send-email-shawn.lin@rock-chips.com>
On 09/02/2016 01:14 PM, Shawn Lin wrote:
> We intend to add more check for descriptors when
> preparing desc. Let's spilt out the separate body
> to make the dw_mci_translate_sglist not so lengthy.
> After spliting out these two functions, we could
> remove dw_mci_translate_sglist and call both of them
> when staring idmac.
Applied on my tree. Thanks!
Best Regards,
Jaehoon Chung
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>
> ---
>
> Changes in v2:
> - remove dw_mci_translate_sglist
>
> drivers/mmc/host/dw_mmc.c | 149 ++++++++++++++++++++++++----------------------
> 1 file changed, 79 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 22dacae..782b303 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -467,112 +467,121 @@ static void dw_mci_dmac_complete_dma(void *arg)
> }
> }
>
> -static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data,
> - unsigned int sg_len)
> +static inline void 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;
> int i;
>
> - if (host->dma_64bit_address == 1) {
> - struct idmac_desc_64addr *desc_first, *desc_last, *desc;
> -
> - desc_first = desc_last = desc = host->sg_cpu;
> + desc_first = desc_last = desc = host->sg_cpu;
>
> - for (i = 0; i < sg_len; i++) {
> - unsigned int length = sg_dma_len(&data->sg[i]);
> + for (i = 0; i < sg_len; i++) {
> + unsigned int length = sg_dma_len(&data->sg[i]);
>
> - u64 mem_addr = sg_dma_address(&data->sg[i]);
> + u64 mem_addr = sg_dma_address(&data->sg[i]);
>
> - for ( ; length ; desc++) {
> - desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ?
> - length : DW_MCI_DESC_DATA_LENGTH;
> + for ( ; length ; desc++) {
> + desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ?
> + length : DW_MCI_DESC_DATA_LENGTH;
>
> - length -= desc_len;
> + length -= desc_len;
>
> - /*
> - * Set the OWN bit and disable interrupts
> - * for this descriptor
> - */
> - desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC |
> - IDMAC_DES0_CH;
> + /*
> + * Set the OWN bit and disable interrupts
> + * for this descriptor
> + */
> + desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC |
> + IDMAC_DES0_CH;
>
> - /* Buffer length */
> - IDMAC_64ADDR_SET_BUFFER1_SIZE(desc, desc_len);
> + /* Buffer length */
> + IDMAC_64ADDR_SET_BUFFER1_SIZE(desc, desc_len);
>
> - /* Physical address to DMA to/from */
> - desc->des4 = mem_addr & 0xffffffff;
> - desc->des5 = mem_addr >> 32;
> + /* Physical address to DMA to/from */
> + desc->des4 = mem_addr & 0xffffffff;
> + desc->des5 = mem_addr >> 32;
>
> - /* Update physical address for the next desc */
> - mem_addr += desc_len;
> + /* Update physical address for the next desc */
> + mem_addr += desc_len;
>
> - /* Save pointer to the last descriptor */
> - desc_last = desc;
> - }
> + /* Save pointer to the last descriptor */
> + desc_last = desc;
> }
> + }
>
> - /* Set first descriptor */
> - desc_first->des0 |= IDMAC_DES0_FD;
> + /* Set first descriptor */
> + desc_first->des0 |= IDMAC_DES0_FD;
>
> - /* Set last descriptor */
> - desc_last->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC);
> - desc_last->des0 |= IDMAC_DES0_LD;
> + /* Set last descriptor */
> + desc_last->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC);
> + desc_last->des0 |= IDMAC_DES0_LD;
> +}
>
> - } else {
> - struct idmac_desc *desc_first, *desc_last, *desc;
>
> - desc_first = desc_last = desc = host->sg_cpu;
> +static inline void 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;
> + int i;
>
> - for (i = 0; i < sg_len; i++) {
> - unsigned int length = sg_dma_len(&data->sg[i]);
> + desc_first = desc_last = desc = host->sg_cpu;
>
> - u32 mem_addr = sg_dma_address(&data->sg[i]);
> + for (i = 0; i < sg_len; i++) {
> + unsigned int length = sg_dma_len(&data->sg[i]);
>
> - for ( ; length ; desc++) {
> - desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ?
> - length : DW_MCI_DESC_DATA_LENGTH;
> + u32 mem_addr = sg_dma_address(&data->sg[i]);
>
> - length -= desc_len;
> + for ( ; length ; desc++) {
> + desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ?
> + length : DW_MCI_DESC_DATA_LENGTH;
>
> - /*
> - * Set the OWN bit and disable interrupts
> - * for this descriptor
> - */
> - desc->des0 = cpu_to_le32(IDMAC_DES0_OWN |
> - IDMAC_DES0_DIC |
> - IDMAC_DES0_CH);
> + length -= desc_len;
>
> - /* Buffer length */
> - IDMAC_SET_BUFFER1_SIZE(desc, desc_len);
> + /*
> + * Set the OWN bit and disable interrupts
> + * for this descriptor
> + */
> + desc->des0 = cpu_to_le32(IDMAC_DES0_OWN |
> + IDMAC_DES0_DIC |
> + IDMAC_DES0_CH);
>
> - /* Physical address to DMA to/from */
> - desc->des2 = cpu_to_le32(mem_addr);
> + /* Buffer length */
> + IDMAC_SET_BUFFER1_SIZE(desc, desc_len);
>
> - /* Update physical address for the next desc */
> - mem_addr += desc_len;
> + /* Physical address to DMA to/from */
> + desc->des2 = cpu_to_le32(mem_addr);
>
> - /* Save pointer to the last descriptor */
> - desc_last = desc;
> - }
> - }
> -
> - /* Set first descriptor */
> - desc_first->des0 |= cpu_to_le32(IDMAC_DES0_FD);
> + /* Update physical address for the next desc */
> + mem_addr += desc_len;
>
> - /* Set last descriptor */
> - desc_last->des0 &= cpu_to_le32(~(IDMAC_DES0_CH |
> - IDMAC_DES0_DIC));
> - desc_last->des0 |= cpu_to_le32(IDMAC_DES0_LD);
> + /* Save pointer to the last descriptor */
> + desc_last = desc;
> + }
> }
>
> - wmb(); /* drain writebuffer */
> + /* Set first descriptor */
> + desc_first->des0 |= cpu_to_le32(IDMAC_DES0_FD);
> +
> + /* Set last descriptor */
> + desc_last->des0 &= cpu_to_le32(~(IDMAC_DES0_CH |
> + IDMAC_DES0_DIC));
> + desc_last->des0 |= cpu_to_le32(IDMAC_DES0_LD);
> }
>
> static int dw_mci_idmac_start_dma(struct dw_mci *host, unsigned int sg_len)
> {
> u32 temp;
>
> - dw_mci_translate_sglist(host, host->data, sg_len);
> + if (host->dma_64bit_address == 1)
> + dw_mci_prepare_desc64(host, host->data, sg_len);
> + else
> + dw_mci_prepare_desc32(host, host->data, sg_len);
> +
> + /* drain writebuffer */
> + wmb();
>
> /* Make sure to reset DMA in case we did PIO before this */
> dw_mci_ctrl_reset(host, SDMMC_CTRL_DMA_RESET);
>
^ permalink raw reply
* Re: [PATCH v3 1/6] mmc: add define for R1 response without CRC
From: Jaehoon Chung @ 2016-09-22 3:58 UTC (permalink / raw)
To: Wolfram Sang
Cc: Wolfram Sang, linux-mmc, linux-renesas-soc, Simon Horman,
Dirk Behme
In-Reply-To: <20160921060033.GD1484@katana>
On 09/21/2016 03:00 PM, Wolfram Sang wrote:
> On Tue, Sep 20, 2016 at 06:25:37PM +0900, Jaehoon Chung wrote:
>> Hi Wolfram,
>>
>> On 09/20/2016 05:57 AM, Wolfram Sang wrote:
>>> The core uses it for polling. Give drivers a proper define handle this
>>> case like for other response types.
>>>
>>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>> ---
>>> include/linux/mmc/core.h | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
>>> index b01e77de1a74de..4caee099b63a28 100644
>>> --- a/include/linux/mmc/core.h
>>> +++ b/include/linux/mmc/core.h
>>> @@ -55,6 +55,9 @@ struct mmc_command {
>>> #define MMC_RSP_R6 (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
>>> #define MMC_RSP_R7 (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
>>>
>>> +/* Can be used by core to poll after switch to MMC HS mode */
>>> +#define MMC_RSP_R1_NO_CRC (MMC_RSP_PRESENT|MMC_RSP_OPCODE)
>>
>> MMC_RSP_RQ_NO_CRC is described at Specification?
>
> Yes. JEDEC eMMC Spec v5.1, chapter 6.6.2, in the note:
>
> "NOTE While the actual timing change is done, the behavior of any
> command sent (like CMD13) cannot be guaranteed due to the asynchronous
> operation. Therefore it is not recommended to use CMD13 to check the
> busy completion of the timing change indication. In case CMD13 is used
> the host must ignore CRC errors, if appear."
Thanks for noticing this. I read the spec again.
Best Regards,
Jaehoon Chung
>
> And the MMC core does that. In mmc_ops.c:
>
> 70 if (ignore_crc)
> 71 cmd.flags &= ~MMC_RSP_CRC;
>
> And ignore_crc is used in mmc.c (last 'true' parameter):
>
> 1023 err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> 1024 EXT_CSD_HS_TIMING, EXT_CSD_TIMING_HS,
> 1025 card->ext_csd.generic_cmd6_time,
> 1026 true, false, true);
>
> This is why I put the comment above the define.
>
^ 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