[PATCH v2] mmc: core: add mmc-card hardware reset enable support

[PATCH v2] mmc: core: add mmc-card hardware reset enable support

[Richard Leitner]

Some eMMCs disable their hardware reset line (RST_N) by default. To enable
it the host must set the corresponding bit in ECSD. An example for such
a device is the Micron MTFCxGACAANA-4M.

This patch adds a new mmc-card devicetree property to let the host enable
this feature during card initialization.

Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
---
CHANGES v2:
	- add RST_N_FUNCTION value to dt documentation
	- set RST_N_FUNCTION only if it was not set before
---
 Documentation/devicetree/bindings/mmc/mmc-card.txt |  4 ++++
 drivers/mmc/core/mmc.c                             | 23 ++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-card.txt b/Documentation/devicetree/bindings/mmc/mmc-card.txt
index a70fcd6..bbfccce 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-card.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc-card.txt
@@ -12,6 +12,10 @@ Required properties:
 Optional properties:
 -broken-hpi : Use this to indicate that the mmc-card has a broken hpi
               implementation, and that hpi should not be used
+-enable-hw-reset : some eMMC devices have disabled the hw reset functionality
+                   (RST_N_FUNCTION) by default. By adding this property the
+                   host will enable it during initialization. This will set
+                   RST_N_FUNCTION to 0x1 (permanently enabled).
 
 Example:
 
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index b502601..30066be 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1520,9 +1520,16 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
 	int err;
 	u32 cid[4];
 	u32 rocr;
+	struct device_node *np;
+	bool enable_rst_n = false;
 
 	WARN_ON(!host->claimed);
 
+	np = mmc_of_find_child_device(host, 0);
+	if (np && of_device_is_compatible(np, "mmc-card"))
+		enable_rst_n = of_property_read_bool(np, "enable-hw-reset");
+	of_node_put(np);
+
 	/* Set correct bus mode for MMC before attempting init */
 	if (!mmc_host_is_spi(host))
 		mmc_set_bus_mode(host, MMC_BUSMODE_OPENDRAIN);
@@ -1810,6 +1817,22 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
 		}
 	}
 
+	/*
+	 * enable RST_N if requested (and rst_n_function is not set)
+	 * This is needed because some eMMC chips disable this function by
+	 * default.
+	 */
+	if (enable_rst_n &&
+	    !(card->ext_csd.rst_n_function & EXT_CSD_RST_N_EN_MASK)) {
+		err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+				 EXT_CSD_RST_N_FUNCTION, EXT_CSD_RST_N_ENABLED,
+				 card->ext_csd.generic_cmd6_time);
+		card->ext_csd.rst_n_function = EXT_CSD_RST_N_ENABLED;
+		if (err && err != -EBADMSG)
+			pr_warn("%s: Enabling RST_N feature failed\n",
+				mmc_hostname(card->host));
+	}
+
 	if (!oldcard)
 		host->card = card;
 
-- 
2.1.4

Re: [PATCH v2] mmc: core: add mmc-card hardware reset enable support

[Linus Walleij]

On Tue, Apr 11, 2017 at 9:31 AM, Richard Leitner
<richard.leitner@skidata.com> wrote:

> Some eMMCs disable their hardware reset line (RST_N) by default. To enable
> it the host must set the corresponding bit in ECSD. An example for such
> a device is the Micron MTFCxGACAANA-4M.
>
> This patch adds a new mmc-card devicetree property to let the host enable
> this feature during card initialization.
>
> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>

Do we know *WHY* these cards disable their hardware reset lines?

If it is just some random over-cautious panic thing we might consider
just force re-enableing it, maybe with a warning in the dmesg, so we can
always reset the card. No DT property needed.

Putting some people who work for eMMC vendors in the To: line so they
can say if they know about this.

Yours,
Linus Walleij

Re: [PATCH v2] mmc: core: add mmc-card hardware reset enable support

[Ulf Hansson]

On 11 April 2017 at 10:17, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Apr 11, 2017 at 9:31 AM, Richard Leitner
> <richard.leitner@skidata.com> wrote:
>
>> Some eMMCs disable their hardware reset line (RST_N) by default. To enable
>> it the host must set the corresponding bit in ECSD. An example for such
>> a device is the Micron MTFCxGACAANA-4M.
>>
>> This patch adds a new mmc-card devicetree property to let the host enable
>> this feature during card initialization.
>>
>> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
>
> Do we know *WHY* these cards disable their hardware reset lines?

Allow me to make a guess. In case the reset isn't enabled, the
internal eMMC card firmware don't monitor the pin for the reset. I
guess that could makes sense if SoC vendors has failed to properly
connect the pin, avoiding the eMMC card to be reset when it shouldn't.

>
> If it is just some random over-cautious panic thing we might consider
> just force re-enableing it, maybe with a warning in the dmesg, so we can
> always reset the card. No DT property needed.

There is actually already a DT property "cap-mmc-hw-reset"
(MMC_CAP_HW_RESET), which tells whether the eMMC reset is supported by
the host.

Perhaps we can consider to force-enabling it for the eMMC card, when
this property is set for the mmc host!? At least, inventing yet
another binding doesn't make sense to me.

>
> Putting some people who work for eMMC vendors in the To: line so they
> can say if they know about this.

Yes, let's see what they say about it.

>
> Yours,
> Linus Walleij

Kind regards
Uffe

Re: [PATCH v2] mmc: core: add mmc-card hardware reset enable support

[Richard Leitner]

On 04/11/2017 12:43 PM, Ulf Hansson wrote:
> On 11 April 2017 at 10:17, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Tue, Apr 11, 2017 at 9:31 AM, Richard Leitner
>> <richard.leitner@skidata.com> wrote:
>>
>>> Some eMMCs disable their hardware reset line (RST_N) by default. To enable
>>> it the host must set the corresponding bit in ECSD. An example for such
>>> a device is the Micron MTFCxGACAANA-4M.
>>>
>>> This patch adds a new mmc-card devicetree property to let the host enable
>>> this feature during card initialization.
>>>
>>> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
>>
>> Do we know *WHY* these cards disable their hardware reset lines?
> 
> Allow me to make a guess. In case the reset isn't enabled, the
> internal eMMC card firmware don't monitor the pin for the reset. I
> guess that could makes sense if SoC vendors has failed to properly
> connect the pin, avoiding the eMMC card to be reset when it shouldn't.
> 
>> If it is just some random over-cautious panic thing we might consider
>> just force re-enableing it, maybe with a warning in the dmesg, so we can
>> always reset the card. No DT property needed.
> 
> There is actually already a DT property "cap-mmc-hw-reset"
> (MMC_CAP_HW_RESET), which tells whether the eMMC reset is supported by
> the host.
> 
> Perhaps we can consider to force-enabling it for the eMMC card, when
> this property is set for the mmc host!? At least, inventing yet
> another binding doesn't make sense to me.

IMHO setting this (permanent) configuration bit should be done
explicitly and not within another DT property. Otherwise somebody may
accidentally set this configuration by (for example) including a dtsi
which has cap-mmc-hw-reset configured.

Nonetheless if the majority agrees to implicitly set it when
"cap-mmc-hw-reset" is configured I'm fine with that too.

Another possibility is to don't include it in the kernel at all and
therefore require the mmc userspace program to configure it...?

> 
>> Putting some people who work for eMMC vendors in the To: line so they
>> can say if they know about this.
> 
> Yes, let's see what they say about it.

Any news/comments?

kind regards,
Richard.L

RE: [EXT] Re: [PATCH v2] mmc: core: add mmc-card hardware reset enable support

[Luca Porzio (lporzio)]

Hi,

> -----Original Message-----
> From: Richard Leitner [mailto:richard.leitner@skidata.com]
> Sent: Wednesday, June 14, 2017 9:25 AM
> To: Ulf Hansson <ulf.hansson@linaro.org>; Linus Walleij
> <linus.walleij@linaro.org>
> Cc: Bart Van Assche <bart.vanassche@sandisk.com>; Luca Porzio (lporzio)
> <lporzio@micron.com>; Rob Herring <robh+dt@kernel.org>; Mark Rutland
> <mark.rutland@arm.com>; Shawn Lin <shawn.lin@rock-chips.com>; Adrian
> Hunter <adrian.hunter@intel.com>; Jaehoon Chung
> <jh80.chung@samsung.com>; linux-mmc@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; Richard Leitner
> <dev@g0hl1n.net>
> Subject: [EXT] Re: [PATCH v2] mmc: core: add mmc-card hardware reset
> enable support
> 
> On 04/11/2017 12:43 PM, Ulf Hansson wrote:
> > On 11 April 2017 at 10:17, Linus Walleij <linus.walleij@linaro.org> wrote:
> >> On Tue, Apr 11, 2017 at 9:31 AM, Richard Leitner
> >> <richard.leitner@skidata.com> wrote:
> >>
> >>> Some eMMCs disable their hardware reset line (RST_N) by default. To
> >>> enable it the host must set the corresponding bit in ECSD. An
> >>> example for such a device is the Micron MTFCxGACAANA-4M.
> >>>

This behavior is not Micron specific but instead it is enforced by the Jedec 
Specification. All eMMC must have HW reset disabled by default.

> >>> This patch adds a new mmc-card devicetree property to let the host
> >>> enable this feature during card initialization.
> >>>
> >>> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
> >>
> >> Do we know *WHY* these cards disable their hardware reset lines?
> >
> > Allow me to make a guess. In case the reset isn't enabled, the
> > internal eMMC card firmware don't monitor the pin for the reset. I
> > guess that could makes sense if SoC vendors has failed to properly
> > connect the pin, avoiding the eMMC card to be reset when it shouldn't.
> >
> >> If it is just some random over-cautious panic thing we might consider
> >> just force re-enableing it, maybe with a warning in the dmesg, so we
> >> can always reset the card. No DT property needed.
> >

This specification (as Ulf correctly hinted) originated from badly connected 
HW Reset pins which caused unnecessary eMMC reset glitches.
Disabling this feature is a good option to contain glitches and avoid 
system level bugs.

> > There is actually already a DT property "cap-mmc-hw-reset"
> > (MMC_CAP_HW_RESET), which tells whether the eMMC reset is
> supported by
> > the host.
> >
> > Perhaps we can consider to force-enabling it for the eMMC card, when
> > this property is set for the mmc host!? At least, inventing yet
> > another binding doesn't make sense to me.
> 
> IMHO setting this (permanent) configuration bit should be done explicitly
> and not within another DT property. Otherwise somebody may accidentally
> set this configuration by (for example) including a dtsi which has cap-mmc-
> hw-reset configured.
> 
> Nonetheless if the majority agrees to implicitly set it when "cap-mmc-hw-
> reset" is configured I'm fine with that too.
> 
> Another possibility is to don't include it in the kernel at all and therefore
> require the mmc userspace program to configure it...?
> 
> >
> >> Putting some people who work for eMMC vendors in the To: line so they
> >> can say if they know about this.
> >
> > Yes, let's see what they say about it.

Here I am :-)

> 
> Any news/comments?
> 

IMHO mmc-util is where the patch really stands: the enabling is OTP, 
the programmer have to use mmc-util only once and the kernel 
will behave accordingly. 

Any platform vendor must check that the HW Reset pin is actually
Connected BEFORE enabling this feature otherwise the system may be
unstable. A DT Binding may be dangerous if this condition is not met 
as well as the code execution at each MMC init is honestly redundant
for an OTP location of the extCSD.

Cheers,
   Luca

> kind regards,
> Richard.L

Re: [EXT] Re: [PATCH v2] mmc: core: add mmc-card hardware reset enable support

[Linus Walleij]

Hi Luca,

thanks for joining!

On Wed, Jun 14, 2017 at 6:12 PM, Luca Porzio (lporzio)
<lporzio@micron.com> wrote:

> This behavior is not Micron specific but instead it is enforced by the Jedec
> Specification. All eMMC must have HW reset disabled by default.

That's very good to know.

> This specification (as Ulf correctly hinted) originated from badly connected
> HW Reset pins which caused unnecessary eMMC reset glitches.
> Disabling this feature is a good option to contain glitches and avoid
> system level bugs.

Ah, makes sense.

> IMHO mmc-util is where the patch really stands: the enabling is OTP,
> the programmer have to use mmc-util only once and the kernel
> will behave accordingly.

So we should not add it to the device tree.

> Any platform vendor must check that the HW Reset pin is actually
> Connected BEFORE enabling this feature otherwise the system may be
> unstable. A DT Binding may be dangerous if this condition is not met
> as well as the code execution at each MMC init is honestly redundant
> for an OTP location of the extCSD.

I agree. So the device should be configured during production, or
a user who know exactly what they are doing may reconfigure it
using the mmc-utils.

Thus the kernel should just read what the device says and stay with
that, no DT props or anything.

This makes a lot of sense.

When say ethernet devices need MAC addresses written to their OTP
we do not put that into the device tree to be programmed either, we assume
production tools to do that job.

Yours,
Linus Walleij

Re: [EXT] Re: [PATCH v2] mmc: core: add mmc-card hardware reset enable support

[Richard Leitner]

On 06/20/2017 11:45 AM, Linus Walleij wrote:
>> IMHO mmc-util is where the patch really stands: the enabling is OTP,
>> the programmer have to use mmc-util only once and the kernel
>> will behave accordingly.
> 
> So we should not add it to the device tree.
> 
>> Any platform vendor must check that the HW Reset pin is actually
>> Connected BEFORE enabling this feature otherwise the system may be
>> unstable. A DT Binding may be dangerous if this condition is not met
>> as well as the code execution at each MMC init is honestly redundant
>> for an OTP location of the extCSD.
> 
> I agree. So the device should be configured during production, or
> a user who know exactly what they are doing may reconfigure it
> using the mmc-utils.
> 
> Thus the kernel should just read what the device says and stay with
> that, no DT props or anything.

Ok. Thanks for that clarification to everybody involved!

Should we then add a warning/info message to the kernel if we have a
reset gpio configured in the DT, but the eMMC hasn't this OTP bit set?

Or should we just leave it as it is? (Due to the fact the reset-gpio
could be connected to an external reset chip?)

kind regards,
Richard.L

Re: [EXT] Re: [PATCH v2] mmc: core: add mmc-card hardware reset enable support

[Linus Walleij]

On Tue, Jun 20, 2017 at 12:24 PM, Richard Leitner
<richard.leitner@skidata.com> wrote:

> Should we then add a warning/info message to the kernel if we have a
> reset gpio configured in the DT, but the eMMC hasn't this OTP bit set?

That makes a lot of sense. Because the situation is ambigous.
We provide the hardware to reset the card, but the card says it
does not accept a reset.

Yours,
Linus Walleij