| Message ID | 20230401165723.19762-2-quic_sartgarg@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce new vendor op and export few symbols | expand |
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
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.
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
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.
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.
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.
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.
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.
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 {
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(-)