[PATCH-v2 0/7] mmc: sdhci-pxav3: Enable support for PXA1928 SDCHI controller

[PATCH-v2 0/7] mmc: sdhci-pxav3: Enable support for PXA1928 SDCHI controller

[Vaibhav Hiremath]

PXA1928 SDHCI controller has few differences, for example,

                         PXAxxx                    PXA1928
                         ======                    =======
 SDCLK_DELAY field       0x10A                     0x114
 SDCLK_DELAY mask        0x1F                      0x3FF
 SDCLK_DELAY shift        9                          8
 SDCLK_SEL shift          8                          2 (SEL1)

So this patch series introduces new compatible device_id
(marvell,pxav3-1928-sdhci), and makes use of .data for handling
such differences.

The series also adds support like,

 - independent ->set_clock() api, as we need to enable internal clock gate
   and TX clock
 - pinctrl configuration based on bus speed.
 - introduce new quirk SDHCI_QUIRK2_MUST_SET_SDHCI_BUS_POWER
   SD_BUS_POWER & SD_BUS_VLT bit-fields are used internally to gate the
   clocks, so it is important to configure them as part of ->set_power()
   More detailed description is written into commit log.
 - Enable SDHCI_QUIRK_BROKEN_TIMEOUT_VAL for PXA1928 device_id
 - Enable SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON quirk for pxa1928 device


V1 => V2:
=========
 - Fixed typo issue (residual change remained by mistake)
 - Added new patch updating DT binding document for pxa1928 support
 - Added new patch enabling CARD_ON_NEEDS_BUS_ON for pxa1928 device

Testing:
I have done basic testing on both eMMC and SD card on PXA1928 based
platform.

Note: I tried to made sure that I do not break any other platform, which
used sdhci, except HS200 configuration.·
Unfortunately I do not have access to any other datasheets, where I can
cross check the details on HS200 bit-fields. Probably someone who has
access can confirm [PATCH 4/5], whether it impacts other platforms.

Kevin Liu (1):
  mmc: sdhci-pxav3: Fix HS200 mode support

Vaibhav Hiremath (6):
  mmc: sdhci-pxav3: Enable pxa1928 device support
  mmc: sdhci-pxav3: binding: Add pxa1928 compatible support
  mmc: sdhci-pxav3: Add platform specific set_clock ops
  mmc: sdhci-pxav3: Add pinctl setting according to bus clock
  mmc: sdhci: add new quirk for setting BUS_POWER & BUS_VLT fields
  mmc: sdhci: enable SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON for pxa1928

 .../devicetree/bindings/mmc/sdhci-pxa.txt          |   2 +-
 drivers/mmc/host/sdhci-pltfm.c                     |   4 +
 drivers/mmc/host/sdhci-pxav3.c                     | 168 +++++++++++++++++++--
 drivers/mmc/host/sdhci.c                           |   3 +-
 drivers/mmc/host/sdhci.h                           |   2 +
 5 files changed, 165 insertions(+), 14 deletions(-)

-- 
1.9.1

[PATCH-v2 1/7] mmc: sdhci-pxav3: Enable pxa1928 device support

[Vaibhav Hiremath]

SDHCI controller present in PXA1928 has few differences as far as
register map is concerned.
For example,

                         PXAxxx                    PXA1928
                         ======                    =======
 SDCLK_DELAY field       0x10A                     0x114
 SDCLK_DELAY mask        0x1F                      0x3FF
 SDCLK_DELAY shift        9                          8
 SDCLK_SEL shift          8                          2 (SEL1)

So in order to support multi-platform, use sdhci_pxa_regdata structure
as a variant data according to platform.

Note that, there are some more differences, which would be added
as and when respective feature gets added to the driver.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/mmc/host/sdhci-pxav3.c | 62 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 51 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index 6d4bad4..aecae04 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -42,9 +42,6 @@
 #define PXAV3_RPM_DELAY_MS		50
 
 #define SD_CLOCK_BURST_SIZE_SETUP	0x10A
-#define SDCLK_SEL			0x100
-#define  SDCLK_DELAY_SHIFT		9
-#define  SDCLK_DELAY_MASK		0x1f
 
 #define SD_CFG_FIFO_PARAM		0x100
 #define  SDCFG_GEN_PAD_CLK_ON		BIT(6)
@@ -58,11 +55,25 @@
 #define  SDCE_MISC_INT			BIT(2)
 #define  SDCE_MISC_INT_EN		BIT(1)
 
+#define SD_RX_CFG_REG			0x114
+
 /* IO Power control */
 #define IO_PWR_AKEY_ASFAR		0xbaba
 #define IO_PWR_AKEY_ASSAR		0xeb10
 #define IO_PWR_MMC1_PAD_1V8		BIT(2)
 
+struct sdhci_pxa_data {
+	u32 sdclk_delay_reg;
+	u32 sdclk_delay_mask;
+	u8 sdclk_delay_shift;
+	u8 sdclk_sel_mask;
+	u8 sdclk_sel_shift;
+	/*
+	 * We have few more differences, add them along with their
+	 * respective feature support
+	 */
+};
+
 struct sdhci_pxa {
 	struct clk *clk_core;
 	struct clk *clk_io;
@@ -70,6 +81,24 @@ struct sdhci_pxa {
 	void __iomem *sdio3_conf_reg;
 	void __iomem *io_pwr_reg;
 	void __iomem *io_pwr_lock_reg;
+	struct sdhci_pxa_data *data;
+};
+
+static struct sdhci_pxa_data pxav3_data_v1 = {
+	.sdclk_delay_reg	= SD_CLOCK_BURST_SIZE_SETUP,
+	.sdclk_delay_mask	= 0x1F,
+	.sdclk_delay_shift	= 9,
+	.sdclk_sel_mask		= 0x1,
+	.sdclk_sel_shift	= 8,
+};
+
+static struct sdhci_pxa_data pxav3_data_v2 = {
+	.sdclk_delay_reg	= SD_RX_CFG_REG,
+	.sdclk_delay_mask	= 0x3FF,
+	.sdclk_delay_shift	= 8,
+	/* Only set SDCLK_SEL1, as driver uses default value of SDCLK_SEL0 */
+	.sdclk_sel_mask		= 0x3,
+	.sdclk_sel_shift	= 2,	/* SDCLK_SEL1 */
 };
 
 /*
@@ -183,6 +212,8 @@ static void pxav3_reset(struct sdhci_host *host, u8 mask)
 {
 	struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc));
 	struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data;
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_pxa *pxa = pltfm_host->priv;
 
 	sdhci_reset(host, mask);
 
@@ -193,12 +224,14 @@ static void pxav3_reset(struct sdhci_host *host, u8 mask)
 		 */
 		if (pdata && 0 != pdata->clk_delay_cycles) {
 			u16 tmp;
-
-			tmp = readw(host->ioaddr + SD_CLOCK_BURST_SIZE_SETUP);
-			tmp |= (pdata->clk_delay_cycles & SDCLK_DELAY_MASK)
-				<< SDCLK_DELAY_SHIFT;
-			tmp |= SDCLK_SEL;
-			writew(tmp, host->ioaddr + SD_CLOCK_BURST_SIZE_SETUP);
+			struct sdhci_pxa_data *data = pxa->data;
+
+			tmp = readw(host->ioaddr + data->sdclk_delay_reg);
+			tmp |= (pdata->clk_delay_cycles & data->sdclk_delay_mask)
+				<< data->sdclk_delay_shift;
+			tmp &= ~(data->sdclk_sel_mask << data->sdclk_sel_shift);
+			tmp |= 1 << data->sdclk_sel_shift;
+			writew(tmp, host->ioaddr + data->sdclk_delay_reg);
 		}
 	}
 }
@@ -363,10 +396,16 @@ static struct sdhci_pltfm_data sdhci_pxav3_pdata = {
 #ifdef CONFIG_OF
 static const struct of_device_id sdhci_pxav3_of_match[] = {
 	{
-		.compatible = "mrvl,pxav3-mmc",
+		.compatible	= "mrvl,pxav3-mmc",
+		.data		= (void *)&pxav3_data_v1,
+	},
+	{
+		.compatible	= "marvell,armada-380-sdhci",
+		.data		= (void *)&pxav3_data_v1,
 	},
 	{
-		.compatible = "marvell,armada-380-sdhci",
+		.compatible	= "marvell,pxav3-1928-sdhci",
+		.data		= (void *)&pxav3_data_v2,
 	},
 	{},
 };
@@ -470,6 +509,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
 			goto err_of_parse;
 		sdhci_get_of_property(pdev);
 		pdata = pxav3_get_mmc_pdata(dev);
+		pxa->data = (struct sdhci_pxa_data *)match->data;
 		pdev->dev.platform_data = pdata;
 	} else if (pdata) {
 		/* on-chip device */
-- 
1.9.1

[PATCH-v2 2/7] mmc: sdhci-pxav3: binding: Add pxa1928 compatible support

[Vaibhav Hiremath]

With support for pxa1928 family of devices , this patch
updates the binding document with compatible property
of "marvell,pxav3-1928-sdhci".

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 Documentation/devicetree/bindings/mmc/sdhci-pxa.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt b/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt
index 3d1b449..29edcf5 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt
@@ -5,7 +5,7 @@ and the properties used by the sdhci-pxav2 and sdhci-pxav3 drivers.
 
 Required properties:
 - compatible: Should be "mrvl,pxav2-mmc", "mrvl,pxav3-mmc" or
-  "marvell,armada-380-sdhci".
+  "marvell,armada-380-sdhci" or "marvell,pxav3-1928-sdhci".
 - reg:
   * for "mrvl,pxav2-mmc" and "mrvl,pxav3-mmc", one register area for
     the SDHCI registers.
-- 
1.9.1

Re: [PATCH-v2 2/7] mmc: sdhci-pxav3: binding: Add pxa1928 compatible support

[Rob Herring]

On 09/07/2015 06:18 AM, Vaibhav Hiremath wrote:
> With support for pxa1928 family of devices , this patch
> updates the binding document with compatible property
> of "marvell,pxav3-1928-sdhci".
> 
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/mmc/sdhci-pxa.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt b/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt
> index 3d1b449..29edcf5 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt
> @@ -5,7 +5,7 @@ and the properties used by the sdhci-pxav2 and sdhci-pxav3 drivers.
>  
>  Required properties:
>  - compatible: Should be "mrvl,pxav2-mmc", "mrvl,pxav3-mmc" or
> -  "marvell,armada-380-sdhci".
> +  "marvell,armada-380-sdhci" or "marvell,pxav3-1928-sdhci".

v3 is implied by pxa1928. So I'd just do "marvell,pxa1928-sdhci" to
better match the chip name.

Rob

>  - reg:
>    * for "mrvl,pxav2-mmc" and "mrvl,pxav3-mmc", one register area for
>      the SDHCI registers.
> 

--
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 2/7] mmc: sdhci-pxav3: binding: Add pxa1928 compatible support

[Vaibhav Hiremath]

On Wednesday 09 September 2015 05:19 AM, Rob Herring wrote:
> On 09/07/2015 06:18 AM, Vaibhav Hiremath wrote:
>> With support for pxa1928 family of devices , this patch
>> updates the binding document with compatible property
>> of "marvell,pxav3-1928-sdhci".
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>   Documentation/devicetree/bindings/mmc/sdhci-pxa.txt | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt b/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt
>> index 3d1b449..29edcf5 100644
>> --- a/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt
>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt
>> @@ -5,7 +5,7 @@ and the properties used by the sdhci-pxav2 and sdhci-pxav3 drivers.
>>
>>   Required properties:
>>   - compatible: Should be "mrvl,pxav2-mmc", "mrvl,pxav3-mmc" or
>> -  "marvell,armada-380-sdhci".
>> +  "marvell,armada-380-sdhci" or "marvell,pxav3-1928-sdhci".
>
> v3 is implied by pxa1928. So I'd just do "marvell,pxa1928-sdhci" to
> better match the chip name.
>

Ok, No issues.

I followed the existing "armada-380-sdhci" naming style.
Will correct it in next version.


Thanks,
Vaibhav
--
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 3/7] mmc: sdhci-pxav3: Add platform specific set_clock ops

[Vaibhav Hiremath]

In case of PXA1928 & family of devices, the TX BUS and internal clock
need to be set as part of ->set_clock() ops, so this patch adds
platform specific ->set_clock() operation.

Note that, in order to not break other platforms, this patch
introduced the flag, which controls whether controller/platform
specific clock configuration needs to be executed.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/mmc/host/sdhci-pxav3.c | 46 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index aecae04..c2b2b78 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -48,6 +48,10 @@
 #define  SDCFG_GEN_PAD_CLK_CNT_MASK	0xFF
 #define  SDCFG_GEN_PAD_CLK_CNT_SHIFT	24
 
+#define SD_FIFO_PARAM			0x104
+#define  INTERNAL_CLK_GATE_CTRL		BIT(8)
+#define  INTERNAL_CLK_GATE_ON		BIT(9)
+
 #define SD_SPI_MODE			0x108
 #define SD_CE_ATA_1			0x10C
 
@@ -57,6 +61,9 @@
 
 #define SD_RX_CFG_REG			0x114
 
+#define TX_CFG_REG			0x118
+#define  TX_INTERNAL_SEL_BUS_CLK	BIT(30)
+
 /* IO Power control */
 #define IO_PWR_AKEY_ASFAR		0xbaba
 #define IO_PWR_AKEY_ASSAR		0xeb10
@@ -68,6 +75,9 @@ struct sdhci_pxa_data {
 	u8 sdclk_delay_shift;
 	u8 sdclk_sel_mask;
 	u8 sdclk_sel_shift;
+
+	/* set this if platform needs separate clock configuration */
+	bool set_pltfrm_clk;
 	/*
 	 * We have few more differences, add them along with their
 	 * respective feature support
@@ -90,6 +100,7 @@ static struct sdhci_pxa_data pxav3_data_v1 = {
 	.sdclk_delay_shift	= 9,
 	.sdclk_sel_mask		= 0x1,
 	.sdclk_sel_shift	= 8,
+	.set_pltfrm_clk		= false,
 };
 
 static struct sdhci_pxa_data pxav3_data_v2 = {
@@ -99,6 +110,7 @@ static struct sdhci_pxa_data pxav3_data_v2 = {
 	/* Only set SDCLK_SEL1, as driver uses default value of SDCLK_SEL0 */
 	.sdclk_sel_mask		= 0x3,
 	.sdclk_sel_shift	= 2,	/* SDCLK_SEL1 */
+	.set_pltfrm_clk		= true,
 };
 
 /*
@@ -375,8 +387,40 @@ static void pxav3_voltage_switch(struct sdhci_host *host,
 	writel(val, pxa->io_pwr_reg);
 }
 
+static void pxav3_set_tx_clock(struct sdhci_host *host)
+{
+	u32 val;
+
+	val = sdhci_readl(host, TX_CFG_REG);
+	val |= TX_INTERNAL_SEL_BUS_CLK;
+	sdhci_writel(host, val, TX_CFG_REG);
+}
+
+static void pxav3_set_clock(struct sdhci_host *host, unsigned int clock)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_pxa *pxa = pltfm_host->priv;
+
+	/* We still use common sdhci_set_clock() */
+	sdhci_set_clock(host, clock);
+
+	/* platform/controller specific clock configuration */
+	if (pxa->data->set_pltfrm_clk && clock != 0) {
+		u32 val;
+
+		val = sdhci_readw(host, SD_FIFO_PARAM);
+		/* Internal clock gate ON and CTRL = 0b11 */
+		val |= INTERNAL_CLK_GATE_CTRL | INTERNAL_CLK_GATE_ON;
+		sdhci_writew(host, val, SD_FIFO_PARAM);
+
+		/* TX internal clock selection */
+		pxav3_set_tx_clock(host);
+	}
+
+}
+
 static const struct sdhci_ops pxav3_sdhci_ops = {
-	.set_clock			= sdhci_set_clock,
+	.set_clock			= pxav3_set_clock,
 	.platform_send_init_74_clocks	= pxav3_gen_init_74_clocks,
 	.get_max_clock			= sdhci_pltfm_clk_get_max_clock,
 	.set_bus_width			= sdhci_set_bus_width,
-- 
1.9.1

[PATCH-v2 4/7] mmc: sdhci-pxav3: Add pinctl setting according to bus clock

[Vaibhav Hiremath]

Different bus clock may need different pin setting.
For example, fast bus clock like 208Mhz need pin drive fast
while slow bus clock prefer pin drive slow to guarantee
signal quality.

So this patch creates two states,
  - Default (slow/normal) pin state
  - And fast pin state for higher freq bus speed.

And selection of pin state is done based on timing mode.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index c2b2b78..d933f75 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -35,6 +35,7 @@
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/mbus.h>
+#include <linux/pinctrl/consumer.h>
 
 #include "sdhci.h"
 #include "sdhci-pltfm.h"
@@ -92,6 +93,10 @@ struct sdhci_pxa {
 	void __iomem *io_pwr_reg;
 	void __iomem *io_pwr_lock_reg;
 	struct sdhci_pxa_data *data;
+
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *pins_default;
+	struct pinctrl_state *pins_fast;
 };
 
 static struct sdhci_pxa_data pxav3_data_v1 = {
@@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode)
 	pxa->power_mode = power_mode;
 }
 
+static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_pxa *pxa = pltfm_host->priv;
+	struct pinctrl_state *pinctrl;
+
+	if (IS_ERR(pxa->pinctrl) ||
+		IS_ERR(pxa->pins_default) ||
+		IS_ERR(pxa->pins_fast))
+		return -EINVAL;
+
+	switch (uhs) {
+	case MMC_TIMING_UHS_SDR50:
+	case MMC_TIMING_UHS_SDR104:
+	case MMC_TIMING_MMC_HS200:
+	case MMC_TIMING_MMC_HS400:
+		pinctrl = pxa->pins_fast;
+		break;
+	default:
+		/* back to default state for other legacy timing */
+		pinctrl = pxa->pins_default;
+		break;
+	}
+
+	return pinctrl_select_state(pxa->pinctrl, pinctrl);
+}
+
 static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -353,6 +385,8 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
 	dev_dbg(mmc_dev(host->mmc),
 		"%s uhs = %d, ctrl_2 = %04X\n",
 		__func__, uhs, ctrl_2);
+
+	pxav3_select_pinstate(host, uhs);
 }
 
 static void pxav3_voltage_switch(struct sdhci_host *host,
@@ -416,7 +450,6 @@ static void pxav3_set_clock(struct sdhci_host *host, unsigned int clock)
 		/* TX internal clock selection */
 		pxav3_set_tx_clock(host);
 	}
-
 }
 
 static const struct sdhci_ops pxav3_sdhci_ops = {
@@ -586,6 +619,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
 		}
 	}
 
+	pxa->pinctrl = devm_pinctrl_get(dev);
+	if (!IS_ERR(pxa->pinctrl)) {
+		pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default");
+		if (IS_ERR(pxa->pins_default))
+			dev_err(dev, "could not get default pinstate\n");
+		pxa->pins_fast = pinctrl_lookup_state(pxa->pinctrl, "fast");
+		if (IS_ERR(pxa->pins_fast))
+			dev_info(dev, "could not get fast pinstate\n");
+	}
+
 	pm_runtime_get_noresume(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_set_autosuspend_delay(&pdev->dev, PXAV3_RPM_DELAY_MS);
-- 
1.9.1

Re: [PATCH-v2 4/7] mmc: sdhci-pxav3: Add pinctl setting according to bus clock

[Jisheng Zhang]

On Mon, 7 Sep 2015 16:48:38 +0530
Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote:

> Different bus clock may need different pin setting.
> For example, fast bus clock like 208Mhz need pin drive fast
> while slow bus clock prefer pin drive slow to guarantee
> signal quality.
> 
> So this patch creates two states,
>   - Default (slow/normal) pin state
>   - And fast pin state for higher freq bus speed.
> 
> And selection of pin state is done based on timing mode.
> 
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> Signed-off-by: Kevin Liu <kliu5@marvell.com>
> ---
>  drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> index c2b2b78..d933f75 100644
> --- a/drivers/mmc/host/sdhci-pxav3.c
> +++ b/drivers/mmc/host/sdhci-pxav3.c
> @@ -35,6 +35,7 @@
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/mbus.h>
> +#include <linux/pinctrl/consumer.h>
>  
>  #include "sdhci.h"
>  #include "sdhci-pltfm.h"
> @@ -92,6 +93,10 @@ struct sdhci_pxa {
>  	void __iomem *io_pwr_reg;
>  	void __iomem *io_pwr_lock_reg;
>  	struct sdhci_pxa_data *data;
> +
> +	struct pinctrl *pinctrl;
> +	struct pinctrl_state *pins_default;
> +	struct pinctrl_state *pins_fast;
>  };
>  
>  static struct sdhci_pxa_data pxav3_data_v1 = {
> @@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode)
>  	pxa->power_mode = power_mode;
>  }
>  
> +static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_pxa *pxa = pltfm_host->priv;
> +	struct pinctrl_state *pinctrl;
> +
> +	if (IS_ERR(pxa->pinctrl) ||
> +		IS_ERR(pxa->pins_default) ||
> +		IS_ERR(pxa->pins_fast))
> +		return -EINVAL;
> +
> +	switch (uhs) {
> +	case MMC_TIMING_UHS_SDR50:
> +	case MMC_TIMING_UHS_SDR104:
> +	case MMC_TIMING_MMC_HS200:
> +	case MMC_TIMING_MMC_HS400:
> +		pinctrl = pxa->pins_fast;
> +		break;
> +	default:
> +		/* back to default state for other legacy timing */
> +		pinctrl = pxa->pins_default;
> +		break;
> +	}
> +
> +	return pinctrl_select_state(pxa->pinctrl, pinctrl);
> +}
> +
>  static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -353,6 +385,8 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>  	dev_dbg(mmc_dev(host->mmc),
>  		"%s uhs = %d, ctrl_2 = %04X\n",
>  		__func__, uhs, ctrl_2);
> +
> +	pxav3_select_pinstate(host, uhs);

return pxav3_select_pinstate(host, uhs);?

And could we ignore this call for those SDHCI hosts that don't need it?

>  }
>  
>  static void pxav3_voltage_switch(struct sdhci_host *host,
> @@ -416,7 +450,6 @@ static void pxav3_set_clock(struct sdhci_host *host, unsigned int clock)
>  		/* TX internal clock selection */
>  		pxav3_set_tx_clock(host);
>  	}
> -

why not fix this in patch 3?

>  }
>  
>  static const struct sdhci_ops pxav3_sdhci_ops = {
> @@ -586,6 +619,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	pxa->pinctrl = devm_pinctrl_get(dev);

could we ignore this for those SDHCI hosts that don't need it?

> +	if (!IS_ERR(pxa->pinctrl)) {
> +		pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default");
> +		if (IS_ERR(pxa->pins_default))
> +			dev_err(dev, "could not get default pinstate\n");

Why those SDHCI hosts that don't need pinctl setting should got this error?

> +		pxa->pins_fast = pinctrl_lookup_state(pxa->pinctrl, "fast");
> +		if (IS_ERR(pxa->pins_fast))
> +			dev_info(dev, "could not get fast pinstate\n");
> +	}
> +
>  	pm_runtime_get_noresume(&pdev->dev);
>  	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, PXAV3_RPM_DELAY_MS);

Re: [PATCH-v2 4/7] mmc: sdhci-pxav3: Add pinctl setting according to bus clock

[Vaibhav Hiremath]

On Tuesday 08 September 2015 12:22 PM, Jisheng Zhang wrote:
> On Mon, 7 Sep 2015 16:48:38 +0530
> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote:
>
>> Different bus clock may need different pin setting.
>> For example, fast bus clock like 208Mhz need pin drive fast
>> while slow bus clock prefer pin drive slow to guarantee
>> signal quality.
>>
>> So this patch creates two states,
>>    - Default (slow/normal) pin state
>>    - And fast pin state for higher freq bus speed.
>>
>> And selection of pin state is done based on timing mode.
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> Signed-off-by: Kevin Liu <kliu5@marvell.com>
>> ---
>>   drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 44 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
>> index c2b2b78..d933f75 100644
>> --- a/drivers/mmc/host/sdhci-pxav3.c
>> +++ b/drivers/mmc/host/sdhci-pxav3.c
>> @@ -35,6 +35,7 @@
>>   #include <linux/pm.h>
>>   #include <linux/pm_runtime.h>
>>   #include <linux/mbus.h>
>> +#include <linux/pinctrl/consumer.h>
>>
>>   #include "sdhci.h"
>>   #include "sdhci-pltfm.h"
>> @@ -92,6 +93,10 @@ struct sdhci_pxa {
>>   	void __iomem *io_pwr_reg;
>>   	void __iomem *io_pwr_lock_reg;
>>   	struct sdhci_pxa_data *data;
>> +
>> +	struct pinctrl *pinctrl;
>> +	struct pinctrl_state *pins_default;
>> +	struct pinctrl_state *pins_fast;
>>   };
>>
>>   static struct sdhci_pxa_data pxav3_data_v1 = {
>> @@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode)
>>   	pxa->power_mode = power_mode;
>>   }
>>
>> +static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs)
>> +{
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_pxa *pxa = pltfm_host->priv;
>> +	struct pinctrl_state *pinctrl;
>> +
>> +	if (IS_ERR(pxa->pinctrl) ||
>> +		IS_ERR(pxa->pins_default) ||
>> +		IS_ERR(pxa->pins_fast))
>> +		return -EINVAL;
>> +
>> +	switch (uhs) {
>> +	case MMC_TIMING_UHS_SDR50:
>> +	case MMC_TIMING_UHS_SDR104:
>> +	case MMC_TIMING_MMC_HS200:
>> +	case MMC_TIMING_MMC_HS400:
>> +		pinctrl = pxa->pins_fast;
>> +		break;
>> +	default:
>> +		/* back to default state for other legacy timing */
>> +		pinctrl = pxa->pins_default;
>> +		break;
>> +	}
>> +
>> +	return pinctrl_select_state(pxa->pinctrl, pinctrl);
>> +}
>> +
>>   static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>>   {
>>   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> @@ -353,6 +385,8 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>>   	dev_dbg(mmc_dev(host->mmc),
>>   		"%s uhs = %d, ctrl_2 = %04X\n",
>>   		__func__, uhs, ctrl_2);
>> +
>> +	pxav3_select_pinstate(host, uhs);
>
> return pxav3_select_pinstate(host, uhs);?
>

Its void function, so don't need it.

> And could we ignore this call for those SDHCI hosts that don't need it?
>

I do not want to introduce flags here.
And also, it is already ignored for those who don't need it.

The first thing in the function pxav3_select_pinstate() is


	if (IS_ERR(pxa->pinctrl) ||
		IS_ERR(pxa->pins_default) ||
		IS_ERR(pxa->pins_fast))
		return -EINVAL;


So its already handled.

>>   }
>>
>>   static void pxav3_voltage_switch(struct sdhci_host *host,
>> @@ -416,7 +450,6 @@ static void pxav3_set_clock(struct sdhci_host *host, unsigned int clock)
>>   		/* TX internal clock selection */
>>   		pxav3_set_tx_clock(host);
>>   	}
>> -
>
> why not fix this in patch 3?
>

Oops. it got missed from my eyes :)
Will fix in next version.

>>   }
>>
>>   static const struct sdhci_ops pxav3_sdhci_ops = {
>> @@ -586,6 +619,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>>   		}
>>   	}
>>
>> +	pxa->pinctrl = devm_pinctrl_get(dev);
>
> could we ignore this for those SDHCI hosts that don't need it?
>

Again, no need to introduce flags here. This is standard call and
handled properly. So for the platforms not using this, it really should
not matter.
Also, lookup is getting executed only when pinctrl is populated.

So I do not see any need here.

>> +	if (!IS_ERR(pxa->pinctrl)) {
>> +		pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default");
>> +		if (IS_ERR(pxa->pins_default))
>> +			dev_err(dev, "could not get default pinstate\n");
>
> Why those SDHCI hosts that don't need pinctl setting should got this error?
>

It won't.

If Host does not need pinctrl, the execution would never reach this
point.
The if condition check would handle it, isn't it?

	pxa->pinctrl = devm_pinctrl_get(dev);
	if (!IS_ERR(pxa->pinctrl)) {

		...

	}

So I do not see why is it issue here?


Thanks,
Vaibhav

Re: [PATCH-v2 4/7] mmc: sdhci-pxav3: Add pinctl setting according to bus clock

[Jisheng Zhang]

On Tue, 8 Sep 2015 15:04:41 +0530
Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> 
> 
> On Tuesday 08 September 2015 12:22 PM, Jisheng Zhang wrote:
> > On Mon, 7 Sep 2015 16:48:38 +0530
> > Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> >
> >> Different bus clock may need different pin setting.
> >> For example, fast bus clock like 208Mhz need pin drive fast
> >> while slow bus clock prefer pin drive slow to guarantee
> >> signal quality.
> >>
> >> So this patch creates two states,
> >>    - Default (slow/normal) pin state
> >>    - And fast pin state for higher freq bus speed.
> >>
> >> And selection of pin state is done based on timing mode.
> >>
> >> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> Signed-off-by: Kevin Liu <kliu5-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> >> ---
> >>   drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++-
> >>   1 file changed, 44 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> >> index c2b2b78..d933f75 100644
> >> --- a/drivers/mmc/host/sdhci-pxav3.c
> >> +++ b/drivers/mmc/host/sdhci-pxav3.c
> >> @@ -35,6 +35,7 @@
> >>   #include <linux/pm.h>
> >>   #include <linux/pm_runtime.h>
> >>   #include <linux/mbus.h>
> >> +#include <linux/pinctrl/consumer.h>
> >>
> >>   #include "sdhci.h"
> >>   #include "sdhci-pltfm.h"
> >> @@ -92,6 +93,10 @@ struct sdhci_pxa {
> >>   	void __iomem *io_pwr_reg;
> >>   	void __iomem *io_pwr_lock_reg;
> >>   	struct sdhci_pxa_data *data;
> >> +
> >> +	struct pinctrl *pinctrl;
> >> +	struct pinctrl_state *pins_default;
> >> +	struct pinctrl_state *pins_fast;
> >>   };
> >>
> >>   static struct sdhci_pxa_data pxav3_data_v1 = {
> >> @@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode)
> >>   	pxa->power_mode = power_mode;
> >>   }
> >>
> >> +static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs)
> >> +{
> >> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >> +	struct sdhci_pxa *pxa = pltfm_host->priv;
> >> +	struct pinctrl_state *pinctrl;
> >> +
> >> +	if (IS_ERR(pxa->pinctrl) ||
> >> +		IS_ERR(pxa->pins_default) ||
> >> +		IS_ERR(pxa->pins_fast))
> >> +		return -EINVAL;
> >> +
> >> +	switch (uhs) {
> >> +	case MMC_TIMING_UHS_SDR50:
> >> +	case MMC_TIMING_UHS_SDR104:
> >> +	case MMC_TIMING_MMC_HS200:
> >> +	case MMC_TIMING_MMC_HS400:
> >> +		pinctrl = pxa->pins_fast;
> >> +		break;
> >> +	default:
> >> +		/* back to default state for other legacy timing */
> >> +		pinctrl = pxa->pins_default;
> >> +		break;
> >> +	}
> >> +
> >> +	return pinctrl_select_state(pxa->pinctrl, pinctrl);
> >> +}
> >> +
> >>   static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
> >>   {
> >>   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >> @@ -353,6 +385,8 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
> >>   	dev_dbg(mmc_dev(host->mmc),
> >>   		"%s uhs = %d, ctrl_2 = %04X\n",
> >>   		__func__, uhs, ctrl_2);
> >> +
> >> +	pxav3_select_pinstate(host, uhs);
> >
> > return pxav3_select_pinstate(host, uhs);?
> >
> 
> Its void function, so don't need it.

Can you please have a look at the function? 

static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs)

> 
> > And could we ignore this call for those SDHCI hosts that don't need it?
> >
> 
> I do not want to introduce flags here.
> And also, it is already ignored for those who don't need it.
> 
> The first thing in the function pxav3_select_pinstate() is
> 
> 
> 	if (IS_ERR(pxa->pinctrl) ||
> 		IS_ERR(pxa->pins_default) ||
> 		IS_ERR(pxa->pins_fast))
> 		return -EINVAL;
> 
> 
> So its already handled.
> 
> >>   }
> >>
> >>   static void pxav3_voltage_switch(struct sdhci_host *host,
> >> @@ -416,7 +450,6 @@ static void pxav3_set_clock(struct sdhci_host *host, unsigned int clock)
> >>   		/* TX internal clock selection */
> >>   		pxav3_set_tx_clock(host);
> >>   	}
> >> -
> >
> > why not fix this in patch 3?
> >
> 
> Oops. it got missed from my eyes :)
> Will fix in next version.
> 
> >>   }
> >>
> >>   static const struct sdhci_ops pxav3_sdhci_ops = {
> >> @@ -586,6 +619,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
> >>   		}
> >>   	}
> >>
> >> +	pxa->pinctrl = devm_pinctrl_get(dev);
> >
> > could we ignore this for those SDHCI hosts that don't need it?
> >
> 
> Again, no need to introduce flags here. This is standard call and
> handled properly. So for the platforms not using this, it really should
> not matter.
> Also, lookup is getting executed only when pinctrl is populated.
> 
> So I do not see any need here.
> 
> >> +	if (!IS_ERR(pxa->pinctrl)) {
> >> +		pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default");
> >> +		if (IS_ERR(pxa->pins_default))
> >> +			dev_err(dev, "could not get default pinstate\n");
> >
> > Why those SDHCI hosts that don't need pinctl setting should got this error?
> >
> 
> It won't.

It does. On Marvell Berlin SoCs, I got

[    1.070000] sdhci-pxav3 f7ab0800.sdhci: could not get default pinstate
[    1.080000] sdhci-pxav3 f7ab0800.sdhci: could not get fast pinstate



> 
> If Host does not need pinctrl, the execution would never reach this
> point.
> The if condition check would handle it, isn't it?
> 
> 	pxa->pinctrl = devm_pinctrl_get(dev);

It seems this function always succeed...

>From another side, we may have default pin in dts, for example: pin muxed between
emmc and nandflash. But we don't have fast pinstate, so we at least need the
flag to fast pinstate. Otherwise, in such platforms, we could get something like

[    1.000000] sdhci-pxav3 f7ab0000.sdhci: get default pinstate
[    1.000000] sdhci-pxav3 f7ab0000.sdhci: could not get fast pinstate


> 	if (!IS_ERR(pxa->pinctrl)) {
> 
> 		...
> 
> 	}
> 
> So I do not see why is it issue here?
> 
> 
> Thanks,
> Vaibhav

--
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 4/7] mmc: sdhci-pxav3: Add pinctl setting according to bus clock

[Vaibhav Hiremath]

On Tuesday 08 September 2015 03:22 PM, Jisheng Zhang wrote:
> On Tue, 8 Sep 2015 15:04:41 +0530
> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote:
>
>>
>>
>> On Tuesday 08 September 2015 12:22 PM, Jisheng Zhang wrote:
>>> On Mon, 7 Sep 2015 16:48:38 +0530
>>> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote:
>>>
>>>> Different bus clock may need different pin setting.
>>>> For example, fast bus clock like 208Mhz need pin drive fast
>>>> while slow bus clock prefer pin drive slow to guarantee
>>>> signal quality.
>>>>
>>>> So this patch creates two states,
>>>>     - Default (slow/normal) pin state
>>>>     - And fast pin state for higher freq bus speed.
>>>>
>>>> And selection of pin state is done based on timing mode.
>>>>
>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>> Signed-off-by: Kevin Liu <kliu5@marvell.com>
>>>> ---
>>>>    drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 44 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
>>>> index c2b2b78..d933f75 100644
>>>> --- a/drivers/mmc/host/sdhci-pxav3.c
>>>> +++ b/drivers/mmc/host/sdhci-pxav3.c
>>>> @@ -35,6 +35,7 @@
>>>>    #include <linux/pm.h>
>>>>    #include <linux/pm_runtime.h>
>>>>    #include <linux/mbus.h>
>>>> +#include <linux/pinctrl/consumer.h>
>>>>
>>>>    #include "sdhci.h"
>>>>    #include "sdhci-pltfm.h"
>>>> @@ -92,6 +93,10 @@ struct sdhci_pxa {
>>>>    	void __iomem *io_pwr_reg;
>>>>    	void __iomem *io_pwr_lock_reg;
>>>>    	struct sdhci_pxa_data *data;
>>>> +
>>>> +	struct pinctrl *pinctrl;
>>>> +	struct pinctrl_state *pins_default;
>>>> +	struct pinctrl_state *pins_fast;
>>>>    };
>>>>
>>>>    static struct sdhci_pxa_data pxav3_data_v1 = {
>>>> @@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode)
>>>>    	pxa->power_mode = power_mode;
>>>>    }
>>>>
>>>> +static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs)
>>>> +{
>>>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +	struct sdhci_pxa *pxa = pltfm_host->priv;
>>>> +	struct pinctrl_state *pinctrl;
>>>> +
>>>> +	if (IS_ERR(pxa->pinctrl) ||
>>>> +		IS_ERR(pxa->pins_default) ||
>>>> +		IS_ERR(pxa->pins_fast))
>>>> +		return -EINVAL;
>>>> +
>>>> +	switch (uhs) {
>>>> +	case MMC_TIMING_UHS_SDR50:
>>>> +	case MMC_TIMING_UHS_SDR104:
>>>> +	case MMC_TIMING_MMC_HS200:
>>>> +	case MMC_TIMING_MMC_HS400:
>>>> +		pinctrl = pxa->pins_fast;
>>>> +		break;
>>>> +	default:
>>>> +		/* back to default state for other legacy timing */
>>>> +		pinctrl = pxa->pins_default;
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	return pinctrl_select_state(pxa->pinctrl, pinctrl);
>>>> +}
>>>> +
>>>>    static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>>>>    {
>>>>    	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> @@ -353,6 +385,8 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>>>>    	dev_dbg(mmc_dev(host->mmc),
>>>>    		"%s uhs = %d, ctrl_2 = %04X\n",
>>>>    		__func__, uhs, ctrl_2);
>>>> +
>>>> +	pxav3_select_pinstate(host, uhs);
>>>
>>> return pxav3_select_pinstate(host, uhs);?
>>>
>>
>> Its void function, so don't need it.
>
> Can you please have a look at the function?
>
> static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs)
>

I was referring to pxav3_set_uhs_signaling().
It is void, so what you are suggesting would generate warning.

>>
>>> And could we ignore this call for those SDHCI hosts that don't need it?
>>>
>>
>> I do not want to introduce flags here.
>> And also, it is already ignored for those who don't need it.
>>
>> The first thing in the function pxav3_select_pinstate() is
>>
>>
>> 	if (IS_ERR(pxa->pinctrl) ||
>> 		IS_ERR(pxa->pins_default) ||
>> 		IS_ERR(pxa->pins_fast))
>> 		return -EINVAL;
>>
>>
>> So its already handled.
>>
>>>>    }
>>>>
>>>>    static void pxav3_voltage_switch(struct sdhci_host *host,
>>>> @@ -416,7 +450,6 @@ static void pxav3_set_clock(struct sdhci_host *host, unsigned int clock)
>>>>    		/* TX internal clock selection */
>>>>    		pxav3_set_tx_clock(host);
>>>>    	}
>>>> -
>>>
>>> why not fix this in patch 3?
>>>
>>
>> Oops. it got missed from my eyes :)
>> Will fix in next version.
>>
>>>>    }
>>>>
>>>>    static const struct sdhci_ops pxav3_sdhci_ops = {
>>>> @@ -586,6 +619,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>>>>    		}
>>>>    	}
>>>>
>>>> +	pxa->pinctrl = devm_pinctrl_get(dev);
>>>
>>> could we ignore this for those SDHCI hosts that don't need it?
>>>
>>
>> Again, no need to introduce flags here. This is standard call and
>> handled properly. So for the platforms not using this, it really should
>> not matter.
>> Also, lookup is getting executed only when pinctrl is populated.
>>
>> So I do not see any need here.
>>
>>>> +	if (!IS_ERR(pxa->pinctrl)) {
>>>> +		pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default");
>>>> +		if (IS_ERR(pxa->pins_default))
>>>> +			dev_err(dev, "could not get default pinstate\n");
>>>
>>> Why those SDHCI hosts that don't need pinctl setting should got this error?
>>>
>>
>> It won't.
>
> It does. On Marvell Berlin SoCs, I got
>
> [    1.070000] sdhci-pxav3 f7ab0800.sdhci: could not get default pinstate
> [    1.080000] sdhci-pxav3 f7ab0800.sdhci: could not get fast pinstate
>
>
>
>>
>> If Host does not need pinctrl, the execution would never reach this
>> point.
>> The if condition check would handle it, isn't it?
>>
>> 	pxa->pinctrl = devm_pinctrl_get(dev);
>
> It seems this function always succeed...
>

Not always.
I would succeed only if you have pinctrl defined in DT for this device.

And if you have pinctrl defined, isn't it is expected to have "default"
pin state to be always present?
And if answer is yes here, then it is fair to be prompting error for it.

>  From another side, we may have default pin in dts, for example: pin muxed between
> emmc and nandflash. But we don't have fast pinstate, so we at least need the
> flag to fast pinstate. Otherwise, in such platforms, we could get something like
>

That is exactly the reason behind keeping it as dev_info.

> [    1.000000] sdhci-pxav3 f7ab0000.sdhci: get default pinstate
> [    1.000000] sdhci-pxav3 f7ab0000.sdhci: could not get fast pinstate
>
>
Thanks,
Vaibhav

Re: [PATCH-v2 4/7] mmc: sdhci-pxav3: Add pinctl setting according to bus clock

[Jisheng Zhang]

On Tue, 8 Sep 2015 15:32:34 +0530
Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote:

> 
> 
> On Tuesday 08 September 2015 03:22 PM, Jisheng Zhang wrote:
> > On Tue, 8 Sep 2015 15:04:41 +0530
> > Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote:
> >
> >>
> >>
> >> On Tuesday 08 September 2015 12:22 PM, Jisheng Zhang wrote:
> >>> On Mon, 7 Sep 2015 16:48:38 +0530
> >>> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote:
> >>>
> >>>> Different bus clock may need different pin setting.
> >>>> For example, fast bus clock like 208Mhz need pin drive fast
> >>>> while slow bus clock prefer pin drive slow to guarantee
> >>>> signal quality.
> >>>>
> >>>> So this patch creates two states,
> >>>>     - Default (slow/normal) pin state
> >>>>     - And fast pin state for higher freq bus speed.
> >>>>
> >>>> And selection of pin state is done based on timing mode.
> >>>>
> >>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> >>>> Signed-off-by: Kevin Liu <kliu5@marvell.com>
> >>>> ---
> >>>>    drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++-
> >>>>    1 file changed, 44 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> >>>> index c2b2b78..d933f75 100644
> >>>> --- a/drivers/mmc/host/sdhci-pxav3.c
> >>>> +++ b/drivers/mmc/host/sdhci-pxav3.c
> >>>> @@ -35,6 +35,7 @@
> >>>>    #include <linux/pm.h>
> >>>>    #include <linux/pm_runtime.h>
> >>>>    #include <linux/mbus.h>
> >>>> +#include <linux/pinctrl/consumer.h>
> >>>>
> >>>>    #include "sdhci.h"
> >>>>    #include "sdhci-pltfm.h"
> >>>> @@ -92,6 +93,10 @@ struct sdhci_pxa {
> >>>>    	void __iomem *io_pwr_reg;
> >>>>    	void __iomem *io_pwr_lock_reg;
> >>>>    	struct sdhci_pxa_data *data;
> >>>> +
> >>>> +	struct pinctrl *pinctrl;
> >>>> +	struct pinctrl_state *pins_default;
> >>>> +	struct pinctrl_state *pins_fast;
> >>>>    };
> >>>>
> >>>>    static struct sdhci_pxa_data pxav3_data_v1 = {
> >>>> @@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode)
> >>>>    	pxa->power_mode = power_mode;
> >>>>    }
> >>>>
> >>>> +static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs)
> >>>> +{
> >>>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >>>> +	struct sdhci_pxa *pxa = pltfm_host->priv;
> >>>> +	struct pinctrl_state *pinctrl;
> >>>> +
> >>>> +	if (IS_ERR(pxa->pinctrl) ||
> >>>> +		IS_ERR(pxa->pins_default) ||
> >>>> +		IS_ERR(pxa->pins_fast))
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	switch (uhs) {
> >>>> +	case MMC_TIMING_UHS_SDR50:
> >>>> +	case MMC_TIMING_UHS_SDR104:
> >>>> +	case MMC_TIMING_MMC_HS200:
> >>>> +	case MMC_TIMING_MMC_HS400:
> >>>> +		pinctrl = pxa->pins_fast;
> >>>> +		break;
> >>>> +	default:
> >>>> +		/* back to default state for other legacy timing */
> >>>> +		pinctrl = pxa->pins_default;
> >>>> +		break;
> >>>> +	}
> >>>> +
> >>>> +	return pinctrl_select_state(pxa->pinctrl, pinctrl);
> >>>> +}
> >>>> +
> >>>>    static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
> >>>>    {
> >>>>    	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >>>> @@ -353,6 +385,8 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
> >>>>    	dev_dbg(mmc_dev(host->mmc),
> >>>>    		"%s uhs = %d, ctrl_2 = %04X\n",
> >>>>    		__func__, uhs, ctrl_2);
> >>>> +
> >>>> +	pxav3_select_pinstate(host, uhs);
> >>>
> >>> return pxav3_select_pinstate(host, uhs);?
> >>>
> >>
> >> Its void function, so don't need it.
> >
> > Can you please have a look at the function?
> >
> > static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs)
> >
> 
> I was referring to pxav3_set_uhs_signaling().
> It is void, so what you are suggesting would generate warning.

Oops, you are correct.

> 
> >>
> >>> And could we ignore this call for those SDHCI hosts that don't need it?
> >>>
> >>
> >> I do not want to introduce flags here.
> >> And also, it is already ignored for those who don't need it.
> >>
> >> The first thing in the function pxav3_select_pinstate() is
> >>
> >>
> >> 	if (IS_ERR(pxa->pinctrl) ||
> >> 		IS_ERR(pxa->pins_default) ||
> >> 		IS_ERR(pxa->pins_fast))
> >> 		return -EINVAL;
> >>
> >>
> >> So its already handled.
> >>
> >>>>    }
> >>>>
> >>>>    static void pxav3_voltage_switch(struct sdhci_host *host,
> >>>> @@ -416,7 +450,6 @@ static void pxav3_set_clock(struct sdhci_host *host, unsigned int clock)
> >>>>    		/* TX internal clock selection */
> >>>>    		pxav3_set_tx_clock(host);
> >>>>    	}
> >>>> -
> >>>
> >>> why not fix this in patch 3?
> >>>
> >>
> >> Oops. it got missed from my eyes :)
> >> Will fix in next version.
> >>
> >>>>    }
> >>>>
> >>>>    static const struct sdhci_ops pxav3_sdhci_ops = {
> >>>> @@ -586,6 +619,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
> >>>>    		}
> >>>>    	}
> >>>>
> >>>> +	pxa->pinctrl = devm_pinctrl_get(dev);
> >>>
> >>> could we ignore this for those SDHCI hosts that don't need it?
> >>>
> >>
> >> Again, no need to introduce flags here. This is standard call and
> >> handled properly. So for the platforms not using this, it really should
> >> not matter.
> >> Also, lookup is getting executed only when pinctrl is populated.
> >>
> >> So I do not see any need here.
> >>
> >>>> +	if (!IS_ERR(pxa->pinctrl)) {
> >>>> +		pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default");
> >>>> +		if (IS_ERR(pxa->pins_default))
> >>>> +			dev_err(dev, "could not get default pinstate\n");
> >>>
> >>> Why those SDHCI hosts that don't need pinctl setting should got this error?
> >>>
> >>
> >> It won't.
> >
> > It does. On Marvell Berlin SoCs, I got
> >
> > [    1.070000] sdhci-pxav3 f7ab0800.sdhci: could not get default pinstate
> > [    1.080000] sdhci-pxav3 f7ab0800.sdhci: could not get fast pinstate
> >
> >
> >
> >>
> >> If Host does not need pinctrl, the execution would never reach this
> >> point.
> >> The if condition check would handle it, isn't it?
> >>
> >> 	pxa->pinctrl = devm_pinctrl_get(dev);
> >
> > It seems this function always succeed...
> >
> 
> Not always.
> I would succeed only if you have pinctrl defined in DT for this device.

Yes, that's what I thought, but I got

[    1.070000] sdhci-pxav3 f7ab0800.sdhci: could not get default pinstate
[    1.080000] sdhci-pxav3 f7ab0800.sdhci: could not get fast pinstate

there's no pinctrl for f7ab0800.sdhci. Am I missing somthing?

Thanks,
Jisheng

> 
> And if you have pinctrl defined, isn't it is expected to have "default"
> pin state to be always present?
> And if answer is yes here, then it is fair to be prompting error for it.
> 
> >  From another side, we may have default pin in dts, for example: pin muxed between
> > emmc and nandflash. But we don't have fast pinstate, so we at least need the
> > flag to fast pinstate. Otherwise, in such platforms, we could get something like
> >
> 
> That is exactly the reason behind keeping it as dev_info.
> 
> > [    1.000000] sdhci-pxav3 f7ab0000.sdhci: get default pinstate
> > [    1.000000] sdhci-pxav3 f7ab0000.sdhci: could not get fast pinstate
> >
> >
> Thanks,
> Vaibhav

Re: [PATCH-v2 4/7] mmc: sdhci-pxav3: Add pinctl setting according to bus clock

[Vaibhav Hiremath]

On Tuesday 08 September 2015 03:34 PM, Jisheng Zhang wrote:
> On Tue, 8 Sep 2015 15:32:34 +0530
> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote:
>
>>
>>
>> On Tuesday 08 September 2015 03:22 PM, Jisheng Zhang wrote:
>>> On Tue, 8 Sep 2015 15:04:41 +0530
>>> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote:
>>>

<snip>

>>>>>>     static const struct sdhci_ops pxav3_sdhci_ops = {
>>>>>> @@ -586,6 +619,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>>>>>>     		}
>>>>>>     	}
>>>>>>
>>>>>> +	pxa->pinctrl = devm_pinctrl_get(dev);
>>>>>
>>>>> could we ignore this for those SDHCI hosts that don't need it?
>>>>>
>>>>
>>>> Again, no need to introduce flags here. This is standard call and
>>>> handled properly. So for the platforms not using this, it really should
>>>> not matter.
>>>> Also, lookup is getting executed only when pinctrl is populated.
>>>>
>>>> So I do not see any need here.
>>>>
>>>>>> +	if (!IS_ERR(pxa->pinctrl)) {
>>>>>> +		pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default");
>>>>>> +		if (IS_ERR(pxa->pins_default))
>>>>>> +			dev_err(dev, "could not get default pinstate\n");
>>>>>
>>>>> Why those SDHCI hosts that don't need pinctl setting should got this error?
>>>>>
>>>>
>>>> It won't.
>>>
>>> It does. On Marvell Berlin SoCs, I got
>>>
>>> [    1.070000] sdhci-pxav3 f7ab0800.sdhci: could not get default pinstate
>>> [    1.080000] sdhci-pxav3 f7ab0800.sdhci: could not get fast pinstate
>>>
>>>
>>>
>>>>
>>>> If Host does not need pinctrl, the execution would never reach this
>>>> point.
>>>> The if condition check would handle it, isn't it?
>>>>
>>>> 	pxa->pinctrl = devm_pinctrl_get(dev);
>>>
>>> It seems this function always succeed...
>>>
>>
>> Not always.
>> I would succeed only if you have pinctrl defined in DT for this device.
>
> Yes, that's what I thought, but I got
>
> [    1.070000] sdhci-pxav3 f7ab0800.sdhci: could not get default pinstate
> [    1.080000] sdhci-pxav3 f7ab0800.sdhci: could not get fast pinstate
>
> there's no pinctrl for f7ab0800.sdhci. Am I missing somthing?
>
> Thanks,
> Jisheng
>
>>
>> And if you have pinctrl defined, isn't it is expected to have "default"
>> pin state to be always present?
>> And if answer is yes here, then it is fair to be prompting error for it.
>>
>>>   From another side, we may have default pin in dts, for example: pin muxed between
>>> emmc and nandflash. But we don't have fast pinstate, so we at least need the
>>> flag to fast pinstate. Otherwise, in such platforms, we could get something like
>>>
>>
>> That is exactly the reason behind keeping it as dev_info.
>>
>>> [    1.000000] sdhci-pxav3 f7ab0000.sdhci: get default pinstate
>>> [    1.000000] sdhci-pxav3 f7ab0000.sdhci: could not get fast pinstate
>>>
>>>

I did some invastigation here on the execution flow,
and you know what, you are right here.

It seems, devm_pinctrl_get() always returns valid pinctrl pointer, even
though the DT property is not populated.

The return value from I did some invastigation () should have been 
treated differently, but it is not. Instead it creates the
"struct pinctrl" and return back to the driver.

I am looping Linus Walleji here, probably he can comment/confirm on
this.


Thanks,
Vaibhav


>> Thanks,
>> Vaibhav
>

Re: [PATCH-v2 4/7] mmc: sdhci-pxav3: Add pinctl setting according to bus clock

[Jisheng Zhang]

On Mon, 7 Sep 2015 16:48:38 +0530
Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote:

> Different bus clock may need different pin setting.
> For example, fast bus clock like 208Mhz need pin drive fast
> while slow bus clock prefer pin drive slow to guarantee
> signal quality.
> 
> So this patch creates two states,
>   - Default (slow/normal) pin state
>   - And fast pin state for higher freq bus speed.
> 
> And selection of pin state is done based on timing mode.
> 
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> Signed-off-by: Kevin Liu <kliu5@marvell.com>
> ---
>  drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> index c2b2b78..d933f75 100644
> --- a/drivers/mmc/host/sdhci-pxav3.c
> +++ b/drivers/mmc/host/sdhci-pxav3.c
> @@ -35,6 +35,7 @@
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/mbus.h>
> +#include <linux/pinctrl/consumer.h>
>  
>  #include "sdhci.h"
>  #include "sdhci-pltfm.h"
> @@ -92,6 +93,10 @@ struct sdhci_pxa {
>  	void __iomem *io_pwr_reg;
>  	void __iomem *io_pwr_lock_reg;
>  	struct sdhci_pxa_data *data;
> +
> +	struct pinctrl *pinctrl;
> +	struct pinctrl_state *pins_default;
> +	struct pinctrl_state *pins_fast;
>  };
>  
>  static struct sdhci_pxa_data pxav3_data_v1 = {
> @@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode)
>  	pxa->power_mode = power_mode;
>  }
>  
> +static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_pxa *pxa = pltfm_host->priv;
> +	struct pinctrl_state *pinctrl;
> +
> +	if (IS_ERR(pxa->pinctrl) ||
> +		IS_ERR(pxa->pins_default) ||
> +		IS_ERR(pxa->pins_fast))
> +		return -EINVAL;
> +
> +	switch (uhs) {
> +	case MMC_TIMING_UHS_SDR50:
> +	case MMC_TIMING_UHS_SDR104:
> +	case MMC_TIMING_MMC_HS200:
> +	case MMC_TIMING_MMC_HS400:

Are you sure this IP can support HS400?

> +		pinctrl = pxa->pins_fast;
> +		break;
> +	default:
> +		/* back to default state for other legacy timing */
> +		pinctrl = pxa->pins_default;
> +		break;
> +	}
> +
> +	return pinctrl_select_state(pxa->pinctrl, pinctrl);
> +}
> +
>  static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -353,6 +385,8 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>  	dev_dbg(mmc_dev(host->mmc),
>  		"%s uhs = %d, ctrl_2 = %04X\n",
>  		__func__, uhs, ctrl_2);
> +
> +	pxav3_select_pinstate(host, uhs);
>  }
>  
>  static void pxav3_voltage_switch(struct sdhci_host *host,
> @@ -416,7 +450,6 @@ static void pxav3_set_clock(struct sdhci_host *host, unsigned int clock)
>  		/* TX internal clock selection */
>  		pxav3_set_tx_clock(host);
>  	}
> -
>  }
>  
>  static const struct sdhci_ops pxav3_sdhci_ops = {
> @@ -586,6 +619,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	pxa->pinctrl = devm_pinctrl_get(dev);
> +	if (!IS_ERR(pxa->pinctrl)) {
> +		pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default");
> +		if (IS_ERR(pxa->pins_default))
> +			dev_err(dev, "could not get default pinstate\n");
> +		pxa->pins_fast = pinctrl_lookup_state(pxa->pinctrl, "fast");
> +		if (IS_ERR(pxa->pins_fast))
> +			dev_info(dev, "could not get fast pinstate\n");
> +	}
> +
>  	pm_runtime_get_noresume(&pdev->dev);
>  	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, PXAV3_RPM_DELAY_MS);

Re: [PATCH-v2 4/7] mmc: sdhci-pxav3: Add pinctl setting according to bus clock

[Vaibhav Hiremath]

On Tuesday 08 September 2015 12:24 PM, Jisheng Zhang wrote:
> On Mon, 7 Sep 2015 16:48:38 +0530
> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote:
>
>> Different bus clock may need different pin setting.
>> For example, fast bus clock like 208Mhz need pin drive fast
>> while slow bus clock prefer pin drive slow to guarantee
>> signal quality.
>>
>> So this patch creates two states,
>>    - Default (slow/normal) pin state
>>    - And fast pin state for higher freq bus speed.
>>
>> And selection of pin state is done based on timing mode.
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> Signed-off-by: Kevin Liu <kliu5@marvell.com>
>> ---
>>   drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 44 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
>> index c2b2b78..d933f75 100644
>> --- a/drivers/mmc/host/sdhci-pxav3.c
>> +++ b/drivers/mmc/host/sdhci-pxav3.c
>> @@ -35,6 +35,7 @@
>>   #include <linux/pm.h>
>>   #include <linux/pm_runtime.h>
>>   #include <linux/mbus.h>
>> +#include <linux/pinctrl/consumer.h>
>>
>>   #include "sdhci.h"
>>   #include "sdhci-pltfm.h"
>> @@ -92,6 +93,10 @@ struct sdhci_pxa {
>>   	void __iomem *io_pwr_reg;
>>   	void __iomem *io_pwr_lock_reg;
>>   	struct sdhci_pxa_data *data;
>> +
>> +	struct pinctrl *pinctrl;
>> +	struct pinctrl_state *pins_default;
>> +	struct pinctrl_state *pins_fast;
>>   };
>>
>>   static struct sdhci_pxa_data pxav3_data_v1 = {
>> @@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode)
>>   	pxa->power_mode = power_mode;
>>   }
>>
>> +static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs)
>> +{
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_pxa *pxa = pltfm_host->priv;
>> +	struct pinctrl_state *pinctrl;
>> +
>> +	if (IS_ERR(pxa->pinctrl) ||
>> +		IS_ERR(pxa->pins_default) ||
>> +		IS_ERR(pxa->pins_fast))
>> +		return -EINVAL;
>> +
>> +	switch (uhs) {
>> +	case MMC_TIMING_UHS_SDR50:
>> +	case MMC_TIMING_UHS_SDR104:
>> +	case MMC_TIMING_MMC_HS200:
>> +	case MMC_TIMING_MMC_HS400:
>
> Are you sure this IP can support HS400?
>
>> +		pinctrl = pxa->pins_fast;


No it does not.

The reason I put it here is,
code under discussion is related to pinctrl configuration, so for all
higher bus speed mode we want to set pins into fast mode.

And from hs400 perspective, I would expect it must get handled much
before than this stage and should not reach here at all.

So this portion of the code is complete in itself.

I hope I clarified your doubt.

Thanks,
Vaibhav

Re: [PATCH-v2 4/7] mmc: sdhci-pxav3: Add pinctl setting according to bus clock

[Linus Walleij]

On Mon, Sep 7, 2015 at 1:18 PM, Vaibhav Hiremath
<vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> Different bus clock may need different pin setting.
> For example, fast bus clock like 208Mhz need pin drive fast
> while slow bus clock prefer pin drive slow to guarantee
> signal quality.
>
> So this patch creates two states,
>   - Default (slow/normal) pin state
>   - And fast pin state for higher freq bus speed.
>
> And selection of pin state is done based on timing mode.
>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Kevin Liu <kliu5-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
(...)
> +       pxa->pinctrl = devm_pinctrl_get(dev);
> +       if (!IS_ERR(pxa->pinctrl)) {
> +               pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default");
> +               if (IS_ERR(pxa->pins_default))
> +                       dev_err(dev, "could not get default pinstate\n");
> +               pxa->pins_fast = pinctrl_lookup_state(pxa->pinctrl, "fast");
> +               if (IS_ERR(pxa->pins_fast))
> +                       dev_info(dev, "could not get fast pinstate\n");
> +       }

This is exactly how I think it should be used from a pin control
point of view.

If you depended on CONFIG_PM you could use
pinctrl_pm_select_default_state() but for this simple scenario
this is fine.

Reviewed-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>From a pinctrl point of view.

Yours,
Linus Walleij
--
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 4/7] mmc: sdhci-pxav3: Add pinctl setting according to bus clock

[Vaibhav Hiremath]

On Tuesday 08 September 2015 08:12 PM, Linus Walleij wrote:
> On Mon, Sep 7, 2015 at 1:18 PM, Vaibhav Hiremath
> <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
>> Different bus clock may need different pin setting.
>> For example, fast bus clock like 208Mhz need pin drive fast
>> while slow bus clock prefer pin drive slow to guarantee
>> signal quality.
>>
>> So this patch creates two states,
>>    - Default (slow/normal) pin state
>>    - And fast pin state for higher freq bus speed.
>>
>> And selection of pin state is done based on timing mode.
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Signed-off-by: Kevin Liu <kliu5-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> (...)
>> +       pxa->pinctrl = devm_pinctrl_get(dev);
>> +       if (!IS_ERR(pxa->pinctrl)) {
>> +               pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default");
>> +               if (IS_ERR(pxa->pins_default))
>> +                       dev_err(dev, "could not get default pinstate\n");
>> +               pxa->pins_fast = pinctrl_lookup_state(pxa->pinctrl, "fast");
>> +               if (IS_ERR(pxa->pins_fast))
>> +                       dev_info(dev, "could not get fast pinstate\n");
>> +       }
>
> This is exactly how I think it should be used from a pin control
> point of view.
>
> If you depended on CONFIG_PM you could use
> pinctrl_pm_select_default_state() but for this simple scenario
> this is fine.
>
> Reviewed-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>  From a pinctrl point of view.
>


Thanks for your review.

Linus,

I agree this is how it should be used.
But I still have one small doubt on expectation from
devm_pinctrl_get() function.


If pinctrl properties are not populated in Devicetree node,
then, shouldn't devm_pinctrl_get() return error ?
I followed the code flow, and it seems even if pinctrl properties are
not populated in DT node, the devm_pinctrl_get() returns valid
pointer to "struct pinctrl", isn't this against the expectation of the
call?


Code flow -

devm_pinctrl_get()
...
--> creat_pinctrl()
--> pinctrl_dt_to_map()
...



pinctrl_dt_to_map() iterates for pinctrl-x (x = 0,1,...) and if it
founds the entry then it parses the node. If it doesn't find any
pinctrl property then also it returns 0. and subsequently rreturns
handle to "struct pinctrl" for the device. Why is so?


Thanks,
Vaibhav
--
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 4/7] mmc: sdhci-pxav3: Add pinctl setting according to bus clock

[Linus Walleij]

On Tue, Sep 8, 2015 at 5:07 PM, Vaibhav Hiremath
<vaibhav.hiremath@linaro.org> wrote:

> But I still have one small doubt on expectation from
> devm_pinctrl_get() function.
>
> If pinctrl properties are not populated in Devicetree node,
> then, shouldn't devm_pinctrl_get() return error ?
> I followed the code flow, and it seems even if pinctrl properties are
> not populated in DT node, the devm_pinctrl_get() returns valid
> pointer to "struct pinctrl", isn't this against the expectation of the
> call?
>
>
> Code flow -
>
> devm_pinctrl_get()
> ...
> --> creat_pinctrl()

That is the spelling mistake Dennis Ritchie and Ken Thompson
wish they had avoided in the first syscall interface :D

> --> pinctrl_dt_to_map()
> ...
>
> pinctrl_dt_to_map() iterates for pinctrl-x (x = 0,1,...) and if it
> founds the entry then it parses the node. If it doesn't find any
> pinctrl property then also it returns 0. and subsequently rreturns
> handle to "struct pinctrl" for the device. Why is so?

So pinctrl_dt_to_map() returns 0 if there are no maps in the
device tree.

This is correct: there may be systems that have a mixture
of device tree and platform data, and in that case the code
needs to continue after calling pinctrl_dt_to_map() because
else it bails out before coming to the loop in
create_pinctrl() where we iterate over the static maps.

So after the loop in create_pinctrl() it should be possible
to return something like -ENOENT if we didn't find
anything. The device core pinctrl handling in
drivers/base/pinctrl.c seems to cope with it.

It's a semantic change so we would need to test to toss
it in and see what happens on some different systems
but in principe I think you're right. What we get if there is
no state is basically a dummy pinctrl that does nothing.

Do you wanna make a patch for this?

(Looping in Stephen Warren so he can tell if I miss something
obviously evident in the design that require stubs to
be present.)

Yours,
Linus Walleij

[PATCH-v2 5/7] mmc: sdhci-pxav3: Fix HS200 mode support

[Vaibhav Hiremath]

From: Kevin Liu <kliu5@marvell.com>

IN case of MMC HS200 mode, current code does not enable
SD_CE_ATA_2.MMC_HS200 & SD_CE_ATA_2.MMC_CARD bit configurations.

So this patch updates the above bit fields correctly.

Signed-off-by: Tim Wang <wangtt@marvell.com>
Signed-off-by: Kevin Liu <kliu5@marvell.com>
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
Note: Unfortunately I do not have access to any other datasheets
which uses sdhci-pxav3 driver, so quite not sure whether this would
break any existing platform, probably NOT, as I do not see any
references for this change.
If anyone can confirm that would be really great.

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

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index d933f75..6978810 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -57,6 +57,8 @@
 #define SD_CE_ATA_1			0x10C
 
 #define SD_CE_ATA_2			0x10E
+#define  SD_CE_ATA2_HS200_EN		BIT(10)
+#define  SD_CE_ATA2_MMC_MODE		BIT(12)
 #define  SDCE_MISC_INT			BIT(2)
 #define  SDCE_MISC_INT_EN		BIT(1)
 
@@ -330,6 +332,17 @@ static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs)
 	return pinctrl_select_state(pxa->pinctrl, pinctrl);
 }
 
+static int pxav3_select_hs200(struct sdhci_host *host)
+{
+	u16 reg = 0;
+
+	reg = sdhci_readw(host, SD_CE_ATA_2);
+	reg |= SD_CE_ATA2_HS200_EN | SD_CE_ATA2_MMC_MODE;
+	sdhci_writew(host, reg, SD_CE_ATA_2);
+
+	return 0;
+}
+
 static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -361,6 +374,10 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
 	case MMC_TIMING_UHS_DDR50:
 		ctrl_2 |= SDHCI_CTRL_UHS_DDR50 | SDHCI_CTRL_VDD_180;
 		break;
+	case MMC_TIMING_MMC_HS200:
+		ctrl_2 |= SDHCI_CTRL_UHS_SDR104 | SDHCI_CTRL_VDD_180;
+		pxav3_select_hs200(host);
+		break;
 	}
 
 	/*
-- 
1.9.1

Re: [PATCH-v2 5/7] mmc: sdhci-pxav3: Fix HS200 mode support

[Jisheng Zhang]

On Mon, 7 Sep 2015 16:48:39 +0530
Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> From: Kevin Liu <kliu5-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> 
> IN case of MMC HS200 mode, current code does not enable
> SD_CE_ATA_2.MMC_HS200 & SD_CE_ATA_2.MMC_CARD bit configurations.
> 
> So this patch updates the above bit fields correctly.
> 
> Signed-off-by: Tim Wang <wangtt-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Kevin Liu <kliu5-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> Note: Unfortunately I do not have access to any other datasheets
> which uses sdhci-pxav3 driver, so quite not sure whether this would
> break any existing platform, probably NOT, as I do not see any
> references for this change.
> If anyone can confirm that would be really great.
> 
>  drivers/mmc/host/sdhci-pxav3.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> index d933f75..6978810 100644
> --- a/drivers/mmc/host/sdhci-pxav3.c
> +++ b/drivers/mmc/host/sdhci-pxav3.c
> @@ -57,6 +57,8 @@
>  #define SD_CE_ATA_1			0x10C
>  
>  #define SD_CE_ATA_2			0x10E
> +#define  SD_CE_ATA2_HS200_EN		BIT(10)
> +#define  SD_CE_ATA2_MMC_MODE		BIT(12)
>  #define  SDCE_MISC_INT			BIT(2)
>  #define  SDCE_MISC_INT_EN		BIT(1)
>  
> @@ -330,6 +332,17 @@ static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs)
>  	return pinctrl_select_state(pxa->pinctrl, pinctrl);
>  }
>  
> +static int pxav3_select_hs200(struct sdhci_host *host)

I didn't see why we need the return value, make it void would be better?

> +{
> +	u16 reg = 0;
> +
> +	reg = sdhci_readw(host, SD_CE_ATA_2);
> +	reg |= SD_CE_ATA2_HS200_EN | SD_CE_ATA2_MMC_MODE;
> +	sdhci_writew(host, reg, SD_CE_ATA_2);
> +
> +	return 0;
> +}
> +
>  static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -361,6 +374,10 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>  	case MMC_TIMING_UHS_DDR50:
>  		ctrl_2 |= SDHCI_CTRL_UHS_DDR50 | SDHCI_CTRL_VDD_180;
>  		break;
> +	case MMC_TIMING_MMC_HS200:
> +		ctrl_2 |= SDHCI_CTRL_UHS_SDR104 | SDHCI_CTRL_VDD_180;
> +		pxav3_select_hs200(host);
> +		break;
>  	}
>  
>  	/*

--
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 5/7] mmc: sdhci-pxav3: Fix HS200 mode support

[Vaibhav Hiremath]

On Tuesday 08 September 2015 12:23 PM, Jisheng Zhang wrote:
> On Mon, 7 Sep 2015 16:48:39 +0530
> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote:
>
>> From: Kevin Liu <kliu5@marvell.com>
>>
>> IN case of MMC HS200 mode, current code does not enable
>> SD_CE_ATA_2.MMC_HS200 & SD_CE_ATA_2.MMC_CARD bit configurations.
>>
>> So this patch updates the above bit fields correctly.
>>
>> Signed-off-by: Tim Wang <wangtt@marvell.com>
>> Signed-off-by: Kevin Liu <kliu5@marvell.com>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>> Note: Unfortunately I do not have access to any other datasheets
>> which uses sdhci-pxav3 driver, so quite not sure whether this would
>> break any existing platform, probably NOT, as I do not see any
>> references for this change.
>> If anyone can confirm that would be really great.
>>
>>   drivers/mmc/host/sdhci-pxav3.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
>> index d933f75..6978810 100644
>> --- a/drivers/mmc/host/sdhci-pxav3.c
>> +++ b/drivers/mmc/host/sdhci-pxav3.c
>> @@ -57,6 +57,8 @@
>>   #define SD_CE_ATA_1			0x10C
>>
>>   #define SD_CE_ATA_2			0x10E
>> +#define  SD_CE_ATA2_HS200_EN		BIT(10)
>> +#define  SD_CE_ATA2_MMC_MODE		BIT(12)
>>   #define  SDCE_MISC_INT			BIT(2)
>>   #define  SDCE_MISC_INT_EN		BIT(1)
>>
>> @@ -330,6 +332,17 @@ static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs)
>>   	return pinctrl_select_state(pxa->pinctrl, pinctrl);
>>   }
>>
>> +static int pxav3_select_hs200(struct sdhci_host *host)
>
> I didn't see why we need the return value, make it void would be better?
>

Fair enough.
Will fix it in next version.


Thanks,
Vaibhav

[PATCH-v2 6/7] mmc: sdhci: add new quirk for setting BUS_POWER & BUS_VLT fields

[Vaibhav Hiremath]

IN case of Marvell 1928 family of devices, the SD_BUS_POWER and
SD_BUS_VLT bits are used internally to gate the clocks, so
we have to set these fields.

Pasting Spec words here,

The <SD_BUS_VLT> and <SD_BUS_POWER> fields should be configured
to correct values. These actually do not do the voltage selection
or switch power to the SD card. However these fields are used
internally to gate the clock. So if these fields are set
incorrectly, SD module will not function.

And during my development, I have seen that SD card wouldn't function
without right configuration into these fields.

So this patch adds new quirk (SDHCI_QUIRK2_MUST_SET_SDHCI_BUS_POWER),
which make sure that ->set_power() sets these fields.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/mmc/host/sdhci-pltfm.c | 3 +++
 drivers/mmc/host/sdhci.c       | 3 ++-
 drivers/mmc/host/sdhci.h       | 2 ++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index a207f5a..5788a8c 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -101,6 +101,9 @@ void sdhci_get_of_property(struct platform_device *pdev)
 	    of_device_is_compatible(np, "fsl,mpc8536-esdhc"))
 		host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
 
+	if (of_device_is_compatible(np, "marvell,pxav3-1928-sdhci"))
+		host->quirks2 |= SDHCI_QUIRK2_MUST_SET_SDHCI_BUS_POWER;
+
 	clk = of_get_property(np, "clock-frequency", &size);
 	if (clk && size == sizeof(*clk) && *clk)
 		pltfm_host->clock = be32_to_cpup(clk);
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 2d58b31..418f381 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1265,7 +1265,8 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
 		else
 			sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
 
-		return;
+		if (!(host->quirks2 | SDHCI_QUIRK2_MUST_SET_SDHCI_BUS_POWER))
+			return;
 	}
 
 	if (mode != MMC_POWER_OFF) {
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 9b0e2a8..8802a0c 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -409,6 +409,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_SUPPORT_SINGLE			(1<<13)
 /* Controller broken with using ACMD23 */
 #define SDHCI_QUIRK2_ACMD23_BROKEN			(1<<14)
+/* Voltage capabilities of Controller must be set  */
+#define SDHCI_QUIRK2_MUST_SET_SDHCI_BUS_POWER		(1<<15)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
1.9.1

[PATCH-v2 7/7] mmc: sdhci: enable SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON for pxa1928

[Vaibhav Hiremath]

Card power is dependent on bus power, without that card
wouldn't respond (No CARD_INT). So this patch enables the
quirk SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/mmc/host/sdhci-pltfm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index 5788a8c..057190c 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -102,7 +102,8 @@ void sdhci_get_of_property(struct platform_device *pdev)
 		host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
 
 	if (of_device_is_compatible(np, "marvell,pxav3-1928-sdhci"))
-		host->quirks2 |= SDHCI_QUIRK2_MUST_SET_SDHCI_BUS_POWER;
+		host->quirks2 |= SDHCI_QUIRK2_MUST_SET_SDHCI_BUS_POWER |
+				SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON;
 
 	clk = of_get_property(np, "clock-frequency", &size);
 	if (clk && size == sizeof(*clk) && *clk)
-- 
1.9.1