CQHCI_SSC1 indicates to CQE the polling period to use when using periodic
SEND_QUEUE_STATUS(CMD13) polling.
Since MSDC CQE uses msdc_hclk as ITCFVAL, so driver should use hclk
frequency to get the actual time.
The default value 0x1000 that corresponds to 150us for MediaTek SoCs, let's
decrease it to 0x40 that corresponds to 2.35us, which can improve the
performance of some eMMC devices.
Signed-off-by: Wenbin Mei <[email protected]>
---
drivers/mmc/host/cqhci.h | 1 +
drivers/mmc/host/mtk-sd.c | 47 +++++++++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+)
diff --git a/drivers/mmc/host/cqhci.h b/drivers/mmc/host/cqhci.h
index ba9387ed90eb..292b89ebd978 100644
--- a/drivers/mmc/host/cqhci.h
+++ b/drivers/mmc/host/cqhci.h
@@ -23,6 +23,7 @@
/* capabilities */
#define CQHCI_CAP 0x04
#define CQHCI_CAP_CS 0x10000000 /* Crypto Support */
+#define CQHCI_CAP_ITCFMUL(x) (((x) & GENMASK(15, 12)) >> 12)
/* configuration */
#define CQHCI_CFG 0x08
diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
index edade0e54a0c..c221ef8a6992 100644
--- a/drivers/mmc/host/mtk-sd.c
+++ b/drivers/mmc/host/mtk-sd.c
@@ -473,6 +473,7 @@ struct msdc_host {
struct msdc_tune_para def_tune_para; /* default tune setting */
struct msdc_tune_para saved_tune_para; /* tune result of CMD21/CMD19 */
struct cqhci_host *cq_host;
+ u32 cq_ssc1_time;
};
static const struct mtk_mmc_compatible mt2701_compat = {
@@ -2450,9 +2451,50 @@ static void msdc_hs400_enhanced_strobe(struct mmc_host *mmc,
}
}
+static void msdc_cqe_cit_cal(struct msdc_host *host, u64 timer_ns)
+{
+ struct mmc_host *mmc = mmc_from_priv(host);
+ struct cqhci_host *cq_host = mmc->cqe_private;
+ u8 itcfmul;
+ u32 hclk_freq;
+ u64 value;
+
+ /* Since MSDC CQE uses msdc_hclk as ITCFVAL, so driver should use hclk
+ * frequency to get the actual time for CIT.
+ */
+ if (host->h_clk) {
+ hclk_freq = clk_get_rate(host->h_clk);
+ itcfmul = CQHCI_CAP_ITCFMUL(cqhci_readl(cq_host, CQHCI_CAP));
+ switch (itcfmul) {
+ case 0x0:
+ do_div(hclk_freq, 1000);
+ break;
+ case 0x1:
+ do_div(hclk_freq, 100);
+ break;
+ case 0x2:
+ do_div(hclk_freq, 10);
+ break;
+ case 0x3:
+ break;
+ case 0x4:
+ hclk_freq = hclk_freq * 10;
+ break;
+ default:
+ host->cq_ssc1_time = 0x40;
+ return;
+ value = hclk_freq * timer_ns;
+ do_div(value, 1000000000ULL);
+ host->cq_ssc1_time = value;
+ } else {
+ host->cq_ssc1_time = 0x40;
+ }
+}
+
static void msdc_cqe_enable(struct mmc_host *mmc)
{
struct msdc_host *host = mmc_priv(mmc);
+ struct cqhci_host *cq_host = mmc->cqe_private;
/* enable cmdq irq */
writel(MSDC_INT_CMDQ, host->base + MSDC_INTEN);
@@ -2462,6 +2504,9 @@ static void msdc_cqe_enable(struct mmc_host *mmc)
msdc_set_busy_timeout(host, 20 * 1000000000ULL, 0);
/* default read data timeout 1s */
msdc_set_timeout(host, 1000000000ULL, 0);
+
+ /* Set the send status command idle timer */
+ cqhci_writel(cq_host, host->cq_ssc1_time, CQHCI_SSC1);
}
static void msdc_cqe_disable(struct mmc_host *mmc, bool recovery)
@@ -2803,6 +2848,8 @@ static int msdc_drv_probe(struct platform_device *pdev)
/* cqhci 16bit length */
/* 0 size, means 65536 so we don't have to -1 here */
mmc->max_seg_size = 64 * 1024;
+ /* Reduce CIT to 0x40 that corresponds to 2.35us */
+ msdc_cqe_cit_cal(host, 2350);
}
ret = devm_request_irq(&pdev->dev, host->irq, msdc_irq,
--
2.25.1
Hi Wenbin,
kernel test robot noticed the following build errors:
[auto build test ERROR on linus/master]
[also build test ERROR on ulf-hansson-mmc-mirror/next v6.4-rc5 next-20230605]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Wenbin-Mei/mmc-mtk-sd-reduce-CIT-for-better-performance/20230605-140238
base: linus/master
patch link: https://lore.kernel.org/r/20230605060107.22044-1-wenbin.mei%40mediatek.com
patch subject: [PATCH v3] mmc: mtk-sd: reduce CIT for better performance
config: powerpc-buildonly-randconfig-r006-20230605 (https://download.01.org/0day-ci/archive/20230605/[email protected]/config)
compiler: powerpc-linux-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/225e46f420d48f7ad73253636a0553bd5f986435
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Wenbin-Mei/mmc-mtk-sd-reduce-CIT-for-better-performance/20230605-140238
git checkout 225e46f420d48f7ad73253636a0553bd5f986435
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=powerpc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/mmc/host/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
In file included from ./arch/powerpc/include/generated/asm/div64.h:1,
from include/linux/math.h:6,
from include/linux/math64.h:6,
from include/linux/time.h:6,
from include/linux/stat.h:19,
from include/linux/module.h:13,
from drivers/mmc/host/mtk-sd.c:7:
drivers/mmc/host/mtk-sd.c: In function 'msdc_cqe_cit_cal':
include/asm-generic/div64.h:222:35: warning: comparison of distinct pointer types lacks a cast
222 | (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
| ^~
drivers/mmc/host/mtk-sd.c:2470:25: note: in expansion of macro 'do_div'
2470 | do_div(hclk_freq, 1000);
| ^~~~~~
In file included from include/linux/build_bug.h:5,
from include/linux/container_of.h:5,
from include/linux/list.h:5,
from include/linux/module.h:12:
include/asm-generic/div64.h:234:32: warning: right shift count >= width of type [-Wshift-count-overflow]
234 | } else if (likely(((n) >> 32) == 0)) { \
| ^~
include/linux/compiler.h:76:45: note: in definition of macro 'likely'
76 | # define likely(x) __builtin_expect(!!(x), 1)
| ^
drivers/mmc/host/mtk-sd.c:2470:25: note: in expansion of macro 'do_div'
2470 | do_div(hclk_freq, 1000);
| ^~~~~~
include/asm-generic/div64.h:238:36: error: passing argument 1 of '__div64_32' from incompatible pointer type [-Werror=incompatible-pointer-types]
238 | __rem = __div64_32(&(n), __base); \
| ^~~~
| |
| u32 * {aka unsigned int *}
drivers/mmc/host/mtk-sd.c:2470:25: note: in expansion of macro 'do_div'
2470 | do_div(hclk_freq, 1000);
| ^~~~~~
include/asm-generic/div64.h:213:38: note: expected 'uint64_t *' {aka 'long long unsigned int *'} but argument is of type 'u32 *' {aka 'unsigned int *'}
213 | extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
| ~~~~~~~~~~^~~~~~~~
include/asm-generic/div64.h:222:35: warning: comparison of distinct pointer types lacks a cast
222 | (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
| ^~
drivers/mmc/host/mtk-sd.c:2473:25: note: in expansion of macro 'do_div'
2473 | do_div(hclk_freq, 100);
| ^~~~~~
include/asm-generic/div64.h:234:32: warning: right shift count >= width of type [-Wshift-count-overflow]
234 | } else if (likely(((n) >> 32) == 0)) { \
| ^~
include/linux/compiler.h:76:45: note: in definition of macro 'likely'
76 | # define likely(x) __builtin_expect(!!(x), 1)
| ^
drivers/mmc/host/mtk-sd.c:2473:25: note: in expansion of macro 'do_div'
2473 | do_div(hclk_freq, 100);
| ^~~~~~
include/asm-generic/div64.h:238:36: error: passing argument 1 of '__div64_32' from incompatible pointer type [-Werror=incompatible-pointer-types]
238 | __rem = __div64_32(&(n), __base); \
| ^~~~
| |
| u32 * {aka unsigned int *}
drivers/mmc/host/mtk-sd.c:2473:25: note: in expansion of macro 'do_div'
2473 | do_div(hclk_freq, 100);
| ^~~~~~
include/asm-generic/div64.h:213:38: note: expected 'uint64_t *' {aka 'long long unsigned int *'} but argument is of type 'u32 *' {aka 'unsigned int *'}
213 | extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
| ~~~~~~~~~~^~~~~~~~
include/asm-generic/div64.h:222:35: warning: comparison of distinct pointer types lacks a cast
222 | (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
| ^~
drivers/mmc/host/mtk-sd.c:2476:25: note: in expansion of macro 'do_div'
2476 | do_div(hclk_freq, 10);
| ^~~~~~
include/asm-generic/div64.h:234:32: warning: right shift count >= width of type [-Wshift-count-overflow]
234 | } else if (likely(((n) >> 32) == 0)) { \
| ^~
include/linux/compiler.h:76:45: note: in definition of macro 'likely'
76 | # define likely(x) __builtin_expect(!!(x), 1)
| ^
drivers/mmc/host/mtk-sd.c:2476:25: note: in expansion of macro 'do_div'
2476 | do_div(hclk_freq, 10);
| ^~~~~~
include/asm-generic/div64.h:238:36: error: passing argument 1 of '__div64_32' from incompatible pointer type [-Werror=incompatible-pointer-types]
238 | __rem = __div64_32(&(n), __base); \
| ^~~~
| |
| u32 * {aka unsigned int *}
drivers/mmc/host/mtk-sd.c:2476:25: note: in expansion of macro 'do_div'
2476 | do_div(hclk_freq, 10);
| ^~~~~~
include/asm-generic/div64.h:213:38: note: expected 'uint64_t *' {aka 'long long unsigned int *'} but argument is of type 'u32 *' {aka 'unsigned int *'}
213 | extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
| ~~~~~~~~~~^~~~~~~~
>> drivers/mmc/host/mtk-sd.c:2489:11: error: expected '}' before 'else'
2489 | } else {
| ^~~~
cc1: some warnings being treated as errors
vim +2489 drivers/mmc/host/mtk-sd.c
2453
2454 static void msdc_cqe_cit_cal(struct msdc_host *host, u64 timer_ns)
2455 {
2456 struct mmc_host *mmc = mmc_from_priv(host);
2457 struct cqhci_host *cq_host = mmc->cqe_private;
2458 u8 itcfmul;
2459 u32 hclk_freq;
2460 u64 value;
2461
2462 /* Since MSDC CQE uses msdc_hclk as ITCFVAL, so driver should use hclk
2463 * frequency to get the actual time for CIT.
2464 */
2465 if (host->h_clk) {
2466 hclk_freq = clk_get_rate(host->h_clk);
2467 itcfmul = CQHCI_CAP_ITCFMUL(cqhci_readl(cq_host, CQHCI_CAP));
2468 switch (itcfmul) {
2469 case 0x0:
2470 do_div(hclk_freq, 1000);
2471 break;
2472 case 0x1:
2473 do_div(hclk_freq, 100);
2474 break;
2475 case 0x2:
2476 do_div(hclk_freq, 10);
2477 break;
2478 case 0x3:
2479 break;
2480 case 0x4:
2481 hclk_freq = hclk_freq * 10;
2482 break;
2483 default:
2484 host->cq_ssc1_time = 0x40;
2485 return;
2486 value = hclk_freq * timer_ns;
2487 do_div(value, 1000000000ULL);
2488 host->cq_ssc1_time = value;
> 2489 } else {
2490 host->cq_ssc1_time = 0x40;
2491 }
2492 }
2493
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Il 05/06/23 08:01, Wenbin Mei ha scritto:
> CQHCI_SSC1 indicates to CQE the polling period to use when using periodic
> SEND_QUEUE_STATUS(CMD13) polling.
> Since MSDC CQE uses msdc_hclk as ITCFVAL, so driver should use hclk
> frequency to get the actual time.
> The default value 0x1000 that corresponds to 150us for MediaTek SoCs, let's
> decrease it to 0x40 that corresponds to 2.35us, which can improve the
> performance of some eMMC devices.
>
> Signed-off-by: Wenbin Mei <[email protected]>
OK! That's almost good now. There's only one consideration here: if MediaTek
SoCs *require* msdc_hclk to calculate the CIT time, this means that this clock
is critical for CQHCI functionality.
If msdc_hclk is not present, CQHCI cannot work correctly... so you don't have
to cover the case in which there's no msdc_hclk clock: if that's not present,
either fail probing, or disable CQHCI.
> ---
> drivers/mmc/host/cqhci.h | 1 +
> drivers/mmc/host/mtk-sd.c | 47 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 48 insertions(+)
>
> diff --git a/drivers/mmc/host/cqhci.h b/drivers/mmc/host/cqhci.h
> index ba9387ed90eb..292b89ebd978 100644
> --- a/drivers/mmc/host/cqhci.h
> +++ b/drivers/mmc/host/cqhci.h
> @@ -23,6 +23,7 @@
> /* capabilities */
> #define CQHCI_CAP 0x04
> #define CQHCI_CAP_CS 0x10000000 /* Crypto Support */
> +#define CQHCI_CAP_ITCFMUL(x) (((x) & GENMASK(15, 12)) >> 12)
>
> /* configuration */
> #define CQHCI_CFG 0x08
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index edade0e54a0c..c221ef8a6992 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -473,6 +473,7 @@ struct msdc_host {
> struct msdc_tune_para def_tune_para; /* default tune setting */
> struct msdc_tune_para saved_tune_para; /* tune result of CMD21/CMD19 */
> struct cqhci_host *cq_host;
> + u32 cq_ssc1_time;
> };
>
> static const struct mtk_mmc_compatible mt2701_compat = {
> @@ -2450,9 +2451,50 @@ static void msdc_hs400_enhanced_strobe(struct mmc_host *mmc,
> }
> }
>
> +static void msdc_cqe_cit_cal(struct msdc_host *host, u64 timer_ns)
static int msdc_cqe_cit_cal(....)
> +{
> + struct mmc_host *mmc = mmc_from_priv(host);
> + struct cqhci_host *cq_host = mmc->cqe_private;
> + u8 itcfmul;
> + u32 hclk_freq;
hclk_freq should be `unsigned long`, as that's what clk_get_rate() returns.
> + u64 value;
> +
> + /* Since MSDC CQE uses msdc_hclk as ITCFVAL, so driver should use hclk
> + * frequency to get the actual time for CIT.
> + */
/*
* On MediaTek SoCs the MSDC controller's CQE uses msdc_hclk as ITCFVAL
* so we multiply/divide the HCLK frequency by ITCFMUL to calculate the
* Send Status Command Idle Timer (CIT) value.
*/
if (!host->h_clk)
return -EINVAL;
hclk_freq = clk_get_rate(host->h_clk);
itcfmul = CQHCI_CAP_ITFCMUL(cqhci_readl(cq_host, CQHCI_CAP));
switch (itcfmul) {
....
}
> + if (host->h_clk) {
> + hclk_freq = clk_get_rate(host->h_clk);
> + itcfmul = CQHCI_CAP_ITCFMUL(cqhci_readl(cq_host, CQHCI_CAP));
> + switch (itcfmul) {
> + case 0x0:
> + do_div(hclk_freq, 1000);
> + break;
> + case 0x1:
> + do_div(hclk_freq, 100);
> + break;
> + case 0x2:
> + do_div(hclk_freq, 10);
> + break;
> + case 0x3:
> + break;
> + case 0x4:
> + hclk_freq = hclk_freq * 10;
> + break;
> + default:
> + host->cq_ssc1_time = 0x40;
> + return;
> + value = hclk_freq * timer_ns;
> + do_div(value, 1000000000ULL);
> + host->cq_ssc1_time = value;
> + } else {
> + host->cq_ssc1_time = 0x40;
> + }
> +}
> +
> static void msdc_cqe_enable(struct mmc_host *mmc)
> {
> struct msdc_host *host = mmc_priv(mmc);
> + struct cqhci_host *cq_host = mmc->cqe_private;
>
> /* enable cmdq irq */
> writel(MSDC_INT_CMDQ, host->base + MSDC_INTEN);
> @@ -2462,6 +2504,9 @@ static void msdc_cqe_enable(struct mmc_host *mmc)
> msdc_set_busy_timeout(host, 20 * 1000000000ULL, 0);
> /* default read data timeout 1s */
> msdc_set_timeout(host, 1000000000ULL, 0);
> +
> + /* Set the send status command idle timer */
> + cqhci_writel(cq_host, host->cq_ssc1_time, CQHCI_SSC1);
> }
>
> static void msdc_cqe_disable(struct mmc_host *mmc, bool recovery)
> @@ -2803,6 +2848,8 @@ static int msdc_drv_probe(struct platform_device *pdev)
> /* cqhci 16bit length */
> /* 0 size, means 65536 so we don't have to -1 here */
> mmc->max_seg_size = 64 * 1024;
> + /* Reduce CIT to 0x40 that corresponds to 2.35us */
> + msdc_cqe_cit_cal(host, 2350);
ret = msdc_cqe_cit_cal(...)
if (ret)
goto release;
^^^^ either fail probe, or use the eMMC/SD without CQHCI support.
Regards,
Angelo
> }
>
> ret = devm_request_irq(&pdev->dev, host->irq, msdc_irq,
Hi Wenbin,
kernel test robot noticed the following build errors:
[auto build test ERROR on linus/master]
[also build test ERROR on ulf-hansson-mmc-mirror/next v6.4-rc5 next-20230605]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Wenbin-Mei/mmc-mtk-sd-reduce-CIT-for-better-performance/20230605-140238
base: linus/master
patch link: https://lore.kernel.org/r/20230605060107.22044-1-wenbin.mei%40mediatek.com
patch subject: [PATCH v3] mmc: mtk-sd: reduce CIT for better performance
config: x86_64-randconfig-a004-20230605 (https://download.01.org/0day-ci/archive/20230605/[email protected]/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/225e46f420d48f7ad73253636a0553bd5f986435
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Wenbin-Mei/mmc-mtk-sd-reduce-CIT-for-better-performance/20230605-140238
git checkout 225e46f420d48f7ad73253636a0553bd5f986435
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/mmc/host/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> drivers/mmc/host/mtk-sd.c:2489:4: error: expected expression
} else {
^
>> drivers/mmc/host/mtk-sd.c:2495:1: error: function definition is not allowed here
{
^
drivers/mmc/host/mtk-sd.c:2513:1: error: function definition is not allowed here
{
^
drivers/mmc/host/mtk-sd.c:2539:1: error: function definition is not allowed here
{
^
drivers/mmc/host/mtk-sd.c:2549:1: error: function definition is not allowed here
{
^
>> drivers/mmc/host/mtk-sd.c:2577:20: error: use of undeclared identifier 'msdc_cqe_enable'
.enable = msdc_cqe_enable,
^
>> drivers/mmc/host/mtk-sd.c:2578:20: error: use of undeclared identifier 'msdc_cqe_disable'
.disable = msdc_cqe_disable,
^
>> drivers/mmc/host/mtk-sd.c:2579:16: error: use of undeclared identifier 'msdc_cqe_pre_enable'; did you mean 'msdc_cqe_cit_cal'?
.pre_enable = msdc_cqe_pre_enable,
^~~~~~~~~~~~~~~~~~~
msdc_cqe_cit_cal
drivers/mmc/host/mtk-sd.c:2454:13: note: 'msdc_cqe_cit_cal' declared here
static void msdc_cqe_cit_cal(struct msdc_host *host, u64 timer_ns)
^
>> drivers/mmc/host/mtk-sd.c:2580:18: error: use of undeclared identifier 'msdc_cqe_post_disable'
.post_disable = msdc_cqe_post_disable,
^
drivers/mmc/host/mtk-sd.c:2585:1: error: function definition is not allowed here
{
^
drivers/mmc/host/mtk-sd.c:2616:1: error: function definition is not allowed here
{
^
drivers/mmc/host/mtk-sd.c:2668:1: error: function definition is not allowed here
{
^
drivers/mmc/host/mtk-sd.c:2892:1: error: function definition is not allowed here
{
^
drivers/mmc/host/mtk-sd.c:2920:1: error: function definition is not allowed here
{
^
drivers/mmc/host/mtk-sd.c:2947:1: error: function definition is not allowed here
{
^
drivers/mmc/host/mtk-sd.c:2978:1: error: function definition is not allowed here
{
^
drivers/mmc/host/mtk-sd.c:2997:1: error: function definition is not allowed here
{
^
drivers/mmc/host/mtk-sd.c:3016:1: error: function definition is not allowed here
{
^
drivers/mmc/host/mtk-sd.c:3041:1: error: function definition is not allowed here
{
^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.
vim +2489 drivers/mmc/host/mtk-sd.c
2453
2454 static void msdc_cqe_cit_cal(struct msdc_host *host, u64 timer_ns)
2455 {
2456 struct mmc_host *mmc = mmc_from_priv(host);
2457 struct cqhci_host *cq_host = mmc->cqe_private;
2458 u8 itcfmul;
2459 u32 hclk_freq;
2460 u64 value;
2461
2462 /* Since MSDC CQE uses msdc_hclk as ITCFVAL, so driver should use hclk
2463 * frequency to get the actual time for CIT.
2464 */
2465 if (host->h_clk) {
2466 hclk_freq = clk_get_rate(host->h_clk);
2467 itcfmul = CQHCI_CAP_ITCFMUL(cqhci_readl(cq_host, CQHCI_CAP));
2468 switch (itcfmul) {
2469 case 0x0:
2470 do_div(hclk_freq, 1000);
2471 break;
2472 case 0x1:
2473 do_div(hclk_freq, 100);
2474 break;
2475 case 0x2:
2476 do_div(hclk_freq, 10);
2477 break;
2478 case 0x3:
2479 break;
2480 case 0x4:
2481 hclk_freq = hclk_freq * 10;
2482 break;
2483 default:
2484 host->cq_ssc1_time = 0x40;
2485 return;
2486 value = hclk_freq * timer_ns;
2487 do_div(value, 1000000000ULL);
2488 host->cq_ssc1_time = value;
> 2489 } else {
2490 host->cq_ssc1_time = 0x40;
2491 }
2492 }
2493
2494 static void msdc_cqe_enable(struct mmc_host *mmc)
> 2495 {
2496 struct msdc_host *host = mmc_priv(mmc);
2497 struct cqhci_host *cq_host = mmc->cqe_private;
2498
2499 /* enable cmdq irq */
2500 writel(MSDC_INT_CMDQ, host->base + MSDC_INTEN);
2501 /* enable busy check */
2502 sdr_set_bits(host->base + MSDC_PATCH_BIT1, MSDC_PB1_BUSY_CHECK_SEL);
2503 /* default write data / busy timeout 20s */
2504 msdc_set_busy_timeout(host, 20 * 1000000000ULL, 0);
2505 /* default read data timeout 1s */
2506 msdc_set_timeout(host, 1000000000ULL, 0);
2507
2508 /* Set the send status command idle timer */
2509 cqhci_writel(cq_host, host->cq_ssc1_time, CQHCI_SSC1);
2510 }
2511
2512 static void msdc_cqe_disable(struct mmc_host *mmc, bool recovery)
2513 {
2514 struct msdc_host *host = mmc_priv(mmc);
2515 unsigned int val = 0;
2516
2517 /* disable cmdq irq */
2518 sdr_clr_bits(host->base + MSDC_INTEN, MSDC_INT_CMDQ);
2519 /* disable busy check */
2520 sdr_clr_bits(host->base + MSDC_PATCH_BIT1, MSDC_PB1_BUSY_CHECK_SEL);
2521
2522 val = readl(host->base + MSDC_INT);
2523 writel(val, host->base + MSDC_INT);
2524
2525 if (recovery) {
2526 sdr_set_field(host->base + MSDC_DMA_CTRL,
2527 MSDC_DMA_CTRL_STOP, 1);
2528 if (WARN_ON(readl_poll_timeout(host->base + MSDC_DMA_CTRL, val,
2529 !(val & MSDC_DMA_CTRL_STOP), 1, 3000)))
2530 return;
2531 if (WARN_ON(readl_poll_timeout(host->base + MSDC_DMA_CFG, val,
2532 !(val & MSDC_DMA_CFG_STS), 1, 3000)))
2533 return;
2534 msdc_reset_hw(host);
2535 }
2536 }
2537
2538 static void msdc_cqe_pre_enable(struct mmc_host *mmc)
2539 {
2540 struct cqhci_host *cq_host = mmc->cqe_private;
2541 u32 reg;
2542
2543 reg = cqhci_readl(cq_host, CQHCI_CFG);
2544 reg |= CQHCI_ENABLE;
2545 cqhci_writel(cq_host, reg, CQHCI_CFG);
2546 }
2547
2548 static void msdc_cqe_post_disable(struct mmc_host *mmc)
2549 {
2550 struct cqhci_host *cq_host = mmc->cqe_private;
2551 u32 reg;
2552
2553 reg = cqhci_readl(cq_host, CQHCI_CFG);
2554 reg &= ~CQHCI_ENABLE;
2555 cqhci_writel(cq_host, reg, CQHCI_CFG);
2556 }
2557
2558 static const struct mmc_host_ops mt_msdc_ops = {
2559 .post_req = msdc_post_req,
2560 .pre_req = msdc_pre_req,
2561 .request = msdc_ops_request,
2562 .set_ios = msdc_ops_set_ios,
2563 .get_ro = mmc_gpio_get_ro,
2564 .get_cd = msdc_get_cd,
2565 .hs400_enhanced_strobe = msdc_hs400_enhanced_strobe,
2566 .enable_sdio_irq = msdc_enable_sdio_irq,
2567 .ack_sdio_irq = msdc_ack_sdio_irq,
2568 .start_signal_voltage_switch = msdc_ops_switch_volt,
2569 .card_busy = msdc_card_busy,
2570 .execute_tuning = msdc_execute_tuning,
2571 .prepare_hs400_tuning = msdc_prepare_hs400_tuning,
2572 .execute_hs400_tuning = msdc_execute_hs400_tuning,
2573 .card_hw_reset = msdc_hw_reset,
2574 };
2575
2576 static const struct cqhci_host_ops msdc_cmdq_ops = {
> 2577 .enable = msdc_cqe_enable,
> 2578 .disable = msdc_cqe_disable,
> 2579 .pre_enable = msdc_cqe_pre_enable,
> 2580 .post_disable = msdc_cqe_post_disable,
2581 };
2582
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Wenbin,
kernel test robot noticed the following build errors:
[auto build test ERROR on linus/master]
[also build test ERROR on ulf-hansson-mmc-mirror/next v6.4-rc5 next-20230605]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Wenbin-Mei/mmc-mtk-sd-reduce-CIT-for-better-performance/20230605-140238
base: linus/master
patch link: https://lore.kernel.org/r/20230605060107.22044-1-wenbin.mei%40mediatek.com
patch subject: [PATCH v3] mmc: mtk-sd: reduce CIT for better performance
config: openrisc-randconfig-r035-20230605 (https://download.01.org/0day-ci/archive/20230605/[email protected]/config)
compiler: or1k-linux-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/225e46f420d48f7ad73253636a0553bd5f986435
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Wenbin-Mei/mmc-mtk-sd-reduce-CIT-for-better-performance/20230605-140238
git checkout 225e46f420d48f7ad73253636a0553bd5f986435
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=openrisc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=openrisc SHELL=/bin/bash drivers/mmc/host/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All error/warnings (new ones prefixed by >>):
In file included from ./arch/openrisc/include/generated/asm/div64.h:1,
from include/linux/math.h:6,
from include/linux/math64.h:6,
from include/linux/time.h:6,
from include/linux/stat.h:19,
from include/linux/module.h:13,
from drivers/mmc/host/mtk-sd.c:7:
drivers/mmc/host/mtk-sd.c: In function 'msdc_cqe_cit_cal':
>> include/asm-generic/div64.h:222:35: warning: comparison of distinct pointer types lacks a cast
222 | (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
| ^~
drivers/mmc/host/mtk-sd.c:2470:25: note: in expansion of macro 'do_div'
2470 | do_div(hclk_freq, 1000);
| ^~~~~~
In file included from include/linux/build_bug.h:5,
from include/linux/container_of.h:5,
from include/linux/list.h:5,
from include/linux/module.h:12:
>> include/asm-generic/div64.h:234:32: warning: right shift count >= width of type [-Wshift-count-overflow]
234 | } else if (likely(((n) >> 32) == 0)) { \
| ^~
include/linux/compiler.h:76:45: note: in definition of macro 'likely'
76 | # define likely(x) __builtin_expect(!!(x), 1)
| ^
drivers/mmc/host/mtk-sd.c:2470:25: note: in expansion of macro 'do_div'
2470 | do_div(hclk_freq, 1000);
| ^~~~~~
>> include/asm-generic/div64.h:238:36: error: passing argument 1 of '__div64_32' from incompatible pointer type [-Werror=incompatible-pointer-types]
238 | __rem = __div64_32(&(n), __base); \
| ^~~~
| |
| u32 * {aka unsigned int *}
drivers/mmc/host/mtk-sd.c:2470:25: note: in expansion of macro 'do_div'
2470 | do_div(hclk_freq, 1000);
| ^~~~~~
include/asm-generic/div64.h:213:38: note: expected 'uint64_t *' {aka 'long long unsigned int *'} but argument is of type 'u32 *' {aka 'unsigned int *'}
213 | extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
| ~~~~~~~~~~^~~~~~~~
>> include/asm-generic/div64.h:222:35: warning: comparison of distinct pointer types lacks a cast
222 | (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
| ^~
drivers/mmc/host/mtk-sd.c:2473:25: note: in expansion of macro 'do_div'
2473 | do_div(hclk_freq, 100);
| ^~~~~~
>> include/asm-generic/div64.h:234:32: warning: right shift count >= width of type [-Wshift-count-overflow]
234 | } else if (likely(((n) >> 32) == 0)) { \
| ^~
include/linux/compiler.h:76:45: note: in definition of macro 'likely'
76 | # define likely(x) __builtin_expect(!!(x), 1)
| ^
drivers/mmc/host/mtk-sd.c:2473:25: note: in expansion of macro 'do_div'
2473 | do_div(hclk_freq, 100);
| ^~~~~~
>> include/asm-generic/div64.h:238:36: error: passing argument 1 of '__div64_32' from incompatible pointer type [-Werror=incompatible-pointer-types]
238 | __rem = __div64_32(&(n), __base); \
| ^~~~
| |
| u32 * {aka unsigned int *}
drivers/mmc/host/mtk-sd.c:2473:25: note: in expansion of macro 'do_div'
2473 | do_div(hclk_freq, 100);
| ^~~~~~
include/asm-generic/div64.h:213:38: note: expected 'uint64_t *' {aka 'long long unsigned int *'} but argument is of type 'u32 *' {aka 'unsigned int *'}
213 | extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
| ~~~~~~~~~~^~~~~~~~
>> include/asm-generic/div64.h:222:35: warning: comparison of distinct pointer types lacks a cast
222 | (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
| ^~
drivers/mmc/host/mtk-sd.c:2476:25: note: in expansion of macro 'do_div'
2476 | do_div(hclk_freq, 10);
| ^~~~~~
>> include/asm-generic/div64.h:234:32: warning: right shift count >= width of type [-Wshift-count-overflow]
234 | } else if (likely(((n) >> 32) == 0)) { \
| ^~
include/linux/compiler.h:76:45: note: in definition of macro 'likely'
76 | # define likely(x) __builtin_expect(!!(x), 1)
| ^
drivers/mmc/host/mtk-sd.c:2476:25: note: in expansion of macro 'do_div'
2476 | do_div(hclk_freq, 10);
| ^~~~~~
>> include/asm-generic/div64.h:238:36: error: passing argument 1 of '__div64_32' from incompatible pointer type [-Werror=incompatible-pointer-types]
238 | __rem = __div64_32(&(n), __base); \
| ^~~~
| |
| u32 * {aka unsigned int *}
drivers/mmc/host/mtk-sd.c:2476:25: note: in expansion of macro 'do_div'
2476 | do_div(hclk_freq, 10);
| ^~~~~~
include/asm-generic/div64.h:213:38: note: expected 'uint64_t *' {aka 'long long unsigned int *'} but argument is of type 'u32 *' {aka 'unsigned int *'}
213 | extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
| ~~~~~~~~~~^~~~~~~~
>> drivers/mmc/host/mtk-sd.c:2489:11: error: expected '}' before 'else'
2489 | } else {
| ^~~~
cc1: some warnings being treated as errors
vim +2489 drivers/mmc/host/mtk-sd.c
2453
2454 static void msdc_cqe_cit_cal(struct msdc_host *host, u64 timer_ns)
2455 {
2456 struct mmc_host *mmc = mmc_from_priv(host);
2457 struct cqhci_host *cq_host = mmc->cqe_private;
2458 u8 itcfmul;
2459 u32 hclk_freq;
2460 u64 value;
2461
2462 /* Since MSDC CQE uses msdc_hclk as ITCFVAL, so driver should use hclk
2463 * frequency to get the actual time for CIT.
2464 */
2465 if (host->h_clk) {
2466 hclk_freq = clk_get_rate(host->h_clk);
2467 itcfmul = CQHCI_CAP_ITCFMUL(cqhci_readl(cq_host, CQHCI_CAP));
2468 switch (itcfmul) {
2469 case 0x0:
2470 do_div(hclk_freq, 1000);
2471 break;
2472 case 0x1:
2473 do_div(hclk_freq, 100);
2474 break;
2475 case 0x2:
2476 do_div(hclk_freq, 10);
2477 break;
2478 case 0x3:
2479 break;
2480 case 0x4:
2481 hclk_freq = hclk_freq * 10;
2482 break;
2483 default:
2484 host->cq_ssc1_time = 0x40;
2485 return;
2486 value = hclk_freq * timer_ns;
2487 do_div(value, 1000000000ULL);
2488 host->cq_ssc1_time = value;
> 2489 } else {
2490 host->cq_ssc1_time = 0x40;
2491 }
2492 }
2493
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Mon, 2023-06-05 at 10:48 +0200, AngeloGioacchino Del Regno wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> Il 05/06/23 08:01, Wenbin Mei ha scritto:
> > CQHCI_SSC1 indicates to CQE the polling period to use when using
> periodic
> > SEND_QUEUE_STATUS(CMD13) polling.
> > Since MSDC CQE uses msdc_hclk as ITCFVAL, so driver should use hclk
> > frequency to get the actual time.
> > The default value 0x1000 that corresponds to 150us for MediaTek
> SoCs, let's
> > decrease it to 0x40 that corresponds to 2.35us, which can improve
> the
> > performance of some eMMC devices.
> >
> > Signed-off-by: Wenbin Mei <[email protected]>
>
> OK! That's almost good now. There's only one consideration here: if
> MediaTek
> SoCs *require* msdc_hclk to calculate the CIT time, this means that
> this clock
> is critical for CQHCI functionality.
>
> If msdc_hclk is not present, CQHCI cannot work correctly... so you
> don't have
> to cover the case in which there's no msdc_hclk clock: if that's not
> present,
> either fail probing, or disable CQHCI.
That's right, i will remove the else case.
Begards,
Wenbin
>
> > ---
> > drivers/mmc/host/cqhci.h | 1 +
> > drivers/mmc/host/mtk-sd.c | 47
> +++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 48 insertions(+)
> >
> > diff --git a/drivers/mmc/host/cqhci.h b/drivers/mmc/host/cqhci.h
> > index ba9387ed90eb..292b89ebd978 100644
> > --- a/drivers/mmc/host/cqhci.h
> > +++ b/drivers/mmc/host/cqhci.h
> > @@ -23,6 +23,7 @@
> > /* capabilities */
> > #define CQHCI_CAP0x04
> > #define CQHCI_CAP_CS0x10000000 /* Crypto Support */
> > +#define CQHCI_CAP_ITCFMUL(x)(((x) & GENMASK(15, 12)) >> 12)
> >
> > /* configuration */
> > #define CQHCI_CFG0x08
> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> > index edade0e54a0c..c221ef8a6992 100644
> > --- a/drivers/mmc/host/mtk-sd.c
> > +++ b/drivers/mmc/host/mtk-sd.c
> > @@ -473,6 +473,7 @@ struct msdc_host {
> > struct msdc_tune_para def_tune_para; /* default tune setting */
> > struct msdc_tune_para saved_tune_para; /* tune result of
> CMD21/CMD19 */
> > struct cqhci_host *cq_host;
> > +u32 cq_ssc1_time;
> > };
> >
> > static const struct mtk_mmc_compatible mt2701_compat = {
> > @@ -2450,9 +2451,50 @@ static void
> msdc_hs400_enhanced_strobe(struct mmc_host *mmc,
> > }
> > }
> >
> > +static void msdc_cqe_cit_cal(struct msdc_host *host, u64 timer_ns)
>
> static int msdc_cqe_cit_cal(....)
>
> > +{
> > +struct mmc_host *mmc = mmc_from_priv(host);
> > +struct cqhci_host *cq_host = mmc->cqe_private;
> > +u8 itcfmul;
> > +u32 hclk_freq;
>
> hclk_freq should be `unsigned long`, as that's what clk_get_rate()
> returns.
>
> > +u64 value;
> > +
> > +/* Since MSDC CQE uses msdc_hclk as ITCFVAL, so driver should use
> hclk
> > + * frequency to get the actual time for CIT.
> > + */
>
> /*
> * On MediaTek SoCs the MSDC controller's CQE uses msdc_hclk as
> ITCFVAL
> * so we multiply/divide the HCLK frequency by ITCFMUL to calculate
> the
> * Send Status Command Idle Timer (CIT) value.
> */
> if (!host->h_clk)
> return -EINVAL;
>
> hclk_freq = clk_get_rate(host->h_clk);
> itcfmul = CQHCI_CAP_ITFCMUL(cqhci_readl(cq_host, CQHCI_CAP));
> switch (itcfmul) {
> ....
> }
>
> > +if (host->h_clk) {
> > +hclk_freq = clk_get_rate(host->h_clk);
> > +itcfmul = CQHCI_CAP_ITCFMUL(cqhci_readl(cq_host, CQHCI_CAP));
> > +switch (itcfmul) {
> > +case 0x0:
> > +do_div(hclk_freq, 1000);
> > +break;
> > +case 0x1:
> > +do_div(hclk_freq, 100);
> > +break;
> > +case 0x2:
> > +do_div(hclk_freq, 10);
> > +break;
> > +case 0x3:
> > +break;
> > +case 0x4:
> > +hclk_freq = hclk_freq * 10;
> > +break;
> > +default:
> > +host->cq_ssc1_time = 0x40;
> > +return;
> > +value = hclk_freq * timer_ns;
> > +do_div(value, 1000000000ULL);
> > +host->cq_ssc1_time = value;
> > +} else {
> > +host->cq_ssc1_time = 0x40;
> > +}
> > +}
> > +
> > static void msdc_cqe_enable(struct mmc_host *mmc)
> > {
> > struct msdc_host *host = mmc_priv(mmc);
> > +struct cqhci_host *cq_host = mmc->cqe_private;
> >
> > /* enable cmdq irq */
> > writel(MSDC_INT_CMDQ, host->base + MSDC_INTEN);
> > @@ -2462,6 +2504,9 @@ static void msdc_cqe_enable(struct mmc_host
> *mmc)
> > msdc_set_busy_timeout(host, 20 * 1000000000ULL, 0);
> > /* default read data timeout 1s */
> > msdc_set_timeout(host, 1000000000ULL, 0);
> > +
> > +/* Set the send status command idle timer */
> > +cqhci_writel(cq_host, host->cq_ssc1_time, CQHCI_SSC1);
> > }
> >
> > static void msdc_cqe_disable(struct mmc_host *mmc, bool recovery)
> > @@ -2803,6 +2848,8 @@ static int msdc_drv_probe(struct
> platform_device *pdev)
> > /* cqhci 16bit length */
> > /* 0 size, means 65536 so we don't have to -1 here */
> > mmc->max_seg_size = 64 * 1024;
> > +/* Reduce CIT to 0x40 that corresponds to 2.35us */
> > +msdc_cqe_cit_cal(host, 2350);
>
> ret = msdc_cqe_cit_cal(...)
> if (ret)
> goto release;
>
> ^^^^ either fail probe, or use the eMMC/SD without CQHCI support.
>
> Regards,
> Angelo
>
> > }
> >
> > ret = devm_request_irq(&pdev->dev, host->irq, msdc_irq,
>
On Mon, 2023-06-05 at 10:48 +0200, AngeloGioacchino Del Regno wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> Il 05/06/23 08:01, Wenbin Mei ha scritto:
> > CQHCI_SSC1 indicates to CQE the polling period to use when using
> periodic
> > SEND_QUEUE_STATUS(CMD13) polling.
> > Since MSDC CQE uses msdc_hclk as ITCFVAL, so driver should use hclk
> > frequency to get the actual time.
> > The default value 0x1000 that corresponds to 150us for MediaTek
> SoCs, let's
> > decrease it to 0x40 that corresponds to 2.35us, which can improve
> the
> > performance of some eMMC devices.
> >
> > Signed-off-by: Wenbin Mei <[email protected]>
>
> OK! That's almost good now. There's only one consideration here: if
> MediaTek
> SoCs *require* msdc_hclk to calculate the CIT time, this means that
> this clock
> is critical for CQHCI functionality.
>
> If msdc_hclk is not present, CQHCI cannot work correctly... so you
> don't have
> to cover the case in which there's no msdc_hclk clock: if that's not
> present,
> either fail probing, or disable CQHCI.
>
> > ---
> > drivers/mmc/host/cqhci.h | 1 +
> > drivers/mmc/host/mtk-sd.c | 47
> +++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 48 insertions(+)
> >
> > diff --git a/drivers/mmc/host/cqhci.h b/drivers/mmc/host/cqhci.h
> > index ba9387ed90eb..292b89ebd978 100644
> > --- a/drivers/mmc/host/cqhci.h
> > +++ b/drivers/mmc/host/cqhci.h
> > @@ -23,6 +23,7 @@
> > /* capabilities */
> > #define CQHCI_CAP0x04
> > #define CQHCI_CAP_CS0x10000000 /* Crypto Support */
> > +#define CQHCI_CAP_ITCFMUL(x)(((x) & GENMASK(15, 12)) >> 12)
> >
> > /* configuration */
> > #define CQHCI_CFG0x08
> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> > index edade0e54a0c..c221ef8a6992 100644
> > --- a/drivers/mmc/host/mtk-sd.c
> > +++ b/drivers/mmc/host/mtk-sd.c
> > @@ -473,6 +473,7 @@ struct msdc_host {
> > struct msdc_tune_para def_tune_para; /* default tune setting */
> > struct msdc_tune_para saved_tune_para; /* tune result of
> CMD21/CMD19 */
> > struct cqhci_host *cq_host;
> > +u32 cq_ssc1_time;
> > };
> >
> > static const struct mtk_mmc_compatible mt2701_compat = {
> > @@ -2450,9 +2451,50 @@ static void
> msdc_hs400_enhanced_strobe(struct mmc_host *mmc,
> > }
> > }
> >
> > +static void msdc_cqe_cit_cal(struct msdc_host *host, u64 timer_ns)
>
> static int msdc_cqe_cit_cal(....)
>
Sorry, I missed this comment.
I think there is no need to return a value.
Becuase msdc_hclk is exist, and if not present, it will return earily.
Even if it goes to the default case in the switch flow, we will assign
a default value.
So I think it's better to return null, do you think it is okay?
> > +{
> > +struct mmc_host *mmc = mmc_from_priv(host);
> > +struct cqhci_host *cq_host = mmc->cqe_private;
> > +u8 itcfmul;
> > +u32 hclk_freq;
>
> hclk_freq should be `unsigned long`, as that's what clk_get_rate()
> returns.
>
I will change it in the v5 version.
> > +u64 value;
> > +
> > +/* Since MSDC CQE uses msdc_hclk as ITCFVAL, so driver should use
> hclk
> > + * frequency to get the actual time for CIT.
> > + */
>
> /*
> * On MediaTek SoCs the MSDC controller's CQE uses msdc_hclk as
> ITCFVAL
> * so we multiply/divide the HCLK frequency by ITCFMUL to calculate
> the
> * Send Status Command Idle Timer (CIT) value.
> */
I will change it in the v5 version.
> if (!host->h_clk)
> return -EINVAL;
>
This change should be unnecessary, because it has been checked in
msdc_drv_probe() function.
> hclk_freq = clk_get_rate(host->h_clk);
> itcfmul = CQHCI_CAP_ITFCMUL(cqhci_readl(cq_host, CQHCI_CAP));
> switch (itcfmul) {
> ....
> }
>
> > +if (host->h_clk) {
> > +hclk_freq = clk_get_rate(host->h_clk);
> > +itcfmul = CQHCI_CAP_ITCFMUL(cqhci_readl(cq_host, CQHCI_CAP));
> > +switch (itcfmul) {
> > +case 0x0:
> > +do_div(hclk_freq, 1000);
> > +break;
> > +case 0x1:
> > +do_div(hclk_freq, 100);
> > +break;
> > +case 0x2:
> > +do_div(hclk_freq, 10);
> > +break;
> > +case 0x3:
> > +break;
> > +case 0x4:
> > +hclk_freq = hclk_freq * 10;
> > +break;
> > +default:
> > +host->cq_ssc1_time = 0x40;
> > +return;
> > +value = hclk_freq * timer_ns;
> > +do_div(value, 1000000000ULL);
> > +host->cq_ssc1_time = value;
> > +} else {
> > +host->cq_ssc1_time = 0x40;
> > +}
> > +}
> > +
> > static void msdc_cqe_enable(struct mmc_host *mmc)
> > {
> > struct msdc_host *host = mmc_priv(mmc);
> > +struct cqhci_host *cq_host = mmc->cqe_private;
> >
> > /* enable cmdq irq */
> > writel(MSDC_INT_CMDQ, host->base + MSDC_INTEN);
> > @@ -2462,6 +2504,9 @@ static void msdc_cqe_enable(struct mmc_host
> *mmc)
> > msdc_set_busy_timeout(host, 20 * 1000000000ULL, 0);
> > /* default read data timeout 1s */
> > msdc_set_timeout(host, 1000000000ULL, 0);
> > +
> > +/* Set the send status command idle timer */
> > +cqhci_writel(cq_host, host->cq_ssc1_time, CQHCI_SSC1);
> > }
> >
> > static void msdc_cqe_disable(struct mmc_host *mmc, bool recovery)
> > @@ -2803,6 +2848,8 @@ static int msdc_drv_probe(struct
> platform_device *pdev)
> > /* cqhci 16bit length */
> > /* 0 size, means 65536 so we don't have to -1 here */
> > mmc->max_seg_size = 64 * 1024;
> > +/* Reduce CIT to 0x40 that corresponds to 2.35us */
> > +msdc_cqe_cit_cal(host, 2350);
>
> ret = msdc_cqe_cit_cal(...)
> if (ret)
> goto release;
>
> ^^^^ either fail probe, or use the eMMC/SD without CQHCI support.
>
There is no need to return a value, so there will be no check here.
> Regards,
> Angelo
>
> > }
> >
> > ret = devm_request_irq(&pdev->dev, host->irq, msdc_irq,
>
Il 06/06/23 10:17, Wenbin Mei (梅文彬) ha scritto:
> On Mon, 2023-06-05 at 10:48 +0200, AngeloGioacchino Del Regno wrote:
>>
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>> Il 05/06/23 08:01, Wenbin Mei ha scritto:
>>> CQHCI_SSC1 indicates to CQE the polling period to use when using
>> periodic
>>> SEND_QUEUE_STATUS(CMD13) polling.
>>> Since MSDC CQE uses msdc_hclk as ITCFVAL, so driver should use hclk
>>> frequency to get the actual time.
>>> The default value 0x1000 that corresponds to 150us for MediaTek
>> SoCs, let's
>>> decrease it to 0x40 that corresponds to 2.35us, which can improve
>> the
>>> performance of some eMMC devices.
>>>
>>> Signed-off-by: Wenbin Mei <[email protected]>
>>
>> OK! That's almost good now. There's only one consideration here: if
>> MediaTek
>> SoCs *require* msdc_hclk to calculate the CIT time, this means that
>> this clock
>> is critical for CQHCI functionality.
>>
>> If msdc_hclk is not present, CQHCI cannot work correctly... so you
>> don't have
>> to cover the case in which there's no msdc_hclk clock: if that's not
>> present,
>> either fail probing, or disable CQHCI.
>>
>>> ---
>>> drivers/mmc/host/cqhci.h | 1 +
>>> drivers/mmc/host/mtk-sd.c | 47
>> +++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 48 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/cqhci.h b/drivers/mmc/host/cqhci.h
>>> index ba9387ed90eb..292b89ebd978 100644
>>> --- a/drivers/mmc/host/cqhci.h
>>> +++ b/drivers/mmc/host/cqhci.h
>>> @@ -23,6 +23,7 @@
>>> /* capabilities */
>>> #define CQHCI_CAP0x04
>>> #define CQHCI_CAP_CS0x10000000 /* Crypto Support */
>>> +#define CQHCI_CAP_ITCFMUL(x)(((x) & GENMASK(15, 12)) >> 12)
>>>
>>> /* configuration */
>>> #define CQHCI_CFG0x08
>>> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
>>> index edade0e54a0c..c221ef8a6992 100644
>>> --- a/drivers/mmc/host/mtk-sd.c
>>> +++ b/drivers/mmc/host/mtk-sd.c
>>> @@ -473,6 +473,7 @@ struct msdc_host {
>>> struct msdc_tune_para def_tune_para; /* default tune setting */
>>> struct msdc_tune_para saved_tune_para; /* tune result of
>> CMD21/CMD19 */
>>> struct cqhci_host *cq_host;
>>> +u32 cq_ssc1_time;
>>> };
>>>
>>> static const struct mtk_mmc_compatible mt2701_compat = {
>>> @@ -2450,9 +2451,50 @@ static void
>> msdc_hs400_enhanced_strobe(struct mmc_host *mmc,
>>> }
>>> }
>>>
>>> +static void msdc_cqe_cit_cal(struct msdc_host *host, u64 timer_ns)
>>
>> static int msdc_cqe_cit_cal(....)
>>
> Sorry, I missed this comment.
> I think there is no need to return a value.
> Becuase msdc_hclk is exist, and if not present, it will return earily.
> Even if it goes to the default case in the switch flow, we will assign
> a default value.
> So I think it's better to return null, do you think it is okay?
>
Yeah ignore this comment; I've noticed that HCLK is already mandatory, otherwise
the probe function will fail earlier anyway.
Thanks,
Angelo