[PATCH 0/2] mmc: dw_mmc: use standard "mmc" alias stem

[PATCH 0/2] mmc: dw_mmc: use standard "mmc" alias stem

[John Keeping]

dw_mmc uses the device index internally and currently derives this from
a "mshcN" DT alias.  But the standard DT alias used by the MMC core to
assign the device index is "mmcN".

The first patch here creates a function to replace the two sites that do
this mapping so that the second patch can switch to using "mmcN" if
available (falling back to "mshcN" for compatibility if required).

John Keeping (2):
  mmc: dw_mmc: extract ctrl_id lookup to a function
  mmc: dw_mmc: use standard "mmc" alias stem

 drivers/mmc/host/dw_mmc-k3.c |  4 +---
 drivers/mmc/host/dw_mmc.c    | 28 ++++++++++++++++++++++------
 drivers/mmc/host/dw_mmc.h    |  2 ++
 3 files changed, 25 insertions(+), 9 deletions(-)

-- 
2.34.0

[PATCH 1/2] mmc: dw_mmc: extract ctrl_id lookup to a function

[John Keeping]

An upcoming commit will change the logic to use the standard "mmc" stem
for looking up the alias, while keeping the nonstandard "mshc" stem for
backwards compatibility.  In preparation for this, extract a common
function to replace the two places that currently replicate this lookup.

There is no functional change intended here.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/mmc/host/dw_mmc-k3.c |  4 +---
 drivers/mmc/host/dw_mmc.c    | 24 ++++++++++++++++++------
 drivers/mmc/host/dw_mmc.h    |  2 ++
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c
index 0311a37dd4ab..0d82e6f54cca 100644
--- a/drivers/mmc/host/dw_mmc-k3.c
+++ b/drivers/mmc/host/dw_mmc-k3.c
@@ -127,9 +127,7 @@ static int dw_mci_hi6220_parse_dt(struct dw_mci *host)
 	if (IS_ERR(priv->reg))
 		priv->reg = NULL;
 
-	priv->ctrl_id = of_alias_get_id(host->dev->of_node, "mshc");
-	if (priv->ctrl_id < 0)
-		priv->ctrl_id = 0;
+	priv->ctrl_id = dw_mci_of_alias_get_id(host);
 
 	if (priv->ctrl_id >= TIMING_MODE)
 		return -EINVAL;
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 1bedef827138..37af1245304b 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -105,6 +105,21 @@ struct idmac_desc {
 /* Each descriptor can transfer up to 4KB of data in chained mode */
 #define DW_MCI_DESC_DATA_LENGTH	0x1000
 
+int dw_mci_of_alias_get_id(struct dw_mci *host)
+{
+	int ctrl_id;
+
+	if (WARN_ON(!host->dev->of_node))
+		return 0;
+
+	ctrl_id = of_alias_get_id(host->dev->of_node, "mshc");
+
+	if (ctrl_id < 0)
+		ctrl_id = 0;
+
+	return ctrl_id;
+}
+
 #if defined(CONFIG_DEBUG_FS)
 static int dw_mci_req_show(struct seq_file *s, void *v)
 {
@@ -2855,13 +2870,10 @@ static int dw_mci_init_slot_caps(struct dw_mci_slot *slot)
 	if (host->pdata->pm_caps)
 		mmc->pm_caps = host->pdata->pm_caps;
 
-	if (host->dev->of_node) {
-		ctrl_id = of_alias_get_id(host->dev->of_node, "mshc");
-		if (ctrl_id < 0)
-			ctrl_id = 0;
-	} else {
+	if (host->dev->of_node)
+		ctrl_id = dw_mci_of_alias_get_id(host);
+	else
 		ctrl_id = to_platform_device(host->dev)->id;
-	}
 
 	if (drv_data && drv_data->caps) {
 		if (ctrl_id >= drv_data->num_caps) {
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index ce05d81477d9..9e643d7fe90b 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -508,6 +508,8 @@ extern int dw_mci_runtime_suspend(struct device *device);
 extern int dw_mci_runtime_resume(struct device *device);
 #endif
 
+extern int dw_mci_of_alias_get_id(struct dw_mci *host);
+
 /**
  * struct dw_mci_slot - MMC slot state
  * @mmc: The mmc_host representing this slot.
-- 
2.34.0

[PATCH 2/2] mmc: dw_mmc: use standard "mmc" alias stem

[John Keeping]

The standard stem for MMC aliases is "mmc" and this is used by the MMC
core to set the slot index.

Use this in preference to the non-standard "mshc" stem when setting the
controller ID to avoid needing two aliases for each MMC device in order
to cover both the core and dw_mmc-specific functionality.

The old "mshc" lookup is kept for backwards compatibility.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/mmc/host/dw_mmc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 37af1245304b..6ffaa354410a 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -112,7 +112,11 @@ int dw_mci_of_alias_get_id(struct dw_mci *host)
 	if (WARN_ON(!host->dev->of_node))
 		return 0;
 
-	ctrl_id = of_alias_get_id(host->dev->of_node, "mshc");
+	ctrl_id = of_alias_get_id(host->dev->of_node, "mmc");
+
+	/* Compatibility fallback for old device trees. */
+	if (ctrl_id < 0)
+		ctrl_id = of_alias_get_id(host->dev->of_node, "mshc");
 
 	if (ctrl_id < 0)
 		ctrl_id = 0;
-- 
2.34.0

Re: [PATCH 2/2] mmc: dw_mmc: use standard "mmc" alias stem

[Ulf Hansson]

On Tue, 16 Nov 2021 at 20:02, John Keeping <john@metanate.com> wrote:
>
> The standard stem for MMC aliases is "mmc" and this is used by the MMC
> core to set the slot index.

This isn't the correct description of the mmc aliases. The below text
is copied from the DT doc:

"It is possible to assign a fixed index mmcN to an MMC host controller
(and the corresponding mmcblkN devices) by defining an alias in the
/aliases device tree node."

>
> Use this in preference to the non-standard "mshc" stem when setting the
> controller ID to avoid needing two aliases for each MMC device in order
> to cover both the core and dw_mmc-specific functionality.
>
> The old "mshc" lookup is kept for backwards compatibility.

The mshc alias is really weird!

It looks like some leftover from when the dw_mmc controller supported
multiple slots. This support was dropped a long time ago, simply
because it never really worked - and it was not worth trying to. Only
one slot per controller is supported.

Rather than re-using the mmc alias in the same weird way as the mshc
alias, I suggest we try to remove parsing of the mshc aliases
completely. By looking at the corresponding code and in combination
with the DTS files, it certainly looks doable to me. Do you want to
have a look at it?

Additionally, there is no need to deprecate the mshc alias binding, as
it seems like it has never been documented. :-)

Kind regards
Uffe

>
> Signed-off-by: John Keeping <john@metanate.com>
> ---
>  drivers/mmc/host/dw_mmc.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 37af1245304b..6ffaa354410a 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -112,7 +112,11 @@ int dw_mci_of_alias_get_id(struct dw_mci *host)
>         if (WARN_ON(!host->dev->of_node))
>                 return 0;
>
> -       ctrl_id = of_alias_get_id(host->dev->of_node, "mshc");
> +       ctrl_id = of_alias_get_id(host->dev->of_node, "mmc");
> +
> +       /* Compatibility fallback for old device trees. */
> +       if (ctrl_id < 0)
> +               ctrl_id = of_alias_get_id(host->dev->of_node, "mshc");
>
>         if (ctrl_id < 0)
>                 ctrl_id = 0;
> --
> 2.34.0
>

Re: [PATCH 2/2] mmc: dw_mmc: use standard "mmc" alias stem

[Jaehoon Chung]

On 11/24/21 4:34 AM, Ulf Hansson wrote:
> On Tue, 16 Nov 2021 at 20:02, John Keeping <john@metanate.com> wrote:
>>
>> The standard stem for MMC aliases is "mmc" and this is used by the MMC
>> core to set the slot index.
> 
> This isn't the correct description of the mmc aliases. The below text
> is copied from the DT doc:
> 
> "It is possible to assign a fixed index mmcN to an MMC host controller
> (and the corresponding mmcblkN devices) by defining an alias in the
> /aliases device tree node."
> 
>>
>> Use this in preference to the non-standard "mshc" stem when setting the
>> controller ID to avoid needing two aliases for each MMC device in order
>> to cover both the core and dw_mmc-specific functionality.
>>
>> The old "mshc" lookup is kept for backwards compatibility.
> 
> The mshc alias is really weird!
> 
> It looks like some leftover from when the dw_mmc controller supported
> multiple slots. This support was dropped a long time ago, simply
> because it never really worked - and it was not worth trying to. Only
> one slot per controller is supported.

As Ulf mentioned, dw_mmc controller can be supported multiple slot.
But I didn't see its case to use multiple slot. And I had been done to drop a long time ago.

mshc was used because of Mobile Storage Host Controller.

> 
> Rather than re-using the mmc alias in the same weird way as the mshc
> alias, I suggest we try to remove parsing of the mshc aliases
> completely. By looking at the corresponding code and in combination
> with the DTS files, it certainly looks doable to me. Do you want to
> have a look at it?

If possible to remove mshc, it's best.
I will check that removing mshc parsing in dw_mmc.c.

Best Regards,
Jaehoon Chung

> 
> Additionally, there is no need to deprecate the mshc alias binding, as
> it seems like it has never been documented. :-)
> 
> Kind regards
> Uffe
> 
>>
>> Signed-off-by: John Keeping <john@metanate.com>
>> ---
>>  drivers/mmc/host/dw_mmc.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 37af1245304b..6ffaa354410a 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -112,7 +112,11 @@ int dw_mci_of_alias_get_id(struct dw_mci *host)
>>         if (WARN_ON(!host->dev->of_node))
>>                 return 0;
>>
>> -       ctrl_id = of_alias_get_id(host->dev->of_node, "mshc");
>> +       ctrl_id = of_alias_get_id(host->dev->of_node, "mmc");
>> +
>> +       /* Compatibility fallback for old device trees. */
>> +       if (ctrl_id < 0)
>> +               ctrl_id = of_alias_get_id(host->dev->of_node, "mshc");
>>
>>         if (ctrl_id < 0)
>>                 ctrl_id = 0;
>> --
>> 2.34.0
>>
> 

Re: [PATCH 2/2] mmc: dw_mmc: use standard "mmc" alias stem

[John Keeping]

On Wed, Nov 24, 2021 at 06:54:12PM +0900, Jaehoon Chung wrote:
> On 11/24/21 4:34 AM, Ulf Hansson wrote:
> > On Tue, 16 Nov 2021 at 20:02, John Keeping <john@metanate.com> wrote:
> >>
> >> The standard stem for MMC aliases is "mmc" and this is used by the MMC
> >> core to set the slot index.
> > 
> > This isn't the correct description of the mmc aliases. The below text
> > is copied from the DT doc:
> > 
> > "It is possible to assign a fixed index mmcN to an MMC host controller
> > (and the corresponding mmcblkN devices) by defining an alias in the
> > /aliases device tree node."
> > 
> >>
> >> Use this in preference to the non-standard "mshc" stem when setting the
> >> controller ID to avoid needing two aliases for each MMC device in order
> >> to cover both the core and dw_mmc-specific functionality.
> >>
> >> The old "mshc" lookup is kept for backwards compatibility.
> > 
> > The mshc alias is really weird!
> > 
> > It looks like some leftover from when the dw_mmc controller supported
> > multiple slots. This support was dropped a long time ago, simply
> > because it never really worked - and it was not worth trying to. Only
> > one slot per controller is supported.
> 
> As Ulf mentioned, dw_mmc controller can be supported multiple slot.
> But I didn't see its case to use multiple slot. And I had been done to drop a long time ago.
> 
> mshc was used because of Mobile Storage Host Controller.
> 
> > 
> > Rather than re-using the mmc alias in the same weird way as the mshc
> > alias, I suggest we try to remove parsing of the mshc aliases
> > completely. By looking at the corresponding code and in combination
> > with the DTS files, it certainly looks doable to me. Do you want to
> > have a look at it?
> 
> If possible to remove mshc, it's best.
> I will check that removing mshc parsing in dw_mmc.c.

Unfortunately it doesn't look like it's easy to remove as there is some
behaviour depending on this via dw_mci_drv_data::caps, as well as
different timing setup in dw_mmc-k3.c which uses
dw_mci_of_alias_get_id() to identify SD and SDIO hosts.

Looking across the dw_mmc-*.c files that use dw_mci_drv_data::caps to
set capabilities per host controller:

- dw_mmc-exynos.c sets additional capabilities for mshc0, although both
  MMC_CAP_1_8V_DDR and MMC_CAP_8_BIT_DATA should be set via DT (in fact
  in some cases it looks like device trees are setting bus-width = <4>
  so MMC_CAP_8_BIT_DATA seems wrong!); I can't see any device trees
  setting mmc-ddr-1_8v for these devices at the moment though, so
  removing that is a change in behaviour

- dw_mmc-k3.c sets different capabilities for mshc2 and, as mentioned
  above, uses the alias index to select timing parameters and change
  voltage switching behaviour

- dw_mmc-hi3798cv200.c and dw_mmc-rockchip.c set the same caps for all
  slots, so can easily remove the dependency on the alias


I'm mostly interested in Rockchip myself, which is one of the easy ones,
so I'm not that familiar with Exynos or K3 - I'd guess the Exynos
version can remove its dependency on the mshc alias pretty easily, but
the use in dw_mmc-k3.c looks much more difficult given that I can't see
any other way to derive the necessary info from the current device
trees.


Regards,
John

Re: [PATCH 2/2] mmc: dw_mmc: use standard "mmc" alias stem

[Ulf Hansson]

On Wed, 24 Nov 2021 at 13:29, John Keeping <john@metanate.com> wrote:
>
> On Wed, Nov 24, 2021 at 06:54:12PM +0900, Jaehoon Chung wrote:
> > On 11/24/21 4:34 AM, Ulf Hansson wrote:
> > > On Tue, 16 Nov 2021 at 20:02, John Keeping <john@metanate.com> wrote:
> > >>
> > >> The standard stem for MMC aliases is "mmc" and this is used by the MMC
> > >> core to set the slot index.
> > >
> > > This isn't the correct description of the mmc aliases. The below text
> > > is copied from the DT doc:
> > >
> > > "It is possible to assign a fixed index mmcN to an MMC host controller
> > > (and the corresponding mmcblkN devices) by defining an alias in the
> > > /aliases device tree node."
> > >
> > >>
> > >> Use this in preference to the non-standard "mshc" stem when setting the
> > >> controller ID to avoid needing two aliases for each MMC device in order
> > >> to cover both the core and dw_mmc-specific functionality.
> > >>
> > >> The old "mshc" lookup is kept for backwards compatibility.
> > >
> > > The mshc alias is really weird!
> > >
> > > It looks like some leftover from when the dw_mmc controller supported
> > > multiple slots. This support was dropped a long time ago, simply
> > > because it never really worked - and it was not worth trying to. Only
> > > one slot per controller is supported.
> >
> > As Ulf mentioned, dw_mmc controller can be supported multiple slot.
> > But I didn't see its case to use multiple slot. And I had been done to drop a long time ago.
> >
> > mshc was used because of Mobile Storage Host Controller.
> >
> > >
> > > Rather than re-using the mmc alias in the same weird way as the mshc
> > > alias, I suggest we try to remove parsing of the mshc aliases
> > > completely. By looking at the corresponding code and in combination
> > > with the DTS files, it certainly looks doable to me. Do you want to
> > > have a look at it?
> >
> > If possible to remove mshc, it's best.
> > I will check that removing mshc parsing in dw_mmc.c.
>
> Unfortunately it doesn't look like it's easy to remove as there is some
> behaviour depending on this via dw_mci_drv_data::caps, as well as
> different timing setup in dw_mmc-k3.c which uses
> dw_mci_of_alias_get_id() to identify SD and SDIO hosts.
>
> Looking across the dw_mmc-*.c files that use dw_mci_drv_data::caps to
> set capabilities per host controller:
>
> - dw_mmc-exynos.c sets additional capabilities for mshc0, although both
>   MMC_CAP_1_8V_DDR and MMC_CAP_8_BIT_DATA should be set via DT (in fact
>   in some cases it looks like device trees are setting bus-width = <4>
>   so MMC_CAP_8_BIT_DATA seems wrong!); I can't see any device trees
>   setting mmc-ddr-1_8v for these devices at the moment though, so
>   removing that is a change in behaviour

Of course we need to be careful to not break anything. But having some
clever fallback methods could work.

For example, for an SD/SDIO card slot it shouldn't matter if
MMC_CAP_8_BIT_DATA gets set. But I didn't look that closely if that
could be an option.

>
> - dw_mmc-k3.c sets different capabilities for mshc2 and, as mentioned
>   above, uses the alias index to select timing parameters and change
>   voltage switching behaviour

Yeah, the timing thingy looks harder to get rid of for the k3 variant.

Although, if we could limit the alias parsing to the k3 variant, that
would still be a nice cleanup and improvement.

As a next step we could look at introducing some new DT properties for
k3, to specify the timing thingy - and then make the mshc alias
deprecated.

>
> - dw_mmc-hi3798cv200.c and dw_mmc-rockchip.c set the same caps for all
>   slots, so can easily remove the dependency on the alias

Great.

>
>
> I'm mostly interested in Rockchip myself, which is one of the easy ones,
> so I'm not that familiar with Exynos or K3 - I'd guess the Exynos
> version can remove its dependency on the mshc alias pretty easily, but
> the use in dw_mmc-k3.c looks much more difficult given that I can't see
> any other way to derive the necessary info from the current device
> trees.

Right. I will try to get some time to help clean this up.

>
>
> Regards,
> John

Kind regards
Uffe

Re: [PATCH 2/2] mmc: dw_mmc: use standard "mmc" alias stem

[Christian Löhle]

Sorry for hijacking this thread.


From: Jaehoon Chung <jh80.chung@samsung.com>
Sent: Wednesday, November 24, 2021 10:54 AM
To: Ulf Hansson; John Keeping
Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] mmc: dw_mmc: use standard "mmc" alias stem

>>> The old "mshc" lookup is kept for backwards compatibility.
>> 
>> The mshc alias is really weird!
>> 
>> It looks like some leftover from when the dw_mmc controller supported
>> multiple slots. This support was dropped a long time ago, simply
>> because it never really worked - and it was not worth trying to. Only
>> one slot per controller is supported.
>
>As Ulf mentioned, dw_mmc controller can be supported multiple slot.
>But I didn't see its case to use multiple slot. And I had been done to drop a long time ago.
>
>mshc was used because of Mobile Storage Host Controller.

I assume this means you are open to a rework for dw_mmc?
I have a bigger patch for dw_mmc that I work on myself.
I removed the slot functionality at the start because there is no way for me to verify that I did not break this feature in the meantime.
But if it never worked and there is no platform for it, then removing this is fine for everyone?
Otherwise I probably would submit my rework for staging.

Regards,
Christian=
Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782