[PATCH 1/3] dt-bindings: mmc: Add 'fixed-emmc-driver-type-hs{200,400}'

[PATCH 1/3] dt-bindings: mmc: Add 'fixed-emmc-driver-type-hs{200,400}'

[Eugeniu Rosca]

A certain eMMC manufacturer provided below requirement:
 ---snip---
 Use "drive strength" value of 4 or 1 for HS400 or 0 for HS200.
 ---snip---

The existing "fixed-emmc-driver-type" property [1] is the closest one
to implement the above, but it falls short due to being unable to define
two values to differentiate between HS200 and HS400 (both modes may be
supported by the same non-removable MMC device).

To allow users to set a preferred HS200/HS400 "drive strength", provide
two more bindings inspired from [1]:
 - fixed-emmc-driver-type-hs200
 - fixed-emmc-driver-type-hs400

For more details about eMMC I/O driver strength types, see Jedec spec.
Keep "fixed-emmc-driver-type" in place for backward compatibility.

[1] commit 6186d06c519e21 ("mmc: parse new binding for eMMC fixed driver type")

Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 .../bindings/mmc/mmc-controller.yaml          | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
index 080754e0ef35..1c64b14f91a3 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
+++ b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
@@ -248,6 +248,24 @@ properties:
       the driver type as specified in the eMMC specification (table
       206 in spec version 5.1)
 
+  fixed-emmc-driver-type-hs200:
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - minimum: 0
+      - maximum: 4
+    description:
+      Same as "fixed-emmc-driver-type", but specific to HS200 mode.
+      If defined, overrides "fixed-emmc-driver-type" in HS200 mode.
+
+  fixed-emmc-driver-type-hs400:
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - minimum: 0
+      - maximum: 4
+    description:
+      Same as "fixed-emmc-driver-type", but specific to HS400 mode.
+      If defined, overrides "fixed-emmc-driver-type" in HS400 mode.
+
   post-power-on-delay-ms:
     allOf:
       - $ref: /schemas/types.yaml#/definitions/uint32
@@ -336,6 +354,8 @@ patternProperties:
 dependencies:
   cd-debounce-delay-ms: [ cd-gpios ]
   fixed-emmc-driver-type: [ non-removable ]
+  fixed-emmc-driver-type-hs200: [ non-removable ]
+  fixed-emmc-driver-type-hs400: [ non-removable ]
 
 examples:
   - |
-- 
2.23.0

[PATCH 2/3] mmc: host: Compress 'fixed-emmc-driver-type' handling

[Eugeniu Rosca]

Prepare for reading two additional OF properties (inspired from
"fixed-emmc-driver-type"):
 - fixed-emmc-driver-type-hs200
 - fixed-emmc-driver-type-hs400

The parsing mechanism is common too all three, thus factored out.
The only functional change is a tiny update in the error message.

Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 drivers/mmc/core/host.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 105b7a7c0251..54abfdfc69ba 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -161,6 +161,20 @@ static void mmc_retune_timer(struct timer_list *t)
 	mmc_retune_needed(host);
 }
 
+static void mmc_of_read_drv_type(struct mmc_host *host, char *prop, u32 *val)
+{
+	struct device *dev = host->parent;
+	u32 drv_type;
+
+	if (device_property_read_u32(dev, prop, &drv_type))
+		return;
+
+	if (host->caps & MMC_CAP_NONREMOVABLE)
+		*val = drv_type;
+	else
+		dev_err(dev, "can't use %s, media is removable\n", prop);
+}
+
 /**
  *	mmc_of_parse() - parse host's device-tree node
  *	@host: host whose node should be parsed.
@@ -173,7 +187,7 @@ static void mmc_retune_timer(struct timer_list *t)
 int mmc_of_parse(struct mmc_host *host)
 {
 	struct device *dev = host->parent;
-	u32 bus_width, drv_type, cd_debounce_delay_ms;
+	u32 bus_width, cd_debounce_delay_ms;
 	int ret;
 	bool cd_cap_invert, cd_gpio_invert = false;
 	bool ro_cap_invert, ro_gpio_invert = false;
@@ -321,13 +335,7 @@ int mmc_of_parse(struct mmc_host *host)
 		host->caps2 |= MMC_CAP2_NO_MMC;
 
 	/* Must be after "non-removable" check */
-	if (device_property_read_u32(dev, "fixed-emmc-driver-type", &drv_type) == 0) {
-		if (host->caps & MMC_CAP_NONREMOVABLE)
-			host->fixed_drv_type = drv_type;
-		else
-			dev_err(host->parent,
-				"can't use fixed driver type, media is removable\n");
-	}
+	mmc_of_read_drv_type(host, "fixed-emmc-driver-type", &host->fixed_drv_type);
 
 	host->dsr_req = !device_property_read_u32(dev, "dsr", &host->dsr);
 	if (host->dsr_req && (host->dsr & ~0xffff)) {
-- 
2.23.0

[PATCH 3/3] mmc: core: Add 'fixed-emmc-driver-type-hs{200,400}'

[Eugeniu Rosca]

Add support for two more DT bindings, which stem from the need to
implement below real-life requirement shared by eMMC vendor:

 ---snip---
 Use "drive strength" value of 4 or 1 for HS400 or 0 for HS200.
 ---snip---

Inspire from [Y] and [Z] during implementation and testing (H3ULCB-KF).
Below decision matrix is intended as function of user's input:

      [0]        [2]        [4]
[0]  hs200:[0]  hs200:[2]  hs200:[0]
     hs400:[0]  hs400:[0]  hs400:[4]
[2]  hs200:[2]  hs200:[2]  hs200:[2]
     hs400:[0]  hs400:[R]  hs400:[4]
[4]  hs200:[0]  hs200:[2]  hs200:[R]
     hs400:[4]  hs400:[4]  hs400:[4]

[0] "fixed-emmc-driver-type"
[2] "fixed-emmc-driver-type-hs200"
[4] "fixed-emmc-driver-type-hs400"
[R] RAW/ECSD drive strength as implemented in
    commit cc4f414c885cd0 ("mmc: mmc: Add driver strength selection")
[Y] commit 6186d06c519e21 ("mmc: parse new binding for eMMC fixed driver type")
[Z] https://www.elinux.org/Tests:eMMC-fixed-drive-strength

Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 drivers/mmc/core/host.c  |  4 ++++
 drivers/mmc/core/mmc.c   | 19 ++++++++++++++++---
 include/linux/mmc/host.h |  2 ++
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 54abfdfc69ba..2a3d3b542e0d 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -336,6 +336,8 @@ int mmc_of_parse(struct mmc_host *host)
 
 	/* Must be after "non-removable" check */
 	mmc_of_read_drv_type(host, "fixed-emmc-driver-type", &host->fixed_drv_type);
+	mmc_of_read_drv_type(host, "fixed-emmc-driver-type-hs200", &host->fixed_drv_type_hs200);
+	mmc_of_read_drv_type(host, "fixed-emmc-driver-type-hs400", &host->fixed_drv_type_hs400);
 
 	host->dsr_req = !device_property_read_u32(dev, "dsr", &host->dsr);
 	if (host->dsr_req && (host->dsr & ~0xffff)) {
@@ -455,6 +457,8 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
 	host->max_blk_count = PAGE_SIZE / 512;
 
 	host->fixed_drv_type = -EINVAL;
+	host->fixed_drv_type_hs200 = -EINVAL;
+	host->fixed_drv_type_hs400 = -EINVAL;
 	host->ios.power_delay_ms = 10;
 
 	return host;
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index c8804895595f..89e6fb9aedeb 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -62,6 +62,8 @@ static const unsigned int taac_mant[] = {
 		__res & __mask;						\
 	})
 
+static void mmc_select_driver_type(struct mmc_card *card, int timing);
+
 /*
  * Given the decoded CSD structure, decode the raw CID to our CID structure.
  */
@@ -1192,6 +1194,8 @@ static int mmc_select_hs400(struct mmc_card *card)
 		return err;
 	}
 
+	mmc_select_driver_type(card, EXT_CSD_TIMING_HS400);
+
 	/* Switch card to HS400 */
 	val = EXT_CSD_TIMING_HS400 |
 	      card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
@@ -1270,6 +1274,8 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 	if (err)
 		goto out_err;
 
+	mmc_select_driver_type(card, EXT_CSD_TIMING_HS200);
+
 	/* Switch HS to HS200 */
 	val = EXT_CSD_TIMING_HS200 |
 	      card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
@@ -1304,10 +1310,17 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 	return err;
 }
 
-static void mmc_select_driver_type(struct mmc_card *card)
+static void mmc_select_driver_type(struct mmc_card *card, int timing)
 {
 	int card_drv_type, drive_strength, drv_type = 0;
 	int fixed_drv_type = card->host->fixed_drv_type;
+	int fixed_drv_type_hs200 = card->host->fixed_drv_type_hs200;
+	int fixed_drv_type_hs400 = card->host->fixed_drv_type_hs400;
+
+	if (fixed_drv_type_hs200 >= 0 && timing == EXT_CSD_TIMING_HS200)
+		fixed_drv_type = fixed_drv_type_hs200;
+	else if (fixed_drv_type_hs400 >= 0 && timing == EXT_CSD_TIMING_HS400)
+		fixed_drv_type = fixed_drv_type_hs400;
 
 	card_drv_type = card->ext_csd.raw_driver_strength |
 			mmc_driver_type_mask(0);
@@ -1385,7 +1398,7 @@ static int mmc_select_hs400es(struct mmc_card *card)
 		goto out_err;
 	}
 
-	mmc_select_driver_type(card);
+	mmc_select_driver_type(card, EXT_CSD_TIMING_HS400);
 
 	/* Switch card to HS400 */
 	val = EXT_CSD_TIMING_HS400 |
@@ -1445,7 +1458,7 @@ static int mmc_select_hs200(struct mmc_card *card)
 	if (err)
 		return err;
 
-	mmc_select_driver_type(card);
+	mmc_select_driver_type(card, EXT_CSD_TIMING_HS200);
 
 	/*
 	 * Set the bus width(4 or 8) with host's support and
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index ba703384bea0..6960ba98810a 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -371,6 +371,8 @@ struct mmc_host {
 #define MMC_CAP2_MERGE_CAPABLE	(1 << 26)	/* Host can merge a segment over the segment size */
 
 	int			fixed_drv_type;	/* fixed driver type for non-removable media */
+	int			fixed_drv_type_hs200;	/* HS200-specific fixed_drv_type */
+	int			fixed_drv_type_hs400;	/* HS400-specific fixed_drv_type */
 
 	mmc_pm_flag_t		pm_caps;	/* supported pm features */
 
-- 
2.23.0

Re: [PATCH 1/3] dt-bindings: mmc: Add 'fixed-emmc-driver-type-hs{200,400}'

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1144 bytes --]

Hi Eugeniu,

thanks for this work!

> A certain eMMC manufacturer provided below requirement:
>  ---snip---
>  Use "drive strength" value of 4 or 1 for HS400 or 0 for HS200.
>  ---snip---

I see.

> The existing "fixed-emmc-driver-type" property [1] is the closest one
> to implement the above, but it falls short due to being unable to define
> two values to differentiate between HS200 and HS400 (both modes may be
> supported by the same non-removable MMC device).
> 
> To allow users to set a preferred HS200/HS400 "drive strength", provide
> two more bindings inspired from [1]:
>  - fixed-emmc-driver-type-hs200
>  - fixed-emmc-driver-type-hs400

Main question before looking at the code: Can't we just extend the
existing binding with an optional second parameter?

        minItems: 1
        maxItems: 2

I tend to favour this approach...

> For more details about eMMC I/O driver strength types, see Jedec spec.
> Keep "fixed-emmc-driver-type" in place for backward compatibility.

If we decide for the path proposed here, should the old binding be
deprecated then?

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] dt-bindings: mmc: Add 'fixed-emmc-driver-type-hs{200,400}'

[Eugeniu Rosca]

Hi Wolfram,

On Tue, Nov 05, 2019 at 07:22:23AM +0100, Wolfram Sang wrote:
> Hi Eugeniu,
> 
> thanks for this work!

Thanks for the prompt response. Very much appreciated.

> 
> > A certain eMMC manufacturer provided below requirement:
> >  ---snip---
> >  Use "drive strength" value of 4 or 1 for HS400 or 0 for HS200.
> >  ---snip---
> 
> I see.
> 
> > The existing "fixed-emmc-driver-type" property [1] is the closest one
> > to implement the above, but it falls short due to being unable to define
> > two values to differentiate between HS200 and HS400 (both modes may be
> > supported by the same non-removable MMC device).
> > 
> > To allow users to set a preferred HS200/HS400 "drive strength", provide
> > two more bindings inspired from [1]:
> >  - fixed-emmc-driver-type-hs200
> >  - fixed-emmc-driver-type-hs400
> 
> Main question before looking at the code: Can't we just extend the
> existing binding with an optional second parameter?

That's a great question/proposal, but before pushing the v2 right away,
I would like to first share some thoughts.

>         minItems: 1
>         maxItems: 2
> 
> I tend to favour this approach...

The first question which pops up in my mind is related to the meaning
of each item. The option which I envision based on your proposal is:

  * minItems: 1
  * maxItems: 2
  * Item[0]: Presumably equivalent to the current
    "fixed-emmc-driver-type", i.e. the strength value applied in both
    HS200 and HS400 modes.
  * Item[1] (optional): Presumably equivalent to
    "fixed-emmc-driver-type-hs400" proposed in this series. If this
    element is provided, the first one should likely change its role
    and become an equivalent of "fixed-emmc-driver-type-hs200" from
    this series.
  + Pro: Full backward compatibility. No need to touch the existing
    users of "fixed-emmc-driver-type".
  - Con: Not sure we have such DT bindings which dynamically change
    their semantics based on the usage pattern.
  - Con: Can't easily achieve the same flexibility as accomplished in
    this series. For example, current implementation allows users to
    define each of the three parameters (i.e. HSx00-agnostic drive
    strength, HS200 and HS400 specific drive strengths) individually,
    as well as in all possible combinations. This might be needed if,
    in certain HSx00 mode, users still need to rely on the
    RAW/unmodified drive strength. I am unsure if/how this can be
    achieved with an array OF property with a constant or variable
    number of elements (I try to sketch one solution below).

One option to achieve a similar degree of flexibility by using an array
OF property (instead of several u32 properties) would be to agree on a
convention based on magic values, i.e. below DT one-liner could be an
example of providing solely the "fixed-emmc-driver-type-hs200" value
(based on the agreement that 0xFF values are discarded by the driver):

    fixed-emmc-driver-type = <0xFF 0x1 0xFF>;

> 
> > For more details about eMMC I/O driver strength types, see Jedec spec.
> > Keep "fixed-emmc-driver-type" in place for backward compatibility.
> 
> If we decide for the path proposed here, should the old binding be
> deprecated then?

I can either zap "fixed-emmc-driver-type" or extend its type and
meaning, depending on the feedback from the reviewers.
Looking forward to any comments and suggestions.

> 
> Happy hacking,
> 
>    Wolfram

Thanks.

-- 
Best Regards,
Eugeniu

Re: [PATCH 1/3] dt-bindings: mmc: Add 'fixed-emmc-driver-type-hs{200,400}'

[Rob Herring]

On Tue, Nov 05, 2019 at 09:32:13AM +0100, Eugeniu Rosca wrote:
> Hi Wolfram,
> 
> On Tue, Nov 05, 2019 at 07:22:23AM +0100, Wolfram Sang wrote:
> > Hi Eugeniu,
> > 
> > thanks for this work!
> 
> Thanks for the prompt response. Very much appreciated.
> 
> > 
> > > A certain eMMC manufacturer provided below requirement:
> > >  ---snip---
> > >  Use "drive strength" value of 4 or 1 for HS400 or 0 for HS200.
> > >  ---snip---
> > 
> > I see.
> > 
> > > The existing "fixed-emmc-driver-type" property [1] is the closest one
> > > to implement the above, but it falls short due to being unable to define
> > > two values to differentiate between HS200 and HS400 (both modes may be
> > > supported by the same non-removable MMC device).
> > > 
> > > To allow users to set a preferred HS200/HS400 "drive strength", provide
> > > two more bindings inspired from [1]:
> > >  - fixed-emmc-driver-type-hs200
> > >  - fixed-emmc-driver-type-hs400
> > 
> > Main question before looking at the code: Can't we just extend the
> > existing binding with an optional second parameter?

I was thinking the same thing...

> 
> That's a great question/proposal, but before pushing the v2 right away,
> I would like to first share some thoughts.
> 
> >         minItems: 1
> >         maxItems: 2
> > 
> > I tend to favour this approach...
> 
> The first question which pops up in my mind is related to the meaning
> of each item. The option which I envision based on your proposal is:
> 
>   * minItems: 1
>   * maxItems: 2
>   * Item[0]: Presumably equivalent to the current
>     "fixed-emmc-driver-type", i.e. the strength value applied in both
>     HS200 and HS400 modes.
>   * Item[1] (optional): Presumably equivalent to
>     "fixed-emmc-driver-type-hs400" proposed in this series. If this
>     element is provided, the first one should likely change its role
>     and become an equivalent of "fixed-emmc-driver-type-hs200" from
>     this series.
>   + Pro: Full backward compatibility. No need to touch the existing
>     users of "fixed-emmc-driver-type".
>   - Con: Not sure we have such DT bindings which dynamically change
>     their semantics based on the usage pattern.
>   - Con: Can't easily achieve the same flexibility as accomplished in
>     this series. For example, current implementation allows users to
>     define each of the three parameters (i.e. HSx00-agnostic drive
>     strength, HS200 and HS400 specific drive strengths) individually,
>     as well as in all possible combinations. This might be needed if,
>     in certain HSx00 mode, users still need to rely on the
>     RAW/unmodified drive strength. I am unsure if/how this can be
>     achieved with an array OF property with a constant or variable
>     number of elements (I try to sketch one solution below).
> 
> One option to achieve a similar degree of flexibility by using an array
> OF property (instead of several u32 properties) would be to agree on a
> convention based on magic values, i.e. below DT one-liner could be an
> example of providing solely the "fixed-emmc-driver-type-hs200" value
> (based on the agreement that 0xFF values are discarded by the driver):
> 
>     fixed-emmc-driver-type = <0xFF 0x1 0xFF>;

I don't understand why you have 3 values instead of 2.

I would just use -1 if you want to ignore an entry. If that's the common 
case, then I'd stick with what you originally proposed. If rare, then I 
think an array is the better route.

Rob

Re: [PATCH 1/3] dt-bindings: mmc: Add 'fixed-emmc-driver-type-hs{200,400}'

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2811 bytes --]

Hi everyone,

> > The first question which pops up in my mind is related to the meaning
> > of each item. The option which I envision based on your proposal is:
> > 
> >   * minItems: 1
> >   * maxItems: 2
> >   * Item[0]: Presumably equivalent to the current
> >     "fixed-emmc-driver-type", i.e. the strength value applied in both
> >     HS200 and HS400 modes.
> >   * Item[1] (optional): Presumably equivalent to
> >     "fixed-emmc-driver-type-hs400" proposed in this series. If this
> >     element is provided, the first one should likely change its role
> >     and become an equivalent of "fixed-emmc-driver-type-hs200" from
> >     this series.
> >   + Pro: Full backward compatibility. No need to touch the existing
> >     users of "fixed-emmc-driver-type".
> >   - Con: Not sure we have such DT bindings which dynamically change
> >     their semantics based on the usage pattern.
> >   - Con: Can't easily achieve the same flexibility as accomplished in
> >     this series. For example, current implementation allows users to
> >     define each of the three parameters (i.e. HSx00-agnostic drive
> >     strength, HS200 and HS400 specific drive strengths) individually,
> >     as well as in all possible combinations. This might be needed if,
> >     in certain HSx00 mode, users still need to rely on the
> >     RAW/unmodified drive strength. I am unsure if/how this can be
> >     achieved with an array OF property with a constant or variable
> >     number of elements (I try to sketch one solution below).
> > 
> > One option to achieve a similar degree of flexibility by using an array
> > OF property (instead of several u32 properties) would be to agree on a
> > convention based on magic values, i.e. below DT one-liner could be an
> > example of providing solely the "fixed-emmc-driver-type-hs200" value
> > (based on the agreement that 0xFF values are discarded by the driver):
> > 
> >     fixed-emmc-driver-type = <0xFF 0x1 0xFF>;
> 
> I don't understand why you have 3 values instead of 2.

Because he sketches maximum flexibility here. Have a non-HS (default)
value, one for HS200, and one for HS400:

	fixed-emmc-driver-type = <[default] [HS200] [HS400]>;

> I would just use -1 if you want to ignore an entry. If that's the common 

'-1' sounds good to me, too.

> case, then I'd stick with what you originally proposed. If rare, then I 
> think an array is the better route.

What I have seen so far: setting drive strength alone is more on the
rare side. Setting specific values for default and HS200/400 seems even
more rare to me. With this patchset, it is the first time I hear about
it.

Yet, my experience might be a bit limited, maybe others (Hi Ulf! ;)) can add
their views, too?

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] dt-bindings: mmc: Add 'fixed-emmc-driver-type-hs{200,400}'

[Linus Walleij]

On Tue, Nov 12, 2019 at 10:19 PM Wolfram Sang <wsa@the-dreams.de> wrote:

> What I have seen so far: setting drive strength alone is more on the
> rare side. Setting specific values for default and HS200/400 seems even
> more rare to me. With this patchset, it is the first time I hear about
> it.

Like I wrote to Eugeniu this sounds like some kind of errata
for the eMMC ext CSD and should likely be a card quirk rather
than some generic device tree properties.

I might be wrong, we'll hash it out.

Yours,
Linus Walleij

Re: [PATCH 1/3] dt-bindings: mmc: Add 'fixed-emmc-driver-type-hs{200,400}'

[Ulf Hansson]

[...]

> > >
> > > One option to achieve a similar degree of flexibility by using an array
> > > OF property (instead of several u32 properties) would be to agree on a
> > > convention based on magic values, i.e. below DT one-liner could be an
> > > example of providing solely the "fixed-emmc-driver-type-hs200" value
> > > (based on the agreement that 0xFF values are discarded by the driver):
> > >
> > >     fixed-emmc-driver-type = <0xFF 0x1 0xFF>;
> >
> > I don't understand why you have 3 values instead of 2.
>
> Because he sketches maximum flexibility here. Have a non-HS (default)
> value, one for HS200, and one for HS400:
>
>         fixed-emmc-driver-type = <[default] [HS200] [HS400]>;
>
> > I would just use -1 if you want to ignore an entry. If that's the common
>
> '-1' sounds good to me, too.
>
> > case, then I'd stick with what you originally proposed. If rare, then I
> > think an array is the better route.
>
> What I have seen so far: setting drive strength alone is more on the
> rare side. Setting specific values for default and HS200/400 seems even
> more rare to me. With this patchset, it is the first time I hear about
> it.
>
> Yet, my experience might be a bit limited, maybe others (Hi Ulf! ;)) can add
> their views, too?

My experience in this field is quite limited. No input from me, sorry.

Kind regards
Uffe

Re: [PATCH 1/3] dt-bindings: mmc: Add 'fixed-emmc-driver-type-hs{200,400}'

[Linus Walleij]

Hi Eugeniu,

thanks for your patch!

On Tue, Nov 5, 2019 at 6:50 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:

> A certain eMMC manufacturer provided below requirement:
>  ---snip---
>  Use "drive strength" value of 4 or 1 for HS400 or 0 for HS200.
>  ---snip---
>
> The existing "fixed-emmc-driver-type" property [1] is the closest one
> to implement the above, but it falls short due to being unable to define
> two values to differentiate between HS200 and HS400 (both modes may be
> supported by the same non-removable MMC device).
>
> To allow users to set a preferred HS200/HS400 "drive strength", provide
> two more bindings inspired from [1]:
>  - fixed-emmc-driver-type-hs200
>  - fixed-emmc-driver-type-hs400

I am sorry that I do not quite understand but as pin control maintainer I
am of course triggered by the talk about selecting "drive strength".

In my book this means that the pad driver on the chip, driving the
line low/high with push-pull (totempole output, usually) is connecting
more driver stages, usually just shunting in more totempoles.
(Ref https://en.wikipedia.org/wiki/Push%E2%80%93pull_output)

If say one totempole gives 2mA drive strength then 4 totempoles
gives 8mA drive strength.

Are we on the same page here that this is what physically happens?

Usually selection of drive strength is done with the pin control
framework, so this would need to be backed by code (not in this
patch set) that select pin control states that reconfigure the
SoC pad drivers to use the requested strength.

Alternatively, the (e)MMC block would implement this control
directly, but I doubt it.

Please clarify which hardware is eventually going to provide the
drive strength alteration, because I just don't see it in the patch
set. Is the assumption that the (e)MMC hardware will do this
autonomously or something? That may be a pecularity to the hardware
you're using in that case.

I find the fixed-emmc-driver-type-* assignment a but puzzling
to be honest, isnt' the driver device tree already specifying
what the hardware can do with all of these:

mmc-ddr-1_2v
mmc-ddr-1_8v
mmc-ddr-3_3v
mmc-hs200-1_2v
mmc-hs200-1_8v
mmc-hs400-1_2v
mmc-hs400-1_8v
mmc-hs400-enhanced-strobe

If the host is already specifying mmc-hs200-* or
mmc-hs400-* then certainly it should be implied that the
host supports hs200 and hs400 and there is no need for
the fixed-emmc-driver-type-hs* properties.

The code detects when to use each mode and that is when
you can insert the code to switch drive strengths, whether using
the pin control framework or something else.

So to me it seems these DT properties are just introduced to
hammer down a certain usecase instead of letting the code with the
help of DT speed capabilities flags determine what speed is to be used
and select the appropriate drive strength.

Yours,
Linus Walleij

Re: [PATCH 1/3] dt-bindings: mmc: Add 'fixed-emmc-driver-type-hs{200,400}'

[Eugeniu Rosca]

Hi Linus,

On Wed, Nov 06, 2019 at 12:07:38PM +0100, Linus Walleij wrote:
> Hi Eugeniu,
> 
> thanks for your patch!

Thanks for your comments.

> 
> On Tue, Nov 5, 2019 at 6:50 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> 
> > A certain eMMC manufacturer provided below requirement:
> >  ---snip---
> >  Use "drive strength" value of 4 or 1 for HS400 or 0 for HS200.
> >  ---snip---
> >
> > The existing "fixed-emmc-driver-type" property [1] is the closest one
> > to implement the above, but it falls short due to being unable to define
> > two values to differentiate between HS200 and HS400 (both modes may be
> > supported by the same non-removable MMC device).
> >
> > To allow users to set a preferred HS200/HS400 "drive strength", provide
> > two more bindings inspired from [1]:
> >  - fixed-emmc-driver-type-hs200
> >  - fixed-emmc-driver-type-hs400
> 
> I am sorry that I do not quite understand but as pin control maintainer I
> am of course triggered by the talk about selecting "drive strength".
> 
> In my book this means that the pad driver on the chip, driving the
> line low/high with push-pull (totempole output, usually) is connecting
> more driver stages, usually just shunting in more totempoles.
> (Ref https://en.wikipedia.org/wiki/Push%E2%80%93pull_output)
> 
> If say one totempole gives 2mA drive strength then 4 totempoles
> gives 8mA drive strength.
> 
> Are we on the same page here that this is what physically happens?

This matches my view with below amendments:
 - Your passage seems to describe a single-duplex communication (one end
   is always a sender and the other one is always a receiver). Since the
   CMD and DAT[0-7] eMMC lines are bidirectional (carrying half-duplex
   data transfers), this is what seems to justify the
   "drive(r) strength/type" feature/setting to be present on both host
   controller and eMMC device ends (which does happen in real life).
 - I am unsure whether to endorse the "totempole output" as being the
   usual foundation for how "drive strength" is really implemented in
   the modern CMOS ICs, based on the following:
   - All eMMC Jedec specs mention "push-pull" for CMD/DAT[0-7]
   - All eMMC device datasheets I could find reference "push pull"
     and none mentions "totem pole" for CMD/DAT[0-7]
   - The "totem pole" topology seems to originate from and be much more
     popular in the TTL/BJT world, where it tries to harness the
     symmetry of two NPN transistors, replacing the PNP-NPN pair used
     in the bipolar "push-pull" configuration [1-2].
   - Jedec calls totem-pole "a bipolar output" (i.e. TTL/BJT) [3]
   - Jedec claims [3] that "vanilla" tottempole doesn't support
     tristate/hi-Z outputs, making it impossible to connect several such
     circuits in parallel, while we assume the latter to be a hard
     prerequisite for sourcing programmable amounts of current.
   - Some users say that "CMOS outputs are generally push-pull" [4]
   - TI states [5] that the "MOSFET equivalent of the bipolar totempole
     driver [..] is rarely implemented"

Abstracting from the above, I agree that a programmable "drive strength"
is likely achieved by connecting several tristate-capable output
circuits in a "wired OR", as exemplified for Raspberry Pi in [6].

> 
> Usually selection of drive strength is done with the pin control
> framework, so this would need to be backed by code (not in this
> patch set) that select pin control states that reconfigure the
> SoC pad drivers to use the requested strength.

That's true. In the same context of overcoming HS400 issues, our SoC
vendor suggested adjusting the "drive-strength" binding, added in:
 - 7db9af4b6e41be ("pinctrl: add function to parse generic pinconfig
   properties from a dt node")
 - 3caa7d8c3f03ad ("pinctrl: sh-pfc: Add drive strength support")

> Alternatively, the (e)MMC block would implement this control
> directly, but I doubt it.

There _is_ a "drive strength" specific to eMMC device and the
justification for it to exist has been made above.

According to JESD84-B50.1 spec, the host controller is able to find
the "drive strength" values supported by a particular eMMC device by
reading the DRIVER_STRENGTH field of the Extended CSD. The host then
may (if needed), make use of this value to set the "Driver Strength"
parameter in the HS_TIMING field of the Extended CSD register.

Essentially, current series gives the host controller a chance to limit
the drive strength value written to HS_TIMING, if eMMC vendor decides
that some of the values advertised in DRIVER_STRENGTH are forbidden
or should be avoided in a specific bus speed mode (HS200/HS400).

> Please clarify which hardware is eventually going to provide the
> drive strength alteration, because I just don't see it in the patch
> set. Is the assumption that the (e)MMC hardware will do this
> autonomously or something? That may be a pecularity to the hardware
> you're using in that case.

Hopefully clarified above.

> I find the fixed-emmc-driver-type-* assignment a but puzzling
> to be honest, isnt' the driver device tree already specifying
> what the hardware can do with all of these:
> 
> mmc-ddr-1_2v
> mmc-ddr-1_8v
> mmc-ddr-3_3v

JFTR, for these (<HS200) bus speed modes, the eMMC Jedec spec doesn't
talk about changing eMMC's driver strength. That's probably because
there are no signal integrity issues to be fixed in these low-speed
modes.

> mmc-hs200-1_2v
> mmc-hs200-1_8v
> mmc-hs400-1_2v
> mmc-hs400-1_8v
> mmc-hs400-enhanced-strobe

Below is a quote from JESD84-B50.1 (10.5.4.1 Driver Types Definition):
 -> NOTE: Drive strength definitions are same for 1.8 V signaling level
 -> and for 1.2 V signaling level.

I read the above as:
 -> "the driver strength is independent/agnostic on signaling level"

> If the host is already specifying mmc-hs200-* or
> mmc-hs400-* then certainly it should be implied that the
> host supports hs200 and hs400 and there is no need for
> the fixed-emmc-driver-type-hs* properties.

Not sure I see the linkage between the cause and effect here.
The host cannot infer anything about the supported drive strength values
on eMMC side purely based on the mmc-hs200-*/mmc-hs400-* DT properties.

> 
> The code detects when to use each mode and that is when
> you can insert the code to switch drive strengths, whether using
> the pin control framework or something else.

As explained above, this series allows to customize the eMMC-specific
drive strength. The eMMC vendor did not ask to make the SoC-side
drive strength dependent on bus speed mode and that was not needed in
the testing performed by the customer.

> 
> So to me it seems these DT properties are just introduced to
> hammer down a certain usecase instead of letting the code with the
> help of DT speed capabilities flags determine what speed is to be used
> and select the appropriate drive strength.

Does this mean that the "fixed-emmc-driver-type" binding which
pre-exists my series falls under the same sentence? Or is this only
when customizing Wolfram's binding to HS200/HS400 bus speed mode?

Note that there is no other objective in this series but to allow Linux
to run on hardware which doesn't strictly follow its specification [7].
If you have any alternative ideas of how to follow the eMMC vendor's
recommendation quoted in the description of this patch, I will happily
review those.

> 
> Yours,
> Linus Walleij

[1] https://electronics.stackexchange.com/q/358151/235971
[2] https://electronics.stackexchange.com/a/17819/235971
   ---snip----
   Due to the difference in N and P carrier mobilities, NPN and PNP
   transistors are never truly symmetric, and there were advantages to
   using NPN. [..] In CMOS logic, the N and P channel drivers are
   symmetric and the driver designs are truly complementary.
   ---snip----
[3] https://www.jedec.org/standards-documents/dictionary/terms/totem-pole-output
   ---snip----
   The term "totem-pole output", as commonly used, does not include
   three-state outputs.
   ---snip----
[4] http://piclist.com/techref/postbot.asp?by=thread&id=%5BEE%5D+Push-pull+vs+totem+pole&w=body&tgt=post
   ---snip----
   CMOS outputs are generally push-pull. They have a P-channel FET
   above and an N-channel fet below. Both are in digital model
   (on/off). 'Totem-pole' will never apply to a CMOS output.
   ---snip----
[5] http://www.ti.com/lit/ml/slua618a/slua618a.pdf
   ("Fundamentals of MOSFET and IGBT Gate Driver Circuits")
   ---snip----
   The MOSFET equivalent of the bipolar totempole driver is pictured in
   Figure 11. [..] Unfortunately, this circuit has several drawbacks
   compared to the bipolar version that explain that it is very rarely
   implemented discretely.
   ---snip----
[6] https://www.raspberrypi.org/documentation/hardware/raspberrypi/gpio/gpio_pads_control.md
[7] linux (v5.4-rc7) git grep -i quirk | wc -l
   12047

-- 
Best Regards,
Eugeniu

Re: [PATCH 1/3] dt-bindings: mmc: Add 'fixed-emmc-driver-type-hs{200,400}'

[Linus Walleij]

Hi Eugeniu,

On Mon, Nov 11, 2019 at 11:25 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:

> This matches my view with below amendments:
>  - Your passage seems to describe a single-duplex communication (one end
>    is always a sender and the other one is always a receiver). Since the
>    CMD and DAT[0-7] eMMC lines are bidirectional (carrying half-duplex
>    data transfers), this is what seems to justify the
>    "drive(r) strength/type" feature/setting to be present on both host
>    controller and eMMC device ends (which does happen in real life).
>  - I am unsure whether to endorse the "totempole output" as being the
>    usual foundation for how "drive strength" is really implemented in
>    the modern CMOS ICs, based on the following:
>    - All eMMC Jedec specs mention "push-pull" for CMD/DAT[0-7]
>    - All eMMC device datasheets I could find reference "push pull"
>      and none mentions "totem pole" for CMD/DAT[0-7]
>    - The "totem pole" topology seems to originate from and be much more
>      popular in the TTL/BJT world, where it tries to harness the
>      symmetry of two NPN transistors, replacing the PNP-NPN pair used
>      in the bipolar "push-pull" configuration [1-2].
>    - Jedec calls totem-pole "a bipolar output" (i.e. TTL/BJT) [3]
>    - Jedec claims [3] that "vanilla" tottempole doesn't support
>      tristate/hi-Z outputs, making it impossible to connect several such
>      circuits in parallel, while we assume the latter to be a hard
>      prerequisite for sourcing programmable amounts of current.
>    - Some users say that "CMOS outputs are generally push-pull" [4]
>    - TI states [5] that the "MOSFET equivalent of the bipolar totempole
>      driver [..] is rarely implemented"
>
> Abstracting from the above, I agree that a programmable "drive strength"
> is likely achieved by connecting several tristate-capable output
> circuits in a "wired OR", as exemplified for Raspberry Pi in [6].

OK that's established. I am sorry for using the TTL "totempole"
term here, it is out of place for CMOS indeed.

Very nice detailing and references, I read them all :)

> > Usually selection of drive strength is done with the pin control
> > framework, so this would need to be backed by code (not in this
> > patch set) that select pin control states that reconfigure the
> > SoC pad drivers to use the requested strength.
>
> That's true. In the same context of overcoming HS400 issues, our SoC
> vendor suggested adjusting the "drive-strength" binding, added in:
>  - 7db9af4b6e41be ("pinctrl: add function to parse generic pinconfig
>    properties from a dt node")
>  - 3caa7d8c3f03ad ("pinctrl: sh-pfc: Add drive strength support")

OK so the pin controller will act as back-end for this and the
drivers are expected to use that.

I suppose you are defining new HSxxx-specific pin control
states for them? I suppose it would be good to see how it
works end-to-end. (But fine, I get it so far.)

> > Alternatively, the (e)MMC block would implement this control
> > directly, but I doubt it.
>
> There _is_ a "drive strength" specific to eMMC device and the
> justification for it to exist has been made above.
>
> According to JESD84-B50.1 spec, the host controller is able to find
> the "drive strength" values supported by a particular eMMC device by
> reading the DRIVER_STRENGTH field of the Extended CSD. The host then
> may (if needed), make use of this value to set the "Driver Strength"
> parameter in the HS_TIMING field of the Extended CSD register.

So the operating system reads the ext CSD and uses that
information to set the drive strength using pin control in the
Linux case.

What is the unit of this driver strength field in the ext CSD?

And consequently this:

+  fixed-emmc-driver-type-hs200:
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - minimum: 0
+      - maximum: 4
+    description:
+      Same as "fixed-emmc-driver-type", but specific to HS200 mode.
+      If defined, overrides "fixed-emmc-driver-type" in HS200 mode.

This thing that is 0,1,2,3,4, what unit is that?

If we established it is the number of push-pull stages OR:ed
together we should document it. Since it is supposed to be
used with different host controllers, it certainly cannot be
unitless or "vendor specific" since you have two sides to
it, the card ext CSD and this, and then the pin controller
side. (I'm not a standards person buy certainly JEDEC
must have thought of that?)

> Essentially, current series gives the host controller a chance to limit
> the drive strength value written to HS_TIMING, if eMMC vendor decides
> that some of the values advertised in DRIVER_STRENGTH are forbidden
> or should be avoided in a specific bus speed mode (HS200/HS400).

OK this text should really be in the commit message and the
bindings because this isn't clear from context.

It confused me so it will confuse others.

I still don't quite get it, sorry if I'm dumb :/

Do you mean that the eMMC advertise some drive strengths in
the ext CSD and the device tree properties are there to mitigate
or override these and disallow them because of limitations in the
host controller or associated electronics? That sounds more
like something the system integrator/manufacturer decide than
the eMMC vendor.

Or it it a bug in the ext CSD so that the eMMC advertise strengths
it shouldn't? Then we should use a quirk for the card ID
rather than a DT property.

> As explained above, this series allows to customize the eMMC-specific
> drive strength. The eMMC vendor did not ask to make the SoC-side
> drive strength dependent on bus speed mode and that was not needed in
> the testing performed by the customer.

I think I understand this now! Some nice details above
makes it clear what these values are for.

> > So to me it seems these DT properties are just introduced to
> > hammer down a certain usecase instead of letting the code with the
> > help of DT speed capabilities flags determine what speed is to be used
> > and select the appropriate drive strength.
>
> Does this mean that the "fixed-emmc-driver-type" binding which
> pre-exists my series falls under the same sentence? Or is this only
> when customizing Wolfram's binding to HS200/HS400 bus speed mode?

Now that it's clear that this is to restrict the drive strengths
used I understand it better!

> Note that there is no other objective in this series but to allow Linux
> to run on hardware which doesn't strictly follow its specification [7].
> If you have any alternative ideas of how to follow the eMMC vendor's
> recommendation quoted in the description of this patch, I will happily
> review those.

I'm a bit confused. This "recommendation" sounds like some
errata actually.

If the case is that eMMC vendor has recommended certain stuff
in the ext CSD and you need to augment that with the device tree
config, that sounds like the wrong approach. It is a bug in that
eMMCs ext CSD is it not?

Why can't we use code for this and just add a per-card quirk
instead if there are errors in the drive strengths recommended
in the ext CSD? Like the other stuff in drivers/mmc/core/quirks.h.

That makes the same card work on any other host without any
device tree special properties, hopefully.

Yours,
Linus Walleij