[PATCH 0/2] mmc: Fix max current limit handling for SD cards

[PATCH 0/2] mmc: Fix max current limit handling for SD cards

[Avri Altman]

This series addresses a long-standing issue in the mmc driver regarding
the setting of the maximum current limit for SD cards.

The current implementation, which dates back nearly a decade,
misunderstood the intention of the SD specification. To be fair, the
relevant section of the spec is notoriously confusing and difficult to
interpret. As a result, the old code almost always limited cards to the
default low power setting of 0.72W, regardless of the card’s actual
capabilities.

With this change, we will now correctly set the maximum current limit,
allowing SD cards to receive enough power to operate at their true
potential and unleash their maximum performance.

Avri Altman (2):
  mmc: core sd: Simplify current limit logic for 200mA default
  mmc: core: sd: Fix and simplify SD card current limit handling

 drivers/mmc/core/sd.c    | 43 ++++++++++++++--------------------------
 include/linux/mmc/card.h |  7 -------
 2 files changed, 15 insertions(+), 35 deletions(-)

-- 
2.25.1

[PATCH 1/2] mmc: core sd: Simplify current limit logic for 200mA default

[Avri Altman]

The SD current limit logic is updated to avoid explicitly setting the
current limit when the maximum power is 200mA (0.72W) or less, as this
is already the default value. The code now only issues a current limit
switch if a higher limit is required, and the unused
SD_SET_CURRENT_NO_CHANGE constant is removed. This reduces unnecessary
commands and simplifies the logic.

Fixes: 0aa6770000ba ("mmc: sdhci: only set 200mA support for 1.8v if 200mA is available")
Signed-off-by: Avri Altman <avri.altman@sandisk.com>
Cc: stable@vger.kernel.org
---
 drivers/mmc/core/sd.c    | 7 ++-----
 include/linux/mmc/card.h | 1 -
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index ec02067f03c5..cf92c5b2059a 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -554,7 +554,7 @@ static u32 sd_get_host_max_current(struct mmc_host *host)
 
 static int sd_set_current_limit(struct mmc_card *card, u8 *status)
 {
-	int current_limit = SD_SET_CURRENT_NO_CHANGE;
+	int current_limit = SD_SET_CURRENT_LIMIT_200;
 	int err;
 	u32 max_current;
 
@@ -598,11 +598,8 @@ static int sd_set_current_limit(struct mmc_card *card, u8 *status)
 	else if (max_current >= 400 &&
 		 card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_400)
 		current_limit = SD_SET_CURRENT_LIMIT_400;
-	else if (max_current >= 200 &&
-		 card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_200)
-		current_limit = SD_SET_CURRENT_LIMIT_200;
 
-	if (current_limit != SD_SET_CURRENT_NO_CHANGE) {
+	if (current_limit != SD_SET_CURRENT_LIMIT_200) {
 		err = mmc_sd_switch(card, SD_SWITCH_SET, 3,
 				current_limit, status);
 		if (err)
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index ddcdf23d731c..e9e964c20e53 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -182,7 +182,6 @@ struct sd_switch_caps {
 #define SD_SET_CURRENT_LIMIT_400	1
 #define SD_SET_CURRENT_LIMIT_600	2
 #define SD_SET_CURRENT_LIMIT_800	3
-#define SD_SET_CURRENT_NO_CHANGE	(-1)
 
 #define SD_MAX_CURRENT_200	(1 << SD_SET_CURRENT_LIMIT_200)
 #define SD_MAX_CURRENT_400	(1 << SD_SET_CURRENT_LIMIT_400)
-- 
2.25.1

[PATCH 2/2] mmc: core: sd: Fix and simplify SD card current limit handling

[Avri Altman]

The SD spec says: "In UHS-I mode, after selecting one of SDR50, SDR104,
or DDR50 mode by Function Group 1, host needs to change the Power Limit
to enable the card to operate in higher performance".

The driver previously determined SD card current limits incorrectly by
checking capability bits before bus speed was established, and by using
support bits in function group 4 (bytes 6 & 7) rather than the actual
current requirement (bytes 0 & 1). This is wrong because the card
responds for a given bus speed.

This patch queries the card's current requirement after setting the bus
speed, and uses the reported value to select the appropriate current
limit.

while at it, remove some unused constants and the misleading comment in
the code.

Fixes: d9812780a020 ("mmc: sd: limit SD card power limit according to cards capabilities")
Signed-off-by: Avri Altman <avri.altman@sandisk.com>
Cc: stable@vger.kernel.org
---
 drivers/mmc/core/sd.c    | 36 +++++++++++++-----------------------
 include/linux/mmc/card.h |  6 ------
 2 files changed, 13 insertions(+), 29 deletions(-)

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index cf92c5b2059a..357edfb910df 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -365,7 +365,6 @@ static int mmc_read_switch(struct mmc_card *card)
 		card->sw_caps.sd3_bus_mode = status[13];
 		/* Driver Strengths supported by the card */
 		card->sw_caps.sd3_drv_type = status[9];
-		card->sw_caps.sd3_curr_limit = status[7] | status[6] << 8;
 	}
 
 out:
@@ -556,7 +555,7 @@ static int sd_set_current_limit(struct mmc_card *card, u8 *status)
 {
 	int current_limit = SD_SET_CURRENT_LIMIT_200;
 	int err;
-	u32 max_current;
+	u32 max_current, card_needs;
 
 	/*
 	 * Current limit switch is only defined for SDR50, SDR104, and DDR50
@@ -575,33 +574,24 @@ static int sd_set_current_limit(struct mmc_card *card, u8 *status)
 	max_current = sd_get_host_max_current(card->host);
 
 	/*
-	 * We only check host's capability here, if we set a limit that is
-	 * higher than the card's maximum current, the card will be using its
-	 * maximum current, e.g. if the card's maximum current is 300ma, and
-	 * when we set current limit to 200ma, the card will draw 200ma, and
-	 * when we set current limit to 400/600/800ma, the card will draw its
-	 * maximum 300ma from the host.
-	 *
-	 * The above is incorrect: if we try to set a current limit that is
-	 * not supported by the card, the card can rightfully error out the
-	 * attempt, and remain at the default current limit.  This results
-	 * in a 300mA card being limited to 200mA even though the host
-	 * supports 800mA. Failures seen with SanDisk 8GB UHS cards with
-	 * an iMX6 host. --rmk
+	 * query the card of its maximun current/power consumption given the
+	 * bus speed mode
 	 */
-	if (max_current >= 800 &&
-	    card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_800)
+	err = mmc_sd_switch(card, 0, 0, card->sd_bus_speed, status);
+	if (err)
+		return err;
+
+	card_needs = status[1] | status[0] << 8;
+
+	if (max_current >= 800 && card_needs > 600)
 		current_limit = SD_SET_CURRENT_LIMIT_800;
-	else if (max_current >= 600 &&
-		 card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_600)
+	else if (max_current >= 600 && card_needs > 400)
 		current_limit = SD_SET_CURRENT_LIMIT_600;
-	else if (max_current >= 400 &&
-		 card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_400)
+	else if (max_current >= 400 && card_needs > 200)
 		current_limit = SD_SET_CURRENT_LIMIT_400;
 
 	if (current_limit != SD_SET_CURRENT_LIMIT_200) {
-		err = mmc_sd_switch(card, SD_SWITCH_SET, 3,
-				current_limit, status);
+		err = mmc_sd_switch(card, SD_SWITCH_SET, 3, current_limit, status);
 		if (err)
 			return err;
 
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index e9e964c20e53..67c1386ca574 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -177,17 +177,11 @@ struct sd_switch_caps {
 #define SD_DRIVER_TYPE_A	0x02
 #define SD_DRIVER_TYPE_C	0x04
 #define SD_DRIVER_TYPE_D	0x08
-	unsigned int		sd3_curr_limit;
 #define SD_SET_CURRENT_LIMIT_200	0
 #define SD_SET_CURRENT_LIMIT_400	1
 #define SD_SET_CURRENT_LIMIT_600	2
 #define SD_SET_CURRENT_LIMIT_800	3
 
-#define SD_MAX_CURRENT_200	(1 << SD_SET_CURRENT_LIMIT_200)
-#define SD_MAX_CURRENT_400	(1 << SD_SET_CURRENT_LIMIT_400)
-#define SD_MAX_CURRENT_600	(1 << SD_SET_CURRENT_LIMIT_600)
-#define SD_MAX_CURRENT_800	(1 << SD_SET_CURRENT_LIMIT_800)
-
 #define SD4_SET_POWER_LIMIT_0_72W	0
 #define SD4_SET_POWER_LIMIT_1_44W	1
 #define SD4_SET_POWER_LIMIT_2_16W	2
-- 
2.25.1

RE: [PATCH 0/2] mmc: Fix max current limit handling for SD cards

[Avri Altman]

 
> This series addresses a long-standing issue in the mmc driver regarding
> the setting of the maximum current limit for SD cards.
> 
> The current implementation, which dates back nearly a decade,
> misunderstood the intention of the SD specification. To be fair, the
> relevant section of the spec is notoriously confusing and difficult to
> interpret. As a result, the old code almost always limited cards to the
> default low power setting of 0.72W, regardless of the card’s actual
> capabilities.
> 
> With this change, we will now correctly set the maximum current limit,
> allowing SD cards to receive enough power to operate at their true
> potential and unleash their maximum performance.
A gentle ping.
Would be interested to get some comments and testing.
Internally, I saw a +65% improvement, should the platform allow higher power than the 0.72w default.

Thanks,
Avri

> 
> Avri Altman (2):
>   mmc: core sd: Simplify current limit logic for 200mA default
>   mmc: core: sd: Fix and simplify SD card current limit handling
> 
>  drivers/mmc/core/sd.c    | 43 ++++++++++++++--------------------------
>  include/linux/mmc/card.h |  7 -------
>  2 files changed, 15 insertions(+), 35 deletions(-)
> 
> --
> 2.25.1

Re: [PATCH 0/2] mmc: Fix max current limit handling for SD cards

[Ulf Hansson]

On Fri, 4 Jul 2025 at 10:01, Avri Altman <Avri.Altman@sandisk.com> wrote:
>
>
> > This series addresses a long-standing issue in the mmc driver regarding
> > the setting of the maximum current limit for SD cards.
> >
> > The current implementation, which dates back nearly a decade,
> > misunderstood the intention of the SD specification. To be fair, the
> > relevant section of the spec is notoriously confusing and difficult to
> > interpret. As a result, the old code almost always limited cards to the
> > default low power setting of 0.72W, regardless of the card’s actual
> > capabilities.
> >
> > With this change, we will now correctly set the maximum current limit,
> > allowing SD cards to receive enough power to operate at their true
> > potential and unleash their maximum performance.
> A gentle ping.
> Would be interested to get some comments and testing.
> Internally, I saw a +65% improvement, should the platform allow higher power than the 0.72w default.

Sorry for the delay - and unfortunately my review needs to wait a few
more days, as I need to look at Oleksij's series [1] first.

Anyway, what does 65% percent mean? It sure sounds impressive! Perhaps
you can elaborate a bit on the tests you are running and possibly also
whether this has an impact from a consumed energy point of view?

>
> Thanks,
> Avri
>
> >
> > Avri Altman (2):
> >   mmc: core sd: Simplify current limit logic for 200mA default
> >   mmc: core: sd: Fix and simplify SD card current limit handling
> >
> >  drivers/mmc/core/sd.c    | 43 ++++++++++++++--------------------------
> >  include/linux/mmc/card.h |  7 -------
> >  2 files changed, 15 insertions(+), 35 deletions(-)
> >
> > --
> > 2.25.1
>

Kind regards
Uffe

[1]
https://lore.kernel.org/all/20250616102917.1677116-1-o.rempel@pengutronix.de/

RE: [PATCH 0/2] mmc: Fix max current limit handling for SD cards

[Avri Altman]

> On Fri, 4 Jul 2025 at 10:01, Avri Altman <Avri.Altman@sandisk.com> wrote:
> >
> >
> > > This series addresses a long-standing issue in the mmc driver
> > > regarding the setting of the maximum current limit for SD cards.
> > >
> > > The current implementation, which dates back nearly a decade,
> > > misunderstood the intention of the SD specification. To be fair, the
> > > relevant section of the spec is notoriously confusing and difficult
> > > to interpret. As a result, the old code almost always limited cards
> > > to the default low power setting of 0.72W, regardless of the card’s
> > > actual capabilities.
> > >
> > > With this change, we will now correctly set the maximum current
> > > limit, allowing SD cards to receive enough power to operate at their
> > > true potential and unleash their maximum performance.
> > A gentle ping.
> > Would be interested to get some comments and testing.
> > Internally, I saw a +65% improvement, should the platform allow higher
> power than the 0.72w default.
> 
> Sorry for the delay - and unfortunately my review needs to wait a few more
> days, as I need to look at Oleksij's series [1] first.
> 
> Anyway, what does 65% percent mean? It sure sounds impressive! Perhaps
> you can elaborate a bit on the tests you are running and possibly also whether
> this has an impact from a consumed energy point of view?
I was using a plain direct sequential write.  The results are similar for different chunk size.
I know it's weird that the spec was misinterpreted for so many years.
It might happen that there are various hosts in the market that prefer to operate the SD cards
using the ambient temp thermal conditions spec which is relating only to low power cards up to 0.72W…
while from 1.44W and above (high power cards) should be treated by hosts with control of the case temp of the card.
While most of the UHS-I cards seems to operate quite well around 0.72W no body felt the difference…
however, cards that demands closer to 1.44W might suffer from it … and such hosts might suffer from such perf issue.

Thanks,
Avri

> 
> >
> > Thanks,
> > Avri
> >
> > >
> > > Avri Altman (2):
> > >   mmc: core sd: Simplify current limit logic for 200mA default
> > >   mmc: core: sd: Fix and simplify SD card current limit handling
> > >
> > >  drivers/mmc/core/sd.c    | 43 ++++++++++++++--------------------------
> > >  include/linux/mmc/card.h |  7 -------
> > >  2 files changed, 15 insertions(+), 35 deletions(-)
> > >
> > > --
> > > 2.25.1
> >
> 
> Kind regards
> Uffe
> 
> [1]
> https://lore.kernel.org/all/20250616102917.1677116-1-
> o.rempel@pengutronix.de/

Re: [PATCH 1/2] mmc: core sd: Simplify current limit logic for 200mA default

[Ulf Hansson]

On Thu, 19 Jun 2025 at 11:03, Avri Altman <avri.altman@sandisk.com> wrote:
>
> The SD current limit logic is updated to avoid explicitly setting the
> current limit when the maximum power is 200mA (0.72W) or less, as this
> is already the default value. The code now only issues a current limit
> switch if a higher limit is required, and the unused
> SD_SET_CURRENT_NO_CHANGE constant is removed. This reduces unnecessary
> commands and simplifies the logic.
>
> Fixes: 0aa6770000ba ("mmc: sdhci: only set 200mA support for 1.8v if 200mA is available")
> Signed-off-by: Avri Altman <avri.altman@sandisk.com>
> Cc: stable@vger.kernel.org

I am not sure there is really a bug here. To me, it rather looks like
an optimization, as we are avoiding one unnecessary switch command.

Otherwise this looks good to me.

Kind regards
Uffe

> ---
>  drivers/mmc/core/sd.c    | 7 ++-----
>  include/linux/mmc/card.h | 1 -
>  2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index ec02067f03c5..cf92c5b2059a 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -554,7 +554,7 @@ static u32 sd_get_host_max_current(struct mmc_host *host)
>
>  static int sd_set_current_limit(struct mmc_card *card, u8 *status)
>  {
> -       int current_limit = SD_SET_CURRENT_NO_CHANGE;
> +       int current_limit = SD_SET_CURRENT_LIMIT_200;
>         int err;
>         u32 max_current;
>
> @@ -598,11 +598,8 @@ static int sd_set_current_limit(struct mmc_card *card, u8 *status)
>         else if (max_current >= 400 &&
>                  card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_400)
>                 current_limit = SD_SET_CURRENT_LIMIT_400;
> -       else if (max_current >= 200 &&
> -                card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_200)
> -               current_limit = SD_SET_CURRENT_LIMIT_200;
>
> -       if (current_limit != SD_SET_CURRENT_NO_CHANGE) {
> +       if (current_limit != SD_SET_CURRENT_LIMIT_200) {
>                 err = mmc_sd_switch(card, SD_SWITCH_SET, 3,
>                                 current_limit, status);
>                 if (err)
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index ddcdf23d731c..e9e964c20e53 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -182,7 +182,6 @@ struct sd_switch_caps {
>  #define SD_SET_CURRENT_LIMIT_400       1
>  #define SD_SET_CURRENT_LIMIT_600       2
>  #define SD_SET_CURRENT_LIMIT_800       3
> -#define SD_SET_CURRENT_NO_CHANGE       (-1)
>
>  #define SD_MAX_CURRENT_200     (1 << SD_SET_CURRENT_LIMIT_200)
>  #define SD_MAX_CURRENT_400     (1 << SD_SET_CURRENT_LIMIT_400)
> --
> 2.25.1
>

Re: [PATCH 2/2] mmc: core: sd: Fix and simplify SD card current limit handling

[Ulf Hansson]

On Thu, 19 Jun 2025 at 11:04, Avri Altman <avri.altman@sandisk.com> wrote:
>
> The SD spec says: "In UHS-I mode, after selecting one of SDR50, SDR104,
> or DDR50 mode by Function Group 1, host needs to change the Power Limit
> to enable the card to operate in higher performance".
>
> The driver previously determined SD card current limits incorrectly by
> checking capability bits before bus speed was established, and by using
> support bits in function group 4 (bytes 6 & 7) rather than the actual
> current requirement (bytes 0 & 1). This is wrong because the card
> responds for a given bus speed.
>
> This patch queries the card's current requirement after setting the bus
> speed, and uses the reported value to select the appropriate current
> limit.
>
> while at it, remove some unused constants and the misleading comment in
> the code.
>
> Fixes: d9812780a020 ("mmc: sd: limit SD card power limit according to cards capabilities")
> Signed-off-by: Avri Altman <avri.altman@sandisk.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/mmc/core/sd.c    | 36 +++++++++++++-----------------------
>  include/linux/mmc/card.h |  6 ------
>  2 files changed, 13 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index cf92c5b2059a..357edfb910df 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -365,7 +365,6 @@ static int mmc_read_switch(struct mmc_card *card)
>                 card->sw_caps.sd3_bus_mode = status[13];
>                 /* Driver Strengths supported by the card */
>                 card->sw_caps.sd3_drv_type = status[9];
> -               card->sw_caps.sd3_curr_limit = status[7] | status[6] << 8;
>         }
>
>  out:
> @@ -556,7 +555,7 @@ static int sd_set_current_limit(struct mmc_card *card, u8 *status)
>  {
>         int current_limit = SD_SET_CURRENT_LIMIT_200;
>         int err;
> -       u32 max_current;
> +       u32 max_current, card_needs;

Please clarify this by renaming "card_needs" to "card_max_current".

>
>         /*
>          * Current limit switch is only defined for SDR50, SDR104, and DDR50
> @@ -575,33 +574,24 @@ static int sd_set_current_limit(struct mmc_card *card, u8 *status)
>         max_current = sd_get_host_max_current(card->host);

Looking at the implementation of sd_get_host_max_current(), it's very limiting.

For example, if we are using MMC_VDD_34_35 or MMC_VDD_35_36, the
function returns 0. Maybe this is good enough based upon those host
drivers that actually sets host->max_current_180|300|330, but it kind
of looks wrong to me.

I think we should re-work this interface to let us retrieve the
maximum current from the host in a more flexible way. What we are
really looking for is a value in Watt instead, I think. Don't get me
wrong, this deserved it's own standalone patch on top of $subject
patch.

>
>         /*
> -        * We only check host's capability here, if we set a limit that is
> -        * higher than the card's maximum current, the card will be using its
> -        * maximum current, e.g. if the card's maximum current is 300ma, and
> -        * when we set current limit to 200ma, the card will draw 200ma, and
> -        * when we set current limit to 400/600/800ma, the card will draw its
> -        * maximum 300ma from the host.
> -        *
> -        * The above is incorrect: if we try to set a current limit that is
> -        * not supported by the card, the card can rightfully error out the
> -        * attempt, and remain at the default current limit.  This results
> -        * in a 300mA card being limited to 200mA even though the host
> -        * supports 800mA. Failures seen with SanDisk 8GB UHS cards with
> -        * an iMX6 host. --rmk

I think it's important to keep some of the information from above, as
it still stands, if I understand correctly.

> +        * query the card of its maximun current/power consumption given the
> +        * bus speed mode
>          */
> -       if (max_current >= 800 &&
> -           card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_800)
> +       err = mmc_sd_switch(card, 0, 0, card->sd_bus_speed, status);
> +       if (err)
> +               return err;
> +
> +       card_needs = status[1] | status[0] << 8;

Please add a comment on what bits/fields we are parsing for. This
looks like magic to me. :-)

> +
> +       if (max_current >= 800 && card_needs > 600)
>                 current_limit = SD_SET_CURRENT_LIMIT_800;
> -       else if (max_current >= 600 &&
> -                card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_600)
> +       else if (max_current >= 600 && card_needs > 400)
>                 current_limit = SD_SET_CURRENT_LIMIT_600;
> -       else if (max_current >= 400 &&
> -                card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_400)
> +       else if (max_current >= 400 && card_needs > 200)
>                 current_limit = SD_SET_CURRENT_LIMIT_400;
>
>         if (current_limit != SD_SET_CURRENT_LIMIT_200) {
> -               err = mmc_sd_switch(card, SD_SWITCH_SET, 3,
> -                               current_limit, status);
> +               err = mmc_sd_switch(card, SD_SWITCH_SET, 3, current_limit, status);
>                 if (err)
>                         return err;
>
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index e9e964c20e53..67c1386ca574 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -177,17 +177,11 @@ struct sd_switch_caps {
>  #define SD_DRIVER_TYPE_A       0x02
>  #define SD_DRIVER_TYPE_C       0x04
>  #define SD_DRIVER_TYPE_D       0x08
> -       unsigned int            sd3_curr_limit;
>  #define SD_SET_CURRENT_LIMIT_200       0
>  #define SD_SET_CURRENT_LIMIT_400       1
>  #define SD_SET_CURRENT_LIMIT_600       2
>  #define SD_SET_CURRENT_LIMIT_800       3
>
> -#define SD_MAX_CURRENT_200     (1 << SD_SET_CURRENT_LIMIT_200)
> -#define SD_MAX_CURRENT_400     (1 << SD_SET_CURRENT_LIMIT_400)
> -#define SD_MAX_CURRENT_600     (1 << SD_SET_CURRENT_LIMIT_600)
> -#define SD_MAX_CURRENT_800     (1 << SD_SET_CURRENT_LIMIT_800)
> -
>  #define SD4_SET_POWER_LIMIT_0_72W      0
>  #define SD4_SET_POWER_LIMIT_1_44W      1
>  #define SD4_SET_POWER_LIMIT_2_16W      2
> --
> 2.25.1
>

Finally, it would be nice to have some more information about the test
you have done to verify this. The performance numbers are very
interesting, as it seems like some cards/platforms could really
benefit from this a lot. Would you mind extending the commit message
with some more information about this?

Kind regards
Uffe

RE: [PATCH 1/2] mmc: core sd: Simplify current limit logic for 200mA default

[Avri Altman]

> On Thu, 19 Jun 2025 at 11:03, Avri Altman <avri.altman@sandisk.com>
> wrote:
> >
> > The SD current limit logic is updated to avoid explicitly setting the
> > current limit when the maximum power is 200mA (0.72W) or less, as this
> > is already the default value. The code now only issues a current limit
> > switch if a higher limit is required, and the unused
> > SD_SET_CURRENT_NO_CHANGE constant is removed. This reduces
> unnecessary
> > commands and simplifies the logic.
> >
> > Fixes: 0aa6770000ba ("mmc: sdhci: only set 200mA support for 1.8v if
> > 200mA is available")
> > Signed-off-by: Avri Altman <avri.altman@sandisk.com>
> > Cc: stable@vger.kernel.org
> 
> I am not sure there is really a bug here. To me, it rather looks like an
> optimization, as we are avoiding one unnecessary switch command.
Done.

Thanks,
Avri

> 
> Otherwise this looks good to me.
> 
> Kind regards
> Uffe
> 
> > ---
> >  drivers/mmc/core/sd.c    | 7 ++-----
> >  include/linux/mmc/card.h | 1 -
> >  2 files changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c index
> > ec02067f03c5..cf92c5b2059a 100644
> > --- a/drivers/mmc/core/sd.c
> > +++ b/drivers/mmc/core/sd.c
> > @@ -554,7 +554,7 @@ static u32 sd_get_host_max_current(struct
> mmc_host
> > *host)
> >
> >  static int sd_set_current_limit(struct mmc_card *card, u8 *status)  {
> > -       int current_limit = SD_SET_CURRENT_NO_CHANGE;
> > +       int current_limit = SD_SET_CURRENT_LIMIT_200;
> >         int err;
> >         u32 max_current;
> >
> > @@ -598,11 +598,8 @@ static int sd_set_current_limit(struct mmc_card
> *card, u8 *status)
> >         else if (max_current >= 400 &&
> >                  card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_400)
> >                 current_limit = SD_SET_CURRENT_LIMIT_400;
> > -       else if (max_current >= 200 &&
> > -                card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_200)
> > -               current_limit = SD_SET_CURRENT_LIMIT_200;
> >
> > -       if (current_limit != SD_SET_CURRENT_NO_CHANGE) {
> > +       if (current_limit != SD_SET_CURRENT_LIMIT_200) {
> >                 err = mmc_sd_switch(card, SD_SWITCH_SET, 3,
> >                                 current_limit, status);
> >                 if (err)
> > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h index
> > ddcdf23d731c..e9e964c20e53 100644
> > --- a/include/linux/mmc/card.h
> > +++ b/include/linux/mmc/card.h
> > @@ -182,7 +182,6 @@ struct sd_switch_caps {
> >  #define SD_SET_CURRENT_LIMIT_400       1
> >  #define SD_SET_CURRENT_LIMIT_600       2
> >  #define SD_SET_CURRENT_LIMIT_800       3
> > -#define SD_SET_CURRENT_NO_CHANGE       (-1)
> >
> >  #define SD_MAX_CURRENT_200     (1 << SD_SET_CURRENT_LIMIT_200)
> >  #define SD_MAX_CURRENT_400     (1 << SD_SET_CURRENT_LIMIT_400)
> > --
> > 2.25.1
> >

RE: [PATCH 2/2] mmc: core: sd: Fix and simplify SD card current limit handling

[Avri Altman]

> On Thu, 19 Jun 2025 at 11:04, Avri Altman <avri.altman@sandisk.com> wrote:
> >
> > The SD spec says: "In UHS-I mode, after selecting one of SDR50,
> > SDR104, or DDR50 mode by Function Group 1, host needs to change the
> > Power Limit to enable the card to operate in higher performance".
> >
> > The driver previously determined SD card current limits incorrectly by
> > checking capability bits before bus speed was established, and by
> > using support bits in function group 4 (bytes 6 & 7) rather than the
> > actual current requirement (bytes 0 & 1). This is wrong because the
> > card responds for a given bus speed.
> >
> > This patch queries the card's current requirement after setting the
> > bus speed, and uses the reported value to select the appropriate
> > current limit.
> >
> > while at it, remove some unused constants and the misleading comment
> > in the code.
> >
> > Fixes: d9812780a020 ("mmc: sd: limit SD card power limit according to
> > cards capabilities")
> > Signed-off-by: Avri Altman <avri.altman@sandisk.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/mmc/core/sd.c    | 36 +++++++++++++-----------------------
> >  include/linux/mmc/card.h |  6 ------
> >  2 files changed, 13 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c index
> > cf92c5b2059a..357edfb910df 100644
> > --- a/drivers/mmc/core/sd.c
> > +++ b/drivers/mmc/core/sd.c
> > @@ -365,7 +365,6 @@ static int mmc_read_switch(struct mmc_card *card)
> >                 card->sw_caps.sd3_bus_mode = status[13];
> >                 /* Driver Strengths supported by the card */
> >                 card->sw_caps.sd3_drv_type = status[9];
> > -               card->sw_caps.sd3_curr_limit = status[7] | status[6] << 8;
> >         }
> >
> >  out:
> > @@ -556,7 +555,7 @@ static int sd_set_current_limit(struct mmc_card
> > *card, u8 *status)  {
> >         int current_limit = SD_SET_CURRENT_LIMIT_200;
> >         int err;
> > -       u32 max_current;
> > +       u32 max_current, card_needs;
> 
> Please clarify this by renaming "card_needs" to "card_max_current".
Done.

> 
> >
> >         /*
> >          * Current limit switch is only defined for SDR50, SDR104, and
> > DDR50 @@ -575,33 +574,24 @@ static int sd_set_current_limit(struct
> mmc_card *card, u8 *status)
> >         max_current = sd_get_host_max_current(card->host);
> 
> Looking at the implementation of sd_get_host_max_current(), it's very limiting.
> 
> For example, if we are using MMC_VDD_34_35 or MMC_VDD_35_36, the
> function returns 0. Maybe this is good enough based upon those host drivers that
> actually sets host->max_current_180|300|330, but it kind of looks wrong to me.
> 
> I think we should re-work this interface to let us retrieve the maximum current
> from the host in a more flexible way. What we are really looking for is a value in
> Watt instead, I think. Don't get me wrong, this deserved it's own standalone patch
> on top of $subject patch.
I still need to consult internally, but Yes - I agree.
Ultimately however, CMD6 expects us to fill the current limit value, so multiplying by voltage and dividing it back seems superfluous.
How about adding to missing vdd and treat them as max_current_330, like in sdhci_get_vdd_value?
Maybe something like:

+/*
+ * Get host's max current setting at its current voltage normalized to 3.6
+ * volt which is the voltage in which the card defines its limits
+ */
+static u32 sd_host_normalized_max_current(struct mmc_host *host)
+{
+       u32 voltage, max_current;
+
+       voltage = 1 << host->ios.vdd;
+       switch (voltage) {
+       case MMC_VDD_165_195:
+               max_current = host->max_current_180 * 180 / 360;
+               break;
+       case MMC_VDD_29_30:
+       case MMC_VDD_30_31:
+               max_current = host->max_current_300 * 300 / 360;
+               break;
+       case MMC_VDD_32_33:
+       case MMC_VDD_33_34:
+       case MMC_VDD_34_35:
+       case MMC_VDD_35_36:
+               max_current = host->max_current_330 * 330 / 360;
+               break;
+       default:
+               max_current = 0;
+       }
+
+       return max_current;
+}
+
 /* Get host's max current setting at its current voltage */
 static u32 sd_get_host_max_current(struct mmc_host *host)
 {
@@ -572,7 +602,7 @@ static int sd_set_current_limit(struct mmc_card *card, u8 *status)
         * Host has different current capabilities when operating at
         * different voltages, so find out its max current first.
         */
-       max_current = sd_get_host_max_current(card->host);
+       max_current = sd_host_normalized_max_current(card->host);


> 
> >
> >         /*
> > -        * We only check host's capability here, if we set a limit that is
> > -        * higher than the card's maximum current, the card will be using its
> > -        * maximum current, e.g. if the card's maximum current is 300ma, and
> > -        * when we set current limit to 200ma, the card will draw 200ma, and
> > -        * when we set current limit to 400/600/800ma, the card will draw its
> > -        * maximum 300ma from the host.
> > -        *
> > -        * The above is incorrect: if we try to set a current limit that is
> > -        * not supported by the card, the card can rightfully error out the
> > -        * attempt, and remain at the default current limit.  This results
> > -        * in a 300mA card being limited to 200mA even though the host
> > -        * supports 800mA. Failures seen with SanDisk 8GB UHS cards with
> > -        * an iMX6 host. --rmk
> 
> I think it's important to keep some of the information from above, as it still stands,
> if I understand correctly.
I believe this highlights a common misunderstanding: it conflates the card’s capabilities (i.e., the maximum current it can support)
with its actual power consumption, which depends on the required bus speed and operating conditions.
The card will never specify or attempt to draw more current than it is capable of handling.
If a current limit is set that exceeds the card’s capability, the card will not draw beyond its maximum;
instead, it will operate within its own limits or may reject unsupported current limit settings as per the specification.
Therefore, the logic should distinguish between the card’s advertised capabilities and its real-time power requirements,
ensuring we do not confuse the two concepts.

> 
> > +        * query the card of its maximun current/power consumption given the
> > +        * bus speed mode
> >          */
> > -       if (max_current >= 800 &&
> > -           card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_800)
> > +       err = mmc_sd_switch(card, 0, 0, card->sd_bus_speed, status);
> > +       if (err)
> > +               return err;
> > +
> > +       card_needs = status[1] | status[0] << 8;
> 
> Please add a comment on what bits/fields we are parsing for. This looks like magic
> to me. :-)
Done.

> 
> > +
> > +       if (max_current >= 800 && card_needs > 600)
> >                 current_limit = SD_SET_CURRENT_LIMIT_800;
> > -       else if (max_current >= 600 &&
> > -                card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_600)
> > +       else if (max_current >= 600 && card_needs > 400)
> >                 current_limit = SD_SET_CURRENT_LIMIT_600;
> > -       else if (max_current >= 400 &&
> > -                card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_400)
> > +       else if (max_current >= 400 && card_needs > 200)
> >                 current_limit = SD_SET_CURRENT_LIMIT_400;
> >
> >         if (current_limit != SD_SET_CURRENT_LIMIT_200) {
> > -               err = mmc_sd_switch(card, SD_SWITCH_SET, 3,
> > -                               current_limit, status);
> > +               err = mmc_sd_switch(card, SD_SWITCH_SET, 3,
> > + current_limit, status);
> >                 if (err)
> >                         return err;
> >
> > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h index
> > e9e964c20e53..67c1386ca574 100644
> > --- a/include/linux/mmc/card.h
> > +++ b/include/linux/mmc/card.h
> > @@ -177,17 +177,11 @@ struct sd_switch_caps {
> >  #define SD_DRIVER_TYPE_A       0x02
> >  #define SD_DRIVER_TYPE_C       0x04
> >  #define SD_DRIVER_TYPE_D       0x08
> > -       unsigned int            sd3_curr_limit;
> >  #define SD_SET_CURRENT_LIMIT_200       0
> >  #define SD_SET_CURRENT_LIMIT_400       1
> >  #define SD_SET_CURRENT_LIMIT_600       2
> >  #define SD_SET_CURRENT_LIMIT_800       3
> >
> > -#define SD_MAX_CURRENT_200     (1 << SD_SET_CURRENT_LIMIT_200)
> > -#define SD_MAX_CURRENT_400     (1 << SD_SET_CURRENT_LIMIT_400)
> > -#define SD_MAX_CURRENT_600     (1 << SD_SET_CURRENT_LIMIT_600)
> > -#define SD_MAX_CURRENT_800     (1 << SD_SET_CURRENT_LIMIT_800)
> > -
> >  #define SD4_SET_POWER_LIMIT_0_72W      0
> >  #define SD4_SET_POWER_LIMIT_1_44W      1
> >  #define SD4_SET_POWER_LIMIT_2_16W      2
> > --
> > 2.25.1
> >
> 
> Finally, it would be nice to have some more information about the test you have
> done to verify this. The performance numbers are very interesting, as it seems
> like some cards/platforms could really benefit from this a lot. Would you mind
> extending the commit message with some more information about this?
Done.

Thanks,
Avri
> 
> Kind regards
> Uffe

Re: [PATCH 2/2] mmc: core: sd: Fix and simplify SD card current limit handling

[Ulf Hansson]

[...]

> >
> > >
> > >         /*
> > >          * Current limit switch is only defined for SDR50, SDR104, and
> > > DDR50 @@ -575,33 +574,24 @@ static int sd_set_current_limit(struct
> > mmc_card *card, u8 *status)
> > >         max_current = sd_get_host_max_current(card->host);
> >
> > Looking at the implementation of sd_get_host_max_current(), it's very limiting.
> >
> > For example, if we are using MMC_VDD_34_35 or MMC_VDD_35_36, the
> > function returns 0. Maybe this is good enough based upon those host drivers that
> > actually sets host->max_current_180|300|330, but it kind of looks wrong to me.
> >
> > I think we should re-work this interface to let us retrieve the maximum current
> > from the host in a more flexible way. What we are really looking for is a value in
> > Watt instead, I think. Don't get me wrong, this deserved it's own standalone patch
> > on top of $subject patch.
> I still need to consult internally, but Yes - I agree.
> Ultimately however, CMD6 expects us to fill the current limit value, so multiplying by voltage and dividing it back seems superfluous.
> How about adding to missing vdd and treat them as max_current_330, like in sdhci_get_vdd_value?
> Maybe something like:
>
> +/*
> + * Get host's max current setting at its current voltage normalized to 3.6
> + * volt which is the voltage in which the card defines its limits
> + */
> +static u32 sd_host_normalized_max_current(struct mmc_host *host)
> +{
> +       u32 voltage, max_current;
> +
> +       voltage = 1 << host->ios.vdd;
> +       switch (voltage) {
> +       case MMC_VDD_165_195:
> +               max_current = host->max_current_180 * 180 / 360;
> +               break;
> +       case MMC_VDD_29_30:
> +       case MMC_VDD_30_31:
> +               max_current = host->max_current_300 * 300 / 360;
> +               break;
> +       case MMC_VDD_32_33:
> +       case MMC_VDD_33_34:
> +       case MMC_VDD_34_35:
> +       case MMC_VDD_35_36:
> +               max_current = host->max_current_330 * 330 / 360;
> +               break;
> +       default:
> +               max_current = 0;
> +       }
> +
> +       return max_current;
> +}

I think it's way better than the current implementation in
sd_get_host_max_current().

Still, I still think it's weird to have three different variables in
the host, max_current_180, max_current_300 and max_current_330. That
seems like an SDHCI specific thing to me, unless I am mistaken.

I would rather see a more flexible interface where we move away from
using host->max_current_180|300|330 entirely and have a function that
returns the supported limit (whatever unit we decide). Maybe it also
makes sense to provide some additional helpers from the core that host
drivers can call, to fetch/translate the values it should provide for
this.

> +
>  /* Get host's max current setting at its current voltage */
>  static u32 sd_get_host_max_current(struct mmc_host *host)
>  {
> @@ -572,7 +602,7 @@ static int sd_set_current_limit(struct mmc_card *card, u8 *status)
>          * Host has different current capabilities when operating at
>          * different voltages, so find out its max current first.
>          */
> -       max_current = sd_get_host_max_current(card->host);
> +       max_current = sd_host_normalized_max_current(card->host);
>
>
> >
> > >
> > >         /*
> > > -        * We only check host's capability here, if we set a limit that is
> > > -        * higher than the card's maximum current, the card will be using its
> > > -        * maximum current, e.g. if the card's maximum current is 300ma, and
> > > -        * when we set current limit to 200ma, the card will draw 200ma, and
> > > -        * when we set current limit to 400/600/800ma, the card will draw its
> > > -        * maximum 300ma from the host.
> > > -        *
> > > -        * The above is incorrect: if we try to set a current limit that is
> > > -        * not supported by the card, the card can rightfully error out the
> > > -        * attempt, and remain at the default current limit.  This results
> > > -        * in a 300mA card being limited to 200mA even though the host
> > > -        * supports 800mA. Failures seen with SanDisk 8GB UHS cards with
> > > -        * an iMX6 host. --rmk
> >
> > I think it's important to keep some of the information from above, as it still stands,
> > if I understand correctly.
> I believe this highlights a common misunderstanding: it conflates the card’s capabilities (i.e., the maximum current it can support)
> with its actual power consumption, which depends on the required bus speed and operating conditions.
> The card will never specify or attempt to draw more current than it is capable of handling.
> If a current limit is set that exceeds the card’s capability, the card will not draw beyond its maximum;
> instead, it will operate within its own limits or may reject unsupported current limit settings as per the specification.
> Therefore, the logic should distinguish between the card’s advertised capabilities and its real-time power requirements,
> ensuring we do not confuse the two concepts.

I understand what you are saying and it makes perfect sense to me.

My point is, I think we need some more information in the comment
rather than the two lines below that you propose. It doesn't matter to
me if you drop the comments above, just make sure we understand what
goes on here by giving more details, then I will be happy. :-)

>
> >
> > > +        * query the card of its maximun current/power consumption given the
> > > +        * bus speed mode
> > >          */
> > > -       if (max_current >= 800 &&
> > > -           card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_800)
> > > +       err = mmc_sd_switch(card, 0, 0, card->sd_bus_speed, status);
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       card_needs = status[1] | status[0] << 8;
> >

[...]

Kind regards
Uffe

RE: [PATCH 2/2] mmc: core: sd: Fix and simplify SD card current limit handling

[Avri Altman]

> 
> > >
> > > >
> > > >         /*
> > > >          * Current limit switch is only defined for SDR50, SDR104,
> > > > and
> > > > DDR50 @@ -575,33 +574,24 @@ static int sd_set_current_limit(struct
> > > mmc_card *card, u8 *status)
> > > >         max_current = sd_get_host_max_current(card->host);
> > >
> > > Looking at the implementation of sd_get_host_max_current(), it's very
> limiting.
> > >
> > > For example, if we are using MMC_VDD_34_35 or MMC_VDD_35_36, the
> > > function returns 0. Maybe this is good enough based upon those host
> > > drivers that actually sets host->max_current_180|300|330, but it kind of
> looks wrong to me.
> > >
> > > I think we should re-work this interface to let us retrieve the
> > > maximum current from the host in a more flexible way. What we are
> > > really looking for is a value in Watt instead, I think. Don't get me
> > > wrong, this deserved it's own standalone patch on top of $subject patch.
> > I still need to consult internally, but Yes - I agree.
> > Ultimately however, CMD6 expects us to fill the current limit value, so
> multiplying by voltage and dividing it back seems superfluous.
> > How about adding to missing vdd and treat them as max_current_330, like in
> sdhci_get_vdd_value?
> > Maybe something like:
> >
> > +/*
> > + * Get host's max current setting at its current voltage normalized
> > +to 3.6
> > + * volt which is the voltage in which the card defines its limits  */
> > +static u32 sd_host_normalized_max_current(struct mmc_host *host) {
> > +       u32 voltage, max_current;
> > +
> > +       voltage = 1 << host->ios.vdd;
> > +       switch (voltage) {
> > +       case MMC_VDD_165_195:
> > +               max_current = host->max_current_180 * 180 / 360;
> > +               break;
> > +       case MMC_VDD_29_30:
> > +       case MMC_VDD_30_31:
> > +               max_current = host->max_current_300 * 300 / 360;
> > +               break;
> > +       case MMC_VDD_32_33:
> > +       case MMC_VDD_33_34:
> > +       case MMC_VDD_34_35:
> > +       case MMC_VDD_35_36:
> > +               max_current = host->max_current_330 * 330 / 360;
> > +               break;
> > +       default:
> > +               max_current = 0;
> > +       }
> > +
> > +       return max_current;
> > +}
> 
> I think it's way better than the current implementation in
> sd_get_host_max_current().
> 
> Still, I still think it's weird to have three different variables in the host,
> max_current_180, max_current_300 and max_current_330. That seems like an
> SDHCI specific thing to me, unless I am mistaken.
> 
> I would rather see a more flexible interface where we move away from using
> host->max_current_180|300|330 entirely and have a function that returns the
> supported limit (whatever unit we decide). Maybe it also makes sense to provide
> some additional helpers from the core that host drivers can call, to fetch/translate
> the values it should provide for this.
+Adrian

IIUC, you are looking for a host->max_power to replace the above.
However, giver that:
a) there is no power class in SD like in mmc, and the card needs to be set to a power-limit
b) the platform supported voltages can be either be given via DT as well as hard-coded and it's shared with mmc, and
c) the platform supported max current is either read from the sdhci register as well as can be hard-coded
I am not sure if and where we should set this max_power member, but I am open for suggestions.

> 
> > +
> >  /* Get host's max current setting at its current voltage */  static
> > u32 sd_get_host_max_current(struct mmc_host *host)  { @@ -572,7 +602,7
> > @@ static int sd_set_current_limit(struct mmc_card *card, u8 *status)
> >          * Host has different current capabilities when operating at
> >          * different voltages, so find out its max current first.
> >          */
> > -       max_current = sd_get_host_max_current(card->host);
> > +       max_current = sd_host_normalized_max_current(card->host);
> >
> >
> > >
> > > >
> > > >         /*
> > > > -        * We only check host's capability here, if we set a limit that is
> > > > -        * higher than the card's maximum current, the card will be using its
> > > > -        * maximum current, e.g. if the card's maximum current is 300ma, and
> > > > -        * when we set current limit to 200ma, the card will draw 200ma, and
> > > > -        * when we set current limit to 400/600/800ma, the card will draw its
> > > > -        * maximum 300ma from the host.
> > > > -        *
> > > > -        * The above is incorrect: if we try to set a current limit that is
> > > > -        * not supported by the card, the card can rightfully error out the
> > > > -        * attempt, and remain at the default current limit.  This results
> > > > -        * in a 300mA card being limited to 200mA even though the host
> > > > -        * supports 800mA. Failures seen with SanDisk 8GB UHS cards with
> > > > -        * an iMX6 host. --rmk
> > >
> > > I think it's important to keep some of the information from above,
> > > as it still stands, if I understand correctly.
> > I believe this highlights a common misunderstanding: it conflates the
> > card’s capabilities (i.e., the maximum current it can support) with its actual
> power consumption, which depends on the required bus speed and operating
> conditions.
> > The card will never specify or attempt to draw more current than it is capable of
> handling.
> > If a current limit is set that exceeds the card’s capability, the card
> > will not draw beyond its maximum; instead, it will operate within its own limits
> or may reject unsupported current limit settings as per the specification.
> > Therefore, the logic should distinguish between the card’s advertised
> > capabilities and its real-time power requirements, ensuring we do not confuse
> the two concepts.
> 
> I understand what you are saying and it makes perfect sense to me.
> 
> My point is, I think we need some more information in the comment rather than
> the two lines below that you propose. It doesn't matter to me if you drop the
> comments above, just make sure we understand what goes on here by giving
> more details, then I will be happy. :-)
Done.

Thanks,
Avri

> 
> >
> > >
> > > > +        * query the card of its maximun current/power consumption given the
> > > > +        * bus speed mode
> > > >          */
> > > > -       if (max_current >= 800 &&
> > > > -           card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_800)
> > > > +       err = mmc_sd_switch(card, 0, 0, card->sd_bus_speed, status);
> > > > +       if (err)
> > > > +               return err;
> > > > +
> > > > +       card_needs = status[1] | status[0] << 8;
> > >
> 
> [...]
> 
> Kind regards
> Uffe

Re: [PATCH 2/2] mmc: core: sd: Fix and simplify SD card current limit handling

[Ulf Hansson]

On Thu, 10 Jul 2025 at 09:57, Avri Altman <Avri.Altman@sandisk.com> wrote:
>
> >
> > > >
> > > > >
> > > > >         /*
> > > > >          * Current limit switch is only defined for SDR50, SDR104,
> > > > > and
> > > > > DDR50 @@ -575,33 +574,24 @@ static int sd_set_current_limit(struct
> > > > mmc_card *card, u8 *status)
> > > > >         max_current = sd_get_host_max_current(card->host);
> > > >
> > > > Looking at the implementation of sd_get_host_max_current(), it's very
> > limiting.
> > > >
> > > > For example, if we are using MMC_VDD_34_35 or MMC_VDD_35_36, the
> > > > function returns 0. Maybe this is good enough based upon those host
> > > > drivers that actually sets host->max_current_180|300|330, but it kind of
> > looks wrong to me.
> > > >
> > > > I think we should re-work this interface to let us retrieve the
> > > > maximum current from the host in a more flexible way. What we are
> > > > really looking for is a value in Watt instead, I think. Don't get me
> > > > wrong, this deserved it's own standalone patch on top of $subject patch.
> > > I still need to consult internally, but Yes - I agree.
> > > Ultimately however, CMD6 expects us to fill the current limit value, so
> > multiplying by voltage and dividing it back seems superfluous.
> > > How about adding to missing vdd and treat them as max_current_330, like in
> > sdhci_get_vdd_value?
> > > Maybe something like:
> > >
> > > +/*
> > > + * Get host's max current setting at its current voltage normalized
> > > +to 3.6
> > > + * volt which is the voltage in which the card defines its limits  */
> > > +static u32 sd_host_normalized_max_current(struct mmc_host *host) {
> > > +       u32 voltage, max_current;
> > > +
> > > +       voltage = 1 << host->ios.vdd;
> > > +       switch (voltage) {
> > > +       case MMC_VDD_165_195:
> > > +               max_current = host->max_current_180 * 180 / 360;
> > > +               break;
> > > +       case MMC_VDD_29_30:
> > > +       case MMC_VDD_30_31:
> > > +               max_current = host->max_current_300 * 300 / 360;
> > > +               break;
> > > +       case MMC_VDD_32_33:
> > > +       case MMC_VDD_33_34:
> > > +       case MMC_VDD_34_35:
> > > +       case MMC_VDD_35_36:
> > > +               max_current = host->max_current_330 * 330 / 360;
> > > +               break;
> > > +       default:
> > > +               max_current = 0;
> > > +       }
> > > +
> > > +       return max_current;
> > > +}
> >
> > I think it's way better than the current implementation in
> > sd_get_host_max_current().
> >
> > Still, I still think it's weird to have three different variables in the host,
> > max_current_180, max_current_300 and max_current_330. That seems like an
> > SDHCI specific thing to me, unless I am mistaken.
> >
> > I would rather see a more flexible interface where we move away from using
> > host->max_current_180|300|330 entirely and have a function that returns the
> > supported limit (whatever unit we decide). Maybe it also makes sense to provide
> > some additional helpers from the core that host drivers can call, to fetch/translate
> > the values it should provide for this.
> +Adrian
>
> IIUC, you are looking for a host->max_power to replace the above.

No, the new function/callback should provide us the value immediately,
rather than having it stored in the host struct.

> However, giver that:
> a) there is no power class in SD like in mmc, and the card needs to be set to a power-limit
> b) the platform supported voltages can be either be given via DT as well as hard-coded and it's shared with mmc, and
> c) the platform supported max current is either read from the sdhci register as well as can be hard-coded
> I am not sure if and where we should set this max_power member, but I am open for suggestions.

I will certainly be host specific, so we need to have a host ops for
it. Depending on how the host is powering the card, it may need to do
different things to get the max-current/max-power for the currently
selected voltage-level for vcc/vdd.

I can take a stab and post a draft for it. I will keep you posted.

[...]

Kind regards
Uffe

RE: [PATCH 2/2] mmc: core: sd: Fix and simplify SD card current limit handling

[Avri Altman]

> On Thu, 10 Jul 2025 at 09:57, Avri Altman <Avri.Altman@sandisk.com> wrote:
> >
> > >
> > > > >
> > > > > >
> > > > > >         /*
> > > > > >          * Current limit switch is only defined for SDR50,
> > > > > > SDR104, and
> > > > > > DDR50 @@ -575,33 +574,24 @@ static int
> > > > > > sd_set_current_limit(struct
> > > > > mmc_card *card, u8 *status)
> > > > > >         max_current = sd_get_host_max_current(card->host);
> > > > >
> > > > > Looking at the implementation of sd_get_host_max_current(), it's
> > > > > very
> > > limiting.
> > > > >
> > > > > For example, if we are using MMC_VDD_34_35 or MMC_VDD_35_36, the
> > > > > function returns 0. Maybe this is good enough based upon those
> > > > > host drivers that actually sets host->max_current_180|300|330,
> > > > > but it kind of
> > > looks wrong to me.
> > > > >
> > > > > I think we should re-work this interface to let us retrieve the
> > > > > maximum current from the host in a more flexible way. What we
> > > > > are really looking for is a value in Watt instead, I think.
> > > > > Don't get me wrong, this deserved it's own standalone patch on top of
> $subject patch.
> > > > I still need to consult internally, but Yes - I agree.
> > > > Ultimately however, CMD6 expects us to fill the current limit
> > > > value, so
> > > multiplying by voltage and dividing it back seems superfluous.
> > > > How about adding to missing vdd and treat them as max_current_330,
> > > > like in
> > > sdhci_get_vdd_value?
> > > > Maybe something like:
> > > >
> > > > +/*
> > > > + * Get host's max current setting at its current voltage
> > > > +normalized to 3.6
> > > > + * volt which is the voltage in which the card defines its limits
> > > > +*/ static u32 sd_host_normalized_max_current(struct mmc_host *host) {
> > > > +       u32 voltage, max_current;
> > > > +
> > > > +       voltage = 1 << host->ios.vdd;
> > > > +       switch (voltage) {
> > > > +       case MMC_VDD_165_195:
> > > > +               max_current = host->max_current_180 * 180 / 360;
> > > > +               break;
> > > > +       case MMC_VDD_29_30:
> > > > +       case MMC_VDD_30_31:
> > > > +               max_current = host->max_current_300 * 300 / 360;
> > > > +               break;
> > > > +       case MMC_VDD_32_33:
> > > > +       case MMC_VDD_33_34:
> > > > +       case MMC_VDD_34_35:
> > > > +       case MMC_VDD_35_36:
> > > > +               max_current = host->max_current_330 * 330 / 360;
> > > > +               break;
> > > > +       default:
> > > > +               max_current = 0;
> > > > +       }
> > > > +
> > > > +       return max_current;
> > > > +}
> > >
> > > I think it's way better than the current implementation in
> > > sd_get_host_max_current().
> > >
> > > Still, I still think it's weird to have three different variables in
> > > the host, max_current_180, max_current_300 and max_current_330. That
> > > seems like an SDHCI specific thing to me, unless I am mistaken.
> > >
> > > I would rather see a more flexible interface where we move away from
> > > using
> > > host->max_current_180|300|330 entirely and have a function that
> > > host->returns the
> > > supported limit (whatever unit we decide). Maybe it also makes sense
> > > to provide some additional helpers from the core that host drivers
> > > can call, to fetch/translate the values it should provide for this.
> > +Adrian
> >
> > IIUC, you are looking for a host->max_power to replace the above.
> 
> No, the new function/callback should provide us the value immediately, rather
> than having it stored in the host struct.
> 
> > However, giver that:
> > a) there is no power class in SD like in mmc, and the card needs to be
> > set to a power-limit
> > b) the platform supported voltages can be either be given via DT as
> > well as hard-coded and it's shared with mmc, and
> > c) the platform supported max current is either read from the sdhci
> > register as well as can be hard-coded I am not sure if and where we should set
> this max_power member, but I am open for suggestions.
> 
> I will certainly be host specific, so we need to have a host ops for it. Depending on
> how the host is powering the card, it may need to do different things to get the
> max-current/max-power for the currently selected voltage-level for vcc/vdd.
> 
> I can take a stab and post a draft for it. I will keep you posted.
That would be great.
Thanks a lot.

Thanks,
Avri

> 
> [...]
> 
> Kind regards
> Uffe

Re: [PATCH 1/2] mmc: core sd: Simplify current limit logic for 200mA default

[Ulf Hansson]

On Thu, 19 Jun 2025 at 11:03, Avri Altman <avri.altman@sandisk.com> wrote:
>
> The SD current limit logic is updated to avoid explicitly setting the
> current limit when the maximum power is 200mA (0.72W) or less, as this
> is already the default value. The code now only issues a current limit
> switch if a higher limit is required, and the unused
> SD_SET_CURRENT_NO_CHANGE constant is removed. This reduces unnecessary
> commands and simplifies the logic.
>
> Fixes: 0aa6770000ba ("mmc: sdhci: only set 200mA support for 1.8v if 200mA is available")
> Signed-off-by: Avri Altman <avri.altman@sandisk.com>
> Cc: stable@vger.kernel.org

This has fallen behind, sorry!

I have dropped the stable/fixes tag and clarified the commit message a
bit, before I applied this for next, thanks!

Let's try to move forward on patch 2 too. Avri, if you have the time
to do a re-spin? Otherwise, I will try to get some time to have a stab
at it soon.

Kind regards
Uffe

> ---
>  drivers/mmc/core/sd.c    | 7 ++-----
>  include/linux/mmc/card.h | 1 -
>  2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index ec02067f03c5..cf92c5b2059a 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -554,7 +554,7 @@ static u32 sd_get_host_max_current(struct mmc_host *host)
>
>  static int sd_set_current_limit(struct mmc_card *card, u8 *status)
>  {
> -       int current_limit = SD_SET_CURRENT_NO_CHANGE;
> +       int current_limit = SD_SET_CURRENT_LIMIT_200;
>         int err;
>         u32 max_current;
>
> @@ -598,11 +598,8 @@ static int sd_set_current_limit(struct mmc_card *card, u8 *status)
>         else if (max_current >= 400 &&
>                  card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_400)
>                 current_limit = SD_SET_CURRENT_LIMIT_400;
> -       else if (max_current >= 200 &&
> -                card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_200)
> -               current_limit = SD_SET_CURRENT_LIMIT_200;
>
> -       if (current_limit != SD_SET_CURRENT_NO_CHANGE) {
> +       if (current_limit != SD_SET_CURRENT_LIMIT_200) {
>                 err = mmc_sd_switch(card, SD_SWITCH_SET, 3,
>                                 current_limit, status);
>                 if (err)
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index ddcdf23d731c..e9e964c20e53 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -182,7 +182,6 @@ struct sd_switch_caps {
>  #define SD_SET_CURRENT_LIMIT_400       1
>  #define SD_SET_CURRENT_LIMIT_600       2
>  #define SD_SET_CURRENT_LIMIT_800       3
> -#define SD_SET_CURRENT_NO_CHANGE       (-1)
>
>  #define SD_MAX_CURRENT_200     (1 << SD_SET_CURRENT_LIMIT_200)
>  #define SD_MAX_CURRENT_400     (1 << SD_SET_CURRENT_LIMIT_400)
> --
> 2.25.1
>

RE: [PATCH 1/2] mmc: core sd: Simplify current limit logic for 200mA default

[Avri Altman]

> I have dropped the stable/fixes tag and clarified the commit message a bit,
> before I applied this for next, thanks!
Thanks a lot.

> 
> Let's try to move forward on patch 2 too. Avri, if you have the time to do a re-
> spin? Otherwise, I will try to get some time to have a stab at it soon.
Please do.
I would be interested in testing it using our SD-Express cards that tend to draw more current.
Can also provide you with a sample of our latest cards if it helps.

Thanks a lot,
Avri

> 
> Kind regards
> Uffe
> 
> > ---
> >  drivers/mmc/core/sd.c    | 7 ++-----
> >  include/linux/mmc/card.h | 1 -
> >  2 files changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c index
> > ec02067f03c5..cf92c5b2059a 100644
> > --- a/drivers/mmc/core/sd.c
> > +++ b/drivers/mmc/core/sd.c
> > @@ -554,7 +554,7 @@ static u32 sd_get_host_max_current(struct
> mmc_host
> > *host)
> >
> >  static int sd_set_current_limit(struct mmc_card *card, u8 *status)  {
> > -       int current_limit = SD_SET_CURRENT_NO_CHANGE;
> > +       int current_limit = SD_SET_CURRENT_LIMIT_200;
> >         int err;
> >         u32 max_current;
> >
> > @@ -598,11 +598,8 @@ static int sd_set_current_limit(struct mmc_card
> *card, u8 *status)
> >         else if (max_current >= 400 &&
> >                  card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_400)
> >                 current_limit = SD_SET_CURRENT_LIMIT_400;
> > -       else if (max_current >= 200 &&
> > -                card->sw_caps.sd3_curr_limit & SD_MAX_CURRENT_200)
> > -               current_limit = SD_SET_CURRENT_LIMIT_200;
> >
> > -       if (current_limit != SD_SET_CURRENT_NO_CHANGE) {
> > +       if (current_limit != SD_SET_CURRENT_LIMIT_200) {
> >                 err = mmc_sd_switch(card, SD_SWITCH_SET, 3,
> >                                 current_limit, status);
> >                 if (err)
> > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h index
> > ddcdf23d731c..e9e964c20e53 100644
> > --- a/include/linux/mmc/card.h
> > +++ b/include/linux/mmc/card.h
> > @@ -182,7 +182,6 @@ struct sd_switch_caps {
> >  #define SD_SET_CURRENT_LIMIT_400       1
> >  #define SD_SET_CURRENT_LIMIT_600       2
> >  #define SD_SET_CURRENT_LIMIT_800       3
> > -#define SD_SET_CURRENT_NO_CHANGE       (-1)
> >
> >  #define SD_MAX_CURRENT_200     (1 << SD_SET_CURRENT_LIMIT_200)
> >  #define SD_MAX_CURRENT_400     (1 << SD_SET_CURRENT_LIMIT_400)
> > --
> > 2.25.1
> >