Re: Fwd: [PATCH v3 0/5] mmc: Add access to RPMB partition
On Wed, 2012-11-14 at 12:58 -0800, Krishna Konda wrote: > From: Loic Pallardy > Date: Mon, Aug 6, 2012 at 8:12 AM > Subject: [PATCH v3 0/5] mmc: Add access to RPMB partition > To: linux-mmc@vger.kernel.org, Chris Ball > Cc: Linus Walleij , STEricsson_nomadik_linux > , Ulf Hansson > , Loic Pallardy > > > > The goal of this patchserie is to offer access to MMC RPMB > (Replay Protected Memory Block) partition. > The RPMB partition is used in general to store some secure data. > It is accessible through a trusted mechanism described in > JEDEC standard JESD84-A441. > > This patchserie proposes following modifications: > - detect RPMB capability and create RPMB block device if supported > - extend MMC sysfs to provide access to RPMB partition size and > reliable write sector count (information needed by user space to > acces RPMB partition) > - update IOCTL to support RPMB access. This includes automatic > partition > switch and sending of Set Block Count (CMD23) message. > > RPMB partition becomes accessible using standard IOCTL interface. > Patches don't include trusted mechanism or any verification. > It is user space or secure application responsability to provide the > right > command and the entire data frame as defined by JEDEC standard. > --- > Changes in v2: > - Correction in patch 2: mmc: card: Do not scan RPMB partitions > Remove GENHD_FL_EXT_DEVT flag > > Changes in v3: > - Add acked-by and reviewed-by tags > --- > Loic Pallardy (5): > mmc: core: Expose access to RPMB partition > mmc: card: Do not scan RPMB partitions > mmc: core: Extend sysfs to ext_csd parameters for RPMB support > mmc: core: Add mmc_set_blockcount feature > mmc: card: Add RPMB support in IOCTL interface > > Documentation/mmc/mmc-dev-attrs.txt | 7 > drivers/mmc/card/block.c| 66 > + > drivers/mmc/core/core.c | 14 > drivers/mmc/core/mmc.c | 15 + > include/linux/mmc/card.h| 2 ++ > include/linux/mmc/core.h| 2 ++ > include/linux/mmc/mmc.h | 2 ++ > 7 files changed, 108 insertions(+) > Hi Loic, Chris, are there any plans to merge these patchsets? I did not see this in mmc-next. Thanks, Krishna Konda -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] mmc: fix async request mechanism for sequential read scenarios
Hello Per,
On 11/13/2012 11:10 PM, Per Forlin wrote:
> On Tue, Oct 30, 2012 at 1:19 PM, Konstantin Dorfman
> wrote:
>> Hello,
>>
>> On 10/29/2012 11:40 PM, Per Forlin wrote:
>>> Hi,
>>>
>>> I would like to move the focus of my concerns from root cause analysis
>>> to the actual patch,
>>> My first reflection is that the patch is relatively complex and some
>>> of the code looks duplicated.
>>> Would it be possible to simplify it and re-use the current execution flow.
>>>
>>> Is the following flow feasible?
>>>
>>> in mmc_start_req():
>>> --
>>> if (rqc == NULL && not_resend)
>>> wait_for_both_mmc_and_arrival_of_new_req
>>> else
>>> wait_only_for_mmc
>>>
>>> if (arrival_of_new_req) {
>>>set flag to indicate fetch-new_req
>>> return NULL;
>>> }
>>> -
>>>
>>> in queue.c:
>>> if (fetch-new_req)
>>> don't overwrite previous req.
>>>
>>> BR
>>> Per
>>
>> You are talking about using both waiting mechanisms, old (wait on
>> completion) and new - wait_event_interruptible()? But how done()
>> callback, called on host controller irq context, will differentiate
>> which one is relevant for the request?
>>
>> I think it is better to use single wait point for mmc thread.
>
> I have sketch a patch to better explain my point. It's not tested it
> barely compiles :)
> The patch tries to introduce your feature and still keep the same code
> path. And it exposes an API that could be used by SDIO as well.
> The intention of my sketch patch is only to explain what I tried to
> visualize in the pseudo code previously in this thread.
> The out come of your final patch should be documented here I think:
> Documentation/mmc/mmc-async-req.txt
This document is ready, attaching it to this mail and will be included
in next version of the patch (or RESEND).
>
> Here follows my patch sketch:
>
> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
> index e360a97..08036a1 100644
> --- a/drivers/mmc/card/queue.c
> +++ b/drivers/mmc/card/queue.c
> @@ -66,6 +66,8 @@ static int mmc_queue_thread(void *d)
> spin_unlock_irq(q->queue_lock);
>
> if (req || mq->mqrq_prev->req) {
> + if (!req)
> + mmc_prefetch_start(&mq->mqrq_prev->mmc_active,
> true);
> set_current_state(TASK_RUNNING);
> mq->issue_fn(mq, req);
> } else {
> @@ -79,11 +81,13 @@ static int mmc_queue_thread(void *d)
> }
>
> /* Current request becomes previous request and vice versa. */
> - mq->mqrq_prev->brq.mrq.data = NULL;
> - mq->mqrq_prev->req = NULL;
> - tmp = mq->mqrq_prev;
> - mq->mqrq_prev = mq->mqrq_cur;
> - mq->mqrq_cur = tmp;
> + if (!mmc_prefetch_pending(&mq->mqrq_prev->mmc_active)) {
> + mq->mqrq_prev->brq.mrq.data = NULL;
> + mq->mqrq_prev->req = NULL;
> + tmp = mq->mqrq_prev;
> + mq->mqrq_prev = mq->mqrq_cur;
> + mq->mqrq_cur = tmp;
> + }
> } while (1);
> up(&mq->thread_sem);
>
> @@ -109,10 +113,44 @@ static void mmc_request_fn(struct request_queue *q)
> return;
> }
>
> + if (mq->prefetch_enable) {
> + spin_lock(&mq->prefetch_lock);
> + if (mq->prefetch_completion)
> + complete(mq->prefetch_completion);
> + mq->prefetch_pending = true;
> + spin_unlock(&mq->prefetch_lock);
> + }
> +
> if (!mq->mqrq_cur->req && !mq->mqrq_prev->req)
> wake_up_process(mq->thread);
> }
>
> +static void mmc_req_init(struct mmc_async_req *areq, struct completion
> *compl)
> +{
> + struct mmc_queue *mq =
> + container_of(areq->prefetch, struct mmc_queue, prefetch);
> +
> + spin_lock(&mq->prefetch_lock);
> + mq->prefetch_completion = compl;
> + if (mq->prefetch_pending)
> + complete(mq->prefetch_completion);
> + spin_unlock(&mq->prefetch_lock);
> +}
> +
> +static void mmc_req_start(struct mmc_async_req *areq, bool enable)
> +{
> + struct mmc_queue *mq =
> + container_of(areq->prefetch, struct mmc_queue, prefetch);
> + mq->prefetch_enable = enable;
> +}
> +
> +static bool mmc_req_pending(struct mmc_async_req *areq)
> +{
> + struct mmc_queue *mq =
> + container_of(areq->prefetch, struct mmc_queue, prefetch);
> + return mq->prefetch_pending;
> +}
> +
> static struct scatterlist *mmc_alloc_sg(int sg_len, int *err)
> {
> struct scatterlist *sg;
> @@ -166,6 +204,12 @@ int mmc_init_queue(struct mmc_queue *mq, struct
> mmc_card *card,
> int ret;
> struct mmc_queue_req *mqrq_cur = &mq->mqrq[0];
> struct mmc_queue_req *mqrq_prev = &mq->mqrq[1];
> + spin_lock_init(&mq->prefetch_lock);
> + mq->prefetch.wai
[PATCH] mmc: Standardise capability type
There are discrepancies with regards to how MMC capabilities
are carried throughout the subsystem. Let's standardise them
to elevate any confusion.
Cc: Chris Ball
Cc: linux-mmc@vger.kernel.org
Signed-off-by: Lee Jones
---
drivers/mmc/core/mmc.c |2 +-
include/linux/mmc/dw_mmc.h |4 ++--
include/linux/mmc/host.h|4 ++--
include/linux/mmc/sdhci.h |4 ++--
include/linux/platform_data/pxa_sdhci.h |4 ++--
5 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 7cc4638..8c2fa80 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -239,7 +239,7 @@ static void mmc_select_card_type(struct mmc_card *card)
{
struct mmc_host *host = card->host;
u8 card_type = card->ext_csd.raw_card_type & EXT_CSD_CARD_TYPE_MASK;
- unsigned int caps = host->caps, caps2 = host->caps2;
+ u32 caps = host->caps, caps2 = host->caps2;
unsigned int hs_max_dtr = 0;
if (card_type & EXT_CSD_CARD_TYPE_26)
diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index 7c6a113..f825379 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h
@@ -229,8 +229,8 @@ struct dw_mci_board {
u32 quirks; /* Workaround / Quirk flags */
unsigned int bus_hz; /* Clock speed at the cclk_in pad */
- unsigned int caps; /* Capabilities */
- unsigned int caps2; /* More capabilities */
+ u32 caps; /* Capabilities */
+ u32 caps2; /* More capabilities */
/*
* Override fifo depth. If 0, autodetect it from the FIFOTH register,
* but note that this may not be reliable after a bootloader has used
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 7abb0e1..37442b2 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -211,7 +211,7 @@ struct mmc_host {
#define MMC_VDD_34_35 0x0040 /* VDD voltage 3.4 ~ 3.5 */
#define MMC_VDD_35_36 0x0080 /* VDD voltage 3.5 ~ 3.6 */
- unsigned long caps; /* Host capabilities */
+ u32 caps; /* Host capabilities */
#define MMC_CAP_4_BIT_DATA (1 << 0)/* Can the host do 4 bit
transfers */
#define MMC_CAP_MMC_HIGHSPEED (1 << 1)/* Can do MMC high-speed timing
*/
@@ -241,7 +241,7 @@ struct mmc_host {
#define MMC_CAP_CMD23 (1 << 30) /* CMD23 supported. */
#define MMC_CAP_HW_RESET (1 << 31) /* Hardware reset */
- unsigned intcaps2; /* More host capabilities */
+ u32 caps2; /* More host capabilities */
#define MMC_CAP2_BOOTPART_NOACC(1 << 0)/* Boot partition no
access */
#define MMC_CAP2_CACHE_CTRL(1 << 1)/* Allow cache control */
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index fa8529a..c76b4a3 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -157,8 +157,8 @@ struct sdhci_host {
struct timer_list timer;/* Timer for timeouts */
- unsigned int caps; /* Alternative CAPABILITY_0 */
- unsigned int caps1; /* Alternative CAPABILITY_1 */
+ u32 caps; /* Alternative CAPABILITY_0 */
+ u32 caps1; /* Alternative CAPABILITY_1 */
unsigned intocr_avail_sdio; /* OCR bit masks */
unsigned intocr_avail_sd;
diff --git a/include/linux/platform_data/pxa_sdhci.h
b/include/linux/platform_data/pxa_sdhci.h
index 59acd98..0d75008 100644
--- a/include/linux/platform_data/pxa_sdhci.h
+++ b/include/linux/platform_data/pxa_sdhci.h
@@ -48,8 +48,8 @@ struct sdhci_pxa_platdata {
unsigned intext_cd_gpio;
boolext_cd_gpio_invert;
unsigned intmax_speed;
- unsigned inthost_caps;
- unsigned inthost_caps2;
+ u32 host_caps;
+ u32 host_caps2;
unsigned intquirks;
unsigned intpm_caps;
};
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators
2012/11/14 Philip Rakity : > > On Nov 14, 2012, at 8:57 AM, Kevin Liu wrote: > >> 2012/11/14 Mark Brown : >>> On Wed, Nov 14, 2012 at 04:36:28PM +0800, Kevin Liu wrote: 2012/11/14 Mark Brown : >>> > Should this be regulator_set_voltage_tol()? Otherwise it'd be good to > explain where the numbers come from. >>> In SD physical layer spec 3.01 chapter 6.6.1, the threshold level for voltage range is defined as below: Vdd(min) = 2.7V while Vdd(max) = 3.6V. The card should work within the voltage range. >>> If you are afraid the voltage value is too aggressive, maybe we can use regulator_set_voltage_tol() to set a smaller range. But which range should be reasonable? >>> >>> The above makes total sense - thanks! I just wasn't aware that the >>> range was specified in this fashion in the spec. Might be worth a >>> comment in the code if you need to respin. >> >> Sure, I will update the patch. Thanks! >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > re-read spec. Please apply Kevin;s patch. > > Reviewed-by: Philip Rakity Philip, thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators
On Nov 14, 2012, at 8:57 AM, Kevin Liu wrote: > 2012/11/14 Mark Brown : >> On Wed, Nov 14, 2012 at 04:36:28PM +0800, Kevin Liu wrote: >>> 2012/11/14 Mark Brown : >> Should this be regulator_set_voltage_tol()? Otherwise it'd be good to explain where the numbers come from. >> >>> In SD physical layer spec 3.01 chapter 6.6.1, the threshold level for >>> voltage range is defined as below: >>> Vdd(min) = 2.7V while Vdd(max) = 3.6V. >>> The card should work within the voltage range. >> >>> If you are afraid the voltage value is too aggressive, maybe we can >>> use regulator_set_voltage_tol() to set a smaller range. >>> But which range should be reasonable? >> >> The above makes total sense - thanks! I just wasn't aware that the >> range was specified in this fashion in the spec. Might be worth a >> comment in the code if you need to respin. > > Sure, I will update the patch. Thanks! > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html re-read spec. Please apply Kevin;s patch. Reviewed-by: Philip Rakity -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: FW: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators
2012/11/14 Mark Brown : > On Wed, Nov 14, 2012 at 04:36:28PM +0800, Kevin Liu wrote: >> 2012/11/14 Mark Brown : > >> > Should this be regulator_set_voltage_tol()? Otherwise it'd be good to >> > explain where the numbers come from. > >> In SD physical layer spec 3.01 chapter 6.6.1, the threshold level for >> voltage range is defined as below: >> Vdd(min) = 2.7V while Vdd(max) = 3.6V. >> The card should work within the voltage range. > >> If you are afraid the voltage value is too aggressive, maybe we can >> use regulator_set_voltage_tol() to set a smaller range. >> But which range should be reasonable? > > The above makes total sense - thanks! I just wasn't aware that the > range was specified in this fashion in the spec. Might be worth a > comment in the code if you need to respin. Sure, I will update the patch. Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: FW: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators
On Wed, Nov 14, 2012 at 04:36:28PM +0800, Kevin Liu wrote: > 2012/11/14 Mark Brown : > > Should this be regulator_set_voltage_tol()? Otherwise it'd be good to > > explain where the numbers come from. > In SD physical layer spec 3.01 chapter 6.6.1, the threshold level for > voltage range is defined as below: > Vdd(min) = 2.7V while Vdd(max) = 3.6V. > The card should work within the voltage range. > If you are afraid the voltage value is too aggressive, maybe we can > use regulator_set_voltage_tol() to set a smaller range. > But which range should be reasonable? The above makes total sense - thanks! I just wasn't aware that the range was specified in this fashion in the spec. Might be worth a comment in the code if you need to respin. signature.asc Description: Digital signature
Re: FW: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators
2012/11/14 Mark Brown : > On Wed, Nov 14, 2012 at 03:11:37PM +0800, Kevin Liu wrote: > >> - ret = regulator_set_voltage(host->vqmmc, 330, 330); >> + ret = regulator_set_voltage(host->vqmmc, 270, 360); > > Should this be regulator_set_voltage_tol()? Otherwise it'd be good to > explain where the numbers come from. In SD physical layer spec 3.01 chapter 6.6.1, the threshold level for voltage range is defined as below: Vdd(min) = 2.7V while Vdd(max) = 3.6V. The card should work within the voltage range. If you are afraid the voltage value is too aggressive, maybe we can use regulator_set_voltage_tol() to set a smaller range. But which range should be reasonable? >> + ret = regulator_is_supported_voltage(host->vmmc, 170, >> + 195); > > We should really add a regulator_is_supported_voltage_tol... let me > just do that. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 2/2] mmc: support packed write command for eMMC4.5 device
Hi Chris, The amount of improvement from the packed commands, as from any other eMMC4.5 feature, depends on several parameters: 1. The card support of this feature. If the card supports only the feature interface, then you'll see no improvement when using the feature. 2. The benchmark tool used. Since the packed command preparation is stopped due to a FLUSH request, a benchmark that issues many FLUSH requests can result in a small amount of packing and you will see no improvement. You can use the following patch to get the packed commands statistics: http://marc.info/?l=linux-mmc&m=134374508625826&w=2 With this patch you will be able to see the amount of packing and what caused the packed preparation to stop. We tested the packed commands feature with SanDisk cards and got improvement of 30% when using lmdd and tiotest. We don't use iozone for sequential tests but if you'll send me the exact command that you use we can try it as well. It is true that packed commands can cause degradation of read in read-write collisions. However, it is only nature that when having longer write request a read request has to wait for a longer time and its latency will increase. I believe that it is not our duty to decide if this is a reason to exclude this feature. Everyone should take its own decision if he wants to benefit from the write improvement, while risking the read-write collisions scenarios. eMMC4.5 introduces the HPI and stop transmission to overcome the degradation of read latency due to write (regardless of the packed commands). The packing control is our own enhancement that we believe can also be used to overcome this degradation. It is tunable and requires a specific enabling, so it can be the developers decision whether to use it or not. Since it is not a standard feature we can discuss separately if it should be accepted or not and what is the best way to use it. Packed commands is not the only eMMC4.5 feature that can cause degradation in specific scenarios. If we will look at the cache feature, it causes degradation by almost a half in random operations when FLUSH is being used. When using the following iozone command when cache is enabled, you will see degradation in the iozone results: ./data/iozone -i0 -i2 -r4k -s50m -O -o -I -f /data/mmc0/file3 However, cache support was accepted regardless of this degradation and it is the developers responsibility to decide if to use this feature or not. To summarize, all eMMC4.5 features that were added are tunable and disabled by default. I believe that when someone would enable a certain feature he will do all the required testing for determining if he can benefit from this feature or not in his own environment. Thanks, Maya On Tue, November 13, 2012 6:54 pm, Chris Ball wrote: > Hi Maya, > > On Sun, Nov 04 2012, me...@codeaurora.org wrote: >> Packed commands is a mandatory eMMC4.5 feature and is supported by all the card vendors. > > We're still only talking about using packed writes, though, right? > >> It wa proven to be beneficial for eMMC4.5 cards and harmless for non eMMC4.5 cards. > > My understanding is that write packing causes a regression in read performance that can be tuned/fixed by your num_wr_reqs_to_start_packing tunable (and read packing causes a read regression with current eMMC 4.5 cards). Is that wrong? > >> I don't see a point to hold it back while it can be enabled or >> disabled by a flag and most of the code it adds is guarded in specific functions and is not active when packed commands is disabled. > > Earlier in the thread I wrote: > >>> * I still don't have a good set of representative benchmarks showing >>> what kind of performance changes come with this patchset. It seems like we've had a small amount of testing on one controller/eMMC part combo from Seungwon, and an entirely different test from Maya, and the results aren't documented fully anywhere to the level of describing what the hardware was, what the test was, and what the results were before and after the patchset. > > I still feel this way. I'm worried that we might be merging code that works well on your controller/card but causes large regressions for everyone else. I don't want to handle this by making a tunable that everyone has to tune for their system, because I don't think anyone will tune it. I don't think that shipping a capability that will probably lead to performance regressions if you turn it on is a good idea. > > I'm in a better position to help now, though -- I have some motherboards with Marvell SoCs and a socketed eMMC slot, and I have eMMC 4.5 parts from Sandisk and Toshiba. So I can try to help work out how > generalizable your results are across other controllers and cards. > > So far I've only tried the Sandisk part, but it didn't show any write improvement with write packing. I've verified that the switch command to turn on packed_event_en happens and succeeds, and that the caps are set correctly, so I'm not sure what's wrong yet. Wit
