[PATCH v2 0/6] Add enhanced strobe support for emmc version 5.1 or later

[PATCH v2 0/6] Add enhanced strobe support for emmc version 5.1 or later

[Shawn Lin]

Hello Ulf and Adrian,

This patch is going to support enhanced strobe function
for emmc version 5.1+ introduced by JEDEC recently.

Enchanced strobe is a optional function, so we add a new cap* for drivers to
decide whether to use it.

When introduing hs400 mode, JEDEC asks controllers to use data strobe line
to latch the data from emmc devives. But for cmd-reponse, not mentioned yet.
Since emmc version 5.1 published, JEDEC adds enhanced strobe function to deal
with cmd-response the same way as data-read. This feature is optional.

>From the spec(section 6.6.2.3), the standard scenario to select HS400 enhanced
strobe mode illustrated like this:
(1) set HS_TIMIMG (Highspeed)
(2) Host change freq to <= 52Mhz
(3) set the bus width to Enhanced strobe and DDR8Bit(CMD6), EXT_CSD[183] = 0x86
instead of 0x80
(4) set HS_TIMING to 0x3 (HS400)
(5) Host change freq to <= 200Mhz
(6) Host select HS400 enhanced strobe complete

I can't find a upstreamed controller claimed to support it, as well as the
mmc stack. But my "arasan,sdhci-5.1" actually supports this function. So I decide
to work for this part.

By looking into the SDHCI spec, I find there isn't any registers to enable the
enhanced strobe function. But from my "arasan,sdhci-5.1" databook, it describes a
register called VENDOR_REGISTER(0x78) to trigger this mode. So I guess other sdhci
variant drivers may also need s vendor specific register to deal with it.

If we are sure that our controller supports enhanced strobe mode, just add
mmc-hs400-enhanced-strobe in DT. Once emmc devices claims to support this mode,
we enable it automatically. Of course, other sdhci*/non-sdhci drivers should
implement/overwrite the prepare_enhanced_strobe by themselves. I believe all of the
platforms which need to support this mode should do some basic preparation for their
controllers. Currently I just limit the callback within ths scope of arasan,sdhci-5.1,
but I prone to believe arasan will use VENDOR_REGISTER to tirgger this mode from now on
because I do believe vendor will consider the registers' compatibility.

With this patchset applied, we can successfully run in HS400 enhanced strobe mode on
RK3399 platform with Samsung eMMC KLMBG2JENB-B041(v5.1, 16GB).

mmc1: new HS400 Enhanced strobe MMC card at address 0001
mmcblk0: mmc1:0001 AJNB4R 14.6 GiB
mmcblk0boot0: mmc1:0001 AJNB4R partition 1 4.00 MiB
mmcblk0boot1: mmc1:0001 AJNB4R partition 2 4.00 MiB
mmcblk0rpmb: mmc1:0001 AJNB4R partition 3 4.00 MiB


Changes in v2:
- switch to HS400ES from Highspeed mode directly

Shawn Lin (6):
  Documentation: mmc: add mmc-hs400-enhanced-strobe
  mmc: core: add mmc-hs400-enhanced-strobe support
  mmc: core: implement enhanced strobe support
  mmc: debugfs: add HS400 enhanced strobe description
  mmc: sdhci: implement enhanced strobe callback
  mmc: sdhci-of-arasan: overwrite enhanced strobe callback

 Documentation/devicetree/bindings/mmc/mmc.txt |  1 +
 drivers/mmc/core/bus.c                        |  3 +-
 drivers/mmc/core/debugfs.c                    |  4 +-
 drivers/mmc/core/host.c                       |  2 +
 drivers/mmc/core/mmc.c                        | 77 ++++++++++++++++++++++++---
 drivers/mmc/host/sdhci-of-arasan.c            | 22 ++++++++
 drivers/mmc/host/sdhci.c                      | 11 ++++
 include/linux/mmc/card.h                      |  1 +
 include/linux/mmc/host.h                      | 18 +++++++
 include/linux/mmc/mmc.h                       |  3 ++
 10 files changed, 134 insertions(+), 8 deletions(-)

-- 
2.3.7

[PATCH v2 0/6] Add enhanced strobe support for emmc version 5.1 or later

[Shawn Lin]

Hello Ulf and Adrian,

This patch is going to support enhanced strobe function
for emmc version 5.1+ introduced by JEDEC recently.

Enchanced strobe is a optional function, so we add a new cap* for drivers to
decide whether to use it.

When introduing hs400 mode, JEDEC asks controllers to use data strobe line
to latch the data from emmc devives. But for cmd-reponse, not mentioned yet.
Since emmc version 5.1 published, JEDEC adds enhanced strobe function to deal
with cmd-response the same way as data-read. This feature is optional.

From the spec(section 6.6.2.3), the standard scenario to select HS400 enhanced
strobe mode illustrated like this:
(1) set HS_TIMIMG (Highspeed)
(2) Host change freq to <= 52Mhz
(3) set the bus width to Enhanced strobe and DDR8Bit(CMD6), EXT_CSD[183] = 0x86
instead of 0x80
(4) set HS_TIMING to 0x3 (HS400)
(5) Host change freq to <= 200Mhz
(6) Host select HS400 enhanced strobe complete

I can't find a upstreamed controller claimed to support it, as well as the
mmc stack. But my "arasan,sdhci-5.1" actually supports this function. So I decide
to work for this part.

By looking into the SDHCI spec, I find there isn't any registers to enable the
enhanced strobe function. But from my "arasan,sdhci-5.1" databook, it describes a
register called VENDOR_REGISTER(0x78) to trigger this mode. So I guess other sdhci
variant drivers may also need s vendor specific register to deal with it.

If we are sure that our controller supports enhanced strobe mode, just add
mmc-hs400-enhanced-strobe in DT. Once emmc devices claims to support this mode,
we enable it automatically. Of course, other sdhci*/non-sdhci drivers should
implement/overwrite the prepare_enhanced_strobe by themselves. I believe all of the
platforms which need to support this mode should do some basic preparation for their
controllers. Currently I just limit the callback within ths scope of arasan,sdhci-5.1,
but I prone to believe arasan will use VENDOR_REGISTER to tirgger this mode from now on
because I do believe vendor will consider the registers' compatibility.

With this patchset applied, we can successfully run in HS400 enhanced strobe mode on
RK3399 platform with Samsung eMMC KLMBG2JENB-B041(v5.1, 16GB).

mmc1: new HS400 Enhanced strobe MMC card at address 0001
mmcblk0: mmc1:0001 AJNB4R 14.6 GiB
mmcblk0boot0: mmc1:0001 AJNB4R partition 1 4.00 MiB
mmcblk0boot1: mmc1:0001 AJNB4R partition 2 4.00 MiB
mmcblk0rpmb: mmc1:0001 AJNB4R partition 3 4.00 MiB


Changes in v2:
- switch to HS400ES from Highspeed mode directly

Shawn Lin (6):
  Documentation: mmc: add mmc-hs400-enhanced-strobe
  mmc: core: add mmc-hs400-enhanced-strobe support
  mmc: core: implement enhanced strobe support
  mmc: debugfs: add HS400 enhanced strobe description
  mmc: sdhci: implement enhanced strobe callback
  mmc: sdhci-of-arasan: overwrite enhanced strobe callback

 Documentation/devicetree/bindings/mmc/mmc.txt |  1 +
 drivers/mmc/core/bus.c                        |  3 +-
 drivers/mmc/core/debugfs.c                    |  4 +-
 drivers/mmc/core/host.c                       |  2 +
 drivers/mmc/core/mmc.c                        | 77 ++++++++++++++++++++++++---
 drivers/mmc/host/sdhci-of-arasan.c            | 22 ++++++++
 drivers/mmc/host/sdhci.c                      | 11 ++++
 include/linux/mmc/card.h                      |  1 +
 include/linux/mmc/host.h                      | 18 +++++++
 include/linux/mmc/mmc.h                       |  3 ++
 10 files changed, 134 insertions(+), 8 deletions(-)

-- 
2.3.7

[PATCH v2 0/6] Add enhanced strobe support for emmc version 5.1 or later

[Shawn Lin]

Hello Ulf and Adrian,

This patch is going to support enhanced strobe function
for emmc version 5.1+ introduced by JEDEC recently.

Enchanced strobe is a optional function, so we add a new cap* for drivers to
decide whether to use it.

When introduing hs400 mode, JEDEC asks controllers to use data strobe line
to latch the data from emmc devives. But for cmd-reponse, not mentioned yet.
Since emmc version 5.1 published, JEDEC adds enhanced strobe function to deal
with cmd-response the same way as data-read. This feature is optional.

>From the spec(section 6.6.2.3), the standard scenario to select HS400 enhanced
strobe mode illustrated like this:
(1) set HS_TIMIMG (Highspeed)
(2) Host change freq to <= 52Mhz
(3) set the bus width to Enhanced strobe and DDR8Bit(CMD6), EXT_CSD[183] = 0x86
instead of 0x80
(4) set HS_TIMING to 0x3 (HS400)
(5) Host change freq to <= 200Mhz
(6) Host select HS400 enhanced strobe complete

I can't find a upstreamed controller claimed to support it, as well as the
mmc stack. But my "arasan,sdhci-5.1" actually supports this function. So I decide
to work for this part.

By looking into the SDHCI spec, I find there isn't any registers to enable the
enhanced strobe function. But from my "arasan,sdhci-5.1" databook, it describes a
register called VENDOR_REGISTER(0x78) to trigger this mode. So I guess other sdhci
variant drivers may also need s vendor specific register to deal with it.

If we are sure that our controller supports enhanced strobe mode, just add
mmc-hs400-enhanced-strobe in DT. Once emmc devices claims to support this mode,
we enable it automatically. Of course, other sdhci*/non-sdhci drivers should
implement/overwrite the prepare_enhanced_strobe by themselves. I believe all of the
platforms which need to support this mode should do some basic preparation for their
controllers. Currently I just limit the callback within ths scope of arasan,sdhci-5.1,
but I prone to believe arasan will use VENDOR_REGISTER to tirgger this mode from now on
because I do believe vendor will consider the registers' compatibility.

With this patchset applied, we can successfully run in HS400 enhanced strobe mode on
RK3399 platform with Samsung eMMC KLMBG2JENB-B041(v5.1, 16GB).

mmc1: new HS400 Enhanced strobe MMC card at address 0001
mmcblk0: mmc1:0001 AJNB4R 14.6 GiB
mmcblk0boot0: mmc1:0001 AJNB4R partition 1 4.00 MiB
mmcblk0boot1: mmc1:0001 AJNB4R partition 2 4.00 MiB
mmcblk0rpmb: mmc1:0001 AJNB4R partition 3 4.00 MiB


Changes in v2:
- switch to HS400ES from Highspeed mode directly

Shawn Lin (6):
  Documentation: mmc: add mmc-hs400-enhanced-strobe
  mmc: core: add mmc-hs400-enhanced-strobe support
  mmc: core: implement enhanced strobe support
  mmc: debugfs: add HS400 enhanced strobe description
  mmc: sdhci: implement enhanced strobe callback
  mmc: sdhci-of-arasan: overwrite enhanced strobe callback

 Documentation/devicetree/bindings/mmc/mmc.txt |  1 +
 drivers/mmc/core/bus.c                        |  3 +-
 drivers/mmc/core/debugfs.c                    |  4 +-
 drivers/mmc/core/host.c                       |  2 +
 drivers/mmc/core/mmc.c                        | 77 ++++++++++++++++++++++++---
 drivers/mmc/host/sdhci-of-arasan.c            | 22 ++++++++
 drivers/mmc/host/sdhci.c                      | 11 ++++
 include/linux/mmc/card.h                      |  1 +
 include/linux/mmc/host.h                      | 18 +++++++
 include/linux/mmc/mmc.h                       |  3 ++
 10 files changed, 134 insertions(+), 8 deletions(-)

-- 
2.3.7

[PATCH v2 1/6] Documentation: mmc: add mmc-hs400-enhanced-strobe

[Shawn Lin]

mmc-hs400-enhanced-strobe is used to claim that the
host can support hs400 mode with enhanced strobe
introduced by emmc 5.1 spec.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v2: None

 Documentation/devicetree/bindings/mmc/mmc.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
index ed23b9b..ecc007a 100644
--- a/Documentation/devicetree/bindings/mmc/mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc.txt
@@ -46,6 +46,7 @@ Optional properties:
 - mmc-hs200-1_2v: eMMC HS200 mode(1.2V I/O) is supported
 - mmc-hs400-1_8v: eMMC HS400 mode(1.8V I/O) is supported
 - mmc-hs400-1_2v: eMMC HS400 mode(1.2V I/O) is supported
+- mmc-hs400-enhanced-strobe: eMMC HS400 enhanced strobe mode is supported
 - dsr: Value the card's (optional) Driver Stage Register (DSR) should be
   programmed with. Valid range: [0 .. 0xffff].
 
-- 
2.3.7

[PATCH v2 1/6] Documentation: mmc: add mmc-hs400-enhanced-strobe

[Shawn Lin]

mmc-hs400-enhanced-strobe is used to claim that the
host can support hs400 mode with enhanced strobe
introduced by emmc 5.1 spec.

Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

Changes in v2: None

 Documentation/devicetree/bindings/mmc/mmc.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
index ed23b9b..ecc007a 100644
--- a/Documentation/devicetree/bindings/mmc/mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc.txt
@@ -46,6 +46,7 @@ Optional properties:
 - mmc-hs200-1_2v: eMMC HS200 mode(1.2V I/O) is supported
 - mmc-hs400-1_8v: eMMC HS400 mode(1.8V I/O) is supported
 - mmc-hs400-1_2v: eMMC HS400 mode(1.2V I/O) is supported
+- mmc-hs400-enhanced-strobe: eMMC HS400 enhanced strobe mode is supported
 - dsr: Value the card's (optional) Driver Stage Register (DSR) should be
   programmed with. Valid range: [0 .. 0xffff].
 
-- 
2.3.7

Re: [PATCH v2 1/6] Documentation: mmc: add mmc-hs400-enhanced-strobe

[Rob Herring]

On Fri, Apr 29, 2016 at 10:46:00AM +0800, Shawn Lin wrote:
> mmc-hs400-enhanced-strobe is used to claim that the
> host can support hs400 mode with enhanced strobe
> introduced by emmc 5.1 spec.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
> Changes in v2: None
> 
>  Documentation/devicetree/bindings/mmc/mmc.txt | 1 +
>  1 file changed, 1 insertion(+)

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v2 2/6] mmc: core: add mmc-hs400-enhanced-strobe support

[Shawn Lin]

This patch introduce mmc-hs400-enhanced-strobe for platforms
which want to enable enhanced strobe function from DT if the
mmc host controller claims to support enhanced strobe.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v2:
- switch to HS400ES from Highspeed mode directly

 drivers/mmc/core/host.c  | 2 ++
 include/linux/mmc/host.h | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index e0a3ee1..eb1530a 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -289,6 +289,8 @@ int mmc_of_parse(struct mmc_host *host)
 		host->caps2 |= MMC_CAP2_HS400_1_8V | MMC_CAP2_HS200_1_8V_SDR;
 	if (of_property_read_bool(np, "mmc-hs400-1_2v"))
 		host->caps2 |= MMC_CAP2_HS400_1_2V | MMC_CAP2_HS200_1_2V_SDR;
+	if (of_property_read_bool(np, "mmc-hs400-enhanced-strobe"))
+		host->caps2 |= MMC_CAP2_HS400_ENHANCED_STROBE;
 
 	host->dsr_req = !of_property_read_u32(np, "dsr", &host->dsr);
 	if (host->dsr_req && (host->dsr & ~0xffff)) {
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 85800b4..11e89d3 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -302,6 +302,7 @@ struct mmc_host {
 #define MMC_CAP2_SDIO_IRQ_NOTHREAD (1 << 17)
 #define MMC_CAP2_NO_WRITE_PROTECT (1 << 18)	/* No physical write protect pin, assume that card is always read-write */
 #define MMC_CAP2_NO_SDIO	(1 << 19)	/* Do not send SDIO commands during initialization */
+#define MMC_CAP2_HS400_ENHANCED_STROBE (1 << 20) /* Host supports enhanced strobe */
 
 	mmc_pm_flag_t		pm_caps;	/* supported pm features */
 
@@ -480,6 +481,11 @@ static inline int mmc_host_uhs(struct mmc_host *host)
 		 MMC_CAP_UHS_DDR50);
 }
 
+static inline int mmc_host_hs400_enhanced_strobe(struct mmc_host *host)
+{
+	return host->caps2 & MMC_CAP2_HS400_ENHANCED_STROBE;
+}
+
 static inline int mmc_host_packed_wr(struct mmc_host *host)
 {
 	return host->caps2 & MMC_CAP2_PACKED_WR;
-- 
2.3.7

Re: [PATCH v2 2/6] mmc: core: add mmc-hs400-enhanced-strobe support

[Ulf Hansson]

On 29 April 2016 at 04:46, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> This patch introduce mmc-hs400-enhanced-strobe for platforms
> which want to enable enhanced strobe function from DT if the
> mmc host controller claims to support enhanced strobe.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>
> ---
>
> Changes in v2:
> - switch to HS400ES from Highspeed mode directly
>
>  drivers/mmc/core/host.c  | 2 ++
>  include/linux/mmc/host.h | 6 ++++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index e0a3ee1..eb1530a 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -289,6 +289,8 @@ int mmc_of_parse(struct mmc_host *host)
>                 host->caps2 |= MMC_CAP2_HS400_1_8V | MMC_CAP2_HS200_1_8V_SDR;
>         if (of_property_read_bool(np, "mmc-hs400-1_2v"))
>                 host->caps2 |= MMC_CAP2_HS400_1_2V | MMC_CAP2_HS200_1_2V_SDR;
> +       if (of_property_read_bool(np, "mmc-hs400-enhanced-strobe"))
> +               host->caps2 |= MMC_CAP2_HS400_ENHANCED_STROBE;

How about MMC_CAP2_HS400_ES instead?

>
>         host->dsr_req = !of_property_read_u32(np, "dsr", &host->dsr);
>         if (host->dsr_req && (host->dsr & ~0xffff)) {
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 85800b4..11e89d3 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -302,6 +302,7 @@ struct mmc_host {
>  #define MMC_CAP2_SDIO_IRQ_NOTHREAD (1 << 17)
>  #define MMC_CAP2_NO_WRITE_PROTECT (1 << 18)    /* No physical write protect pin, assume that card is always read-write */
>  #define MMC_CAP2_NO_SDIO       (1 << 19)       /* Do not send SDIO commands during initialization */
> +#define MMC_CAP2_HS400_ENHANCED_STROBE (1 << 20) /* Host supports enhanced strobe */
>
>         mmc_pm_flag_t           pm_caps;        /* supported pm features */
>
> @@ -480,6 +481,11 @@ static inline int mmc_host_uhs(struct mmc_host *host)
>                  MMC_CAP_UHS_DDR50);
>  }
>
> +static inline int mmc_host_hs400_enhanced_strobe(struct mmc_host *host)
> +{
> +       return host->caps2 & MMC_CAP2_HS400_ENHANCED_STROBE;
> +}
> +

This helper function is to me a bit unnecessary, can you please just
check the MMC_CAP2_* when needed instead!?

>  static inline int mmc_host_packed_wr(struct mmc_host *host)
>  {
>         return host->caps2 & MMC_CAP2_PACKED_WR;

Kind regards
Uffe

Re: [PATCH v2 2/6] mmc: core: add mmc-hs400-enhanced-strobe support

[Shawn Lin]

On 2016/5/4 16:12, Ulf Hansson 写道:
> On 29 April 2016 at 04:46, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> This patch introduce mmc-hs400-enhanced-strobe for platforms
>> which want to enable enhanced strobe function from DT if the
>> mmc host controller claims to support enhanced strobe.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> ---
>>
>> Changes in v2:
>> - switch to HS400ES from Highspeed mode directly
>>
>>  drivers/mmc/core/host.c  | 2 ++
>>  include/linux/mmc/host.h | 6 ++++++
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
>> index e0a3ee1..eb1530a 100644
>> --- a/drivers/mmc/core/host.c
>> +++ b/drivers/mmc/core/host.c
>> @@ -289,6 +289,8 @@ int mmc_of_parse(struct mmc_host *host)
>>                 host->caps2 |= MMC_CAP2_HS400_1_8V | MMC_CAP2_HS200_1_8V_SDR;
>>         if (of_property_read_bool(np, "mmc-hs400-1_2v"))
>>                 host->caps2 |= MMC_CAP2_HS400_1_2V | MMC_CAP2_HS200_1_2V_SDR;
>> +       if (of_property_read_bool(np, "mmc-hs400-enhanced-strobe"))
>> +               host->caps2 |= MMC_CAP2_HS400_ENHANCED_STROBE;
>
> How about MMC_CAP2_HS400_ES instead?

MMC_CAP2_HS400_ES is fine to me.

>
>>
>>         host->dsr_req = !of_property_read_u32(np, "dsr", &host->dsr);
>>         if (host->dsr_req && (host->dsr & ~0xffff)) {
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index 85800b4..11e89d3 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -302,6 +302,7 @@ struct mmc_host {
>>  #define MMC_CAP2_SDIO_IRQ_NOTHREAD (1 << 17)
>>  #define MMC_CAP2_NO_WRITE_PROTECT (1 << 18)    /* No physical write protect pin, assume that card is always read-write */
>>  #define MMC_CAP2_NO_SDIO       (1 << 19)       /* Do not send SDIO commands during initialization */
>> +#define MMC_CAP2_HS400_ENHANCED_STROBE (1 << 20) /* Host supports enhanced strobe */
>>
>>         mmc_pm_flag_t           pm_caps;        /* supported pm features */
>>
>> @@ -480,6 +481,11 @@ static inline int mmc_host_uhs(struct mmc_host *host)
>>                  MMC_CAP_UHS_DDR50);
>>  }
>>
>> +static inline int mmc_host_hs400_enhanced_strobe(struct mmc_host *host)
>> +{
>> +       return host->caps2 & MMC_CAP2_HS400_ENHANCED_STROBE;
>> +}
>> +
>
> This helper function is to me a bit unnecessary, can you please just
> check the MMC_CAP2_* when needed instead!?

okay, I will deal with these two comments above.

Thanks.

>
>>  static inline int mmc_host_packed_wr(struct mmc_host *host)
>>  {
>>         return host->caps2 & MMC_CAP2_PACKED_WR;
>
> Kind regards
> Uffe
>
>
>


-- 
Best Regards
Shawn Lin

[PATCH v2 3/6] mmc: core: implement enhanced strobe support

[Shawn Lin]

Controllers use data strobe line to latch data from devices
under hs400 mode, but not for cmd line. So since emmc 5.1, JEDEC
introduces enhanced strobe mode for latching cmd response from
emmc devices to host controllers. This new feature is optional,
so it depends both on device's cap and host's cap to decide
whether to use it or not.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v2: None

 drivers/mmc/core/bus.c   |  3 +-
 drivers/mmc/core/mmc.c   | 77 ++++++++++++++++++++++++++++++++++++++++++++----
 include/linux/mmc/card.h |  1 +
 include/linux/mmc/host.h | 12 ++++++++
 include/linux/mmc/mmc.h  |  3 ++
 5 files changed, 89 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 4bc48f1..7e94b9d 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -332,12 +332,13 @@ int mmc_add_card(struct mmc_card *card)
 			mmc_card_ddr52(card) ? "DDR " : "",
 			type);
 	} else {
-		pr_info("%s: new %s%s%s%s%s card at address %04x\n",
+		pr_info("%s: new %s%s%s%s%s%s card at address %04x\n",
 			mmc_hostname(card->host),
 			mmc_card_uhs(card) ? "ultra high speed " :
 			(mmc_card_hs(card) ? "high speed " : ""),
 			mmc_card_hs400(card) ? "HS400 " :
 			(mmc_card_hs200(card) ? "HS200 " : ""),
+			mmc_card_hs400es(card) ? "Enhanced strobe" : "",
 			mmc_card_ddr52(card) ? "DDR " : "",
 			uhs_bus_speed_mode, type, card->rca);
 	}
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 28b477d..f45ed10 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -235,6 +235,12 @@ static void mmc_select_card_type(struct mmc_card *card)
 		avail_type |= EXT_CSD_CARD_TYPE_HS400_1_2V;
 	}
 
+	if ((caps2 & MMC_CAP2_HS400_ENHANCED_STROBE) &&
+	    card->ext_csd.strobe_support &&
+	    ((card_type & EXT_CSD_CARD_TYPE_HS400_1_2V) ||
+	     (card_type & EXT_CSD_CARD_TYPE_HS400_1_8V)))
+		avail_type |= EXT_CSD_CARD_TYPE_HS400ES;
+
 	card->ext_csd.hs_max_dtr = hs_max_dtr;
 	card->ext_csd.hs200_max_dtr = hs200_max_dtr;
 	card->mmc_avail_type = avail_type;
@@ -383,6 +389,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
 			mmc_card_set_blockaddr(card);
 	}
 
+	card->ext_csd.strobe_support = ext_csd[EXT_CSD_STROBE_SUPPORT];
 	card->ext_csd.raw_card_type = ext_csd[EXT_CSD_CARD_TYPE];
 	mmc_select_card_type(card);
 
@@ -1062,9 +1069,10 @@ static int mmc_select_hs400(struct mmc_card *card)
 	u8 val;
 
 	/*
-	 * HS400 mode requires 8-bit bus width
+	 * HS400(ES) mode requires 8-bit bus width
 	 */
-	if (!(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400 &&
+	if (!(((card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400) ||
+		(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)) &&
 	      host->ios.bus_width == MMC_BUS_WIDTH_8))
 		return 0;
 
@@ -1097,9 +1105,26 @@ static int mmc_select_hs400(struct mmc_card *card)
 	}
 
 	/* Switch card to DDR */
+	val = EXT_CSD_DDR_BUS_WIDTH_8;
+	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
+		val |= EXT_CSD_BUS_WIDTH_STROBE;
+		/*
+		* Make sure we are in non-enhanced strobe mode before we
+		* actually enable it in ext_csd.
+		*/
+		if (host->ops->prepare_enhanced_strobe)
+			err = host->ops->prepare_enhanced_strobe(host, false);
+
+		if (err) {
+			pr_err("%s: unprepare_enhanced strobe failed, err:%d\n",
+				mmc_hostname(host), err);
+			return err;
+		}
+	}
+
 	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			 EXT_CSD_BUS_WIDTH,
-			 EXT_CSD_DDR_BUS_WIDTH_8,
+			 val,
 			 card->ext_csd.generic_cmd6_time);
 	if (err) {
 		pr_err("%s: switch to bus width for hs400 failed, err:%d\n",
@@ -1124,6 +1149,35 @@ static int mmc_select_hs400(struct mmc_card *card)
 	mmc_set_timing(host, MMC_TIMING_MMC_HS400);
 	mmc_set_bus_speed(card);
 
+	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
+		/* Controller enable enhanced strobe function */
+		if (host->ops->prepare_enhanced_strobe)
+			err = host->ops->prepare_enhanced_strobe(host, true);
+
+		if (err) {
+			pr_err("%s: prepare enhanced strobe failed, err:%d\n",
+				mmc_hostname(host), err);
+			return err;
+		}
+
+		/*
+		 * After switching to hs400 enhanced strobe mode, we expect to
+		 * verify whether it works or not. If controller can't handle
+		 * bus width test, compare ext_csd previously read in 1 bit mode
+		 * against ext_csd at new bus width
+		 */
+		if (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))
+			err = mmc_compare_ext_csds(card, MMC_BUS_WIDTH_8);
+		else
+			err = mmc_bus_test(card, MMC_BUS_WIDTH_8);
+
+		if (err) {
+			pr_warn("%s: switch to enhanced strobe failed\n",
+				mmc_hostname(host));
+			return err;
+		}
+	}
+
 	if (!send_status) {
 		err = mmc_switch_status(card);
 		if (err)
@@ -1297,7 +1351,8 @@ err:
 }
 
 /*
- * Activate High Speed or HS200 mode if supported.
+ * Activate High Speed or HS200 mode if supported. For HS400
+ * with enhanced strobe mode, we should activate High Speed.
  */
 static int mmc_select_timing(struct mmc_card *card)
 {
@@ -1306,7 +1361,9 @@ static int mmc_select_timing(struct mmc_card *card)
 	if (!mmc_can_ext_csd(card))
 		goto bus_speed;
 
-	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
+	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)
+		err = mmc_select_hs(card);
+	else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
 		err = mmc_select_hs200(card);
 	else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
 		err = mmc_select_hs(card);
@@ -1578,7 +1635,15 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
 	} else if (mmc_card_hs(card)) {
 		/* Select the desired bus width optionally */
 		err = mmc_select_bus_width(card);
-		if (!IS_ERR_VALUE(err)) {
+		if (IS_ERR_VALUE(err))
+			goto free_card;
+
+		if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
+			/* Directly from HS to HS400-ES */
+			err = mmc_select_hs400(card);
+			if (err)
+				goto free_card;
+		} else {
 			err = mmc_select_hs_ddr(card);
 			if (err)
 				goto free_card;
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index eb0151b..22defc2 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -95,6 +95,7 @@ struct mmc_ext_csd {
 	u8			raw_partition_support;	/* 160 */
 	u8			raw_rpmb_size_mult;	/* 168 */
 	u8			raw_erased_mem_count;	/* 181 */
+	u8			strobe_support;		/* 184 */
 	u8			raw_ext_csd_structure;	/* 194 */
 	u8			raw_card_type;		/* 196 */
 	u8			raw_driver_strength;	/* 197 */
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 11e89d3..19de56c 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -19,6 +19,7 @@
 
 #include <linux/mmc/core.h>
 #include <linux/mmc/card.h>
+#include <linux/mmc/mmc.h>
 #include <linux/mmc/pm.h>
 
 struct mmc_ios {
@@ -143,6 +144,8 @@ struct mmc_host_ops {
 
 	/* Prepare HS400 target operating frequency depending host driver */
 	int	(*prepare_hs400_tuning)(struct mmc_host *host, struct mmc_ios *ios);
+	/* Prepare enhanced strobe depending host driver */
+	int	(*prepare_enhanced_strobe)(struct mmc_host *host, bool enable);
 	int	(*select_drive_strength)(struct mmc_card *card,
 					 unsigned int max_dtr, int host_drv,
 					 int card_drv, int *drv_type);
@@ -518,6 +521,15 @@ static inline bool mmc_card_hs400(struct mmc_card *card)
 	return card->host->ios.timing == MMC_TIMING_MMC_HS400;
 }
 
+static inline bool mmc_card_hs400es(struct mmc_card *card)
+{
+	if (mmc_card_hs400(card) &&
+	    (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES))
+		return 1;
+
+	return 0;
+}
+
 void mmc_retune_timer_stop(struct mmc_host *host);
 
 static inline void mmc_retune_needed(struct mmc_host *host)
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
index 15f2c4a..c376209 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -297,6 +297,7 @@ struct _mmc_csd {
 #define EXT_CSD_PART_CONFIG		179	/* R/W */
 #define EXT_CSD_ERASED_MEM_CONT		181	/* RO */
 #define EXT_CSD_BUS_WIDTH		183	/* R/W */
+#define EXT_CSD_STROBE_SUPPORT		184	/* RO */
 #define EXT_CSD_HS_TIMING		185	/* R/W */
 #define EXT_CSD_POWER_CLASS		187	/* R/W */
 #define EXT_CSD_REV			192	/* RO */
@@ -380,12 +381,14 @@ struct _mmc_csd {
 #define EXT_CSD_CARD_TYPE_HS400_1_2V	(1<<7)	/* Card can run at 200MHz DDR, 1.2V */
 #define EXT_CSD_CARD_TYPE_HS400		(EXT_CSD_CARD_TYPE_HS400_1_8V | \
 					 EXT_CSD_CARD_TYPE_HS400_1_2V)
+#define EXT_CSD_CARD_TYPE_HS400ES	(1<<8)	/* Card can run at HS400ES */
 
 #define EXT_CSD_BUS_WIDTH_1	0	/* Card is in 1 bit mode */
 #define EXT_CSD_BUS_WIDTH_4	1	/* Card is in 4 bit mode */
 #define EXT_CSD_BUS_WIDTH_8	2	/* Card is in 8 bit mode */
 #define EXT_CSD_DDR_BUS_WIDTH_4	5	/* Card is in 4 bit DDR mode */
 #define EXT_CSD_DDR_BUS_WIDTH_8	6	/* Card is in 8 bit DDR mode */
+#define EXT_CSD_BUS_WIDTH_STROBE BIT(7)	/* Enhanced strobe mode */
 
 #define EXT_CSD_TIMING_BC	0	/* Backwards compatility */
 #define EXT_CSD_TIMING_HS	1	/* High speed */
-- 
2.3.7

[PATCH v2 3/6] mmc: core: implement enhanced strobe support

[Shawn Lin]

Controllers use data strobe line to latch data from devices
under hs400 mode, but not for cmd line. So since emmc 5.1, JEDEC
introduces enhanced strobe mode for latching cmd response from
emmc devices to host controllers. This new feature is optional,
so it depends both on device's cap and host's cap to decide
whether to use it or not.

Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

Changes in v2: None

 drivers/mmc/core/bus.c   |  3 +-
 drivers/mmc/core/mmc.c   | 77 ++++++++++++++++++++++++++++++++++++++++++++----
 include/linux/mmc/card.h |  1 +
 include/linux/mmc/host.h | 12 ++++++++
 include/linux/mmc/mmc.h  |  3 ++
 5 files changed, 89 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 4bc48f1..7e94b9d 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -332,12 +332,13 @@ int mmc_add_card(struct mmc_card *card)
 			mmc_card_ddr52(card) ? "DDR " : "",
 			type);
 	} else {
-		pr_info("%s: new %s%s%s%s%s card at address %04x\n",
+		pr_info("%s: new %s%s%s%s%s%s card at address %04x\n",
 			mmc_hostname(card->host),
 			mmc_card_uhs(card) ? "ultra high speed " :
 			(mmc_card_hs(card) ? "high speed " : ""),
 			mmc_card_hs400(card) ? "HS400 " :
 			(mmc_card_hs200(card) ? "HS200 " : ""),
+			mmc_card_hs400es(card) ? "Enhanced strobe" : "",
 			mmc_card_ddr52(card) ? "DDR " : "",
 			uhs_bus_speed_mode, type, card->rca);
 	}
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 28b477d..f45ed10 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -235,6 +235,12 @@ static void mmc_select_card_type(struct mmc_card *card)
 		avail_type |= EXT_CSD_CARD_TYPE_HS400_1_2V;
 	}
 
+	if ((caps2 & MMC_CAP2_HS400_ENHANCED_STROBE) &&
+	    card->ext_csd.strobe_support &&
+	    ((card_type & EXT_CSD_CARD_TYPE_HS400_1_2V) ||
+	     (card_type & EXT_CSD_CARD_TYPE_HS400_1_8V)))
+		avail_type |= EXT_CSD_CARD_TYPE_HS400ES;
+
 	card->ext_csd.hs_max_dtr = hs_max_dtr;
 	card->ext_csd.hs200_max_dtr = hs200_max_dtr;
 	card->mmc_avail_type = avail_type;
@@ -383,6 +389,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
 			mmc_card_set_blockaddr(card);
 	}
 
+	card->ext_csd.strobe_support = ext_csd[EXT_CSD_STROBE_SUPPORT];
 	card->ext_csd.raw_card_type = ext_csd[EXT_CSD_CARD_TYPE];
 	mmc_select_card_type(card);
 
@@ -1062,9 +1069,10 @@ static int mmc_select_hs400(struct mmc_card *card)
 	u8 val;
 
 	/*
-	 * HS400 mode requires 8-bit bus width
+	 * HS400(ES) mode requires 8-bit bus width
 	 */
-	if (!(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400 &&
+	if (!(((card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400) ||
+		(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)) &&
 	      host->ios.bus_width == MMC_BUS_WIDTH_8))
 		return 0;
 
@@ -1097,9 +1105,26 @@ static int mmc_select_hs400(struct mmc_card *card)
 	}
 
 	/* Switch card to DDR */
+	val = EXT_CSD_DDR_BUS_WIDTH_8;
+	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
+		val |= EXT_CSD_BUS_WIDTH_STROBE;
+		/*
+		* Make sure we are in non-enhanced strobe mode before we
+		* actually enable it in ext_csd.
+		*/
+		if (host->ops->prepare_enhanced_strobe)
+			err = host->ops->prepare_enhanced_strobe(host, false);
+
+		if (err) {
+			pr_err("%s: unprepare_enhanced strobe failed, err:%d\n",
+				mmc_hostname(host), err);
+			return err;
+		}
+	}
+
 	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			 EXT_CSD_BUS_WIDTH,
-			 EXT_CSD_DDR_BUS_WIDTH_8,
+			 val,
 			 card->ext_csd.generic_cmd6_time);
 	if (err) {
 		pr_err("%s: switch to bus width for hs400 failed, err:%d\n",
@@ -1124,6 +1149,35 @@ static int mmc_select_hs400(struct mmc_card *card)
 	mmc_set_timing(host, MMC_TIMING_MMC_HS400);
 	mmc_set_bus_speed(card);
 
+	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
+		/* Controller enable enhanced strobe function */
+		if (host->ops->prepare_enhanced_strobe)
+			err = host->ops->prepare_enhanced_strobe(host, true);
+
+		if (err) {
+			pr_err("%s: prepare enhanced strobe failed, err:%d\n",
+				mmc_hostname(host), err);
+			return err;
+		}
+
+		/*
+		 * After switching to hs400 enhanced strobe mode, we expect to
+		 * verify whether it works or not. If controller can't handle
+		 * bus width test, compare ext_csd previously read in 1 bit mode
+		 * against ext_csd at new bus width
+		 */
+		if (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))
+			err = mmc_compare_ext_csds(card, MMC_BUS_WIDTH_8);
+		else
+			err = mmc_bus_test(card, MMC_BUS_WIDTH_8);
+
+		if (err) {
+			pr_warn("%s: switch to enhanced strobe failed\n",
+				mmc_hostname(host));
+			return err;
+		}
+	}
+
 	if (!send_status) {
 		err = mmc_switch_status(card);
 		if (err)
@@ -1297,7 +1351,8 @@ err:
 }
 
 /*
- * Activate High Speed or HS200 mode if supported.
+ * Activate High Speed or HS200 mode if supported. For HS400
+ * with enhanced strobe mode, we should activate High Speed.
  */
 static int mmc_select_timing(struct mmc_card *card)
 {
@@ -1306,7 +1361,9 @@ static int mmc_select_timing(struct mmc_card *card)
 	if (!mmc_can_ext_csd(card))
 		goto bus_speed;
 
-	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
+	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)
+		err = mmc_select_hs(card);
+	else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
 		err = mmc_select_hs200(card);
 	else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
 		err = mmc_select_hs(card);
@@ -1578,7 +1635,15 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
 	} else if (mmc_card_hs(card)) {
 		/* Select the desired bus width optionally */
 		err = mmc_select_bus_width(card);
-		if (!IS_ERR_VALUE(err)) {
+		if (IS_ERR_VALUE(err))
+			goto free_card;
+
+		if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
+			/* Directly from HS to HS400-ES */
+			err = mmc_select_hs400(card);
+			if (err)
+				goto free_card;
+		} else {
 			err = mmc_select_hs_ddr(card);
 			if (err)
 				goto free_card;
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index eb0151b..22defc2 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -95,6 +95,7 @@ struct mmc_ext_csd {
 	u8			raw_partition_support;	/* 160 */
 	u8			raw_rpmb_size_mult;	/* 168 */
 	u8			raw_erased_mem_count;	/* 181 */
+	u8			strobe_support;		/* 184 */
 	u8			raw_ext_csd_structure;	/* 194 */
 	u8			raw_card_type;		/* 196 */
 	u8			raw_driver_strength;	/* 197 */
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 11e89d3..19de56c 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -19,6 +19,7 @@
 
 #include <linux/mmc/core.h>
 #include <linux/mmc/card.h>
+#include <linux/mmc/mmc.h>
 #include <linux/mmc/pm.h>
 
 struct mmc_ios {
@@ -143,6 +144,8 @@ struct mmc_host_ops {
 
 	/* Prepare HS400 target operating frequency depending host driver */
 	int	(*prepare_hs400_tuning)(struct mmc_host *host, struct mmc_ios *ios);
+	/* Prepare enhanced strobe depending host driver */
+	int	(*prepare_enhanced_strobe)(struct mmc_host *host, bool enable);
 	int	(*select_drive_strength)(struct mmc_card *card,
 					 unsigned int max_dtr, int host_drv,
 					 int card_drv, int *drv_type);
@@ -518,6 +521,15 @@ static inline bool mmc_card_hs400(struct mmc_card *card)
 	return card->host->ios.timing == MMC_TIMING_MMC_HS400;
 }
 
+static inline bool mmc_card_hs400es(struct mmc_card *card)
+{
+	if (mmc_card_hs400(card) &&
+	    (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES))
+		return 1;
+
+	return 0;
+}
+
 void mmc_retune_timer_stop(struct mmc_host *host);
 
 static inline void mmc_retune_needed(struct mmc_host *host)
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
index 15f2c4a..c376209 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -297,6 +297,7 @@ struct _mmc_csd {
 #define EXT_CSD_PART_CONFIG		179	/* R/W */
 #define EXT_CSD_ERASED_MEM_CONT		181	/* RO */
 #define EXT_CSD_BUS_WIDTH		183	/* R/W */
+#define EXT_CSD_STROBE_SUPPORT		184	/* RO */
 #define EXT_CSD_HS_TIMING		185	/* R/W */
 #define EXT_CSD_POWER_CLASS		187	/* R/W */
 #define EXT_CSD_REV			192	/* RO */
@@ -380,12 +381,14 @@ struct _mmc_csd {
 #define EXT_CSD_CARD_TYPE_HS400_1_2V	(1<<7)	/* Card can run at 200MHz DDR, 1.2V */
 #define EXT_CSD_CARD_TYPE_HS400		(EXT_CSD_CARD_TYPE_HS400_1_8V | \
 					 EXT_CSD_CARD_TYPE_HS400_1_2V)
+#define EXT_CSD_CARD_TYPE_HS400ES	(1<<8)	/* Card can run at HS400ES */
 
 #define EXT_CSD_BUS_WIDTH_1	0	/* Card is in 1 bit mode */
 #define EXT_CSD_BUS_WIDTH_4	1	/* Card is in 4 bit mode */
 #define EXT_CSD_BUS_WIDTH_8	2	/* Card is in 8 bit mode */
 #define EXT_CSD_DDR_BUS_WIDTH_4	5	/* Card is in 4 bit DDR mode */
 #define EXT_CSD_DDR_BUS_WIDTH_8	6	/* Card is in 8 bit DDR mode */
+#define EXT_CSD_BUS_WIDTH_STROBE BIT(7)	/* Enhanced strobe mode */
 
 #define EXT_CSD_TIMING_BC	0	/* Backwards compatility */
 #define EXT_CSD_TIMING_HS	1	/* High speed */
-- 
2.3.7

Re: [PATCH v2 3/6] mmc: core: implement enhanced strobe support

[Ulf Hansson]

On 29 April 2016 at 04:47, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> Controllers use data strobe line to latch data from devices
> under hs400 mode, but not for cmd line. So since emmc 5.1, JEDEC
> introduces enhanced strobe mode for latching cmd response from
> emmc devices to host controllers. This new feature is optional,
> so it depends both on device's cap and host's cap to decide
> whether to use it or not.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
> Changes in v2: None
>
>  drivers/mmc/core/bus.c   |  3 +-
>  drivers/mmc/core/mmc.c   | 77 ++++++++++++++++++++++++++++++++++++++++++++----
>  include/linux/mmc/card.h |  1 +
>  include/linux/mmc/host.h | 12 ++++++++
>  include/linux/mmc/mmc.h  |  3 ++
>  5 files changed, 89 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> index 4bc48f1..7e94b9d 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -332,12 +332,13 @@ int mmc_add_card(struct mmc_card *card)
>                         mmc_card_ddr52(card) ? "DDR " : "",
>                         type);
>         } else {
> -               pr_info("%s: new %s%s%s%s%s card at address %04x\n",
> +               pr_info("%s: new %s%s%s%s%s%s card at address %04x\n",
>                         mmc_hostname(card->host),
>                         mmc_card_uhs(card) ? "ultra high speed " :
>                         (mmc_card_hs(card) ? "high speed " : ""),
>                         mmc_card_hs400(card) ? "HS400 " :
>                         (mmc_card_hs200(card) ? "HS200 " : ""),
> +                       mmc_card_hs400es(card) ? "Enhanced strobe" : "",

I am not sure this informations should be printed. It's still and
HS400 card, there is no performance impact, right?

>                         mmc_card_ddr52(card) ? "DDR " : "",
>                         uhs_bus_speed_mode, type, card->rca);
>         }
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 28b477d..f45ed10 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -235,6 +235,12 @@ static void mmc_select_card_type(struct mmc_card *card)
>                 avail_type |= EXT_CSD_CARD_TYPE_HS400_1_2V;
>         }
>
> +       if ((caps2 & MMC_CAP2_HS400_ENHANCED_STROBE) &&
> +           card->ext_csd.strobe_support &&
> +           ((card_type & EXT_CSD_CARD_TYPE_HS400_1_2V) ||
> +            (card_type & EXT_CSD_CARD_TYPE_HS400_1_8V)))
> +               avail_type |= EXT_CSD_CARD_TYPE_HS400ES;
> +
>         card->ext_csd.hs_max_dtr = hs_max_dtr;
>         card->ext_csd.hs200_max_dtr = hs200_max_dtr;
>         card->mmc_avail_type = avail_type;
> @@ -383,6 +389,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
>                         mmc_card_set_blockaddr(card);
>         }
>
> +       card->ext_csd.strobe_support = ext_csd[EXT_CSD_STROBE_SUPPORT];
>         card->ext_csd.raw_card_type = ext_csd[EXT_CSD_CARD_TYPE];
>         mmc_select_card_type(card);
>
> @@ -1062,9 +1069,10 @@ static int mmc_select_hs400(struct mmc_card *card)
>         u8 val;
>
>         /*
> -        * HS400 mode requires 8-bit bus width
> +        * HS400(ES) mode requires 8-bit bus width
>          */
> -       if (!(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400 &&
> +       if (!(((card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400) ||
> +               (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)) &&
>               host->ios.bus_width == MMC_BUS_WIDTH_8))
>                 return 0;
>
> @@ -1097,9 +1105,26 @@ static int mmc_select_hs400(struct mmc_card *card)
>         }
>
>         /* Switch card to DDR */
> +       val = EXT_CSD_DDR_BUS_WIDTH_8;
> +       if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
> +               val |= EXT_CSD_BUS_WIDTH_STROBE;
> +               /*
> +               * Make sure we are in non-enhanced strobe mode before we
> +               * actually enable it in ext_csd.
> +               */
> +               if (host->ops->prepare_enhanced_strobe)
> +                       err = host->ops->prepare_enhanced_strobe(host, false);

1) I suggest we rename the callback to "hs400_enhanced_strobe".

2) I think this might be is a bit late to deal with restoring enhanced
strobe to off. Perhaps we should do that in mmc_set_initial_state()
instead!?

> +
> +               if (err) {
> +                       pr_err("%s: unprepare_enhanced strobe failed, err:%d\n",

Maybe pr_dbg instead?

/s/unprepare_enhanced strobe/Disable hs400 es

> +                               mmc_hostname(host), err);
> +                       return err;
> +               }
> +       }
> +
>         err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>                          EXT_CSD_BUS_WIDTH,
> -                        EXT_CSD_DDR_BUS_WIDTH_8,
> +                        val,
>                          card->ext_csd.generic_cmd6_time);
>         if (err) {
>                 pr_err("%s: switch to bus width for hs400 failed, err:%d\n",
> @@ -1124,6 +1149,35 @@ static int mmc_select_hs400(struct mmc_card *card)
>         mmc_set_timing(host, MMC_TIMING_MMC_HS400);
>         mmc_set_bus_speed(card);
>
> +       if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
> +               /* Controller enable enhanced strobe function */
> +               if (host->ops->prepare_enhanced_strobe)
> +                       err = host->ops->prepare_enhanced_strobe(host, true);
> +
> +               if (err) {
> +                       pr_err("%s: prepare enhanced strobe failed, err:%d\n",

Maybe pr_dbg/warn instead?

/s/prepare enhanced strobe/Enable hs400 es

> +                               mmc_hostname(host), err);
> +                       return err;
> +               }
> +
> +               /*
> +                * After switching to hs400 enhanced strobe mode, we expect to
> +                * verify whether it works or not. If controller can't handle
> +                * bus width test, compare ext_csd previously read in 1 bit mode
> +                * against ext_csd at new bus width
> +                */

Is this according to spec? If so, we should probably state that here.
If not, further explanation for *why* we do this is needed.

> +               if (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))
> +                       err = mmc_compare_ext_csds(card, MMC_BUS_WIDTH_8);
> +               else
> +                       err = mmc_bus_test(card, MMC_BUS_WIDTH_8);
> +
> +               if (err) {
> +                       pr_warn("%s: switch to enhanced strobe failed\n",
> +                               mmc_hostname(host));
> +                       return err;
> +               }
> +       }
> +
>         if (!send_status) {
>                 err = mmc_switch_status(card);
>                 if (err)
> @@ -1297,7 +1351,8 @@ err:
>  }
>
>  /*
> - * Activate High Speed or HS200 mode if supported.
> + * Activate High Speed or HS200 mode if supported. For HS400
> + * with enhanced strobe mode, we should activate High Speed.
>   */
>  static int mmc_select_timing(struct mmc_card *card)
>  {
> @@ -1306,7 +1361,9 @@ static int mmc_select_timing(struct mmc_card *card)
>         if (!mmc_can_ext_csd(card))
>                 goto bus_speed;
>
> -       if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
> +       if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)
> +               err = mmc_select_hs(card);
> +       else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
>                 err = mmc_select_hs200(card);
>         else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
>                 err = mmc_select_hs(card);
> @@ -1578,7 +1635,15 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>         } else if (mmc_card_hs(card)) {
>                 /* Select the desired bus width optionally */
>                 err = mmc_select_bus_width(card);
> -               if (!IS_ERR_VALUE(err)) {
> +               if (IS_ERR_VALUE(err))
> +                       goto free_card;

This is going to change the behaviour, as earlier switching the bus
width was optional. See the comment a few lines above and the
implementation of mmc_select_bus_width().

> +
> +               if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
> +                       /* Directly from HS to HS400-ES */
> +                       err = mmc_select_hs400(card);
> +                       if (err)
> +                               goto free_card;
> +               } else {
>                         err = mmc_select_hs_ddr(card);
>                         if (err)
>                                 goto free_card;
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index eb0151b..22defc2 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -95,6 +95,7 @@ struct mmc_ext_csd {
>         u8                      raw_partition_support;  /* 160 */
>         u8                      raw_rpmb_size_mult;     /* 168 */
>         u8                      raw_erased_mem_count;   /* 181 */
> +       u8                      strobe_support;         /* 184 */
>         u8                      raw_ext_csd_structure;  /* 194 */
>         u8                      raw_card_type;          /* 196 */
>         u8                      raw_driver_strength;    /* 197 */
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 11e89d3..19de56c 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -19,6 +19,7 @@
>
>  #include <linux/mmc/core.h>
>  #include <linux/mmc/card.h>
> +#include <linux/mmc/mmc.h>
>  #include <linux/mmc/pm.h>
>
>  struct mmc_ios {
> @@ -143,6 +144,8 @@ struct mmc_host_ops {
>
>         /* Prepare HS400 target operating frequency depending host driver */
>         int     (*prepare_hs400_tuning)(struct mmc_host *host, struct mmc_ios *ios);
> +       /* Prepare enhanced strobe depending host driver */
> +       int     (*prepare_enhanced_strobe)(struct mmc_host *host, bool enable);
>         int     (*select_drive_strength)(struct mmc_card *card,
>                                          unsigned int max_dtr, int host_drv,
>                                          int card_drv, int *drv_type);
> @@ -518,6 +521,15 @@ static inline bool mmc_card_hs400(struct mmc_card *card)
>         return card->host->ios.timing == MMC_TIMING_MMC_HS400;
>  }
>
> +static inline bool mmc_card_hs400es(struct mmc_card *card)
> +{
> +       if (mmc_card_hs400(card) &&
> +           (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES))
> +               return 1;
> +
> +       return 0;
> +}
> +
>  void mmc_retune_timer_stop(struct mmc_host *host);
>
>  static inline void mmc_retune_needed(struct mmc_host *host)
> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
> index 15f2c4a..c376209 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -297,6 +297,7 @@ struct _mmc_csd {
>  #define EXT_CSD_PART_CONFIG            179     /* R/W */
>  #define EXT_CSD_ERASED_MEM_CONT                181     /* RO */
>  #define EXT_CSD_BUS_WIDTH              183     /* R/W */
> +#define EXT_CSD_STROBE_SUPPORT         184     /* RO */
>  #define EXT_CSD_HS_TIMING              185     /* R/W */
>  #define EXT_CSD_POWER_CLASS            187     /* R/W */
>  #define EXT_CSD_REV                    192     /* RO */
> @@ -380,12 +381,14 @@ struct _mmc_csd {
>  #define EXT_CSD_CARD_TYPE_HS400_1_2V   (1<<7)  /* Card can run at 200MHz DDR, 1.2V */
>  #define EXT_CSD_CARD_TYPE_HS400                (EXT_CSD_CARD_TYPE_HS400_1_8V | \
>                                          EXT_CSD_CARD_TYPE_HS400_1_2V)
> +#define EXT_CSD_CARD_TYPE_HS400ES      (1<<8)  /* Card can run at HS400ES */
>
>  #define EXT_CSD_BUS_WIDTH_1    0       /* Card is in 1 bit mode */
>  #define EXT_CSD_BUS_WIDTH_4    1       /* Card is in 4 bit mode */
>  #define EXT_CSD_BUS_WIDTH_8    2       /* Card is in 8 bit mode */
>  #define EXT_CSD_DDR_BUS_WIDTH_4        5       /* Card is in 4 bit DDR mode */
>  #define EXT_CSD_DDR_BUS_WIDTH_8        6       /* Card is in 8 bit DDR mode */
> +#define EXT_CSD_BUS_WIDTH_STROBE BIT(7)        /* Enhanced strobe mode */
>
>  #define EXT_CSD_TIMING_BC      0       /* Backwards compatility */
>  #define EXT_CSD_TIMING_HS      1       /* High speed */
> --
> 2.3.7
>
>

Kind regards
Uffe

Re: [PATCH v2 3/6] mmc: core: implement enhanced strobe support

[Ulf Hansson]

On 29 April 2016 at 04:47, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
> Controllers use data strobe line to latch data from devices
> under hs400 mode, but not for cmd line. So since emmc 5.1, JEDEC
> introduces enhanced strobe mode for latching cmd response from
> emmc devices to host controllers. This new feature is optional,
> so it depends both on device's cap and host's cap to decide
> whether to use it or not.
>
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
>
> Changes in v2: None
>
>  drivers/mmc/core/bus.c   |  3 +-
>  drivers/mmc/core/mmc.c   | 77 ++++++++++++++++++++++++++++++++++++++++++++----
>  include/linux/mmc/card.h |  1 +
>  include/linux/mmc/host.h | 12 ++++++++
>  include/linux/mmc/mmc.h  |  3 ++
>  5 files changed, 89 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> index 4bc48f1..7e94b9d 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -332,12 +332,13 @@ int mmc_add_card(struct mmc_card *card)
>                         mmc_card_ddr52(card) ? "DDR " : "",
>                         type);
>         } else {
> -               pr_info("%s: new %s%s%s%s%s card at address %04x\n",
> +               pr_info("%s: new %s%s%s%s%s%s card at address %04x\n",
>                         mmc_hostname(card->host),
>                         mmc_card_uhs(card) ? "ultra high speed " :
>                         (mmc_card_hs(card) ? "high speed " : ""),
>                         mmc_card_hs400(card) ? "HS400 " :
>                         (mmc_card_hs200(card) ? "HS200 " : ""),
> +                       mmc_card_hs400es(card) ? "Enhanced strobe" : "",

I am not sure this informations should be printed. It's still and
HS400 card, there is no performance impact, right?

>                         mmc_card_ddr52(card) ? "DDR " : "",
>                         uhs_bus_speed_mode, type, card->rca);
>         }
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 28b477d..f45ed10 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -235,6 +235,12 @@ static void mmc_select_card_type(struct mmc_card *card)
>                 avail_type |= EXT_CSD_CARD_TYPE_HS400_1_2V;
>         }
>
> +       if ((caps2 & MMC_CAP2_HS400_ENHANCED_STROBE) &&
> +           card->ext_csd.strobe_support &&
> +           ((card_type & EXT_CSD_CARD_TYPE_HS400_1_2V) ||
> +            (card_type & EXT_CSD_CARD_TYPE_HS400_1_8V)))
> +               avail_type |= EXT_CSD_CARD_TYPE_HS400ES;
> +
>         card->ext_csd.hs_max_dtr = hs_max_dtr;
>         card->ext_csd.hs200_max_dtr = hs200_max_dtr;
>         card->mmc_avail_type = avail_type;
> @@ -383,6 +389,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
>                         mmc_card_set_blockaddr(card);
>         }
>
> +       card->ext_csd.strobe_support = ext_csd[EXT_CSD_STROBE_SUPPORT];
>         card->ext_csd.raw_card_type = ext_csd[EXT_CSD_CARD_TYPE];
>         mmc_select_card_type(card);
>
> @@ -1062,9 +1069,10 @@ static int mmc_select_hs400(struct mmc_card *card)
>         u8 val;
>
>         /*
> -        * HS400 mode requires 8-bit bus width
> +        * HS400(ES) mode requires 8-bit bus width
>          */
> -       if (!(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400 &&
> +       if (!(((card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400) ||
> +               (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)) &&
>               host->ios.bus_width == MMC_BUS_WIDTH_8))
>                 return 0;
>
> @@ -1097,9 +1105,26 @@ static int mmc_select_hs400(struct mmc_card *card)
>         }
>
>         /* Switch card to DDR */
> +       val = EXT_CSD_DDR_BUS_WIDTH_8;
> +       if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
> +               val |= EXT_CSD_BUS_WIDTH_STROBE;
> +               /*
> +               * Make sure we are in non-enhanced strobe mode before we
> +               * actually enable it in ext_csd.
> +               */
> +               if (host->ops->prepare_enhanced_strobe)
> +                       err = host->ops->prepare_enhanced_strobe(host, false);

1) I suggest we rename the callback to "hs400_enhanced_strobe".

2) I think this might be is a bit late to deal with restoring enhanced
strobe to off. Perhaps we should do that in mmc_set_initial_state()
instead!?

> +
> +               if (err) {
> +                       pr_err("%s: unprepare_enhanced strobe failed, err:%d\n",

Maybe pr_dbg instead?

/s/unprepare_enhanced strobe/Disable hs400 es

> +                               mmc_hostname(host), err);
> +                       return err;
> +               }
> +       }
> +
>         err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>                          EXT_CSD_BUS_WIDTH,
> -                        EXT_CSD_DDR_BUS_WIDTH_8,
> +                        val,
>                          card->ext_csd.generic_cmd6_time);
>         if (err) {
>                 pr_err("%s: switch to bus width for hs400 failed, err:%d\n",
> @@ -1124,6 +1149,35 @@ static int mmc_select_hs400(struct mmc_card *card)
>         mmc_set_timing(host, MMC_TIMING_MMC_HS400);
>         mmc_set_bus_speed(card);
>
> +       if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
> +               /* Controller enable enhanced strobe function */
> +               if (host->ops->prepare_enhanced_strobe)
> +                       err = host->ops->prepare_enhanced_strobe(host, true);
> +
> +               if (err) {
> +                       pr_err("%s: prepare enhanced strobe failed, err:%d\n",

Maybe pr_dbg/warn instead?

/s/prepare enhanced strobe/Enable hs400 es

> +                               mmc_hostname(host), err);
> +                       return err;
> +               }
> +
> +               /*
> +                * After switching to hs400 enhanced strobe mode, we expect to
> +                * verify whether it works or not. If controller can't handle
> +                * bus width test, compare ext_csd previously read in 1 bit mode
> +                * against ext_csd at new bus width
> +                */

Is this according to spec? If so, we should probably state that here.
If not, further explanation for *why* we do this is needed.

> +               if (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))
> +                       err = mmc_compare_ext_csds(card, MMC_BUS_WIDTH_8);
> +               else
> +                       err = mmc_bus_test(card, MMC_BUS_WIDTH_8);
> +
> +               if (err) {
> +                       pr_warn("%s: switch to enhanced strobe failed\n",
> +                               mmc_hostname(host));
> +                       return err;
> +               }
> +       }
> +
>         if (!send_status) {
>                 err = mmc_switch_status(card);
>                 if (err)
> @@ -1297,7 +1351,8 @@ err:
>  }
>
>  /*
> - * Activate High Speed or HS200 mode if supported.
> + * Activate High Speed or HS200 mode if supported. For HS400
> + * with enhanced strobe mode, we should activate High Speed.
>   */
>  static int mmc_select_timing(struct mmc_card *card)
>  {
> @@ -1306,7 +1361,9 @@ static int mmc_select_timing(struct mmc_card *card)
>         if (!mmc_can_ext_csd(card))
>                 goto bus_speed;
>
> -       if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
> +       if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)
> +               err = mmc_select_hs(card);
> +       else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
>                 err = mmc_select_hs200(card);
>         else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
>                 err = mmc_select_hs(card);
> @@ -1578,7 +1635,15 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>         } else if (mmc_card_hs(card)) {
>                 /* Select the desired bus width optionally */
>                 err = mmc_select_bus_width(card);
> -               if (!IS_ERR_VALUE(err)) {
> +               if (IS_ERR_VALUE(err))
> +                       goto free_card;

This is going to change the behaviour, as earlier switching the bus
width was optional. See the comment a few lines above and the
implementation of mmc_select_bus_width().

> +
> +               if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
> +                       /* Directly from HS to HS400-ES */
> +                       err = mmc_select_hs400(card);
> +                       if (err)
> +                               goto free_card;
> +               } else {
>                         err = mmc_select_hs_ddr(card);
>                         if (err)
>                                 goto free_card;
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index eb0151b..22defc2 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -95,6 +95,7 @@ struct mmc_ext_csd {
>         u8                      raw_partition_support;  /* 160 */
>         u8                      raw_rpmb_size_mult;     /* 168 */
>         u8                      raw_erased_mem_count;   /* 181 */
> +       u8                      strobe_support;         /* 184 */
>         u8                      raw_ext_csd_structure;  /* 194 */
>         u8                      raw_card_type;          /* 196 */
>         u8                      raw_driver_strength;    /* 197 */
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 11e89d3..19de56c 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -19,6 +19,7 @@
>
>  #include <linux/mmc/core.h>
>  #include <linux/mmc/card.h>
> +#include <linux/mmc/mmc.h>
>  #include <linux/mmc/pm.h>
>
>  struct mmc_ios {
> @@ -143,6 +144,8 @@ struct mmc_host_ops {
>
>         /* Prepare HS400 target operating frequency depending host driver */
>         int     (*prepare_hs400_tuning)(struct mmc_host *host, struct mmc_ios *ios);
> +       /* Prepare enhanced strobe depending host driver */
> +       int     (*prepare_enhanced_strobe)(struct mmc_host *host, bool enable);
>         int     (*select_drive_strength)(struct mmc_card *card,
>                                          unsigned int max_dtr, int host_drv,
>                                          int card_drv, int *drv_type);
> @@ -518,6 +521,15 @@ static inline bool mmc_card_hs400(struct mmc_card *card)
>         return card->host->ios.timing == MMC_TIMING_MMC_HS400;
>  }
>
> +static inline bool mmc_card_hs400es(struct mmc_card *card)
> +{
> +       if (mmc_card_hs400(card) &&
> +           (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES))
> +               return 1;
> +
> +       return 0;
> +}
> +
>  void mmc_retune_timer_stop(struct mmc_host *host);
>
>  static inline void mmc_retune_needed(struct mmc_host *host)
> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
> index 15f2c4a..c376209 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -297,6 +297,7 @@ struct _mmc_csd {
>  #define EXT_CSD_PART_CONFIG            179     /* R/W */
>  #define EXT_CSD_ERASED_MEM_CONT                181     /* RO */
>  #define EXT_CSD_BUS_WIDTH              183     /* R/W */
> +#define EXT_CSD_STROBE_SUPPORT         184     /* RO */
>  #define EXT_CSD_HS_TIMING              185     /* R/W */
>  #define EXT_CSD_POWER_CLASS            187     /* R/W */
>  #define EXT_CSD_REV                    192     /* RO */
> @@ -380,12 +381,14 @@ struct _mmc_csd {
>  #define EXT_CSD_CARD_TYPE_HS400_1_2V   (1<<7)  /* Card can run at 200MHz DDR, 1.2V */
>  #define EXT_CSD_CARD_TYPE_HS400                (EXT_CSD_CARD_TYPE_HS400_1_8V | \
>                                          EXT_CSD_CARD_TYPE_HS400_1_2V)
> +#define EXT_CSD_CARD_TYPE_HS400ES      (1<<8)  /* Card can run at HS400ES */
>
>  #define EXT_CSD_BUS_WIDTH_1    0       /* Card is in 1 bit mode */
>  #define EXT_CSD_BUS_WIDTH_4    1       /* Card is in 4 bit mode */
>  #define EXT_CSD_BUS_WIDTH_8    2       /* Card is in 8 bit mode */
>  #define EXT_CSD_DDR_BUS_WIDTH_4        5       /* Card is in 4 bit DDR mode */
>  #define EXT_CSD_DDR_BUS_WIDTH_8        6       /* Card is in 8 bit DDR mode */
> +#define EXT_CSD_BUS_WIDTH_STROBE BIT(7)        /* Enhanced strobe mode */
>
>  #define EXT_CSD_TIMING_BC      0       /* Backwards compatility */
>  #define EXT_CSD_TIMING_HS      1       /* High speed */
> --
> 2.3.7
>
>

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/6] mmc: core: implement enhanced strobe support

[Adrian Hunter]

>> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
>> index 4bc48f1..7e94b9d 100644
>> --- a/drivers/mmc/core/bus.c
>> +++ b/drivers/mmc/core/bus.c
>> @@ -332,12 +332,13 @@ int mmc_add_card(struct mmc_card *card)
>>                         mmc_card_ddr52(card) ? "DDR " : "",
>>                         type);
>>         } else {
>> -               pr_info("%s: new %s%s%s%s%s card at address %04x\n",
>> +               pr_info("%s: new %s%s%s%s%s%s card at address %04x\n",
>>                         mmc_hostname(card->host),
>>                         mmc_card_uhs(card) ? "ultra high speed " :
>>                         (mmc_card_hs(card) ? "high speed " : ""),
>>                         mmc_card_hs400(card) ? "HS400 " :
>>                         (mmc_card_hs200(card) ? "HS200 " : ""),
>> +                       mmc_card_hs400es(card) ? "Enhanced strobe" : "",
> 
> I am not sure this informations should be printed. It's still and
> HS400 card, there is no performance impact, right?

I actually asked for that I think.  HS400ES does not need re-tuning so there
is a performance and behavioural change which I think it is very useful to
know.  Kernels which don't support HS400ES will show "HS400" whereas kernels
which do support it will show something different.

Re: [PATCH v2 3/6] mmc: core: implement enhanced strobe support

[Adrian Hunter]

>> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
>> index 4bc48f1..7e94b9d 100644
>> --- a/drivers/mmc/core/bus.c
>> +++ b/drivers/mmc/core/bus.c
>> @@ -332,12 +332,13 @@ int mmc_add_card(struct mmc_card *card)
>>                         mmc_card_ddr52(card) ? "DDR " : "",
>>                         type);
>>         } else {
>> -               pr_info("%s: new %s%s%s%s%s card at address %04x\n",
>> +               pr_info("%s: new %s%s%s%s%s%s card at address %04x\n",
>>                         mmc_hostname(card->host),
>>                         mmc_card_uhs(card) ? "ultra high speed " :
>>                         (mmc_card_hs(card) ? "high speed " : ""),
>>                         mmc_card_hs400(card) ? "HS400 " :
>>                         (mmc_card_hs200(card) ? "HS200 " : ""),
>> +                       mmc_card_hs400es(card) ? "Enhanced strobe" : "",
> 
> I am not sure this informations should be printed. It's still and
> HS400 card, there is no performance impact, right?

I actually asked for that I think.  HS400ES does not need re-tuning so there
is a performance and behavioural change which I think it is very useful to
know.  Kernels which don't support HS400ES will show "HS400" whereas kernels
which do support it will show something different.

Re: [PATCH v2 3/6] mmc: core: implement enhanced strobe support

[Shawn Lin]

On 2016/5/4 20:02, Ulf Hansson wrote:
> On 29 April 2016 at 04:47, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> Controllers use data strobe line to latch data from devices
>> under hs400 mode, but not for cmd line. So since emmc 5.1, JEDEC
>> introduces enhanced strobe mode for latching cmd response from
>> emmc devices to host controllers. This new feature is optional,
>> so it depends both on device's cap and host's cap to decide
>> whether to use it or not.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>
>> Changes in v2: None
>>
>>  drivers/mmc/core/bus.c   |  3 +-
>>  drivers/mmc/core/mmc.c   | 77 ++++++++++++++++++++++++++++++++++++++++++++----
>>  include/linux/mmc/card.h |  1 +
>>  include/linux/mmc/host.h | 12 ++++++++
>>  include/linux/mmc/mmc.h  |  3 ++
>>  5 files changed, 89 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
>> index 4bc48f1..7e94b9d 100644
>> --- a/drivers/mmc/core/bus.c
>> +++ b/drivers/mmc/core/bus.c
>> @@ -332,12 +332,13 @@ int mmc_add_card(struct mmc_card *card)
>>                         mmc_card_ddr52(card) ? "DDR " : "",
>>                         type);
>>         } else {
>> -               pr_info("%s: new %s%s%s%s%s card at address %04x\n",
>> +               pr_info("%s: new %s%s%s%s%s%s card at address %04x\n",
>>                         mmc_hostname(card->host),
>>                         mmc_card_uhs(card) ? "ultra high speed " :
>>                         (mmc_card_hs(card) ? "high speed " : ""),
>>                         mmc_card_hs400(card) ? "HS400 " :
>>                         (mmc_card_hs200(card) ? "HS200 " : ""),
>> +                       mmc_card_hs400es(card) ? "Enhanced strobe" : "",
>
> I am not sure this informations should be printed. It's still and
> HS400 card, there is no performance impact, right?

Just as Adrain said, a significant behaviour for hs400es is that
it doesn't need tuning/re-tune stuff. So I prone to keep it here.

>
>>                         mmc_card_ddr52(card) ? "DDR " : "",
>>                         uhs_bus_speed_mode, type, card->rca);
>>         }
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 28b477d..f45ed10 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -235,6 +235,12 @@ static void mmc_select_card_type(struct mmc_card *card)
>>                 avail_type |= EXT_CSD_CARD_TYPE_HS400_1_2V;
>>         }
>>
>> +       if ((caps2 & MMC_CAP2_HS400_ENHANCED_STROBE) &&
>> +           card->ext_csd.strobe_support &&
>> +           ((card_type & EXT_CSD_CARD_TYPE_HS400_1_2V) ||
>> +            (card_type & EXT_CSD_CARD_TYPE_HS400_1_8V)))
>> +               avail_type |= EXT_CSD_CARD_TYPE_HS400ES;
>> +
>>         card->ext_csd.hs_max_dtr = hs_max_dtr;
>>         card->ext_csd.hs200_max_dtr = hs200_max_dtr;
>>         card->mmc_avail_type = avail_type;
>> @@ -383,6 +389,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
>>                         mmc_card_set_blockaddr(card);
>>         }
>>
>> +       card->ext_csd.strobe_support = ext_csd[EXT_CSD_STROBE_SUPPORT];
>>         card->ext_csd.raw_card_type = ext_csd[EXT_CSD_CARD_TYPE];
>>         mmc_select_card_type(card);
>>
>> @@ -1062,9 +1069,10 @@ static int mmc_select_hs400(struct mmc_card *card)
>>         u8 val;
>>
>>         /*
>> -        * HS400 mode requires 8-bit bus width
>> +        * HS400(ES) mode requires 8-bit bus width
>>          */
>> -       if (!(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400 &&
>> +       if (!(((card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400) ||
>> +               (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)) &&
>>               host->ios.bus_width == MMC_BUS_WIDTH_8))
>>                 return 0;
>>
>> @@ -1097,9 +1105,26 @@ static int mmc_select_hs400(struct mmc_card *card)
>>         }
>>
>>         /* Switch card to DDR */
>> +       val = EXT_CSD_DDR_BUS_WIDTH_8;
>> +       if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
>> +               val |= EXT_CSD_BUS_WIDTH_STROBE;
>> +               /*
>> +               * Make sure we are in non-enhanced strobe mode before we
>> +               * actually enable it in ext_csd.
>> +               */
>> +               if (host->ops->prepare_enhanced_strobe)
>> +                       err = host->ops->prepare_enhanced_strobe(host, false);
>
> 1) I suggest we rename the callback to "hs400_enhanced_strobe".

Aha, personal taste?
I'm ok for whatever the name to be :)

>
> 2) I think this might be is a bit late to deal with restoring enhanced
> strobe to off. Perhaps we should do that in mmc_set_initial_state()
> instead!?
>

Good idea.

>> +
>> +               if (err) {
>> +                       pr_err("%s: unprepare_enhanced strobe failed, err:%d\n",
>
> Maybe pr_dbg instead?
>
> /s/unprepare_enhanced strobe/Disable hs400 es

ok, most of the failure in mmc_init_card use
pr_warn/dbg.

So will fix it.

>
>> +                               mmc_hostname(host), err);
>> +                       return err;
>> +               }
>> +       }
>> +
>>         err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>                          EXT_CSD_BUS_WIDTH,
>> -                        EXT_CSD_DDR_BUS_WIDTH_8,
>> +                        val,
>>                          card->ext_csd.generic_cmd6_time);
>>         if (err) {
>>                 pr_err("%s: switch to bus width for hs400 failed, err:%d\n",
>> @@ -1124,6 +1149,35 @@ static int mmc_select_hs400(struct mmc_card *card)
>>         mmc_set_timing(host, MMC_TIMING_MMC_HS400);
>>         mmc_set_bus_speed(card);
>>
>> +       if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
>> +               /* Controller enable enhanced strobe function */
>> +               if (host->ops->prepare_enhanced_strobe)
>> +                       err = host->ops->prepare_enhanced_strobe(host, true);
>> +
>> +               if (err) {
>> +                       pr_err("%s: prepare enhanced strobe failed, err:%d\n",
>
> Maybe pr_dbg/warn instead?
>
> /s/prepare enhanced strobe/Enable hs400 es
>
>> +                               mmc_hostname(host), err);
>> +                       return err;
>> +               }
>> +
>> +               /*
>> +                * After switching to hs400 enhanced strobe mode, we expect to
>> +                * verify whether it works or not. If controller can't handle
>> +                * bus width test, compare ext_csd previously read in 1 bit mode
>> +                * against ext_csd at new bus width
>> +                */
>
> Is this according to spec? If so, we should probably state that here.
> If not, further explanation for *why* we do this is needed.

It's NOT from spec. Just like switch bus width, enabling enhanced strobe
brings different IO-interface timing, so I add this check to make sure
both the device and host to be ok under hs400es, otherwise we show
hs400es successfully but failing to read superblock later if mounted.

So check it in advanced is probably sane?

>
>> +               if (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))
>> +                       err = mmc_compare_ext_csds(card, MMC_BUS_WIDTH_8);
>> +               else
>> +                       err = mmc_bus_test(card, MMC_BUS_WIDTH_8);
>> +
>> +               if (err) {
>> +                       pr_warn("%s: switch to enhanced strobe failed\n",
>> +                               mmc_hostname(host));
>> +                       return err;
>> +               }
>> +       }
>> +
>>         if (!send_status) {
>>                 err = mmc_switch_status(card);
>>                 if (err)
>> @@ -1297,7 +1351,8 @@ err:
>>  }
>>
>>  /*
>> - * Activate High Speed or HS200 mode if supported.
>> + * Activate High Speed or HS200 mode if supported. For HS400
>> + * with enhanced strobe mode, we should activate High Speed.
>>   */
>>  static int mmc_select_timing(struct mmc_card *card)
>>  {
>> @@ -1306,7 +1361,9 @@ static int mmc_select_timing(struct mmc_card *card)
>>         if (!mmc_can_ext_csd(card))
>>                 goto bus_speed;
>>
>> -       if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
>> +       if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)
>> +               err = mmc_select_hs(card);
>> +       else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
>>                 err = mmc_select_hs200(card);
>>         else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
>>                 err = mmc_select_hs(card);
>> @@ -1578,7 +1635,15 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>>         } else if (mmc_card_hs(card)) {
>>                 /* Select the desired bus width optionally */
>>                 err = mmc_select_bus_width(card);
>> -               if (!IS_ERR_VALUE(err)) {
>> +               if (IS_ERR_VALUE(err))
>> +                       goto free_card;
>
> This is going to change the behaviour, as earlier switching the bus
> width was optional. See the comment a few lines above and the
> implementation of mmc_select_bus_width().

Sorry, but I cannot see any behavioural change for bus width.
before swithing to HS400, we should make sure it's in 8-bit mode.

Before this change, the path seems to be:
(1) select_timing : hs200 --> mmc_select_hs200 -> mmc_select_bus_width
		    hs --> mmc_select_hs
(2) mmc_card_hs200 -> tuning --> mmc_select_hs400
     mmc_card_hs -> mmc_select_bus_width -> mmc_select_hs_ddr

Now the behaviour should be:
(1) select_timing : hs400es -> mmc_select_hs
		    hs200 --> mmc_select_hs200 -> mmc_select_bus_width
		    hs --> mmc_select_hs
(2) mmc_card_hs200 -> tuning --> mmc_select_hs400
     mmc_card_hs -> mmc_select_bus_width |-> mmc_select_hs_ddr
					|-> mmc_select_hs400

did I miss something?

Thanks.

>
>> +
>> +               if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
>> +                       /* Directly from HS to HS400-ES */
>> +                       err = mmc_select_hs400(card);
>> +                       if (err)
>> +                               goto free_card;
>> +               } else {
>>                         err = mmc_select_hs_ddr(card);
>>                         if (err)
>>                                 goto free_card;
>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>> index eb0151b..22defc2 100644
>> --- a/include/linux/mmc/card.h
>> +++ b/include/linux/mmc/card.h
>> @@ -95,6 +95,7 @@ struct mmc_ext_csd {
>>         u8                      raw_partition_support;  /* 160 */
>>         u8                      raw_rpmb_size_mult;     /* 168 */
>>         u8                      raw_erased_mem_count;   /* 181 */
>> +       u8                      strobe_support;         /* 184 */
>>         u8                      raw_ext_csd_structure;  /* 194 */
>>         u8                      raw_card_type;          /* 196 */
>>         u8                      raw_driver_strength;    /* 197 */
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index 11e89d3..19de56c 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -19,6 +19,7 @@
>>
>>  #include <linux/mmc/core.h>
>>  #include <linux/mmc/card.h>
>> +#include <linux/mmc/mmc.h>
>>  #include <linux/mmc/pm.h>
>>
>>  struct mmc_ios {
>> @@ -143,6 +144,8 @@ struct mmc_host_ops {
>>
>>         /* Prepare HS400 target operating frequency depending host driver */
>>         int     (*prepare_hs400_tuning)(struct mmc_host *host, struct mmc_ios *ios);
>> +       /* Prepare enhanced strobe depending host driver */
>> +       int     (*prepare_enhanced_strobe)(struct mmc_host *host, bool enable);
>>         int     (*select_drive_strength)(struct mmc_card *card,
>>                                          unsigned int max_dtr, int host_drv,
>>                                          int card_drv, int *drv_type);
>> @@ -518,6 +521,15 @@ static inline bool mmc_card_hs400(struct mmc_card *card)
>>         return card->host->ios.timing == MMC_TIMING_MMC_HS400;
>>  }
>>
>> +static inline bool mmc_card_hs400es(struct mmc_card *card)
>> +{
>> +       if (mmc_card_hs400(card) &&
>> +           (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES))
>> +               return 1;
>> +
>> +       return 0;
>> +}
>> +
>>  void mmc_retune_timer_stop(struct mmc_host *host);
>>
>>  static inline void mmc_retune_needed(struct mmc_host *host)
>> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
>> index 15f2c4a..c376209 100644
>> --- a/include/linux/mmc/mmc.h
>> +++ b/include/linux/mmc/mmc.h
>> @@ -297,6 +297,7 @@ struct _mmc_csd {
>>  #define EXT_CSD_PART_CONFIG            179     /* R/W */
>>  #define EXT_CSD_ERASED_MEM_CONT                181     /* RO */
>>  #define EXT_CSD_BUS_WIDTH              183     /* R/W */
>> +#define EXT_CSD_STROBE_SUPPORT         184     /* RO */
>>  #define EXT_CSD_HS_TIMING              185     /* R/W */
>>  #define EXT_CSD_POWER_CLASS            187     /* R/W */
>>  #define EXT_CSD_REV                    192     /* RO */
>> @@ -380,12 +381,14 @@ struct _mmc_csd {
>>  #define EXT_CSD_CARD_TYPE_HS400_1_2V   (1<<7)  /* Card can run at 200MHz DDR, 1.2V */
>>  #define EXT_CSD_CARD_TYPE_HS400                (EXT_CSD_CARD_TYPE_HS400_1_8V | \
>>                                          EXT_CSD_CARD_TYPE_HS400_1_2V)
>> +#define EXT_CSD_CARD_TYPE_HS400ES      (1<<8)  /* Card can run at HS400ES */
>>
>>  #define EXT_CSD_BUS_WIDTH_1    0       /* Card is in 1 bit mode */
>>  #define EXT_CSD_BUS_WIDTH_4    1       /* Card is in 4 bit mode */
>>  #define EXT_CSD_BUS_WIDTH_8    2       /* Card is in 8 bit mode */
>>  #define EXT_CSD_DDR_BUS_WIDTH_4        5       /* Card is in 4 bit DDR mode */
>>  #define EXT_CSD_DDR_BUS_WIDTH_8        6       /* Card is in 8 bit DDR mode */
>> +#define EXT_CSD_BUS_WIDTH_STROBE BIT(7)        /* Enhanced strobe mode */
>>
>>  #define EXT_CSD_TIMING_BC      0       /* Backwards compatility */
>>  #define EXT_CSD_TIMING_HS      1       /* High speed */
>> --
>> 2.3.7
>>
>>
>
> Kind regards
> Uffe
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH v2 3/6] mmc: core: implement enhanced strobe support

[Shawn Lin]

On 2016/5/4 20:02, Ulf Hansson wrote:
> On 29 April 2016 at 04:47, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>> Controllers use data strobe line to latch data from devices
>> under hs400 mode, but not for cmd line. So since emmc 5.1, JEDEC
>> introduces enhanced strobe mode for latching cmd response from
>> emmc devices to host controllers. This new feature is optional,
>> so it depends both on device's cap and host's cap to decide
>> whether to use it or not.
>>
>> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> ---
>>
>> Changes in v2: None
>>
>>  drivers/mmc/core/bus.c   |  3 +-
>>  drivers/mmc/core/mmc.c   | 77 ++++++++++++++++++++++++++++++++++++++++++++----
>>  include/linux/mmc/card.h |  1 +
>>  include/linux/mmc/host.h | 12 ++++++++
>>  include/linux/mmc/mmc.h  |  3 ++
>>  5 files changed, 89 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
>> index 4bc48f1..7e94b9d 100644
>> --- a/drivers/mmc/core/bus.c
>> +++ b/drivers/mmc/core/bus.c
>> @@ -332,12 +332,13 @@ int mmc_add_card(struct mmc_card *card)
>>                         mmc_card_ddr52(card) ? "DDR " : "",
>>                         type);
>>         } else {
>> -               pr_info("%s: new %s%s%s%s%s card at address %04x\n",
>> +               pr_info("%s: new %s%s%s%s%s%s card at address %04x\n",
>>                         mmc_hostname(card->host),
>>                         mmc_card_uhs(card) ? "ultra high speed " :
>>                         (mmc_card_hs(card) ? "high speed " : ""),
>>                         mmc_card_hs400(card) ? "HS400 " :
>>                         (mmc_card_hs200(card) ? "HS200 " : ""),
>> +                       mmc_card_hs400es(card) ? "Enhanced strobe" : "",
>
> I am not sure this informations should be printed. It's still and
> HS400 card, there is no performance impact, right?

Just as Adrain said, a significant behaviour for hs400es is that
it doesn't need tuning/re-tune stuff. So I prone to keep it here.

>
>>                         mmc_card_ddr52(card) ? "DDR " : "",
>>                         uhs_bus_speed_mode, type, card->rca);
>>         }
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 28b477d..f45ed10 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -235,6 +235,12 @@ static void mmc_select_card_type(struct mmc_card *card)
>>                 avail_type |= EXT_CSD_CARD_TYPE_HS400_1_2V;
>>         }
>>
>> +       if ((caps2 & MMC_CAP2_HS400_ENHANCED_STROBE) &&
>> +           card->ext_csd.strobe_support &&
>> +           ((card_type & EXT_CSD_CARD_TYPE_HS400_1_2V) ||
>> +            (card_type & EXT_CSD_CARD_TYPE_HS400_1_8V)))
>> +               avail_type |= EXT_CSD_CARD_TYPE_HS400ES;
>> +
>>         card->ext_csd.hs_max_dtr = hs_max_dtr;
>>         card->ext_csd.hs200_max_dtr = hs200_max_dtr;
>>         card->mmc_avail_type = avail_type;
>> @@ -383,6 +389,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
>>                         mmc_card_set_blockaddr(card);
>>         }
>>
>> +       card->ext_csd.strobe_support = ext_csd[EXT_CSD_STROBE_SUPPORT];
>>         card->ext_csd.raw_card_type = ext_csd[EXT_CSD_CARD_TYPE];
>>         mmc_select_card_type(card);
>>
>> @@ -1062,9 +1069,10 @@ static int mmc_select_hs400(struct mmc_card *card)
>>         u8 val;
>>
>>         /*
>> -        * HS400 mode requires 8-bit bus width
>> +        * HS400(ES) mode requires 8-bit bus width
>>          */
>> -       if (!(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400 &&
>> +       if (!(((card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400) ||
>> +               (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)) &&
>>               host->ios.bus_width == MMC_BUS_WIDTH_8))
>>                 return 0;
>>
>> @@ -1097,9 +1105,26 @@ static int mmc_select_hs400(struct mmc_card *card)
>>         }
>>
>>         /* Switch card to DDR */
>> +       val = EXT_CSD_DDR_BUS_WIDTH_8;
>> +       if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
>> +               val |= EXT_CSD_BUS_WIDTH_STROBE;
>> +               /*
>> +               * Make sure we are in non-enhanced strobe mode before we
>> +               * actually enable it in ext_csd.
>> +               */
>> +               if (host->ops->prepare_enhanced_strobe)
>> +                       err = host->ops->prepare_enhanced_strobe(host, false);
>
> 1) I suggest we rename the callback to "hs400_enhanced_strobe".

Aha, personal taste?
I'm ok for whatever the name to be :)

>
> 2) I think this might be is a bit late to deal with restoring enhanced
> strobe to off. Perhaps we should do that in mmc_set_initial_state()
> instead!?
>

Good idea.

>> +
>> +               if (err) {
>> +                       pr_err("%s: unprepare_enhanced strobe failed, err:%d\n",
>
> Maybe pr_dbg instead?
>
> /s/unprepare_enhanced strobe/Disable hs400 es

ok, most of the failure in mmc_init_card use
pr_warn/dbg.

So will fix it.

>
>> +                               mmc_hostname(host), err);
>> +                       return err;
>> +               }
>> +       }
>> +
>>         err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>                          EXT_CSD_BUS_WIDTH,
>> -                        EXT_CSD_DDR_BUS_WIDTH_8,
>> +                        val,
>>                          card->ext_csd.generic_cmd6_time);
>>         if (err) {
>>                 pr_err("%s: switch to bus width for hs400 failed, err:%d\n",
>> @@ -1124,6 +1149,35 @@ static int mmc_select_hs400(struct mmc_card *card)
>>         mmc_set_timing(host, MMC_TIMING_MMC_HS400);
>>         mmc_set_bus_speed(card);
>>
>> +       if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
>> +               /* Controller enable enhanced strobe function */
>> +               if (host->ops->prepare_enhanced_strobe)
>> +                       err = host->ops->prepare_enhanced_strobe(host, true);
>> +
>> +               if (err) {
>> +                       pr_err("%s: prepare enhanced strobe failed, err:%d\n",
>
> Maybe pr_dbg/warn instead?
>
> /s/prepare enhanced strobe/Enable hs400 es
>
>> +                               mmc_hostname(host), err);
>> +                       return err;
>> +               }
>> +
>> +               /*
>> +                * After switching to hs400 enhanced strobe mode, we expect to
>> +                * verify whether it works or not. If controller can't handle
>> +                * bus width test, compare ext_csd previously read in 1 bit mode
>> +                * against ext_csd at new bus width
>> +                */
>
> Is this according to spec? If so, we should probably state that here.
> If not, further explanation for *why* we do this is needed.

It's NOT from spec. Just like switch bus width, enabling enhanced strobe
brings different IO-interface timing, so I add this check to make sure
both the device and host to be ok under hs400es, otherwise we show
hs400es successfully but failing to read superblock later if mounted.

So check it in advanced is probably sane?

>
>> +               if (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))
>> +                       err = mmc_compare_ext_csds(card, MMC_BUS_WIDTH_8);
>> +               else
>> +                       err = mmc_bus_test(card, MMC_BUS_WIDTH_8);
>> +
>> +               if (err) {
>> +                       pr_warn("%s: switch to enhanced strobe failed\n",
>> +                               mmc_hostname(host));
>> +                       return err;
>> +               }
>> +       }
>> +
>>         if (!send_status) {
>>                 err = mmc_switch_status(card);
>>                 if (err)
>> @@ -1297,7 +1351,8 @@ err:
>>  }
>>
>>  /*
>> - * Activate High Speed or HS200 mode if supported.
>> + * Activate High Speed or HS200 mode if supported. For HS400
>> + * with enhanced strobe mode, we should activate High Speed.
>>   */
>>  static int mmc_select_timing(struct mmc_card *card)
>>  {
>> @@ -1306,7 +1361,9 @@ static int mmc_select_timing(struct mmc_card *card)
>>         if (!mmc_can_ext_csd(card))
>>                 goto bus_speed;
>>
>> -       if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
>> +       if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)
>> +               err = mmc_select_hs(card);
>> +       else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
>>                 err = mmc_select_hs200(card);
>>         else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
>>                 err = mmc_select_hs(card);
>> @@ -1578,7 +1635,15 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>>         } else if (mmc_card_hs(card)) {
>>                 /* Select the desired bus width optionally */
>>                 err = mmc_select_bus_width(card);
>> -               if (!IS_ERR_VALUE(err)) {
>> +               if (IS_ERR_VALUE(err))
>> +                       goto free_card;
>
> This is going to change the behaviour, as earlier switching the bus
> width was optional. See the comment a few lines above and the
> implementation of mmc_select_bus_width().

Sorry, but I cannot see any behavioural change for bus width.
before swithing to HS400, we should make sure it's in 8-bit mode.

Before this change, the path seems to be:
(1) select_timing : hs200 --> mmc_select_hs200 -> mmc_select_bus_width
		    hs --> mmc_select_hs
(2) mmc_card_hs200 -> tuning --> mmc_select_hs400
     mmc_card_hs -> mmc_select_bus_width -> mmc_select_hs_ddr

Now the behaviour should be:
(1) select_timing : hs400es -> mmc_select_hs
		    hs200 --> mmc_select_hs200 -> mmc_select_bus_width
		    hs --> mmc_select_hs
(2) mmc_card_hs200 -> tuning --> mmc_select_hs400
     mmc_card_hs -> mmc_select_bus_width |-> mmc_select_hs_ddr
					|-> mmc_select_hs400

did I miss something?

Thanks.

>
>> +
>> +               if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
>> +                       /* Directly from HS to HS400-ES */
>> +                       err = mmc_select_hs400(card);
>> +                       if (err)
>> +                               goto free_card;
>> +               } else {
>>                         err = mmc_select_hs_ddr(card);
>>                         if (err)
>>                                 goto free_card;
>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>> index eb0151b..22defc2 100644
>> --- a/include/linux/mmc/card.h
>> +++ b/include/linux/mmc/card.h
>> @@ -95,6 +95,7 @@ struct mmc_ext_csd {
>>         u8                      raw_partition_support;  /* 160 */
>>         u8                      raw_rpmb_size_mult;     /* 168 */
>>         u8                      raw_erased_mem_count;   /* 181 */
>> +       u8                      strobe_support;         /* 184 */
>>         u8                      raw_ext_csd_structure;  /* 194 */
>>         u8                      raw_card_type;          /* 196 */
>>         u8                      raw_driver_strength;    /* 197 */
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index 11e89d3..19de56c 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -19,6 +19,7 @@
>>
>>  #include <linux/mmc/core.h>
>>  #include <linux/mmc/card.h>
>> +#include <linux/mmc/mmc.h>
>>  #include <linux/mmc/pm.h>
>>
>>  struct mmc_ios {
>> @@ -143,6 +144,8 @@ struct mmc_host_ops {
>>
>>         /* Prepare HS400 target operating frequency depending host driver */
>>         int     (*prepare_hs400_tuning)(struct mmc_host *host, struct mmc_ios *ios);
>> +       /* Prepare enhanced strobe depending host driver */
>> +       int     (*prepare_enhanced_strobe)(struct mmc_host *host, bool enable);
>>         int     (*select_drive_strength)(struct mmc_card *card,
>>                                          unsigned int max_dtr, int host_drv,
>>                                          int card_drv, int *drv_type);
>> @@ -518,6 +521,15 @@ static inline bool mmc_card_hs400(struct mmc_card *card)
>>         return card->host->ios.timing == MMC_TIMING_MMC_HS400;
>>  }
>>
>> +static inline bool mmc_card_hs400es(struct mmc_card *card)
>> +{
>> +       if (mmc_card_hs400(card) &&
>> +           (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES))
>> +               return 1;
>> +
>> +       return 0;
>> +}
>> +
>>  void mmc_retune_timer_stop(struct mmc_host *host);
>>
>>  static inline void mmc_retune_needed(struct mmc_host *host)
>> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
>> index 15f2c4a..c376209 100644
>> --- a/include/linux/mmc/mmc.h
>> +++ b/include/linux/mmc/mmc.h
>> @@ -297,6 +297,7 @@ struct _mmc_csd {
>>  #define EXT_CSD_PART_CONFIG            179     /* R/W */
>>  #define EXT_CSD_ERASED_MEM_CONT                181     /* RO */
>>  #define EXT_CSD_BUS_WIDTH              183     /* R/W */
>> +#define EXT_CSD_STROBE_SUPPORT         184     /* RO */
>>  #define EXT_CSD_HS_TIMING              185     /* R/W */
>>  #define EXT_CSD_POWER_CLASS            187     /* R/W */
>>  #define EXT_CSD_REV                    192     /* RO */
>> @@ -380,12 +381,14 @@ struct _mmc_csd {
>>  #define EXT_CSD_CARD_TYPE_HS400_1_2V   (1<<7)  /* Card can run at 200MHz DDR, 1.2V */
>>  #define EXT_CSD_CARD_TYPE_HS400                (EXT_CSD_CARD_TYPE_HS400_1_8V | \
>>                                          EXT_CSD_CARD_TYPE_HS400_1_2V)
>> +#define EXT_CSD_CARD_TYPE_HS400ES      (1<<8)  /* Card can run at HS400ES */
>>
>>  #define EXT_CSD_BUS_WIDTH_1    0       /* Card is in 1 bit mode */
>>  #define EXT_CSD_BUS_WIDTH_4    1       /* Card is in 4 bit mode */
>>  #define EXT_CSD_BUS_WIDTH_8    2       /* Card is in 8 bit mode */
>>  #define EXT_CSD_DDR_BUS_WIDTH_4        5       /* Card is in 4 bit DDR mode */
>>  #define EXT_CSD_DDR_BUS_WIDTH_8        6       /* Card is in 8 bit DDR mode */
>> +#define EXT_CSD_BUS_WIDTH_STROBE BIT(7)        /* Enhanced strobe mode */
>>
>>  #define EXT_CSD_TIMING_BC      0       /* Backwards compatility */
>>  #define EXT_CSD_TIMING_HS      1       /* High speed */
>> --
>> 2.3.7
>>
>>
>
> Kind regards
> Uffe
>
>
>


-- 
Best Regards
Shawn Lin

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/6] mmc: core: implement enhanced strobe support

[Adrian Hunter]

On 29/04/16 05:47, Shawn Lin wrote:
> Controllers use data strobe line to latch data from devices
> under hs400 mode, but not for cmd line. So since emmc 5.1, JEDEC
> introduces enhanced strobe mode for latching cmd response from
> emmc devices to host controllers. This new feature is optional,
> so it depends both on device's cap and host's cap to decide
> whether to use it or not.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

Thanks for doing this.  There are a couple of comments below, but generally
I tend to think you should split out selecting HS400ES as a separate function.

> ---
> 
> Changes in v2: None
> 
>  drivers/mmc/core/bus.c   |  3 +-
>  drivers/mmc/core/mmc.c   | 77 ++++++++++++++++++++++++++++++++++++++++++++----
>  include/linux/mmc/card.h |  1 +
>  include/linux/mmc/host.h | 12 ++++++++
>  include/linux/mmc/mmc.h  |  3 ++
>  5 files changed, 89 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> index 4bc48f1..7e94b9d 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -332,12 +332,13 @@ int mmc_add_card(struct mmc_card *card)
>  			mmc_card_ddr52(card) ? "DDR " : "",
>  			type);
>  	} else {
> -		pr_info("%s: new %s%s%s%s%s card at address %04x\n",
> +		pr_info("%s: new %s%s%s%s%s%s card at address %04x\n",
>  			mmc_hostname(card->host),
>  			mmc_card_uhs(card) ? "ultra high speed " :
>  			(mmc_card_hs(card) ? "high speed " : ""),
>  			mmc_card_hs400(card) ? "HS400 " :
>  			(mmc_card_hs200(card) ? "HS200 " : ""),
> +			mmc_card_hs400es(card) ? "Enhanced strobe" : "",
>  			mmc_card_ddr52(card) ? "DDR " : "",
>  			uhs_bus_speed_mode, type, card->rca);
>  	}
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 28b477d..f45ed10 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -235,6 +235,12 @@ static void mmc_select_card_type(struct mmc_card *card)
>  		avail_type |= EXT_CSD_CARD_TYPE_HS400_1_2V;
>  	}
>  
> +	if ((caps2 & MMC_CAP2_HS400_ENHANCED_STROBE) &&
> +	    card->ext_csd.strobe_support &&
> +	    ((card_type & EXT_CSD_CARD_TYPE_HS400_1_2V) ||
> +	     (card_type & EXT_CSD_CARD_TYPE_HS400_1_8V)))

Could use just "card_type & EXT_CSD_CARD_TYPE_HS400" here

> +		avail_type |= EXT_CSD_CARD_TYPE_HS400ES;
> +
>  	card->ext_csd.hs_max_dtr = hs_max_dtr;
>  	card->ext_csd.hs200_max_dtr = hs200_max_dtr;
>  	card->mmc_avail_type = avail_type;
> @@ -383,6 +389,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
>  			mmc_card_set_blockaddr(card);
>  	}
>  
> +	card->ext_csd.strobe_support = ext_csd[EXT_CSD_STROBE_SUPPORT];
>  	card->ext_csd.raw_card_type = ext_csd[EXT_CSD_CARD_TYPE];
>  	mmc_select_card_type(card);
>  
> @@ -1062,9 +1069,10 @@ static int mmc_select_hs400(struct mmc_card *card)
>  	u8 val;
>  
>  	/*
> -	 * HS400 mode requires 8-bit bus width
> +	 * HS400(ES) mode requires 8-bit bus width
>  	 */
> -	if (!(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400 &&
> +	if (!(((card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400) ||
> +		(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)) &&
>  	      host->ios.bus_width == MMC_BUS_WIDTH_8))
>  		return 0;
>  
> @@ -1097,9 +1105,26 @@ static int mmc_select_hs400(struct mmc_card *card)
>  	}
>  
>  	/* Switch card to DDR */
> +	val = EXT_CSD_DDR_BUS_WIDTH_8;
> +	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
> +		val |= EXT_CSD_BUS_WIDTH_STROBE;
> +		/*
> +		* Make sure we are in non-enhanced strobe mode before we
> +		* actually enable it in ext_csd.
> +		*/
> +		if (host->ops->prepare_enhanced_strobe)
> +			err = host->ops->prepare_enhanced_strobe(host, false);
> +
> +		if (err) {
> +			pr_err("%s: unprepare_enhanced strobe failed, err:%d\n",
> +				mmc_hostname(host), err);
> +			return err;
> +		}
> +	}
> +
>  	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>  			 EXT_CSD_BUS_WIDTH,
> -			 EXT_CSD_DDR_BUS_WIDTH_8,
> +			 val,
>  			 card->ext_csd.generic_cmd6_time);
>  	if (err) {
>  		pr_err("%s: switch to bus width for hs400 failed, err:%d\n",
> @@ -1124,6 +1149,35 @@ static int mmc_select_hs400(struct mmc_card *card)
>  	mmc_set_timing(host, MMC_TIMING_MMC_HS400);
>  	mmc_set_bus_speed(card);
>  
> +	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
> +		/* Controller enable enhanced strobe function */
> +		if (host->ops->prepare_enhanced_strobe)
> +			err = host->ops->prepare_enhanced_strobe(host, true);
> +
> +		if (err) {
> +			pr_err("%s: prepare enhanced strobe failed, err:%d\n",
> +				mmc_hostname(host), err);
> +			return err;
> +		}
> +
> +		/*
> +		 * After switching to hs400 enhanced strobe mode, we expect to
> +		 * verify whether it works or not. If controller can't handle
> +		 * bus width test, compare ext_csd previously read in 1 bit mode
> +		 * against ext_csd at new bus width
> +		 */

I don't think we need this.  Just checking (host->caps & MMC_CAP_8_BIT_DATA)
ought to be enough.  But if you want to play if safe, you could use
mmc_select_bus_width() in advance.

> +		if (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))
> +			err = mmc_compare_ext_csds(card, MMC_BUS_WIDTH_8);
> +		else
> +			err = mmc_bus_test(card, MMC_BUS_WIDTH_8);
> +
> +		if (err) {
> +			pr_warn("%s: switch to enhanced strobe failed\n",
> +				mmc_hostname(host));
> +			return err;
> +		}
> +	}
> +
>  	if (!send_status) {
>  		err = mmc_switch_status(card);
>  		if (err)
> @@ -1297,7 +1351,8 @@ err:
>  }
>  
>  /*
> - * Activate High Speed or HS200 mode if supported.
> + * Activate High Speed or HS200 mode if supported. For HS400
> + * with enhanced strobe mode, we should activate High Speed.
>   */
>  static int mmc_select_timing(struct mmc_card *card)
>  {
> @@ -1306,7 +1361,9 @@ static int mmc_select_timing(struct mmc_card *card)
>  	if (!mmc_can_ext_csd(card))
>  		goto bus_speed;
>  
> -	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
> +	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)
> +		err = mmc_select_hs(card);

In my view, you should just make a new function: mmc_select_hs400es()

> +	else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
>  		err = mmc_select_hs200(card);
>  	else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
>  		err = mmc_select_hs(card);
> @@ -1578,7 +1635,15 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>  	} else if (mmc_card_hs(card)) {
>  		/* Select the desired bus width optionally */
>  		err = mmc_select_bus_width(card);
> -		if (!IS_ERR_VALUE(err)) {
> +		if (IS_ERR_VALUE(err))
> +			goto free_card;
> +
> +		if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
> +			/* Directly from HS to HS400-ES */
> +			err = mmc_select_hs400(card);
> +			if (err)
> +				goto free_card;
> +		} else {
>  			err = mmc_select_hs_ddr(card);
>  			if (err)
>  				goto free_card;
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index eb0151b..22defc2 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -95,6 +95,7 @@ struct mmc_ext_csd {
>  	u8			raw_partition_support;	/* 160 */
>  	u8			raw_rpmb_size_mult;	/* 168 */
>  	u8			raw_erased_mem_count;	/* 181 */
> +	u8			strobe_support;		/* 184 */
>  	u8			raw_ext_csd_structure;	/* 194 */
>  	u8			raw_card_type;		/* 196 */
>  	u8			raw_driver_strength;	/* 197 */
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 11e89d3..19de56c 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -19,6 +19,7 @@
>  
>  #include <linux/mmc/core.h>
>  #include <linux/mmc/card.h>
> +#include <linux/mmc/mmc.h>
>  #include <linux/mmc/pm.h>
>  
>  struct mmc_ios {
> @@ -143,6 +144,8 @@ struct mmc_host_ops {
>  
>  	/* Prepare HS400 target operating frequency depending host driver */
>  	int	(*prepare_hs400_tuning)(struct mmc_host *host, struct mmc_ios *ios);
> +	/* Prepare enhanced strobe depending host driver */
> +	int	(*prepare_enhanced_strobe)(struct mmc_host *host, bool enable);

I don't see any need for an error return, so maybe make this return void.
Also, we could put "bool enhanced_strobe" in struct mmc_ios and pass ios
instead of enable.

>  	int	(*select_drive_strength)(struct mmc_card *card,
>  					 unsigned int max_dtr, int host_drv,
>  					 int card_drv, int *drv_type);
> @@ -518,6 +521,15 @@ static inline bool mmc_card_hs400(struct mmc_card *card)
>  	return card->host->ios.timing == MMC_TIMING_MMC_HS400;
>  }
>  
> +static inline bool mmc_card_hs400es(struct mmc_card *card)
> +{
> +	if (mmc_card_hs400(card) &&
> +	    (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES))
> +		return 1;

We shouldn't assume that we are using it, just because it is available.
How about putting "bool enhanced_strobe" in struct mmc_ios and checking that.

> +
> +	return 0;
> +}
> +
>  void mmc_retune_timer_stop(struct mmc_host *host);
>  
>  static inline void mmc_retune_needed(struct mmc_host *host)
> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
> index 15f2c4a..c376209 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -297,6 +297,7 @@ struct _mmc_csd {
>  #define EXT_CSD_PART_CONFIG		179	/* R/W */
>  #define EXT_CSD_ERASED_MEM_CONT		181	/* RO */
>  #define EXT_CSD_BUS_WIDTH		183	/* R/W */
> +#define EXT_CSD_STROBE_SUPPORT		184	/* RO */
>  #define EXT_CSD_HS_TIMING		185	/* R/W */
>  #define EXT_CSD_POWER_CLASS		187	/* R/W */
>  #define EXT_CSD_REV			192	/* RO */
> @@ -380,12 +381,14 @@ struct _mmc_csd {
>  #define EXT_CSD_CARD_TYPE_HS400_1_2V	(1<<7)	/* Card can run at 200MHz DDR, 1.2V */
>  #define EXT_CSD_CARD_TYPE_HS400		(EXT_CSD_CARD_TYPE_HS400_1_8V | \
>  					 EXT_CSD_CARD_TYPE_HS400_1_2V)
> +#define EXT_CSD_CARD_TYPE_HS400ES	(1<<8)	/* Card can run at HS400ES */
>  
>  #define EXT_CSD_BUS_WIDTH_1	0	/* Card is in 1 bit mode */
>  #define EXT_CSD_BUS_WIDTH_4	1	/* Card is in 4 bit mode */
>  #define EXT_CSD_BUS_WIDTH_8	2	/* Card is in 8 bit mode */
>  #define EXT_CSD_DDR_BUS_WIDTH_4	5	/* Card is in 4 bit DDR mode */
>  #define EXT_CSD_DDR_BUS_WIDTH_8	6	/* Card is in 8 bit DDR mode */
> +#define EXT_CSD_BUS_WIDTH_STROBE BIT(7)	/* Enhanced strobe mode */
>  
>  #define EXT_CSD_TIMING_BC	0	/* Backwards compatility */
>  #define EXT_CSD_TIMING_HS	1	/* High speed */
> 

Re: [PATCH v2 3/6] mmc: core: implement enhanced strobe support

[Adrian Hunter]

On 29/04/16 05:47, Shawn Lin wrote:
> Controllers use data strobe line to latch data from devices
> under hs400 mode, but not for cmd line. So since emmc 5.1, JEDEC
> introduces enhanced strobe mode for latching cmd response from
> emmc devices to host controllers. This new feature is optional,
> so it depends both on device's cap and host's cap to decide
> whether to use it or not.
> 
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>

Thanks for doing this.  There are a couple of comments below, but generally
I tend to think you should split out selecting HS400ES as a separate function.

> ---
> 
> Changes in v2: None
> 
>  drivers/mmc/core/bus.c   |  3 +-
>  drivers/mmc/core/mmc.c   | 77 ++++++++++++++++++++++++++++++++++++++++++++----
>  include/linux/mmc/card.h |  1 +
>  include/linux/mmc/host.h | 12 ++++++++
>  include/linux/mmc/mmc.h  |  3 ++
>  5 files changed, 89 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> index 4bc48f1..7e94b9d 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -332,12 +332,13 @@ int mmc_add_card(struct mmc_card *card)
>  			mmc_card_ddr52(card) ? "DDR " : "",
>  			type);
>  	} else {
> -		pr_info("%s: new %s%s%s%s%s card at address %04x\n",
> +		pr_info("%s: new %s%s%s%s%s%s card at address %04x\n",
>  			mmc_hostname(card->host),
>  			mmc_card_uhs(card) ? "ultra high speed " :
>  			(mmc_card_hs(card) ? "high speed " : ""),
>  			mmc_card_hs400(card) ? "HS400 " :
>  			(mmc_card_hs200(card) ? "HS200 " : ""),
> +			mmc_card_hs400es(card) ? "Enhanced strobe" : "",
>  			mmc_card_ddr52(card) ? "DDR " : "",
>  			uhs_bus_speed_mode, type, card->rca);
>  	}
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 28b477d..f45ed10 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -235,6 +235,12 @@ static void mmc_select_card_type(struct mmc_card *card)
>  		avail_type |= EXT_CSD_CARD_TYPE_HS400_1_2V;
>  	}
>  
> +	if ((caps2 & MMC_CAP2_HS400_ENHANCED_STROBE) &&
> +	    card->ext_csd.strobe_support &&
> +	    ((card_type & EXT_CSD_CARD_TYPE_HS400_1_2V) ||
> +	     (card_type & EXT_CSD_CARD_TYPE_HS400_1_8V)))

Could use just "card_type & EXT_CSD_CARD_TYPE_HS400" here

> +		avail_type |= EXT_CSD_CARD_TYPE_HS400ES;
> +
>  	card->ext_csd.hs_max_dtr = hs_max_dtr;
>  	card->ext_csd.hs200_max_dtr = hs200_max_dtr;
>  	card->mmc_avail_type = avail_type;
> @@ -383,6 +389,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
>  			mmc_card_set_blockaddr(card);
>  	}
>  
> +	card->ext_csd.strobe_support = ext_csd[EXT_CSD_STROBE_SUPPORT];
>  	card->ext_csd.raw_card_type = ext_csd[EXT_CSD_CARD_TYPE];
>  	mmc_select_card_type(card);
>  
> @@ -1062,9 +1069,10 @@ static int mmc_select_hs400(struct mmc_card *card)
>  	u8 val;
>  
>  	/*
> -	 * HS400 mode requires 8-bit bus width
> +	 * HS400(ES) mode requires 8-bit bus width
>  	 */
> -	if (!(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400 &&
> +	if (!(((card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400) ||
> +		(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)) &&
>  	      host->ios.bus_width == MMC_BUS_WIDTH_8))
>  		return 0;
>  
> @@ -1097,9 +1105,26 @@ static int mmc_select_hs400(struct mmc_card *card)
>  	}
>  
>  	/* Switch card to DDR */
> +	val = EXT_CSD_DDR_BUS_WIDTH_8;
> +	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
> +		val |= EXT_CSD_BUS_WIDTH_STROBE;
> +		/*
> +		* Make sure we are in non-enhanced strobe mode before we
> +		* actually enable it in ext_csd.
> +		*/
> +		if (host->ops->prepare_enhanced_strobe)
> +			err = host->ops->prepare_enhanced_strobe(host, false);
> +
> +		if (err) {
> +			pr_err("%s: unprepare_enhanced strobe failed, err:%d\n",
> +				mmc_hostname(host), err);
> +			return err;
> +		}
> +	}
> +
>  	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>  			 EXT_CSD_BUS_WIDTH,
> -			 EXT_CSD_DDR_BUS_WIDTH_8,
> +			 val,
>  			 card->ext_csd.generic_cmd6_time);
>  	if (err) {
>  		pr_err("%s: switch to bus width for hs400 failed, err:%d\n",
> @@ -1124,6 +1149,35 @@ static int mmc_select_hs400(struct mmc_card *card)
>  	mmc_set_timing(host, MMC_TIMING_MMC_HS400);
>  	mmc_set_bus_speed(card);
>  
> +	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
> +		/* Controller enable enhanced strobe function */
> +		if (host->ops->prepare_enhanced_strobe)
> +			err = host->ops->prepare_enhanced_strobe(host, true);
> +
> +		if (err) {
> +			pr_err("%s: prepare enhanced strobe failed, err:%d\n",
> +				mmc_hostname(host), err);
> +			return err;
> +		}
> +
> +		/*
> +		 * After switching to hs400 enhanced strobe mode, we expect to
> +		 * verify whether it works or not. If controller can't handle
> +		 * bus width test, compare ext_csd previously read in 1 bit mode
> +		 * against ext_csd at new bus width
> +		 */

I don't think we need this.  Just checking (host->caps & MMC_CAP_8_BIT_DATA)
ought to be enough.  But if you want to play if safe, you could use
mmc_select_bus_width() in advance.

> +		if (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))
> +			err = mmc_compare_ext_csds(card, MMC_BUS_WIDTH_8);
> +		else
> +			err = mmc_bus_test(card, MMC_BUS_WIDTH_8);
> +
> +		if (err) {
> +			pr_warn("%s: switch to enhanced strobe failed\n",
> +				mmc_hostname(host));
> +			return err;
> +		}
> +	}
> +
>  	if (!send_status) {
>  		err = mmc_switch_status(card);
>  		if (err)
> @@ -1297,7 +1351,8 @@ err:
>  }
>  
>  /*
> - * Activate High Speed or HS200 mode if supported.
> + * Activate High Speed or HS200 mode if supported. For HS400
> + * with enhanced strobe mode, we should activate High Speed.
>   */
>  static int mmc_select_timing(struct mmc_card *card)
>  {
> @@ -1306,7 +1361,9 @@ static int mmc_select_timing(struct mmc_card *card)
>  	if (!mmc_can_ext_csd(card))
>  		goto bus_speed;
>  
> -	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
> +	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)
> +		err = mmc_select_hs(card);

In my view, you should just make a new function: mmc_select_hs400es()

> +	else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
>  		err = mmc_select_hs200(card);
>  	else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
>  		err = mmc_select_hs(card);
> @@ -1578,7 +1635,15 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>  	} else if (mmc_card_hs(card)) {
>  		/* Select the desired bus width optionally */
>  		err = mmc_select_bus_width(card);
> -		if (!IS_ERR_VALUE(err)) {
> +		if (IS_ERR_VALUE(err))
> +			goto free_card;
> +
> +		if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
> +			/* Directly from HS to HS400-ES */
> +			err = mmc_select_hs400(card);
> +			if (err)
> +				goto free_card;
> +		} else {
>  			err = mmc_select_hs_ddr(card);
>  			if (err)
>  				goto free_card;
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index eb0151b..22defc2 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -95,6 +95,7 @@ struct mmc_ext_csd {
>  	u8			raw_partition_support;	/* 160 */
>  	u8			raw_rpmb_size_mult;	/* 168 */
>  	u8			raw_erased_mem_count;	/* 181 */
> +	u8			strobe_support;		/* 184 */
>  	u8			raw_ext_csd_structure;	/* 194 */
>  	u8			raw_card_type;		/* 196 */
>  	u8			raw_driver_strength;	/* 197 */
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 11e89d3..19de56c 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -19,6 +19,7 @@
>  
>  #include <linux/mmc/core.h>
>  #include <linux/mmc/card.h>
> +#include <linux/mmc/mmc.h>
>  #include <linux/mmc/pm.h>
>  
>  struct mmc_ios {
> @@ -143,6 +144,8 @@ struct mmc_host_ops {
>  
>  	/* Prepare HS400 target operating frequency depending host driver */
>  	int	(*prepare_hs400_tuning)(struct mmc_host *host, struct mmc_ios *ios);
> +	/* Prepare enhanced strobe depending host driver */
> +	int	(*prepare_enhanced_strobe)(struct mmc_host *host, bool enable);

I don't see any need for an error return, so maybe make this return void.
Also, we could put "bool enhanced_strobe" in struct mmc_ios and pass ios
instead of enable.

>  	int	(*select_drive_strength)(struct mmc_card *card,
>  					 unsigned int max_dtr, int host_drv,
>  					 int card_drv, int *drv_type);
> @@ -518,6 +521,15 @@ static inline bool mmc_card_hs400(struct mmc_card *card)
>  	return card->host->ios.timing == MMC_TIMING_MMC_HS400;
>  }
>  
> +static inline bool mmc_card_hs400es(struct mmc_card *card)
> +{
> +	if (mmc_card_hs400(card) &&
> +	    (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES))
> +		return 1;

We shouldn't assume that we are using it, just because it is available.
How about putting "bool enhanced_strobe" in struct mmc_ios and checking that.

> +
> +	return 0;
> +}
> +
>  void mmc_retune_timer_stop(struct mmc_host *host);
>  
>  static inline void mmc_retune_needed(struct mmc_host *host)
> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
> index 15f2c4a..c376209 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -297,6 +297,7 @@ struct _mmc_csd {
>  #define EXT_CSD_PART_CONFIG		179	/* R/W */
>  #define EXT_CSD_ERASED_MEM_CONT		181	/* RO */
>  #define EXT_CSD_BUS_WIDTH		183	/* R/W */
> +#define EXT_CSD_STROBE_SUPPORT		184	/* RO */
>  #define EXT_CSD_HS_TIMING		185	/* R/W */
>  #define EXT_CSD_POWER_CLASS		187	/* R/W */
>  #define EXT_CSD_REV			192	/* RO */
> @@ -380,12 +381,14 @@ struct _mmc_csd {
>  #define EXT_CSD_CARD_TYPE_HS400_1_2V	(1<<7)	/* Card can run at 200MHz DDR, 1.2V */
>  #define EXT_CSD_CARD_TYPE_HS400		(EXT_CSD_CARD_TYPE_HS400_1_8V | \
>  					 EXT_CSD_CARD_TYPE_HS400_1_2V)
> +#define EXT_CSD_CARD_TYPE_HS400ES	(1<<8)	/* Card can run at HS400ES */
>  
>  #define EXT_CSD_BUS_WIDTH_1	0	/* Card is in 1 bit mode */
>  #define EXT_CSD_BUS_WIDTH_4	1	/* Card is in 4 bit mode */
>  #define EXT_CSD_BUS_WIDTH_8	2	/* Card is in 8 bit mode */
>  #define EXT_CSD_DDR_BUS_WIDTH_4	5	/* Card is in 4 bit DDR mode */
>  #define EXT_CSD_DDR_BUS_WIDTH_8	6	/* Card is in 8 bit DDR mode */
> +#define EXT_CSD_BUS_WIDTH_STROBE BIT(7)	/* Enhanced strobe mode */
>  
>  #define EXT_CSD_TIMING_BC	0	/* Backwards compatility */
>  #define EXT_CSD_TIMING_HS	1	/* High speed */
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/6] mmc: core: implement enhanced strobe support

[Shawn Lin]

On 2016/5/9 19:56, Adrian Hunter wrote:
> On 29/04/16 05:47, Shawn Lin wrote:
>> Controllers use data strobe line to latch data from devices
>> under hs400 mode, but not for cmd line. So since emmc 5.1, JEDEC
>> introduces enhanced strobe mode for latching cmd response from
>> emmc devices to host controllers. This new feature is optional,
>> so it depends both on device's cap and host's cap to decide
>> whether to use it or not.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>
> Thanks for doing this.  There are a couple of comments below, but generally
> I tend to think you should split out selecting HS400ES as a separate function.
>
>> ---
>>
>> Changes in v2: None
>>
>>  drivers/mmc/core/bus.c   |  3 +-
>>  drivers/mmc/core/mmc.c   | 77 ++++++++++++++++++++++++++++++++++++++++++++----
>>  include/linux/mmc/card.h |  1 +
>>  include/linux/mmc/host.h | 12 ++++++++
>>  include/linux/mmc/mmc.h  |  3 ++
>>  5 files changed, 89 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
>> index 4bc48f1..7e94b9d 100644
>> --- a/drivers/mmc/core/bus.c
>> +++ b/drivers/mmc/core/bus.c
>> @@ -332,12 +332,13 @@ int mmc_add_card(struct mmc_card *card)
>>  			mmc_card_ddr52(card) ? "DDR " : "",
>>  			type);
>>  	} else {
>> -		pr_info("%s: new %s%s%s%s%s card at address %04x\n",
>> +		pr_info("%s: new %s%s%s%s%s%s card at address %04x\n",
>>  			mmc_hostname(card->host),
>>  			mmc_card_uhs(card) ? "ultra high speed " :
>>  			(mmc_card_hs(card) ? "high speed " : ""),
>>  			mmc_card_hs400(card) ? "HS400 " :
>>  			(mmc_card_hs200(card) ? "HS200 " : ""),
>> +			mmc_card_hs400es(card) ? "Enhanced strobe" : "",
>>  			mmc_card_ddr52(card) ? "DDR " : "",
>>  			uhs_bus_speed_mode, type, card->rca);
>>  	}
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 28b477d..f45ed10 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -235,6 +235,12 @@ static void mmc_select_card_type(struct mmc_card *card)
>>  		avail_type |= EXT_CSD_CARD_TYPE_HS400_1_2V;
>>  	}
>>
>> +	if ((caps2 & MMC_CAP2_HS400_ENHANCED_STROBE) &&
>> +	    card->ext_csd.strobe_support &&
>> +	    ((card_type & EXT_CSD_CARD_TYPE_HS400_1_2V) ||
>> +	     (card_type & EXT_CSD_CARD_TYPE_HS400_1_8V)))
>
> Could use just "card_type & EXT_CSD_CARD_TYPE_HS400" here

okay, EXT_CSD_CARD_TYPE_HS400 contains 1_2V & 1_8V, so I will
use it.

>
>> +		avail_type |= EXT_CSD_CARD_TYPE_HS400ES;
>> +
>>  	card->ext_csd.hs_max_dtr = hs_max_dtr;
>>  	card->ext_csd.hs200_max_dtr = hs200_max_dtr;
>>  	card->mmc_avail_type = avail_type;
>> @@ -383,6 +389,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
>>  			mmc_card_set_blockaddr(card);
>>  	}
>>
>> +	card->ext_csd.strobe_support = ext_csd[EXT_CSD_STROBE_SUPPORT];
>>  	card->ext_csd.raw_card_type = ext_csd[EXT_CSD_CARD_TYPE];
>>  	mmc_select_card_type(card);
>>
>> @@ -1062,9 +1069,10 @@ static int mmc_select_hs400(struct mmc_card *card)
>>  	u8 val;
>>
>>  	/*
>> -	 * HS400 mode requires 8-bit bus width
>> +	 * HS400(ES) mode requires 8-bit bus width
>>  	 */
>> -	if (!(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400 &&
>> +	if (!(((card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400) ||
>> +		(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)) &&
>>  	      host->ios.bus_width == MMC_BUS_WIDTH_8))
>>  		return 0;
>>
>> @@ -1097,9 +1105,26 @@ static int mmc_select_hs400(struct mmc_card *card)
>>  	}
>>
>>  	/* Switch card to DDR */
>> +	val = EXT_CSD_DDR_BUS_WIDTH_8;
>> +	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
>> +		val |= EXT_CSD_BUS_WIDTH_STROBE;
>> +		/*
>> +		* Make sure we are in non-enhanced strobe mode before we
>> +		* actually enable it in ext_csd.
>> +		*/
>> +		if (host->ops->prepare_enhanced_strobe)
>> +			err = host->ops->prepare_enhanced_strobe(host, false);
>> +
>> +		if (err) {
>> +			pr_err("%s: unprepare_enhanced strobe failed, err:%d\n",
>> +				mmc_hostname(host), err);
>> +			return err;
>> +		}
>> +	}
>> +
>>  	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>  			 EXT_CSD_BUS_WIDTH,
>> -			 EXT_CSD_DDR_BUS_WIDTH_8,
>> +			 val,
>>  			 card->ext_csd.generic_cmd6_time);
>>  	if (err) {
>>  		pr_err("%s: switch to bus width for hs400 failed, err:%d\n",
>> @@ -1124,6 +1149,35 @@ static int mmc_select_hs400(struct mmc_card *card)
>>  	mmc_set_timing(host, MMC_TIMING_MMC_HS400);
>>  	mmc_set_bus_speed(card);
>>
>> +	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
>> +		/* Controller enable enhanced strobe function */
>> +		if (host->ops->prepare_enhanced_strobe)
>> +			err = host->ops->prepare_enhanced_strobe(host, true);
>> +
>> +		if (err) {
>> +			pr_err("%s: prepare enhanced strobe failed, err:%d\n",
>> +				mmc_hostname(host), err);
>> +			return err;
>> +		}
>> +
>> +		/*
>> +		 * After switching to hs400 enhanced strobe mode, we expect to
>> +		 * verify whether it works or not. If controller can't handle
>> +		 * bus width test, compare ext_csd previously read in 1 bit mode
>> +		 * against ext_csd at new bus width
>> +		 */
>
> I don't think we need this.  Just checking (host->caps & MMC_CAP_8_BIT_DATA)
> ought to be enough.  But if you want to play if safe, you could use
> mmc_select_bus_width() in advance.

Ulf also questioned the requirment of doing this.
I consider droping this checking for the next version.

>
>> +		if (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))
>> +			err = mmc_compare_ext_csds(card, MMC_BUS_WIDTH_8);
>> +		else
>> +			err = mmc_bus_test(card, MMC_BUS_WIDTH_8);
>> +
>> +		if (err) {
>> +			pr_warn("%s: switch to enhanced strobe failed\n",
>> +				mmc_hostname(host));
>> +			return err;
>> +		}
>> +	}
>> +
>>  	if (!send_status) {
>>  		err = mmc_switch_status(card);
>>  		if (err)
>> @@ -1297,7 +1351,8 @@ err:
>>  }
>>
>>  /*
>> - * Activate High Speed or HS200 mode if supported.
>> + * Activate High Speed or HS200 mode if supported. For HS400
>> + * with enhanced strobe mode, we should activate High Speed.
>>   */
>>  static int mmc_select_timing(struct mmc_card *card)
>>  {
>> @@ -1306,7 +1361,9 @@ static int mmc_select_timing(struct mmc_card *card)
>>  	if (!mmc_can_ext_csd(card))
>>  		goto bus_speed;
>>
>> -	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
>> +	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)
>> +		err = mmc_select_hs(card);
>
> In my view, you should just make a new function: mmc_select_hs400es()

I will spilt a new function to handle the hs400es timing slection.

>
>> +	else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
>>  		err = mmc_select_hs200(card);
>>  	else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
>>  		err = mmc_select_hs(card);
>> @@ -1578,7 +1635,15 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>>  	} else if (mmc_card_hs(card)) {
>>  		/* Select the desired bus width optionally */
>>  		err = mmc_select_bus_width(card);
>> -		if (!IS_ERR_VALUE(err)) {
>> +		if (IS_ERR_VALUE(err))
>> +			goto free_card;
>> +
>> +		if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
>> +			/* Directly from HS to HS400-ES */
>> +			err = mmc_select_hs400(card);
>> +			if (err)
>> +				goto free_card;
>> +		} else {
>>  			err = mmc_select_hs_ddr(card);
>>  			if (err)
>>  				goto free_card;
>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>> index eb0151b..22defc2 100644
>> --- a/include/linux/mmc/card.h
>> +++ b/include/linux/mmc/card.h
>> @@ -95,6 +95,7 @@ struct mmc_ext_csd {
>>  	u8			raw_partition_support;	/* 160 */
>>  	u8			raw_rpmb_size_mult;	/* 168 */
>>  	u8			raw_erased_mem_count;	/* 181 */
>> +	u8			strobe_support;		/* 184 */
>>  	u8			raw_ext_csd_structure;	/* 194 */
>>  	u8			raw_card_type;		/* 196 */
>>  	u8			raw_driver_strength;	/* 197 */
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index 11e89d3..19de56c 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -19,6 +19,7 @@
>>
>>  #include <linux/mmc/core.h>
>>  #include <linux/mmc/card.h>
>> +#include <linux/mmc/mmc.h>
>>  #include <linux/mmc/pm.h>
>>
>>  struct mmc_ios {
>> @@ -143,6 +144,8 @@ struct mmc_host_ops {
>>
>>  	/* Prepare HS400 target operating frequency depending host driver */
>>  	int	(*prepare_hs400_tuning)(struct mmc_host *host, struct mmc_ios *ios);
>> +	/* Prepare enhanced strobe depending host driver */
>> +	int	(*prepare_enhanced_strobe)(struct mmc_host *host, bool enable);
>
> I don't see any need for an error return, so maybe make this return void.
> Also, we could put "bool enhanced_strobe" in struct mmc_ios and pass ios
> instead of enable.

yep, currently there's no need to check it.
I will change the name to hs400_enhacned_strobe as Ulf's suggestion,
and pass mmc_ios to this callback as well.

>
>>  	int	(*select_drive_strength)(struct mmc_card *card,
>>  					 unsigned int max_dtr, int host_drv,
>>  					 int card_drv, int *drv_type);
>> @@ -518,6 +521,15 @@ static inline bool mmc_card_hs400(struct mmc_card *card)
>>  	return card->host->ios.timing == MMC_TIMING_MMC_HS400;
>>  }
>>
>> +static inline bool mmc_card_hs400es(struct mmc_card *card)
>> +{
>> +	if (mmc_card_hs400(card) &&
>> +	    (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES))
>> +		return 1;
>
> We shouldn't assume that we are using it, just because it is available.
> How about putting "bool enhanced_strobe" in struct mmc_ios and checking that.
>

Good catch. Thanks.

>> +
>> +	return 0;
>> +}
>> +
>>  void mmc_retune_timer_stop(struct mmc_host *host);
>>
>>  static inline void mmc_retune_needed(struct mmc_host *host)
>> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
>> index 15f2c4a..c376209 100644
>> --- a/include/linux/mmc/mmc.h
>> +++ b/include/linux/mmc/mmc.h
>> @@ -297,6 +297,7 @@ struct _mmc_csd {
>>  #define EXT_CSD_PART_CONFIG		179	/* R/W */
>>  #define EXT_CSD_ERASED_MEM_CONT		181	/* RO */
>>  #define EXT_CSD_BUS_WIDTH		183	/* R/W */
>> +#define EXT_CSD_STROBE_SUPPORT		184	/* RO */
>>  #define EXT_CSD_HS_TIMING		185	/* R/W */
>>  #define EXT_CSD_POWER_CLASS		187	/* R/W */
>>  #define EXT_CSD_REV			192	/* RO */
>> @@ -380,12 +381,14 @@ struct _mmc_csd {
>>  #define EXT_CSD_CARD_TYPE_HS400_1_2V	(1<<7)	/* Card can run at 200MHz DDR, 1.2V */
>>  #define EXT_CSD_CARD_TYPE_HS400		(EXT_CSD_CARD_TYPE_HS400_1_8V | \
>>  					 EXT_CSD_CARD_TYPE_HS400_1_2V)
>> +#define EXT_CSD_CARD_TYPE_HS400ES	(1<<8)	/* Card can run at HS400ES */
>>
>>  #define EXT_CSD_BUS_WIDTH_1	0	/* Card is in 1 bit mode */
>>  #define EXT_CSD_BUS_WIDTH_4	1	/* Card is in 4 bit mode */
>>  #define EXT_CSD_BUS_WIDTH_8	2	/* Card is in 8 bit mode */
>>  #define EXT_CSD_DDR_BUS_WIDTH_4	5	/* Card is in 4 bit DDR mode */
>>  #define EXT_CSD_DDR_BUS_WIDTH_8	6	/* Card is in 8 bit DDR mode */
>> +#define EXT_CSD_BUS_WIDTH_STROBE BIT(7)	/* Enhanced strobe mode */
>>
>>  #define EXT_CSD_TIMING_BC	0	/* Backwards compatility */
>>  #define EXT_CSD_TIMING_HS	1	/* High speed */
>>
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>

Re: [PATCH v2 3/6] mmc: core: implement enhanced strobe support

[Shawn Lin]

On 2016/5/9 19:56, Adrian Hunter wrote:
> On 29/04/16 05:47, Shawn Lin wrote:
>> Controllers use data strobe line to latch data from devices
>> under hs400 mode, but not for cmd line. So since emmc 5.1, JEDEC
>> introduces enhanced strobe mode for latching cmd response from
>> emmc devices to host controllers. This new feature is optional,
>> so it depends both on device's cap and host's cap to decide
>> whether to use it or not.
>>
>> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>
> Thanks for doing this.  There are a couple of comments below, but generally
> I tend to think you should split out selecting HS400ES as a separate function.
>
>> ---
>>
>> Changes in v2: None
>>
>>  drivers/mmc/core/bus.c   |  3 +-
>>  drivers/mmc/core/mmc.c   | 77 ++++++++++++++++++++++++++++++++++++++++++++----
>>  include/linux/mmc/card.h |  1 +
>>  include/linux/mmc/host.h | 12 ++++++++
>>  include/linux/mmc/mmc.h  |  3 ++
>>  5 files changed, 89 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
>> index 4bc48f1..7e94b9d 100644
>> --- a/drivers/mmc/core/bus.c
>> +++ b/drivers/mmc/core/bus.c
>> @@ -332,12 +332,13 @@ int mmc_add_card(struct mmc_card *card)
>>  			mmc_card_ddr52(card) ? "DDR " : "",
>>  			type);
>>  	} else {
>> -		pr_info("%s: new %s%s%s%s%s card at address %04x\n",
>> +		pr_info("%s: new %s%s%s%s%s%s card at address %04x\n",
>>  			mmc_hostname(card->host),
>>  			mmc_card_uhs(card) ? "ultra high speed " :
>>  			(mmc_card_hs(card) ? "high speed " : ""),
>>  			mmc_card_hs400(card) ? "HS400 " :
>>  			(mmc_card_hs200(card) ? "HS200 " : ""),
>> +			mmc_card_hs400es(card) ? "Enhanced strobe" : "",
>>  			mmc_card_ddr52(card) ? "DDR " : "",
>>  			uhs_bus_speed_mode, type, card->rca);
>>  	}
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 28b477d..f45ed10 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -235,6 +235,12 @@ static void mmc_select_card_type(struct mmc_card *card)
>>  		avail_type |= EXT_CSD_CARD_TYPE_HS400_1_2V;
>>  	}
>>
>> +	if ((caps2 & MMC_CAP2_HS400_ENHANCED_STROBE) &&
>> +	    card->ext_csd.strobe_support &&
>> +	    ((card_type & EXT_CSD_CARD_TYPE_HS400_1_2V) ||
>> +	     (card_type & EXT_CSD_CARD_TYPE_HS400_1_8V)))
>
> Could use just "card_type & EXT_CSD_CARD_TYPE_HS400" here

okay, EXT_CSD_CARD_TYPE_HS400 contains 1_2V & 1_8V, so I will
use it.

>
>> +		avail_type |= EXT_CSD_CARD_TYPE_HS400ES;
>> +
>>  	card->ext_csd.hs_max_dtr = hs_max_dtr;
>>  	card->ext_csd.hs200_max_dtr = hs200_max_dtr;
>>  	card->mmc_avail_type = avail_type;
>> @@ -383,6 +389,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
>>  			mmc_card_set_blockaddr(card);
>>  	}
>>
>> +	card->ext_csd.strobe_support = ext_csd[EXT_CSD_STROBE_SUPPORT];
>>  	card->ext_csd.raw_card_type = ext_csd[EXT_CSD_CARD_TYPE];
>>  	mmc_select_card_type(card);
>>
>> @@ -1062,9 +1069,10 @@ static int mmc_select_hs400(struct mmc_card *card)
>>  	u8 val;
>>
>>  	/*
>> -	 * HS400 mode requires 8-bit bus width
>> +	 * HS400(ES) mode requires 8-bit bus width
>>  	 */
>> -	if (!(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400 &&
>> +	if (!(((card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400) ||
>> +		(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)) &&
>>  	      host->ios.bus_width == MMC_BUS_WIDTH_8))
>>  		return 0;
>>
>> @@ -1097,9 +1105,26 @@ static int mmc_select_hs400(struct mmc_card *card)
>>  	}
>>
>>  	/* Switch card to DDR */
>> +	val = EXT_CSD_DDR_BUS_WIDTH_8;
>> +	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
>> +		val |= EXT_CSD_BUS_WIDTH_STROBE;
>> +		/*
>> +		* Make sure we are in non-enhanced strobe mode before we
>> +		* actually enable it in ext_csd.
>> +		*/
>> +		if (host->ops->prepare_enhanced_strobe)
>> +			err = host->ops->prepare_enhanced_strobe(host, false);
>> +
>> +		if (err) {
>> +			pr_err("%s: unprepare_enhanced strobe failed, err:%d\n",
>> +				mmc_hostname(host), err);
>> +			return err;
>> +		}
>> +	}
>> +
>>  	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>  			 EXT_CSD_BUS_WIDTH,
>> -			 EXT_CSD_DDR_BUS_WIDTH_8,
>> +			 val,
>>  			 card->ext_csd.generic_cmd6_time);
>>  	if (err) {
>>  		pr_err("%s: switch to bus width for hs400 failed, err:%d\n",
>> @@ -1124,6 +1149,35 @@ static int mmc_select_hs400(struct mmc_card *card)
>>  	mmc_set_timing(host, MMC_TIMING_MMC_HS400);
>>  	mmc_set_bus_speed(card);
>>
>> +	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
>> +		/* Controller enable enhanced strobe function */
>> +		if (host->ops->prepare_enhanced_strobe)
>> +			err = host->ops->prepare_enhanced_strobe(host, true);
>> +
>> +		if (err) {
>> +			pr_err("%s: prepare enhanced strobe failed, err:%d\n",
>> +				mmc_hostname(host), err);
>> +			return err;
>> +		}
>> +
>> +		/*
>> +		 * After switching to hs400 enhanced strobe mode, we expect to
>> +		 * verify whether it works or not. If controller can't handle
>> +		 * bus width test, compare ext_csd previously read in 1 bit mode
>> +		 * against ext_csd at new bus width
>> +		 */
>
> I don't think we need this.  Just checking (host->caps & MMC_CAP_8_BIT_DATA)
> ought to be enough.  But if you want to play if safe, you could use
> mmc_select_bus_width() in advance.

Ulf also questioned the requirment of doing this.
I consider droping this checking for the next version.

>
>> +		if (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))
>> +			err = mmc_compare_ext_csds(card, MMC_BUS_WIDTH_8);
>> +		else
>> +			err = mmc_bus_test(card, MMC_BUS_WIDTH_8);
>> +
>> +		if (err) {
>> +			pr_warn("%s: switch to enhanced strobe failed\n",
>> +				mmc_hostname(host));
>> +			return err;
>> +		}
>> +	}
>> +
>>  	if (!send_status) {
>>  		err = mmc_switch_status(card);
>>  		if (err)
>> @@ -1297,7 +1351,8 @@ err:
>>  }
>>
>>  /*
>> - * Activate High Speed or HS200 mode if supported.
>> + * Activate High Speed or HS200 mode if supported. For HS400
>> + * with enhanced strobe mode, we should activate High Speed.
>>   */
>>  static int mmc_select_timing(struct mmc_card *card)
>>  {
>> @@ -1306,7 +1361,9 @@ static int mmc_select_timing(struct mmc_card *card)
>>  	if (!mmc_can_ext_csd(card))
>>  		goto bus_speed;
>>
>> -	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
>> +	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)
>> +		err = mmc_select_hs(card);
>
> In my view, you should just make a new function: mmc_select_hs400es()

I will spilt a new function to handle the hs400es timing slection.

>
>> +	else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
>>  		err = mmc_select_hs200(card);
>>  	else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
>>  		err = mmc_select_hs(card);
>> @@ -1578,7 +1635,15 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>>  	} else if (mmc_card_hs(card)) {
>>  		/* Select the desired bus width optionally */
>>  		err = mmc_select_bus_width(card);
>> -		if (!IS_ERR_VALUE(err)) {
>> +		if (IS_ERR_VALUE(err))
>> +			goto free_card;
>> +
>> +		if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
>> +			/* Directly from HS to HS400-ES */
>> +			err = mmc_select_hs400(card);
>> +			if (err)
>> +				goto free_card;
>> +		} else {
>>  			err = mmc_select_hs_ddr(card);
>>  			if (err)
>>  				goto free_card;
>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>> index eb0151b..22defc2 100644
>> --- a/include/linux/mmc/card.h
>> +++ b/include/linux/mmc/card.h
>> @@ -95,6 +95,7 @@ struct mmc_ext_csd {
>>  	u8			raw_partition_support;	/* 160 */
>>  	u8			raw_rpmb_size_mult;	/* 168 */
>>  	u8			raw_erased_mem_count;	/* 181 */
>> +	u8			strobe_support;		/* 184 */
>>  	u8			raw_ext_csd_structure;	/* 194 */
>>  	u8			raw_card_type;		/* 196 */
>>  	u8			raw_driver_strength;	/* 197 */
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index 11e89d3..19de56c 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -19,6 +19,7 @@
>>
>>  #include <linux/mmc/core.h>
>>  #include <linux/mmc/card.h>
>> +#include <linux/mmc/mmc.h>
>>  #include <linux/mmc/pm.h>
>>
>>  struct mmc_ios {
>> @@ -143,6 +144,8 @@ struct mmc_host_ops {
>>
>>  	/* Prepare HS400 target operating frequency depending host driver */
>>  	int	(*prepare_hs400_tuning)(struct mmc_host *host, struct mmc_ios *ios);
>> +	/* Prepare enhanced strobe depending host driver */
>> +	int	(*prepare_enhanced_strobe)(struct mmc_host *host, bool enable);
>
> I don't see any need for an error return, so maybe make this return void.
> Also, we could put "bool enhanced_strobe" in struct mmc_ios and pass ios
> instead of enable.

yep, currently there's no need to check it.
I will change the name to hs400_enhacned_strobe as Ulf's suggestion,
and pass mmc_ios to this callback as well.

>
>>  	int	(*select_drive_strength)(struct mmc_card *card,
>>  					 unsigned int max_dtr, int host_drv,
>>  					 int card_drv, int *drv_type);
>> @@ -518,6 +521,15 @@ static inline bool mmc_card_hs400(struct mmc_card *card)
>>  	return card->host->ios.timing == MMC_TIMING_MMC_HS400;
>>  }
>>
>> +static inline bool mmc_card_hs400es(struct mmc_card *card)
>> +{
>> +	if (mmc_card_hs400(card) &&
>> +	    (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES))
>> +		return 1;
>
> We shouldn't assume that we are using it, just because it is available.
> How about putting "bool enhanced_strobe" in struct mmc_ios and checking that.
>

Good catch. Thanks.

>> +
>> +	return 0;
>> +}
>> +
>>  void mmc_retune_timer_stop(struct mmc_host *host);
>>
>>  static inline void mmc_retune_needed(struct mmc_host *host)
>> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
>> index 15f2c4a..c376209 100644
>> --- a/include/linux/mmc/mmc.h
>> +++ b/include/linux/mmc/mmc.h
>> @@ -297,6 +297,7 @@ struct _mmc_csd {
>>  #define EXT_CSD_PART_CONFIG		179	/* R/W */
>>  #define EXT_CSD_ERASED_MEM_CONT		181	/* RO */
>>  #define EXT_CSD_BUS_WIDTH		183	/* R/W */
>> +#define EXT_CSD_STROBE_SUPPORT		184	/* RO */
>>  #define EXT_CSD_HS_TIMING		185	/* R/W */
>>  #define EXT_CSD_POWER_CLASS		187	/* R/W */
>>  #define EXT_CSD_REV			192	/* RO */
>> @@ -380,12 +381,14 @@ struct _mmc_csd {
>>  #define EXT_CSD_CARD_TYPE_HS400_1_2V	(1<<7)	/* Card can run at 200MHz DDR, 1.2V */
>>  #define EXT_CSD_CARD_TYPE_HS400		(EXT_CSD_CARD_TYPE_HS400_1_8V | \
>>  					 EXT_CSD_CARD_TYPE_HS400_1_2V)
>> +#define EXT_CSD_CARD_TYPE_HS400ES	(1<<8)	/* Card can run at HS400ES */
>>
>>  #define EXT_CSD_BUS_WIDTH_1	0	/* Card is in 1 bit mode */
>>  #define EXT_CSD_BUS_WIDTH_4	1	/* Card is in 4 bit mode */
>>  #define EXT_CSD_BUS_WIDTH_8	2	/* Card is in 8 bit mode */
>>  #define EXT_CSD_DDR_BUS_WIDTH_4	5	/* Card is in 4 bit DDR mode */
>>  #define EXT_CSD_DDR_BUS_WIDTH_8	6	/* Card is in 8 bit DDR mode */
>> +#define EXT_CSD_BUS_WIDTH_STROBE BIT(7)	/* Enhanced strobe mode */
>>
>>  #define EXT_CSD_TIMING_BC	0	/* Backwards compatility */
>>  #define EXT_CSD_TIMING_HS	1	/* High speed */
>>
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>

[PATCH v2 4/6] mmc: debugfs: add HS400 enhanced strobe description

[Shawn Lin]

We inctroduce HS400 with enhanced strobe function, so we need
add it for debug show.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v2: None

 drivers/mmc/core/debugfs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index 9382a57..51d7c38 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -148,7 +148,9 @@ static int mmc_ios_show(struct seq_file *s, void *data)
 		str = "mmc HS200";
 		break;
 	case MMC_TIMING_MMC_HS400:
-		str = "mmc HS400";
+		mmc_card_hs400es(host->card) ?
+			(str = "mmc HS400 enhanced strobe") :
+			(str = "mmc HS400");
 		break;
 	default:
 		str = "invalid";
-- 
2.3.7

Re: [PATCH v2 4/6] mmc: debugfs: add HS400 enhanced strobe description

[Adrian Hunter]

On 29/04/16 05:48, Shawn Lin wrote:
> We inctroduce HS400 with enhanced strobe function, so we need

inctroduce -> introduced

> add it for debug show.

add -> to add

> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
> Changes in v2: None
> 
>  drivers/mmc/core/debugfs.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
> index 9382a57..51d7c38 100644
> --- a/drivers/mmc/core/debugfs.c
> +++ b/drivers/mmc/core/debugfs.c
> @@ -148,7 +148,9 @@ static int mmc_ios_show(struct seq_file *s, void *data)
>  		str = "mmc HS200";
>  		break;
>  	case MMC_TIMING_MMC_HS400:
> -		str = "mmc HS400";
> +		mmc_card_hs400es(host->card) ?
> +			(str = "mmc HS400 enhanced strobe") :
> +			(str = "mmc HS400");
>  		break;
>  	default:
>  		str = "invalid";
> 

Re: [PATCH v2 4/6] mmc: debugfs: add HS400 enhanced strobe description

[Adrian Hunter]

On 29/04/16 05:48, Shawn Lin wrote:
> We inctroduce HS400 with enhanced strobe function, so we need

inctroduce -> introduced

> add it for debug show.

add -> to add

> 
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> 
> Changes in v2: None
> 
>  drivers/mmc/core/debugfs.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
> index 9382a57..51d7c38 100644
> --- a/drivers/mmc/core/debugfs.c
> +++ b/drivers/mmc/core/debugfs.c
> @@ -148,7 +148,9 @@ static int mmc_ios_show(struct seq_file *s, void *data)
>  		str = "mmc HS200";
>  		break;
>  	case MMC_TIMING_MMC_HS400:
> -		str = "mmc HS400";
> +		mmc_card_hs400es(host->card) ?
> +			(str = "mmc HS400 enhanced strobe") :
> +			(str = "mmc HS400");
>  		break;
>  	default:
>  		str = "invalid";
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 5/6] mmc: sdhci: implement enhanced strobe callback

[Shawn Lin]

Enhanced strobe stuff currently is beyond the scope
of Secure Digital Host Controller Interface. So we can't
find a register here to enable/disable it. We experct
variant drivers to finish the details according to their
vendor settings.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v2: None

 drivers/mmc/host/sdhci.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 94cffa7..a04e4c4 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2049,6 +2049,16 @@ out_unlock:
 	return err;
 }
 
+static int sdhci_prepare_enhanced_strobe(struct mmc_host *mmc, bool enable)
+{
+	/*
+	* Currently we can't find a register to enable enhanced strobe
+	* function for standard sdhci, so we expect variant drivers to
+	* overwrite it.
+	*/
+	return -EINVAL;
+}
+
 static int sdhci_select_drive_strength(struct mmc_card *card,
 				       unsigned int max_dtr, int host_drv,
 				       int card_drv, int *drv_type)
@@ -2158,6 +2168,7 @@ static const struct mmc_host_ops sdhci_ops = {
 	.enable_sdio_irq = sdhci_enable_sdio_irq,
 	.start_signal_voltage_switch	= sdhci_start_signal_voltage_switch,
 	.prepare_hs400_tuning		= sdhci_prepare_hs400_tuning,
+	.prepare_enhanced_strobe	= sdhci_prepare_enhanced_strobe,
 	.execute_tuning			= sdhci_execute_tuning,
 	.select_drive_strength		= sdhci_select_drive_strength,
 	.card_event			= sdhci_card_event,
-- 
2.3.7

Re: [PATCH v2 5/6] mmc: sdhci: implement enhanced strobe callback

[Adrian Hunter]

On 29/04/16 05:48, Shawn Lin wrote:
> Enhanced strobe stuff currently is beyond the scope
> of Secure Digital Host Controller Interface. So we can't
> find a register here to enable/disable it. We experct
> variant drivers to finish the details according to their
> vendor settings.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
> Changes in v2: None
> 
>  drivers/mmc/host/sdhci.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 94cffa7..a04e4c4 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2049,6 +2049,16 @@ out_unlock:
>  	return err;
>  }
>  
> +static int sdhci_prepare_enhanced_strobe(struct mmc_host *mmc, bool enable)
> +{
> +	/*
> +	* Currently we can't find a register to enable enhanced strobe
> +	* function for standard sdhci, so we expect variant drivers to
> +	* overwrite it.
> +	*/
> +	return -EINVAL;
> +}

Since we don't need it at the moment, let's not have it at all for now.

> +
>  static int sdhci_select_drive_strength(struct mmc_card *card,
>  				       unsigned int max_dtr, int host_drv,
>  				       int card_drv, int *drv_type)
> @@ -2158,6 +2168,7 @@ static const struct mmc_host_ops sdhci_ops = {
>  	.enable_sdio_irq = sdhci_enable_sdio_irq,
>  	.start_signal_voltage_switch	= sdhci_start_signal_voltage_switch,
>  	.prepare_hs400_tuning		= sdhci_prepare_hs400_tuning,
> +	.prepare_enhanced_strobe	= sdhci_prepare_enhanced_strobe,
>  	.execute_tuning			= sdhci_execute_tuning,
>  	.select_drive_strength		= sdhci_select_drive_strength,
>  	.card_event			= sdhci_card_event,
> 

Re: [PATCH v2 5/6] mmc: sdhci: implement enhanced strobe callback

[Adrian Hunter]

On 29/04/16 05:48, Shawn Lin wrote:
> Enhanced strobe stuff currently is beyond the scope
> of Secure Digital Host Controller Interface. So we can't
> find a register here to enable/disable it. We experct
> variant drivers to finish the details according to their
> vendor settings.
> 
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> 
> Changes in v2: None
> 
>  drivers/mmc/host/sdhci.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 94cffa7..a04e4c4 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2049,6 +2049,16 @@ out_unlock:
>  	return err;
>  }
>  
> +static int sdhci_prepare_enhanced_strobe(struct mmc_host *mmc, bool enable)
> +{
> +	/*
> +	* Currently we can't find a register to enable enhanced strobe
> +	* function for standard sdhci, so we expect variant drivers to
> +	* overwrite it.
> +	*/
> +	return -EINVAL;
> +}

Since we don't need it at the moment, let's not have it at all for now.

> +
>  static int sdhci_select_drive_strength(struct mmc_card *card,
>  				       unsigned int max_dtr, int host_drv,
>  				       int card_drv, int *drv_type)
> @@ -2158,6 +2168,7 @@ static const struct mmc_host_ops sdhci_ops = {
>  	.enable_sdio_irq = sdhci_enable_sdio_irq,
>  	.start_signal_voltage_switch	= sdhci_start_signal_voltage_switch,
>  	.prepare_hs400_tuning		= sdhci_prepare_hs400_tuning,
> +	.prepare_enhanced_strobe	= sdhci_prepare_enhanced_strobe,
>  	.execute_tuning			= sdhci_execute_tuning,
>  	.select_drive_strength		= sdhci_select_drive_strength,
>  	.card_event			= sdhci_card_event,
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 6/6] mmc: sdhci-of-arasan: overwrite enhanced strobe callback

[Shawn Lin]

Currently sdhci-arasan 5.1 can support enhanced strobe function,
and we now limit it just for "arasan,sdhci-5.1". Add
mmc-hs400-enhanced-strobe in DT to enable the function if we'r sure
our controller can support it.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v2: None

 drivers/mmc/host/sdhci-of-arasan.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index 20b859e..a0e4536 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -25,7 +25,9 @@
 #include "sdhci-pltfm.h"
 
 #define SDHCI_ARASAN_CLK_CTRL_OFFSET	0x2c
+#define SDHCI_ARASAN_VENDOR_REGISTER	0x78
 
+#define VENDOR_ENHANCED_STROBE		BIT(0)
 #define CLK_CTRL_TIMEOUT_SHIFT		16
 #define CLK_CTRL_TIMEOUT_MASK		(0xf << CLK_CTRL_TIMEOUT_SHIFT)
 #define CLK_CTRL_TIMEOUT_MIN_EXP	13
@@ -73,6 +75,23 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
 		phy_power_on(sdhci_arasan->phy);
 }
 
+static int sdhci_arasan_enhanced_strobe(struct mmc_host *mmc,
+					bool enable)
+{
+	u32 vendor;
+	struct sdhci_host *host = mmc_priv(mmc);
+
+	vendor = readl(host->ioaddr + SDHCI_ARASAN_VENDOR_REGISTER);
+	if (enable)
+		vendor |= VENDOR_ENHANCED_STROBE;
+	else
+		vendor &= (~VENDOR_ENHANCED_STROBE);
+
+	writel(vendor, host->ioaddr + SDHCI_ARASAN_VENDOR_REGISTER);
+
+	return 0;
+}
+
 static struct sdhci_ops sdhci_arasan_ops = {
 	.set_clock = sdhci_arasan_set_clock,
 	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
@@ -239,6 +258,9 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 			dev_err(&pdev->dev, "phy_power_on err.\n");
 			goto err_phy_power;
 		}
+
+		host->mmc_host_ops.prepare_enhanced_strobe =
+					sdhci_arasan_enhanced_strobe;
 	}
 
 	ret = sdhci_add_host(host);
-- 
2.3.7

Re: [PATCH v2 6/6] mmc: sdhci-of-arasan: overwrite enhanced strobe callback

[Sören Brinkmann]

On Fri, 2016-04-29 at 10:48:34 +0800, Shawn Lin wrote:
> Currently sdhci-arasan 5.1 can support enhanced strobe function,
> and we now limit it just for "arasan,sdhci-5.1". Add
> mmc-hs400-enhanced-strobe in DT to enable the function if we'r sure
> our controller can support it.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
> Changes in v2: None
> 
>  drivers/mmc/host/sdhci-of-arasan.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 20b859e..a0e4536 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -25,7 +25,9 @@
>  #include "sdhci-pltfm.h"
>  
>  #define SDHCI_ARASAN_CLK_CTRL_OFFSET	0x2c
> +#define SDHCI_ARASAN_VENDOR_REGISTER	0x78

Just as a note: This register doesn't seem to exist in the IP version
Zynq is using. So, as long as we exclude that version from accessing it,
things should be fine (IIUC, that is the case).

Acked-by: Sören Brinkmann <soren.brinkmann@xilinx.com>

	Sören

Re: [PATCH v2 6/6] mmc: sdhci-of-arasan: overwrite enhanced strobe callback

[Sören Brinkmann]

On Fri, 2016-04-29 at 10:48:34 +0800, Shawn Lin wrote:
> Currently sdhci-arasan 5.1 can support enhanced strobe function,
> and we now limit it just for "arasan,sdhci-5.1". Add
> mmc-hs400-enhanced-strobe in DT to enable the function if we'r sure
> our controller can support it.
> 
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> 
> Changes in v2: None
> 
>  drivers/mmc/host/sdhci-of-arasan.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 20b859e..a0e4536 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -25,7 +25,9 @@
>  #include "sdhci-pltfm.h"
>  
>  #define SDHCI_ARASAN_CLK_CTRL_OFFSET	0x2c
> +#define SDHCI_ARASAN_VENDOR_REGISTER	0x78

Just as a note: This register doesn't seem to exist in the IP version
Zynq is using. So, as long as we exclude that version from accessing it,
things should be fine (IIUC, that is the case).

Acked-by: Sören Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>

	Sören
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html