[PATCH V1 0/2] Introduce new vendor op and export few symbols

[PATCH V1 0/2] Introduce new vendor op and export few symbols

[Sarthak Garg]

For our earlier discussions on clock scaling and partial init features
we have come up with a new design where we have moved the entire logic
for both the features in our vendor related files.

But to support this new design we need a vendor op in
_mmc_suspend/_mmc_resume functions to control our feature functionality
in suspend/resume paths.
Moreover export of few symbols is also needed to make core layer
functions accessible to our vendor module.

Old discussion for Clock scaling feature :
https://patchwork.kernel.org/project/linux-mmc/cover/1571668177-3766-1-git-send-email-rampraka@codeaurora.org/

Old discussion for Partial init feature :
https://patchwork.kernel.org/project/linux-mmc/patch/1650963852-4173-1-git-send-email-quic_spathi@quicinc.com/

Hence introduce new vendor op in suspend/resume and export few symbols
nedeed for our feature.

Sarthak Garg (2):
  mmc: core: Define new vendor ops to enable internal features
  mmc: core: Export core functions to let vendors use for their features

 drivers/mmc/core/core.c    |  6 ++++++
 drivers/mmc/core/host.c    |  1 +
 drivers/mmc/core/mmc.c     | 31 ++++++++++++++++++++++---------
 drivers/mmc/core/mmc_ops.c |  1 +
 drivers/mmc/core/queue.c   |  1 +
 include/linux/mmc/host.h   |  4 ++++
 6 files changed, 35 insertions(+), 9 deletions(-)

-- 
2.17.1

[PATCH V1 1/2] mmc: core: Define new vendor ops to enable internal features

[Sarthak Garg]

Define new ops to let vendor enable internal features in
mmc_suspend/resume paths like partial init feature.

Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
---
 drivers/mmc/core/mmc.c   | 13 ++++++++++---
 include/linux/mmc/host.h |  4 ++++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 89cd48fcec79..32386e4644df 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -2112,9 +2112,11 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
 	    ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend ||
 	     (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND)))
 		err = mmc_poweroff_notify(host->card, notify_type);
-	else if (mmc_can_sleep(host->card))
+	else if (mmc_can_sleep(host->card)) {
+		if (host->ops->cache_card_properties)
+			host->ops->cache_card_properties(host);
 		err = mmc_sleep(host);
-	else if (!mmc_host_is_spi(host))
+	} else if (!mmc_host_is_spi(host))
 		err = mmc_deselect_cards(host);
 
 	if (!err) {
@@ -2149,6 +2151,7 @@ static int mmc_suspend(struct mmc_host *host)
 static int _mmc_resume(struct mmc_host *host)
 {
 	int err = 0;
+	bool partial_init_success = false;
 
 	mmc_claim_host(host);
 
@@ -2156,7 +2159,11 @@ static int _mmc_resume(struct mmc_host *host)
 		goto out;
 
 	mmc_power_up(host, host->card->ocr);
-	err = mmc_init_card(host, host->card->ocr, host->card);
+
+	if (host->ops->partial_init_card)
+		partial_init_success = host->ops->partial_init_card(host);
+	if (!partial_init_success)
+		err = mmc_init_card(host, host->card->ocr, host->card);
 	mmc_card_clr_suspended(host->card);
 
 out:
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 461d1543893b..0a796a34b83d 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -212,6 +212,10 @@ struct mmc_host_ops {
 
 	/* Initialize an SD express card, mandatory for MMC_CAP2_SD_EXP. */
 	int	(*init_sd_express)(struct mmc_host *host, struct mmc_ios *ios);
+
+	void	(*cache_card_properties)(struct mmc_host *host);
+	bool	(*partial_init_card)(struct mmc_host *host);
+
 };
 
 struct mmc_cqe_ops {
-- 
2.17.1

[PATCH V1 2/2] mmc: core: Export core functions to let vendors use for their features

[Sarthak Garg]

Export core functions to let vendors use for their internal features.

Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
---
 drivers/mmc/core/core.c    |  6 ++++++
 drivers/mmc/core/host.c    |  1 +
 drivers/mmc/core/mmc.c     | 18 ++++++++++++------
 drivers/mmc/core/mmc_ops.c |  1 +
 drivers/mmc/core/queue.c   |  1 +
 5 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 3d3e0ca52614..ed44b65f19e0 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -916,6 +916,7 @@ void mmc_set_clock(struct mmc_host *host, unsigned int hz)
 	host->ios.clock = hz;
 	mmc_set_ios(host);
 }
+EXPORT_SYMBOL_GPL(mmc_set_clock);
 
 int mmc_execute_tuning(struct mmc_card *card)
 {
@@ -950,6 +951,7 @@ int mmc_execute_tuning(struct mmc_card *card)
 
 	return err;
 }
+EXPORT_SYMBOL_GPL(mmc_execute_tuning);
 
 /*
  * Change the bus mode (open drain/push-pull) of a host.
@@ -959,6 +961,7 @@ void mmc_set_bus_mode(struct mmc_host *host, unsigned int mode)
 	host->ios.bus_mode = mode;
 	mmc_set_ios(host);
 }
+EXPORT_SYMBOL_GPL(mmc_set_bus_mode);
 
 /*
  * Change data bus width of a host.
@@ -968,6 +971,7 @@ void mmc_set_bus_width(struct mmc_host *host, unsigned int width)
 	host->ios.bus_width = width;
 	mmc_set_ios(host);
 }
+EXPORT_SYMBOL_GPL(mmc_set_bus_width);
 
 /*
  * Set initial state after a power cycle or a hw_reset.
@@ -1001,6 +1005,7 @@ void mmc_set_initial_state(struct mmc_host *host)
 
 	mmc_crypto_set_initial_state(host);
 }
+EXPORT_SYMBOL_GPL(mmc_set_initial_state);
 
 /**
  * mmc_vdd_to_ocrbitnum - Convert a voltage to the OCR bit number
@@ -1270,6 +1275,7 @@ void mmc_set_timing(struct mmc_host *host, unsigned int timing)
 	host->ios.timing = timing;
 	mmc_set_ios(host);
 }
+EXPORT_SYMBOL_GPL(mmc_set_timing);
 
 /*
  * Select appropriate driver type for host.
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 76900f67c782..1c5eb1d9d585 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -166,6 +166,7 @@ void mmc_retune_hold(struct mmc_host *host)
 		host->retune_now = 1;
 	host->hold_retune += 1;
 }
+EXPORT_SYMBOL(mmc_retune_hold);
 
 void mmc_retune_release(struct mmc_host *host)
 {
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 32386e4644df..b984a4f90535 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1002,7 +1002,7 @@ static void mmc_set_bus_speed(struct mmc_card *card)
  * If the bus width is changed successfully, return the selected width value.
  * Zero is returned instead of error value if the wide width is not supported.
  */
-static int mmc_select_bus_width(struct mmc_card *card)
+int mmc_select_bus_width(struct mmc_card *card)
 {
 	static unsigned ext_csd_bits[] = {
 		EXT_CSD_BUS_WIDTH_8,
@@ -1067,11 +1067,12 @@ static int mmc_select_bus_width(struct mmc_card *card)
 
 	return err;
 }
+EXPORT_SYMBOL_GPL(mmc_select_bus_width);
 
 /*
  * Switch to the high-speed mode
  */
-static int mmc_select_hs(struct mmc_card *card)
+int mmc_select_hs(struct mmc_card *card)
 {
 	int err;
 
@@ -1085,11 +1086,12 @@ static int mmc_select_hs(struct mmc_card *card)
 
 	return err;
 }
+EXPORT_SYMBOL_GPL(mmc_select_hs);
 
 /*
  * Activate wide bus and DDR if supported.
  */
-static int mmc_select_hs_ddr(struct mmc_card *card)
+int mmc_select_hs_ddr(struct mmc_card *card)
 {
 	struct mmc_host *host = card->host;
 	u32 bus_width, ext_csd_bits;
@@ -1158,8 +1160,9 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
 
 	return err;
 }
+EXPORT_SYMBOL_GPL(mmc_select_hs_ddr);
 
-static int mmc_select_hs400(struct mmc_card *card)
+int mmc_select_hs400(struct mmc_card *card)
 {
 	struct mmc_host *host = card->host;
 	unsigned int max_dtr;
@@ -1253,6 +1256,7 @@ static int mmc_select_hs400(struct mmc_card *card)
 	       __func__, err);
 	return err;
 }
+EXPORT_SYMBOL_GPL(mmc_select_hs400);
 
 int mmc_hs200_to_hs400(struct mmc_card *card)
 {
@@ -1533,7 +1537,7 @@ static int mmc_select_hs200(struct mmc_card *card)
 /*
  * Activate High Speed, HS200 or HS400ES mode if supported.
  */
-static int mmc_select_timing(struct mmc_card *card)
+int mmc_select_timing(struct mmc_card *card)
 {
 	int err = 0;
 
@@ -1568,12 +1572,13 @@ static int mmc_select_timing(struct mmc_card *card)
 	mmc_set_bus_speed(card);
 	return 0;
 }
+EXPORT_SYMBOL_GPL(mmc_select_timing);
 
 /*
  * Execute tuning sequence to seek the proper bus operating
  * conditions for HS200 and HS400, which sends CMD21 to the device.
  */
-static int mmc_hs200_tuning(struct mmc_card *card)
+int mmc_hs200_tuning(struct mmc_card *card)
 {
 	struct mmc_host *host = card->host;
 
@@ -1588,6 +1593,7 @@ static int mmc_hs200_tuning(struct mmc_card *card)
 
 	return mmc_execute_tuning(card);
 }
+EXPORT_SYMBOL_GPL(mmc_hs200_tuning);
 
 /*
  * Handle the detection and initialisation of a card.
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 3b3adbddf664..62c16dac9d62 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -118,6 +118,7 @@ int mmc_select_card(struct mmc_card *card)
 
 	return _mmc_select_card(card->host, card);
 }
+EXPORT_SYMBOL_GPL(mmc_select_card);
 
 int mmc_deselect_cards(struct mmc_host *host)
 {
diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
index b396e3900717..2c710d736032 100644
--- a/drivers/mmc/core/queue.c
+++ b/drivers/mmc/core/queue.c
@@ -69,6 +69,7 @@ enum mmc_issue_type mmc_issue_type(struct mmc_queue *mq, struct request *req)
 
 	return MMC_ISSUE_SYNC;
 }
+EXPORT_SYMBOL_GPL(mmc_issue_type);
 
 static void __mmc_cqe_recovery_notifier(struct mmc_queue *mq)
 {
-- 
2.17.1

Re: [PATCH V1 2/2] mmc: core: Export core functions to let vendors use for their features

[Heiner Kallweit]

On 01.04.2023 18:57, Sarthak Garg wrote:
> Export core functions to let vendors use for their internal features.
> 
Typically extensions to the core require at least one user. So you should
add your driver, that makes use of the changes, to the series.
And best explain what's special with your hardware so that it needs a core
extension whilst drivers for other hardware are fine with the core as-is.

> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
> ---
>  drivers/mmc/core/core.c    |  6 ++++++
>  drivers/mmc/core/host.c    |  1 +
>  drivers/mmc/core/mmc.c     | 18 ++++++++++++------
>  drivers/mmc/core/mmc_ops.c |  1 +
>  drivers/mmc/core/queue.c   |  1 +
>  5 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 3d3e0ca52614..ed44b65f19e0 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -916,6 +916,7 @@ void mmc_set_clock(struct mmc_host *host, unsigned int hz)
>  	host->ios.clock = hz;
>  	mmc_set_ios(host);
>  }
> +EXPORT_SYMBOL_GPL(mmc_set_clock);
>  
>  int mmc_execute_tuning(struct mmc_card *card)
>  {
> @@ -950,6 +951,7 @@ int mmc_execute_tuning(struct mmc_card *card)
>  
>  	return err;
>  }
> +EXPORT_SYMBOL_GPL(mmc_execute_tuning);
>  
>  /*
>   * Change the bus mode (open drain/push-pull) of a host.
> @@ -959,6 +961,7 @@ void mmc_set_bus_mode(struct mmc_host *host, unsigned int mode)
>  	host->ios.bus_mode = mode;
>  	mmc_set_ios(host);
>  }
> +EXPORT_SYMBOL_GPL(mmc_set_bus_mode);
>  
>  /*
>   * Change data bus width of a host.
> @@ -968,6 +971,7 @@ void mmc_set_bus_width(struct mmc_host *host, unsigned int width)
>  	host->ios.bus_width = width;
>  	mmc_set_ios(host);
>  }
> +EXPORT_SYMBOL_GPL(mmc_set_bus_width);
>  
>  /*
>   * Set initial state after a power cycle or a hw_reset.
> @@ -1001,6 +1005,7 @@ void mmc_set_initial_state(struct mmc_host *host)
>  
>  	mmc_crypto_set_initial_state(host);
>  }
> +EXPORT_SYMBOL_GPL(mmc_set_initial_state);
>  
>  /**
>   * mmc_vdd_to_ocrbitnum - Convert a voltage to the OCR bit number
> @@ -1270,6 +1275,7 @@ void mmc_set_timing(struct mmc_host *host, unsigned int timing)
>  	host->ios.timing = timing;
>  	mmc_set_ios(host);
>  }
> +EXPORT_SYMBOL_GPL(mmc_set_timing);
>  
>  /*
>   * Select appropriate driver type for host.
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 76900f67c782..1c5eb1d9d585 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -166,6 +166,7 @@ void mmc_retune_hold(struct mmc_host *host)
>  		host->retune_now = 1;
>  	host->hold_retune += 1;
>  }
> +EXPORT_SYMBOL(mmc_retune_hold);
>  
>  void mmc_retune_release(struct mmc_host *host)
>  {
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 32386e4644df..b984a4f90535 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1002,7 +1002,7 @@ static void mmc_set_bus_speed(struct mmc_card *card)
>   * If the bus width is changed successfully, return the selected width value.
>   * Zero is returned instead of error value if the wide width is not supported.
>   */
> -static int mmc_select_bus_width(struct mmc_card *card)
> +int mmc_select_bus_width(struct mmc_card *card)
>  {
>  	static unsigned ext_csd_bits[] = {
>  		EXT_CSD_BUS_WIDTH_8,
> @@ -1067,11 +1067,12 @@ static int mmc_select_bus_width(struct mmc_card *card)
>  
>  	return err;
>  }
> +EXPORT_SYMBOL_GPL(mmc_select_bus_width);
>  
>  /*
>   * Switch to the high-speed mode
>   */
> -static int mmc_select_hs(struct mmc_card *card)
> +int mmc_select_hs(struct mmc_card *card)
>  {
>  	int err;
>  
> @@ -1085,11 +1086,12 @@ static int mmc_select_hs(struct mmc_card *card)
>  
>  	return err;
>  }
> +EXPORT_SYMBOL_GPL(mmc_select_hs);
>  
>  /*
>   * Activate wide bus and DDR if supported.
>   */
> -static int mmc_select_hs_ddr(struct mmc_card *card)
> +int mmc_select_hs_ddr(struct mmc_card *card)
>  {
>  	struct mmc_host *host = card->host;
>  	u32 bus_width, ext_csd_bits;
> @@ -1158,8 +1160,9 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
>  
>  	return err;
>  }
> +EXPORT_SYMBOL_GPL(mmc_select_hs_ddr);
>  
> -static int mmc_select_hs400(struct mmc_card *card)
> +int mmc_select_hs400(struct mmc_card *card)
>  {
>  	struct mmc_host *host = card->host;
>  	unsigned int max_dtr;
> @@ -1253,6 +1256,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>  	       __func__, err);
>  	return err;
>  }
> +EXPORT_SYMBOL_GPL(mmc_select_hs400);
>  
>  int mmc_hs200_to_hs400(struct mmc_card *card)
>  {
> @@ -1533,7 +1537,7 @@ static int mmc_select_hs200(struct mmc_card *card)
>  /*
>   * Activate High Speed, HS200 or HS400ES mode if supported.
>   */
> -static int mmc_select_timing(struct mmc_card *card)
> +int mmc_select_timing(struct mmc_card *card)
>  {
>  	int err = 0;
>  
> @@ -1568,12 +1572,13 @@ static int mmc_select_timing(struct mmc_card *card)
>  	mmc_set_bus_speed(card);
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(mmc_select_timing);
>  
>  /*
>   * Execute tuning sequence to seek the proper bus operating
>   * conditions for HS200 and HS400, which sends CMD21 to the device.
>   */
> -static int mmc_hs200_tuning(struct mmc_card *card)
> +int mmc_hs200_tuning(struct mmc_card *card)
>  {
>  	struct mmc_host *host = card->host;
>  
> @@ -1588,6 +1593,7 @@ static int mmc_hs200_tuning(struct mmc_card *card)
>  
>  	return mmc_execute_tuning(card);
>  }
> +EXPORT_SYMBOL_GPL(mmc_hs200_tuning);
>  
>  /*
>   * Handle the detection and initialisation of a card.
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 3b3adbddf664..62c16dac9d62 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -118,6 +118,7 @@ int mmc_select_card(struct mmc_card *card)
>  
>  	return _mmc_select_card(card->host, card);
>  }
> +EXPORT_SYMBOL_GPL(mmc_select_card);
>  
>  int mmc_deselect_cards(struct mmc_host *host)
>  {
> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
> index b396e3900717..2c710d736032 100644
> --- a/drivers/mmc/core/queue.c
> +++ b/drivers/mmc/core/queue.c
> @@ -69,6 +69,7 @@ enum mmc_issue_type mmc_issue_type(struct mmc_queue *mq, struct request *req)
>  
>  	return MMC_ISSUE_SYNC;
>  }
> +EXPORT_SYMBOL_GPL(mmc_issue_type);
>  
>  static void __mmc_cqe_recovery_notifier(struct mmc_queue *mq)
>  {

Re: [PATCH V1 1/2] mmc: core: Define new vendor ops to enable internal features

[Linus Walleij]

Hi Sarthak,

thanks for your patch!

On Sat, Apr 1, 2023 at 6:57 PM Sarthak Garg <quic_sartgarg@quicinc.com> wrote:

> Define new ops to let vendor enable internal features in
> mmc_suspend/resume paths like partial init feature.
>
> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
(...)

> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -212,6 +212,10 @@ struct mmc_host_ops {
>
>         /* Initialize an SD express card, mandatory for MMC_CAP2_SD_EXP. */
>         int     (*init_sd_express)(struct mmc_host *host, struct mmc_ios *ios);
> +
> +       void    (*cache_card_properties)(struct mmc_host *host);
> +       bool    (*partial_init_card)(struct mmc_host *host);

These have confusing names, first it has nothing to do with
cards since the callbacks are to the host. Second the functions
are named after usecases in a certain host rather than function.

I would just call them .suspend() and .resume(), the use
is obvious and they are called from the driver .suspend()
and .resume() hooks.

Yours,
Linus Walleij

Re: [PATCH V1 1/2] mmc: core: Define new vendor ops to enable internal features

[Christoph Hellwig]

On Sat, Apr 01, 2023 at 10:27:22PM +0530, Sarthak Garg wrote:
> Define new ops to let vendor enable internal features in
> mmc_suspend/resume paths like partial init feature.

1) vendors have absolutely no business doing anything, you might be
   doing either something entirely wrong or use the wrong terminology
   here.

2) any kind of core hook not only needs a very good description, but
   also an actual user that goes along in the same series.

RE: [PATCH V1 1/2] mmc: core: Define new vendor ops to enable internal features

[Sarthak Garg (QUIC)]

Hi linus,

Thanks for your comments.

As mentioned in the cover letter that these ops are needed to implement clock scaling and partial init features for which we already had below discussions but faced strong resistance from community. Since these were huge code changes so maintainability was the main concern. Hence I have redesigned our entire logic and moved complete code to vendor specific file and to support this new design now I just need these two hooks in suspend and resume functions that’s why I have added these callbacks in host_ops.

I can rename these to vendor_suspend/resume ops to let vendor modify few things needed in suspend/resume paths. Also if you want to suggest any better name then I am open for that also.

Old discussion for Clock scaling feature :
https://patchwork.kernel.org/project/linux-mmc/cover/1571668177-3766-1-git-send-email-rampraka@codeaurora.org/

Old discussion for Partial init feature :
https://patchwork.kernel.org/project/linux-mmc/patch/1650963852-4173-1-git-send-email-quic_spathi@quicinc.com/

Thanks,
Sarthak

> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: Sunday, April 2, 2023 6:19 PM
> To: Sarthak Garg (QUIC) <quic_sartgarg@quicinc.com>
> Cc: adrian.hunter@intel.com; ulf.hansson@linaro.org; linux-
> mmc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> msm@vger.kernel.org; Ram Prakash Gupta (QUIC)
> <quic_rampraka@quicinc.com>; Bhaskar Valaboju (QUIC)
> <quic_bhaskarv@quicinc.com>; Sachin Gupta (QUIC)
> <quic_sachgupt@quicinc.com>; Pradeep Pragallapati (QUIC)
> <quic_pragalla@quicinc.com>; Sayali Lokhande (QUIC)
> <quic_sayalil@quicinc.com>; Brian Norris <briannorris@chromium.org>;
> Wolfram Sang <wsa+renesas@sang-engineering.com>
> Subject: Re: [PATCH V1 1/2] mmc: core: Define new vendor ops to enable
> internal features
> 
> Hi Sarthak,
> 
> thanks for your patch!
> 
> On Sat, Apr 1, 2023 at 6:57 PM Sarthak Garg <quic_sartgarg@quicinc.com>
> wrote:
> 
> > Define new ops to let vendor enable internal features in
> > mmc_suspend/resume paths like partial init feature.
> >
> > Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
> (...)
> 
> > --- a/include/linux/mmc/host.h
> > +++ b/include/linux/mmc/host.h
> > @@ -212,6 +212,10 @@ struct mmc_host_ops {
> >
> >         /* Initialize an SD express card, mandatory for MMC_CAP2_SD_EXP. */
> >         int     (*init_sd_express)(struct mmc_host *host, struct mmc_ios *ios);
> > +
> > +       void    (*cache_card_properties)(struct mmc_host *host);
> > +       bool    (*partial_init_card)(struct mmc_host *host);
> 
> These have confusing names, first it has nothing to do with cards since the
> callbacks are to the host. Second the functions are named after usecases in a
> certain host rather than function.
> 
> I would just call them .suspend() and .resume(), the use is obvious and they are
> called from the driver .suspend() and .resume() hooks.
> 
> Yours,
> Linus Walleij

RE: [PATCH V1 1/2] mmc: core: Define new vendor ops to enable internal features

[Sarthak Garg (QUIC)]

Hi christoph,

Thanks for your comments.

As mentioned in the cover letter that these ops are needed to implement clock scaling and partial init features for which we already had below discussions but faced strong resistance from community. Since these were huge code changes so maintainability was the main concern. Hence we have redesigned our entire logic and moved complete code to vendor specific file and to support this new design now we just need these two hooks in suspend and resume functions along with few symbols to be exported so that we can use those symbols in our vendor files. I will push the vendor specific changes in the next patchset.


Old discussion for Clock scaling feature :
https://patchwork.kernel.org/project/linux-mmc/cover/1571668177-3766-1-git-send-email-rampraka@codeaurora.org/

Old discussion for Partial init feature :
https://patchwork.kernel.org/project/linux-mmc/patch/1650963852-4173-1-git-send-email-quic_spathi@quicinc.com/

Thanks,
Sarthak

> -----Original Message-----
> From: Christoph Hellwig <hch@infradead.org>
> Sent: Tuesday, April 4, 2023 10:44 AM
> To: Sarthak Garg (QUIC) <quic_sartgarg@quicinc.com>
> Cc: adrian.hunter@intel.com; ulf.hansson@linaro.org; linux-
> mmc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> msm@vger.kernel.org; Ram Prakash Gupta (QUIC)
> <quic_rampraka@quicinc.com>; Bhaskar Valaboju (QUIC)
> <quic_bhaskarv@quicinc.com>; Sachin Gupta (QUIC)
> <quic_sachgupt@quicinc.com>; Pradeep Pragallapati (QUIC)
> <quic_pragalla@quicinc.com>; Sayali Lokhande (QUIC)
> <quic_sayalil@quicinc.com>; Brian Norris <briannorris@chromium.org>;
> Wolfram Sang <wsa+renesas@sang-engineering.com>; Linus Walleij
> <linus.walleij@linaro.org>
> Subject: Re: [PATCH V1 1/2] mmc: core: Define new vendor ops to enable
> internal features
> 
> On Sat, Apr 01, 2023 at 10:27:22PM +0530, Sarthak Garg wrote:
> > Define new ops to let vendor enable internal features in
> > mmc_suspend/resume paths like partial init feature.
> 
> 1) vendors have absolutely no business doing anything, you might be
>    doing either something entirely wrong or use the wrong terminology
>    here.
> 
> 2) any kind of core hook not only needs a very good description, but
>    also an actual user that goes along in the same series.

Re: [PATCH V1 1/2] mmc: core: Define new vendor ops to enable internal features

[Christoph Hellwig]

You don't get it.  There is no such thing "as vendor files".

I'm not sure where you get your terminology from, but whatever that is
might be your source of the fundamental misunderstanding how Linux
kernel development works.  I would recommend to take some training
before wasting everyones time.

RE: [PATCH V1 1/2] mmc: core: Define new vendor ops to enable internal features

[Sarthak Garg (QUIC)]

Sorry for the confusion by vendor file I meant driver file for Qualcomm SDCC controller (sdhci-msm.c).
Further to make things more clear I will push the complete series.

> -----Original Message-----
> From: Christoph Hellwig <hch@infradead.org>
> Sent: Friday, April 14, 2023 11:06 AM
> To: Sarthak Garg (QUIC) <quic_sartgarg@quicinc.com>
> Cc: Christoph Hellwig <hch@infradead.org>; adrian.hunter@intel.com;
> ulf.hansson@linaro.org; linux-mmc@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-arm-msm@vger.kernel.org; Ram Prakash Gupta
> (QUIC) <quic_rampraka@quicinc.com>; Bhaskar Valaboju (QUIC)
> <quic_bhaskarv@quicinc.com>; Sachin Gupta (QUIC)
> <quic_sachgupt@quicinc.com>; Pradeep Pragallapati (QUIC)
> <quic_pragalla@quicinc.com>; Sayali Lokhande (QUIC)
> <quic_sayalil@quicinc.com>; Brian Norris <briannorris@chromium.org>;
> Wolfram Sang <wsa+renesas@sang-engineering.com>; Linus Walleij
> <linus.walleij@linaro.org>
> Subject: Re: [PATCH V1 1/2] mmc: core: Define new vendor ops to enable
> internal features
> 
> You don't get it.  There is no such thing "as vendor files".
> 
> I'm not sure where you get your terminology from, but whatever that is might
> be your source of the fundamental misunderstanding how Linux kernel
> development works.  I would recommend to take some training before wasting
> everyones time.

Re: [PATCH V1 1/2] mmc: core: Define new vendor ops to enable internal features

[Christoph Hellwig]

On Fri, Apr 14, 2023 at 06:52:18AM +0000, Sarthak Garg (QUIC) wrote:
> Sorry for the confusion by vendor file I meant driver file for Qualcomm SDCC controller (sdhci-msm.c).

This is still not how we do development.  The two series you've been
pointed out got valuable feedback that;s been ignored for between one
and four years, that needs to be followed up with.

You're not going to get magic hooks for your driver that you're not
sharing with us just because you're too lazy to follow up on the review
comments.

RE: [PATCH V1 1/2] mmc: core: Define new vendor ops to enable internal features

[Sarthak Garg (QUIC)]

Thanks for your valuable comments. We didn't ignore the previous comments instead we tried to address most of the comments by trying the suggested alternatives as well but didn't see power improvement as compared to this feature. Moreover we got the intuition that maintainability was the main concern hence we came up with this newer approach of hooks to limit the lines of code in core layer. Every change was pushed earlier in the previous posts and this time we just refactored the code and was about to push the series but as per current discussion we'll be reviving the old discussion and try to close all the comments. Closing this thread now.

> -----Original Message-----
> From: Christoph Hellwig <hch@infradead.org>
> Sent: Friday, April 14, 2023 6:46 PM
> To: Sarthak Garg (QUIC) <quic_sartgarg@quicinc.com>
> Cc: Christoph Hellwig <hch@infradead.org>; adrian.hunter@intel.com;
> ulf.hansson@linaro.org; linux-mmc@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-arm-msm@vger.kernel.org; Ram Prakash Gupta
> (QUIC) <quic_rampraka@quicinc.com>; Bhaskar Valaboju (QUIC)
> <quic_bhaskarv@quicinc.com>; Sachin Gupta (QUIC)
> <quic_sachgupt@quicinc.com>; Pradeep Pragallapati (QUIC)
> <quic_pragalla@quicinc.com>; Sayali Lokhande (QUIC)
> <quic_sayalil@quicinc.com>; Brian Norris <briannorris@chromium.org>;
> Wolfram Sang <wsa+renesas@sang-engineering.com>; Linus Walleij
> <linus.walleij@linaro.org>
> Subject: Re: [PATCH V1 1/2] mmc: core: Define new vendor ops to enable
> internal features
> 
> On Fri, Apr 14, 2023 at 06:52:18AM +0000, Sarthak Garg (QUIC) wrote:
> > Sorry for the confusion by vendor file I meant driver file for Qualcomm SDCC
> controller (sdhci-msm.c).
> 
> This is still not how we do development.  The two series you've been pointed out
> got valuable feedback that;s been ignored for between one and four years, that
> needs to be followed up with.
> 
> You're not going to get magic hooks for your driver that you're not sharing with
> us just because you're too lazy to follow up on the review comments.