mmc: dw_mmc: add dw_mmc-k3

Suggested by Jaehoon: Use slot-gpio to handle cd-gpio Add function dw_mci_of_get_cd_gpio to check "cd-gpios" from dts. mmc_gpio_request_cd and mmc_gpio_get_cd are used to handle cd pin

Signed-off-by Zhangfei Gao <[email protected]>

Acked-by Jaehoon Chung <[email protected]>

---
 drivers/mmc/host/dw_mmc.c |   48 +++++++++++++++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 13 deletions(-)

Suggested by Jaehoon: Use slot-gpio to handle cd-gpio Add function dw_mci_of_get_cd_gpio to check "cd-gpios" from dts. mmc_gpio_request_cd and mmc_gpio_get_cd are used to handle cd pin

Signed-off-by Zhangfei Gao <[email protected]>

Acked-by Jaehoon Chung <[email protected]>

Quoted 4 lines · Zhangfei Gao

Suggested by Jaehoon: Use slot-gpio to handle cd-gpio Add function dw_mci_of_get_cd_gpio to check "cd-gpios" from dts. mmc_gpio_request_cd and mmc_gpio_get_cd are used to handle cd pin

Quoted 2 lines

Signed-off-by Zhangfei Gao <[email protected]>

Acked-by Jaehoon Chung <[email protected]>

The Samsung Arndale board started failing boot from MMC root tests starting with next-20140113 and I bisected it down to this patch. Reverting this patch on top of next-20140114 gets Arndale booting again from MMC. Is there some supporting DT data that's missing for Arndale?

Quoted 5 lines · Kevin Hilman

Quoted 4 lines · Zhangfei Gao

Suggested by Jaehoon: Use slot-gpio to handle cd-gpio Add function dw_mci_of_get_cd_gpio to check "cd-gpios" from dts. mmc_gpio_request_cd and mmc_gpio_get_cd are used to handle cd pin

Quoted 8 lines

Quoted 2 lines

Signed-off-by Zhangfei Gao <[email protected]>

Acked-by Jaehoon Chung <[email protected]>

The Samsung Arndale board started failing boot from MMC root tests starting with next-20140113 and I bisected it down to this patch. Reverting this patch on top of next-20140114 gets Arndale booting again from MMC. Is there some supporting DT data that's missing for Arndale?

Rather, it looks like this patch changes behaviour and no longer uses dw_mci_get_cd() to find out if there's a card attached -- it switches to rely only on GPIO (see the last chunk in the patch). That seems broken?

Quoted 16 lines · Olof Johansson

Quoted 6 lines · Kevin Hilman

Quoted 4 lines · Zhangfei Gao

Suggested by Jaehoon: Use slot-gpio to handle cd-gpio Add function dw_mci_of_get_cd_gpio to check "cd-gpios" from dts. mmc_gpio_request_cd and mmc_gpio_get_cd are used to handle cd pin

Quoted 8 lines

Quoted 2 lines

Signed-off-by Zhangfei Gao <[email protected]>

Acked-by Jaehoon Chung <[email protected]>

The Samsung Arndale board started failing boot from MMC root tests starting with next-20140113 and I bisected it down to this patch. Reverting this patch on top of next-20140114 gets Arndale booting again from MMC. Is there some supporting DT data that's missing for Arndale?

Quoted 5 lines

Rather, it looks like this patch changes behaviour and no longer uses dw_mci_get_cd() to find out if there's a card attached -- it switches to rely only on GPIO (see the last chunk in the patch). That seems broken?

Oops,
Change using dw_mci_get_cd set flag DW_MMC_CARD_PRESENT. And dw_mci_get_cd is called from mmc_rescan, a litter later than dw_mci_probe.
Is that too late?

Introdeced from commit b7db86adfa58b

CDETECT is ingored since IS_ERR_VALUE(!mmc_gpio_get_cd(mmc)) does not work Add spin_lock_bh(&host->lock) for atomic accessing DW_MMC_CARD_PRESENT

Signed-off-by Zhangfei Gao <[email protected]>

Reported-by Kevin Hilman <[email protected]>

---
  drivers/mmc/host/dw_mmc.c |    9 ++++++---
  1 file changed, 6 insertions(+), 3 deletions(-)

Introduced from commit bf626e5550f24aec24975a0e85ad8e572ca76a6b CDETECT is ignored since negated return value of mmc_gpio_get_cd(mmc) can not be checked by IS_ERR_VALUE.
Add spin_lock_bh(&host->lock) for atomic accessing DW_MMC_CARD_PRESENT, otherwise sd detect may occasionally fail.

Signed-off-by Zhangfei Gao <[email protected]>

Reported-by Kevin Hilman <[email protected]>

Reviewed-by Sachin Kamat <[email protected]>

Tested-by Sachin Kamat <[email protected]>

---
 drivers/mmc/host/dw_mmc.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/mmc/host/dw_mmc.c +2 -1

index a776f24f4311..9ded62c8225e 100644
@@ -1033,7 +1033,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 	int present;
 	struct dw_mci_slot *slot = mmc_priv(mmc);
 	struct dw_mci_board *brd = slot->host->pdata;
-	int gpio_cd = !mmc_gpio_get_cd(mmc);
+	struct dw_mci *host = slot->host;
+	int gpio_cd = mmc_gpio_get_cd(mmc);
 
 	/* Use platform get_cd function, else try onboard card detect */
 	if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION)

drivers/mmc/host/dw_mmc.c +3 -2

 	else
 		present = (mci_readl(slot->host, CDETECT) & (1 << slot->id))
 			== 0 ? 1 : 0;
 
+	spin_lock_bh(&host->lock);
 	if (present) {
 		set_bit(DW_MMC_CARD_PRESENT, &slot->flags);
 		dev_dbg(&mmc->class_dev, "card is present\n");

@@ -1053,6 +1055,7 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 		clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);
 		dev_dbg(&mmc->class_dev, "card is not present\n");
 	}
+	spin_unlock_bh(&host->lock);
 
 	return present;
 }

@@ -2081,7 +2084,7 @@ static int dw_mci_of_get_wp_gpio(struct device *dev, u8 slot)
 	return gpio;
 }
 
-/* find the cd gpio for a given slot; or -1 if none specified */
+/* find the cd gpio for a given slot */
 static void dw_mci_of_get_cd_gpio(struct device *dev, u8 slot,
 					struct mmc_host *mmc)
 {
--

1.7.9.5

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

Quoted 26 lines · Zhangfei Gao

Introduced from commit bf626e5550f24aec24975a0e85ad8e572ca76a6b CDETECT is ignored since negated return value of mmc_gpio_get_cd(mmc) can not be checked by IS_ERR_VALUE.
Add spin_lock_bh(&host->lock) for atomic accessing DW_MMC_CARD_PRESENT, otherwise sd detect may occasionally fail.

Signed-off-by Zhangfei Gao <[email protected]>

Reported-by Kevin Hilman <[email protected]>

Reviewed-by Sachin Kamat <[email protected]>

Tested-by Sachin Kamat <[email protected]>

---
  drivers/mmc/host/dw_mmc.c |    9 ++++++---
  1 file changed, 6 insertions(+), 3 deletions(-)

drivers/mmc/host/dw_mmc.c +2 -1

index a776f24f4311..9ded62c8225e 100644
@@ -1033,7 +1033,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
  	int present;
  	struct dw_mci_slot *slot = mmc_priv(mmc);
  	struct dw_mci_board *brd = slot->host->pdata;
-	int gpio_cd = !mmc_gpio_get_cd(mmc);
+	struct dw_mci *host = slot->host;
+	int gpio_cd = mmc_gpio_get_cd(mmc);
 

drivers/mmc/host/dw_mmc.c static int dw_mci_get_cd(struct mmc_host *mmc) · line 1045

Quoted 8 lines

drivers/mmc/host/dw_mmc.c +1 -1

  	/* Use platform get_cd function, else try onboard card detect */
  	if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION)

@@ -1041,11 +1042,12 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
  	else if (brd->get_cd)
  		present = !brd->get_cd(slot->id);
  	else if (!IS_ERR_VALUE(gpio_cd))
-		present = !!gpio_cd;
+		present = !gpio_cd;

!!gpio_cd or gpio_cd is correct, isn't it?

Quoted 27 lines · Seungwon Jeon

Quoted 26 lines · Zhangfei Gao

Introduced from commit bf626e5550f24aec24975a0e85ad8e572ca76a6b CDETECT is ignored since negated return value of mmc_gpio_get_cd(mmc) can not be checked by IS_ERR_VALUE.
Add spin_lock_bh(&host->lock) for atomic accessing DW_MMC_CARD_PRESENT, otherwise sd detect may occasionally fail.

Signed-off-by Zhangfei Gao <[email protected]>

Reported-by Kevin Hilman <[email protected]>

Reviewed-by Sachin Kamat <[email protected]>

Tested-by Sachin Kamat <[email protected]>

---
  drivers/mmc/host/dw_mmc.c |    9 ++++++---
  1 file changed, 6 insertions(+), 3 deletions(-)

drivers/mmc/host/dw_mmc.c +2 -1

index a776f24f4311..9ded62c8225e 100644
@@ -1033,7 +1033,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
  	int present;
  	struct dw_mci_slot *slot = mmc_priv(mmc);
  	struct dw_mci_board *brd = slot->host->pdata;
-	int gpio_cd = !mmc_gpio_get_cd(mmc);
+	struct dw_mci *host = slot->host;
+	int gpio_cd = mmc_gpio_get_cd(mmc);
 

Quoted 10 lines

drivers/mmc/host/dw_mmc.c static int dw_mci_get_cd(struct mmc_host *mmc) · line 1045

Quoted 8 lines

drivers/mmc/host/dw_mmc.c +1 -1

  	/* Use platform get_cd function, else try onboard card detect */
  	if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION)

@@ -1041,11 +1042,12 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
  	else if (brd->get_cd)
  		present = !brd->get_cd(slot->id);
  	else if (!IS_ERR_VALUE(gpio_cd))
-		present = !!gpio_cd;
+		present = !gpio_cd;

!!gpio_cd or gpio_cd is correct, isn't it?

No, mmc_gpio_get_cd(mmc) has to revert.

!!gpio_cd is used before since gpio_cd = !mmc_gpio_get_cd(mmc) is used before.

Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 19 lines

Quoted 18 lines

Quoted 11 lines

(quoted diff) +2 -1

@@ -1033,7 +1033,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
   	int present;
   	struct dw_mci_slot *slot = mmc_priv(mmc);
   	struct dw_mci_board *brd = slot->host->pdata;
-	int gpio_cd = !mmc_gpio_get_cd(mmc);
+	struct dw_mci *host = slot->host;
+	int gpio_cd = mmc_gpio_get_cd(mmc);
 
   	/* Use platform get_cd function, else try onboard card detect */
   	if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION)

Review static int dw_mci_get_cd(struct mmc_host *mmc) · line 1045

Quoted 5 lines

(quoted diff) +1 -1

@@ -1041,11 +1042,12 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
   	else if (brd->get_cd)
   		present = !brd->get_cd(slot->id);
   	else if (!IS_ERR_VALUE(gpio_cd))
-		present = !!gpio_cd;
+		present = !gpio_cd;

!!gpio_cd or gpio_cd is correct, isn't it?

Quoted 1 line

No, mmc_gpio_get_cd(mmc) has to revert.

I'm missing something?
If card is detected, mmc_gpio_get_cd() returns non-zero, right? I guess gpio_cd should be kept.

However, in our board cd =0 when card is deteced while cd=1 when card is removed.
In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be set, as well as new property "caps2-mmc-cd-active-low".

Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt +2 -0

@@ -73,6 +73,8 @@ Optional properties:
+* caps2-mmc-cd-active-low: cd pin is low when card active
+
 

Quoted 11 lines · On Wednesday 15 January 2014 21:56:26 zhangfei wrote:

However, in our board cd =0 when card is deteced while cd=1 when card is removed.
In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be set, as well as new property "caps2-mmc-cd-active-low".

Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt +2 -0

@@ -73,6 +73,8 @@ Optional properties:
+* caps2-mmc-cd-active-low: cd pin is low when card active
+
 

drivers/mmc/host/dw_mmc.c

Quoted 4 lines

drivers/mmc/host/dw_mmc.c +3 -0

+       if (of_find_property(np, "caps2-mmc-cd-active-low", NULL))
+               pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
+

The MMC_CAP2_CD_ACTIVE_HIGH flag should only be required for legacy platforms. With DT parsing, you can normally specify the polarity of the GPIO line in the GPIO specifier in DT.

Quoted 12 lines · On Wednesday 15 January 2014 14:59:59 Arnd Bergmann wrote:

Quoted 11 lines · On Wednesday 15 January 2014 21:56:26 zhangfei wrote:

However, in our board cd =0 when card is deteced while cd=1 when card is removed.
In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be set, as well as new property "caps2-mmc-cd-active-low".

Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt +2 -0

@@ -73,6 +73,8 @@ Optional properties:
+* caps2-mmc-cd-active-low: cd pin is low when card active
+
 

Quoted 8 lines

drivers/mmc/host/dw_mmc.c

Quoted 4 lines

drivers/mmc/host/dw_mmc.c +3 -0

+       if (of_find_property(np, "caps2-mmc-cd-active-low", NULL))
+               pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
+

The MMC_CAP2_CD_ACTIVE_HIGH flag should only be required for legacy platforms. With DT parsing, you can normally specify the polarity of the GPIO line in the GPIO specifier in DT.

I missed the fact that we already have a "cd-inverted" property as specified in bindings/mmc/mmc.txt. If your GPIO controller does not handle polarity, you can use that.

Quoted 11 lines · On Wednesday 15 January 2014 21:56:26 zhangfei wrote:

However, in our board cd =0 when card is deteced while cd=1 when card is removed.
In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be set, as well as new property "caps2-mmc-cd-active-low".

Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt +2 -0

@@ -73,6 +73,8 @@ Optional properties:
+* caps2-mmc-cd-active-low: cd pin is low when card active
+
 

drivers/mmc/host/dw_mmc.c

Quoted 4 lines

drivers/mmc/host/dw_mmc.c +3 -0

+       if (of_find_property(np, "caps2-mmc-cd-active-low", NULL))
+               pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
+

The MMC_CAP2_CD_ACTIVE_HIGH flag should only be required for legacy platforms. With DT parsing, you can normally specify the polarity of the GPIO line in the GPIO specifier in DT.

Quoted 12 lines · Arnd Bergmann

Quoted 11 lines · On Wednesday 15 January 2014 21:56:26 zhangfei wrote:

However, in our board cd =0 when card is deteced while cd=1 when card is removed.
In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be set, as well as new property "caps2-mmc-cd-active-low".

Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt +2 -0

@@ -73,6 +73,8 @@ Optional properties:
+* caps2-mmc-cd-active-low: cd pin is low when card active
+
 

Quoted 8 lines

drivers/mmc/host/dw_mmc.c

Quoted 4 lines

drivers/mmc/host/dw_mmc.c +3 -0

+       if (of_find_property(np, "caps2-mmc-cd-active-low", NULL))
+               pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
+

The MMC_CAP2_CD_ACTIVE_HIGH flag should only be required for legacy platforms. With DT parsing, you can normally specify the polarity of the GPIO line in the GPIO specifier in DT.

Since when?

Quoted 22 lines · On Wednesday 15 January 2014 16:01:46 Russell King - ARM Linux wrote:

Quoted 12 lines · Arnd Bergmann

Quoted 11 lines · On Wednesday 15 January 2014 21:56:26 zhangfei wrote:

However, in our board cd =0 when card is deteced while cd=1 when card is removed.
In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be set, as well as new property "caps2-mmc-cd-active-low".

Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt +2 -0

@@ -73,6 +73,8 @@ Optional properties:
+* caps2-mmc-cd-active-low: cd pin is low when card active
+
 

Quoted 8 lines

drivers/mmc/host/dw_mmc.c

Quoted 4 lines

drivers/mmc/host/dw_mmc.c +3 -0

+       if (of_find_property(np, "caps2-mmc-cd-active-low", NULL))
+               pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
+

The MMC_CAP2_CD_ACTIVE_HIGH flag should only be required for legacy platforms. With DT parsing, you can normally specify the polarity of the GPIO line in the GPIO specifier in DT.

Quoted 1 line

Since when?

I just looked through the bindings and found that about half of them allow passing polarity in the gpio specifier. I thought it was more than that and IIRC the outcome of a previous discussion was that when the gpio controller allows passing polarity, you should use that rather than a separate flag.

Not a problem though, since we can use the cd-inverted and wp-inverted properties.

Well, having this in the gpio level as well as in subsystems like MMC is going to create confusion - and from the results of this patch _IS_ causing confusion. You've just encouraged one implementation to have things setup such that mmc_gpio_get_cd() returns false when a card is inserted, rather than it correctly returning true.

The issue here is that we'll potentially now end up with _three_ inversions. One in the GPIO layers. One in mmc_gpio_get_cd(), and another in every driver which uses the GPIO layer inversion. This is hardly the right approach as it leads to multiple points where confusion about the inverted status can occur.

The issue here is that we'll potentially now end up with three inversions. One in the GPIO layers. One in mmc_gpio_get_cd(), and another in every driver which uses the GPIO layer inversion. This is hardly the right approach as it leads to multiple points where confusion about the inverted status can occur.

Quoted 5 lines · Russell King - ARM Linux

The issue here is that we'll potentially now end up with three inversions. One in the GPIO layers. One in mmc_gpio_get_cd(), and another in every driver which uses the GPIO layer inversion. This is hardly the right approach as it leads to multiple points where confusion about the inverted status can occur.

I did not mean to suggest adding a third inversion. We already have to deal with the one in the GPIO layer and the one in mmc_gpio_get_cd(), which I absolutely agree is unfortunate. As was pointed out already, the gpio driver used in this system does not support the inversion in the DT binding, so the only proper solution is to use the inversion from mmc_gpio_get_cd if MMC_CAP2_CD_ACTIVE_HIGH is set in host->caps2. This flag gets set from mmc_of_parse based on the presence of the "cd-inverted" flag, in the absence of the gpio flags. The dw_mmc driver does not use mmc_of_parse() at the moment, and while it probably should use that in the future, it should definitely read the same DT properties with the same semantics already and not introduce new properties.

Quoted 19 lines · Seungwon Jeon

Quoted 19 lines

Quoted 18 lines

Quoted 11 lines

(quoted diff) +2 -1

@@ -1033,7 +1033,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
   	int present;
   	struct dw_mci_slot *slot = mmc_priv(mmc);
   	struct dw_mci_board *brd = slot->host->pdata;
-	int gpio_cd = !mmc_gpio_get_cd(mmc);
+	struct dw_mci *host = slot->host;
+	int gpio_cd = mmc_gpio_get_cd(mmc);
 
   	/* Use platform get_cd function, else try onboard card detect */
   	if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION)

Review static int dw_mci_get_cd(struct mmc_host *mmc) · line 1045

Quoted 5 lines

(quoted diff) +1 -1

@@ -1041,11 +1042,12 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
   	else if (brd->get_cd)
   		present = !brd->get_cd(slot->id);
   	else if (!IS_ERR_VALUE(gpio_cd))
-		present = !!gpio_cd;
+		present = !gpio_cd;

!!gpio_cd or gpio_cd is correct, isn't it?

Quoted 5 lines

Quoted 1 line

No, mmc_gpio_get_cd(mmc) has to revert.

I'm missing something?
If card is detected, mmc_gpio_get_cd() returns non-zero, right? I guess gpio_cd should be kept.

Hmm, looks you are right.
Though not see clearly mmc_gpio_get_cd declaratoin, other drivers directly set get_cd as mmc_gpio_get_cd.
.get_cd = mmc_gpio_get_cd

However, in our board cd =0 when card is deteced while cd=1 when card is removed.
In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be set, as well as new property "caps2-mmc-cd-active-low".

Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt +2 -0

@@ -73,6 +73,8 @@ Optional properties:
+* caps2-mmc-cd-active-low: cd pin is low when card active
+
 

drivers/mmc/host/dw_mmc.c +3 -0

+       if (of_find_property(np, "caps2-mmc-cd-active-low", NULL))
+               pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
+

But it looks strange "cd-active-low" describing "CD_ACTIVE_HIGH" flag. When card active, cd = 0, and ACTIVE_HIGH is required to make mmc_gpio_get_cd return 1.

int mmc_gpio_get_cd(struct mmc_host *host)

{
        return !gpio_get_value_cansleep(ctx->cd_gpio) ^
                !!(host->caps2 & MMC_CAP2_CD_ACTIVE_HIGH);
}

Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines

Hmm, looks you are right.
Though not see clearly mmc_gpio_get_cd declaratoin, other drivers directly set get_cd as mmc_gpio_get_cd.
.get_cd = mmc_gpio_get_cd

However, in our board cd =0 when card is deteced while cd=1 when card is removed.
In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be set, as well as new property "caps2-mmc-cd-active-low".

Ok, you could do more.
mmc_gpio_get_cd() is expected to return non-zero if card is detection.

I think your board seems not to use pull-up on GPIO line for card detection. So, MMC_CAP2_CD_ACTIVE_HIGH would be needed.

Introduced from commit bf626e5550f24aec24975a0e85ad8e572ca76a6b CDETECT is ignored since negated return value of mmc_gpio_get_cd(mmc) can not be checked by IS_ERR_VALUE.
Add spin_lock_bh(&host->lock) for atomic accessing DW_MMC_CARD_PRESENT, otherwise sd detect may occasionally fail.

Signed-off-by Zhangfei Gao <[email protected]>

Reported-by Kevin Hilman <[email protected]>

Reviewed-by Sachin Kamat <[email protected]>

Tested-by Sachin Kamat <[email protected]>

---
 drivers/mmc/host/dw_mmc.c  |   12 +++++++++---
 include/linux/mmc/dw_mmc.h |    1 +
 2 files changed, 10 insertions(+), 3 deletions(-)

drivers/mmc/host/dw_mmc.c +2 -1

index a776f24f4311..8326e54b96a8 100644
@@ -1033,7 +1033,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 	int present;
 	struct dw_mci_slot *slot = mmc_priv(mmc);
 	struct dw_mci_board *brd = slot->host->pdata;
-	int gpio_cd = !mmc_gpio_get_cd(mmc);
+	struct dw_mci *host = slot->host;
+	int gpio_cd = mmc_gpio_get_cd(mmc);
 
 	/* Use platform get_cd function, else try onboard card detect */
 	if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION)

Quoted 10 lines · Seungwon Jeon

Quoted 9 lines

Hmm, looks you are right.
Though not see clearly mmc_gpio_get_cd declaratoin, other drivers directly set get_cd as mmc_gpio_get_cd.
.get_cd = mmc_gpio_get_cd

However, in our board cd =0 when card is deteced while cd=1 when card is removed.
In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be set, as well as new property "caps2-mmc-cd-active-low".

Quoted 5 lines

Ok, you could do more.
mmc_gpio_get_cd() is expected to return non-zero if card is detection.

I think your board seems not to use pull-up on GPIO line for card detection. So, MMC_CAP2_CD_ACTIVE_HIGH would be needed.

Unfortunately, the specific cd pin can not be configured. Suspect on our board the cd =1 when card is inserted, can not measure as no clear resister and need check with hardware guy tomorrow.

"cd-inverted" may be required as mentioned by Arnd. Will update the patch as following if no problem.

Add dw_mmc-k3.c for k3v2, support sd/emmc

Signed-off-by Zhangfei Gao <[email protected]>

Signed-off-by Zhigang Wang <[email protected]>

Add dw_mmc-k3.c for k3v2, support sd/emmc

Signed-off-by Zhangfei Gao <[email protected]>

Signed-off-by Zhigang Wang <[email protected]>

---
 .../devicetree/bindings/mmc/k3-dw-mshc.txt         |   60 +++++++++
 drivers/mmc/host/Kconfig                           |   10 ++
 drivers/mmc/host/Makefile                          |    1 +
 drivers/mmc/host/dw_mmc-k3.c                       |  132 ++++++++++++++++++++
 4 files changed, 203 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt
 create mode 100644 drivers/mmc/host/dw_mmc-k3.c

Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt new file +60 -0

new file mode 100644
index 000000000000..d7e2d7f159bb
@@ -0,0 +1,60 @@
+* Hisilicon specific extensions to the Synopsys Designware Mobile
+  Storage Host Controller
+
+Read synopsys-dw-mshc.txt for more details
+
+The Synopsys designware mobile storage host controller is used to interface
+a SoC with storage medium such as eMMC or SD/MMC cards. This file documents
+differences between the core Synopsys dw mshc controller properties described
+by synopsys-dw-mshc.txt and the properties used by the Hisilicon specific
+extensions to the Synopsys Designware Mobile Storage Host Controller.
+
+Required Properties:
+
+* compatible: should be one of the following.
+  - "hisilicon,hi4511-dw-mshc": for controllers with hi4511 specific extentions.
+
+* clock-freq-table: should be the frequency (in Hz) array of the ciu clock
+	in each	supported mode.
+	0. CIU clock rate in Hz for DS mode
+	1. CIU clock rate in Hz for MMC HS mode
+	2. CIU clock rate in Hz for SD HS mode
+	3. CIU clock rate in Hz for SDR12 mode
+	4. CIU clock rate in Hz for SDR25 mode
+	5. CIU clock rate in Hz for SDR50 mode
+	6. CIU clock rate in Hz for SDR104 mode
+	7. CIU clock rate in Hz for DDR50 mode
+	8. CIU clock rate in Hz for HS200 mode
+
+Example:
+
+	/* for Hi3620 */
+
+	/* SoC portion */
+	dwmmc_0: dwmmc0 at fcd03000 {
+		compatible = "hisilicon,hi4511-dw-mshc";
+		reg = <0xfcd03000 0x1000>;
+		interrupts = <0 16 4>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clocks = <&mmc_clock HI3620_SD_CIUCLK>, <&clock HI3620_DDRC_PER_CLK>;
+		clock-names = "ciu", "biu";
+		clock-freq-table =
+		<25000000 0 50000000 25000000 50000000 100000000 0 50000000>;
+	};
+
+	/* Board portion */
+	dwmmc0 at fcd03000 {
+		num-slots = <1>;
+		vmmc-supply = <&ldo12>;
+		fifo-depth = <0x100>;
+		supports-highspeed;
+		pinctrl-names = "default";
+		pinctrl-0 = <&sd_pmx_pins &sd_cfg_func1 &sd_cfg_func2>;
+		slot at 0 {
+			reg = <0>;
+			bus-width = <4>;
+			disable-wp;
+			cd-gpios = <&gpio10 3 0>;
+		};
+	};

drivers/mmc/host/Kconfig +10 -0

index 7fc5099e44b2..45aaa2de0f58 100644
@@ -575,6 +575,16 @@ config MMC_DW_SOCFPGA
 	  This selects support for Altera SoCFPGA specific extensions to the
 	  Synopsys DesignWare Memory Card Interface driver.
 
+config MMC_DW_K3
+	tristate "K3 specific extensions for Synopsys DW Memory Card Interface"
+	depends on MMC_DW
+	select MMC_DW_PLTFM
+	select MMC_DW_IDMAC
+	help
+	  This selects support for Hisilicon K3 SoC specific extensions to the
+	  Synopsys DesignWare Memory Card Interface driver. Select this option
+	  for platforms based on Hisilicon K3 SoC's.
+
 config MMC_DW_PCI
 	tristate "Synopsys Designware MCI support on PCI bus"
 	depends on MMC_DW && PCI

drivers/mmc/host/Makefile +1 -0

index c41d0c364509..64f5f8d35839 100644
@@ -43,6 +43,7 @@ obj-$(CONFIG_MMC_DW)		+= dw_mmc.o
 obj-$(CONFIG_MMC_DW_PLTFM)	+= dw_mmc-pltfm.o
 obj-$(CONFIG_MMC_DW_EXYNOS)	+= dw_mmc-exynos.o
 obj-$(CONFIG_MMC_DW_SOCFPGA)	+= dw_mmc-socfpga.o
+obj-$(CONFIG_MMC_DW_K3)		+= dw_mmc-k3.o
 obj-$(CONFIG_MMC_DW_PCI)	+= dw_mmc-pci.o
 obj-$(CONFIG_MMC_SH_MMCIF)	+= sh_mmcif.o
 obj-$(CONFIG_MMC_JZ4740)	+= jz4740_mmc.o

drivers/mmc/host/dw_mmc-k3.c new file +19 -0

new file mode 100644
index 000000000000..68e5e428e8f6
@@ -0,0 +1,132 @@
+/*
+ * Copyright (c) 2013 Linaro Ltd.
+ * Copyright (c) 2013 Hisilicon Limited.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/mmc/host.h>
+#include <linux/mmc/dw_mmc.h>
+#include <linux/of_address.h>
+
+#include "dw_mmc.h"
+#include "dw_mmc-pltfm.h"

drivers/mmc/host/dw_mmc-k3.c new file +103 -1

+	int ret;
+
+	if (!rate) {
+		dev_warn(host->dev,
+			"no specified rate in timing %u\n", ios->timing);
+		return;
+	}
+
+	ret = clk_set_rate(host->ciu_clk, rate);
+	if (ret)
+		dev_warn(host->dev, "failed to set clock rate %uHz\n", rate);
+
+	host->bus_hz = clk_get_rate(host->ciu_clk);
+}
+
+static int dw_mci_k3_parse_dt(struct dw_mci *host)
+{
+	struct dw_mci_k3_priv_data *priv;
+	struct device_node *node = host->dev->of_node;
+	struct property *prop;
+	const __be32 *cur;
+	u32 val, num = 0;
+
+	priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		dev_err(host->dev, "mem alloc failed for private data\n");
+		return -ENOMEM;
+	}
+	host->priv = priv;
+
+	of_property_for_each_u32(node, "clock-freq-table", prop, cur, val) {
+		if (num >= MAX_NUMS)
+			break;
+		priv->clk_table[num++] = val;
+	}
+	return 0;
+}
+
+static const struct dw_mci_drv_data k3_drv_data = {
+	.set_ios		= dw_mci_k3_set_ios,
+	.parse_dt		= dw_mci_k3_parse_dt,
+};
+
+static const struct of_device_id dw_mci_k3_match[] = {
+	{ .compatible = "hisilicon,hi4511-dw-mshc", .data = &k3_drv_data, },
+	{},
+};
+MODULE_DEVICE_TABLE(of, dw_mci_k3_match);
+
+static int dw_mci_k3_probe(struct platform_device *pdev)
+{
+	const struct dw_mci_drv_data *drv_data;
+	const struct of_device_id *match;
+
+	match = of_match_node(dw_mci_k3_match, pdev->dev.of_node);
+	drv_data = match->data;
+
+	return dw_mci_pltfm_register(pdev, drv_data);
+}
+
+static int dw_mci_k3_suspend(struct device *dev)
+{
+	struct dw_mci *host = dev_get_drvdata(dev);
+	int ret;
+
+	ret = dw_mci_suspend(host);
+	if (!ret)
+		clk_disable_unprepare(host->ciu_clk);
+
+	return ret;
+}
+
+static int dw_mci_k3_resume(struct device *dev)
+{
+	struct dw_mci *host = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(host->ciu_clk);
+	if (ret) {
+		dev_err(host->dev, "failed to enable ciu clock\n");
+		return ret;
+	}
+
+	return dw_mci_resume(host);
+}
+
+SIMPLE_DEV_PM_OPS(dw_mci_k3_pmops, dw_mci_k3_suspend, dw_mci_k3_resume);
+
+static struct platform_driver dw_mci_k3_pltfm_driver = {
+	.probe		= dw_mci_k3_probe,
+	.remove		= dw_mci_pltfm_remove,
+	.driver		= {
+		.name		= "dwmmc_k3",
+		.of_match_table	= dw_mci_k3_match,
+		.pm		= &dw_mci_k3_pmops,
+	},
+};
+
+module_platform_driver(dw_mci_k3_pltfm_driver);
+
+MODULE_DESCRIPTION("K3 Specific DW-MSHC Driver Extension");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:dwmmc-k3");
--

1.7.9.5

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

drivers/mmc/host/dw_mmc-k3.c line 29

Quoted 4 lines

drivers/mmc/host/dw_mmc-k3.c new file +4 -0

+static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios *ios)
+{
+	struct dw_mci_k3_priv_data *priv = host->priv;
+	u32 rate = priv->clk_table[ios->timing];

First, sorry for quick review even though your effort.
But I still worry about this change.
Currently k3 host's clock table depends on value number of SD/MMC mode value.
It seems close to identifier for eachg mode. I think it's not good way to use as table's index. Can you modify to mmc_clk_determine_rate() in your another patch-set?
I guess required actual target rate could be determined depending on ios->clock.

Quoted 12 lines · Seungwon Jeon

drivers/mmc/host/dw_mmc-k3.c line 29

Quoted 4 lines

drivers/mmc/host/dw_mmc-k3.c new file +4 -0

+static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios *ios)
+{
+	struct dw_mci_k3_priv_data *priv = host->priv;
+	u32 rate = priv->clk_table[ios->timing];

First, sorry for quick review even though your effort.
But I still worry about this change.
Currently k3 host's clock table depends on value number of SD/MMC mode value.
It seems close to identifier for eachg mode. I think it's not good way to use as table's index. Can you modify to mmc_clk_determine_rate() in your another patch-set?
I guess required actual target rate could be determined depending on ios->clock.

No, it can not get input clock source rate simply from ios->clock, also requied info like which controller, which mode etc.
Here is setting clock source rate, not the working clock rate.

For example, emmc init clock source rate is 13M, while sd init clock source is 25M, it can not simply get such info from ios->clock. And for HS200, emmc may have to set clock source rate to 104M since the controller limitation and can not work stable as 208M.

Quoted 14 lines · Zhangfei Gao

Quoted 12 lines · Seungwon Jeon

drivers/mmc/host/dw_mmc-k3.c line 29

Quoted 4 lines

drivers/mmc/host/dw_mmc-k3.c new file +4 -0

+static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios *ios)
+{
+	struct dw_mci_k3_priv_data *priv = host->priv;
+	u32 rate = priv->clk_table[ios->timing];

First, sorry for quick review even though your effort.
But I still worry about this change.
Currently k3 host's clock table depends on value number of SD/MMC mode value.
It seems close to identifier for eachg mode. I think it's not good way to use as table's index. Can you modify to mmc_clk_determine_rate() in your another patch-set?
I guess required actual target rate could be determined depending on ios->clock.

Quoted 8 lines

No, it can not get input clock source rate simply from ios->clock, also requied info like which controller, which mode etc.
Here is setting clock source rate, not the working clock rate.

For example, emmc init clock source rate is 13M, while sd init clock source is 25M, it can not simply get such info from ios->clock. And for HS200, emmc may have to set clock source rate to 104M since the controller limitation and can not work stable as 208M.

Yes, clock table is source clock rate, not actual working clock rate. I was not saying that 'ios->clock' would be used for source clock directly. I meant that you can adjust the source clock rate depending on 'ios->clock'. You've already done with clock table similarly in mmc_clk_determine_rate().

For example, in case of HS200, 'ios->clock' will be passed with 200MHz.
And then, mmc_clk_determine_rate() can set the 'rate & best' to 100000000 and 720000000 respectively. Also, 'ios->clock' with 400KHz or less may be passed for init clock.
If clock id is HI3620_SD_CIUCLK, 'rate & best' can be selected with 13000000 and 26000000. If not and it's case of MMC, 'rate & best' can be selected with 25000000 and 180000000. If it should be considered with other conditions, please let me know.

Quoted 29 lines · Seungwon Jeon

Quoted 14 lines · Zhangfei Gao

Quoted 12 lines · Seungwon Jeon

drivers/mmc/host/dw_mmc-k3.c line 29

Quoted 4 lines

drivers/mmc/host/dw_mmc-k3.c new file +4 -0

+static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios *ios)
+{
+	struct dw_mci_k3_priv_data *priv = host->priv;
+	u32 rate = priv->clk_table[ios->timing];

First, sorry for quick review even though your effort.
But I still worry about this change.
Currently k3 host's clock table depends on value number of SD/MMC mode value.
It seems close to identifier for eachg mode. I think it's not good way to use as table's index. Can you modify to mmc_clk_determine_rate() in your another patch-set?
I guess required actual target rate could be determined depending on ios->clock.

Quoted 8 lines

No, it can not get input clock source rate simply from ios->clock, also requied info like which controller, which mode etc.
Here is setting clock source rate, not the working clock rate.

For example, emmc init clock source rate is 13M, while sd init clock source is 25M, it can not simply get such info from ios->clock. And for HS200, emmc may have to set clock source rate to 104M since the controller limitation and can not work stable as 208M.

Yes, clock table is source clock rate, not actual working clock rate. I was not saying that 'ios->clock' would be used for source clock directly. I meant that you can adjust the source clock rate depending on 'ios->clock'. You've already done with clock table similarly in mmc_clk_determine_rate().

Quoted 6 lines

For example, in case of HS200, 'ios->clock' will be passed with 200MHz.
And then, mmc_clk_determine_rate() can set the 'rate & best' to 100000000 and 720000000 respectively. Also, 'ios->clock' with 400KHz or less may be passed for init clock.
If clock id is HI3620_SD_CIUCLK, 'rate & best' can be selected with 13000000 and 26000000. If not and it's case of MMC, 'rate & best' can be selected with 25000000 and 180000000. If it should be considered with other conditions, please let me know.

However, I am afraid this will make things complicated rather than simplified.

The function mmc_clk_determine_rate() will need the info which controller it is, what's the init clock rate, what's the max clock rate, and what's the limitation, which may be different as different soc, and can not be hardcoded.
The limitation may in HS200 and SDR104 mode.

The plan is only input init rate and max rate instead of the table, while others directly use ios->clock, only if the the limitation resolved.

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/mmc/host/dw_mmc-k3.c line 29

Quoted 4 lines

drivers/mmc/host/dw_mmc-k3.c new file +4 -0

+static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios *ios)
+{
+	struct dw_mci_k3_priv_data *priv = host->priv;
+	u32 rate = priv->clk_table[ios->timing];

Quoted 8 lines

The function mmc_clk_determine_rate() will need the info which controller it is, what's the init clock rate, what's the max clock rate, and what's the limitation, which may be different as different soc, and can not be hardcoded.
The limitation may in HS200 and SDR104 mode.

The plan is only input init rate and max rate instead of the table, while others directly use ios->clock, only if the the limitation resolved.

Handling mmc clock for hi3620 is in drivers/clk/hisilicon/clk-hi3620.c, right?
Currently only hi3620 has been introduced.
If hi3620's host has a limitation, clk-hi3620.c can handle clock range(init, max) for source clock enough. I feel like clock table is redundant.
Ok. It's just my suggestion. But I still point dependency of mode index number.
K3's clock table refers and depends mode definition value from include/linux/mmc/host.h If new mode is added or modified, it should be considered and also may make complicated.

Dear Seungwon

Quoted 13 lines · Seungwon Jeon

Quoted 13 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios *ios)
+{
+	struct dw_mci_k3_priv_data *priv = host->priv;
+	u32 rate = priv->clk_table[ios->timing];

The function mmc_clk_determine_rate() will need the info which controller it is, what's the init clock rate, what's the max clock rate, and what's the limitation, which may be different as different soc, and can not be hardcoded.
The limitation may in HS200 and SDR104 mode.

Quoted 6 lines

Quoted 2 lines

The plan is only input init rate and max rate instead of the table, while others directly use ios->clock, only if the the limitation resolved.

Handling mmc clock for hi3620 is in drivers/clk/hisilicon/clk-hi3620.c, right? Currently only hi3620 has been introduced.
If hi3620's host has a limitation, clk-hi3620.c can handle clock range(init, max) for source clock

enough.

Quoted 5 lines

I feel like clock table is redundant.
Ok. It's just my suggestion. But I still point dependency of mode index number. K3's clock table refers and depends mode definition value from include/linux/mmc/host.h If new mode is added or modified, it should be considered and also may make complicated.

Looks like you dislike the clk_table very much :)

Double checked with the silicon guy, currently it is said the limitation in HS200 and SDR104 can be replaced by the max_rate, so clk_table can be replaced with ios->clock directly, though they have concern it is not so convinent when removing clk_table in case limitation rate is not equal to max_rate again in future.

Will update accordingly, thanks for the suggestion.

Quoted 1 line

Looks like you dislike the clk_table very much :)

Oh, I just want to remove dependency as I mentioned.

Quoted 6 lines

Double checked with the silicon guy, currently it is said the limitation in HS200 and SDR104 can be replaced by the max_rate, so clk_table can be replaced with ios->clock directly, though they have concern it is not so convinent when removing clk_table in case limitation rate is not equal to max_rate again in future.

I guess specific clock handling(clk-hi3620.c) can adjust required source clock rate.

Suggest by Arnd: abstract mmc tuning as clock behavior, also because different soc have different tuning method and registers. hi3620_mmc_clks is added to handle mmc clock specifically on hi3620.

Signed-off-by Zhangfei Gao <[email protected]>

v7:
0002:
use undefined local value in suspend/resume

v6:
0002:
Jaehoon pointed HIGHSPEED cap can be omitted if supports-highspeed is defined. Seungwon mentioned clk operation should be called after suspend. Remove k3_dwmmc_caps

V5:
0002:
Follow advice from Arnd,
Update dt descirption and use of_property_for_each_u32 to get table number.

v4:
Follow Arnd's suggestion abstracting specific tuning to clock,
also because new version ip use different method and not use same tuning registers.

0001 acked by Jaehoon

v3:
0001:
Put set/clear_bit DW_MMC_CARD_PRESENT in dw_mci_get_cd, Since dw_mci_request will check DW_MMC_CARD_PRESENT before sending cmd

0002:
Follow suggestion from Chris, Kumar and Seungwon
Sync to latest mmc-next, which is 3.12-rc2
Remove enum dw_mci_k3_type etc

v2:
Follow Jaehoon's suggestion
Use slot-gpio.c handle cd pin
Move table out to dts
other suggestion

Zhangfei Gao (3):
mmc: dw_mmc: use slot-gpio to handle cd pin
mmc: dw_mmc: add dw_mmc-k3 for k3 platform
clk: hisilicon: add hi3620_mmc_clks

 .../bindings/arm/hisilicon/hisilicon.txt           |   14 ++
 .../devicetree/bindings/clock/hi3620-clock.txt     |    1 +
 .../devicetree/bindings/mmc/k3-dw-mshc.txt         |   60 +++++
 drivers/clk/hisilicon/clk-hi3620.c                 |  262 ++++++++++++++++++++
 drivers/mmc/host/Kconfig                           |   10 +
 drivers/mmc/host/Makefile                          |    1 +
 drivers/mmc/host/dw_mmc-k3.c                       |  126 ++++++++++
 drivers/mmc/host/dw_mmc.c                          |   48 +++-
 include/dt-bindings/clock/hi3620-clock.h           |    5 +
 9 files changed, 514 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt
 create mode 100644 drivers/mmc/host/dw_mmc-k3.c

Hi,

I've pushed patches 1 and 2 to mmc-next for 3.14, with Arnd and Jaehoon's ACK -- since patch 3 depends on files that aren't in my tree, please can you push that one through arm-soc?