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]
