RE: [PATCH] mmc: Add Synopsys DesignWare mmc cmdq host driver

["Jyan Chou [周芷安]" <jyanchou@realtek.com>]

Hi Ben,

Thanks for your comment and suggestion.

> Apart from the difference in register definitions and the addition of cmdq, is there any other behavior that is different from dw_mmc.c?
> I recommend using a patch series and describing the differences from dw_mmc in your cover letter, for an example as follows.

We had modified our patch into a patch series and fixed compile error. Please refer to the commits below.

> Do you forget to add dw_mmc_cqe.o and dw_mmc_cqe-rtk.o to Makefile?

Thanks for your remind, we had added dw_mmc_cqe.o, dw_mmc_cqe-rtk.o into new version patch.

https://patchwork.kernel.org/project/linux-mmc/patch/da1f7fbae1dd34cfc5d4bcecf3a2323f382ffd3a.1693991785.git.jyanchou@realtek.com/
https://patchwork.kernel.org/project/linux-mmc/patch/9617f04133ba8b6907b253c4154083f75956a341.1693991785.git.jyanchou@realtek.com/
https://patchwork.kernel.org/project/linux-mmc/patch/9cc6c51d8513c0dca5399420d753825183aa98f4.1693991785.git.jyanchou@realtek.com/

Best regards,
Jyan Chou

-----Original Message-----
From: Ben Chuang <benchuanggli@gmail.com> 
Sent: Wednesday, September 6, 2023 10:13 AM
To: Jyan Chou [周芷安] <jyanchou@realtek.com>
Cc: adrian.hunter@intel.com; ulf.hansson@linaro.org; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mmc: Add Synopsys DesignWare mmc cmdq host driver


External mail.



Hi Jyan,

On Thu, Aug 31, 2023 at 3:47 PM Jyan Chou [周芷安] <jyanchou@realtek.com> wrote:
>
> Hi Ben,
> Thanks for your suggestion.
>
> > The patch includes two parts: a dw_mmc_cqe driver and dw_mmc_cqe-rtk driver.
> > Adrian and Ulf's comments[1][2] don't seem to be addressed.
>
> [1] The reason why we added many changes was because we found out that 
> synopsys IP data book's register and user guide with cmdq support were 
> different from non cmdq's , so we referred to dw_mmc.c coding style 
> and push dw_mmc_cqe.c to support version after 5.1 JEDEC Standard.
>

Apart from the difference in register definitions and the addition of cmdq, is there any other behavior that is different from dw_mmc.c?
I recommend using a patch series and describing the differences from dw_mmc in your cover letter, for an example as follows
  [00/04] cover letter - Add DesignWare Mobile mmc driver
  [01/04] Introduce a setup_tran_desc ops ...
  [02/04] Add dw mobile_mmc driver .....
  [03/04] Add command queue to dw mobile_mmc driver .....
  [04/04] Add dw mobile mmc rtk driver .....
And please read patiently
https://www.kernel.org/doc/html/latest/process/submitting-patches.html.

>>---
>> drivers/mmc/host/Kconfig          |   22 +

Do you forget to add dw_mmc_cqe.o and dw_mmc_cqe-rtk.o to Makefile?

>> drivers/mmc/host/cqhci-core.c     |    5 +
>> drivers/mmc/host/cqhci.h          |    2 +
>> drivers/mmc/host/dw_mmc_cqe-rtk.c |  999 ++++++++++++++++++ 
>> drivers/mmc/host/dw_mmc_cqe-rtk.h |  160 +++
>> drivers/mmc/host/dw_mmc_cqe.c     | 1633 +++++++++++++++++++++++++++++
>> drivers/mmc/host/dw_mmc_cqe.h     |  442 ++++++++
>> 7 files changed, 3263 insertions(+)


And some compile complains for your reference,
---
drivers/mmc/host/dw_mmc_cqe.c: In function 'dw_mci_cqe_err_handle':
drivers/mmc/host/dw_mmc_cqe.c:723:41: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
  723 |                                         if (err == -DW_MCI_NOT_READY)
      |                                         ^~
drivers/mmc/host/dw_mmc_cqe.c:726:49: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
  726 |                                                 break;
      |                                                 ^~~~~
----
In file included from drivers/mmc/host/dw_mmc_cqe-rtk.c:23:
drivers/mmc/host/dw_mmc_cqe-rtk.h:155:5: error: conflicting types for 'mmc_hw_reset'; have 'int(struct mmc_host *)'
  155 | int mmc_hw_reset(struct mmc_host *host);
      |     ^~~~~~~~~~~~
In file included from drivers/mmc/host/dw_mmc_cqe-rtk.c:11:
./include/linux/mmc/core.h:178:5: note: previous declaration of 'mmc_hw_reset' with type 'int(struct mmc_card *)'
  178 | int mmc_hw_reset(struct mmc_card *card);
----

Best regards,
Ben Chuang

------Please consider the environment before printing this e-mail.