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
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 <[email protected]>
---
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
On Tue, 16 Nov 2021 at 20:02, John Keeping <[email protected]> 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 <[email protected]>
> ---
> 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
>
On 11/24/21 4:34 AM, Ulf Hansson wrote:
> On Tue, 16 Nov 2021 at 20:02, John Keeping <[email protected]> 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 <[email protected]>
>> ---
>> 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
>>
>
Sorry for hijacking this thread.
From: Jaehoon Chung <[email protected]>
Sent: Wednesday, November 24, 2021 10:54 AM
To: Ulf Hansson; John Keeping
Cc: [email protected]; [email protected]
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
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 <[email protected]> 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
On Wed, 24 Nov 2021 at 13:29, John Keeping <[email protected]> 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 <[email protected]> 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