[U-Boot] [PATCH v2] ppc/85xx: PIO Support for FSL eSDHC Controller Driver

[U-Boot] [PATCH v2] ppc/85xx: PIO Support for FSL eSDHC Controller Driver

[Dipen Dudhat]

On some Freescale SoC Internal DMA of eSDHC controller has bug.

So PIO Mode has introduced to do data transfer using CPU.
In PIO mode data transfer performance will be degraded by a large extent.

Note: 
In PIO mode multiple block read/write requires delay to complete the transfer.

Signed-off-by: Dipen Dudhat <dipen.dudhat@freescale.com>
---
 drivers/mmc/fsl_esdhc.c |   87 +++++++++++++++++++++++++++++++++++++++++++++-
 include/fsl_esdhc.h     |    2 +
 2 files changed, 87 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index 27e4c48..7db8c0c 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -73,8 +73,10 @@ uint esdhc_xfertyp(struct mmc_cmd *cmd, struct mmc_data *data)
 	uint xfertyp = 0;
 
 	if (data) {
-		xfertyp |= XFERTYP_DPSEL | XFERTYP_DMAEN;
-
+		xfertyp |= XFERTYP_DPSEL;
+#ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO
+		xfertyp |= XFERTYP_DMAEN;
+#endif
 		if (data->blocks > 1) {
 			xfertyp |= XFERTYP_MSBSEL;
 			xfertyp |= XFERTYP_BCEN;
@@ -98,12 +100,88 @@ uint esdhc_xfertyp(struct mmc_cmd *cmd, struct mmc_data *data)
 	return XFERTYP_CMD(cmd->cmdidx) | xfertyp;
 }
 
+#ifdef CONFIG_SYS_FSL_ESDHC_USE_PIO
+/*
+ * PIO Read/Write Mode reduce the performace as DMA is not used in this mode.
+ */
+static int
+esdhc_pio_read_write(struct mmc *mmc, struct mmc_data *data)
+{
+	struct fsl_esdhc *regs = mmc->priv;
+	uint blocks;
+	char *buffer;
+	uint databuf;
+	uint size;
+	uint irqstat;
+	uint timeout;
+
+	if (data->flags & MMC_DATA_READ) {
+		blocks = data->blocks;
+		buffer = data->dest;
+		while (blocks) {
+			timeout = MAX_TIMEOUT;
+			size = data->blocksize;
+			irqstat = in_be32(&regs->irqstat);
+			while (!(in_be32(&regs->prsstat) & PRSSTAT_BREN)
+				&& --timeout);
+			if (timeout <= 0) {
+				printf("\nData Read Failed in PIO Mode.");
+				return TIMEOUT;
+			}
+			while (size && (!(irqstat & IRQSTAT_TC))) {
+				udelay(100);
+				irqstat = in_be32(&regs->irqstat);
+				databuf = in_le32(&regs->datport);
+				*((uint *)buffer) = databuf;
+				buffer += 4;
+				size -= 4;
+			}
+			blocks--;
+		}
+	} else {
+		blocks = data->blocks;
+		buffer = data->src;
+		while (blocks) {
+			timeout = MAX_TIMEOUT;
+			size = data->blocksize;
+			irqstat = in_be32(&regs->irqstat);
+			while (!(in_be32(&regs->prsstat) & PRSSTAT_BWEN)
+				&& --timeout);
+			if (timeout <= 0) {
+				printf("\nData Write Failed in PIO Mode.");
+				return TIMEOUT;
+			}
+			while (size && (!(irqstat & IRQSTAT_TC))) {
+				udelay(100);
+				databuf = *((uint *)buffer);
+				buffer += 4;
+				size -= 4;
+				irqstat = in_be32(&regs->irqstat);
+				out_le32(&regs->datport, databuf);
+			}
+			blocks--;
+		}
+	}
+}
+#endif
+
 static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data)
 {
 	uint wml_value;
 	int timeout;
 	struct fsl_esdhc *regs = mmc->priv;
 
+#ifdef CONFIG_SYS_FSL_ESDHC_USE_PIO
+	if (!(data->flags & MMC_DATA_READ)) {
+		if ((in_be32(&regs->prsstat) & PRSSTAT_WPSPL) == 0) {
+			printf("\nThe SD card is locked. "
+				"Can not write to a locked card.\n\n");
+			return TIMEOUT;
+		}
+		out_be32(&regs->dsaddr, (u32)data->src);
+	} else
+		out_be32(&regs->dsaddr, (u32)data->dest);
+#else
 	wml_value = data->blocksize/4;
 
 	if (data->flags & MMC_DATA_READ) {
@@ -125,6 +203,7 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data)
 	}
 
 	out_be32(&regs->wml, wml_value);
+#endif
 
 	out_be32(&regs->blkattr, data->blocks << 16 | data->blocksize);
 
@@ -217,6 +296,9 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
 
 	/* Wait until all of the blocks are transferred */
 	if (data) {
+#ifdef CONFIG_SYS_FSL_ESDHC_USE_PIO
+		esdhc_pio_read_write(mmc, data);
+#else
 		do {
 			irqstat = in_be32(&regs->irqstat);
 
@@ -227,6 +309,7 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
 				return TIMEOUT;
 		} while (!(irqstat & IRQSTAT_TC) &&
 				(in_be32(&regs->prsstat) & PRSSTAT_DLA));
+#endif
 	}
 
 	out_be32(&regs->irqstat, -1);
diff --git a/include/fsl_esdhc.h b/include/fsl_esdhc.h
index 89b8304..5948d37 100644
--- a/include/fsl_esdhc.h
+++ b/include/fsl_esdhc.h
@@ -86,6 +86,7 @@
 #define PRSSTAT_CDPL		(0x00040000)
 #define PRSSTAT_CINS		(0x00010000)
 #define PRSSTAT_BREN		(0x00000800)
+#define PRSSTAT_BWEN		(0x00000400)
 #define PRSSTAT_DLA		(0x00000004)
 #define PRSSTAT_CICHB		(0x00000002)
 #define PRSSTAT_CIDHB		(0x00000001)
@@ -117,6 +118,7 @@
 #define XFERTYP_DMAEN		0x00000001
 
 #define CINS_TIMEOUT		1000
+#define MAX_TIMEOUT		100000
 
 #define DSADDR		0x2e004
 
-- 
1.5.6.3

[U-Boot] [PATCH v2] ppc/85xx: PIO Support for FSL eSDHC Controller Driver

[Wolfgang Denk]

Dear Dipen Dudhat,

In message <1252589856-4970-1-git-send-email-dipen.dudhat@freescale.com> you wrote:
> On some Freescale SoC Internal DMA of eSDHC controller has bug.
> 
> So PIO Mode has introduced to do data transfer using CPU.
> In PIO mode data transfer performance will be degraded by a large extent.
> 
> Note: 
> In PIO mode multiple block read/write requires delay to complete the transfer.

Does that mean you just delay, i. e. you do not check for some ready
state?

...
> --- a/include/fsl_esdhc.h
> +++ b/include/fsl_esdhc.h
> @@ -86,6 +86,7 @@
>  #define PRSSTAT_CDPL		(0x00040000)
>  #define PRSSTAT_CINS		(0x00010000)
>  #define PRSSTAT_BREN		(0x00000800)
> +#define PRSSTAT_BWEN		(0x00000400)
>  #define PRSSTAT_DLA		(0x00000004)
>  #define PRSSTAT_CICHB		(0x00000002)
>  #define PRSSTAT_CIDHB		(0x00000001)
> @@ -117,6 +118,7 @@
>  #define XFERTYP_DMAEN		0x00000001
>  
>  #define CINS_TIMEOUT		1000
> +#define MAX_TIMEOUT		100000

MAX_TIMEOUT is a very generic name, please chose a more specific one
so interactions with other code are prevented. (Also, TIMEOUT itself
already means a maximum time, so MAX_TIMEOUT is a tautology.)

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Very ugly or very beautiful women should be flattered on their
understanding, and mediocre ones on their beauty.
                                       -- Philip Earl of Chesterfield

[U-Boot] [PATCH v2] ppc/85xx: PIO Support for FSL eSDHC Controller Driver

[Dudhat Dipen-B09055]

Hi Wolfgang,

There are some IP specific requirements to get the transaction complete
in PIO mode.

Thanks
Dipen


-----Original Message-----
From: Wolfgang Denk [mailto:wd at denx.de] 
Sent: Wednesday, September 23, 2009 12:12 AM
To: Dudhat Dipen-B09055
Cc: u-boot at lists.denx.de
Subject: Re: [U-Boot] [PATCH v2] ppc/85xx: PIO Support for FSL eSDHC
Controller Driver

Dear Dipen Dudhat,

In message <1252589856-4970-1-git-send-email-dipen.dudhat@freescale.com>
you wrote:
> On some Freescale SoC Internal DMA of eSDHC controller has bug.
> 
> So PIO Mode has introduced to do data transfer using CPU.
> In PIO mode data transfer performance will be degraded by a large
extent.
> 
> Note: 
> In PIO mode multiple block read/write requires delay to complete the
transfer.

Does that mean you just delay, i. e. you do not check for some ready
state?


...
> --- a/include/fsl_esdhc.h
> +++ b/include/fsl_esdhc.h
> @@ -86,6 +86,7 @@
>  #define PRSSTAT_CDPL		(0x00040000)
>  #define PRSSTAT_CINS		(0x00010000)
>  #define PRSSTAT_BREN		(0x00000800)
> +#define PRSSTAT_BWEN		(0x00000400)
>  #define PRSSTAT_DLA		(0x00000004)
>  #define PRSSTAT_CICHB		(0x00000002)
>  #define PRSSTAT_CIDHB		(0x00000001)
> @@ -117,6 +118,7 @@
>  #define XFERTYP_DMAEN		0x00000001
>  
>  #define CINS_TIMEOUT		1000
> +#define MAX_TIMEOUT		100000

MAX_TIMEOUT is a very generic name, please chose a more specific one so
interactions with other code are prevented. (Also, TIMEOUT itself
already means a maximum time, so MAX_TIMEOUT is a tautology.)

Dipen,    
Will update that.



Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Very ugly or very beautiful women should be flattered on their
understanding, and mediocre ones on their beauty.
                                       -- Philip Earl of Chesterfield

[U-Boot] [PATCH v2] ppc/85xx: PIO Support for FSL eSDHC Controller Driver

[Bin Meng]

On Thu, Sep 10, 2009 at 9:37 PM, Dipen Dudhat
<dipen.dudhat@freescale.com> wrote:
> +                       while (size && (!(irqstat & IRQSTAT_TC))) {
> +                               udelay(100);
> +                               irqstat = in_be32(&regs->irqstat);
> +                               databuf = in_le32(&regs->datport);
> +                               *((uint *)buffer) = databuf;
> +                               buffer += 4;
> +                               size -= 4;
> +                       }

Is the udelay(100) the recommended delay time by the chip manual or
empirical value?
Can you add a comment before the udelay?

Regards,
Bin Meng

[U-Boot] [PATCH v2] ppc/85xx: PIO Support for FSL eSDHC Controller Driver

[Dudhat Dipen-B09055]

Hi Bin,

We can know the block transfer complete using IRQSTAT(Transfer
Complete).
But reading & writing in PIO mode takes time for byte by byte transfers
and there is no way to poll that transfer, that's why I have added delay
there. 

I can add comment like,
\* Wait before last byte transfer complete *\
Is that ok ??

Regards,
 Dipen

 

-----Original Message-----
From: Bin Meng [mailto:bmeng.cn at gmail.com] 
Sent: Monday, September 28, 2009 4:41 AM
To: Dudhat Dipen-B09055
Cc: u-boot at lists.denx.de
Subject: Re: [U-Boot] [PATCH v2] ppc/85xx: PIO Support for FSL eSDHC
Controller Driver

On Thu, Sep 10, 2009 at 9:37 PM, Dipen Dudhat
<dipen.dudhat@freescale.com> wrote:
> +                       while (size && (!(irqstat & IRQSTAT_TC))) {
> +                               udelay(100);
> +                               irqstat = in_be32(&regs->irqstat);
> +                               databuf = in_le32(&regs->datport);
> +                               *((uint *)buffer) = databuf;
> +                               buffer += 4;
> +                               size -= 4;
> +                       }

Is the udelay(100) the recommended delay time by the chip manual or
empirical value?
Can you add a comment before the udelay?

Regards,
Bin Meng

[U-Boot] [PATCH v2] ppc/85xx: PIO Support for FSL eSDHC Controller Driver

[Bin Meng]

Hi Dipen,

On Thu, Oct 1, 2009 at 12:48 PM, Dudhat Dipen-B09055
<Dipen.Dudhat@freescale.com> wrote:
>
> Hi Bin,
>
> We can know the block transfer complete using IRQSTAT(Transfer
> Complete).
> But reading & writing in PIO mode takes time for byte by byte transfers
> and there is no way to poll that transfer, that's why I have added delay
> there.
>

Yes, I understand there is no such a register in Freescale eSDHC that
provides status check for PIO mode.
I am just wondering where the delay value 100 us comes from. Is it the
required minimum value?

> I can add comment like,
> \* Wait before last byte transfer complete *\
> Is that ok ??
>

Sure, adding a comment line would help a lot.

Regards,
Bin Meng

[U-Boot] [PATCH v2] ppc/85xx: PIO Support for FSL eSDHC Controller Driver

[Dudhat Dipen-B09055]

-----Original Message-----
From: Bin Meng [mailto:bmeng.cn at gmail.com]
Sent: Monday, October 05, 2009 1:59 PM
To: Dudhat Dipen-B09055
Cc: u-boot at lists.denx.de
Subject: Re: [U-Boot] [PATCH v2] ppc/85xx: PIO Support for FSL eSDHC
Controller Driver

Hi Dipen,

On Thu, Oct 1, 2009 at 12:48 PM, Dudhat Dipen-B09055
<Dipen.Dudhat@freescale.com> wrote:
>
> Hi Bin,
>
> We can know the block transfer complete using IRQSTAT(Transfer
> Complete).
> But reading & writing in PIO mode takes time for byte by byte
> transfers and there is no way to poll that transfer, that's why I have
> added delay there.
>

Yes, I understand there is no such a register in Freescale eSDHC that
provides status check for PIO mode.
I am just wondering where the delay value 100 us comes from. Is it the
required minimum value?

Dipen,
        Yes it's a minimum value. Less than that it hangs and read/write
fails in PIO Mode.


> I can add comment like,
> \* Wait before last byte transfer complete *\ Is that ok ??
>

Sure, adding a comment line would help a lot.

Regards,
Bin Meng

Thanks
Dipen

[U-Boot] [PATCH v2] ppc/85xx: PIO Support for FSL eSDHC Controller Driver

[Dipen Dudhat]

On some Freescale SoC Internal DMA of eSDHC controller has bug.

So PIO Mode has introduced to do data transfer using CPU.
In PIO mode data transfer performance will be degraded by a large extent.

Note: 
In PIO mode multiple block read/write requires delay to complete the transfer.

Signed-off-by: Dipen Dudhat <dipen.dudhat@freescale.com>
---
Changes from v1:
- Integrated comments for error handling
- Formatting errors removed
 drivers/mmc/fsl_esdhc.c |   87 +++++++++++++++++++++++++++++++++++++++++++++-
 include/fsl_esdhc.h     |    2 +
 2 files changed, 87 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index 27e4c48..7db8c0c 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -73,8 +73,10 @@ uint esdhc_xfertyp(struct mmc_cmd *cmd, struct mmc_data *data)
 	uint xfertyp = 0;
 
 	if (data) {
-		xfertyp |= XFERTYP_DPSEL | XFERTYP_DMAEN;
-
+		xfertyp |= XFERTYP_DPSEL;
+#ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO
+		xfertyp |= XFERTYP_DMAEN;
+#endif
 		if (data->blocks > 1) {
 			xfertyp |= XFERTYP_MSBSEL;
 			xfertyp |= XFERTYP_BCEN;
@@ -98,12 +100,88 @@ uint esdhc_xfertyp(struct mmc_cmd *cmd, struct mmc_data *data)
 	return XFERTYP_CMD(cmd->cmdidx) | xfertyp;
 }
 
+#ifdef CONFIG_SYS_FSL_ESDHC_USE_PIO
+/*
+ * PIO Read/Write Mode reduce the performace as DMA is not used in this mode.
+ */
+static int
+esdhc_pio_read_write(struct mmc *mmc, struct mmc_data *data)
+{
+	struct fsl_esdhc *regs = mmc->priv;
+	uint blocks;
+	char *buffer;
+	uint databuf;
+	uint size;
+	uint irqstat;
+	uint timeout;
+
+	if (data->flags & MMC_DATA_READ) {
+		blocks = data->blocks;
+		buffer = data->dest;
+		while (blocks) {
+			timeout = MAX_TIMEOUT;
+			size = data->blocksize;
+			irqstat = in_be32(&regs->irqstat);
+			while (!(in_be32(&regs->prsstat) & PRSSTAT_BREN)
+				&& --timeout);
+			if (timeout <= 0) {
+				printf("\nData Read Failed in PIO Mode.");
+				return TIMEOUT;
+			}
+			while (size && (!(irqstat & IRQSTAT_TC))) {
+				udelay(100);
+				irqstat = in_be32(&regs->irqstat);
+				databuf = in_le32(&regs->datport);
+				*((uint *)buffer) = databuf;
+				buffer += 4;
+				size -= 4;
+			}
+			blocks--;
+		}
+	} else {
+		blocks = data->blocks;
+		buffer = data->src;
+		while (blocks) {
+			timeout = MAX_TIMEOUT;
+			size = data->blocksize;
+			irqstat = in_be32(&regs->irqstat);
+			while (!(in_be32(&regs->prsstat) & PRSSTAT_BWEN)
+				&& --timeout);
+			if (timeout <= 0) {
+				printf("\nData Write Failed in PIO Mode.");
+				return TIMEOUT;
+			}
+			while (size && (!(irqstat & IRQSTAT_TC))) {
+				udelay(100);
+				databuf = *((uint *)buffer);
+				buffer += 4;
+				size -= 4;
+				irqstat = in_be32(&regs->irqstat);
+				out_le32(&regs->datport, databuf);
+			}
+			blocks--;
+		}
+	}
+}
+#endif
+
 static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data)
 {
 	uint wml_value;
 	int timeout;
 	struct fsl_esdhc *regs = mmc->priv;
 
+#ifdef CONFIG_SYS_FSL_ESDHC_USE_PIO
+	if (!(data->flags & MMC_DATA_READ)) {
+		if ((in_be32(&regs->prsstat) & PRSSTAT_WPSPL) == 0) {
+			printf("\nThe SD card is locked. "
+				"Can not write to a locked card.\n\n");
+			return TIMEOUT;
+		}
+		out_be32(&regs->dsaddr, (u32)data->src);
+	} else
+		out_be32(&regs->dsaddr, (u32)data->dest);
+#else
 	wml_value = data->blocksize/4;
 
 	if (data->flags & MMC_DATA_READ) {
@@ -125,6 +203,7 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data)
 	}
 
 	out_be32(&regs->wml, wml_value);
+#endif
 
 	out_be32(&regs->blkattr, data->blocks << 16 | data->blocksize);
 
@@ -217,6 +296,9 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
 
 	/* Wait until all of the blocks are transferred */
 	if (data) {
+#ifdef CONFIG_SYS_FSL_ESDHC_USE_PIO
+		esdhc_pio_read_write(mmc, data);
+#else
 		do {
 			irqstat = in_be32(&regs->irqstat);
 
@@ -227,6 +309,7 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
 				return TIMEOUT;
 		} while (!(irqstat & IRQSTAT_TC) &&
 				(in_be32(&regs->prsstat) & PRSSTAT_DLA));
+#endif
 	}
 
 	out_be32(&regs->irqstat, -1);
diff --git a/include/fsl_esdhc.h b/include/fsl_esdhc.h
index 89b8304..5948d37 100644
--- a/include/fsl_esdhc.h
+++ b/include/fsl_esdhc.h
@@ -86,6 +86,7 @@
 #define PRSSTAT_CDPL		(0x00040000)
 #define PRSSTAT_CINS		(0x00010000)
 #define PRSSTAT_BREN		(0x00000800)
+#define PRSSTAT_BWEN		(0x00000400)
 #define PRSSTAT_DLA		(0x00000004)
 #define PRSSTAT_CICHB		(0x00000002)
 #define PRSSTAT_CIDHB		(0x00000001)
@@ -117,6 +118,7 @@
 #define XFERTYP_DMAEN		0x00000001
 
 #define CINS_TIMEOUT		1000
+#define MAX_TIMEOUT		100000
 
 #define DSADDR		0x2e004
 
-- 
1.5.6.3

[U-Boot] [PATCH v2] ppc/85xx: PIO Support for FSL eSDHC Controller Driver

[Wolfgang Denk]

Dear Dipen Dudhat,

In message <1252904203-9129-1-git-send-email-dipen.dudhat@freescale.com> you wrote:
> On some Freescale SoC Internal DMA of eSDHC controller has bug.
> 
> So PIO Mode has introduced to do data transfer using CPU.
> In PIO mode data transfer performance will be degraded by a large extent.
> 
> Note: 
> In PIO mode multiple block read/write requires delay to complete the transfer.
> 
> Signed-off-by: Dipen Dudhat <dipen.dudhat@freescale.com>

My comments to v1 of the patch still apply.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
We do not colonize. We conquer. We rule. There is no  other  way  for
us.
	-- Rojan, "By Any Other Name", stardate 4657.5