[PATCH 0/4] mmc: sdhci: adding support for a new Fujitsu sdhci IP

[PATCH 0/4] mmc: sdhci: adding support for a new Fujitsu sdhci IP

[Vincent Yang]

Hi,
We are adding support for a new Fujitsu sdhci IP.
These patches are preparations and will be used by Fujitsu
SDHCI controller f_sdh30 driver.

These patches are against v3.16-rc4 mainline since nothing in
mmc-next at this moment.

These patches are tested on 3.16-rc4 integration tree.

We welcome any comment and advice about how to make any
improvements or better align them with upstream.

Thanks a lot!

Best regards,
Vincent Yang


Vincent Yang (4):
  mmc: sdhci: add quirk for voltage switch callback
  mmc: sdhci: add quirk for tuning work around
  mmc: sdhci: add quirk for single block transactions
  mmc: core: hold SD Clock before CMD11 during Signal

 drivers/mmc/core/core.c   |  4 ++--
 drivers/mmc/host/sdhci.c  | 15 ++++++++++++---
 drivers/mmc/host/sdhci.h  |  1 +
 include/linux/mmc/sdhci.h |  6 ++++++
 4 files changed, 21 insertions(+), 5 deletions(-)

-- 
1.9.0

[PATCH 1/4] mmc: sdhci: add quirk for voltage switch callback

[Vincent Yang]

This patch defines a quirk to do a callback when
switching voltages so do controller-specific
actions.
It is a preparation and will be used by Fujitsu
SDHCI controller f_sdh30 driver.

Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
---
 drivers/mmc/host/sdhci.c  | 5 +++++
 drivers/mmc/host/sdhci.h  | 1 +
 include/linux/mmc/sdhci.h | 2 ++
 3 files changed, 8 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 47055f3..d62262b 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1763,6 +1763,11 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		ctrl |= SDHCI_CTRL_VDD_180;
 		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
+		/* Some controller need to do more when switching */
+		if ((host->quirks2 & SDHCI_QUIRK2_VOLTAGE_SWITCH) &&
+		    host->ops->voltage_switch)
+			host->ops->voltage_switch(host);
+
 		/* Wait for 5ms */
 		usleep_range(5000, 5500);
 
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 4a5cd5e..63c7a46 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -292,6 +292,7 @@ struct sdhci_ops {
 	void    (*adma_workaround)(struct sdhci_host *host, u32 intmask);
 	void	(*platform_init)(struct sdhci_host *host);
 	void    (*card_event)(struct sdhci_host *host);
+	void	(*voltage_switch)(struct sdhci_host *host);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index 08abe99..5433f04 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -98,6 +98,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_BROKEN_HS200			(1<<6)
 /* Controller does not support DDR50 */
 #define SDHCI_QUIRK2_BROKEN_DDR50			(1<<7)
+/* Do a callback when switching voltages so do controller-specific actions */
+#define SDHCI_QUIRK2_VOLTAGE_SWITCH			(1<<8)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
1.9.0

[PATCH 1/4] mmc: sdhci: add quirk for voltage switch callback

[Russell King - ARM Linux]

On Sun, Jul 13, 2014 at 01:21:07PM +0800, Vincent Yang wrote:
> @@ -1763,6 +1763,11 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
>  		ctrl |= SDHCI_CTRL_VDD_180;
>  		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>  
> +		/* Some controller need to do more when switching */
> +		if ((host->quirks2 & SDHCI_QUIRK2_VOLTAGE_SWITCH) &&
> +		    host->ops->voltage_switch)
> +			host->ops->voltage_switch(host);
> +

Why do you heed SDHCI_QUIRK2_VOLTAGE_SWITCH?  Isn't populating
ops->voltage_switch enough? to indicate that something needs to be
done?

It would also be better to turn sdhci.c into a library, and have
the platform driver call the appropriate functions in sdhci rather
than having sdhci be a core driver with loads of quirks.  This is
what I've done in my previous series where I changed stuff such as
the set_bus_width(), set_uhs_signaling() and similar callbacks.

So, it probably makes more sense to split
sdhci_do_start_signal_voltage_switch() into a load of smaller library
functions which drivers can call in an appropriate sequence, rather
than having a quirk hook.

The problem with quirk hooks is that what is right for one device
is not right for another device - eventually you end up with lots
of quirk callbacks scattered on every alternate line.  That doesn't
scale.

Experienced kernel programmers know this and this is why words like
"framework" fill those who have encountered this problem with dread.
sdhci.c is a prime example of this kind of design mistake.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 1/4] mmc: sdhci: add quirk for voltage switch callback

[Vincent.Yang]

>-----Original Message-----
>From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
>On Sun, Jul 13, 2014 at 01:21:07PM +0800, Vincent Yang wrote:
>> @@ -1763,6 +1763,11 @@ static int sdhci_do_start_signal_voltage_switch(struct
>sdhci_host *host,
>>               ctrl |= SDHCI_CTRL_VDD_180;
>>               sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>
>> +             /* Some controller need to do more when switching */
>> +             if ((host->quirks2 & SDHCI_QUIRK2_VOLTAGE_SWITCH) &&
>> +                 host->ops->voltage_switch)
>> +                     host->ops->voltage_switch(host);
>> +
>
>Why do you heed SDHCI_QUIRK2_VOLTAGE_SWITCH?  Isn't populating
>ops->voltage_switch enough? to indicate that something needs to be
>done?

Hi Russell,
I will update it as below and remove SDHCI_QUIRK2_VOLTAGE_SWITCH in
next version.
+               /* Some controller need to do more when switching */
+               if (host->ops->voltage_switch)
+                       host->ops->voltage_switch(host);
+

Thanks a lot for your review!

Best regards,
Vincent Yang

>
>It would also be better to turn sdhci.c into a library, and have
>the platform driver call the appropriate functions in sdhci rather
>than having sdhci be a core driver with loads of quirks.  This is
>what I've done in my previous series where I changed stuff such as
>the set_bus_width(), set_uhs_signaling() and similar callbacks.
>
>So, it probably makes more sense to split
>sdhci_do_start_signal_voltage_switch() into a load of smaller library
>functions which drivers can call in an appropriate sequence, rather
>than having a quirk hook.
>
>The problem with quirk hooks is that what is right for one device
>is not right for another device - eventually you end up with lots
>of quirk callbacks scattered on every alternate line.  That doesn't
>scale.
>
>Experienced kernel programmers know this and this is why words like
>"framework" fill those who have encountered this problem with dread.
>sdhci.c is a prime example of this kind of design mistake.
>
>--
>FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
>according to speedtest.net.

[PATCH 2/4] mmc: sdhci: add quirk for tuning work around

[Vincent Yang]

This patch defines a quirk for tuning work
around for some sdhci host controller. It sets
both SDHCI_CTRL_EXEC_TUNING and SDHCI_CTRL_TUNED_CLK
for tuning.
It is a preparation and will be used by Fujitsu
SDHCI controller f_sdh30 driver.

Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
---
 drivers/mmc/host/sdhci.c  | 2 ++
 include/linux/mmc/sdhci.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index d62262b..900b4e4 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1867,6 +1867,8 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 
 	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
 	ctrl |= SDHCI_CTRL_EXEC_TUNING;
+	if (host->quirks2 & SDHCI_QUIRK2_TUNING_WORK_AROUND)
+		ctrl |= SDHCI_CTRL_TUNED_CLK;
 	sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
 	/*
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index 5433f04..bcbad45 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -100,6 +100,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_BROKEN_DDR50			(1<<7)
 /* Do a callback when switching voltages so do controller-specific actions */
 #define SDHCI_QUIRK2_VOLTAGE_SWITCH			(1<<8)
+/* forced tuned clock */
+#define SDHCI_QUIRK2_TUNING_WORK_AROUND		(1<<9)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
1.9.0

[PATCH 3/4] mmc: sdhci: add quirk for single block transactions

[Vincent Yang]

This patch defines a quirk to disable the block count
for single block transactions.
It is a preparation and will be used by Fujitsu
SDHCI controller f_sdh30 driver.

Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
---
 drivers/mmc/host/sdhci.c  | 8 +++++---
 include/linux/mmc/sdhci.h | 2 ++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 900b4e4..169e17d 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -876,7 +876,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 static void sdhci_set_transfer_mode(struct sdhci_host *host,
 	struct mmc_command *cmd)
 {
-	u16 mode;
+	u16 mode = 0;
 	struct mmc_data *data = cmd->data;
 
 	if (data == NULL) {
@@ -889,9 +889,11 @@ static void sdhci_set_transfer_mode(struct sdhci_host *host,
 
 	WARN_ON(!host->data);
 
-	mode = SDHCI_TRNS_BLK_CNT_EN;
+	if (!(host->quirks2 & SDHCI_QUIRK2_SUPPORT_SINGLE))
+		mode = SDHCI_TRNS_BLK_CNT_EN;
+
 	if (mmc_op_multi(cmd->opcode) || data->blocks > 1) {
-		mode |= SDHCI_TRNS_MULTI;
+		mode = SDHCI_TRNS_BLK_CNT_EN | SDHCI_TRNS_MULTI;
 		/*
 		 * If we are sending CMD23, CMD12 never gets sent
 		 * on successful completion (so no Auto-CMD12).
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index bcbad45..72072d1 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -102,6 +102,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_VOLTAGE_SWITCH			(1<<8)
 /* forced tuned clock */
 #define SDHCI_QUIRK2_TUNING_WORK_AROUND		(1<<9)
+/* disable the block count for single block transactions */
+#define SDHCI_QUIRK2_SUPPORT_SINGLE			(1<<10)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
1.9.0

[PATCH 4/4] mmc: core: hold SD Clock before CMD11 during Signal

[Vincent Yang]

Voltage Switch Procedure

This patch is to fix an issue found on mb86s7x platforms.

[symptom]
There are some UHS-1 SD memory cards sometimes cannot be detected correctly,
e.g., Transcend 600x SDXC 64GB UHS-1 memory card.
During Signal Voltage Switch Procedure, failure to switch is indicated
by the card holding DAT[3:0] low.

[analysis]
According to SD Host Controller Simplified Specification Version 3.00
chapter 3.6.1, the Signal Voltage Switch Procedure should be:
(1) Check S18A; (2) Issue CMD11; (3) Check CMD 11 response;
(4) Stop providing SD clock; (5) Check DAT[3:0] should be 0000b;
(6) Set 1.8V Signal Enable; (7) Wait 5ms; (8) Check 1.8V Signal Enable;
(9) Provide SD Clock; (10) Wait 1ms; (11) Check DAT[3:0] should be 1111b;
(12) error handling

With CONFIG_MMC_CLKGATE=y, sometimes there is one more gating/un-gating
SD clock between (2) and (3). In this case, some UHS-1 SD cards will hold
DAT[3:0] 0000b at (11) and thus fails Signal Voltage Switch Procedure.

[solution]
By mmc_host_clk_hold() before CMD11, the additional gating/un-gating
SD clock between (2) and (3) can be prevented and thus no failure at (11).
It has been verified with many UHS-1 SD cards on mb86s7x platforms and
works correctly.

Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
---
 drivers/mmc/core/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 7dc0c85..764af63 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1428,6 +1428,8 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, u32 ocr)
 		pr_warning("%s: cannot verify signal voltage switch\n",
 				mmc_hostname(host));
 
+	mmc_host_clk_hold(host);
+
 	cmd.opcode = SD_SWITCH_VOLTAGE;
 	cmd.arg = 0;
 	cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
@@ -1438,8 +1440,6 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, u32 ocr)
 
 	if (!mmc_host_is_spi(host) && (cmd.resp[0] & R1_ERROR))
 		return -EIO;
-
-	mmc_host_clk_hold(host);
 	/*
 	 * The card should drive cmd and dat[0:3] low immediately
 	 * after the response of cmd11, but wait 1 ms to be sure
-- 
1.9.0

[PATCH 4/4] mmc: core: hold SD Clock before CMD11 during Signal

[Russell King - ARM Linux]

On Sun, Jul 13, 2014 at 01:21:10PM +0800, Vincent Yang wrote:
> Voltage Switch Procedure
> 
> This patch is to fix an issue found on mb86s7x platforms.
> 
> [symptom]
> There are some UHS-1 SD memory cards sometimes cannot be detected correctly,
> e.g., Transcend 600x SDXC 64GB UHS-1 memory card.
> During Signal Voltage Switch Procedure, failure to switch is indicated
> by the card holding DAT[3:0] low.
> 
> [analysis]
> According to SD Host Controller Simplified Specification Version 3.00
> chapter 3.6.1, the Signal Voltage Switch Procedure should be:
> (1) Check S18A; (2) Issue CMD11; (3) Check CMD 11 response;
> (4) Stop providing SD clock; (5) Check DAT[3:0] should be 0000b;
> (6) Set 1.8V Signal Enable; (7) Wait 5ms; (8) Check 1.8V Signal Enable;
> (9) Provide SD Clock; (10) Wait 1ms; (11) Check DAT[3:0] should be 1111b;
> (12) error handling
> 
> With CONFIG_MMC_CLKGATE=y, sometimes there is one more gating/un-gating
> SD clock between (2) and (3). In this case, some UHS-1 SD cards will hold
> DAT[3:0] 0000b at (11) and thus fails Signal Voltage Switch Procedure.
> 
> [solution]
> By mmc_host_clk_hold() before CMD11, the additional gating/un-gating
> SD clock between (2) and (3) can be prevented and thus no failure at (11).
> It has been verified with many UHS-1 SD cards on mb86s7x platforms and
> works correctly.
> 
> Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
> ---
>  drivers/mmc/core/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 7dc0c85..764af63 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1428,6 +1428,8 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, u32 ocr)
>  		pr_warning("%s: cannot verify signal voltage switch\n",
>  				mmc_hostname(host));
>  
> +	mmc_host_clk_hold(host);
> +
>  	cmd.opcode = SD_SWITCH_VOLTAGE;
>  	cmd.arg = 0;
>  	cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
> @@ -1438,8 +1440,6 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, u32 ocr)
>  
>  	if (!mmc_host_is_spi(host) && (cmd.resp[0] & R1_ERROR))
>  		return -EIO;
> -
> -	mmc_host_clk_hold(host);

Your analysis may be correct, but implementation leads something to be
desired - what happens if we exit the function just above this deleted
line - returning -EIO ?  Shouldn't there be some releasing of the
clock hold?

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 4/4] mmc: core: hold SD Clock before CMD11 during Signal

[Vincent.Yang]

>-----Original Message-----
>From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
>On Sun, Jul 13, 2014 at 01:21:10PM +0800, Vincent Yang wrote:
>> Voltage Switch Procedure
>>
>> This patch is to fix an issue found on mb86s7x platforms.
>>
>> [symptom]
>> There are some UHS-1 SD memory cards sometimes cannot be detected correctly,
>> e.g., Transcend 600x SDXC 64GB UHS-1 memory card.
>> During Signal Voltage Switch Procedure, failure to switch is indicated
>> by the card holding DAT[3:0] low.
>>
>> [analysis]
>> According to SD Host Controller Simplified Specification Version 3.00
>> chapter 3.6.1, the Signal Voltage Switch Procedure should be:
>> (1) Check S18A; (2) Issue CMD11; (3) Check CMD 11 response;
>> (4) Stop providing SD clock; (5) Check DAT[3:0] should be 0000b;
>> (6) Set 1.8V Signal Enable; (7) Wait 5ms; (8) Check 1.8V Signal Enable;
>> (9) Provide SD Clock; (10) Wait 1ms; (11) Check DAT[3:0] should be 1111b;
>> (12) error handling
>>
>> With CONFIG_MMC_CLKGATE=y, sometimes there is one more gating/un-gating
>> SD clock between (2) and (3). In this case, some UHS-1 SD cards will hold
>> DAT[3:0] 0000b at (11) and thus fails Signal Voltage Switch Procedure.
>>
>> [solution]
>> By mmc_host_clk_hold() before CMD11, the additional gating/un-gating
>> SD clock between (2) and (3) can be prevented and thus no failure at (11).
>> It has been verified with many UHS-1 SD cards on mb86s7x platforms and
>> works correctly.
>>
>> Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
>> ---
>>  drivers/mmc/core/core.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 7dc0c85..764af63 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1428,6 +1428,8 @@ int mmc_set_signal_voltage(struct mmc_host *host, int
>signal_voltage, u32 ocr)
>>               pr_warning("%s: cannot verify signal voltage switch\n",
>>                               mmc_hostname(host));
>>
>> +     mmc_host_clk_hold(host);
>> +
>>       cmd.opcode = SD_SWITCH_VOLTAGE;
>>       cmd.arg = 0;
>>       cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>> @@ -1438,8 +1440,6 @@ int mmc_set_signal_voltage(struct mmc_host *host, int
>signal_voltage, u32 ocr)
>>
>>       if (!mmc_host_is_spi(host) && (cmd.resp[0] & R1_ERROR))
>>               return -EIO;
>> -
>> -     mmc_host_clk_hold(host);
>
>Your analysis may be correct, but implementation leads something to be
>desired - what happens if we exit the function just above this deleted
>line - returning -EIO ?  Shouldn't there be some releasing of the
>clock hold?

Hi Russell,

Thanks a lot for your kind help and also your review.
I'll fix it in next version.

Best regards,
Vincent Yang

>
>--
>FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
>according to speedtest.net.