lkml.org 
[lkml]   [2023]   [Oct]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH 2/3] [02/03] mmc: Add Synopsys DesignWare mmc cmdq host driver
Date
Hi Adrian,

Thanks for your review and advice.

We had updated our new version patches and corrected our code to your suggestions.

https://patchwork.kernel.org/project/linux-mmc/patch/20231018055326.18256-2-jyanchou@realtek.com/
https://patchwork.kernel.org/project/linux-mmc/patch/20231018055326.18256-3-jyanchou@realtek.com/
https://patchwork.kernel.org/project/linux-mmc/patch/20231018055326.18256-4-jyanchou@realtek.com/
https://patchwork.kernel.org/project/linux-mmc/patch/20231018055326.18256-5-jyanchou@realtek.com/


>> Please put version numbers in the patch subject.
We modified version numbers this time, thanks.

> Register differences can be abstracted away, for example by providing callbacks for reading / writing registers.
> So this still needs much more explanation.
We had added more description to explain.

> It would be preferable to use a callback only for setting the descriptor.
> Please see comments about dwcmshc_cqhci_set_tran_desc() and dwcmshc_cqhci_prep_tran_desc() made here:
> https://lore.kernel.org/linux-mmc/0932b124-16da-495c-9706-bbadadb3b076@intel.com/
We EXPORT cqhci_set_tran_desc and used it to replace dw_mci_cqe_set_tran_desc, thanks.

> This should not be necessary. Instead, please try to use
> ->pre_enable() and ->post_disable() like in mtk-sd.c
Thanks for your advice, we had removed unnecessary check code and added cqhci_host_ops callback function.

Best Regards,
Jyan

-----Original Message-----
From: Adrian Hunter <adrian.hunter@intel.com>
Sent: Tuesday, October 10, 2023 3:04 PM
To: Jyan Chou [周芷安] <jyanchou@realtek.com>; ulf.hansson@linaro.org; jh80.chung@samsung.com
Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org; James Tai [戴志峰] <james.tai@realtek.com>
Subject: Re: [PATCH 2/3] [02/03] mmc: Add Synopsys DesignWare mmc cmdq host driver


External mail.



Please put version numbers in the patch subject. Refer to:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html

On 6/09/23 12:28, Jyan Chou wrote:
> We implemented cmdq feature on Synopsys DesignWare mmc driver.
> The difference between dw_mmc.c and dw_mmc_cqe.c were distinct
> register definitions and the addition of cmdq.

Register differences can be abstracted away, for example by providing callbacks for reading / writing registers.
So this still needs much more explanation.

>
> More over, the flow of abort command sequence had change.
> We added a wait status function to satisfy synopsys user guide's
> description, since this flow might be specific in synopsys host driver
> only.
>
> Signed-off-by: Jyan Chou <jyanchou@realtek.com>
>
> —
> v0 to v1 change:
> 1. Seperate different support into single patch.
> 2. Fix the compiler complains.
> ---
> drivers/mmc/host/Kconfig | 13 +
> drivers/mmc/host/Makefile | 1 +
> drivers/mmc/host/dw_mmc_cqe.c | 1634
> +++++++++++++++++++++++++++++++++ drivers/mmc/host/dw_mmc_cqe.h |
> 443 +++++++++
> 4 files changed, 2091 insertions(+)
> create mode 100644 drivers/mmc/host/dw_mmc_cqe.c create mode 100644
> drivers/mmc/host/dw_mmc_cqe.h

My comments pertain only to the use of cqhci.

[SNIP]

> +static void dw_mci_cqe_set_tran_desc(u8 *desc,
> + dma_addr_t addr,
> + int len,
> + bool end,
> + bool dma64) {
> + __le32 *attr = (__le32 __force *)desc;
> +
> + *attr = (CQHCI_VALID(1) |
> + CQHCI_END(end ? 1 : 0) |
> + CQHCI_INT(0) |
> + CQHCI_ACT(0x4) |
> + CQHCI_DAT_LENGTH(len));
> +
> + if (dma64) {
> + __le64 *dataddr = (__le64 __force *)(desc + 4);
> +
> + dataddr[0] = cpu_to_le64(addr);
> + } else {
> + __le32 *dataddr = (__le32 __force *)(desc + 4);
> +
> + dataddr[0] = cpu_to_le32(addr);
> + }
> +}
> +
> +static void dw_mci_cqe_setup_tran_desc(struct mmc_data *data,
> + struct cqhci_host *cq_host,
> + u8 *desc,
> + int sg_count) {
> + struct scatterlist *sg;
> + u32 cur_blk_cnt, remain_blk_cnt;
> + unsigned int begin, end;
> + int i, len;
> + bool last = false;
> + bool dma64 = cq_host->dma64;
> + dma_addr_t addr;
> +
> + for_each_sg(data->sg, sg, sg_count, i) {
> + addr = sg_dma_address(sg);
> + len = sg_dma_len(sg);
> + remain_blk_cnt = len >> 9;
> +
> + while (remain_blk_cnt) {
> + /*DW_MCI_MAX_SCRIPT_BLK is tha max for each descriptor record*/
> + if (remain_blk_cnt > DW_MCI_MAX_SCRIPT_BLK)
> + cur_blk_cnt = DW_MCI_MAX_SCRIPT_BLK;
> + else
> + cur_blk_cnt = remain_blk_cnt;
> +
> + /* In Synopsys DesignWare Databook Page 84,
> + * They mentioned the DMA 128MB restriction
> + */
> + begin = addr / SZ_128M;
> + end = (addr + cur_blk_cnt * SZ_512) / SZ_128M;
> +
> + if (begin != end)
> + cur_blk_cnt = (end * SZ_128M - addr) /
> + SZ_512;
> +
> + if ((i+1) == sg_count && (remain_blk_cnt == cur_blk_cnt))
> + last = true;
> +
> + dw_mci_cqe_set_tran_desc(desc, addr,
> + (cur_blk_cnt << 9), last,
> + dma64);
> +
> + addr = addr + (cur_blk_cnt << 9);
> + remain_blk_cnt -= cur_blk_cnt;
> + desc += cq_host->trans_desc_len;
> + }
> + }
> +}

It would be preferable to use a callback only for setting the descriptor.
Please see comments about dwcmshc_cqhci_set_tran_desc() and dwcmshc_cqhci_prep_tran_desc() made here:
https://lore.kernel.org/linux-mmc/0932b124-16da-495c-9706-bbadadb3b076@intel.com/

[SNIP]

> +static void dw_mci_cqe_request(struct mmc_host *mmc, struct
> +mmc_request *mrq) {
> + struct dw_mci_slot *slot = mmc_priv(mmc);
> + struct dw_mci *host = slot->host;
> +
> + WARN_ON(slot->mrq);
> +
> + /*
> + * The check for card presence and queueing of the request must be
> + * atomic, otherwise the card could be removed in between and the
> + * request wouldn't fail until another card was inserted.
> + */
> +
> + if (!dw_mci_cqe_get_cd(mmc)) {
> + mrq->cmd->error = -ENOMEDIUM;
> + mmc_request_done(mmc, mrq);
> + return;
> + }
> +
> + down_write(&host->cr_rw_sem);
> +
> + /*cmdq case needs extra check*/
> + if (host->pdata && (host->pdata->caps2 & MMC_CAP2_CQE)) {
> + if ((host->cqe) == NULL) {
> + dev_err(host->dev, "dw_mci_request_cqe not done yet\n");
> + mdelay(2);
> + }
> +
> + if (mmc->cqe_on == false && host->cqe->activated == true)
> + cqhci_deactivate(mmc);

This should not be necessary. Instead, please try to use
->pre_enable() and ->post_disable() like in mtk-sd.c

[SNIP]
\
 
 \ /
  Last update: 2023-10-18 08:20    [W:1.643 / U:0.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site