RE: [PATCH 2/3] [02/03] mmc: Add Synopsys DesignWare mmc cmdq host driver

[Jyan Chou [周芷安]]

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@xxxxxxxxxxx/
https://patchwork.kernel.org/project/linux-mmc/patch/20231018055326.18256-3-jyanchou@xxxxxxxxxxx/
https://patchwork.kernel.org/project/linux-mmc/patch/20231018055326.18256-4-jyanchou@xxxxxxxxxxx/
https://patchwork.kernel.org/project/linux-mmc/patch/20231018055326.18256-5-jyanchou@xxxxxxxxxxx/


>> 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@xxxxxxxxx/
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@xxxxxxxxx> 
Sent: Tuesday, October 10, 2023 3:04 PM
To: Jyan Chou [周芷安] <jyanchou@xxxxxxxxxxx>; ulf.hansson@xxxxxxxxxx; jh80.chung@xxxxxxxxxxx
Cc: linux-mmc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; James Tai [戴志峰] <james.tai@xxxxxxxxxxx>
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@xxxxxxxxxxx>
>
> —
> 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@xxxxxxxxx/

[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]