* Re: [PATCH V7 12/25] mmc: block: Disable Command Queue while RPMB is used
From: Ritesh Harjani @ 2016-11-28 4:46 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ulf Hansson, linux-mmc, Alex Lemberg, Mateusz Nowak,
Yuliy Izrailov, Jaehoon Chung, Dong Aisheng, Das Asutosh,
Zhangfei Gao, Dorfman Konstantin, David Griego, Sahitya Tummala,
Venu Byravarasu, Linus Walleij
In-Reply-To: <1480068442-5169-13-git-send-email-adrian.hunter@intel.com>
On 11/25/2016 3:37 PM, Adrian Hunter wrote:
> RPMB does not allow Command Queue commands. Disable and re-enable the
> Command Queue when switching.
>
> Note that the driver only switches partitions when the queue is empty.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Minor comment.
Reviewed-by: Harjani Ritesh <riteshh@codeaurora.org>
Regards
Ritesh
> ---
> drivers/mmc/card/block.c | 46 ++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 38 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index f22df69823cc..157d1b3d58d6 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -746,10 +746,41 @@ static int mmc_blk_compat_ioctl(struct block_device *bdev, fmode_t mode,
> #endif
> };
>
> +static int mmc_blk_part_switch_pre(struct mmc_card *card,
> + unsigned int part_type)
> +{
> + int ret;
int ret = 0;
> +
> + if (part_type == EXT_CSD_PART_CONFIG_ACC_RPMB) {
> + if (card->ext_csd.cmdq_en) {
> + ret = mmc_cmdq_disable(card);
> + if (ret)
> + return ret;
> + }
> + mmc_retune_pause(card->host);
> + }
> +
> + return 0;
Here return ret;
> +}
> +
> +static int mmc_blk_part_switch_post(struct mmc_card *card,
> + unsigned int part_type)
> +{
> + int ret = 0;
> +
> + if (part_type == EXT_CSD_PART_CONFIG_ACC_RPMB) {
> + mmc_retune_unpause(card->host);
> + if (card->reenable_cmdq && !card->ext_csd.cmdq_en)
> + ret = mmc_cmdq_enable(card);
> + }
> +
> + return ret;
> +}
> +
> static inline int mmc_blk_part_switch(struct mmc_card *card,
> struct mmc_blk_data *md)
> {
> - int ret;
> + int ret = 0;
> struct mmc_blk_data *main_md = dev_get_drvdata(&card->dev);
>
> if (main_md->part_curr == md->part_type)
> @@ -758,8 +789,9 @@ static inline int mmc_blk_part_switch(struct mmc_card *card,
> if (mmc_card_mmc(card)) {
> u8 part_config = card->ext_csd.part_config;
>
> - if (md->part_type == EXT_CSD_PART_CONFIG_ACC_RPMB)
> - mmc_retune_pause(card->host);
> + ret = mmc_blk_part_switch_pre(card, md->part_type);
> + if (ret)
> + return ret;
>
> part_config &= ~EXT_CSD_PART_CONFIG_ACC_MASK;
> part_config |= md->part_type;
> @@ -768,19 +800,17 @@ static inline int mmc_blk_part_switch(struct mmc_card *card,
> EXT_CSD_PART_CONFIG, part_config,
> card->ext_csd.part_time);
> if (ret) {
> - if (md->part_type == EXT_CSD_PART_CONFIG_ACC_RPMB)
> - mmc_retune_unpause(card->host);
> + mmc_blk_part_switch_post(card, md->part_type);
> return ret;
> }
>
> card->ext_csd.part_config = part_config;
>
> - if (main_md->part_curr == EXT_CSD_PART_CONFIG_ACC_RPMB)
> - mmc_retune_unpause(card->host);
> + ret = mmc_blk_part_switch_post(card, main_md->part_curr);
> }
>
> main_md->part_curr = md->part_type;
> - return 0;
> + return ret;
> }
>
> static u32 mmc_sd_num_wr_blocks(struct mmc_card *card)
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH V7 13/25] mmc: core: Do not prepare a new request twice
From: Ritesh Harjani @ 2016-11-28 4:48 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ulf Hansson, linux-mmc, Alex Lemberg, Mateusz Nowak,
Yuliy Izrailov, Jaehoon Chung, Dong Aisheng, Das Asutosh,
Zhangfei Gao, Dorfman Konstantin, David Griego, Sahitya Tummala,
Venu Byravarasu, Linus Walleij
In-Reply-To: <1480068442-5169-14-git-send-email-adrian.hunter@intel.com>
On 11/25/2016 3:37 PM, Adrian Hunter wrote:
> mmc_start_req() assumes it is never called with the new request already
> prepared. That is true if the queue consists of only 2 requests, but is not
> true for a longer queue. e.g. mmc_start_req() has a current and previous
> request but still exits to queue a new request if the queue size is
> greater than 2. In that case, when mmc_start_req() is called again, the
> current request will have been prepared already. Fix by flagging if the
> request has been prepared.
>
> That also means ensuring that struct mmc_async_req is always initialized
> to zero, which wasn't the case in mmc_test.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Looks good!
Reviewed-by: Harjani Ritesh <riteshh@codeaurora.org>
Regards
Ritesh
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH V7 14/25] mmc: core: Export mmc_retune_hold() and mmc_retune_release()
From: Ritesh Harjani @ 2016-11-28 4:49 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ulf Hansson, linux-mmc, Alex Lemberg, Mateusz Nowak,
Yuliy Izrailov, Jaehoon Chung, Dong Aisheng, Das Asutosh,
Zhangfei Gao, Dorfman Konstantin, David Griego, Sahitya Tummala,
Venu Byravarasu, Linus Walleij
In-Reply-To: <1480068442-5169-15-git-send-email-adrian.hunter@intel.com>
On 11/25/2016 3:37 PM, Adrian Hunter wrote:
> Re-tuning can only be done when the Command Queue is empty, when means
> holding and releasing re-tuning from the block driver, so export those
> functions.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Looks good!
Reviewed-by: Harjani Ritesh <riteshh@codeaurora.org>
Regards
Ritesh
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH V7 15/25] mmc: block: Factor out mmc_blk_requeue()
From: Ritesh Harjani @ 2016-11-28 4:51 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ulf Hansson, linux-mmc, Alex Lemberg, Mateusz Nowak,
Yuliy Izrailov, Jaehoon Chung, Dong Aisheng, Das Asutosh,
Zhangfei Gao, Dorfman Konstantin, David Griego, Sahitya Tummala,
Venu Byravarasu, Linus Walleij
In-Reply-To: <1480068442-5169-16-git-send-email-adrian.hunter@intel.com>
On 11/25/2016 3:37 PM, Adrian Hunter wrote:
> The same code is used in a couple of places.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Looks good!
Reviewed-by: Harjani Ritesh <riteshh@codeaurora.org>
Regards
Ritesh
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v3] mmc: block: delete packed command support
From: Christoph Hellwig @ 2016-11-28 8:23 UTC (permalink / raw)
To: Alex Lemberg
Cc: Ulf Hansson, Linus Walleij, linux-mmc@vger.kernel.org,
Chunyan Zhang, Baolin Wang, Namjae Jeon, Maya Erez
In-Reply-To: <50128E8A-D7B3-49E3-9E18-4AD240F1EBD0@sandisk.com>
On Sun, Nov 27, 2016 at 02:25:15PM +0000, Alex Lemberg wrote:
> I understand your concerns about the current packed commands
> support and the problem of transition to blk_mq.
> But as I mentioned in previous email thread, the packed commands
> feature is still in use on eMMC4.5 or eMMC5.1 devices on hosts
> without SW/HW CMDQ support.
> Taking this feature out meaning stop supporting one of important
> performance features of eMMC devices.
> Looking forward this feature is replaced with a CMDQ, but should we
> kill it for those who doesn’t have a CMDQ?
No driver actually uses it, as-is it's simply dead code.
^ permalink raw reply
* Re: [PATCH 1/2] mmc: core: add DT binding for CMD23
From: Jaehoon Chung @ 2016-11-28 9:40 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Rob Herring,
Mark Rutland, Adrian Hunter
In-Reply-To: <CAPDyKFr3CMeqDpQxf13Z1Ae5W_Q3X75if4fK9eYstXaVeTTVwg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 11/25/2016 05:19 PM, Ulf Hansson wrote:
> On 25 November 2016 at 07:52, Jaehoon Chung <jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>> Provide the option to configure one type of multiple block read/wrte
>> transatction (CMD23 - it's optional.)
>>
>> Signed-off-by: Jaehoon Chung <jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
>> drivers/mmc/core/host.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
>> index 98f25ff..9bdc369 100644
>> --- a/drivers/mmc/core/host.c
>> +++ b/drivers/mmc/core/host.c
>> @@ -321,6 +321,8 @@ int mmc_of_parse(struct mmc_host *host)
>> host->caps2 |= MMC_CAP2_NO_SD;
>> if (of_property_read_bool(np, "no-mmc"))
>> host->caps2 |= MMC_CAP2_NO_MMC;
>> + if (of_property_read_bool(np, "cap-mmc-cmd23"))
>> + host->caps |= MMC_CAP_CMD23;
>>
>> host->dsr_req = !of_property_read_u32(np, "dsr", &host->dsr);
>> if (host->dsr_req && (host->dsr & ~0xffff)) {
>> --
>> 2.10.1
>>
>
> I don't think this as HW configuration, but more a SW configuration.
> Thus we don't need a DT binding for it, right?
Got it. Then discard these patches.
Best Regards,
Jaehoon Chung
>
> Kind regards
> Uffe
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC
From: Ziji Hu @ 2016-11-28 10:10 UTC (permalink / raw)
To: Ulf Hansson
Cc: Jimmy Xu, Andrew Lunn, Romain Perier, Hanna Hawa,
linux-kernel@vger.kernel.org, Nadav Haklai, Victor Gu, Doug Jones,
Jisheng Zhang, Yehuda Yitschak, Wei(SOCP) Liu, Kostya Porotchkin,
Sebastian Hesselbarth, devicetree@vger.kernel.org, Jason Cooper,
Rob Herring, Ryan Gao, Gregory CLEMENT, Marcin Wojtas,
linux-arm-kernel@lists.infradead.org, Thomas Petazzoni
In-Reply-To: <436c6925-cb0d-afe7-e3a2-384eca15ff42@marvell.com>
Hi Ulf,
On 2016/11/24 23:37, Ziji Hu wrote:
> Hi Ulf,
>
> On 2016/11/24 22:33, Ulf Hansson wrote:
<snip>
>>>
>>> As result, our SDHC driver has to implement the functionality to
>>> send commands and check the results, in host layer.
>>> If directly calling mmc_wait_for_cmd() is improper, could you please
>>> give us some suggestions?
>>>
>>> For eMMC, CMD8 is used to test current sampling point set in PHY.
>>
>> Try to use mmc_send_tuning().
>>
>
> Could you please tell me the requirement of "op_code" parameter in
> mmc_send_tuning()?
> According to mmc_send_tuning(),it seems that tuning command(CMD19/CMD21)
> is required. Thus device will not response mmc_send_tuning() if current
> speed mode doesn't support tuning command.
> Please correct me if I am wrong.
>
As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), to
send commands for testing current sampling point set in our host PHY.
According to my test result, it shows that mmc_send_tuning() can only support
tuning command (CMD21/CMD19).
As a result, we cannot use mmc_send_tuning() when card is in the speed modes
which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in those
speed modes.
Could you please provide suggestions for the speed mode in which tuning is
not available?
Thank you.
Best regards,
Hu Ziji
>>>
>>>>> +
>>>>> + return err;
>>>>> +}
>>>>> +
>>>>> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
>>>>> +{
>>>>> + struct mmc_command cmd = {0};
>>>>> + int err;
>>>>> +
>>>>> + cmd.opcode = SD_IO_RW_DIRECT;
>>>>> + cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
>>>>> +
>>>>> + err = mmc_wait_for_cmd(card->host, &cmd, 0);
>>>>> + if (err)
>>>>> + return err;
>>>>> +
>>>>> + if (cmd.resp[0] & R5_ERROR)
>>>>> + return -EIO;
>>>>> + if (cmd.resp[0] & R5_FUNCTION_NUMBER)
>>>>> + return -EINVAL;
>>>>> + if (cmd.resp[0] & R5_OUT_OF_RANGE)
>>>>> + return -ERANGE;
>>>>> + return 0;
>>>>
>>>> No thanks! MMC/SD/SDIO protocol code belongs in the core.
>>>>
>>> For SDIO, SD_IO_RW_DIRECT command is sent to test current sampling point
>>> in PHY.
>>> Please help provide some suggestion to implement the command transfer.
>>
>> Again, I think mmc_send_tuning() should be possible for you to use.
>>
>> [...]
>>
>>>>> + if (mmc->card)
>>>>> + card = mmc->card;
>>>>> + else
>>>>> + /*
>>>>> + * Only valid during initialization
>>>>> + * before mmc->card is set
>>>>> + */
>>>>> + card = priv->card_candidate;
>>>>> + if (unlikely(!card)) {
>>>>> + dev_warn(mmc_dev(mmc), "card is not present\n");
>>>>> + return -EINVAL;
>>>>> + }
>>>>
>>>> That your host need to hold a copy of the card pointer, tells me that
>>>> something is not really correct.
>>>>
>>>> I might be wrong, if this turns out to be a special case, but I doubt
>>>> it. Although, if it *is* a special such case, we shall most likely try
>>>> to extend the the mmc core layer instead of adding all these hacks in
>>>> your host driver.
>>>>
>>> This card pointer copies the temporary structure mmc_card
>>> used in mmc_init_card(), mmc_sd_init_card() and mmc_sdio_init_card().
>>> Since we call mmc_wait_for_cmd() to send test commands, we need a copy
>>> of that temporary mmc_card here in our host driver.
>>
>> I see, thanks for clarifying.
>>
>>>
>>> During PHY setting in card initialization, mmc_host->card is not updated
>>> yet with that temporary mmc_card. Thus we are not able to directly use
>>> mmc_host->card. Instead, this card pointer is introduced to enable
>>> mmc_wait_for_cmd().
>>>
>>> If we can improve our host driver to send test commands without mmc_card,
>>> this card pointer can be removed.
>>> Could you please share your opinion please?
>>
>> The mmc_send_tuning() API takes the mmc_host as parameter. If you
>> convert to that, perhaps you would be able to remove the need to hold
>> the card pointer.
>>
>> BTW, the reason why mmc_send_tuning() doesn't take the card as a
>> parameter, is exactly those you just described above.
>>
> Got it.
> Thanks a lot for the information.
>
> Thank you for the great help.
>
> Best regards,
> Hu Ziji
>
>> [...]
>>
>> Kind regards
>> Uffe
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC
From: Ulf Hansson @ 2016-11-28 11:09 UTC (permalink / raw)
To: Ziji Hu
Cc: Gregory CLEMENT, Adrian Hunter, linux-mmc@vger.kernel.org,
Jason Cooper, Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
devicetree@vger.kernel.org, Thomas Petazzoni,
linux-arm-kernel@lists.infradead.org, Jimmy Xu, Jisheng Zhang,
Nadav Haklai, Ryan Gao, Doug Jones, Victor Gu, Wei(SOCP) Liu,
Wilson Ding
In-Reply-To: <436c6925-cb0d-afe7-e3a2-384eca15ff42@marvell.com>
[...]
>
> Could you please tell me the requirement of "op_code" parameter in
> mmc_send_tuning()?
> According to mmc_send_tuning(),it seems that tuning command(CMD19/CMD21)
> is required. Thus device will not response mmc_send_tuning() if current
> speed mode doesn't support tuning command.
> Please correct me if I am wrong.
>
When the mmc core decides it's time to execute tuning, it invokes the
->execute_tuning() host ops, which has the "opcode" as a parameter.
You should be able to use it when calling mmc_send_tuning().
[...]
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC
From: Ulf Hansson @ 2016-11-28 11:13 UTC (permalink / raw)
To: Ziji Hu
Cc: Gregory CLEMENT, Adrian Hunter, linux-mmc@vger.kernel.org,
Jason Cooper, Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
devicetree@vger.kernel.org, Thomas Petazzoni,
linux-arm-kernel@lists.infradead.org, Jimmy Xu, Jisheng Zhang,
Nadav Haklai, Ryan Gao, Doug Jones, Victor Gu, Wei(SOCP) Liu,
Wilson Ding
In-Reply-To: <8359307d-5f44-3db9-aae1-eb1fe8e1141d@marvell.com>
>
> As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), to
> send commands for testing current sampling point set in our host PHY.
>
> According to my test result, it shows that mmc_send_tuning() can only support
> tuning command (CMD21/CMD19).
> As a result, we cannot use mmc_send_tuning() when card is in the speed modes
> which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
> SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in those
> speed modes.
>
> Could you please provide suggestions for the speed mode in which tuning is
> not available?
>
Normally the mmc host driver shouldn't have to care about what the
card supports, as that is the responsibility of the mmc core to
manage.
The host should only need to implement the ->execute_tuning() ops,
which gets called when the card supports tuning (CMD19/21). Does it
make sense?
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH v3] mmc: block: delete packed command support
From: Ulf Hansson @ 2016-11-28 11:35 UTC (permalink / raw)
To: Alex Lemberg
Cc: Linus Walleij, linux-mmc@vger.kernel.org, Chunyan Zhang,
Baolin Wang, Namjae Jeon, Maya Erez
In-Reply-To: <50128E8A-D7B3-49E3-9E18-4AD240F1EBD0@sandisk.com>
On 27 November 2016 at 15:25, Alex Lemberg <Alex.Lemberg@sandisk.com> wrote:
> Hi Ulf and Linus,
>
> I understand your concerns about the current packed commands
> support and the problem of transition to blk_mq.
> But as I mentioned in previous email thread, the packed commands
> feature is still in use on eMMC4.5 or eMMC5.1 devices on hosts
> without SW/HW CMDQ support.
> Taking this feature out meaning stop supporting one of important
> performance features of eMMC devices.
How do we get an improved performance from using packed-command?
If this is about decreasing communication overhead, as the JEDEC spec
states, we should compare against using the asynchronous request
mechanism. Then is there really any noticeable improvement?
> Looking forward this feature is replaced with a CMDQ, but should we
> kill it for those who doesn’t have a CMDQ?
Yes, I think so.
>From the performance perspective, but also from that there are no
users of the code, except in some vendor trees - then there are just
no justification for keep maintaining this messy piece of code.
[...]
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC
From: Ziji Hu @ 2016-11-28 11:38 UTC (permalink / raw)
To: Ulf Hansson
Cc: Gregory CLEMENT, Adrian Hunter, linux-mmc@vger.kernel.org,
Jason Cooper, Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
devicetree@vger.kernel.org, Thomas Petazzoni,
linux-arm-kernel@lists.infradead.org, Jimmy Xu, Jisheng Zhang,
Nadav Haklai, Ryan Gao, Doug Jones, Victor Gu, Wei(SOCP) Liu,
Wilson Ding
In-Reply-To: <CAPDyKFo3ezYOywtSZ8GGQ1XK9KPsxCgQNbaiz45EVgbgtnUxjg@mail.gmail.com>
Hi Ulf,
On 2016/11/28 19:13, Ulf Hansson wrote:
>>
>> As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), to
>> send commands for testing current sampling point set in our host PHY.
>>
>> According to my test result, it shows that mmc_send_tuning() can only support
>> tuning command (CMD21/CMD19).
>> As a result, we cannot use mmc_send_tuning() when card is in the speed modes
>> which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
>> SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in those
>> speed modes.
>>
>> Could you please provide suggestions for the speed mode in which tuning is
>> not available?
>>
>
> Normally the mmc host driver shouldn't have to care about what the
> card supports, as that is the responsibility of the mmc core to
> manage.
>
> The host should only need to implement the ->execute_tuning() ops,
> which gets called when the card supports tuning (CMD19/21). Does it
> make sense?
>
I think it is irrelevant to tuning procedure.
Our host requires to adjust PHY setting after each time ios setting
(SDCLK/bus width/speed mode) is changed.
The simplified sequence is:
mmc change ios --> mmc_set_ios() --> ->set_ios() --> after sdhci_set_ios(),
adjust PHY setting.
During PHY setting adjustment, out host driver has to send commands to
test current sampling point. Tuning is another independent step.
Thus our host needs a valid command in PHY setting adjustment. Tuning command
can be borrowed to complete this task in SD SDR50. But for other speed mode,
we have to find out a valid command.
Any suggestion please?
Thank you.
Best regards,
Hu Ziji
> Kind regards
> Uffe
>
^ permalink raw reply
* Re: [PATCH V7 06/25] mmc: queue: Introduce queue depth
From: Adrian Hunter @ 2016-11-28 12:45 UTC (permalink / raw)
To: Ritesh Harjani
Cc: Ulf Hansson, linux-mmc, Alex Lemberg, Mateusz Nowak,
Yuliy Izrailov, Jaehoon Chung, Dong Aisheng, Das Asutosh,
Zhangfei Gao, Dorfman Konstantin, David Griego, Sahitya Tummala,
Venu Byravarasu, Linus Walleij
In-Reply-To: <fc5ca510-e383-e0dc-90e1-397a07dec791@codeaurora.org>
On 28/11/16 06:19, Ritesh Harjani wrote:
>
>
> On 11/25/2016 3:37 PM, Adrian Hunter wrote:
>> Add a mmc_queue member to record the size of the queue, which currently
>> supports 2 requests on-the-go at a time.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>> drivers/mmc/card/block.c | 3 +++
>> drivers/mmc/card/queue.c | 1 +
>> drivers/mmc/card/queue.h | 1 +
>> 3 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> index f8e51640596e..47835b78872f 100644
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -1439,6 +1439,9 @@ static int mmc_packed_init(struct mmc_queue *mq,
>> struct mmc_card *card)
>> struct mmc_queue_req *mqrq_prev = &mq->mqrq[1];
>> int ret = 0;
>>
>> + /* Queue depth is only ever 2 with packed commands */
>> + if (mq->qdepth != 2)
>> + return -EINVAL;
> I think you are referring here that with SWMCDQ, packed commands wont be
> used. Instead of qdepth do you think we should check cmdq_en ?
> Also maybe we shouldn't even call mmc_packed_init if cmdq_en is true?
mmc_packed_init() has gone away but the intention was to make the code
protect itself because the implementation supports only 2 requests. So,
something like this would be more explicit:
/* Packed commands implementation depends on qdepth being 2 */
if (mq->qdepth != 2) {
WARN_ON(1):
return -EINVAL;
}
>
>
>>
>> mqrq_cur->packed = kzalloc(sizeof(struct mmc_packed), GFP_KERNEL);
>> if (!mqrq_cur->packed) {
>> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
>> index cbe92c9cfda1..60fa095adb14 100644
>> --- a/drivers/mmc/card/queue.c
>> +++ b/drivers/mmc/card/queue.c
>> @@ -296,6 +296,7 @@ int mmc_init_queue(struct mmc_queue *mq, struct
>> mmc_card *card,
>> if (!mq->queue)
>> return -ENOMEM;
>>
>> + mq->qdepth = 2;
>> mq->mqrq_cur = &mq->mqrq[0];
>> mq->mqrq_prev = &mq->mqrq[1];
>> mq->queue->queuedata = mq;
>> diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
>> index 0e8133c626c9..8a0a45e5650d 100644
>> --- a/drivers/mmc/card/queue.h
>> +++ b/drivers/mmc/card/queue.h
>> @@ -64,6 +64,7 @@ struct mmc_queue {
>> struct mmc_queue_req mqrq[2];
>> struct mmc_queue_req *mqrq_cur;
>> struct mmc_queue_req *mqrq_prev;
>> + int qdepth;
>> };
>>
>> extern int mmc_init_queue(struct mmc_queue *, struct mmc_card *,
>> spinlock_t *,
>>
>
^ permalink raw reply
* Re: [PATCH V7 09/25] mmc: mmc: Add Command Queue definitions
From: Adrian Hunter @ 2016-11-28 13:08 UTC (permalink / raw)
To: Ritesh Harjani
Cc: Ulf Hansson, linux-mmc, Alex Lemberg, Mateusz Nowak,
Yuliy Izrailov, Jaehoon Chung, Dong Aisheng, Das Asutosh,
Zhangfei Gao, Dorfman Konstantin, David Griego, Sahitya Tummala,
Venu Byravarasu, Linus Walleij
In-Reply-To: <6e1120e4-f13e-354c-9a5e-9dfc9239c9f3@codeaurora.org>
On 28/11/16 06:29, Ritesh Harjani wrote:
>
>
> On 11/25/2016 3:37 PM, Adrian Hunter wrote:
>> Add definitions relating to Command Queuing.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> drivers/mmc/core/mmc.c | 17 +++++++++++++++++
>> include/linux/mmc/card.h | 2 ++
>> include/linux/mmc/mmc.h | 17 +++++++++++++++++
>> 3 files changed, 36 insertions(+)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 3268fcd3378d..6e9830997eef 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -618,6 +618,23 @@ static int mmc_decode_ext_csd(struct mmc_card *card,
>> u8 *ext_csd)
>> (ext_csd[EXT_CSD_SUPPORTED_MODE] & 0x1) &&
>> !(ext_csd[EXT_CSD_FW_CONFIG] & 0x1);
>> }
>> +
>> + /* eMMC v5.1 or later */
>> + if (card->ext_csd.rev >= 8) {
>> + card->ext_csd.cmdq_support = ext_csd[EXT_CSD_CMDQ_SUPPORT] &
>> + EXT_CSD_CMDQ_SUPPORTED;
>> + card->ext_csd.cmdq_depth = (ext_csd[EXT_CSD_CMDQ_DEPTH] &
>> + EXT_CSD_CMDQ_DEPTH_MASK) + 1;
>> + if (card->ext_csd.cmdq_depth <= 2) {
>> + card->ext_csd.cmdq_support = false;
>> + card->ext_csd.cmdq_depth = 0;
>> + }
> Could you please explain, why for cmdq_depth <=2, we are disabling
> cmdq_support ? Maybe we can add a comment there.
It was in the original code. I presumed it was because such a small queue
did not give a performance advantage. Certainly a qdepth of 1 is not a
queue at all. However all the eMMC I have come in contact with have a queue
depth of at least 16, so it is unlikely to have any affect in practice. I
can add a comment.
>
>
>> + if (card->ext_csd.cmdq_support) {
>> + pr_debug("%s: Command Queue supported depth %u\n",
>> + mmc_hostname(card->host),
>> + card->ext_csd.cmdq_depth);
>> + }
>> + }
>> out:
>> return err;
>> }
>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>> index e49a3ff9d0e0..95d69d498296 100644
>> --- a/include/linux/mmc/card.h
>> +++ b/include/linux/mmc/card.h
>> @@ -89,6 +89,8 @@ struct mmc_ext_csd {
>> unsigned int boot_ro_lock; /* ro lock support */
>> bool boot_ro_lockable;
>> bool ffu_capable; /* Firmware upgrade support */
>> + bool cmdq_support; /* Command Queue supported */
>> + unsigned int cmdq_depth; /* Command Queue depth */
>> #define MMC_FIRMWARE_LEN 8
>> u8 fwrev[MMC_FIRMWARE_LEN]; /* FW version */
>> u8 raw_exception_status; /* 54 */
>> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
>> index c376209c70ef..672730acc705 100644
>> --- a/include/linux/mmc/mmc.h
>> +++ b/include/linux/mmc/mmc.h
>> @@ -84,6 +84,13 @@
>> #define MMC_APP_CMD 55 /* ac [31:16] RCA R1 */
>> #define MMC_GEN_CMD 56 /* adtc [0] RD/WR R1 */
>>
>> + /* class 11 */
>> +#define MMC_QUE_TASK_PARAMS 44 /* ac [20:16] task id R1 */
>> +#define MMC_QUE_TASK_ADDR 45 /* ac [31:0] data addr R1 */
>> +#define MMC_EXECUTE_READ_TASK 46 /* adtc [20:16] task id R1 */
>> +#define MMC_EXECUTE_WRITE_TASK 47 /* adtc [20:16] task id R1 */
>> +#define MMC_CMDQ_TASK_MGMT 48 /* ac [20:16] task id R1b */
>> +
>> static inline bool mmc_op_multi(u32 opcode)
>> {
>> return opcode == MMC_WRITE_MULTIPLE_BLOCK ||
>> @@ -272,6 +279,7 @@ struct _mmc_csd {
>> * EXT_CSD fields
>> */
>>
>> +#define EXT_CSD_CMDQ_MODE_EN 15 /* R/W */
>> #define EXT_CSD_FLUSH_CACHE 32 /* W */
>> #define EXT_CSD_CACHE_CTRL 33 /* R/W */
>> #define EXT_CSD_POWER_OFF_NOTIFICATION 34 /* R/W */
>> @@ -331,6 +339,8 @@ struct _mmc_csd {
>> #define EXT_CSD_CACHE_SIZE 249 /* RO, 4 bytes */
>> #define EXT_CSD_PWR_CL_DDR_200_360 253 /* RO */
>> #define EXT_CSD_FIRMWARE_VERSION 254 /* RO, 8 bytes */
>> +#define EXT_CSD_CMDQ_DEPTH 307 /* RO */
>> +#define EXT_CSD_CMDQ_SUPPORT 308 /* RO */
>> #define EXT_CSD_SUPPORTED_MODE 493 /* RO */
>> #define EXT_CSD_TAG_UNIT_SIZE 498 /* RO */
>> #define EXT_CSD_DATA_TAG_SUPPORT 499 /* RO */
>> @@ -438,6 +448,13 @@ struct _mmc_csd {
>> #define EXT_CSD_MANUAL_BKOPS_MASK 0x01
>>
>> /*
>> + * Command Queue
>> + */
>> +#define EXT_CSD_CMDQ_MODE_ENABLED BIT(0)
>
> Is there a need for both EXT_CSD_CMDQ_MODE_ENABLED and
> EXT_CSD_CMDQ_MODE_SUPPORTED. Arent they doing same thing?
They are bits in different ext-csd bytes. EXT_CSD_CMDQ_MODE_ENABLED is in
the CMDQ_MODE_EN byte (15), and EXT_CSD_CMDQ_SUPPORTED is in CMDQ_SUPPORT
(308). AFAIK being the same bit number is coincidence.
>
>
>> +#define EXT_CSD_CMDQ_DEPTH_MASK GENMASK(4, 0)
>> +#define EXT_CSD_CMDQ_SUPPORTED BIT(0)
> Ditto
>
>> +
>> +/*
>> * MMC_SWITCH access modes
>> */
>>
>>
>
^ permalink raw reply
* Re: [PATCH V7 10/25] mmc: mmc: Add functions to enable / disable the Command Queue
From: Adrian Hunter @ 2016-11-28 13:23 UTC (permalink / raw)
To: Ritesh Harjani
Cc: Ulf Hansson, linux-mmc, Alex Lemberg, Mateusz Nowak,
Yuliy Izrailov, Jaehoon Chung, Dong Aisheng, Das Asutosh,
Zhangfei Gao, Dorfman Konstantin, David Griego, Sahitya Tummala,
Venu Byravarasu, Linus Walleij
In-Reply-To: <f995f8fd-8e34-5366-d9e4-df1e0f0504d2@codeaurora.org>
On 28/11/16 06:36, Ritesh Harjani wrote:
>
>
> On 11/25/2016 3:37 PM, Adrian Hunter wrote:
>> Add helper functions to enable or disable the Command Queue.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>> Documentation/mmc/mmc-dev-attrs.txt | 1 +
>> drivers/mmc/core/mmc.c | 2 ++
>> drivers/mmc/core/mmc_ops.c | 27 +++++++++++++++++++++++++++
>> include/linux/mmc/card.h | 1 +
>> include/linux/mmc/core.h | 2 ++
>> 5 files changed, 33 insertions(+)
>>
>> diff --git a/Documentation/mmc/mmc-dev-attrs.txt
>> b/Documentation/mmc/mmc-dev-attrs.txt
>> index 404a0e9e92b0..dcd1252877fb 100644
>> --- a/Documentation/mmc/mmc-dev-attrs.txt
>> +++ b/Documentation/mmc/mmc-dev-attrs.txt
>> @@ -30,6 +30,7 @@ All attributes are read-only.
>> rel_sectors Reliable write sector count
>> ocr Operation Conditions Register
>> dsr Driver Stage Register
>> + cmdq_en Command Queue enabled: 1 => enabled, 0 => not enabled
>>
>> Note on Erase Size and Preferred Erase Size:
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 6e9830997eef..d6a30bbd399d 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -770,6 +770,7 @@ static int mmc_compare_ext_csds(struct mmc_card *card,
>> unsigned bus_width)
>> MMC_DEV_ATTR(raw_rpmb_size_mult, "%#x\n", card->ext_csd.raw_rpmb_size_mult);
>> MMC_DEV_ATTR(rel_sectors, "%#x\n", card->ext_csd.rel_sectors);
>> MMC_DEV_ATTR(ocr, "%08x\n", card->ocr);
>> +MMC_DEV_ATTR(cmdq_en, "%d\n", card->ext_csd.cmdq_en);
>>
>> static ssize_t mmc_fwrev_show(struct device *dev,
>> struct device_attribute *attr,
>> @@ -823,6 +824,7 @@ static ssize_t mmc_dsr_show(struct device *dev,
>> &dev_attr_rel_sectors.attr,
>> &dev_attr_ocr.attr,
>> &dev_attr_dsr.attr,
>> + &dev_attr_cmdq_en.attr,
>> NULL,
>> };
>> ATTRIBUTE_GROUPS(mmc_std);
>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>> index 9b2617cfff67..92a1de9b4981 100644
>> --- a/drivers/mmc/core/mmc_ops.c
>> +++ b/drivers/mmc/core/mmc_ops.c
>> @@ -824,3 +824,30 @@ int mmc_can_ext_csd(struct mmc_card *card)
>> {
>> return (card && card->csd.mmca_vsn > CSD_SPEC_VER_3);
>> }
>> +
>> +int mmc_cmdq_switch(struct mmc_card *card, int enable)
>> +{
>> + int err;
>> +
>> + if (!card->ext_csd.cmdq_support)
>> + return -EOPNOTSUPP;
>> +
>> + err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_CMDQ_MODE_EN,
>> + enable, card->ext_csd.generic_cmd6_time);
>> + if (!err)
>> + card->ext_csd.cmdq_en = enable;
>> +
>> + return err;
>> +}
>> +
>> +int mmc_cmdq_enable(struct mmc_card *card)
>> +{
>> + return mmc_cmdq_switch(card, EXT_CSD_CMDQ_MODE_ENABLED);
>
> EXT_CSD_CMDQ_MODE_ENABLED is defined as a BIT(0), but getting used here as a
> value of 1. EXT_CSD_CMDQ_MODE_ENABLED seems redundant anyways.
> Do you think we can remove it and pass 1 directly?
How about:
static int mmc_cmdq_switch(struct mmc_card *card, bool enable)
{
u8 val = enable ? EXT_CSD_CMDQ_MODE_ENABLED : 0;
int err;
if (!card->ext_csd.cmdq_support)
return -EOPNOTSUPP;
err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_CMDQ_MODE_EN,
val, card->ext_csd.generic_cmd6_time);
if (!err)
card->ext_csd.cmdq_en = enable;
return err;
}
int mmc_cmdq_enable(struct mmc_card *card)
{
return mmc_cmdq_switch(card, true);
}
EXPORT_SYMBOL_GPL(mmc_cmdq_enable);
int mmc_cmdq_disable(struct mmc_card *card)
{
return mmc_cmdq_switch(card, false);
}
EXPORT_SYMBOL_GPL(mmc_cmdq_disable);
>
>> +}
>> +EXPORT_SYMBOL_GPL(mmc_cmdq_enable);
>> +
>> +int mmc_cmdq_disable(struct mmc_card *card)
>> +{
>> + return mmc_cmdq_switch(card, 0);
>> +}
>> +EXPORT_SYMBOL_GPL(mmc_cmdq_disable);
>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>> index 95d69d498296..2d9c24f4e88e 100644
>> --- a/include/linux/mmc/card.h
>> +++ b/include/linux/mmc/card.h
>> @@ -89,6 +89,7 @@ struct mmc_ext_csd {
>> unsigned int boot_ro_lock; /* ro lock support */
>> bool boot_ro_lockable;
>> bool ffu_capable; /* Firmware upgrade support */
>> + bool cmdq_en; /* Command Queue enabled */
>> bool cmdq_support; /* Command Queue supported */
>> unsigned int cmdq_depth; /* Command Queue depth */
>> #define MMC_FIRMWARE_LEN 8
>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
>> index 0ce928b3ce90..d045b06fc7ea 100644
>> --- a/include/linux/mmc/core.h
>> +++ b/include/linux/mmc/core.h
>> @@ -177,6 +177,8 @@ extern int mmc_wait_for_app_cmd(struct mmc_host *,
>> struct mmc_card *,
>> extern int mmc_switch(struct mmc_card *, u8, u8, u8, unsigned int);
>> extern int mmc_send_tuning(struct mmc_host *host, u32 opcode, int
>> *cmd_error);
>> extern int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
>> +extern int mmc_cmdq_enable(struct mmc_card *card);
>> +extern int mmc_cmdq_disable(struct mmc_card *card);
>>
>> #define MMC_ERASE_ARG 0x00000000
>> #define MMC_SECURE_ERASE_ARG 0x80000000
>>
>
^ permalink raw reply
* Re: [PATCH V7 10/25] mmc: mmc: Add functions to enable / disable the Command Queue
From: Ritesh Harjani @ 2016-11-28 14:00 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ulf Hansson, linux-mmc, Alex Lemberg, Mateusz Nowak,
Yuliy Izrailov, Jaehoon Chung, Dong Aisheng, Das Asutosh,
Zhangfei Gao, Dorfman Konstantin, David Griego, Sahitya Tummala,
Venu Byravarasu, Linus Walleij
In-Reply-To: <9984a3e0-a834-41f7-1c59-6f3d78e63ab0@intel.com>
On 11/28/2016 6:53 PM, Adrian Hunter wrote:
> On 28/11/16 06:36, Ritesh Harjani wrote:
>>
>>
>> On 11/25/2016 3:37 PM, Adrian Hunter wrote:
>>> Add helper functions to enable or disable the Command Queue.
>>>
>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>> ---
>>> Documentation/mmc/mmc-dev-attrs.txt | 1 +
>>> drivers/mmc/core/mmc.c | 2 ++
>>> drivers/mmc/core/mmc_ops.c | 27 +++++++++++++++++++++++++++
>>> include/linux/mmc/card.h | 1 +
>>> include/linux/mmc/core.h | 2 ++
>>> 5 files changed, 33 insertions(+)
>>>
>>> diff --git a/Documentation/mmc/mmc-dev-attrs.txt
>>> b/Documentation/mmc/mmc-dev-attrs.txt
>>> index 404a0e9e92b0..dcd1252877fb 100644
>>> --- a/Documentation/mmc/mmc-dev-attrs.txt
>>> +++ b/Documentation/mmc/mmc-dev-attrs.txt
>>> @@ -30,6 +30,7 @@ All attributes are read-only.
>>> rel_sectors Reliable write sector count
>>> ocr Operation Conditions Register
>>> dsr Driver Stage Register
>>> + cmdq_en Command Queue enabled: 1 => enabled, 0 => not enabled
>>>
>>> Note on Erase Size and Preferred Erase Size:
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index 6e9830997eef..d6a30bbd399d 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -770,6 +770,7 @@ static int mmc_compare_ext_csds(struct mmc_card *card,
>>> unsigned bus_width)
>>> MMC_DEV_ATTR(raw_rpmb_size_mult, "%#x\n", card->ext_csd.raw_rpmb_size_mult);
>>> MMC_DEV_ATTR(rel_sectors, "%#x\n", card->ext_csd.rel_sectors);
>>> MMC_DEV_ATTR(ocr, "%08x\n", card->ocr);
>>> +MMC_DEV_ATTR(cmdq_en, "%d\n", card->ext_csd.cmdq_en);
>>>
>>> static ssize_t mmc_fwrev_show(struct device *dev,
>>> struct device_attribute *attr,
>>> @@ -823,6 +824,7 @@ static ssize_t mmc_dsr_show(struct device *dev,
>>> &dev_attr_rel_sectors.attr,
>>> &dev_attr_ocr.attr,
>>> &dev_attr_dsr.attr,
>>> + &dev_attr_cmdq_en.attr,
>>> NULL,
>>> };
>>> ATTRIBUTE_GROUPS(mmc_std);
>>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>>> index 9b2617cfff67..92a1de9b4981 100644
>>> --- a/drivers/mmc/core/mmc_ops.c
>>> +++ b/drivers/mmc/core/mmc_ops.c
>>> @@ -824,3 +824,30 @@ int mmc_can_ext_csd(struct mmc_card *card)
>>> {
>>> return (card && card->csd.mmca_vsn > CSD_SPEC_VER_3);
>>> }
>>> +
>>> +int mmc_cmdq_switch(struct mmc_card *card, int enable)
>>> +{
>>> + int err;
>>> +
>>> + if (!card->ext_csd.cmdq_support)
>>> + return -EOPNOTSUPP;
>>> +
>>> + err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_CMDQ_MODE_EN,
>>> + enable, card->ext_csd.generic_cmd6_time);
>>> + if (!err)
>>> + card->ext_csd.cmdq_en = enable;
>>> +
>>> + return err;
>>> +}
>>> +
>>> +int mmc_cmdq_enable(struct mmc_card *card)
>>> +{
>>> + return mmc_cmdq_switch(card, EXT_CSD_CMDQ_MODE_ENABLED);
>>
>> EXT_CSD_CMDQ_MODE_ENABLED is defined as a BIT(0), but getting used here as a
>> value of 1. EXT_CSD_CMDQ_MODE_ENABLED seems redundant anyways.
>> Do you think we can remove it and pass 1 directly?
>
> How about:
>
> static int mmc_cmdq_switch(struct mmc_card *card, bool enable)
> {
> u8 val = enable ? EXT_CSD_CMDQ_MODE_ENABLED : 0;
> int err;
>
> if (!card->ext_csd.cmdq_support)
> return -EOPNOTSUPP;
>
> err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_CMDQ_MODE_EN,
> val, card->ext_csd.generic_cmd6_time);
> if (!err)
> card->ext_csd.cmdq_en = enable;
>
> return err;
> }
>
> int mmc_cmdq_enable(struct mmc_card *card)
> {
> return mmc_cmdq_switch(card, true);
> }
> EXPORT_SYMBOL_GPL(mmc_cmdq_enable);
>
> int mmc_cmdq_disable(struct mmc_card *card)
> {
> return mmc_cmdq_switch(card, false);
> }
> EXPORT_SYMBOL_GPL(mmc_cmdq_disable);
>
yes. This looks good.
>>
>>> +}
>>> +EXPORT_SYMBOL_GPL(mmc_cmdq_enable);
>>> +
>>> +int mmc_cmdq_disable(struct mmc_card *card)
>>> +{
>>> + return mmc_cmdq_switch(card, 0);
>>> +}
>>> +EXPORT_SYMBOL_GPL(mmc_cmdq_disable);
>>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>>> index 95d69d498296..2d9c24f4e88e 100644
>>> --- a/include/linux/mmc/card.h
>>> +++ b/include/linux/mmc/card.h
>>> @@ -89,6 +89,7 @@ struct mmc_ext_csd {
>>> unsigned int boot_ro_lock; /* ro lock support */
>>> bool boot_ro_lockable;
>>> bool ffu_capable; /* Firmware upgrade support */
>>> + bool cmdq_en; /* Command Queue enabled */
>>> bool cmdq_support; /* Command Queue supported */
>>> unsigned int cmdq_depth; /* Command Queue depth */
>>> #define MMC_FIRMWARE_LEN 8
>>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
>>> index 0ce928b3ce90..d045b06fc7ea 100644
>>> --- a/include/linux/mmc/core.h
>>> +++ b/include/linux/mmc/core.h
>>> @@ -177,6 +177,8 @@ extern int mmc_wait_for_app_cmd(struct mmc_host *,
>>> struct mmc_card *,
>>> extern int mmc_switch(struct mmc_card *, u8, u8, u8, unsigned int);
>>> extern int mmc_send_tuning(struct mmc_host *host, u32 opcode, int
>>> *cmd_error);
>>> extern int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
>>> +extern int mmc_cmdq_enable(struct mmc_card *card);
>>> +extern int mmc_cmdq_disable(struct mmc_card *card);
>>>
>>> #define MMC_ERASE_ARG 0x00000000
>>> #define MMC_SECURE_ERASE_ARG 0x80000000
>>>
>>
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH V7 25/25] mmc: sdhci-acpi: Enable Software Command Queuing for some Intel controllers
From: Adrian Hunter @ 2016-11-28 13:55 UTC (permalink / raw)
To: Linus Walleij
Cc: Ulf Hansson, linux-mmc, Alex Lemberg, Mateusz Nowak,
Yuliy Izrailov, Jaehoon Chung, Dong Aisheng, Das Asutosh,
Zhangfei Gao, Dorfman Konstantin, David Griego, Sahitya Tummala,
Harjani Ritesh, Venu Byravarasu
In-Reply-To: <CACRpkdZZa8O3NPm3+1gDb=mfRDZqAjzgqdW5EZyrxGQn-_XbbQ@mail.gmail.com>
On 25/11/16 17:15, Linus Walleij wrote:
> On Fri, Nov 25, 2016 at 11:07 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
>> Set MMC_CAP_SWCMDQ for Intel BYT and related eMMC host controllers.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>> drivers/mmc/host/sdhci-acpi.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>> index 81d4dc034793..95fc4de05c54 100644
>> --- a/drivers/mmc/host/sdhci-acpi.c
>> +++ b/drivers/mmc/host/sdhci-acpi.c
>> @@ -274,7 +274,7 @@ static int sdhci_acpi_sd_probe_slot(struct platform_device *pdev,
>> static const struct sdhci_acpi_slot sdhci_acpi_slot_int_emmc = {
>> .chip = &sdhci_acpi_chip_int,
>> .caps = MMC_CAP_8_BIT_DATA | MMC_CAP_NONREMOVABLE |
>> - MMC_CAP_HW_RESET | MMC_CAP_1_8V_DDR |
>> + MMC_CAP_HW_RESET | MMC_CAP_1_8V_DDR | MMC_CAP_SWCMDQ |
>
> Actually I don't see why SOFTWARE command queueing would need a cap flag
> in the host at all?
>
> Isn't the whole point with it that if it is available, we don't need any special
> hardware support to use it with any host?
>
> So why not just enable it if the card supports it in that case, why flag
> it in the host at all?
It is a good question. I was trying to remember why I did that way, but
nothing came to mind.
Now it is dependent on MMC_CAP_CMD_DURING_TFR which host controllers may not
support. An example is SDHCI host controllers that have
SDHCI_QUIRK_RESET_AFTER_REQUEST since the reset will interfere with ongoing
transfers.
So I will drop MMC_CAP_SWCMDQ and just check MMC_CAP_CMD_DURING_TFR.
^ permalink raw reply
* Re: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip
From: Rob Herring @ 2016-11-28 14:15 UTC (permalink / raw)
To: Matt Ranostay
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-mmc-u79uwXL29TY76Z2rM5mHXA, Tony Lindgren, Ulf Hansson,
Mark Rutland, Srinivas Kandagatla
In-Reply-To: <1479434109-8745-1-git-send-email-matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
On Thu, Nov 17, 2016 at 05:55:09PM -0800, Matt Ranostay wrote:
> Allow power sequencing for the Marvell SD8787 Wifi/BT chip.
> This can be abstracted to other chipsets if needed in the future.
I don't like the MMC pwrseq bindings, so I won't be acking any. Look at
the USB pwrseq for why.
> Cc: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> Cc: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
> ---
> .../devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt | 14 +++
> .../bindings/net/wireless/marvell-sd8xxx.txt | 4 +
> drivers/mmc/core/Kconfig | 10 ++
> drivers/mmc/core/Makefile | 1 +
> drivers/mmc/core/pwrseq_sd8787.c | 117 +++++++++++++++++++++
> 5 files changed, 146 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
> create mode 100644 drivers/mmc/core/pwrseq_sd8787.c
>
> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
> new file mode 100644
> index 000000000000..1b658351629b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
> @@ -0,0 +1,14 @@
> +* Marvell SD8787 power sequence provider
> +
> +Required properties:
> +- compatible: must be "mmc-pwrseq-sd8787".
> +- pwndn-gpio: contains a power down GPIO specifier.
powerdown-gpios
> +- reset-gpio: contains a reset GPIO specifier.
reset-gpios
> +
> +Example:
> +
> + wifi_pwrseq: wifi_pwrseq {
> + compatible = "mmc-pwrseq-sd8787";
> + pwrdn-gpio = <&twl_gpio 0 GPIO_ACTIVE_LOW>;
> + reset-gpio = <&twl_gpio 1 GPIO_ACTIVE_LOW>;
> + }
> diff --git a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
> index c421aba0a5bc..08fd65d35725 100644
> --- a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
> +++ b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
> @@ -32,6 +32,9 @@ Optional properties:
> so that the wifi chip can wakeup host platform under certain condition.
> during system resume, the irq will be disabled to make sure
> unnecessary interrupt is not received.
> + - vmmc-supply: a phandle of a regulator, supplying VCC to the card
This is why pwrseq is wrong. You have some properties in the card node
and some in pwrseq node. Everything should be in the card node.
> + - mmc-pwrseq: phandle to the MMC power sequence node. See "mmc-pwrseq-*"
> + for documentation of MMC power sequence bindings.
>
> Example:
>
> @@ -44,6 +47,7 @@ so that firmware can wakeup host using this device side pin.
> &mmc3 {
> status = "okay";
> vmmc-supply = <&wlan_en_reg>;
> + mmc-pwrseq = <&wifi_pwrseq>;
> bus-width = <4>;
> cap-power-off-card;
> keep-power-in-suspend;
^ permalink raw reply
* Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC
From: Ulf Hansson @ 2016-11-28 15:16 UTC (permalink / raw)
To: Ziji Hu
Cc: Gregory CLEMENT, Adrian Hunter,
linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jason Cooper,
Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Thomas Petazzoni,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Jimmy Xu, Jisheng Zhang, Nadav Haklai, Ryan Gao, Doug Jones,
Victor Gu, Wei(SOCP) Liu, Wilson Ding
In-Reply-To: <10a885f0-82e9-a35c-f62f-3fc4518ea8b4-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
On 28 November 2016 at 12:38, Ziji Hu <huziji-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> wrote:
> Hi Ulf,
>
> On 2016/11/28 19:13, Ulf Hansson wrote:
>>>
>>> As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), to
>>> send commands for testing current sampling point set in our host PHY.
>>>
>>> According to my test result, it shows that mmc_send_tuning() can only support
>>> tuning command (CMD21/CMD19).
>>> As a result, we cannot use mmc_send_tuning() when card is in the speed modes
>>> which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
>>> SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in those
>>> speed modes.
>>>
>>> Could you please provide suggestions for the speed mode in which tuning is
>>> not available?
>>>
>>
>> Normally the mmc host driver shouldn't have to care about what the
>> card supports, as that is the responsibility of the mmc core to
>> manage.
>>
>> The host should only need to implement the ->execute_tuning() ops,
>> which gets called when the card supports tuning (CMD19/21). Does it
>> make sense?
>>
> I think it is irrelevant to tuning procedure.
>
> Our host requires to adjust PHY setting after each time ios setting
> (SDCLK/bus width/speed mode) is changed.
> The simplified sequence is:
> mmc change ios --> mmc_set_ios() --> ->set_ios() --> after sdhci_set_ios(),
> adjust PHY setting.
> During PHY setting adjustment, out host driver has to send commands to
> test current sampling point. Tuning is another independent step.
For those speed modes (or other ios changes) that *don't* requires
tuning, then what will you do when you send the command to confirm the
change of PHY setting and it fails?
My assumption is that you will fail anyway, by propagating the error
to the mmc core. At least that what was my understanding from your
earlier replies, right!?
Then, I think there are no point having the host driver sending a
command to confirm the PHY settings, as the mmc core will anyway
discover if something goes wrong when the next command is sent.
Please correct me if I am wrong!
>
> Thus our host needs a valid command in PHY setting adjustment. Tuning command
> can be borrowed to complete this task in SD SDR50. But for other speed mode,
> we have to find out a valid command.
I thought we agreed on this wasn't necessary? Please see my upper response.
Kind regards
Uffe
--
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: arasan,sdhci.txt "compatibility" DT binding
From: Mason @ 2016-11-28 15:44 UTC (permalink / raw)
To: linux-mmc, Shawn Lin, Adrian Hunter
Cc: Michal Simek, Rameshwar Sahu, Linux ARM, Soren Brinkmann,
Michal Simek, Anton Vorontsov, Xiaobo Xie, Suman Tripathi,
Linus Walleij, Maxime Ripard, Arnd Bergmann, Rob Herring,
Zach Brown, Ulf Hansson, Douglas Anderson, Heiko Stuebner,
Jisheng Zhang, Suneel Garapati, Russell King, Sebastian Frias
In-Reply-To: <a20b2b4f-15d4-3c29-324c-1ced5dbcc67f@xilinx.com>
Hello,
@Shawn Lin, could you take a look below and tell me exactly
which IP core(s) Rockchip is using in its SoCs?
Based on the feedback I received, here is an updated list of
compatible strings and controller versions dealt with by the
drivers/mmc/host/sdhci-of-arasan.c code.
Xilinx Zynq:
"SD2.0 / SDIO2.0 / MMC3.31 AHB Host Controller"
"arasan,sdhci-8.9a"
NB: 8.9a is the documentation revision (dated 2011-10-19)
subsequent tweaks labeled 9.0a, 9.1a, 9.2a
Xilinx ZynqMP:
"SD3.0 / SDIO3.0 / eMMC4.51 AHB Host Controller"
"arasan,sdhci-8.9a"
NB: using the same compatible string as Zynq
Sigma SMP87xx
"SD3.0 / SDIO3.0 / eMMC4.4 AHB Host Controller"
no compatible string yet, platform-specific init required
APM:
"SD3.0 / SDIO3.0 / eMMC4.41 AHB Host Controller"
"arasan,sdhci-4.9a"
NB: 4.9a appears to be the documentation revision
no functional diff with "arasan,sdhci-8.9a"
Rockchip
Exact IP unknown, waiting for Shawn's answer
"arasan,sdhci-5.1"
NB: 5.1 appears to refer to the eMMC standard supported
On a final note, there are many variations of the Arasan IP.
I've tracked down at least the following:
SD_2.0_SDIO_2.0__MMC_3.31_AHB_Host_Controller.pdf
SD_3.0_SDIO_3.0_eMMC_4.41_OCP_Host_Controller.pdf
SD_3.0_SDIO_3.0_eMMC_4.4__AHB_Host_Controller.pdf
SD_3.0_SDIO_3.0_eMMC_4.51_Host_Controller.pdf
SD_3.0_SDIO_3.0_eMMC_4.5__Host_Controller.pdf
SD_4.1_SDIO_4.1_eMMC_4.51_Host_Controller.pdf
SD_4.1_SDIO_4.1_eMMC_5.1__Host_Controller.pdf
It seems to me the compatible string should specify
the SD/SDIO version AND the eMMC version, since it
seems many combinations are allowed, e.g. eMMC 4.51
has two possible SD versions.
What do you think?
Regards.
^ permalink raw reply
* Re: [PATCH] mmc: sdhci-msm: Add sdhci_reset() implementation
From: Georgi Djakov @ 2016-11-28 15:51 UTC (permalink / raw)
To: Ritesh Harjani, adrian.hunter, ulf.hansson
Cc: linux-mmc, linux-kernel, linux-arm-msm
In-Reply-To: <95ef8403-5e3c-4219-f45e-e289cf59f1b8@codeaurora.org>
On 11/24/2016 06:06 PM, Ritesh Harjani wrote:
> Hi Georgi,
>
> I collected some info on this problem. May be below info might help you.
>
> I think "Reset 0x1" problem is occurring because of below call stack.
> SDHCI_RESET_ALL to SDHCI_SOFTWARE_RESET register will anyway trigger the
> sdhci_msm_pwr_irq.
>
> But I think the problem is that the above occurs in spinlock context
> and because of only one core the IRQ will never be serviced, hence you
> were seeing (Reset 0x1) error.
>
Hi Ritesh,
Thanks for looking into this. So yes, its called in spinlock context and
what we need to do is just handle the power irq after writing to reset
register.
[..]
>
> To prove above I tried this and the problem goes away. But I still dont
> think that the below approach is correct. As it will still trigger a
> pwr_irq as well.
>
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 62aedf1..01e611c 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -174,6 +174,8 @@ void sdhci_reset(struct sdhci_host *host, u8 mask)
> /* Reset-all turns off SD Bus Power */
> if (host->quirks2 & SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON)
> sdhci_runtime_pm_bus_off(host);
> + if (host->ops->voltage_switch)
> + host->ops->voltage_switch(host);
> }
>
Yes, our own reset() function that additionally handles the irq will work.
Thanks,
Georgi
^ permalink raw reply
* Re: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip
From: Ulf Hansson @ 2016-11-28 15:54 UTC (permalink / raw)
To: Rob Herring
Cc: Matt Ranostay, linux-wireless@vger.kernel.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-mmc@vger.kernel.org, Tony Lindgren, Mark Rutland,
Srinivas Kandagatla
In-Reply-To: <20161128141513.agnkyz6ronigbukn@rob-hp-laptop>
[...]
>> +
>> +Example:
>> +
>> + wifi_pwrseq: wifi_pwrseq {
>> + compatible = "mmc-pwrseq-sd8787";
>> + pwrdn-gpio = <&twl_gpio 0 GPIO_ACTIVE_LOW>;
>> + reset-gpio = <&twl_gpio 1 GPIO_ACTIVE_LOW>;
>> + }
>> diff --git a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>> index c421aba0a5bc..08fd65d35725 100644
>> --- a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>> +++ b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>> @@ -32,6 +32,9 @@ Optional properties:
>> so that the wifi chip can wakeup host platform under certain condition.
>> during system resume, the irq will be disabled to make sure
>> unnecessary interrupt is not received.
>> + - vmmc-supply: a phandle of a regulator, supplying VCC to the card
>
> This is why pwrseq is wrong. You have some properties in the card node
> and some in pwrseq node. Everything should be in the card node.
Put "all" in the card node, just doesn't work for MMC. Particular in
cases when we have removable cards, as then it would be wrong to have
a card node.
The mmc pwrseq DT bindings just follows the legacy approach for MMC
and that's why the pwrseq handle is at the controller node. Yes, would
could have done it differently, but this is the case now, so we will
have to accept that.
[...]
Kind regards
Uffe
^ permalink raw reply
* Re: arasan,sdhci.txt "compatibility" DT binding
From: Arnd Bergmann @ 2016-11-28 16:15 UTC (permalink / raw)
To: Mason
Cc: linux-mmc, Shawn Lin, Adrian Hunter, Michal Simek, Rameshwar Sahu,
Linux ARM, Soren Brinkmann, Michal Simek, Anton Vorontsov,
Xiaobo Xie, Suman Tripathi, Linus Walleij, Maxime Ripard,
Rob Herring, Zach Brown, Ulf Hansson, Douglas Anderson,
Heiko Stuebner, Jisheng Zhang, Suneel Garapati <sune>
In-Reply-To: <583C50E7.6030400@free.fr>
On Monday, November 28, 2016 4:44:39 PM CET Mason wrote:
> Hello,
>
> @Shawn Lin, could you take a look below and tell me exactly
> which IP core(s) Rockchip is using in its SoCs?
>
> Based on the feedback I received, here is an updated list of
> compatible strings and controller versions dealt with by the
> drivers/mmc/host/sdhci-of-arasan.c code.
>
>
> Xilinx Zynq:
> "SD2.0 / SDIO2.0 / MMC3.31 AHB Host Controller"
> "arasan,sdhci-8.9a"
> NB: 8.9a is the documentation revision (dated 2011-10-19)
> subsequent tweaks labeled 9.0a, 9.1a, 9.2a
>
> Xilinx ZynqMP:
> "SD3.0 / SDIO3.0 / eMMC4.51 AHB Host Controller"
> "arasan,sdhci-8.9a"
> NB: using the same compatible string as Zynq
>
> Sigma SMP87xx
> "SD3.0 / SDIO3.0 / eMMC4.4 AHB Host Controller"
> no compatible string yet, platform-specific init required
>
> APM:
> "SD3.0 / SDIO3.0 / eMMC4.41 AHB Host Controller"
> "arasan,sdhci-4.9a"
> NB: 4.9a appears to be the documentation revision
> no functional diff with "arasan,sdhci-8.9a"
>
> Rockchip
> Exact IP unknown, waiting for Shawn's answer
> "arasan,sdhci-5.1"
> NB: 5.1 appears to refer to the eMMC standard supported
>
>
> On a final note, there are many variations of the Arasan IP.
> I've tracked down at least the following:
>
> SD_2.0_SDIO_2.0__MMC_3.31_AHB_Host_Controller.pdf
> SD_3.0_SDIO_3.0_eMMC_4.41_OCP_Host_Controller.pdf
> SD_3.0_SDIO_3.0_eMMC_4.4__AHB_Host_Controller.pdf
> SD_3.0_SDIO_3.0_eMMC_4.51_Host_Controller.pdf
> SD_3.0_SDIO_3.0_eMMC_4.5__Host_Controller.pdf
> SD_4.1_SDIO_4.1_eMMC_4.51_Host_Controller.pdf
> SD_4.1_SDIO_4.1_eMMC_5.1__Host_Controller.pdf
>
> It seems to me the compatible string should specify
> the SD/SDIO version AND the eMMC version, since it
> seems many combinations are allowed, e.g. eMMC 4.51
> has two possible SD versions.
>
> What do you think?
It seems wrong to have the eMMC or SD version in the compatible
string. Is that the only difference between the documents you
found? Normally there should be a version of IP block itself,
besides the supported protocol.
Arnd
^ permalink raw reply
* Re: arasan,sdhci.txt "compatibility" DT binding
From: Sebastian Frias @ 2016-11-28 16:23 UTC (permalink / raw)
To: Mason, linux-mmc, Shawn Lin, Adrian Hunter
Cc: Jisheng Zhang, Michal Simek, Rameshwar Sahu, Suman Tripathi,
Arnd Bergmann, Xiaobo Xie, P L Sai Krishna, Linus Walleij,
Anton Vorontsov, Heiko Stuebner, Michal Simek, Ulf Hansson,
Douglas Anderson, Linux ARM, Mark Rutland, Russell King,
Maxime Ripard, Suneel Garapati, Soren Brinkmann, Zach Brown
In-Reply-To: <583C50E7.6030400@free.fr>
On 28/11/16 16:44, Mason wrote:
> Hello,
>
> @Shawn Lin, could you take a look below and tell me exactly
> which IP core(s) Rockchip is using in its SoCs?
>
> Based on the feedback I received, here is an updated list of
> compatible strings and controller versions dealt with by the
> drivers/mmc/host/sdhci-of-arasan.c code.
>
>
> Xilinx Zynq:
> "SD2.0 / SDIO2.0 / MMC3.31 AHB Host Controller"
> "arasan,sdhci-8.9a"
> NB: 8.9a is the documentation revision (dated 2011-10-19)
> subsequent tweaks labeled 9.0a, 9.1a, 9.2a
>
> Xilinx ZynqMP:
> "SD3.0 / SDIO3.0 / eMMC4.51 AHB Host Controller"
> "arasan,sdhci-8.9a"
> NB: using the same compatible string as Zynq
>
> Sigma SMP87xx
> "SD3.0 / SDIO3.0 / eMMC4.4 AHB Host Controller"
> no compatible string yet, platform-specific init required
>
> APM:
> "SD3.0 / SDIO3.0 / eMMC4.41 AHB Host Controller"
> "arasan,sdhci-4.9a"
> NB: 4.9a appears to be the documentation revision
> no functional diff with "arasan,sdhci-8.9a"
>
> Rockchip
> Exact IP unknown, waiting for Shawn's answer
> "arasan,sdhci-5.1"
> NB: 5.1 appears to refer to the eMMC standard supported
>
>
> On a final note, there are many variations of the Arasan IP.
> I've tracked down at least the following:
>
> SD_2.0_SDIO_2.0__MMC_3.31_AHB_Host_Controller.pdf
> SD_3.0_SDIO_3.0_eMMC_4.41_OCP_Host_Controller.pdf
> SD_3.0_SDIO_3.0_eMMC_4.4__AHB_Host_Controller.pdf
> SD_3.0_SDIO_3.0_eMMC_4.51_Host_Controller.pdf
> SD_3.0_SDIO_3.0_eMMC_4.5__Host_Controller.pdf
> SD_4.1_SDIO_4.1_eMMC_4.51_Host_Controller.pdf
> SD_4.1_SDIO_4.1_eMMC_5.1__Host_Controller.pdf
>
> It seems to me the compatible string should specify
> the SD/SDIO version AND the eMMC version, since it
> seems many combinations are allowed, e.g. eMMC 4.51
> has two possible SD versions.
>
> What do you think?
I'm trying to picture this. Imagine:
a) SoC XYZ used two versions:
- SD_3.0_SDIO_3.0_eMMC_4.4__AHB_Host_Controller.pdf
- SD_3.0_SDIO_3.0_eMMC_4.5__Host_Controller.pdf
b) That the compatible suffixes were defined as "sd30-emmc44" and
"sd30-emmc45" respectively
c) That the chip-specific init is the same for both.
What would be the recommended way of dealing with that at DT/driver level?
1) XYZ's DT1: compatible = "arasan,sdhci-sd30-emmc44", "XYZ,sdhci"
DT2: compatible = "arasan,sdhci-sd30-emmc45", "XYZ,sdhci"
driver: match "XYZ,sdhci" for chip-specific init, and then leaves
"arasan,sdhci-sd30-emmc44" or arasan,sdhci-sd30-emmc45" for generic part
2) XYZ's DT1: compatible = "XYZ,arasan-sdhci-sd30-emmc44"
DT2: compatible = "XYZ,arasan-sdhci-sd30-emmc45"
driver: match "XYZ,arasan-sdhci-sd30-emmc44" or "XYZ,arasan-sdhci-sd30-emmc45"
for chip-specific init and generic parts
3) XYZ's DT1: compatible = "arasan,sdhci-sd30-emmc44"
DT2: compatible = "arasan,sdhci-sd30-emmc45"
driver: match "arasan,sdhci-sd30-emmc44" or "arasan,sdhci-sd30-emmc45" for
generic part; chip-specific init done somewhere else (bootloader?)
4) something else?
How would those solutions be affected if condition c) was changed to
"chip-specific init is different for both"?
>
> Regards.
>
^ permalink raw reply
* Re: arasan,sdhci.txt "compatibility" DT binding
From: Mason @ 2016-11-28 16:52 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-mmc, Shawn Lin, Adrian Hunter, Michal Simek, Rameshwar Sahu,
Linux ARM, Soren Brinkmann, Michal Simek, Anton Vorontsov,
Xiaobo Xie, Suman Tripathi, Linus Walleij, Maxime Ripard,
Rob Herring, Zach Brown, Ulf Hansson, Douglas Anderson,
Heiko Stuebner, Jisheng Zhang, Suneel Garapati <sune>
In-Reply-To: <14771688.Q8f97gHC2H@wuerfel>
On 28/11/2016 17:15, Arnd Bergmann wrote:
> On Monday, November 28, 2016 4:44:39 PM CET Mason wrote:
>
>> Hello,
>>
>> @Shawn Lin, could you take a look below and tell me exactly
>> which IP core(s) Rockchip is using in its SoCs?
>>
>> Based on the feedback I received, here is an updated list of
>> compatible strings and controller versions dealt with by the
>> drivers/mmc/host/sdhci-of-arasan.c code.
>>
>>
>> Xilinx Zynq:
>> "SD2.0 / SDIO2.0 / MMC3.31 AHB Host Controller"
>> "arasan,sdhci-8.9a"
>> NB: 8.9a is the documentation revision (dated 2011-10-19)
>> subsequent tweaks labeled 9.0a, 9.1a, 9.2a
>>
>> Xilinx ZynqMP:
>> "SD3.0 / SDIO3.0 / eMMC4.51 AHB Host Controller"
>> "arasan,sdhci-8.9a"
>> NB: using the same compatible string as Zynq
>>
>> Sigma SMP87xx
>> "SD3.0 / SDIO3.0 / eMMC4.4 AHB Host Controller"
>> no compatible string yet, platform-specific init required
>>
>> APM:
>> "SD3.0 / SDIO3.0 / eMMC4.41 AHB Host Controller"
>> "arasan,sdhci-4.9a"
>> NB: 4.9a appears to be the documentation revision
>> no functional diff with "arasan,sdhci-8.9a"
>>
>> Rockchip
>> Exact IP unknown, waiting for Shawn's answer
>> "arasan,sdhci-5.1"
>> NB: 5.1 appears to refer to the eMMC standard supported
>>
>>
>> On a final note, there are many variations of the Arasan IP.
>> I've tracked down at least the following:
>>
>> SD_2.0_SDIO_2.0__MMC_3.31_AHB_Host_Controller.pdf
>> SD_3.0_SDIO_3.0_eMMC_4.41_OCP_Host_Controller.pdf
>> SD_3.0_SDIO_3.0_eMMC_4.4__AHB_Host_Controller.pdf
>> SD_3.0_SDIO_3.0_eMMC_4.51_Host_Controller.pdf
>> SD_3.0_SDIO_3.0_eMMC_4.5__Host_Controller.pdf
>> SD_4.1_SDIO_4.1_eMMC_4.51_Host_Controller.pdf
>> SD_4.1_SDIO_4.1_eMMC_5.1__Host_Controller.pdf
>>
>> It seems to me the compatible string should specify
>> the SD/SDIO version AND the eMMC version, since it
>> seems many combinations are allowed, e.g. eMMC 4.51
>> has two possible SD versions.
>>
>> What do you think?
>
> It seems wrong to have the eMMC or SD version in the compatible
> string. Is that the only difference between the documents you
> found? Normally there should be a version of IP block itself,
> besides the supported protocol.
But that is exactly the problem :-)
Nowhere in the documentation do they specify an "IP version".
Some documents do provide a revision number, but that's just
a *documentation* revision number, e.g.
changes in version 3.6 : fix typos
changes in version 9.1a : update company logo
That's why Xilinx used "arasan,sdhci-8.9a" and APM used
"arasan,sdhci-4.9a". These are documentation revisions.
In my opinion, that information is mostly worthless.
Looking more closely at SD_3.0_SDIO_3.0_eMMC_4.4__AHB_Host_Controller.pdf
(User Guide, which has more info than Datasheet) I see this:
Changed Host Controller Version Register value from 16'h0002 to 16'h7501
Changed Host Controller Version Register value from 16'h8301 to 16'h8401
Changed Host Controller Version Register value from 16'h8401 to 16'h8501
Changed Host Controller Version Register to 16'h9502
Changed Host Controller Version Register to 16'h9602
Changed Host Controller Version Register to 16'h9902
Host controller version register (offset 0FEh)
Vendor Version Number 15:8
HwInit=0x99
This status is reserved for the vendor version number.
The HD should not use this status.
Specification Version Number 7:0
HwInit=0x02
This status indicates the Host Controller Spec. Version.
The upper and lower 4-bits indicate the version.
Description
00 - SD Host Specification version 1.0
01 - SD Host Specification version 2.00
including only the feature of the Test Register
02 - SD Host Specification Version 3.00
others - Reserved
I'm not sure what this "Vendor Version Number" specifies, nor if is
guaranteed to be unique across controllers.
In SD_3.0_SDIO_3.0_eMMC_4.5__Host_Controller_UserGuide.pdf,
they write "The Vendor Version Number is set to 0x10 (1.0)"
I don't have a UserGuide for "arasan,sdhci-5.1".
Regards.
^ permalink raw reply
* [PATCH v2] mmc: sdhci-msm: Add sdhci_reset() implementation
From: Georgi Djakov @ 2016-11-28 17:39 UTC (permalink / raw)
To: adrian.hunter, ulf.hansson
Cc: linux-mmc, linux-kernel, linux-arm-msm, riteshh, georgi.djakov
On apq8016, apq8084 and apq8074 platforms, writing to the software
reset register triggers the "power irq". We need to ack and handle
the irq, otherwise the following message appears:
mmc0: Reset 0x1 never completed.
sdhci: =========== REGISTER DUMP (mmc0)===========
sdhci: Sys addr: 0x00000000 | Version: 0x00002e02
sdhci: Blk size: 0x00004000 | Blk cnt: 0x00000000
sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
sdhci: Present: 0x01f80000 | Host ctl: 0x00000000
sdhci: Power: 0x00000000 | Blk gap: 0x00000000
sdhci: Wake-up: 0x00000000 | Clock: 0x00000003
sdhci: Timeout: 0x00000000 | Int stat: 0x00000000
sdhci: Int enab: 0x00000000 | Sig enab: 0x00000000
sdhci: AC12 err: 0x00000000 | Slot int: 0x00000000
sdhci: Caps: 0x322dc8b2 | Caps_1: 0x00008007
sdhci: Cmd: 0x00000000 | Max curr: 0x00000000
sdhci: Host ctl2: 0x00000000
sdhci: ADMA Err: 0x00000000 | ADMA Ptr: 0x0000000000000000
sdhci: ===========================================
Fix it by implementing the custom sdhci_reset() function,
which performs the software reset and then handles the irq.
Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
Changes since v1: (https://lkml.org/lkml/2016/11/22/411)
* Perform the software reset by just writing to the SDHCI_SOFTWARE_RESET
register and then check for the irq.
drivers/mmc/host/sdhci-msm.c | 29 ++++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 32879b845b75..157ae07f9309 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -1019,6 +1019,33 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
__sdhci_msm_set_clock(host, clock);
}
+void sdhci_msm_reset(struct sdhci_host *host, u8 mask)
+{
+ unsigned long timeout = 100;
+
+ sdhci_writeb(host, mask, SDHCI_SOFTWARE_RESET);
+
+ if (mask & SDHCI_RESET_ALL) {
+ host->clock = 0;
+
+ /*
+ * SDHCI_RESET_ALL triggers the PWR IRQ and we need
+ * to handle it here.
+ */
+ sdhci_msm_voltage_switch(host);
+ }
+
+ while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
+ if (timeout == 0) {
+ pr_err("%s: Reset 0x%x never completed.\n",
+ mmc_hostname(host->mmc), (int)mask);
+ return;
+ }
+ timeout--;
+ mdelay(1);
+ }
+}
+
static const struct of_device_id sdhci_msm_dt_match[] = {
{ .compatible = "qcom,sdhci-msm-v4" },
{},
@@ -1028,7 +1055,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
static const struct sdhci_ops sdhci_msm_ops = {
.platform_execute_tuning = sdhci_msm_execute_tuning,
- .reset = sdhci_reset,
+ .reset = sdhci_msm_reset,
.set_clock = sdhci_msm_set_clock,
.get_min_clock = sdhci_msm_get_min_clock,
.get_max_clock = sdhci_msm_get_max_clock,
^ permalink raw reply related
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