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
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
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, andDDR50 @@ -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. --rmkI 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
[...]
/* * Current limit switch is only defined for SDR50, SDR104, andDDR50 @@ -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. --rmkI 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
/* * 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. --rmkI 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
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
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
linux-stable-mirror@lists.linaro.org
-
Avri Altman -
Avri Altman
-
Ulf Hansson