mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] mmc: core: Don't program invalid host driver type for fixed eMMC type
@ 2026-09-26 12:29 Ian Ray
  2026-09-29 13:06 ` Ulf Hansson
  0 siblings, 1 reply; 3+ messages in thread
From: Ian Ray @ 2026-09-26 12:29 UTC (permalink / raw)
  To: Ulf Hansson, Shawn Lin, Kamal Dasu
  Cc: Ian Ray, stable, linux-mmc, linux-kernel

The host controller driver type value used by the MMC core [1] _mostly_
overlaps with the eMMC device I/O driver strength value [2] as shown in
the table below; however, there is _no_ host mapping for eMMC value 4.

  host value/type   | eMMC value | nominal impedance | strength
  ------------------+------------+-------------------+---------
  0 / Type B        | 0          | 50 Ohm            | x1
  1 / Type A        | 1          | 33 Ohm            | x1.5
  2 / Type C        | 2          | 66 Ohm            | x0.75
  3 / Type D        | 3          | 100 Ohm           | x0.5
  (none)            | 4          | 40 Ohm            | x1.2

[1] MMC_SET_DRIVER_TYPE_* in include/linux/mmc/host.h

[2] 'fixed-emmc-driver-type', see JESD84-B51 Table 206 and
    Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml

Since commit 5a52c5701a67 ("mmc: core: Fix host controller programming
for fixed driver type"), the selected eMMC driver strength is passed
directly to mmc_set_driver_type().  For a fixed eMMC driver type of 4
this reaches sdhci_set_ios(), which only knows types 0..3 and therefore
complains on every ios update:

  mmc2: invalid driver type, default to driver type B

The mmc card itself is still programmed correctly (the HS_TIMING driver
strength field is set from card->drive_strength independently), so this
is a spurious, repeated warning for a valid, documented configuration.

Change to only program the host when the eMMC driver type value maps to
a valid host driver type.

Tested on i.MX8MP (usdhc3, eMMC in HS400) with
`fixed-emmc-driver-type = <4>`: the warning is no longer logged and the
HS_TIMING register contains 0x43 as expected.

Fixes: 5a52c5701a67 ("mmc: core: Fix host controller programming for fixed driver type")
Cc: stable@vger.kernel.org
Signed-off-by: Ian Ray <ian.ray@gehealthcare.com>
---
 drivers/mmc/core/mmc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 05444ecf3909..ff5d4b805998 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1371,9 +1371,11 @@ static void mmc_select_driver_type(struct mmc_card *card)
 
 	card->drive_strength = drive_strength;
 
-	if (fixed_drv_type >= 0 && drive_strength)
-		mmc_set_driver_type(card->host, drive_strength);
-	else if (drv_type)
+	if (fixed_drv_type >= 0 && drive_strength) {
+		/* eMMC driver types >= 4 have no host controller equivalent. */
+		if (drive_strength <= MMC_SET_DRIVER_TYPE_D)
+			mmc_set_driver_type(card->host, drive_strength);
+	} else if (drv_type)
 		mmc_set_driver_type(card->host, drv_type);
 }
 
-- 
2.49.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] mmc: core: Don't program invalid host driver type for fixed eMMC type
  2026-09-26 12:29 [PATCH] mmc: core: Don't program invalid host driver type for fixed eMMC type Ian Ray
@ 2026-09-29 13:06 ` Ulf Hansson
  2026-09-30 19:00   ` Adrian Hunter
  0 siblings, 1 reply; 3+ messages in thread
From: Ulf Hansson @ 2026-09-29 13:06 UTC (permalink / raw)
  To: Ian Ray, Adrian Hunter
  Cc: Ulf Hansson, Shawn Lin, Kamal Dasu, stable, linux-mmc, linux-kernel

+ Adrian

On Sat, Sep 26, 2026 at 2:29 PM Ian Ray <ian.ray@gehealthcare.com> wrote:
>
> The host controller driver type value used by the MMC core [1] _mostly_
> overlaps with the eMMC device I/O driver strength value [2] as shown in
> the table below; however, there is _no_ host mapping for eMMC value 4.

I think the mapping exists only because that is what the SD spec
provided when this was introduced.

We can certainly add something that corresponds to value 4 from the
eMMC spec as well, to make this complete.

>
>   host value/type   | eMMC value | nominal impedance | strength
>   ------------------+------------+-------------------+---------
>   0 / Type B        | 0          | 50 Ohm            | x1
>   1 / Type A        | 1          | 33 Ohm            | x1.5
>   2 / Type C        | 2          | 66 Ohm            | x0.75
>   3 / Type D        | 3          | 100 Ohm           | x0.5
>   (none)            | 4          | 40 Ohm            | x1.2
>
> [1] MMC_SET_DRIVER_TYPE_* in include/linux/mmc/host.h
>
> [2] 'fixed-emmc-driver-type', see JESD84-B51 Table 206 and
>
>
> Since commit 5a52c5701a67 ("mmc: core: Fix host controller programming
> for fixed driver type"), the selected eMMC driver strength is passed
> directly to mmc_set_driver_type().  For a fixed eMMC driver type of 4
> this reaches sdhci_set_ios(), which only knows types 0..3 and therefore
> complains on every ios update:
>
>   mmc2: invalid driver type, default to driver type B

Perhaps the print above from sdhci_set_ios() is simply a bit
misleading. For example, it looks like arasan_select_phy_clock()
actually uses the value in ios.drv_type as is.

To me, it looks like the problem is that mmc_select_drive_strength()
doesn't really do its job correctly, as it should help us to figure
out what is supported by the card *and* by the host, so we can request
a proper driver type when calling the ->set_ios() callback.

>
> The mmc card itself is still programmed correctly (the HS_TIMING driver
> strength field is set from card->drive_strength independently), so this
> is a spurious, repeated warning for a valid, documented configuration.
>
> Change to only program the host when the eMMC driver type value maps to
> a valid host driver type.
>
> Tested on i.MX8MP (usdhc3, eMMC in HS400) with
> `fixed-emmc-driver-type = <4>`: the warning is no longer logged and the
> HS_TIMING register contains 0x43 as expected.
>
> Fixes: 5a52c5701a67 ("mmc: core: Fix host controller programming for fixed driver type")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ian Ray <ian.ray@gehealthcare.com>

I have looped in Adrian, to get his opinion on this.

Kind regards
Uffe

> ---
>  drivers/mmc/core/mmc.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 05444ecf3909..ff5d4b805998 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1371,9 +1371,11 @@ static void mmc_select_driver_type(struct mmc_card *card)
>
>         card->drive_strength = drive_strength;
>
> -       if (fixed_drv_type >= 0 && drive_strength)
> -               mmc_set_driver_type(card->host, drive_strength);
> -       else if (drv_type)
> +       if (fixed_drv_type >= 0 && drive_strength) {
> +               /* eMMC driver types >= 4 have no host controller equivalent. */
> +               if (drive_strength <= MMC_SET_DRIVER_TYPE_D)
> +                       mmc_set_driver_type(card->host, drive_strength);
> +       } else if (drv_type)
>                 mmc_set_driver_type(card->host, drv_type);
>  }
>
> --
> 2.49.0
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] mmc: core: Don't program invalid host driver type for fixed eMMC type
  2026-09-29 13:06 ` Ulf Hansson
@ 2026-09-30 19:00   ` Adrian Hunter
  0 siblings, 0 replies; 3+ messages in thread
From: Adrian Hunter @ 2026-09-30 19:00 UTC (permalink / raw)
  To: Ulf Hansson, Ian Ray
  Cc: Ulf Hansson, Shawn Lin, Kamal Dasu, stable, linux-mmc, linux-kernel

On 29/09/2026 16:06, Ulf Hansson wrote:
> + Adrian
> 
> On Sat, Sep 26, 2026 at 2:29 PM Ian Ray <ian.ray@gehealthcare.com> wrote:
>>
>> The host controller driver type value used by the MMC core [1] _mostly_
>> overlaps with the eMMC device I/O driver strength value [2] as shown in
>> the table below; however, there is _no_ host mapping for eMMC value 4.
> 
> I think the mapping exists only because that is what the SD spec
> provided when this was introduced.
> 
> We can certainly add something that corresponds to value 4 from the
> eMMC spec as well, to make this complete.
> 
>>
>>   host value/type   | eMMC value | nominal impedance | strength
>>   ------------------+------------+-------------------+---------
>>   0 / Type B        | 0          | 50 Ohm            | x1
>>   1 / Type A        | 1          | 33 Ohm            | x1.5
>>   2 / Type C        | 2          | 66 Ohm            | x0.75
>>   3 / Type D        | 3          | 100 Ohm           | x0.5
>>   (none)            | 4          | 40 Ohm            | x1.2
>>
>> [1] MMC_SET_DRIVER_TYPE_* in include/linux/mmc/host.h
>>
>> [2] 'fixed-emmc-driver-type', see JESD84-B51 Table 206 and
>>
>>
>> Since commit 5a52c5701a67 ("mmc: core: Fix host controller programming
>> for fixed driver type"), the selected eMMC driver strength is passed
>> directly to mmc_set_driver_type().  For a fixed eMMC driver type of 4
>> this reaches sdhci_set_ios(), which only knows types 0..3 and therefore
>> complains on every ios update:
>>
>>   mmc2: invalid driver type, default to driver type B
> 
> Perhaps the print above from sdhci_set_ios() is simply a bit
> misleading. For example, it looks like arasan_select_phy_clock()
> actually uses the value in ios.drv_type as is.
> 
> To me, it looks like the problem is that mmc_select_drive_strength()
> doesn't really do its job correctly, as it should help us to figure
> out what is supported by the card *and* by the host, so we can request
> a proper driver type when calling the ->set_ios() callback.

mmc_select_drive_strength() returns the card value directly and
the host value in *drv_type.

fixed-emmc-driver-type as described by
Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
doesn't mention the host controller, and the original commits
6186d06c519e2 "mmc: parse new binding for eMMC fixed driver type"
be17f1ce8572d "mmc: core: properly init drv_type" seemingly
deliberately left it alone.

5a52c5701a67d "mmc: core: Fix host controller programming for
fixed driver type" introduces a new policy: fixed-emmc-driver-type
is a host controller setting also.

If that was done by mmc_select_drive_strength(), it would look
something like below ( note it no longer allows fixed_drv_type to override
->select_drive_strength() , and fixed-emmc-driver-type will then hit
any nonremovable SD or SDIO ):

int mmc_select_drive_strength(struct mmc_card *card, unsigned int max_dtr,
			      int card_drv_type, int *drv_type)
{
	struct mmc_host *host = card->host;
	int fixed_drv_type = host->fixed_drv_type;
	int host_drv_type = SD_DRIVER_TYPE_B;

	*drv_type = 0;

	if (!host->ops->select_drive_strength) {
		if (fixed_drv_type >= 0 &&
		    (card_drv_type & mmc_driver_type_mask(fixed_drv_type))) {
			*drv_type = fixed_drv_type;
			return fixed_drv_type;
		}
		return 0;
	}

	/* Use SD definition of driver strength for hosts */
	if (host->caps & MMC_CAP_DRIVER_TYPE_A)
		host_drv_type |= SD_DRIVER_TYPE_A;

	if (host->caps & MMC_CAP_DRIVER_TYPE_C)
		host_drv_type |= SD_DRIVER_TYPE_C;

	if (host->caps & MMC_CAP_DRIVER_TYPE_D)
		host_drv_type |= SD_DRIVER_TYPE_D;

	/*
	 * The drive strength that the hardware can support
	 * depends on the board design.  Pass the appropriate
	 * information and let the hardware specific code
	 * return what is possible given the options
	 */
	return host->ops->select_drive_strength(card, max_dtr,
						host_drv_type,
						card_drv_type,
						drv_type);
}

and:

static void mmc_select_driver_type(struct mmc_card *card)
{
	int card_drv_type, drive_strength, drv_type = 0;

	card_drv_type = card->ext_csd.raw_driver_strength |
			mmc_driver_type_mask(0);

	drive_strength = mmc_select_drive_strength(card,
						   card->ext_csd.hs200_max_dtr,
						   card_drv_type, &drv_type);

	card->drive_strength = drive_strength;

	if (drv_type)
		mmc_set_driver_type(card->host, drv_type);
}

But that doesn't help with drivers where the controller does
do not support the value passed by fixed-emmc-driver-type even
though the card does.  mmc_select_drive_strength() has no way
of knowing that.  The ->select_drive_strength() callback
allows selecting different values. fixed-emmc-driver-type is
one value.

The warning in sdhci.c seems correct.  SDHCI is an SD standard,
so doesn't define value 4 (and only has 2 bits for the field).
The selected driver strength is not being programmed - deserves
a warning.

Perhaps if the affected driver implements
->select_drive_strength():

static int ???_select_drive_strength(struct mmc_card *card,
				     unsigned int max_dtr, int host_drv,
				     int card_drv, int *host_driver_strength)
{
	struct mmc_host *host = card->host;
	int fixed_drv_type = host->fixed_drv_type;

	if (fixed_drv_type >= 0 &&
	    (card_drv & mmc_driver_type_mask(fixed_drv_type))) {
		if (fixed_drv_type == 4)
			*drv_type = MMC_SET_DRIVER_TYPE_B;
		else
			*drv_type = fixed_drv_type;
		return fixed_drv_type;
	}
	return 0;
}

But that requires the changes to
mmc_select_drive_strength() and mmc_select_driver_type()
above (with its caveats), so that ->select_drive_strength()
overrides fixed-emmc-driver-type.  That would be fine for
drivers/mmc/host/sdhci-pci-core.c, and probably
amd_select_drive_strength(), but it is starting to get
complicated.

A simpler alternative is for the affected driver to hook
->set_ios() and fix up the value.

void ???_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
{
	if (ios->drv_type == 4)
		ios->drv_type = MMC_SET_DRIVER_TYPE_B;

	return sdhci_set_ios(mmc, ios);
}

And in probe (assuming struct sdhci_host *host):

	host->mmc_host_ops.set_ios = ???_set_ios;


> 
>>
>> The mmc card itself is still programmed correctly (the HS_TIMING driver
>> strength field is set from card->drive_strength independently), so this
>> is a spurious, repeated warning for a valid, documented configuration.
>>
>> Change to only program the host when the eMMC driver type value maps to
>> a valid host driver type.
>>
>> Tested on i.MX8MP (usdhc3, eMMC in HS400) with
>> `fixed-emmc-driver-type = <4>`: the warning is no longer logged and the
>> HS_TIMING register contains 0x43 as expected.
>>
>> Fixes: 5a52c5701a67 ("mmc: core: Fix host controller programming for fixed driver type")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Ian Ray <ian.ray@gehealthcare.com>
> 
> I have looped in Adrian, to get his opinion on this.
> 
> Kind regards
> Uffe
> 
>> ---
>>  drivers/mmc/core/mmc.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 05444ecf3909..ff5d4b805998 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1371,9 +1371,11 @@ static void mmc_select_driver_type(struct mmc_card *card)
>>
>>         card->drive_strength = drive_strength;
>>
>> -       if (fixed_drv_type >= 0 && drive_strength)
>> -               mmc_set_driver_type(card->host, drive_strength);
>> -       else if (drv_type)
>> +       if (fixed_drv_type >= 0 && drive_strength) {
>> +               /* eMMC driver types >= 4 have no host controller equivalent. */
>> +               if (drive_strength <= MMC_SET_DRIVER_TYPE_D)
>> +                       mmc_set_driver_type(card->host, drive_strength);
>> +       } else if (drv_type)
>>                 mmc_set_driver_type(card->host, drv_type);
>>  }
>>
>> --
>> 2.49.0
>>


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-09-30 19:00 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-26 12:29 [PATCH] mmc: core: Don't program invalid host driver type for fixed eMMC type Ian Ray
2026-09-29 13:06 ` Ulf Hansson
2026-09-30 19:00   ` Adrian Hunter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®