[PATCH v3 00/13] Add Qualcomm SD Card Controller support

[PATCH v3 00/13] Add Qualcomm SD Card Controller support

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Thankyou Linus W and everyone for reviewing RFC to v3 patches.

This patch series adds Qualcomm SD Card Controller support in pl180 mmci
driver. QCom SDCC is basically a pl180, but bit more customized, some of the
register layouts and offsets are different to the ones mentioned in pl180
datasheet. The plan is to totally remove the standalone SDCC driver
drivers/mmc/host/msm_sdcc.* and start using generic mmci driver for all
Qualcomm parts, as we get chance to test on other Qcom boards.

To start using the existing mmci driver, a fake amba id for Qualcomm is added
in patches:
 mmc: mmci: Add Qualcomm Id to amba id table.

Second change is, adding a 3 clock cycle delay for register writes on QCOM SDCC
registers, which is done in patches:
  mmc: mmci: Add register read/write wrappers.
  mmc: mmci: Qcomm: Add 3 clock cycle delay after register write

Third change is to accommodate CLK, DATCTRL and MMCICLK register layout changes
in Qcom SDCC and provide more flexibity in driver to specify these changes via
variant datastructure. Which are done in patches:
  mmc: mmci: Add Qcom datactrl register variant
  mmc: mmci: add ddrmode mask to variant data
  mmc: mmci: add 8bit bus support in variant data
  mmc: mmci: add edge support to data and command out in variant data.
  mmc: mmci: add Qcom specifics of clk and datactrl registers.
  mmc: mmci: Add support to data commands via variant structure.
  mmc: mmci: add explicit clk control

Fourth major change was to add qcom specfic pio read function, the need for
this is because the way MCIFIFOCNT register behaved in QCOM SDCC is very
 different to the one in pl180. This change is done in patch:
  mmc: mmci: Add Qcom specific pio_read function.

Last some Qcom unrelated changes/cleanup to driver are done in patches:
  mmc: mmci: use NSEC_PER_SEC macro
  mmc: mmci: convert register bits to use BIT() macro.

This patches are tested in PIO mode on IFC8064 board with both eMMC and
external SD card. I would like to get this support in v3.16.

Changes from v2:
	- merged fbclk latch patch with clkreg_enable patch as suggested by Linus W.
	- remove qcom prefix for explicit clk control pointed by Linus W.
	- cleaned up mmci_qcom_pio_read and consider SDIO as suggested by Linus W.

Changes from v1:
	- moved most of the SOC specifics to variant parameters as suggested
	  by Linus W.
	- renamed registers as suggested by Linus W.
	- Added comments in the code as suggested by Linus W.
	- moved out AMBA ID addition patch from this series.
	- rebased the patches to 
		git://git.linaro.org/people/ulf.hansson/mmc.git next 
	  as suggested by Ulf H.

Changes from RFC:
	- moved out clk setup out of spinlock as pointed by Stephen B.

Am hoping to get this for v3.16.

All these patches are tested on IF6410 board on both eMMC and external SD card.

Thanks,
srini

Srinivas Kandagatla (13):
  mmc: mmci: use NSEC_PER_SEC macro
  mmc: mmci: convert register bits to use BIT() macro.
  mmc: mmci: Add Qualcomm Id to amba id table
  mmc: mmci: Add Qcom datactrl register variant
  mmc: mmci: Add register read/write wrappers.
  mmc: mmci: Qcomm: Add 3 clock cycle delay after register write
  mmc: mmci: add ddrmode mask to variant data
  mmc: mmci: add 8bit bus support in variant data
  mmc: mmci: add edge support to data and command out in variant data.
  mmc: mmci: add Qcom specifics of clk and datactrl registers.
  mmc: mmci: Add support to data commands via variant structure.
  mmc: mmci: add explicit clk control
  mmc: mmci: Add Qcom specific pio_read function.

 drivers/mmc/host/mmci.c | 252 ++++++++++++++++++++++++++++++++++++------------
 drivers/mmc/host/mmci.h | 232 ++++++++++++++++++++++++--------------------
 2 files changed, 318 insertions(+), 166 deletions(-)

-- 
1.9.1

[PATCH v3 01/13] mmc: mmci: use NSEC_PER_SEC macro

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

This patch replaces a constant used in calculating timeout with a proper
macro. This is make code more readable.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index a084edd..a38e714 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -718,7 +718,7 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 	data->bytes_xfered = 0;
 
 	clks = (unsigned long long)data->timeout_ns * host->cclk;
-	do_div(clks, 1000000000UL);
+	do_div(clks, NSEC_PER_SEC);
 
 	timeout = data->timeout_clks + (unsigned int)clks;
 
-- 
1.9.1

[PATCH v3 02/13] mmc: mmci: convert register bits to use BIT() macro.

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

This patch converts the register bits in the header file to use BIT(()
macro, which looks much neater.

No functional changes done.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/mmc/host/mmci.h | 208 ++++++++++++++++++++++++------------------------
 1 file changed, 104 insertions(+), 104 deletions(-)

diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index 347d942..cd83ca3 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -11,48 +11,48 @@
 #define MCI_PWR_OFF		0x00
 #define MCI_PWR_UP		0x02
 #define MCI_PWR_ON		0x03
-#define MCI_OD			(1 << 6)
-#define MCI_ROD			(1 << 7)
+#define MCI_OD			BIT(6)
+#define MCI_ROD			BIT(7)
 /*
  * The ST Micro version does not have ROD and reuse the voltage registers for
  * direction settings.
  */
-#define MCI_ST_DATA2DIREN	(1 << 2)
-#define MCI_ST_CMDDIREN		(1 << 3)
-#define MCI_ST_DATA0DIREN	(1 << 4)
-#define MCI_ST_DATA31DIREN	(1 << 5)
-#define MCI_ST_FBCLKEN		(1 << 7)
-#define MCI_ST_DATA74DIREN	(1 << 8)
+#define MCI_ST_DATA2DIREN	BIT(2)
+#define MCI_ST_CMDDIREN		BIT(3)
+#define MCI_ST_DATA0DIREN	BIT(4)
+#define MCI_ST_DATA31DIREN	BIT(5)
+#define MCI_ST_FBCLKEN		BIT(7)
+#define MCI_ST_DATA74DIREN	BIT(8)
 
 #define MMCICLOCK		0x004
-#define MCI_CLK_ENABLE		(1 << 8)
-#define MCI_CLK_PWRSAVE		(1 << 9)
-#define MCI_CLK_BYPASS		(1 << 10)
-#define MCI_4BIT_BUS		(1 << 11)
+#define MCI_CLK_ENABLE		BIT(8)
+#define MCI_CLK_PWRSAVE		BIT(9)
+#define MCI_CLK_BYPASS		BIT(10)
+#define MCI_4BIT_BUS		BIT(11)
 /*
  * 8bit wide buses, hardware flow contronl, negative edges and clock inversion
  * supported in ST Micro U300 and Ux500 versions
  */
-#define MCI_ST_8BIT_BUS		(1 << 12)
-#define MCI_ST_U300_HWFCEN	(1 << 13)
-#define MCI_ST_UX500_NEG_EDGE	(1 << 13)
-#define MCI_ST_UX500_HWFCEN	(1 << 14)
-#define MCI_ST_UX500_CLK_INV	(1 << 15)
+#define MCI_ST_8BIT_BUS		BIT(12)
+#define MCI_ST_U300_HWFCEN	BIT(13)
+#define MCI_ST_UX500_NEG_EDGE	BIT(13)
+#define MCI_ST_UX500_HWFCEN	BIT(14)
+#define MCI_ST_UX500_CLK_INV	BIT(15)
 /* Modified PL180 on Versatile Express platform */
-#define MCI_ARM_HWFCEN		(1 << 12)
+#define MCI_ARM_HWFCEN		BIT(12)
 
 #define MMCIARGUMENT		0x008
 #define MMCICOMMAND		0x00c
-#define MCI_CPSM_RESPONSE	(1 << 6)
-#define MCI_CPSM_LONGRSP	(1 << 7)
-#define MCI_CPSM_INTERRUPT	(1 << 8)
-#define MCI_CPSM_PENDING	(1 << 9)
-#define MCI_CPSM_ENABLE		(1 << 10)
+#define MCI_CPSM_RESPONSE	BIT(6)
+#define MCI_CPSM_LONGRSP	BIT(7)
+#define MCI_CPSM_INTERRUPT	BIT(8)
+#define MCI_CPSM_PENDING	BIT(9)
+#define MCI_CPSM_ENABLE		BIT(10)
 /* Argument flag extenstions in the ST Micro versions */
-#define MCI_ST_SDIO_SUSP	(1 << 11)
-#define MCI_ST_ENCMD_COMPL	(1 << 12)
-#define MCI_ST_NIEN		(1 << 13)
-#define MCI_ST_CE_ATACMD	(1 << 14)
+#define MCI_ST_SDIO_SUSP	BIT(11)
+#define MCI_ST_ENCMD_COMPL	BIT(12)
+#define MCI_ST_NIEN		BIT(13)
+#define MCI_ST_CE_ATACMD	BIT(14)
 
 #define MMCIRESPCMD		0x010
 #define MMCIRESPONSE0		0x014
@@ -62,95 +62,95 @@
 #define MMCIDATATIMER		0x024
 #define MMCIDATALENGTH		0x028
 #define MMCIDATACTRL		0x02c
-#define MCI_DPSM_ENABLE		(1 << 0)
-#define MCI_DPSM_DIRECTION	(1 << 1)
-#define MCI_DPSM_MODE		(1 << 2)
-#define MCI_DPSM_DMAENABLE	(1 << 3)
-#define MCI_DPSM_BLOCKSIZE	(1 << 4)
+#define MCI_DPSM_ENABLE		BIT(0)
+#define MCI_DPSM_DIRECTION	BIT(1)
+#define MCI_DPSM_MODE		BIT(2)
+#define MCI_DPSM_DMAENABLE	BIT(3)
+#define MCI_DPSM_BLOCKSIZE	BIT(4)
 /* Control register extensions in the ST Micro U300 and Ux500 versions */
-#define MCI_ST_DPSM_RWSTART	(1 << 8)
-#define MCI_ST_DPSM_RWSTOP	(1 << 9)
-#define MCI_ST_DPSM_RWMOD	(1 << 10)
-#define MCI_ST_DPSM_SDIOEN	(1 << 11)
+#define MCI_ST_DPSM_RWSTART	BIT(8)
+#define MCI_ST_DPSM_RWSTOP	BIT(9)
+#define MCI_ST_DPSM_RWMOD	BIT(10)
+#define MCI_ST_DPSM_SDIOEN	BIT(11)
 /* Control register extensions in the ST Micro Ux500 versions */
-#define MCI_ST_DPSM_DMAREQCTL	(1 << 12)
-#define MCI_ST_DPSM_DBOOTMODEEN	(1 << 13)
-#define MCI_ST_DPSM_BUSYMODE	(1 << 14)
-#define MCI_ST_DPSM_DDRMODE	(1 << 15)
+#define MCI_ST_DPSM_DMAREQCTL	BIT(12)
+#define MCI_ST_DPSM_DBOOTMODEEN	BIT(13)
+#define MCI_ST_DPSM_BUSYMODE	BIT(14)
+#define MCI_ST_DPSM_DDRMODE	BIT(15)
 
 #define MMCIDATACNT		0x030
 #define MMCISTATUS		0x034
-#define MCI_CMDCRCFAIL		(1 << 0)
-#define MCI_DATACRCFAIL		(1 << 1)
-#define MCI_CMDTIMEOUT		(1 << 2)
-#define MCI_DATATIMEOUT		(1 << 3)
-#define MCI_TXUNDERRUN		(1 << 4)
-#define MCI_RXOVERRUN		(1 << 5)
-#define MCI_CMDRESPEND		(1 << 6)
-#define MCI_CMDSENT		(1 << 7)
-#define MCI_DATAEND		(1 << 8)
-#define MCI_STARTBITERR		(1 << 9)
-#define MCI_DATABLOCKEND	(1 << 10)
-#define MCI_CMDACTIVE		(1 << 11)
-#define MCI_TXACTIVE		(1 << 12)
-#define MCI_RXACTIVE		(1 << 13)
-#define MCI_TXFIFOHALFEMPTY	(1 << 14)
-#define MCI_RXFIFOHALFFULL	(1 << 15)
-#define MCI_TXFIFOFULL		(1 << 16)
-#define MCI_RXFIFOFULL		(1 << 17)
-#define MCI_TXFIFOEMPTY		(1 << 18)
-#define MCI_RXFIFOEMPTY		(1 << 19)
-#define MCI_TXDATAAVLBL		(1 << 20)
-#define MCI_RXDATAAVLBL		(1 << 21)
+#define MCI_CMDCRCFAIL		BIT(0)
+#define MCI_DATACRCFAIL		BIT(1)
+#define MCI_CMDTIMEOUT		BIT(2)
+#define MCI_DATATIMEOUT		BIT(3)
+#define MCI_TXUNDERRUN		BIT(4)
+#define MCI_RXOVERRUN		BIT(5)
+#define MCI_CMDRESPEND		BIT(6)
+#define MCI_CMDSENT		BIT(7)
+#define MCI_DATAEND		BIT(8)
+#define MCI_STARTBITERR		BIT(9)
+#define MCI_DATABLOCKEND	BIT(10)
+#define MCI_CMDACTIVE		BIT(11)
+#define MCI_TXACTIVE		BIT(12)
+#define MCI_RXACTIVE		BIT(13)
+#define MCI_TXFIFOHALFEMPTY	BIT(14)
+#define MCI_RXFIFOHALFFULL	BIT(15)
+#define MCI_TXFIFOFULL		BIT(16)
+#define MCI_RXFIFOFULL		BIT(17)
+#define MCI_TXFIFOEMPTY		BIT(18)
+#define MCI_RXFIFOEMPTY		BIT(19)
+#define MCI_TXDATAAVLBL		BIT(20)
+#define MCI_RXDATAAVLBL		BIT(21)
 /* Extended status bits for the ST Micro variants */
-#define MCI_ST_SDIOIT		(1 << 22)
-#define MCI_ST_CEATAEND		(1 << 23)
-#define MCI_ST_CARDBUSY		(1 << 24)
+#define MCI_ST_SDIOIT		BIT(22)
+#define MCI_ST_CEATAEND		BIT(23)
+#define MCI_ST_CARDBUSY		BIT(24)
 
 #define MMCICLEAR		0x038
-#define MCI_CMDCRCFAILCLR	(1 << 0)
-#define MCI_DATACRCFAILCLR	(1 << 1)
-#define MCI_CMDTIMEOUTCLR	(1 << 2)
-#define MCI_DATATIMEOUTCLR	(1 << 3)
-#define MCI_TXUNDERRUNCLR	(1 << 4)
-#define MCI_RXOVERRUNCLR	(1 << 5)
-#define MCI_CMDRESPENDCLR	(1 << 6)
-#define MCI_CMDSENTCLR		(1 << 7)
-#define MCI_DATAENDCLR		(1 << 8)
-#define MCI_STARTBITERRCLR	(1 << 9)
-#define MCI_DATABLOCKENDCLR	(1 << 10)
+#define MCI_CMDCRCFAILCLR	BIT(0)
+#define MCI_DATACRCFAILCLR	BIT(1)
+#define MCI_CMDTIMEOUTCLR	BIT(2)
+#define MCI_DATATIMEOUTCLR	BIT(3)
+#define MCI_TXUNDERRUNCLR	BIT(4)
+#define MCI_RXOVERRUNCLR	BIT(5)
+#define MCI_CMDRESPENDCLR	BIT(6)
+#define MCI_CMDSENTCLR		BIT(7)
+#define MCI_DATAENDCLR		BIT(8)
+#define MCI_STARTBITERRCLR	BIT(9)
+#define MCI_DATABLOCKENDCLR	BIT(10)
 /* Extended status bits for the ST Micro variants */
-#define MCI_ST_SDIOITC		(1 << 22)
-#define MCI_ST_CEATAENDC	(1 << 23)
-#define MCI_ST_BUSYENDC		(1 << 24)
+#define MCI_ST_SDIOITC		BIT(22)
+#define MCI_ST_CEATAENDC	BIT(23)
+#define MCI_ST_BUSYENDC		BIT(24)
 
 #define MMCIMASK0		0x03c
-#define MCI_CMDCRCFAILMASK	(1 << 0)
-#define MCI_DATACRCFAILMASK	(1 << 1)
-#define MCI_CMDTIMEOUTMASK	(1 << 2)
-#define MCI_DATATIMEOUTMASK	(1 << 3)
-#define MCI_TXUNDERRUNMASK	(1 << 4)
-#define MCI_RXOVERRUNMASK	(1 << 5)
-#define MCI_CMDRESPENDMASK	(1 << 6)
-#define MCI_CMDSENTMASK		(1 << 7)
-#define MCI_DATAENDMASK		(1 << 8)
-#define MCI_STARTBITERRMASK	(1 << 9)
-#define MCI_DATABLOCKENDMASK	(1 << 10)
-#define MCI_CMDACTIVEMASK	(1 << 11)
-#define MCI_TXACTIVEMASK	(1 << 12)
-#define MCI_RXACTIVEMASK	(1 << 13)
-#define MCI_TXFIFOHALFEMPTYMASK	(1 << 14)
-#define MCI_RXFIFOHALFFULLMASK	(1 << 15)
-#define MCI_TXFIFOFULLMASK	(1 << 16)
-#define MCI_RXFIFOFULLMASK	(1 << 17)
-#define MCI_TXFIFOEMPTYMASK	(1 << 18)
-#define MCI_RXFIFOEMPTYMASK	(1 << 19)
-#define MCI_TXDATAAVLBLMASK	(1 << 20)
-#define MCI_RXDATAAVLBLMASK	(1 << 21)
+#define MCI_CMDCRCFAILMASK	BIT(0)
+#define MCI_DATACRCFAILMASK	BIT(1)
+#define MCI_CMDTIMEOUTMASK	BIT(2)
+#define MCI_DATATIMEOUTMASK	BIT(3)
+#define MCI_TXUNDERRUNMASK	BIT(4)
+#define MCI_RXOVERRUNMASK	BIT(5)
+#define MCI_CMDRESPENDMASK	BIT(6)
+#define MCI_CMDSENTMASK		BIT(7)
+#define MCI_DATAENDMASK		BIT(8)
+#define MCI_STARTBITERRMASK	BIT(9)
+#define MCI_DATABLOCKENDMASK	BIT(10)
+#define MCI_CMDACTIVEMASK	BIT(11)
+#define MCI_TXACTIVEMASK	BIT(12)
+#define MCI_RXACTIVEMASK	BIT(13)
+#define MCI_TXFIFOHALFEMPTYMASK	BIT(14)
+#define MCI_RXFIFOHALFFULLMASK	BIT(15)
+#define MCI_TXFIFOFULLMASK	BIT(16)
+#define MCI_RXFIFOFULLMASK	BIT(17)
+#define MCI_TXFIFOEMPTYMASK	BIT(18)
+#define MCI_RXFIFOEMPTYMASK	BIT(19)
+#define MCI_TXDATAAVLBLMASK	BIT(20)
+#define MCI_RXDATAAVLBLMASK	BIT(21)
 /* Extended status bits for the ST Micro variants */
-#define MCI_ST_SDIOITMASK	(1 << 22)
-#define MCI_ST_CEATAENDMASK	(1 << 23)
-#define MCI_ST_BUSYEND		(1 << 24)
+#define MCI_ST_SDIOITMASK	BIT(22)
+#define MCI_ST_CEATAENDMASK	BIT(23)
+#define MCI_ST_BUSYEND		BIT(24)
 
 #define MMCIMASK1		0x040
 #define MMCIFIFOCNT		0x048
-- 
1.9.1

[PATCH v3 03/13] mmc: mmci: Add Qualcomm Id to amba id table

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

This patch adds a fake Qualcomm ID 0x00051180 to the amba_ids, as Qualcomm
SDCC controller is pl180, but amba id registers read 0x0's.
The plan is to remove SDCC driver totally and use mmci as the main SD
controller driver for Qualcomm SOCs.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index a38e714..7bdf4d3 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -160,6 +160,15 @@ static struct variant_data variant_ux500v2 = {
 	.pwrreg_nopower		= true,
 };
 
+static struct variant_data variant_qcom = {
+	.fifosize		= 16 * 4,
+	.fifohalfsize		= 8 * 4,
+	.clkreg			= MCI_CLK_ENABLE,
+	.datalength_bits	= 24,
+	.blksz_datactrl4	= true,
+	.pwrreg_powerup		= MCI_PWR_UP,
+};
+
 static int mmci_card_busy(struct mmc_host *mmc)
 {
 	struct mmci_host *host = mmc_priv(mmc);
@@ -1750,6 +1759,12 @@ static struct amba_id mmci_ids[] = {
 		.mask   = 0xf0ffffff,
 		.data	= &variant_ux500v2,
 	},
+	/* Qualcomm variants */
+	{
+		.id     = 0x00051180,
+		.mask	= 0x000fffff,
+		.data	= &variant_qcom,
+	},
 	{ 0, 0 },
 };
 
-- 
1.9.1

[PATCH v3 04/13] mmc: mmci: Add Qcom datactrl register variant

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Instance of this IP on Qualcomm's SOCs has bit different layout for datactrl
register. Bit position datactrl[16:4] hold the true block size instead of power
of 2.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 7bdf4d3..324a886 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -60,6 +60,8 @@ static unsigned int fmax = 515633;
  * @sdio: variant supports SDIO
  * @st_clkdiv: true if using a ST-specific clock divider algorithm
  * @blksz_datactrl16: true if Block size is at b16..b30 position in datactrl register
+ * @blksz_datactrl4: true if Block size is at b4..b16 position in datactrl
+ *		     register
  * @pwrreg_powerup: power up value for MMCIPOWER register
  * @signal_direction: input/out direction of bus signals can be indicated
  * @pwrreg_clkgate: MMCIPOWER register must be used to gate the clock
@@ -75,6 +77,7 @@ struct variant_data {
 	bool			sdio;
 	bool			st_clkdiv;
 	bool			blksz_datactrl16;
+	bool			blksz_datactrl4;
 	u32			pwrreg_powerup;
 	bool			signal_direction;
 	bool			pwrreg_clkgate;
@@ -164,6 +167,7 @@ static struct variant_data variant_qcom = {
 	.fifosize		= 16 * 4,
 	.fifohalfsize		= 8 * 4,
 	.clkreg			= MCI_CLK_ENABLE,
+	.blksz_datactrl4	= true,
 	.datalength_bits	= 24,
 	.blksz_datactrl4	= true,
 	.pwrreg_powerup		= MCI_PWR_UP,
@@ -740,6 +744,8 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 
 	if (variant->blksz_datactrl16)
 		datactrl = MCI_DPSM_ENABLE | (data->blksz << 16);
+	else if (variant->blksz_datactrl4)
+		datactrl = MCI_DPSM_ENABLE | (data->blksz << 4);
 	else
 		datactrl = MCI_DPSM_ENABLE | blksz_bits << 4;
 
-- 
1.9.1

[PATCH v3 05/13] mmc: mmci: Add register read/write wrappers.

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

This patch adds wrappers for readl/writel functions used in the driver. The
reason for this wrappers is to accommodate SOCs like Qualcomm which has
requirement for delaying the write for few cycles when writing to its SD Card
Controller registers.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c | 110 ++++++++++++++++++++++++++----------------------
 1 file changed, 59 insertions(+), 51 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 324a886..881bb24 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -173,6 +173,16 @@ static struct variant_data variant_qcom = {
 	.pwrreg_powerup		= MCI_PWR_UP,
 };
 
+static inline u32 mmci_readl(struct mmci_host *host, u32 off)
+{
+	return readl(host->base  + off);
+}
+
+static inline void mmci_writel(struct mmci_host *host, u32 data, u32 off)
+{
+	writel(data, host->base + off);
+}
+
 static int mmci_card_busy(struct mmc_host *mmc)
 {
 	struct mmci_host *host = mmc_priv(mmc);
@@ -182,7 +192,7 @@ static int mmci_card_busy(struct mmc_host *mmc)
 	pm_runtime_get_sync(mmc_dev(mmc));
 
 	spin_lock_irqsave(&host->lock, flags);
-	if (readl(host->base + MMCISTATUS) & MCI_ST_CARDBUSY)
+	if (mmci_readl(host, MMCISTATUS) & MCI_ST_CARDBUSY)
 		busy = 1;
 	spin_unlock_irqrestore(&host->lock, flags);
 
@@ -232,7 +242,7 @@ static void mmci_write_clkreg(struct mmci_host *host, u32 clk)
 {
 	if (host->clk_reg != clk) {
 		host->clk_reg = clk;
-		writel(clk, host->base + MMCICLOCK);
+		mmci_writel(host, clk, MMCICLOCK);
 	}
 }
 
@@ -243,7 +253,7 @@ static void mmci_write_pwrreg(struct mmci_host *host, u32 pwr)
 {
 	if (host->pwr_reg != pwr) {
 		host->pwr_reg = pwr;
-		writel(pwr, host->base + MMCIPOWER);
+		mmci_writel(host, pwr, MMCIPOWER);
 	}
 }
 
@@ -257,7 +267,7 @@ static void mmci_write_datactrlreg(struct mmci_host *host, u32 datactrl)
 
 	if (host->datactrl_reg != datactrl) {
 		host->datactrl_reg = datactrl;
-		writel(datactrl, host->base + MMCIDATACTRL);
+		mmci_writel(host, datactrl, MMCIDATACTRL);
 	}
 }
 
@@ -323,7 +333,7 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)
 static void
 mmci_request_end(struct mmci_host *host, struct mmc_request *mrq)
 {
-	writel(0, host->base + MMCICOMMAND);
+	mmci_writel(host, 0, MMCICOMMAND);
 
 	BUG_ON(host->data);
 
@@ -338,18 +348,16 @@ mmci_request_end(struct mmci_host *host, struct mmc_request *mrq)
 
 static void mmci_set_mask1(struct mmci_host *host, unsigned int mask)
 {
-	void __iomem *base = host->base;
-
 	if (host->singleirq) {
-		unsigned int mask0 = readl(base + MMCIMASK0);
+		unsigned int mask0 = mmci_readl(host, MMCIMASK0);
 
 		mask0 &= ~MCI_IRQ1MASK;
 		mask0 |= mask;
 
-		writel(mask0, base + MMCIMASK0);
+		mmci_writel(host, mask0, MMCIMASK0);
 	}
 
-	writel(mask, base + MMCIMASK1);
+	mmci_writel(host, mask, MMCIMASK1);
 }
 
 static void mmci_stop_data(struct mmci_host *host)
@@ -478,7 +486,7 @@ static void mmci_dma_finalize(struct mmci_host *host, struct mmc_data *data)
 
 	/* Wait up to 1ms for the DMA to complete */
 	for (i = 0; ; i++) {
-		status = readl(host->base + MMCISTATUS);
+		status = mmci_readl(host, MMCISTATUS);
 		if (!(status & MCI_RXDATAAVLBLMASK) || i >= 100)
 			break;
 		udelay(10);
@@ -617,8 +625,8 @@ static int mmci_dma_start_data(struct mmci_host *host, unsigned int datactrl)
 	 * to fire next DMA request. When that happens, MMCI will
 	 * call mmci_data_end()
 	 */
-	writel(readl(host->base + MMCIMASK0) | MCI_DATAENDMASK,
-	       host->base + MMCIMASK0);
+	mmci_writel(host, mmci_readl(host, MMCIMASK0) | MCI_DATAENDMASK,
+		    MMCIMASK0);
 	return 0;
 }
 
@@ -736,8 +744,8 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 	timeout = data->timeout_clks + (unsigned int)clks;
 
 	base = host->base;
-	writel(timeout, base + MMCIDATATIMER);
-	writel(host->size, base + MMCIDATALENGTH);
+	mmci_writel(host, timeout, MMCIDATATIMER);
+	mmci_writel(host, host->size, MMCIDATALENGTH);
 
 	blksz_bits = ffs(data->blksz) - 1;
 	BUG_ON(1 << blksz_bits != data->blksz);
@@ -811,20 +819,19 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 	}
 
 	mmci_write_datactrlreg(host, datactrl);
-	writel(readl(base + MMCIMASK0) & ~MCI_DATAENDMASK, base + MMCIMASK0);
+	mmci_writel(host, mmci_readl(host, MMCIMASK0) & ~MCI_DATAENDMASK,
+		    MMCIMASK0);
 	mmci_set_mask1(host, irqmask);
 }
 
 static void
 mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c)
 {
-	void __iomem *base = host->base;
-
 	dev_dbg(mmc_dev(host->mmc), "op %02x arg %08x flags %08x\n",
 	    cmd->opcode, cmd->arg, cmd->flags);
 
-	if (readl(base + MMCICOMMAND) & MCI_CPSM_ENABLE) {
-		writel(0, base + MMCICOMMAND);
+	if (mmci_readl(host, MMCICOMMAND) & MCI_CPSM_ENABLE) {
+		mmci_writel(host, 0, MMCICOMMAND);
 		udelay(1);
 	}
 
@@ -839,8 +846,8 @@ mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c)
 
 	host->cmd = cmd;
 
-	writel(cmd->arg, base + MMCIARGUMENT);
-	writel(c, base + MMCICOMMAND);
+	mmci_writel(host, cmd->arg, MMCIARGUMENT);
+	mmci_writel(host, c, MMCICOMMAND);
 }
 
 static void
@@ -865,7 +872,7 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 		 * can be as much as a FIFO-worth of data ahead.  This
 		 * matters for FIFO overruns only.
 		 */
-		remain = readl(host->base + MMCIDATACNT);
+		remain = mmci_readl(host, MMCIDATACNT);
 		success = data->blksz * data->blocks - remain;
 
 		dev_dbg(mmc_dev(host->mmc), "MCI ERROR IRQ, status 0x%08x at 0x%08x\n",
@@ -947,10 +954,10 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
 	} else if (status & MCI_CMDCRCFAIL && cmd->flags & MMC_RSP_CRC) {
 		cmd->error = -EILSEQ;
 	} else {
-		cmd->resp[0] = readl(base + MMCIRESPONSE0);
-		cmd->resp[1] = readl(base + MMCIRESPONSE1);
-		cmd->resp[2] = readl(base + MMCIRESPONSE2);
-		cmd->resp[3] = readl(base + MMCIRESPONSE3);
+		cmd->resp[0] = mmci_readl(host, MMCIRESPONSE0);
+		cmd->resp[1] = mmci_readl(host, MMCIRESPONSE1);
+		cmd->resp[2] = mmci_readl(host, MMCIRESPONSE2);
+		cmd->resp[3] = mmci_readl(host, MMCIRESPONSE3);
 	}
 
 	if ((!sbc && !cmd->data) || cmd->error) {
@@ -1061,11 +1068,10 @@ static irqreturn_t mmci_pio_irq(int irq, void *dev_id)
 	struct mmci_host *host = dev_id;
 	struct sg_mapping_iter *sg_miter = &host->sg_miter;
 	struct variant_data *variant = host->variant;
-	void __iomem *base = host->base;
 	unsigned long flags;
 	u32 status;
 
-	status = readl(base + MMCISTATUS);
+	status = mmci_readl(host, MMCISTATUS);
 
 	dev_dbg(mmc_dev(host->mmc), "irq1 (pio) %08x\n", status);
 
@@ -1105,7 +1111,7 @@ static irqreturn_t mmci_pio_irq(int irq, void *dev_id)
 		if (remain)
 			break;
 
-		status = readl(base + MMCISTATUS);
+		status = mmci_readl(host, MMCISTATUS);
 	} while (1);
 
 	sg_miter_stop(sg_miter);
@@ -1127,7 +1133,9 @@ static irqreturn_t mmci_pio_irq(int irq, void *dev_id)
 	 */
 	if (host->size == 0) {
 		mmci_set_mask1(host, 0);
-		writel(readl(base + MMCIMASK0) | MCI_DATAENDMASK, base + MMCIMASK0);
+		mmci_writel(host,
+			    mmci_readl(host, MMCIMASK0) | MCI_DATAENDMASK,
+			    MMCIMASK0);
 	}
 
 	return IRQ_HANDLED;
@@ -1148,10 +1156,10 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
 		struct mmc_command *cmd;
 		struct mmc_data *data;
 
-		status = readl(host->base + MMCISTATUS);
+		status = mmci_readl(host, MMCISTATUS);
 
 		if (host->singleirq) {
-			if (status & readl(host->base + MMCIMASK1))
+			if (status & mmci_readl(host, MMCIMASK1))
 				mmci_pio_irq(irq, dev_id);
 
 			status &= ~MCI_IRQ1MASK;
@@ -1162,8 +1170,8 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
 		 * enabled) since the HW seems to be triggering the IRQ on both
 		 * edges while monitoring DAT0 for busy completion.
 		 */
-		status &= readl(host->base + MMCIMASK0);
-		writel(status, host->base + MMCICLEAR);
+		status &= mmci_readl(host, MMCIMASK0);
+		mmci_writel(host, status, MMCICLEAR);
 
 		dev_dbg(mmc_dev(host->mmc), "irq0 (data+cmd) %08x\n", status);
 
@@ -1561,9 +1569,9 @@ static int mmci_probe(struct amba_device *dev,
 
 	spin_lock_init(&host->lock);
 
-	writel(0, host->base + MMCIMASK0);
-	writel(0, host->base + MMCIMASK1);
-	writel(0xfff, host->base + MMCICLEAR);
+	mmci_writel(host, 0, MMCIMASK0);
+	mmci_writel(host, 0, MMCIMASK1);
+	mmci_writel(host, 0xfff, MMCICLEAR);
 
 	/* If DT, cd/wp gpios must be supplied through it. */
 	if (!np && gpio_is_valid(plat->gpio_cd)) {
@@ -1591,7 +1599,7 @@ static int mmci_probe(struct amba_device *dev,
 			goto clk_disable;
 	}
 
-	writel(MCI_IRQENABLE, host->base + MMCIMASK0);
+	mmci_writel(host, MCI_IRQENABLE, MMCIMASK0);
 
 	amba_set_drvdata(dev, mmc);
 
@@ -1632,11 +1640,11 @@ static int mmci_remove(struct amba_device *dev)
 
 		mmc_remove_host(mmc);
 
-		writel(0, host->base + MMCIMASK0);
-		writel(0, host->base + MMCIMASK1);
+		mmci_writel(host, 0, MMCIMASK0);
+		mmci_writel(host, 0, MMCIMASK1);
 
-		writel(0, host->base + MMCICOMMAND);
-		writel(0, host->base + MMCIDATACTRL);
+		mmci_writel(host, 0, MMCICOMMAND);
+		mmci_writel(host, 0, MMCIDATACTRL);
 
 		mmci_dma_release(host);
 		clk_disable_unprepare(host->clk);
@@ -1653,11 +1661,11 @@ static void mmci_save(struct mmci_host *host)
 
 	spin_lock_irqsave(&host->lock, flags);
 
-	writel(0, host->base + MMCIMASK0);
+	mmci_writel(host, 0, MMCIMASK0);
 	if (host->variant->pwrreg_nopower) {
-		writel(0, host->base + MMCIDATACTRL);
-		writel(0, host->base + MMCIPOWER);
-		writel(0, host->base + MMCICLOCK);
+		mmci_writel(host, 0, MMCIDATACTRL);
+		mmci_writel(host, 0, MMCIPOWER);
+		mmci_writel(host, 0, MMCICLOCK);
 	}
 	mmci_reg_delay(host);
 
@@ -1671,11 +1679,11 @@ static void mmci_restore(struct mmci_host *host)
 	spin_lock_irqsave(&host->lock, flags);
 
 	if (host->variant->pwrreg_nopower) {
-		writel(host->clk_reg, host->base + MMCICLOCK);
-		writel(host->datactrl_reg, host->base + MMCIDATACTRL);
-		writel(host->pwr_reg, host->base + MMCIPOWER);
+		mmci_writel(host, host->clk_reg, MMCICLOCK);
+		mmci_writel(host, host->datactrl_reg, MMCIDATACTRL);
+		mmci_writel(host, host->pwr_reg, MMCIPOWER);
 	}
-	writel(MCI_IRQENABLE, host->base + MMCIMASK0);
+	mmci_writel(host, MCI_IRQENABLE, MMCIMASK0);
 	mmci_reg_delay(host);
 
 	spin_unlock_irqrestore(&host->lock, flags);
-- 
1.9.1

[PATCH v3 06/13] mmc: mmci: Qcomm: Add 3 clock cycle delay after register write

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Most of the Qcomm SD card controller registers must be updated to the MCLK
domain so subsequent writes to registers will be ignored until 3 clock cycles
have passed.

This patch adds a 3 clock cycle delay required after writing to controller
registers on Qualcomm SOCs. Without this delay all the register writes are not
successful, resulting in not detecting cards. The write clock delay is
activated by setting up mclk_delayed_writes variable in variant data.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 881bb24..1385554 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -67,6 +67,8 @@ static unsigned int fmax = 515633;
  * @pwrreg_clkgate: MMCIPOWER register must be used to gate the clock
  * @busy_detect: true if busy detection on dat0 is supported
  * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply
+ * @mclk_delayed_writes: enable delayed writes to ensure, subsequent updates
+ *			 are not ignored.
  */
 struct variant_data {
 	unsigned int		clkreg;
@@ -83,6 +85,7 @@ struct variant_data {
 	bool			pwrreg_clkgate;
 	bool			busy_detect;
 	bool			pwrreg_nopower;
+	bool			mclk_delayed_writes;
 };
 
 static struct variant_data variant_arm = {
@@ -171,6 +174,12 @@ static struct variant_data variant_qcom = {
 	.datalength_bits	= 24,
 	.blksz_datactrl4	= true,
 	.pwrreg_powerup		= MCI_PWR_UP,
+	/*
+	 * On QCom SD card controller, registers must be updated to the
+	 * MCLK domain so subsequent writes to this register will be ignored
+	 * for 3 clk cycles.
+	 */
+	.mclk_delayed_writes	= true,
 };
 
 static inline u32 mmci_readl(struct mmci_host *host, u32 off)
@@ -181,6 +190,9 @@ static inline u32 mmci_readl(struct mmci_host *host, u32 off)
 static inline void mmci_writel(struct mmci_host *host, u32 data, u32 off)
 {
 	writel(data, host->base + off);
+
+	if (host->variant->mclk_delayed_writes)
+		udelay(DIV_ROUND_UP((3 * USEC_PER_SEC), host->mclk));
 }
 
 static int mmci_card_busy(struct mmc_host *mmc)
-- 
1.9.1

[PATCH v3 07/13] mmc: mmci: add ddrmode mask to variant data

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

This patch adds ddrmode mask to variant structure giving more flexibility
to the driver to support more SOCs which have different datactrl register
layout.

Without this patch datactrl register is updated with wrong ddrmode mask on non
ST SOCs, resulting in card detection failures.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 1385554..dec70d2 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -59,6 +59,7 @@ static unsigned int fmax = 515633;
  *		  is asserted (likewise for RX)
  * @sdio: variant supports SDIO
  * @st_clkdiv: true if using a ST-specific clock divider algorithm
+ * @datactrl_mask_ddrmode: ddr mode mask in datactrl register.
  * @blksz_datactrl16: true if Block size is at b16..b30 position in datactrl register
  * @blksz_datactrl4: true if Block size is at b4..b16 position in datactrl
  *		     register
@@ -76,6 +77,7 @@ struct variant_data {
 	unsigned int		datalength_bits;
 	unsigned int		fifosize;
 	unsigned int		fifohalfsize;
+	unsigned int		datactrl_mask_ddrmode;
 	bool			sdio;
 	bool			st_clkdiv;
 	bool			blksz_datactrl16;
@@ -114,6 +116,7 @@ static struct variant_data variant_u300 = {
 	.fifosize		= 16 * 4,
 	.fifohalfsize		= 8 * 4,
 	.clkreg_enable		= MCI_ST_U300_HWFCEN,
+	.datactrl_mask_ddrmode	= MCI_ST_DPSM_DDRMODE,
 	.datalength_bits	= 16,
 	.sdio			= true,
 	.pwrreg_powerup		= MCI_PWR_ON,
@@ -126,6 +129,7 @@ static struct variant_data variant_nomadik = {
 	.fifosize		= 16 * 4,
 	.fifohalfsize		= 8 * 4,
 	.clkreg			= MCI_CLK_ENABLE,
+	.datactrl_mask_ddrmode	= MCI_ST_DPSM_DDRMODE,
 	.datalength_bits	= 24,
 	.sdio			= true,
 	.st_clkdiv		= true,
@@ -140,6 +144,7 @@ static struct variant_data variant_ux500 = {
 	.fifohalfsize		= 8 * 4,
 	.clkreg			= MCI_CLK_ENABLE,
 	.clkreg_enable		= MCI_ST_UX500_HWFCEN,
+	.datactrl_mask_ddrmode	= MCI_ST_DPSM_DDRMODE,
 	.datalength_bits	= 24,
 	.sdio			= true,
 	.st_clkdiv		= true,
@@ -155,6 +160,7 @@ static struct variant_data variant_ux500v2 = {
 	.fifohalfsize		= 8 * 4,
 	.clkreg			= MCI_CLK_ENABLE,
 	.clkreg_enable		= MCI_ST_UX500_HWFCEN,
+	.datactrl_mask_ddrmode	= MCI_ST_DPSM_DDRMODE,
 	.datalength_bits	= 24,
 	.sdio			= true,
 	.st_clkdiv		= true,
@@ -800,7 +806,7 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 		}
 
 	if (host->mmc->ios.timing == MMC_TIMING_UHS_DDR50)
-		datactrl |= MCI_ST_DPSM_DDRMODE;
+		datactrl |= variant->datactrl_mask_ddrmode;
 
 	/*
 	 * Attempt to use DMA operation mode, if this
-- 
1.9.1

[PATCH v3 08/13] mmc: mmci: add 8bit bus support in variant data

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

This patch adds 8bit bus enable to variant structure giving more flexibility
to the driver to support more SOCs which have different clock register layout.

Without this patch other new SOCs like Qcom will have to add more code
to special case them.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index dec70d2..a81f303 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -52,6 +52,7 @@ static unsigned int fmax = 515633;
  * struct variant_data - MMCI variant-specific quirks
  * @clkreg: default value for MCICLOCK register
  * @clkreg_enable: enable value for MMCICLOCK register
+ * @clkreg_8bit_bus_enable: enable value for 8 bit bus
  * @datalength_bits: number of bits in the MMCIDATALENGTH register
  * @fifosize: number of bytes that can be written when MMCI_TXFIFOEMPTY
  *	      is asserted (likewise for RX)
@@ -74,6 +75,7 @@ static unsigned int fmax = 515633;
 struct variant_data {
 	unsigned int		clkreg;
 	unsigned int		clkreg_enable;
+	unsigned int		clkreg_8bit_bus_enable;
 	unsigned int		datalength_bits;
 	unsigned int		fifosize;
 	unsigned int		fifohalfsize;
@@ -116,6 +118,7 @@ static struct variant_data variant_u300 = {
 	.fifosize		= 16 * 4,
 	.fifohalfsize		= 8 * 4,
 	.clkreg_enable		= MCI_ST_U300_HWFCEN,
+	.clkreg_8bit_bus_enable = MCI_ST_8BIT_BUS,
 	.datactrl_mask_ddrmode	= MCI_ST_DPSM_DDRMODE,
 	.datalength_bits	= 16,
 	.sdio			= true,
@@ -144,6 +147,7 @@ static struct variant_data variant_ux500 = {
 	.fifohalfsize		= 8 * 4,
 	.clkreg			= MCI_CLK_ENABLE,
 	.clkreg_enable		= MCI_ST_UX500_HWFCEN,
+	.clkreg_8bit_bus_enable = MCI_ST_8BIT_BUS,
 	.datactrl_mask_ddrmode	= MCI_ST_DPSM_DDRMODE,
 	.datalength_bits	= 24,
 	.sdio			= true,
@@ -160,6 +164,7 @@ static struct variant_data variant_ux500v2 = {
 	.fifohalfsize		= 8 * 4,
 	.clkreg			= MCI_CLK_ENABLE,
 	.clkreg_enable		= MCI_ST_UX500_HWFCEN,
+	.clkreg_8bit_bus_enable = MCI_ST_8BIT_BUS,
 	.datactrl_mask_ddrmode	= MCI_ST_DPSM_DDRMODE,
 	.datalength_bits	= 24,
 	.sdio			= true,
@@ -340,7 +345,7 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)
 	if (host->mmc->ios.bus_width == MMC_BUS_WIDTH_4)
 		clk |= MCI_4BIT_BUS;
 	if (host->mmc->ios.bus_width == MMC_BUS_WIDTH_8)
-		clk |= MCI_ST_8BIT_BUS;
+		clk |= variant->clkreg_8bit_bus_enable;
 
 	if (host->mmc->ios.timing == MMC_TIMING_UHS_DDR50)
 		clk |= MCI_ST_UX500_NEG_EDGE;
-- 
1.9.1

[PATCH v3 09/13] mmc: mmci: add edge support to data and command out in variant data.

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

This patch adds edge support for data and command out to variant structure
giving more flexibility to the driver to support more SOCs which have
different clock register layout.

Without this patch other new SOCs like Qcom will have to add more code to
special case them

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index a81f303..17e7f6a 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -53,6 +53,7 @@ static unsigned int fmax = 515633;
  * @clkreg: default value for MCICLOCK register
  * @clkreg_enable: enable value for MMCICLOCK register
  * @clkreg_8bit_bus_enable: enable value for 8 bit bus
+ * @clkreg_neg_edge_enable: enable value for inverted data/cmd output
  * @datalength_bits: number of bits in the MMCIDATALENGTH register
  * @fifosize: number of bytes that can be written when MMCI_TXFIFOEMPTY
  *	      is asserted (likewise for RX)
@@ -76,6 +77,7 @@ struct variant_data {
 	unsigned int		clkreg;
 	unsigned int		clkreg_enable;
 	unsigned int		clkreg_8bit_bus_enable;
+	unsigned int		clkreg_neg_edge_enable;
 	unsigned int		datalength_bits;
 	unsigned int		fifosize;
 	unsigned int		fifohalfsize;
@@ -148,6 +150,7 @@ static struct variant_data variant_ux500 = {
 	.clkreg			= MCI_CLK_ENABLE,
 	.clkreg_enable		= MCI_ST_UX500_HWFCEN,
 	.clkreg_8bit_bus_enable = MCI_ST_8BIT_BUS,
+	.clkreg_neg_edge_enable	= MCI_ST_UX500_NEG_EDGE,
 	.datactrl_mask_ddrmode	= MCI_ST_DPSM_DDRMODE,
 	.datalength_bits	= 24,
 	.sdio			= true,
@@ -165,6 +168,7 @@ static struct variant_data variant_ux500v2 = {
 	.clkreg			= MCI_CLK_ENABLE,
 	.clkreg_enable		= MCI_ST_UX500_HWFCEN,
 	.clkreg_8bit_bus_enable = MCI_ST_8BIT_BUS,
+	.clkreg_neg_edge_enable	= MCI_ST_UX500_NEG_EDGE,
 	.datactrl_mask_ddrmode	= MCI_ST_DPSM_DDRMODE,
 	.datalength_bits	= 24,
 	.sdio			= true,
@@ -348,7 +352,7 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)
 		clk |= variant->clkreg_8bit_bus_enable;
 
 	if (host->mmc->ios.timing == MMC_TIMING_UHS_DDR50)
-		clk |= MCI_ST_UX500_NEG_EDGE;
+		clk |= variant->clkreg_neg_edge_enable;
 
 	mmci_write_clkreg(host, clk);
 }
-- 
1.9.1

[PATCH v3 10/13] mmc: mmci: add Qcom specifics of clk and datactrl registers.

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

This patch adds specifics of clk and datactrl register on Qualcomm SD
Card controller. This patch also populates the Qcom variant data with
these new values specific to Qualcomm SD Card Controller.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c |  4 ++++
 drivers/mmc/host/mmci.h | 24 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 17e7f6a..6434f5b1 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -185,6 +185,10 @@ static struct variant_data variant_qcom = {
 	.fifosize		= 16 * 4,
 	.fifohalfsize		= 8 * 4,
 	.clkreg			= MCI_CLK_ENABLE,
+	.clkreg_enable		= MCI_QCOM_CLK_FLOWENA |
+				  MCI_QCOM_CLK_FEEDBACK_CLK,
+	.clkreg_8bit_bus_enable = MCI_QCOM_CLK_WIDEBUS_8,
+	.datactrl_mask_ddrmode	= MCI_QCOM_CLK_DDR_MODE,
 	.blksz_datactrl4	= true,
 	.datalength_bits	= 24,
 	.blksz_datactrl4	= true,
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index cd83ca3..1b93ae7 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -41,6 +41,22 @@
 /* Modified PL180 on Versatile Express platform */
 #define MCI_ARM_HWFCEN		BIT(12)
 
+/* Modified on Qualcomm Integrations */
+#define MCI_QCOM_CLK_WIDEBUS_4	(2 << 10)
+#define MCI_QCOM_CLK_WIDEBUS_8	(3 << 10)
+#define MCI_QCOM_CLK_FLOWENA	BIT(12)
+#define MCI_QCOM_CLK_INVERTOUT	BIT(13)
+
+/* select in latch data and command */
+#define MCI_QCOM_CLK_SEL_IN_SHIFT	(14)
+#define MCI_QCOM_CLK_SEL_MASK		(0x3)
+#define MCI_QCOM_CLK_SEL_RISING_EDGE	(1)
+#define MCI_QCOM_CLK_FEEDBACK_CLK	(2 << 14)
+#define MCI_QCOM_CLK_DDR_MODE		(3 << 14)
+
+/* mclk selection */
+#define MCI_QCOM_CLK_SEL_MCLK		(2 << 23)
+
 #define MMCIARGUMENT		0x008
 #define MMCICOMMAND		0x00c
 #define MCI_CPSM_RESPONSE	BIT(6)
@@ -54,6 +70,14 @@
 #define MCI_ST_NIEN		BIT(13)
 #define MCI_ST_CE_ATACMD	BIT(14)
 
+/* Modified on Qualcomm Integrations */
+#define MCI_QCOM_CSPM_DATCMD		BIT(12)
+#define MCI_QCOM_CSPM_MCIABORT		BIT(13)
+#define MCI_QCOM_CSPM_CCSENABLE		BIT(14)
+#define MCI_QCOM_CSPM_CCSDISABLE	BIT(15)
+#define MCI_QCOM_CSPM_AUTO_CMD19	BIT(16)
+#define MCI_QCOM_CSPM_AUTO_CMD21	BIT(21)
+
 #define MMCIRESPCMD		0x010
 #define MMCIRESPONSE0		0x014
 #define MMCIRESPONSE1		0x018
-- 
1.9.1

[PATCH v3 11/13] mmc: mmci: Add support to data commands via variant structure.

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

On some SOCs like Qcom there are explicit bits in the command register
to specify if its a data transfer command or not. So this patch adds
support to such bits in variant data, giving more flexibility to the
driver.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 6434f5b1..5cbf644 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -59,6 +59,7 @@ static unsigned int fmax = 515633;
  *	      is asserted (likewise for RX)
  * @fifohalfsize: number of bytes that can be written when MCI_TXFIFOHALFEMPTY
  *		  is asserted (likewise for RX)
+ * @data_cmd_enable: enable value for data commands.
  * @sdio: variant supports SDIO
  * @st_clkdiv: true if using a ST-specific clock divider algorithm
  * @datactrl_mask_ddrmode: ddr mode mask in datactrl register.
@@ -81,6 +82,7 @@ struct variant_data {
 	unsigned int		datalength_bits;
 	unsigned int		fifosize;
 	unsigned int		fifohalfsize;
+	unsigned int		data_cmd_enable;
 	unsigned int		datactrl_mask_ddrmode;
 	bool			sdio;
 	bool			st_clkdiv;
@@ -189,6 +191,7 @@ static struct variant_data variant_qcom = {
 				  MCI_QCOM_CLK_FEEDBACK_CLK,
 	.clkreg_8bit_bus_enable = MCI_QCOM_CLK_WIDEBUS_8,
 	.datactrl_mask_ddrmode	= MCI_QCOM_CLK_DDR_MODE,
+	.data_cmd_enable	= MCI_QCOM_CSPM_DATCMD,
 	.blksz_datactrl4	= true,
 	.datalength_bits	= 24,
 	.blksz_datactrl4	= true,
@@ -875,6 +878,9 @@ mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c)
 	if (/*interrupt*/0)
 		c |= MCI_CPSM_INTERRUPT;
 
+	if (mmc_cmd_type(cmd) == MMC_CMD_ADTC)
+		c |= host->variant->data_cmd_enable;
+
 	host->cmd = cmd;
 
 	mmci_writel(host, cmd->arg, MMCIARGUMENT);
-- 
1.9.1

[PATCH v3 12/13] mmc: mmci: add explicit clk control

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

On Controllers like Qcom SD card controller where cclk is mclk and mclk should
be directly controlled by the driver.

This patch adds support to control mclk directly in the driver, and also
adds explicit_mclk_control and cclk_is_mclk flags in variant structure giving
more flexibility to the driver.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 5cbf644..f6dfd24 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -73,6 +73,8 @@ static unsigned int fmax = 515633;
  * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply
  * @mclk_delayed_writes: enable delayed writes to ensure, subsequent updates
  *			 are not ignored.
+ * @explicit_mclk_control: enable explicit mclk control in driver.
+ * @cclk_is_mclk: enable iff card clock is multimedia card adapter clock.
  */
 struct variant_data {
 	unsigned int		clkreg;
@@ -94,6 +96,8 @@ struct variant_data {
 	bool			busy_detect;
 	bool			pwrreg_nopower;
 	bool			mclk_delayed_writes;
+	bool			explicit_mclk_control;
+	bool			cclk_is_mclk;
 };
 
 static struct variant_data variant_arm = {
@@ -202,6 +206,8 @@ static struct variant_data variant_qcom = {
 	 * for 3 clk cycles.
 	 */
 	.mclk_delayed_writes	= true,
+	.explicit_mclk_control	= true,
+	.cclk_is_mclk	= true,
 };
 
 static inline u32 mmci_readl(struct mmci_host *host, u32 off)
@@ -317,7 +323,9 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)
 	host->cclk = 0;
 
 	if (desired) {
-		if (desired >= host->mclk) {
+		if (variant->cclk_is_mclk) {
+			host->cclk = host->mclk;
+		} else if (desired >= host->mclk) {
 			clk = MCI_CLK_BYPASS;
 			if (variant->st_clkdiv)
 				clk |= MCI_ST_UX500_NEG_EDGE;
@@ -1354,6 +1362,16 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	if (!ios->clock && variant->pwrreg_clkgate)
 		pwr &= ~MCI_PWR_ON;
 
+	if (ios->clock != host->mclk && host->variant->explicit_mclk_control) {
+		int rc = clk_set_rate(host->clk, ios->clock);
+		if (rc < 0) {
+			dev_err(mmc_dev(host->mmc),
+				"Error setting clock rate (%d)\n", rc);
+		} else {
+			host->mclk = clk_get_rate(host->clk);
+		}
+	}
+
 	spin_lock_irqsave(&host->lock, flags);
 
 	mmci_set_clkreg(host, ios->clock);
@@ -1540,10 +1558,12 @@ static int mmci_probe(struct amba_device *dev,
 	 * is not specified. Either value must not exceed the clock rate into
 	 * the block, of course.
 	 */
-	if (mmc->f_max)
-		mmc->f_max = min(host->mclk, mmc->f_max);
-	else
-		mmc->f_max = min(host->mclk, fmax);
+	if (!host->variant->explicit_mclk_control) {
+		if (mmc->f_max)
+			mmc->f_max = min(host->mclk, mmc->f_max);
+		else
+			mmc->f_max = min(host->mclk, fmax);
+	}
 	dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max);
 
 	/* Get regulators and the supported OCR mask */
-- 
1.9.1

[PATCH v3 13/13] mmc: mmci: Add Qcom specific pio_read function.

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

MCIFIFOCNT register behaviour on Qcom chips is very different than the other
pl180 integrations. MCIFIFOCNT register contains the number of
words that are still waiting to be transferred through the FIFO. It keeps
decrementing once the host CPU reads the MCIFIFO. With the existing logic and
the MCIFIFOCNT behaviour, mmci_pio_read will loop forever, as the FIFOCNT
register will always return transfer size before reading the FIFO.

Also the data sheet states that "This register is only useful for debug
purposes and should not be used for normal operation since it does not reflect
data which may or may not be in the pipeline".

This patch implements qcom_pio_read function so as existing mmci_pio_read is
not suitable for Qcom SOCs. qcom_pio_read function is only selected
based on qcom_fifo flag in variant data structure.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/mmc/host/mmci.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index f6dfd24..51ce493 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -75,6 +75,7 @@ static unsigned int fmax = 515633;
  *			 are not ignored.
  * @explicit_mclk_control: enable explicit mclk control in driver.
  * @cclk_is_mclk: enable iff card clock is multimedia card adapter clock.
+ * @qcom_fifo: enables qcom specific fifo pio read function.
  */
 struct variant_data {
 	unsigned int		clkreg;
@@ -98,6 +99,7 @@ struct variant_data {
 	bool			mclk_delayed_writes;
 	bool			explicit_mclk_control;
 	bool			cclk_is_mclk;
+	bool			qcom_fifo;
 };
 
 static struct variant_data variant_arm = {
@@ -208,6 +210,7 @@ static struct variant_data variant_qcom = {
 	.mclk_delayed_writes	= true,
 	.explicit_mclk_control	= true,
 	.cclk_is_mclk	= true,
+	.qcom_fifo		= true,
 };
 
 static inline u32 mmci_readl(struct mmci_host *host, u32 off)
@@ -1022,6 +1025,40 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
 	}
 }
 
+static int mmci_qcom_pio_read(struct mmci_host *host, char *buffer,
+			unsigned int remain)
+{
+	u32 *ptr = (u32 *) buffer;
+	unsigned int count = 0;
+	unsigned int words, bytes;
+	unsigned int fsize = host->variant->fifosize;
+
+	words = remain >> 2;
+	bytes = remain % 4;
+	/* read full words followed by leftover bytes */
+	if (words) {
+		while (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) {
+			*ptr = readl(host->base + MMCIFIFO + (count % fsize));
+			ptr++;
+			count += 4;
+			words--;
+			if (!words)
+				break;
+		}
+	}
+
+	if (unlikely(bytes)) {
+		unsigned char buf[4];
+		if (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) {
+			*buf = readl(host->base + MMCIFIFO + (count % fsize));
+			memcpy(ptr, buf, bytes);
+			count += bytes;
+		}
+	}
+
+	return count;
+}
+
 static int mmci_pio_read(struct mmci_host *host, char *buffer, unsigned int remain)
 {
 	void __iomem *base = host->base;
@@ -1143,8 +1180,13 @@ static irqreturn_t mmci_pio_irq(int irq, void *dev_id)
 		remain = sg_miter->length;
 
 		len = 0;
-		if (status & MCI_RXACTIVE)
-			len = mmci_pio_read(host, buffer, remain);
+		if (status & MCI_RXACTIVE) {
+			if (variant->qcom_fifo)
+				len = mmci_qcom_pio_read(host, buffer, remain);
+			else
+				len = mmci_pio_read(host, buffer, remain);
+		}
+
 		if (status & MCI_TXACTIVE)
 			len = mmci_pio_write(host, buffer, remain, status);
 
-- 
1.9.1

Re: [PATCH v3 13/13] mmc: mmci: Add Qcom specific pio_read function.

[Stephen Boyd]

On 05/23/14 05:53, srinivas.kandagatla@linaro.org wrote:
> @@ -1022,6 +1025,40 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>  	}
>  }
>  
> +static int mmci_qcom_pio_read(struct mmci_host *host, char *buffer,
> +			unsigned int remain)
> +{
> +	u32 *ptr = (u32 *) buffer;
> +	unsigned int count = 0;
> +	unsigned int words, bytes;
> +	unsigned int fsize = host->variant->fifosize;
> +
> +	words = remain >> 2;
> +	bytes = remain % 4;
> +	/* read full words followed by leftover bytes */
> +	if (words) {
> +		while (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) {
> +			*ptr = readl(host->base + MMCIFIFO + (count % fsize));

This doesn't look endianness agnostic. Shouldn't we use ioread32_rep()
to read this fifo?

> +			ptr++;
> +			count += 4;
> +			words--;
> +			if (!words)
> +				break;
> +		}
> +	}
> +
> +	if (unlikely(bytes)) {
> +		unsigned char buf[4];
> +		if (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) {
> +			*buf = readl(host->base + MMCIFIFO + (count % fsize));
> +			memcpy(ptr, buf, bytes);
> +			count += bytes;
> +		}
> +	}
> +
> +	return count;
> +}
> +
>  static int mmci_pio_read(struct mmci_host *host, char *buffer, unsigned int remain)
>  {
>  	void __iomem *base = host->base;
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v3 03/13] mmc: mmci: Add Qualcomm Id to amba id table

[Ulf Hansson]

Hi Srinivas,

>
> +static struct variant_data variant_qcom = {
> +       .fifosize               = 16 * 4,
> +       .fifohalfsize           = 8 * 4,
> +       .clkreg                 = MCI_CLK_ENABLE,
> +       .datalength_bits        = 24,
> +       .blksz_datactrl4        = true,

You get compile error here.

Kind regards
Ulf Hansson

Re: [PATCH v3 06/13] mmc: mmci: Qcomm: Add 3 clock cycle delay after register write

[Ulf Hansson]

On 23 May 2014 14:51,  <srinivas.kandagatla@linaro.org> wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>
> Most of the Qcomm SD card controller registers must be updated to the MCLK
> domain so subsequent writes to registers will be ignored until 3 clock cycles
> have passed.
>
> This patch adds a 3 clock cycle delay required after writing to controller
> registers on Qualcomm SOCs. Without this delay all the register writes are not
> successful, resulting in not detecting cards. The write clock delay is
> activated by setting up mclk_delayed_writes variable in variant data.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mmc/host/mmci.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 881bb24..1385554 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -67,6 +67,8 @@ static unsigned int fmax = 515633;
>   * @pwrreg_clkgate: MMCIPOWER register must be used to gate the clock
>   * @busy_detect: true if busy detection on dat0 is supported
>   * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply
> + * @mclk_delayed_writes: enable delayed writes to ensure, subsequent updates
> + *                      are not ignored.
>   */
>  struct variant_data {
>         unsigned int            clkreg;
> @@ -83,6 +85,7 @@ struct variant_data {
>         bool                    pwrreg_clkgate;
>         bool                    busy_detect;
>         bool                    pwrreg_nopower;
> +       bool                    mclk_delayed_writes;
>  };
>
>  static struct variant_data variant_arm = {
> @@ -171,6 +174,12 @@ static struct variant_data variant_qcom = {
>         .datalength_bits        = 24,
>         .blksz_datactrl4        = true,
>         .pwrreg_powerup         = MCI_PWR_UP,
> +       /*
> +        * On QCom SD card controller, registers must be updated to the
> +        * MCLK domain so subsequent writes to this register will be ignored
> +        * for 3 clk cycles.
> +        */
> +       .mclk_delayed_writes    = true,
>  };
>
>  static inline u32 mmci_readl(struct mmci_host *host, u32 off)
> @@ -181,6 +190,9 @@ static inline u32 mmci_readl(struct mmci_host *host, u32 off)
>  static inline void mmci_writel(struct mmci_host *host, u32 data, u32 off)
>  {
>         writel(data, host->base + off);
> +
> +       if (host->variant->mclk_delayed_writes)
> +               udelay(DIV_ROUND_UP((3 * USEC_PER_SEC), host->mclk));
>  }

I am not sure I like this approach. For each and every writel
(including pio_writes) you will add a few cpu cycles, since you need
to  check for "mclk_delayed_writes" no matter of variant.

How about, adding a new function pointer in the struct mmci_host, for
"writel operations" which you could set up in probe phase instead?

Kind regards
Ulf Hansson

Re: [PATCH v3 07/13] mmc: mmci: add ddrmode mask to variant data

[Ulf Hansson]

On 23 May 2014 14:51,  <srinivas.kandagatla@linaro.org> wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>
> This patch adds ddrmode mask to variant structure giving more flexibility
> to the driver to support more SOCs which have different datactrl register
> layout.
>
> Without this patch datactrl register is updated with wrong ddrmode mask on non
> ST SOCs, resulting in card detection failures.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mmc/host/mmci.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 1385554..dec70d2 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -59,6 +59,7 @@ static unsigned int fmax = 515633;
>   *               is asserted (likewise for RX)
>   * @sdio: variant supports SDIO
>   * @st_clkdiv: true if using a ST-specific clock divider algorithm
> + * @datactrl_mask_ddrmode: ddr mode mask in datactrl register.
>   * @blksz_datactrl16: true if Block size is at b16..b30 position in datactrl register
>   * @blksz_datactrl4: true if Block size is at b4..b16 position in datactrl
>   *                  register
> @@ -76,6 +77,7 @@ struct variant_data {
>         unsigned int            datalength_bits;
>         unsigned int            fifosize;
>         unsigned int            fifohalfsize;
> +       unsigned int            datactrl_mask_ddrmode;
>         bool                    sdio;
>         bool                    st_clkdiv;
>         bool                    blksz_datactrl16;
> @@ -114,6 +116,7 @@ static struct variant_data variant_u300 = {
>         .fifosize               = 16 * 4,
>         .fifohalfsize           = 8 * 4,
>         .clkreg_enable          = MCI_ST_U300_HWFCEN,
> +       .datactrl_mask_ddrmode  = MCI_ST_DPSM_DDRMODE,
>         .datalength_bits        = 16,
>         .sdio                   = true,
>         .pwrreg_powerup         = MCI_PWR_ON,
> @@ -126,6 +129,7 @@ static struct variant_data variant_nomadik = {
>         .fifosize               = 16 * 4,
>         .fifohalfsize           = 8 * 4,
>         .clkreg                 = MCI_CLK_ENABLE,
> +       .datactrl_mask_ddrmode  = MCI_ST_DPSM_DDRMODE,
>         .datalength_bits        = 24,
>         .sdio                   = true,
>         .st_clkdiv              = true,
> @@ -140,6 +144,7 @@ static struct variant_data variant_ux500 = {
>         .fifohalfsize           = 8 * 4,
>         .clkreg                 = MCI_CLK_ENABLE,
>         .clkreg_enable          = MCI_ST_UX500_HWFCEN,
> +       .datactrl_mask_ddrmode  = MCI_ST_DPSM_DDRMODE,
>         .datalength_bits        = 24,
>         .sdio                   = true,
>         .st_clkdiv              = true,
> @@ -155,6 +160,7 @@ static struct variant_data variant_ux500v2 = {
>         .fifohalfsize           = 8 * 4,
>         .clkreg                 = MCI_CLK_ENABLE,
>         .clkreg_enable          = MCI_ST_UX500_HWFCEN,
> +       .datactrl_mask_ddrmode  = MCI_ST_DPSM_DDRMODE,

Only the ux500v2 supports DDR mode, using the MCI_ST_DPSM_DDRMODE bit.

>         .datalength_bits        = 24,
>         .sdio                   = true,
>         .st_clkdiv              = true,
> @@ -800,7 +806,7 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
>                 }
>
>         if (host->mmc->ios.timing == MMC_TIMING_UHS_DDR50)
> -               datactrl |= MCI_ST_DPSM_DDRMODE;
> +               datactrl |= variant->datactrl_mask_ddrmode;
>
>         /*
>          * Attempt to use DMA operation mode, if this
> --
> 1.9.1
>

Kind regards
Ulf Hansson

Re: [PATCH v3 08/13] mmc: mmci: add 8bit bus support in variant data

[Ulf Hansson]

On 23 May 2014 14:52,  <srinivas.kandagatla@linaro.org> wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>
> This patch adds 8bit bus enable to variant structure giving more flexibility
> to the driver to support more SOCs which have different clock register layout.
>
> Without this patch other new SOCs like Qcom will have to add more code
> to special case them.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mmc/host/mmci.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index dec70d2..a81f303 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -52,6 +52,7 @@ static unsigned int fmax = 515633;
>   * struct variant_data - MMCI variant-specific quirks
>   * @clkreg: default value for MCICLOCK register
>   * @clkreg_enable: enable value for MMCICLOCK register
> + * @clkreg_8bit_bus_enable: enable value for 8 bit bus
>   * @datalength_bits: number of bits in the MMCIDATALENGTH register
>   * @fifosize: number of bytes that can be written when MMCI_TXFIFOEMPTY
>   *           is asserted (likewise for RX)
> @@ -74,6 +75,7 @@ static unsigned int fmax = 515633;
>  struct variant_data {
>         unsigned int            clkreg;
>         unsigned int            clkreg_enable;
> +       unsigned int            clkreg_8bit_bus_enable;
>         unsigned int            datalength_bits;
>         unsigned int            fifosize;
>         unsigned int            fifohalfsize;
> @@ -116,6 +118,7 @@ static struct variant_data variant_u300 = {
>         .fifosize               = 16 * 4,
>         .fifohalfsize           = 8 * 4,
>         .clkreg_enable          = MCI_ST_U300_HWFCEN,
> +       .clkreg_8bit_bus_enable = MCI_ST_8BIT_BUS,

Linus, will have to confirm this. I don't know if the u300 variant
support 8-bit.

Kind regards
Ulf Hansson

>         .datactrl_mask_ddrmode  = MCI_ST_DPSM_DDRMODE,
>         .datalength_bits        = 16,
>         .sdio                   = true,
> @@ -144,6 +147,7 @@ static struct variant_data variant_ux500 = {
>         .fifohalfsize           = 8 * 4,
>         .clkreg                 = MCI_CLK_ENABLE,
>         .clkreg_enable          = MCI_ST_UX500_HWFCEN,
> +       .clkreg_8bit_bus_enable = MCI_ST_8BIT_BUS,
>         .datactrl_mask_ddrmode  = MCI_ST_DPSM_DDRMODE,
>         .datalength_bits        = 24,
>         .sdio                   = true,
> @@ -160,6 +164,7 @@ static struct variant_data variant_ux500v2 = {
>         .fifohalfsize           = 8 * 4,
>         .clkreg                 = MCI_CLK_ENABLE,
>         .clkreg_enable          = MCI_ST_UX500_HWFCEN,
> +       .clkreg_8bit_bus_enable = MCI_ST_8BIT_BUS,
>         .datactrl_mask_ddrmode  = MCI_ST_DPSM_DDRMODE,
>         .datalength_bits        = 24,
>         .sdio                   = true,
> @@ -340,7 +345,7 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)
>         if (host->mmc->ios.bus_width == MMC_BUS_WIDTH_4)
>                 clk |= MCI_4BIT_BUS;
>         if (host->mmc->ios.bus_width == MMC_BUS_WIDTH_8)
> -               clk |= MCI_ST_8BIT_BUS;
> +               clk |= variant->clkreg_8bit_bus_enable;
>
>         if (host->mmc->ios.timing == MMC_TIMING_UHS_DDR50)
>                 clk |= MCI_ST_UX500_NEG_EDGE;
> --
> 1.9.1
>

Re: [PATCH v3 10/13] mmc: mmci: add Qcom specifics of clk and datactrl registers.

[Ulf Hansson]

On 23 May 2014 14:52,  <srinivas.kandagatla@linaro.org> wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>
> This patch adds specifics of clk and datactrl register on Qualcomm SD
> Card controller. This patch also populates the Qcom variant data with
> these new values specific to Qualcomm SD Card Controller.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mmc/host/mmci.c |  4 ++++
>  drivers/mmc/host/mmci.h | 24 ++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 17e7f6a..6434f5b1 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -185,6 +185,10 @@ static struct variant_data variant_qcom = {
>         .fifosize               = 16 * 4,
>         .fifohalfsize           = 8 * 4,
>         .clkreg                 = MCI_CLK_ENABLE,
> +       .clkreg_enable          = MCI_QCOM_CLK_FLOWENA |
> +                                 MCI_QCOM_CLK_FEEDBACK_CLK,

Obviously I don't have the in-depth knowledge about the Qcom variant,
but comparing the ST variant here made me think.

Using the feeback clock internal logic in the ST variant, requires the
corresponding feedback clock pin signal on the board, to be
routed/connected. Typically we used this for SD cards, which involved
using an external level shifter circuit.

Is it correct to enable this bit for all cases, including eMMC?

If it is board specific configurations, you should add a DT binding
for it - like there are for the ST variant.

> +       .clkreg_8bit_bus_enable = MCI_QCOM_CLK_WIDEBUS_8,
> +       .datactrl_mask_ddrmode  = MCI_QCOM_CLK_DDR_MODE,
>         .blksz_datactrl4        = true,
>         .datalength_bits        = 24,
>         .blksz_datactrl4        = true,
> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
> index cd83ca3..1b93ae7 100644
> --- a/drivers/mmc/host/mmci.h
> +++ b/drivers/mmc/host/mmci.h
> @@ -41,6 +41,22 @@
>  /* Modified PL180 on Versatile Express platform */
>  #define MCI_ARM_HWFCEN         BIT(12)
>
> +/* Modified on Qualcomm Integrations */
> +#define MCI_QCOM_CLK_WIDEBUS_4 (2 << 10)

This is the same as BIT(11), please use MCI_4BIT_BUS instead.

> +#define MCI_QCOM_CLK_WIDEBUS_8 (3 << 10)

Since you converted to use the "BIT" macro a few patches ago, I
suggest we should stick to it. How about something below:

#define MCI_QCOM_CLK_WIDEBUS_8 BIT (BIT(10) | BIT(11))

Please adopt all defines added in this patch to use the BIT macro.

> +#define MCI_QCOM_CLK_FLOWENA   BIT(12)
> +#define MCI_QCOM_CLK_INVERTOUT BIT(13)
> +
> +/* select in latch data and command */
> +#define MCI_QCOM_CLK_SEL_IN_SHIFT      (14)

BIT (14)?

> +#define MCI_QCOM_CLK_SEL_MASK          (0x3)
> +#define MCI_QCOM_CLK_SEL_RISING_EDGE   (1)

BIT(1)?

> +#define MCI_QCOM_CLK_FEEDBACK_CLK      (2 << 14)
> +#define MCI_QCOM_CLK_DDR_MODE          (3 << 14)
> +
> +/* mclk selection */
> +#define MCI_QCOM_CLK_SEL_MCLK          (2 << 23)

Does this correspond to MCI_CLK_BYPASS? If so, we should maybe state
this in a comment?

> +
>  #define MMCIARGUMENT           0x008
>  #define MMCICOMMAND            0x00c
>  #define MCI_CPSM_RESPONSE      BIT(6)
> @@ -54,6 +70,14 @@
>  #define MCI_ST_NIEN            BIT(13)
>  #define MCI_ST_CE_ATACMD       BIT(14)
>
> +/* Modified on Qualcomm Integrations */
> +#define MCI_QCOM_CSPM_DATCMD           BIT(12)
> +#define MCI_QCOM_CSPM_MCIABORT         BIT(13)
> +#define MCI_QCOM_CSPM_CCSENABLE                BIT(14)
> +#define MCI_QCOM_CSPM_CCSDISABLE       BIT(15)
> +#define MCI_QCOM_CSPM_AUTO_CMD19       BIT(16)
> +#define MCI_QCOM_CSPM_AUTO_CMD21       BIT(21)
> +
>  #define MMCIRESPCMD            0x010
>  #define MMCIRESPONSE0          0x014
>  #define MMCIRESPONSE1          0x018
> --
> 1.9.1
>

Kind regards
Ulf Hansson

Re: [PATCH v3 12/13] mmc: mmci: add explicit clk control

[Ulf Hansson]

On 23 May 2014 14:52,  <srinivas.kandagatla@linaro.org> wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>
> On Controllers like Qcom SD card controller where cclk is mclk and mclk should
> be directly controlled by the driver.
>
> This patch adds support to control mclk directly in the driver, and also
> adds explicit_mclk_control and cclk_is_mclk flags in variant structure giving
> more flexibility to the driver.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mmc/host/mmci.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 5cbf644..f6dfd24 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -73,6 +73,8 @@ static unsigned int fmax = 515633;
>   * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply
>   * @mclk_delayed_writes: enable delayed writes to ensure, subsequent updates
>   *                      are not ignored.
> + * @explicit_mclk_control: enable explicit mclk control in driver.
> + * @cclk_is_mclk: enable iff card clock is multimedia card adapter clock.
>   */
>  struct variant_data {
>         unsigned int            clkreg;
> @@ -94,6 +96,8 @@ struct variant_data {
>         bool                    busy_detect;
>         bool                    pwrreg_nopower;
>         bool                    mclk_delayed_writes;
> +       bool                    explicit_mclk_control;
> +       bool                    cclk_is_mclk;

I can't see why you need to have both these new configurations. Aren't
"cclk_is_mclk" just a fact when you use "explicit_mclk_control".

I also believe I would prefer something like "qcom_clkdiv" instead.

>  };
>
>  static struct variant_data variant_arm = {
> @@ -202,6 +206,8 @@ static struct variant_data variant_qcom = {
>          * for 3 clk cycles.
>          */
>         .mclk_delayed_writes    = true,
> +       .explicit_mclk_control  = true,
> +       .cclk_is_mclk   = true,
>  };
>
>  static inline u32 mmci_readl(struct mmci_host *host, u32 off)
> @@ -317,7 +323,9 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)
>         host->cclk = 0;
>
>         if (desired) {
> -               if (desired >= host->mclk) {
> +               if (variant->cclk_is_mclk) {
> +                       host->cclk = host->mclk;
> +               } else if (desired >= host->mclk) {
>                         clk = MCI_CLK_BYPASS;
>                         if (variant->st_clkdiv)
>                                 clk |= MCI_ST_UX500_NEG_EDGE;
> @@ -1354,6 +1362,16 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>         if (!ios->clock && variant->pwrreg_clkgate)
>                 pwr &= ~MCI_PWR_ON;
>
> +       if (ios->clock != host->mclk && host->variant->explicit_mclk_control) {

I suggest you should clarify the statement by adding a pair of extra
parentheses. Additionally it seems like a good idea to reverse the
order of the statements, to clarify this is for qcom clock handling
only.

More important, what I think you really want to do is to compare
"ios->clock" with it's previous value it had when ->set_ios were
invoked. Then let a changed value act as the trigger to set a new clk
rate. Obvoiusly you need to cache the clock rate in the struct mmci
host to handle this.

> +               int rc = clk_set_rate(host->clk, ios->clock);
> +               if (rc < 0) {
> +                       dev_err(mmc_dev(host->mmc),
> +                               "Error setting clock rate (%d)\n", rc);
> +               } else {
> +                       host->mclk = clk_get_rate(host->clk);

So here you actually find out the new clk rate, but shouldn't you
update "host->mclk" within the spin_lock? Or it might not matter?

> +               }
> +       }
> +
>         spin_lock_irqsave(&host->lock, flags);
>
>         mmci_set_clkreg(host, ios->clock);
> @@ -1540,10 +1558,12 @@ static int mmci_probe(struct amba_device *dev,
>          * is not specified. Either value must not exceed the clock rate into
>          * the block, of course.
>          */
> -       if (mmc->f_max)
> -               mmc->f_max = min(host->mclk, mmc->f_max);
> -       else
> -               mmc->f_max = min(host->mclk, fmax);
> +       if (!host->variant->explicit_mclk_control) {
> +               if (mmc->f_max)
> +                       mmc->f_max = min(host->mclk, mmc->f_max);
> +               else
> +                       mmc->f_max = min(host->mclk, fmax);
> +       }

This means your mmc->f_max value will either be zero or the one you
provided through DT. And since zero won't work, that means you
_require_ to get the value from DT. According to the documentation of
this DT binding, f_max is optional.

So unless you fine another way of dynamically at runtime figure out
the value of f_max (using the clk API), you need to update the DT
documentation for mmci.

Additionally, this makes me wonder about f_min. I haven't seen
anywhere in this patch were that value is being set to proper value,
right?

>         dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max);
>
>         /* Get regulators and the supported OCR mask */
> --
> 1.9.1
>

Kind regards
Ulf Hansson

Re: [PATCH v3 12/13] mmc: mmci: add explicit clk control

[Ulf Hansson]

On 26 May 2014 16:21, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 23 May 2014 14:52,  <srinivas.kandagatla@linaro.org> wrote:
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> On Controllers like Qcom SD card controller where cclk is mclk and mclk should
>> be directly controlled by the driver.
>>
>> This patch adds support to control mclk directly in the driver, and also
>> adds explicit_mclk_control and cclk_is_mclk flags in variant structure giving
>> more flexibility to the driver.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>>  drivers/mmc/host/mmci.c | 30 +++++++++++++++++++++++++-----
>>  1 file changed, 25 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index 5cbf644..f6dfd24 100644
>> --- a/drivers/mmc/host/mmci.c
>> +++ b/drivers/mmc/host/mmci.c
>> @@ -73,6 +73,8 @@ static unsigned int fmax = 515633;
>>   * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply
>>   * @mclk_delayed_writes: enable delayed writes to ensure, subsequent updates
>>   *                      are not ignored.
>> + * @explicit_mclk_control: enable explicit mclk control in driver.
>> + * @cclk_is_mclk: enable iff card clock is multimedia card adapter clock.
>>   */
>>  struct variant_data {
>>         unsigned int            clkreg;
>> @@ -94,6 +96,8 @@ struct variant_data {
>>         bool                    busy_detect;
>>         bool                    pwrreg_nopower;
>>         bool                    mclk_delayed_writes;
>> +       bool                    explicit_mclk_control;
>> +       bool                    cclk_is_mclk;
>
> I can't see why you need to have both these new configurations. Aren't
> "cclk_is_mclk" just a fact when you use "explicit_mclk_control".
>
> I also believe I would prefer something like "qcom_clkdiv" instead.
>
>>  };
>>
>>  static struct variant_data variant_arm = {
>> @@ -202,6 +206,8 @@ static struct variant_data variant_qcom = {
>>          * for 3 clk cycles.
>>          */
>>         .mclk_delayed_writes    = true,
>> +       .explicit_mclk_control  = true,
>> +       .cclk_is_mclk   = true,
>>  };
>>
>>  static inline u32 mmci_readl(struct mmci_host *host, u32 off)
>> @@ -317,7 +323,9 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)
>>         host->cclk = 0;
>>
>>         if (desired) {
>> -               if (desired >= host->mclk) {
>> +               if (variant->cclk_is_mclk) {
>> +                       host->cclk = host->mclk;
>> +               } else if (desired >= host->mclk) {
>>                         clk = MCI_CLK_BYPASS;
>>                         if (variant->st_clkdiv)
>>                                 clk |= MCI_ST_UX500_NEG_EDGE;
>> @@ -1354,6 +1362,16 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>         if (!ios->clock && variant->pwrreg_clkgate)
>>                 pwr &= ~MCI_PWR_ON;
>>
>> +       if (ios->clock != host->mclk && host->variant->explicit_mclk_control) {
>
> I suggest you should clarify the statement by adding a pair of extra
> parentheses. Additionally it seems like a good idea to reverse the
> order of the statements, to clarify this is for qcom clock handling
> only.
>
> More important, what I think you really want to do is to compare
> "ios->clock" with it's previous value it had when ->set_ios were
> invoked. Then let a changed value act as the trigger to set a new clk
> rate. Obvoiusly you need to cache the clock rate in the struct mmci
> host to handle this.
>
>> +               int rc = clk_set_rate(host->clk, ios->clock);
>> +               if (rc < 0) {
>> +                       dev_err(mmc_dev(host->mmc),
>> +                               "Error setting clock rate (%d)\n", rc);
>> +               } else {
>> +                       host->mclk = clk_get_rate(host->clk);
>
> So here you actually find out the new clk rate, but shouldn't you
> update "host->mclk" within the spin_lock? Or it might not matter?
>
>> +               }
>> +       }
>> +
>>         spin_lock_irqsave(&host->lock, flags);
>>
>>         mmci_set_clkreg(host, ios->clock);
>> @@ -1540,10 +1558,12 @@ static int mmci_probe(struct amba_device *dev,
>>          * is not specified. Either value must not exceed the clock rate into
>>          * the block, of course.
>>          */
>> -       if (mmc->f_max)
>> -               mmc->f_max = min(host->mclk, mmc->f_max);
>> -       else
>> -               mmc->f_max = min(host->mclk, fmax);
>> +       if (!host->variant->explicit_mclk_control) {
>> +               if (mmc->f_max)
>> +                       mmc->f_max = min(host->mclk, mmc->f_max);
>> +               else
>> +                       mmc->f_max = min(host->mclk, fmax);
>> +       }
>
> This means your mmc->f_max value will either be zero or the one you
> provided through DT. And since zero won't work, that means you
> _require_ to get the value from DT. According to the documentation of
> this DT binding, f_max is optional.
>
> So unless you fine another way of dynamically at runtime figure out
> the value of f_max (using the clk API), you need to update the DT
> documentation for mmci.
>
> Additionally, this makes me wonder about f_min. I haven't seen
> anywhere in this patch were that value is being set to proper value,
> right?
>
>>         dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max);
>>
>>         /* Get regulators and the supported OCR mask */
>> --
>> 1.9.1
>>
>
> Kind regards
> Ulf Hansson

And one more thing, just for your information. If you intend to
support UHS cards, your need to be able to gate the clock though
->set_ios function, once the clock is 0. Let's handle that in a
separate patch - if needed though.

Kind regards
Ulf Hansson

Re: [PATCH v3 13/13] mmc: mmci: Add Qcom specific pio_read function.

[Ulf Hansson]

On 23 May 2014 14:53,  <srinivas.kandagatla@linaro.org> wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>
> MCIFIFOCNT register behaviour on Qcom chips is very different than the other
> pl180 integrations. MCIFIFOCNT register contains the number of
> words that are still waiting to be transferred through the FIFO. It keeps
> decrementing once the host CPU reads the MCIFIFO. With the existing logic and
> the MCIFIFOCNT behaviour, mmci_pio_read will loop forever, as the FIFOCNT
> register will always return transfer size before reading the FIFO.
>
> Also the data sheet states that "This register is only useful for debug
> purposes and should not be used for normal operation since it does not reflect
> data which may or may not be in the pipeline".
>
> This patch implements qcom_pio_read function so as existing mmci_pio_read is
> not suitable for Qcom SOCs. qcom_pio_read function is only selected
> based on qcom_fifo flag in variant data structure.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/mmc/host/mmci.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index f6dfd24..51ce493 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -75,6 +75,7 @@ static unsigned int fmax = 515633;
>   *                      are not ignored.
>   * @explicit_mclk_control: enable explicit mclk control in driver.
>   * @cclk_is_mclk: enable iff card clock is multimedia card adapter clock.
> + * @qcom_fifo: enables qcom specific fifo pio read function.
>   */
>  struct variant_data {
>         unsigned int            clkreg;
> @@ -98,6 +99,7 @@ struct variant_data {
>         bool                    mclk_delayed_writes;
>         bool                    explicit_mclk_control;
>         bool                    cclk_is_mclk;
> +       bool                    qcom_fifo;
>  };
>
>  static struct variant_data variant_arm = {
> @@ -208,6 +210,7 @@ static struct variant_data variant_qcom = {
>         .mclk_delayed_writes    = true,
>         .explicit_mclk_control  = true,
>         .cclk_is_mclk   = true,
> +       .qcom_fifo              = true,
>  };
>
>  static inline u32 mmci_readl(struct mmci_host *host, u32 off)
> @@ -1022,6 +1025,40 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>         }
>  }
>
> +static int mmci_qcom_pio_read(struct mmci_host *host, char *buffer,
> +                       unsigned int remain)
> +{
> +       u32 *ptr = (u32 *) buffer;
> +       unsigned int count = 0;
> +       unsigned int words, bytes;
> +       unsigned int fsize = host->variant->fifosize;
> +
> +       words = remain >> 2;
> +       bytes = remain % 4;
> +       /* read full words followed by leftover bytes */
> +       if (words) {
> +               while (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) {
> +                       *ptr = readl(host->base + MMCIFIFO + (count % fsize));
> +                       ptr++;
> +                       count += 4;
> +                       words--;
> +                       if (!words)
> +                               break;
> +               }
> +       }
> +
> +       if (unlikely(bytes)) {
> +               unsigned char buf[4];
> +               if (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) {
> +                       *buf = readl(host->base + MMCIFIFO + (count % fsize));
> +                       memcpy(ptr, buf, bytes);
> +                       count += bytes;
> +               }
> +       }
> +
> +       return count;
> +}
> +
>  static int mmci_pio_read(struct mmci_host *host, char *buffer, unsigned int remain)
>  {
>         void __iomem *base = host->base;
> @@ -1143,8 +1180,13 @@ static irqreturn_t mmci_pio_irq(int irq, void *dev_id)
>                 remain = sg_miter->length;
>
>                 len = 0;
> -               if (status & MCI_RXACTIVE)
> -                       len = mmci_pio_read(host, buffer, remain);
> +               if (status & MCI_RXACTIVE) {
> +                       if (variant->qcom_fifo)
> +                               len = mmci_qcom_pio_read(host, buffer, remain);
> +                       else
> +                               len = mmci_pio_read(host, buffer, remain);
> +               }

This is hot path.

As I suggested for the readl and writel wrapper functions, I think it
would be better to use a function pointer in the struct mmci host,
which you set up in the probe phase. That means the variant data don't
need to be checked each an every time this function gets invoked.

> +
>                 if (status & MCI_TXACTIVE)
>                         len = mmci_pio_write(host, buffer, remain, status);

So no changes needed for pio_write at this point? Or those will come later?

>
> --
> 1.9.1
>

Kind regards
Ulf Hansson

Re: [PATCH v3 03/13] mmc: mmci: Add Qualcomm Id to amba id table

[Srinivas Kandagatla]

Hi Ulf,

On 26/05/14 10:10, Ulf Hansson wrote:
> Hi Srinivas,
>
>>
>> +static struct variant_data variant_qcom = {
>> +       .fifosize               = 16 * 4,
>> +       .fifohalfsize           = 8 * 4,
>> +       .clkreg                 = MCI_CLK_ENABLE,
>> +       .datalength_bits        = 24,
>> +       .blksz_datactrl4        = true,
>
> You get compile error here.
yes, You are right, I will reorder this patch after the "mmc: mmci: Add 
Qcom datactrl register variant"


Thanks,
srini
>
> Kind regards
> Ulf Hansson
>

Re: [PATCH v3 06/13] mmc: mmci: Qcomm: Add 3 clock cycle delay after register write

[Srinivas Kandagatla]

>
> I am not sure I like this approach. For each and every writel
> (including pio_writes) you will add a few cpu cycles, since you need
> to  check for "mclk_delayed_writes" no matter of variant.
>

> How about, adding a new function pointer in the struct mmci_host, for
> "writel operations" which you could set up in probe phase instead?
>

Yes, this is an additional check for other variants. I will try the 
function pointer method that you suggested.

Thanks,
srini


> Kind regards
> Ulf Hansson
>

Re: [PATCH v3 07/13] mmc: mmci: add ddrmode mask to variant data

[Srinivas Kandagatla]

On 26/05/14 10:53, Ulf Hansson wrote:
> On 23 May 2014 14:51,  <srinivas.kandagatla@linaro.org> wrote:
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> This patch adds ddrmode mask to variant structure giving more flexibility
>> to the driver to support more SOCs which have different datactrl register
>> layout.
>>
>> Without this patch datactrl register is updated with wrong ddrmode mask on non
>> ST SOCs, resulting in card detection failures.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>>   drivers/mmc/host/mmci.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index 1385554..dec70d2 100644
>> --- a/drivers/mmc/host/mmci.c
>> +++ b/drivers/mmc/host/mmci.c
>> @@ -59,6 +59,7 @@ static unsigned int fmax = 515633;
>>    *               is asserted (likewise for RX)
>>    * @sdio: variant supports SDIO
>>    * @st_clkdiv: true if using a ST-specific clock divider algorithm
>> + * @datactrl_mask_ddrmode: ddr mode mask in datactrl register.
>>    * @blksz_datactrl16: true if Block size is at b16..b30 position in datactrl register
>>    * @blksz_datactrl4: true if Block size is at b4..b16 position in datactrl
>>    *                  register
>> @@ -76,6 +77,7 @@ struct variant_data {
>>          unsigned int            datalength_bits;
>>          unsigned int            fifosize;
>>          unsigned int            fifohalfsize;
>> +       unsigned int            datactrl_mask_ddrmode;
>>          bool                    sdio;
>>          bool                    st_clkdiv;
>>          bool                    blksz_datactrl16;
>> @@ -114,6 +116,7 @@ static struct variant_data variant_u300 = {
>>          .fifosize               = 16 * 4,
>>          .fifohalfsize           = 8 * 4,
>>          .clkreg_enable          = MCI_ST_U300_HWFCEN,
>> +       .datactrl_mask_ddrmode  = MCI_ST_DPSM_DDRMODE,
>>          .datalength_bits        = 16,
>>          .sdio                   = true,
>>          .pwrreg_powerup         = MCI_PWR_ON,
>> @@ -126,6 +129,7 @@ static struct variant_data variant_nomadik = {
>>          .fifosize               = 16 * 4,
>>          .fifohalfsize           = 8 * 4,
>>          .clkreg                 = MCI_CLK_ENABLE,
>> +       .datactrl_mask_ddrmode  = MCI_ST_DPSM_DDRMODE,
>>          .datalength_bits        = 24,
>>          .sdio                   = true,
>>          .st_clkdiv              = true,
>> @@ -140,6 +144,7 @@ static struct variant_data variant_ux500 = {
>>          .fifohalfsize           = 8 * 4,
>>          .clkreg                 = MCI_CLK_ENABLE,
>>          .clkreg_enable          = MCI_ST_UX500_HWFCEN,
>> +       .datactrl_mask_ddrmode  = MCI_ST_DPSM_DDRMODE,
>>          .datalength_bits        = 24,
>>          .sdio                   = true,
>>          .st_clkdiv              = true,
>> @@ -155,6 +160,7 @@ static struct variant_data variant_ux500v2 = {
>>          .fifohalfsize           = 8 * 4,
>>          .clkreg                 = MCI_CLK_ENABLE,
>>          .clkreg_enable          = MCI_ST_UX500_HWFCEN,
>> +       .datactrl_mask_ddrmode  = MCI_ST_DPSM_DDRMODE,
>
> Only the ux500v2 supports DDR mode, using the MCI_ST_DPSM_DDRMODE bit.

Thanks for pointing this out, I was not sure which variants actually 
used this bit, so just replicated what was there before this change.

I will add this flag to only ux500v2 in next version.


Thanks,
srini
>
>>          .datalength_bits        = 24,
>>          .sdio                   = true,
>>          .st_clkdiv              = true,
>> @@ -800,7 +806,7 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
>>                  }
>>
>>          if (host->mmc->ios.timing == MMC_TIMING_UHS_DDR50)
>> -               datactrl |= MCI_ST_DPSM_DDRMODE;
>> +               datactrl |= variant->datactrl_mask_ddrmode;
>>
>>          /*
>>           * Attempt to use DMA operation mode, if this
>> --
>> 1.9.1
>>
>
> Kind regards
> Ulf Hansson
>

Re: [PATCH v3 13/13] mmc: mmci: Add Qcom specific pio_read function.

[Srinivas Kandagatla]

On 26/05/14 15:34, Ulf Hansson wrote:
> This is hot path.
>
> As I suggested for the readl and writel wrapper functions, I think it
> would be better to use a function pointer in the struct mmci host,
> which you set up in the probe phase. That means the variant data don't
> need to be checked each an every time this function gets invoked.
>
>> >+
>> >                 if (status & MCI_TXACTIVE)
>> >                         len = mmci_pio_write(host, buffer, remain, status);
> So no changes needed for pio_write at this point? Or those will come later?
>

Yes, that sounds like more sensible approach to me. I will do this 
change in next version.

Thanks,
srini
>> >
>> >--

Re: [PATCH v3 10/13] mmc: mmci: add Qcom specifics of clk and datactrl registers.

[Srinivas Kandagatla]

Hi Ulf,

Thanks for the comments.

On 26/05/14 14:05, Ulf Hansson wrote:
> On 23 May 2014 14:52,  <srinivas.kandagatla@linaro.org> wrote:
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> This patch adds specifics of clk and datactrl register on Qualcomm SD
>> Card controller. This patch also populates the Qcom variant data with
>> these new values specific to Qualcomm SD Card Controller.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>>   drivers/mmc/host/mmci.c |  4 ++++
>>   drivers/mmc/host/mmci.h | 24 ++++++++++++++++++++++++
>>   2 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index 17e7f6a..6434f5b1 100644
>> --- a/drivers/mmc/host/mmci.c
>> +++ b/drivers/mmc/host/mmci.c
>> @@ -185,6 +185,10 @@ static struct variant_data variant_qcom = {
>>          .fifosize               = 16 * 4,
>>          .fifohalfsize           = 8 * 4,
>>          .clkreg                 = MCI_CLK_ENABLE,
>> +       .clkreg_enable          = MCI_QCOM_CLK_FLOWENA |
>> +                                 MCI_QCOM_CLK_FEEDBACK_CLK,
>
> Obviously I don't have the in-depth knowledge about the Qcom variant,
> but comparing the ST variant here made me think.
>
> Using the feeback clock internal logic in the ST variant, requires the
> corresponding feedback clock pin signal on the board, to be
> routed/connected. Typically we used this for SD cards, which involved
> using an external level shifter circuit.
>
> Is it correct to enable this bit for all cases, including eMMC?
>
You are correct, FBCLK should specific to the board, and I will try to 
do something on the same lines as ST variant in next version.

> If it is board specific configurations, you should add a DT binding
> for it - like there are for the ST variant.
>
>> +       .clkreg_8bit_bus_enable = MCI_QCOM_CLK_WIDEBUS_8,
>> +       .datactrl_mask_ddrmode  = MCI_QCOM_CLK_DDR_MODE,
>>          .blksz_datactrl4        = true,
>>          .datalength_bits        = 24,
>>          .blksz_datactrl4        = true,
>> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
>> index cd83ca3..1b93ae7 100644
>> --- a/drivers/mmc/host/mmci.h
>> +++ b/drivers/mmc/host/mmci.h
>> @@ -41,6 +41,22 @@
>>   /* Modified PL180 on Versatile Express platform */
>>   #define MCI_ARM_HWFCEN         BIT(12)
>>
>> +/* Modified on Qualcomm Integrations */
>> +#define MCI_QCOM_CLK_WIDEBUS_4 (2 << 10)
>
> This is the same as BIT(11), please use MCI_4BIT_BUS instead.
>
This is not used in the code, I will clean it up as you suggested, just 
to be more consistent.

>> +#define MCI_QCOM_CLK_WIDEBUS_8 (3 << 10)
>
> Since you converted to use the "BIT" macro a few patches ago, I
> suggest we should stick to it. How about something below:
>
> #define MCI_QCOM_CLK_WIDEBUS_8 BIT (BIT(10) | BIT(11))
>
Sounds good, I will fix all such instances in next version.

> Please adopt all defines added in this patch to use the BIT macro.
>
>> +#define MCI_QCOM_CLK_FLOWENA   BIT(12)
>> +#define MCI_QCOM_CLK_INVERTOUT BIT(13)
>> +
>> +/* select in latch data and command */
>> +#define MCI_QCOM_CLK_SEL_IN_SHIFT      (14)
>
> BIT (14)?
>
>> +#define MCI_QCOM_CLK_SEL_MASK          (0x3)
>> +#define MCI_QCOM_CLK_SEL_RISING_EDGE   (1)
>
> BIT(1)?
>
>> +#define MCI_QCOM_CLK_FEEDBACK_CLK      (2 << 14)
>> +#define MCI_QCOM_CLK_DDR_MODE          (3 << 14)
>> +
>> +/* mclk selection */
>> +#define MCI_QCOM_CLK_SEL_MCLK          (2 << 23)
>
> Does this correspond to MCI_CLK_BYPASS? If so, we should maybe state
> this in a comment?
>
No, this is not same as MCI_CLK_BYPASS, its selection between 
FBCLK/gated MCLK/freerunning MCLK.

Thanks,
srini

Re: [PATCH v3 12/13] mmc: mmci: add explicit clk control

[Srinivas Kandagatla]

Hi Ulf,
Thankyou for the comments.

On 26/05/14 15:21, Ulf Hansson wrote:
> On 23 May 2014 14:52,  <srinivas.kandagatla@linaro.org> wrote:
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> On Controllers like Qcom SD card controller where cclk is mclk and mclk should
>> be directly controlled by the driver.
>>
>> This patch adds support to control mclk directly in the driver, and also
>> adds explicit_mclk_control and cclk_is_mclk flags in variant structure giving
>> more flexibility to the driver.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>>   drivers/mmc/host/mmci.c | 30 +++++++++++++++++++++++++-----
>>   1 file changed, 25 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index 5cbf644..f6dfd24 100644
>> --- a/drivers/mmc/host/mmci.c
>> +++ b/drivers/mmc/host/mmci.c
>> @@ -73,6 +73,8 @@ static unsigned int fmax = 515633;
>>    * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply
>>    * @mclk_delayed_writes: enable delayed writes to ensure, subsequent updates
>>    *                      are not ignored.
>> + * @explicit_mclk_control: enable explicit mclk control in driver.
>> + * @cclk_is_mclk: enable iff card clock is multimedia card adapter clock.
>>    */
>>   struct variant_data {
>>          unsigned int            clkreg;
>> @@ -94,6 +96,8 @@ struct variant_data {
>>          bool                    busy_detect;
>>          bool                    pwrreg_nopower;
>>          bool                    mclk_delayed_writes;
>> +       bool                    explicit_mclk_control;
>> +       bool                    cclk_is_mclk;
>
> I can't see why you need to have both these new configurations. Aren't
> "cclk_is_mclk" just a fact when you use "explicit_mclk_control".
>


> I also believe I would prefer something like "qcom_clkdiv" instead.

There is a subtle difference between both the flags.  Am happy to change 
it to qcom_clkdiv.

>
>>   };
>>
>>   static struct variant_data variant_arm = {
>> @@ -202,6 +206,8 @@ static struct variant_data variant_qcom = {
>>           * for 3 clk cycles.
>>           */
>>          .mclk_delayed_writes    = true,
>> +       .explicit_mclk_control  = true,
>> +       .cclk_is_mclk   = true,
>>   };
>>
>>   static inline u32 mmci_readl(struct mmci_host *host, u32 off)
>> @@ -317,7 +323,9 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)
>>          host->cclk = 0;
>>
>>          if (desired) {
>> -               if (desired >= host->mclk) {
>> +               if (variant->cclk_is_mclk) {
>> +                       host->cclk = host->mclk;
>> +               } else if (desired >= host->mclk) {
>>                          clk = MCI_CLK_BYPASS;
>>                          if (variant->st_clkdiv)
>>                                  clk |= MCI_ST_UX500_NEG_EDGE;
>> @@ -1354,6 +1362,16 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>          if (!ios->clock && variant->pwrreg_clkgate)
>>                  pwr &= ~MCI_PWR_ON;
>>
>> +       if (ios->clock != host->mclk && host->variant->explicit_mclk_control) {
>
> I suggest you should clarify the statement by adding a pair of extra
> parentheses. Additionally it seems like a good idea to reverse the
> order of the statements, to clarify this is for qcom clock handling
> only.
Yes, sure Will fix this in next version.

>
> More important, what I think you really want to do is to compare
> "ios->clock" with it's previous value it had when ->set_ios were
> invoked. Then let a changed value act as the trigger to set a new clk
> rate. Obvoiusly you need to cache the clock rate in the struct mmci
> host to handle this.

host->mclk already has this cached value.

>
>> +               int rc = clk_set_rate(host->clk, ios->clock);
>> +               if (rc < 0) {
>> +                       dev_err(mmc_dev(host->mmc),
>> +                               "Error setting clock rate (%d)\n", rc);
>> +               } else {
>> +                       host->mclk = clk_get_rate(host->clk);
>
> So here you actually find out the new clk rate, but shouldn't you
> update "host->mclk" within the spin_lock? Or it might not matter?
>
I think it does not matter in this case, as this is the only place mclk 
gets modified.
>> +               }
>> +       }
>> +
>>          spin_lock_irqsave(&host->lock, flags);
>>
>>          mmci_set_clkreg(host, ios->clock);
>> @@ -1540,10 +1558,12 @@ static int mmci_probe(struct amba_device *dev,
>>           * is not specified. Either value must not exceed the clock rate into
>>           * the block, of course.
>>           */
>> -       if (mmc->f_max)
>> -               mmc->f_max = min(host->mclk, mmc->f_max);
>> -       else
>> -               mmc->f_max = min(host->mclk, fmax);
>> +       if (!host->variant->explicit_mclk_control) {
>> +               if (mmc->f_max)
>> +                       mmc->f_max = min(host->mclk, mmc->f_max);
>> +               else
>> +                       mmc->f_max = min(host->mclk, fmax);
>> +       }
>
> This means your mmc->f_max value will either be zero or the one you
> provided through DT. And since zero won't work, that means you
> _require_ to get the value from DT. According to the documentation of
> this DT binding, f_max is optional.
>
> So unless you fine another way of dynamically at runtime figure out
> the value of f_max (using the clk API), you need to update the DT
> documentation for mmci.
>

You are right there is a possibility of f_max to be zero.

This logic could fix it.

if (host->variant->explicit_mclk_control) {
	if (mmc->f_max)
		mmc->f_max = max(host->mclk, mmc->f_max);
	else
		mmc->f_max = max(host->mclk, fmax);
} else {
	if (mmc->f_max)
		mmc->f_max = min(host->mclk, mmc->f_max);
	else
		mmc->f_max = min(host->mclk, fmax);
}



> Additionally, this makes me wonder about f_min. I haven't seen
> anywhere in this patch were that value is being set to proper value,
> right?
>

f_min should be 400000 for qcom, I think with the default mclk frequency 
and a divider of 512 used for calculating the f_min is bringing down the 
f_min to lessthan 400Kz. Which is why its working fine.
I think the possibility of mclk default frequency being greater than 
208Mhz is very less. so I could either leave it as it is Or force this 
to 400000 all the time for qcom chips.

Am ok either way..

Thanks,
srini

>>          dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max);
>>
>>          /* Get regulators and the supported OCR mask */
>> --
>> 1.9.1
>>
>
> Kind regards
> Ulf Hansson
>

Re: [PATCH v3 12/13] mmc: mmci: add explicit clk control

[Ulf Hansson]

On 27 May 2014 00:39, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
> Hi Ulf,
> Thankyou for the comments.
>
>
> On 26/05/14 15:21, Ulf Hansson wrote:
>>
>> On 23 May 2014 14:52,  <srinivas.kandagatla@linaro.org> wrote:
>>>
>>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>>
>>> On Controllers like Qcom SD card controller where cclk is mclk and mclk
>>> should
>>> be directly controlled by the driver.
>>>
>>> This patch adds support to control mclk directly in the driver, and also
>>> adds explicit_mclk_control and cclk_is_mclk flags in variant structure
>>> giving
>>> more flexibility to the driver.
>>>
>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>> ---
>>>   drivers/mmc/host/mmci.c | 30 +++++++++++++++++++++++++-----
>>>   1 file changed, 25 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>>> index 5cbf644..f6dfd24 100644
>>> --- a/drivers/mmc/host/mmci.c
>>> +++ b/drivers/mmc/host/mmci.c
>>> @@ -73,6 +73,8 @@ static unsigned int fmax = 515633;
>>>    * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply
>>>    * @mclk_delayed_writes: enable delayed writes to ensure, subsequent
>>> updates
>>>    *                      are not ignored.
>>> + * @explicit_mclk_control: enable explicit mclk control in driver.
>>> + * @cclk_is_mclk: enable iff card clock is multimedia card adapter
>>> clock.
>>>    */
>>>   struct variant_data {
>>>          unsigned int            clkreg;
>>> @@ -94,6 +96,8 @@ struct variant_data {
>>>          bool                    busy_detect;
>>>          bool                    pwrreg_nopower;
>>>          bool                    mclk_delayed_writes;
>>> +       bool                    explicit_mclk_control;
>>> +       bool                    cclk_is_mclk;
>>
>>
>> I can't see why you need to have both these new configurations. Aren't
>> "cclk_is_mclk" just a fact when you use "explicit_mclk_control".
>>
>
>
>> I also believe I would prefer something like "qcom_clkdiv" instead.
>
>
> There is a subtle difference between both the flags.  Am happy to change it
> to qcom_clkdiv.
>
>
>>
>>>   };
>>>
>>>   static struct variant_data variant_arm = {
>>> @@ -202,6 +206,8 @@ static struct variant_data variant_qcom = {
>>>           * for 3 clk cycles.
>>>           */
>>>          .mclk_delayed_writes    = true,
>>> +       .explicit_mclk_control  = true,
>>> +       .cclk_is_mclk   = true,
>>>   };
>>>
>>>   static inline u32 mmci_readl(struct mmci_host *host, u32 off)
>>> @@ -317,7 +323,9 @@ static void mmci_set_clkreg(struct mmci_host *host,
>>> unsigned int desired)
>>>          host->cclk = 0;
>>>
>>>          if (desired) {
>>> -               if (desired >= host->mclk) {
>>> +               if (variant->cclk_is_mclk) {
>>> +                       host->cclk = host->mclk;
>>> +               } else if (desired >= host->mclk) {
>>>                          clk = MCI_CLK_BYPASS;
>>>                          if (variant->st_clkdiv)
>>>                                  clk |= MCI_ST_UX500_NEG_EDGE;
>>> @@ -1354,6 +1362,16 @@ static void mmci_set_ios(struct mmc_host *mmc,
>>> struct mmc_ios *ios)
>>>          if (!ios->clock && variant->pwrreg_clkgate)
>>>                  pwr &= ~MCI_PWR_ON;
>>>
>>> +       if (ios->clock != host->mclk &&
>>> host->variant->explicit_mclk_control) {
>>
>>
>> I suggest you should clarify the statement by adding a pair of extra
>> parentheses. Additionally it seems like a good idea to reverse the
>> order of the statements, to clarify this is for qcom clock handling
>> only.
>
> Yes, sure Will fix this in next version.
>
>
>>
>> More important, what I think you really want to do is to compare
>> "ios->clock" with it's previous value it had when ->set_ios were
>> invoked. Then let a changed value act as the trigger to set a new clk
>> rate. Obvoiusly you need to cache the clock rate in the struct mmci
>> host to handle this.
>
>
> host->mclk already has this cached value.

There are no guarantees clk_set_rate() will manage to set the exact
requested rate as ios->clock. At least if you follow the clk API
documentation.

>
>
>>
>>> +               int rc = clk_set_rate(host->clk, ios->clock);
>>> +               if (rc < 0) {
>>> +                       dev_err(mmc_dev(host->mmc),
>>> +                               "Error setting clock rate (%d)\n", rc);
>>> +               } else {
>>> +                       host->mclk = clk_get_rate(host->clk);
>>
>>
>> So here you actually find out the new clk rate, but shouldn't you
>> update "host->mclk" within the spin_lock? Or it might not matter?
>>
> I think it does not matter in this case, as this is the only place mclk gets
> modified.

OK.

>
>>> +               }
>>> +       }
>>> +
>>>          spin_lock_irqsave(&host->lock, flags);
>>>
>>>          mmci_set_clkreg(host, ios->clock);
>>> @@ -1540,10 +1558,12 @@ static int mmci_probe(struct amba_device *dev,
>>>           * is not specified. Either value must not exceed the clock rate
>>> into
>>>           * the block, of course.
>>>           */
>>> -       if (mmc->f_max)
>>> -               mmc->f_max = min(host->mclk, mmc->f_max);
>>> -       else
>>> -               mmc->f_max = min(host->mclk, fmax);
>>> +       if (!host->variant->explicit_mclk_control) {
>>> +               if (mmc->f_max)
>>> +                       mmc->f_max = min(host->mclk, mmc->f_max);
>>> +               else
>>> +                       mmc->f_max = min(host->mclk, fmax);
>>> +       }
>>
>>
>> This means your mmc->f_max value will either be zero or the one you
>> provided through DT. And since zero won't work, that means you
>> _require_ to get the value from DT. According to the documentation of
>> this DT binding, f_max is optional.
>>
>> So unless you fine another way of dynamically at runtime figure out
>> the value of f_max (using the clk API), you need to update the DT
>> documentation for mmci.
>>
>
> You are right there is a possibility of f_max to be zero.
>
> This logic could fix it.
>
> if (host->variant->explicit_mclk_control) {
>         if (mmc->f_max)
>                 mmc->f_max = max(host->mclk, mmc->f_max);
>         else
>                 mmc->f_max = max(host->mclk, fmax);
> } else {
>         if (mmc->f_max)
>
>                 mmc->f_max = min(host->mclk, mmc->f_max);
>         else
>
>                 mmc->f_max = min(host->mclk, fmax);
> }
>

Hmm. Looking a bit deeper into this, we have some additional related
code to fixup. :-)

In ->probe(), we do clk_set_rate(100MHz), if the mclk > 100MHz. That's
due to the current variants don't support higher frequency than this.
It seems like the Qcom variant may support up to 208MHz? Now, if
that's the case, we need to add "f_max" to the struct variant_data to
store this information, so we can respect different values while doing
clk_set_rate() at ->probe().

While updating mmc->f_max for host->variant->explicit_mclk_control, we
shouldn't care about using the host->mclk as a limiter, instead, use
min(mmc->f_max, host->variant->f_max) and fallback to fmax.

Not sure how that will affect the logic. :-)

>
>
>> Additionally, this makes me wonder about f_min. I haven't seen
>> anywhere in this patch were that value is being set to proper value,
>> right?
>>
>
> f_min should be 400000 for qcom, I think with the default mclk frequency and
> a divider of 512 used for calculating the f_min is bringing down the f_min
> to lessthan 400Kz. Which is why its working fine.
> I think the possibility of mclk default frequency being greater than 208Mhz
> is very less. so I could either leave it as it is Or force this to 400000
> all the time for qcom chips.

No, this seems like a wrong approach.

I think you would like to do use the clk_round_rate() find out the
lowest possible rate. Or just use a fixed fallback value somehow.

Kind regards
Ulf Hansson

Re: [PATCH v3 12/13] mmc: mmci: add explicit clk control

[Srinivas Kandagatla]

On 27/05/14 10:32, Ulf Hansson wrote:
> On 27 May 2014 00:39, Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>> Hi Ulf,
>> Thankyou for the comments.
>>
>>
>> On 26/05/14 15:21, Ulf Hansson wrote:
>>>
>>> On 23 May 2014 14:52,  <srinivas.kandagatla@linaro.org> wrote:
>>>>
>>>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>>>
>>>> On Controllers like Qcom SD card controller where cclk is mclk and mclk
>>>> should
>>>> be directly controlled by the driver.
>>>>
...

>>>>            * for 3 clk cycles.
>>>>            */
>>>>           .mclk_delayed_writes    = true,
>>>> +       .explicit_mclk_control  = true,
>>>> +       .cclk_is_mclk   = true,
>>>>    };
>>>>
>>>>    static inline u32 mmci_readl(struct mmci_host *host, u32 off)
>>>> @@ -317,7 +323,9 @@ static void mmci_set_clkreg(struct mmci_host *host,
>>>> unsigned int desired)
>>>>           host->cclk = 0;
>>>>
>>>>           if (desired) {
>>>> -               if (desired >= host->mclk) {
>>>> +               if (variant->cclk_is_mclk) {
>>>> +                       host->cclk = host->mclk;
>>>> +               } else if (desired >= host->mclk) {
>>>>                           clk = MCI_CLK_BYPASS;
>>>>                           if (variant->st_clkdiv)
>>>>                                   clk |= MCI_ST_UX500_NEG_EDGE;
>>>> @@ -1354,6 +1362,16 @@ static void mmci_set_ios(struct mmc_host *mmc,
>>>> struct mmc_ios *ios)
>>>>           if (!ios->clock && variant->pwrreg_clkgate)
>>>>                   pwr &= ~MCI_PWR_ON;
>>>>
>>>> +       if (ios->clock != host->mclk &&
>>>> host->variant->explicit_mclk_control) {
>>>
>>>
>>> I suggest you should clarify the statement by adding a pair of extra
>>> parentheses. Additionally it seems like a good idea to reverse the
>>> order of the statements, to clarify this is for qcom clock handling
>>> only.
>>
>> Yes, sure Will fix this in next version.
>>
>>
>>>
>>> More important, what I think you really want to do is to compare
>>> "ios->clock" with it's previous value it had when ->set_ios were
>>> invoked. Then let a changed value act as the trigger to set a new clk
>>> rate. Obvoiusly you need to cache the clock rate in the struct mmci
>>> host to handle this.
>>
>>
>> host->mclk already has this cached value.
>
> There are no guarantees clk_set_rate() will manage to set the exact
> requested rate as ios->clock. At least if you follow the clk API
> documentation.
>

Yes, I agree. caching ios->clock looks like the best option.

>>
>>
>>>

...

>>
>>>> +               }
>>>> +       }
>>>> +
>>>>           spin_lock_irqsave(&host->lock, flags);
>>>>
>>>>           mmci_set_clkreg(host, ios->clock);
>>>> @@ -1540,10 +1558,12 @@ static int mmci_probe(struct amba_device *dev,
>>>>            * is not specified. Either value must not exceed the clock rate
>>>> into
>>>>            * the block, of course.
>>>>            */
>>>> -       if (mmc->f_max)
>>>> -               mmc->f_max = min(host->mclk, mmc->f_max);
>>>> -       else
>>>> -               mmc->f_max = min(host->mclk, fmax);
>>>> +       if (!host->variant->explicit_mclk_control) {
>>>> +               if (mmc->f_max)
>>>> +                       mmc->f_max = min(host->mclk, mmc->f_max);
>>>> +               else
>>>> +                       mmc->f_max = min(host->mclk, fmax);
>>>> +       }
>>>
>>>
>>> This means your mmc->f_max value will either be zero or the one you
>>> provided through DT. And since zero won't work, that means you
>>> _require_ to get the value from DT. According to the documentation of
>>> this DT binding, f_max is optional.
>>>
>>> So unless you fine another way of dynamically at runtime figure out
>>> the value of f_max (using the clk API), you need to update the DT
>>> documentation for mmci.
>>>
>>
>> You are right there is a possibility of f_max to be zero.
>>
>> This logic could fix it.
>>
>> if (host->variant->explicit_mclk_control) {
>>          if (mmc->f_max)
>>                  mmc->f_max = max(host->mclk, mmc->f_max);
>>          else
>>                  mmc->f_max = max(host->mclk, fmax);
>> } else {
>>          if (mmc->f_max)
>>
>>                  mmc->f_max = min(host->mclk, mmc->f_max);
>>          else
>>
>>                  mmc->f_max = min(host->mclk, fmax);
>> }
>>
>
> Hmm. Looking a bit deeper into this, we have some additional related
> code to fixup. :-)
>
> In ->probe(), we do clk_set_rate(100MHz), if the mclk > 100MHz. That's
> due to the current variants don't support higher frequency than this.
> It seems like the Qcom variant may support up to 208MHz? Now, if
> that's the case, we need to add "f_max" to the struct variant_data to
> store this information, so we can respect different values while doing
> clk_set_rate() at ->probe().
>
Yes, qcom SOCs support more than 100Hhz clock.

Probe and clk_set_rate/set_ios should respect this.

On the other thought, Should probe actually care about clocks which are 
explicitly controlled? It should not even attempt to set any frequency 
to start with. mmc-core would set the right frequency depending on the 
mmc state-machine respecting f_min and f_max.
I think for qcom, probe should just check the if f_max and f_min are 
valid and set them to defaults if any in the same lines as existing code.

> While updating mmc->f_max for host->variant->explicit_mclk_control, we
> shouldn't care about using the host->mclk as a limiter, instead, use
> min(mmc->f_max, host->variant->f_max) and fallback to fmax.
>
Yes, that's correct, mclk should not be used as limiter. Adding f_max to 
the variant looks useful.

> Not sure how that will affect the logic. :-)
>
>>
>>
>>> Additionally, this makes me wonder about f_min. I haven't seen
>>> anywhere in this patch were that value is being set to proper value,
>>> right?
>>>
>>
>> f_min should be 400000 for qcom, I think with the default mclk frequency and
>> a divider of 512 used for calculating the f_min is bringing down the f_min
>> to lessthan 400Kz. Which is why its working fine.
>> I think the possibility of mclk default frequency being greater than 208Mhz
>> is very less. so I could either leave it as it is Or force this to 400000
>> all the time for qcom chips.
>
> No, this seems like a wrong approach.
>
> I think you would like to do use the clk_round_rate() find out the
> lowest possible rate. Or just use a fixed fallback value somehow.

clk_round_rate(mclk, 0) might be more generic, instead of fixed 
fallback. Let the mmc-core figure out what frequency would be best from 
its table starting from f_min.

thanks,
srini
>
> Kind regards
> Ulf Hansson
>

Re: [PATCH v3 03/13] mmc: mmci: Add Qualcomm Id to amba id table

[Srinivas Kandagatla]

On 26/05/14 18:00, Srinivas Kandagatla wrote:
> Hi Ulf,
>
> On 26/05/14 10:10, Ulf Hansson wrote:
>> Hi Srinivas,
>>
>>>
>>> +static struct variant_data variant_qcom = {
>>> +       .fifosize               = 16 * 4,
>>> +       .fifohalfsize           = 8 * 4,
>>> +       .clkreg                 = MCI_CLK_ENABLE,
>>> +       .datalength_bits        = 24,
>>> +       .blksz_datactrl4        = true,
>>
>> You get compile error here.
> yes, You are right, I will reorder this patch after the "mmc: mmci: Add
> Qcom datactrl register variant"

Actually, blksz_datactrl4 should not be in this patch. it is part of 
"mmc: mmci: Add Qcom datactrl register variant"  will remove it from 
this patch in next version.

--srini
>
>
> Thanks,
> srini
>>
>> Kind regards
>> Ulf Hansson
>>

Re: [PATCH v3 12/13] mmc: mmci: add explicit clk control

[Ulf Hansson]

>> Hmm. Looking a bit deeper into this, we have some additional related
>> code to fixup. :-)
>>
>> In ->probe(), we do clk_set_rate(100MHz), if the mclk > 100MHz. That's
>> due to the current variants don't support higher frequency than this.
>> It seems like the Qcom variant may support up to 208MHz? Now, if
>> that's the case, we need to add "f_max" to the struct variant_data to
>> store this information, so we can respect different values while doing
>> clk_set_rate() at ->probe().
>>
> Yes, qcom SOCs support more than 100Hhz clock.
>
> Probe and clk_set_rate/set_ios should respect this.
>
> On the other thought, Should probe actually care about clocks which are
> explicitly controlled? It should not even attempt to set any frequency to
> start with.

The 100 MHz is related to constraints set by the specification of the
IP block, not the MMC/SD/SDIO spec.

Thus at ->probe() we must perform the clk_set_rate().

> mmc-core would set the right frequency depending on the mmc
> state-machine respecting f_min and f_max.
> I think for qcom, probe should just check the if f_max and f_min are valid
> and set them to defaults if any in the same lines as existing code.
>
>
>> While updating mmc->f_max for host->variant->explicit_mclk_control, we
>> shouldn't care about using the host->mclk as a limiter, instead, use
>> min(mmc->f_max, host->variant->f_max) and fallback to fmax.
>>
> Yes, that's correct, mclk should not be used as limiter. Adding f_max to the
> variant looks useful.
>
>
>> Not sure how that will affect the logic. :-)
>>
>>>
>>>
>>>> Additionally, this makes me wonder about f_min. I haven't seen
>>>> anywhere in this patch were that value is being set to proper value,
>>>> right?
>>>>
>>>
>>> f_min should be 400000 for qcom, I think with the default mclk frequency
>>> and
>>> a divider of 512 used for calculating the f_min is bringing down the
>>> f_min
>>> to lessthan 400Kz. Which is why its working fine.
>>> I think the possibility of mclk default frequency being greater than
>>> 208Mhz
>>> is very less. so I could either leave it as it is Or force this to 400000
>>> all the time for qcom chips.
>>
>>
>> No, this seems like a wrong approach.
>>
>> I think you would like to do use the clk_round_rate() find out the
>> lowest possible rate. Or just use a fixed fallback value somehow.
>
>
> clk_round_rate(mclk, 0) might be more generic, instead of fixed fallback.
> Let the mmc-core figure out what frequency would be best from its table
> starting from f_min.

Agree!

clk_round_rate(mclk, 100KHz), might be better though - since that is
actually the lowest request frequency whatsoever.

Kind regards
Ulf Hansson

Re: [PATCH v3 12/13] mmc: mmci: add explicit clk control

[Srinivas Kandagatla]

On 27/05/14 15:07, Ulf Hansson wrote:
>>> Hmm. Looking a bit deeper into this, we have some additional related
>>> code to fixup. :-)
>>>
>>> In ->probe(), we do clk_set_rate(100MHz), if the mclk > 100MHz. That's
>>> due to the current variants don't support higher frequency than this.
>>> It seems like the Qcom variant may support up to 208MHz? Now, if
>>> that's the case, we need to add "f_max" to the struct variant_data to
>>> store this information, so we can respect different values while doing
>>> clk_set_rate() at ->probe().
>>>
>> Yes, qcom SOCs support more than 100Hhz clock.
>>
>> Probe and clk_set_rate/set_ios should respect this.
>>
>> On the other thought, Should probe actually care about clocks which are
>> explicitly controlled? It should not even attempt to set any frequency to
>> start with.
>
> The 100 MHz is related to constraints set by the specification of the
> IP block, not the MMC/SD/SDIO spec.
>
> Thus at ->probe() we must perform the clk_set_rate().
I agree its valid for controllers which have this constraints.

>
>> mmc-core would set the right frequency depending on the mmc
>> state-machine respecting f_min and f_max.
>> I think for qcom, probe should just check the if f_max and f_min are valid
>> and set them to defaults if any in the same lines as existing code.
>>
>>
>>> While updating mmc->f_max for host->variant->explicit_mclk_control, we
>>> shouldn't care about using the host->mclk as a limiter, instead, use
>>> min(mmc->f_max, host->variant->f_max) and fallback to fmax.
>>>
>> Yes, that's correct, mclk should not be used as limiter. Adding f_max to the
>> variant looks useful.
>>
>>
>>> Not sure how that will affect the logic. :-)
>>>
>>>>
>>>>
>>>>> Additionally, this makes me wonder about f_min. I haven't seen
>>>>> anywhere in this patch were that value is being set to proper value,
>>>>> right?
>>>>>
>>>>
>>>> f_min should be 400000 for qcom, I think with the default mclk frequency
>>>> and
>>>> a divider of 512 used for calculating the f_min is bringing down the
>>>> f_min
>>>> to lessthan 400Kz. Which is why its working fine.
>>>> I think the possibility of mclk default frequency being greater than
>>>> 208Mhz
>>>> is very less. so I could either leave it as it is Or force this to 400000
>>>> all the time for qcom chips.
>>>
>>>
>>> No, this seems like a wrong approach.
>>>
>>> I think you would like to do use the clk_round_rate() find out the
>>> lowest possible rate. Or just use a fixed fallback value somehow.
>>
>>
>> clk_round_rate(mclk, 0) might be more generic, instead of fixed fallback.
>> Let the mmc-core figure out what frequency would be best from its table
>> starting from f_min.
>
> Agree!
>
> clk_round_rate(mclk, 100KHz), might be better though - since that is
> actually the lowest request frequency whatsoever.
>
Perfect.

--srini
> Kind regards
> Ulf Hansson
>

Re: [PATCH v3 08/13] mmc: mmci: add 8bit bus support in variant data

[Srinivas Kandagatla]

Hi Linus W,


On 26/05/14 11:07, Ulf Hansson wrote:
>>          unsigned int            fifosize;
>> >         unsigned int            fifohalfsize;
>> >@@ -116,6 +118,7 @@ static struct variant_data variant_u300 = {
>> >         .fifosize               = 16 * 4,
>> >         .fifohalfsize           = 8 * 4,
>> >         .clkreg_enable          = MCI_ST_U300_HWFCEN,
>> >+       .clkreg_8bit_bus_enable = MCI_ST_8BIT_BUS,
> Linus, will have to confirm this. I don't know if the u300 variant
> support 8-bit.
>

Do you know if u300 supports 8BIT bus?

thanks,
srini

> Kind regards
> Ulf Hansson
>

Re: [PATCH v3 08/13] mmc: mmci: add 8bit bus support in variant data

[Linus Walleij]

On Wed, May 28, 2014 at 9:27 AM, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
> Hi Linus W,
> On 26/05/14 11:07, Ulf Hansson wrote:
>>>
>>>          unsigned int            fifosize;
>>> >         unsigned int            fifohalfsize;
>>> >@@ -116,6 +118,7 @@ static struct variant_data variant_u300 = {
>>> >         .fifosize               = 16 * 4,
>>> >         .fifohalfsize           = 8 * 4,
>>> >         .clkreg_enable          = MCI_ST_U300_HWFCEN,
>>> >+       .clkreg_8bit_bus_enable = MCI_ST_8BIT_BUS,
>>
>> Linus, will have to confirm this. I don't know if the u300 variant
>> support 8-bit.
>>
>
> Do you know if u300 supports 8BIT bus?

Yes it does actually.

Yours,
Linus Walleij

Re: [PATCH v3 12/13] mmc: mmci: add explicit clk control

[Linus Walleij]

On Tue, May 27, 2014 at 12:39 AM, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
> On 26/05/14 15:21, Ulf Hansson wrote:
>> On 23 May 2014 14:52,  <srinivas.kandagatla@linaro.org> wrote:

>>>
>>> +       bool                    explicit_mclk_control;
>>> +       bool                    cclk_is_mclk;
>>
>> I can't see why you need to have both these new configurations. Aren't
>> "cclk_is_mclk" just a fact when you use "explicit_mclk_control".
>
>> I also believe I would prefer something like "qcom_clkdiv" instead.
>
> There is a subtle difference between both the flags.  Am happy to change it
> to qcom_clkdiv.

I think this was due to me wanting the variant variables to be more about
the actual technical difference they indicate rather than pointing to
a certain vendor or variant where that difference occurs.

It's a very minor thing though, if you prefer it this way, go for it.

Yours,
Linus Walleij

Re: [PATCH v3 13/13] mmc: mmci: Add Qcom specific pio_read function.

[Linus Walleij]

On Fri, May 23, 2014 at 2:53 PM,  <srinivas.kandagatla@linaro.org> wrote:

> +       if (unlikely(bytes)) {
> +               unsigned char buf[4];
(...)

Please think twice about this.
http://lwn.net/Articles/70473/
http://lwn.net/Articles/420019/
http://lwn.net/Articles/182369/

Yours,
Linus Walleij

Re: [PATCH v3 12/13] mmc: mmci: add explicit clk control

[Srinivas Kandagatla]

On 28/05/14 09:02, Linus Walleij wrote:
> On Tue, May 27, 2014 at 12:39 AM, Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>> On 26/05/14 15:21, Ulf Hansson wrote:
>>> On 23 May 2014 14:52,  <srinivas.kandagatla@linaro.org> wrote:
>
>>>>
>>>> +       bool                    explicit_mclk_control;
>>>> +       bool                    cclk_is_mclk;
>>>
>>> I can't see why you need to have both these new configurations. Aren't
>>> "cclk_is_mclk" just a fact when you use "explicit_mclk_control".
>>
>>> I also believe I would prefer something like "qcom_clkdiv" instead.
>>
>> There is a subtle difference between both the flags.  Am happy to change it
>> to qcom_clkdiv.
>
> I think this was due to me wanting the variant variables to be more about
> the actual technical difference they indicate rather than pointing to
> a certain vendor or variant where that difference occurs.
>
Yes, that's correct, I think having these two variables seems to be more 
generic than qcom_clkdiv.

I will keep it as it is and fix other comments from Ulf in next version.

> It's a very minor thing though, if you prefer it this way, go for it.
>

Thanks,
sirni
> Yours,
> Linus Walleij
>

Re: [PATCH v3 13/13] mmc: mmci: Add Qcom specific pio_read function.

[Srinivas Kandagatla]

On 28/05/14 09:08, Linus Walleij wrote:
> On Fri, May 23, 2014 at 2:53 PM,  <srinivas.kandagatla@linaro.org> wrote:
>
>> +       if (unlikely(bytes)) {
>> +               unsigned char buf[4];
> (...)

>
> Please think twice about this.
> http://lwn.net/Articles/70473/
> http://lwn.net/Articles/420019/
> http://lwn.net/Articles/182369/
>
Thanks for the warning.

You are right. I think having likely/unlikely is not always right here.

thanks,
srini
> Yours,
> Linus Walleij
>

Re: [PATCH v3 10/13] mmc: mmci: add Qcom specifics of clk and datactrl registers.

[Srinivas Kandagatla]

Hi Ulf,

On 26/05/14 22:38, Srinivas Kandagatla wrote:
>>>   2 files changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>>> index 17e7f6a..6434f5b1 100644
>>> --- a/drivers/mmc/host/mmci.c
>>> +++ b/drivers/mmc/host/mmci.c
>>> @@ -185,6 +185,10 @@ static struct variant_data variant_qcom = {
>>>          .fifosize               = 16 * 4,
>>>          .fifohalfsize           = 8 * 4,
>>>          .clkreg                 = MCI_CLK_ENABLE,
>>> +       .clkreg_enable          = MCI_QCOM_CLK_FLOWENA |
>>> +                                 MCI_QCOM_CLK_FEEDBACK_CLK,
>>
>> Obviously I don't have the in-depth knowledge about the Qcom variant,
>> but comparing the ST variant here made me think.
>>
>> Using the feeback clock internal logic in the ST variant, requires the
>> corresponding feedback clock pin signal on the board, to be
>> routed/connected. Typically we used this for SD cards, which involved
>> using an external level shifter circuit.
>>
>> Is it correct to enable this bit for all cases, including eMMC?
>>
> You are correct, FBCLK should specific to the board, and I will try to
> do something on the same lines as ST variant in next version.
I get lot of I/O errors when I remove this flag for test.

I rechecked schematics and datasheet, the feedback clk that we refer 
here is the the feedback clk from CLK pad, there is no separate input 
pad for fbclk. So I think this is internally feedbacked clk.

This selection is configuring bits to latch data and command coming in 
using feedback clock from CLK pad.

I will make sure that the macro is named more appropriately to reflect 
the same.

thanks,
srini

Re: [PATCH v3 10/13] mmc: mmci: add Qcom specifics of clk and datactrl registers.

[Ulf Hansson]

On 28 May 2014 11:41, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
> Hi Ulf,
>
>
> On 26/05/14 22:38, Srinivas Kandagatla wrote:
>>>>
>>>>   2 files changed, 28 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>>>> index 17e7f6a..6434f5b1 100644
>>>> --- a/drivers/mmc/host/mmci.c
>>>> +++ b/drivers/mmc/host/mmci.c
>>>> @@ -185,6 +185,10 @@ static struct variant_data variant_qcom = {
>>>>          .fifosize               = 16 * 4,
>>>>          .fifohalfsize           = 8 * 4,
>>>>          .clkreg                 = MCI_CLK_ENABLE,
>>>> +       .clkreg_enable          = MCI_QCOM_CLK_FLOWENA |
>>>> +                                 MCI_QCOM_CLK_FEEDBACK_CLK,
>>>
>>>
>>> Obviously I don't have the in-depth knowledge about the Qcom variant,
>>> but comparing the ST variant here made me think.
>>>
>>> Using the feeback clock internal logic in the ST variant, requires the
>>> corresponding feedback clock pin signal on the board, to be
>>> routed/connected. Typically we used this for SD cards, which involved
>>> using an external level shifter circuit.
>>>
>>> Is it correct to enable this bit for all cases, including eMMC?
>>>
>> You are correct, FBCLK should specific to the board, and I will try to
>> do something on the same lines as ST variant in next version.
>
> I get lot of I/O errors when I remove this flag for test.

Running eMMC I suppose?

>
> I rechecked schematics and datasheet, the feedback clk that we refer here is
> the the feedback clk from CLK pad, there is no separate input pad for fbclk.
> So I think this is internally feedbacked clk.
>
> This selection is configuring bits to latch data and command coming in using
> feedback clock from CLK pad.

Seems like it's strange to have this bit configurable then. I guess it
would  be hard to tell under what circumstances you don't want this
bit set.

Anyway, it's not a big deal to me - let's keep it as is for now.

Kind regards
Ulf Hansson

>
> I will make sure that the macro is named more appropriately to reflect the
> same.
>
> thanks,
> srini

Re: [PATCH v3 12/13] mmc: mmci: add explicit clk control

[Ulf Hansson]

On 28 May 2014 10:28, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
> On 28/05/14 09:02, Linus Walleij wrote:
>>
>> On Tue, May 27, 2014 at 12:39 AM, Srinivas Kandagatla
>> <srinivas.kandagatla@linaro.org> wrote:
>>>
>>> On 26/05/14 15:21, Ulf Hansson wrote:
>>>>
>>>> On 23 May 2014 14:52,  <srinivas.kandagatla@linaro.org> wrote:
>>
>>
>>>>>
>>>>> +       bool                    explicit_mclk_control;
>>>>> +       bool                    cclk_is_mclk;
>>>>
>>>>
>>>> I can't see why you need to have both these new configurations. Aren't
>>>> "cclk_is_mclk" just a fact when you use "explicit_mclk_control".
>>>
>>>
>>>> I also believe I would prefer something like "qcom_clkdiv" instead.
>>>
>>>
>>> There is a subtle difference between both the flags.  Am happy to change
>>> it
>>> to qcom_clkdiv.
>>
>>
>> I think this was due to me wanting the variant variables to be more about
>> the actual technical difference they indicate rather than pointing to
>> a certain vendor or variant where that difference occurs.
>>
> Yes, that's correct, I think having these two variables seems to be more
> generic than qcom_clkdiv.
>
> I will keep it as it is and fix other comments from Ulf in next version.
>

I think this relates to the discussion we had around fetching the
f_min and f_max in ->probe(). It, just seems silly to have to check
for an extra flag there as well, because that is in principle what
this would mean, right?

So, please adjust to my proposal, I strongly think this should be only
one flag. You might want a more generic name of the flag in favour of
qcom_clkdiv, feel free to change to whatever you think make sense.

Kind regards
Ulf Hansson

>
>> It's a very minor thing though, if you prefer it this way, go for it.
>>
>
> Thanks,
> sirni
>>
>> Yours,
>> Linus Walleij
>>
>

Re: [PATCH v3 13/13] mmc: mmci: Add Qcom specific pio_read function.

[Srinivas Kandagatla]

Sorry Stephen for late reply,
Some reason this mail was filtered in other folders.

On 24/05/14 00:28, Stephen Boyd wrote:
> On 05/23/14 05:53, srinivas.kandagatla@linaro.org wrote:
>> @@ -1022,6 +1025,40 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>>   	}
>>   }
>>
>> +static int mmci_qcom_pio_read(struct mmci_host *host, char *buffer,
>> +			unsigned int remain)
>> +{
>> +	u32 *ptr = (u32 *) buffer;
>> +	unsigned int count = 0;
>> +	unsigned int words, bytes;
>> +	unsigned int fsize = host->variant->fifosize;
>> +
>> +	words = remain >> 2;
>> +	bytes = remain % 4;
>> +	/* read full words followed by leftover bytes */
>> +	if (words) {
>> +		while (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) {
>> +			*ptr = readl(host->base + MMCIFIFO + (count % fsize));
>
> This doesn't look endianness agnostic. Shouldn't we use ioread32_rep()
> to read this fifo?
Is'nt readl endianess aware?

we can not use ioread32_rep because as we can not reliably know how many 
words we should read? FIFOCNT would have helped but its not advised to 
be use as per the datasheet.



Thanks,
srini
>
>> +			ptr++;
>> +			count += 4;
>> +			words--;
>> +			if (!words)
>> +				break;
>> +		}
>> +	}
>> +
>> +	if (unlikely(bytes)) {
>> +		unsigned char buf[4];
>> +		if (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) {
>> +			*buf = readl(host->base + MMCIFIFO + (count % fsize));
>> +			memcpy(ptr, buf, bytes);
>> +			count += bytes;
>> +		}
>> +	}
>> +
>> +	return count;
>> +}
>> +
>>   static int mmci_pio_read(struct mmci_host *host, char *buffer, unsigned int remain)
>>   {
>>   	void __iomem *base = host->base;
>>
>

Re: [PATCH v3 13/13] mmc: mmci: Add Qcom specific pio_read function.

[Linus Walleij]

On Wed, May 28, 2014 at 3:57 PM, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:

>> This doesn't look endianness agnostic. Shouldn't we use ioread32_rep()
>> to read this fifo?
>
> Is'nt readl endianess aware?

At least once a year read through arch/arm/include/asm/io.h

static inline u32 __raw_readl(const volatile void __iomem *addr)
{
        u32 val;
        asm volatile("ldr %1, %0"
                     : "+Qo" (*(volatile u32 __force *)addr),
                       "=r" (val));
        return val;
}
(...)
#define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
                                        __raw_readl(c)); __r; })
(...)
#define readl(c)                ({ u32 __v = readl_relaxed(c);
__iormb(); __v; })

readl() reads in little endian.

All PrimeCells are little endian, trouble would occur if and only if
this cell was
used on a big endian CPU, like an ARM used in BE mode, if the macros
didn't look exactly like this.

Thanks to the fact that readl() is always LE things work smoothly.

Then to the question whether to use ioread32() instead:

#define ioread32(p)     ({ unsigned int __v = le32_to_cpu((__force
__le32)__raw_readl(p)); __iormb(); __v; })

Same thing as readl(). The only reason to use ioread32() rather than
readl() is that some archs do not support readl() but only ioread32().
However for that to be a problem, these archs must be utilizing that
primecell! And if they do there are two ways to solve it: either change
the entire world to use ioread/write32 or simply just define readl() and
friends for that arch in its io.h file.

I'd say don't touch it right now.

> we can not use ioread32_rep because as we can not reliably know how many
> words we should read? FIFOCNT would have helped but its not advised to be
> use as per the datasheet.

You are right. readsl() or ioread32_rep() isn't used for this reason.

Yours,
Linus Walleij

Re: [PATCH v3 13/13] mmc: mmci: Add Qcom specific pio_read function.

[Stephen Boyd]

On 05/29/14 00:43, Linus Walleij wrote:
> On Wed, May 28, 2014 at 3:57 PM, Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>
>>> This doesn't look endianness agnostic. Shouldn't we use ioread32_rep()
>>> to read this fifo?
>> Is'nt readl endianess aware?
> At least once a year read through arch/arm/include/asm/io.h
>
> static inline u32 __raw_readl(const volatile void __iomem *addr)
> {
>         u32 val;
>         asm volatile("ldr %1, %0"
>                      : "+Qo" (*(volatile u32 __force *)addr),
>                        "=r" (val));
>         return val;
> }
> (...)
> #define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
>                                         __raw_readl(c)); __r; })
> (...)
> #define readl(c)                ({ u32 __v = readl_relaxed(c);
> __iormb(); __v; })
>
> readl() reads in little endian.
>
> All PrimeCells are little endian, trouble would occur if and only if
> this cell was
> used on a big endian CPU, like an ARM used in BE mode, if the macros
> didn't look exactly like this.
>
> Thanks to the fact that readl() is always LE things work smoothly.
>
> Then to the question whether to use ioread32() instead:
>
> #define ioread32(p)     ({ unsigned int __v = le32_to_cpu((__force
> __le32)__raw_readl(p)); __iormb(); __v; })
>
> Same thing as readl(). The only reason to use ioread32() rather than
> readl() is that some archs do not support readl() but only ioread32().
> However for that to be a problem, these archs must be utilizing that
> primecell! And if they do there are two ways to solve it: either change
> the entire world to use ioread/write32 or simply just define readl() and
> friends for that arch in its io.h file.
>
> I'd say don't touch it right now.

Hm... that doesn't sound right. Please see this thread on lkml[1], and
also this video from Ben H.[2] My understanding is that this is a fifo
so I would think we want to use the ioread32_rep() function here (or
other "string" io functionality). Otherwise we're going to byteswap the
data that we're trying to interpret as a buffer and big endian CPUs
won't work.

>
>> we can not use ioread32_rep because as we can not reliably know how many
>> words we should read? FIFOCNT would have helped but its not advised to be
>> use as per the datasheet.
> You are right. readsl() or ioread32_rep() isn't used for this reason.
>
>

We could always use a size of 1 right?

[1] https://lkml.org/lkml/2012/10/22/671
[2] http://linuxplumbers.ubicast.tv/videos/big-and-little-endian-inside-out/

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v3 13/13] mmc: mmci: Add Qcom specific pio_read function.

[Linus Walleij]

On Fri, May 30, 2014 at 3:30 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:

> Hm... that doesn't sound right. Please see this thread on lkml[1], and
> also this video from Ben H.[2]

But Benji is talking about the *PCI BUS*, and this is raw register
access. He just says that "This is how a BE CPU is normally
wired to a LE bus" we have no idea whether that is actually the
case on any existing system with this peripheral. Such things
have to be tested.

> My understanding is that this is a fifo
> so I would think we want to use the ioread32_rep() function here (or
> other "string" io functionality). Otherwise we're going to byteswap the
> data that we're trying to interpret as a buffer and big endian CPUs
> won't work.

You don't know that until you have tested on real BE ARM
hardware with this PrimeCell. How do you know the SoC
designers will have been smart enough to insert byte swap
logic?

>> You are right. readsl() or ioread32_rep() isn't used for this reason.
>
> We could always use a size of 1 right?

What do you mean? Do you mean using:

ioread32_rep(io, buf, 1)?

Notice this is totally equivalent to readsl() in arm/include/asm/io.h:

#define readsl(p,d,l)           __raw_readsl(p,d,l)
#define ioread32_rep(p,d,c)     __raw_readsl(p,d,c)

The semantic effect is switching from readl() -> readsl() or
conversely ioread32 -> ioread32_rep() which will use
__raw accessors in native endianness rather than LE access.

Which is what you want if the bus will do byte swapping. Which
we don't know if it does, because noone has such a system and
can verify.

If the native bus is not byte swapping this will just force you to
use cpu_to_le32() on the result. That is not helpful at all.

We need to experiment with real BE hardware with this
PrimeCell to determine whether to do this, and until that is
done just don't try to outsmart the hardware by thinking ahead
and designing for the unknown, that will just fail.

Yours,
Linus Walleij