| Message ID | 20241104060722.10642-1-quic_sartgarg@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V1] mmc: sdhci-msm: Enable MMC_CAP_AGGRESSIVE_PM for qualcomm controllers | expand |
On 11/4/2024 4:19 PM, Dmitry Baryshkov wrote: > On Mon, Nov 04, 2024 at 11:37:22AM +0530, Sarthak Garg wrote: >> Enable MMC_CAP_AGGRESSIVE_PM for qualcomm controllers. >> This enables runtime PM for eMMC/SD card. > > Could you please mention, which platforms were tested with this patch? > Note, upstream kernel supports a lot of platforms, including MSM8974, I > think the oldest one, which uses SDHCI. > This was tested with qdu1000 platform. >> >> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com> >> --- >> drivers/mmc/host/sdhci-msm.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c >> index e00208535bd1..6657f7db1b8e 100644 >> --- a/drivers/mmc/host/sdhci-msm.c >> +++ b/drivers/mmc/host/sdhci-msm.c >> @@ -2626,6 +2626,7 @@ static int sdhci_msm_probe(struct platform_device *pdev) >> goto clk_disable; >> } >> >> + msm_host->mmc->caps |= MMC_CAP_AGGRESSIVE_PM; >> msm_host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY | MMC_CAP_NEED_RSP_BUSY; >> >> /* Set the timeout value to max possible */ >> -- >> 2.17.1 >> >
On Mon, 4 Nov 2024 at 07:07, Sarthak Garg <quic_sartgarg@quicinc.com> wrote: > > Enable MMC_CAP_AGGRESSIVE_PM for qualcomm controllers. > This enables runtime PM for eMMC/SD card. > > Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com> In general I think using MMC_CAP_AGGRESSIVE_PM needs to be carefully selected. I am not saying it's a bad idea to use it, but the commit message above kind of indicates that this has only been enabled to make sure we avoid wasting energy at any cost. Maybe I am wrong? Today the default autosuspend timeout is set to 3000 ms, which means that beyond this idle-period the card internally will no longer be able to manage "garbage collect". For a poorly behaving SD card, for example, that could hurt future read/writes. Or maybe that isn't such a big problem after all? Also note that userspace via sysfs is able to change the autosuspend timeout and even disable runtime PM for the card, if that is needed. Kind regards Uffe > --- > drivers/mmc/host/sdhci-msm.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c > index e00208535bd1..6657f7db1b8e 100644 > --- a/drivers/mmc/host/sdhci-msm.c > +++ b/drivers/mmc/host/sdhci-msm.c > @@ -2626,6 +2626,7 @@ static int sdhci_msm_probe(struct platform_device *pdev) > goto clk_disable; > } > > + msm_host->mmc->caps |= MMC_CAP_AGGRESSIVE_PM; > msm_host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY | MMC_CAP_NEED_RSP_BUSY; > > /* Set the timeout value to max possible */ > -- > 2.17.1 >
On Fri, 15 Nov 2024 at 12:23, Sarthak Garg <quic_sartgarg@quicinc.com> wrote: > > > > On 11/4/2024 4:19 PM, Dmitry Baryshkov wrote: > > On Mon, Nov 04, 2024 at 11:37:22AM +0530, Sarthak Garg wrote: > >> Enable MMC_CAP_AGGRESSIVE_PM for qualcomm controllers. > >> This enables runtime PM for eMMC/SD card. > > > > Could you please mention, which platforms were tested with this patch? > > Note, upstream kernel supports a lot of platforms, including MSM8974, I > > think the oldest one, which uses SDHCI. > > > > This was tested with qdu1000 platform. Are you sure that it won't break other platforms? > > >> > >> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com> > >> --- > >> drivers/mmc/host/sdhci-msm.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c > >> index e00208535bd1..6657f7db1b8e 100644 > >> --- a/drivers/mmc/host/sdhci-msm.c > >> +++ b/drivers/mmc/host/sdhci-msm.c > >> @@ -2626,6 +2626,7 @@ static int sdhci_msm_probe(struct platform_device *pdev) > >> goto clk_disable; > >> } > >> > >> + msm_host->mmc->caps |= MMC_CAP_AGGRESSIVE_PM; > >> msm_host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY | MMC_CAP_NEED_RSP_BUSY; > >> > >> /* Set the timeout value to max possible */ > >> -- > >> 2.17.1 > >> > >
On 11/15/2024 4:28 PM, Ulf Hansson wrote: > On Mon, 4 Nov 2024 at 07:07, Sarthak Garg <quic_sartgarg@quicinc.com> wrote: >> >> Enable MMC_CAP_AGGRESSIVE_PM for qualcomm controllers. >> This enables runtime PM for eMMC/SD card. >> >> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com> > > In general I think using MMC_CAP_AGGRESSIVE_PM needs to be carefully > selected. I am not saying it's a bad idea to use it, but the commit > message above kind of indicates that this has only been enabled to > make sure we avoid wasting energy at any cost. Maybe I am wrong? > > Today the default autosuspend timeout is set to 3000 ms, which means > that beyond this idle-period the card internally will no longer be > able to manage "garbage collect". For a poorly behaving SD card, for > example, that could hurt future read/writes. Or maybe that isn't such > a big problem after all? > > Also note that userspace via sysfs is able to change the autosuspend > timeout and even disable runtime PM for the card, if that is needed. > > Kind regards > Uffe > Thanks for your valuable comment. First of all sorry Ulf for the late reply as I was on break and came back now. Yes you are right this has been enabled to make sure we avoid wasting power at any cost. Moreover for a poorly behaving SD card we can't penalize the power consumption of all the SOC's so thats we won't to enable this flag. >> --- >> drivers/mmc/host/sdhci-msm.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c >> index e00208535bd1..6657f7db1b8e 100644 >> --- a/drivers/mmc/host/sdhci-msm.c >> +++ b/drivers/mmc/host/sdhci-msm.c >> @@ -2626,6 +2626,7 @@ static int sdhci_msm_probe(struct platform_device *pdev) >> goto clk_disable; >> } >> >> + msm_host->mmc->caps |= MMC_CAP_AGGRESSIVE_PM; >> msm_host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY | MMC_CAP_NEED_RSP_BUSY; >> >> /* Set the timeout value to max possible */ >> -- >> 2.17.1 >>
On Wed, May 21, 2025 at 12:46:49PM +0530, Sarthak Garg wrote: > > > On 11/15/2024 6:53 PM, Dmitry Baryshkov wrote: > > On Fri, 15 Nov 2024 at 12:23, Sarthak Garg <quic_sartgarg@quicinc.com> wrote: > > > > > > > > > > > > On 11/4/2024 4:19 PM, Dmitry Baryshkov wrote: > > > > On Mon, Nov 04, 2024 at 11:37:22AM +0530, Sarthak Garg wrote: > > > > > Enable MMC_CAP_AGGRESSIVE_PM for qualcomm controllers. > > > > > This enables runtime PM for eMMC/SD card. > > > > > > > > Could you please mention, which platforms were tested with this patch? > > > > Note, upstream kernel supports a lot of platforms, including MSM8974, I > > > > think the oldest one, which uses SDHCI. > > > > > > > > > > This was tested with qdu1000 platform. > > > > Are you sure that it won't break other platforms? > > > > Thanks for your valuable comment. > I am not sure about the older platforms so to avoid issues on older > platforms we can enable this for all SDCC version 5.0 targets ? No, there are still a lot of platforms. Either explain why this is required for all v5 platforms (and won't break those) or find some other way, e.g. limit the change to QDU1000, explaining why it is _not_ applicable to other platforms.
On 5/21/2025 6:25 PM, Dmitry Baryshkov wrote: > On Wed, May 21, 2025 at 12:46:49PM +0530, Sarthak Garg wrote: >> >> >> On 11/15/2024 6:53 PM, Dmitry Baryshkov wrote: >>> On Fri, 15 Nov 2024 at 12:23, Sarthak Garg <quic_sartgarg@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 11/4/2024 4:19 PM, Dmitry Baryshkov wrote: >>>>> On Mon, Nov 04, 2024 at 11:37:22AM +0530, Sarthak Garg wrote: >>>>>> Enable MMC_CAP_AGGRESSIVE_PM for qualcomm controllers. >>>>>> This enables runtime PM for eMMC/SD card. >>>>> >>>>> Could you please mention, which platforms were tested with this patch? >>>>> Note, upstream kernel supports a lot of platforms, including MSM8974, I >>>>> think the oldest one, which uses SDHCI. >>>>> >>>> >>>> This was tested with qdu1000 platform. >>> >>> Are you sure that it won't break other platforms? >>> >> >> Thanks for your valuable comment. >> I am not sure about the older platforms so to avoid issues on older >> platforms we can enable this for all SDCC version 5.0 targets ? > > No, there are still a lot of platforms. Either explain why this is > required for all v5 platforms (and won't break those) or find some other > way, e.g. limit the change to QDU1000, explaining why it is _not_ > applicable to other platforms. > Thanks for your comment. I agree with your concern but for me also its not possible to test on all the platforms. Lets say if I want to enable this caps for QDU1000 for which it has been tested and on any other upcoming target after testing, then how can I proceed to enable? One option I had thought of was to implement this using compatible string, then for all the upcoming platforms using this compatible string as a fallback. But this doesn't look optimal to use compatible string for just one flag and also this capability is not platform specific and we will be needing for all the platforms. Please share your opinion on this. Another option that I could have thought of is using device tree based approach but seems that was not accepted earlier : https://patchwork.kernel.org/project/linux-mmc/patch/20230129023630.830764-1-chenhuiz@axis.com/ So it would be helpful if you can suggest some approach?
On 21/05/2025 17:35, Sarthak Garg wrote: > > > On 5/21/2025 6:25 PM, Dmitry Baryshkov wrote: >> On Wed, May 21, 2025 at 12:46:49PM +0530, Sarthak Garg wrote: >>> >>> >>> On 11/15/2024 6:53 PM, Dmitry Baryshkov wrote: >>>> On Fri, 15 Nov 2024 at 12:23, Sarthak Garg >>>> <quic_sartgarg@quicinc.com> wrote: >>>>> >>>>> >>>>> >>>>> On 11/4/2024 4:19 PM, Dmitry Baryshkov wrote: >>>>>> On Mon, Nov 04, 2024 at 11:37:22AM +0530, Sarthak Garg wrote: >>>>>>> Enable MMC_CAP_AGGRESSIVE_PM for qualcomm controllers. >>>>>>> This enables runtime PM for eMMC/SD card. >>>>>> >>>>>> Could you please mention, which platforms were tested with this >>>>>> patch? >>>>>> Note, upstream kernel supports a lot of platforms, including >>>>>> MSM8974, I >>>>>> think the oldest one, which uses SDHCI. >>>>>> >>>>> >>>>> This was tested with qdu1000 platform. >>>> >>>> Are you sure that it won't break other platforms? >>>> >>> >>> Thanks for your valuable comment. >>> I am not sure about the older platforms so to avoid issues on older >>> platforms we can enable this for all SDCC version 5.0 targets ? >> >> No, there are still a lot of platforms. Either explain why this is >> required for all v5 platforms (and won't break those) or find some other >> way, e.g. limit the change to QDU1000, explaining why it is _not_ >> applicable to other platforms. >> > > Thanks for your comment. No need to. > I agree with your concern but for me also its not possible to test on > all the platforms. Sure. > Lets say if I want to enable this caps for QDU1000 for which it has been > tested and on any other upcoming target after testing, then how can I > proceed to enable? Let's start from the beginning: why do you want to enable it on QDU1000? > > One option I had thought of was to implement this using compatible > string, then for all the upcoming platforms using this compatible string > as a fallback. > But this doesn't look optimal to use compatible string for just one flag > and also this capability is not platform specific and we will be needing > for all the platforms. Please share your opinion on this. > > Another option that I could have thought of is using device tree based > approach but seems that was not accepted earlier : > https://patchwork.kernel.org/project/linux-mmc/ > patch/20230129023630.830764-1-chenhuiz@axis.com/ > > So it would be helpful if you can suggest some approach? Worst case, just tie it to the SoC-specific compat string that is already a part of the bindings.
On 5/21/2025 8:19 PM, Dmitry Baryshkov wrote: > On 21/05/2025 17:35, Sarthak Garg wrote: >> >> >> On 5/21/2025 6:25 PM, Dmitry Baryshkov wrote: >>> On Wed, May 21, 2025 at 12:46:49PM +0530, Sarthak Garg wrote: >>>> >>>> >>>> On 11/15/2024 6:53 PM, Dmitry Baryshkov wrote: >>>>> On Fri, 15 Nov 2024 at 12:23, Sarthak Garg >>>>> <quic_sartgarg@quicinc.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 11/4/2024 4:19 PM, Dmitry Baryshkov wrote: >>>>>>> On Mon, Nov 04, 2024 at 11:37:22AM +0530, Sarthak Garg wrote: >>>>>>>> Enable MMC_CAP_AGGRESSIVE_PM for qualcomm controllers. >>>>>>>> This enables runtime PM for eMMC/SD card. >>>>>>> >>>>>>> Could you please mention, which platforms were tested with this >>>>>>> patch? >>>>>>> Note, upstream kernel supports a lot of platforms, including >>>>>>> MSM8974, I >>>>>>> think the oldest one, which uses SDHCI. >>>>>>> >>>>>> >>>>>> This was tested with qdu1000 platform. >>>>> >>>>> Are you sure that it won't break other platforms? >>>>> >>>> >>>> Thanks for your valuable comment. >>>> I am not sure about the older platforms so to avoid issues on older >>>> platforms we can enable this for all SDCC version 5.0 targets ? >>> >>> No, there are still a lot of platforms. Either explain why this is >>> required for all v5 platforms (and won't break those) or find some other >>> way, e.g. limit the change to QDU1000, explaining why it is _not_ >>> applicable to other platforms. >>> >> >> Thanks for your comment. > > No need to. > >> I agree with your concern but for me also its not possible to test on >> all the platforms. > > Sure. > >> Lets say if I want to enable this caps for QDU1000 for which it has >> been tested and on any other upcoming target after testing, then how >> can I proceed to enable? > > Let's start from the beginning: why do you want to enable it on QDU1000? > QDU1000 is one latest available target where we have enabled this and tested. This has been enabled to save power. >> >> One option I had thought of was to implement this using compatible >> string, then for all the upcoming platforms using this compatible >> string as a fallback. >> But this doesn't look optimal to use compatible string for just one >> flag and also this capability is not platform specific and we will be >> needing for all the platforms. Please share your opinion on this. >> >> Another option that I could have thought of is using device tree based >> approach but seems that was not accepted earlier : >> https://patchwork.kernel.org/project/linux-mmc/ >> patch/20230129023630.830764-1-chenhuiz@axis.com/ >> >> So it would be helpful if you can suggest some approach? > > Worst case, just tie it to the SoC-specific compat string that is > already a part of the bindings. > > Sure will try to explore better solution before going for the worst case.
On 21/05/2025 18:36, Sarthak Garg wrote: > > > On 5/21/2025 8:19 PM, Dmitry Baryshkov wrote: >> On 21/05/2025 17:35, Sarthak Garg wrote: >>> >>> >>> On 5/21/2025 6:25 PM, Dmitry Baryshkov wrote: >>>> On Wed, May 21, 2025 at 12:46:49PM +0530, Sarthak Garg wrote: >>>>> >>>>> >>>>> On 11/15/2024 6:53 PM, Dmitry Baryshkov wrote: >>>>>> On Fri, 15 Nov 2024 at 12:23, Sarthak Garg >>>>>> <quic_sartgarg@quicinc.com> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 11/4/2024 4:19 PM, Dmitry Baryshkov wrote: >>>>>>>> On Mon, Nov 04, 2024 at 11:37:22AM +0530, Sarthak Garg wrote: >>>>>>>>> Enable MMC_CAP_AGGRESSIVE_PM for qualcomm controllers. >>>>>>>>> This enables runtime PM for eMMC/SD card. >>>>>>>> >>>>>>>> Could you please mention, which platforms were tested with this >>>>>>>> patch? >>>>>>>> Note, upstream kernel supports a lot of platforms, including >>>>>>>> MSM8974, I >>>>>>>> think the oldest one, which uses SDHCI. >>>>>>>> >>>>>>> >>>>>>> This was tested with qdu1000 platform. >>>>>> >>>>>> Are you sure that it won't break other platforms? >>>>>> >>>>> >>>>> Thanks for your valuable comment. >>>>> I am not sure about the older platforms so to avoid issues on older >>>>> platforms we can enable this for all SDCC version 5.0 targets ? >>>> >>>> No, there are still a lot of platforms. Either explain why this is >>>> required for all v5 platforms (and won't break those) or find some >>>> other >>>> way, e.g. limit the change to QDU1000, explaining why it is _not_ >>>> applicable to other platforms. >>>> >>> >>> Thanks for your comment. >> >> No need to. >> >> I agree with your concern but for me also its not possible to test on >>> all the platforms. >> >> Sure. >> >> Lets say if I want to enable this caps for QDU1000 for which it has >>> been tested and on any other upcoming target after testing, then how >>> can I proceed to enable? >> >> Let's start from the beginning: why do you want to enable it on QDU1000? >> > > QDU1000 is one latest available target where we have enabled this and > tested. This has been enabled to save power. Isn't it a powered device? How much power is the save? Is it worth it?
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c index e00208535bd1..6657f7db1b8e 100644 --- a/drivers/mmc/host/sdhci-msm.c +++ b/drivers/mmc/host/sdhci-msm.c @@ -2626,6 +2626,7 @@ static int sdhci_msm_probe(struct platform_device *pdev) goto clk_disable; } + msm_host->mmc->caps |= MMC_CAP_AGGRESSIVE_PM; msm_host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY | MMC_CAP_NEED_RSP_BUSY; /* Set the timeout value to max possible */
Enable MMC_CAP_AGGRESSIVE_PM for qualcomm controllers. This enables runtime PM for eMMC/SD card. Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com> --- drivers/mmc/host/sdhci-msm.c | 1 + 1 file changed, 1 insertion(+)