Re: [PATCH] mmc: core: Don't program invalid host driver type for fixed eMMC type

[Adrian Hunter <adrian.hunter@intel.com>]

On 01/10/2026 18:39, Ian Ray wrote:
> On Wed, Sep 30, 2026 at 10:00:47PM +0300, Adrian Hunter wrote:
>> 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;
>>
> 
> Thank you for the feedback.
> 
> The "invalid driver type" warning that I originally observed originates
> in sdhci_set_ios_common() which is shared by all SDHCI hosts (IIUC).
> 
> Thus any SDHCI host configured with `fixed-emmc-driver-type = <4>` would
> log that warning.

The warning is valid.  SDHCI does not support value 4 and doesn't know
what is a valid alternative or even if there is one.  That is up to the
driver to sort out.  If other drivers do it the same way, then a
helper or something could be added.

> 
> The affected driver for my scenario is sdhci-esdhc-imx.c, and the
> proposed change (below) suppresses the warning.
> 
> But I am hesitant to open-code the same fixup in every SDHCI driver.
> And I am unsure whether this belongs per-driver, or whether the right
> fix is in the core / or to the warning itself.
> 
> Comments welcome!
> 
> drivers/mmc/host/sdhci-esdhc-imx.c
> 
> ```
> static void sdhci_esdhc_imx_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> {
>        /*
>         * A drv_type of 4 reaches this callback only via the eMMC
>         * 'fixed-emmc-driver-type' path: it is the eMMC I/O driver strength
>         * value 4 (40 Ohm, see JESD84-B51 Table 206), which has no equivalent
>         * host driver type in the SD standard that SDHCI implements (only
>         * types B/A/C/D are defined).  Map it to the default Type B here to
>         * avoid an "invalid driver type" warning from sdhci_set_ios().  The

The objective is not to avoid the warning.  The warning is valid.  The
objective is to provide a valid value.  4 is not.

If the actual value doesn't matter here at all, then say so and set
it to MMC_SET_DRIVER_TYPE_B always.

>         * card's HS_TIMING field is programmed separately from
>         * card->drive_strength and is unaffected.
>         */
>        if (ios->drv_type == 4)
>                ios->drv_type = MMC_SET_DRIVER_TYPE_B;
> 
>        sdhci_set_ios(mmc, ios);
> }
> 
> static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
> {
> ...
>                /*
>                 * Support fix up of eMMC driver types which have no SDHCI
>                 * host driver type equivalent.
>                 */
>                host->mmc_host_ops.set_ios = sdhci_esdhc_imx_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
>>>>
>>