Message-ID: <e366f4a13d5c47afa9e4e44c27e4db24@realtek.com>
Date:   Mon, 18 Sep 2023 08:23:18 +0000
From:   Jyan Chou [周芷安] <jyanchou@...ltek.com>
To:     "adrian.hunter@...el.com" <adrian.hunter@...el.com>,
        "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
        "jh80.chung@...sung.com" <jh80.chung@...sung.com>
CC:     "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Ben Chuang <benchuanggli@...il.com>
Subject: RE: [PATCH] mmc: Add Synopsys DesignWare mmc cmdq host driver

Hi Ulf, Adrain, Jaehoon,

We had push synopsys mmc cmdq driver and would like you to help us review.

Would you please give some suggestions or help us review?

Thanks a lot.

Best regards,

Jyan

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/ 

-----Original Message-----
From: Jyan Chou [周芷安] 
Sent: Wednesday, September 6, 2023 6:36 PM
To: 'Ben Chuang' <benchuanggli@...il.com>
Cc: adrian.hunter@...el.com; ulf.hansson@...aro.org; linux-mmc@...r.kernel.org; linux-kernel@...r.kernel.org; 'jh80.chung@...sung.com' <jh80.chung@...sung.com>
Subject: RE: [PATCH] mmc: Add Synopsys DesignWare mmc cmdq host driver

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@...il.com> 
Sent: Wednesday, September 6, 2023 10:13 AM
To: Jyan Chou [周芷安] <jyanchou@...ltek.com>
Cc: adrian.hunter@...el.com; ulf.hansson@...aro.org; linux-mmc@...r.kernel.org; linux-kernel@...r.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@...ltek.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.

Powered by blists - more mailing lists