[PATCH] memstick: rtsx: fix ms card data transfer bug

[PATCH] memstick: rtsx: fix ms card data transfer bug

[micky_ching]

From: Micky Ching <micky_ching@realsil.com.cn>

unlike mspro card, ms card use normal read/write mode for DMA
data transfer.

Signed-off-by: Micky Ching <micky_ching@realsil.com.cn>
---
 drivers/memstick/host/rtsx_pci_ms.c |   87 ++++++++++++++++++++++++++++++++---
 1 file changed, 81 insertions(+), 6 deletions(-)

diff --git a/drivers/memstick/host/rtsx_pci_ms.c b/drivers/memstick/host/rtsx_pci_ms.c
index 25f8f93..95eb208 100644
--- a/drivers/memstick/host/rtsx_pci_ms.c
+++ b/drivers/memstick/host/rtsx_pci_ms.c
@@ -137,8 +137,9 @@ static int ms_power_off(struct realtek_pci_ms *host)
 	return rtsx_pci_card_pull_ctl_disable(pcr, RTSX_MS_CARD);
 }
 
-static int ms_transfer_data(struct realtek_pci_ms *host, unsigned char data_dir,
-		u8 tpc, u8 cfg, struct scatterlist *sg)
+static int mspro_transfer_data(struct realtek_pci_ms *host,
+			unsigned char data_dir, u8 tpc, u8 cfg,
+			struct scatterlist *sg)
 {
 	struct rtsx_pcr *pcr = host->pcr;
 	int err;
@@ -198,6 +199,77 @@ static int ms_transfer_data(struct realtek_pci_ms *host, unsigned char data_dir,
 	return 0;
 }
 
+static int ms_transfer_data(struct realtek_pci_ms *host,
+			unsigned char data_dir, u8 tpc, u8 cfg,
+			struct scatterlist *sg)
+{
+	struct rtsx_pcr *pcr = host->pcr;
+	int err;
+	unsigned int length = sg->length;
+	u8 val, trans_mode, dma_dir;
+	struct completion trans_done;
+	int timeleft;
+
+	dev_dbg(ms_dev(host), "%s: tpc = 0x%02x, data_dir = %s, length = %d\n",
+			__func__, tpc, (data_dir == READ) ? "READ" : "WRITE",
+			length);
+
+	if (data_dir == READ) {
+		dma_dir = DMA_DIR_FROM_CARD;
+		trans_mode = MS_TM_NORMAL_READ;
+	} else {
+		dma_dir = DMA_DIR_TO_CARD;
+		trans_mode = MS_TM_NORMAL_WRITE;
+	}
+
+	rtsx_pci_init_cmd(pcr);
+
+	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, MS_TPC, 0xFF, tpc);
+	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, MS_TRANS_CFG, 0xFF, cfg);
+
+	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, IRQSTAT0,
+			DMA_DONE_INT, DMA_DONE_INT);
+	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, DMATC3, 0xFF, (u8)(length >> 24));
+	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, DMATC2, 0xFF, (u8)(length >> 16));
+	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, DMATC1, 0xFF, (u8)(length >> 8));
+	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, DMATC0, 0xFF, (u8)length);
+	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, DMACTL,
+			0x03 | DMA_PACK_SIZE_MASK, dma_dir | DMA_EN | DMA_512);
+	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, CARD_DATA_SOURCE,
+			0x01, RING_BUFFER);
+
+	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, MS_TRANSFER,
+			0xFF, MS_TRANSFER_START | trans_mode);
+	rtsx_pci_add_cmd(pcr, CHECK_REG_CMD, MS_TRANSFER,
+			MS_TRANSFER_END, MS_TRANSFER_END);
+
+	rtsx_pci_send_cmd_no_wait(pcr);
+
+	err = rtsx_pci_transfer_data(pcr, sg, 1, data_dir == READ, 10000);
+	if (err < 0) {
+		ms_print_debug_regs(host);
+		ms_clear_error(host);
+		return err;
+	}
+
+	if (pcr->trans_result == TRANS_NOT_READY) {
+		init_completion(&trans_done);
+		timeleft = wait_for_completion_interruptible_timeout(
+			&trans_done, 1000);
+		if (timeleft < 0) {
+			dev_dbg(ms_dev(host),
+				"%s: timeout wait for ok interrupt.\n",
+				__func__);
+			return -ETIMEDOUT;
+		}
+	}
+	rtsx_pci_read_register(pcr, MS_TRANS_CFG, &val);
+	if (val & (MS_CRC16_ERR | MS_RDY_TIMEOUT))
+		return -EIO;
+
+	return 0;
+}
+
 static int ms_write_bytes(struct realtek_pci_ms *host, u8 tpc,
 		u8 cfg, u8 cnt, u8 *data, u8 *int_reg)
 {
@@ -340,6 +412,7 @@ static int ms_read_bytes(struct realtek_pci_ms *host, u8 tpc,
 static int rtsx_pci_ms_issue_cmd(struct realtek_pci_ms *host)
 {
 	struct memstick_request *req = host->req;
+	struct memstick_dev *card = host->msh->card;
 	int err = 0;
 	u8 cfg = 0, int_reg;
 
@@ -350,9 +423,12 @@ static int rtsx_pci_ms_issue_cmd(struct realtek_pci_ms *host)
 			cfg = WAIT_INT;
 	}
 
-	if (req->long_data) {
+	if (req->long_data && card->id.type != MEMSTICK_TYPE_PRO) {
 		err = ms_transfer_data(host, req->data_dir,
 				req->tpc, cfg, &(req->sg));
+	} else if (req->long_data) {
+		err = mspro_transfer_data(host, req->data_dir,
+				req->tpc, cfg, &(req->sg));
 	} else {
 		if (req->data_dir == READ) {
 			err = ms_read_bytes(host, req->tpc, cfg,
@@ -461,9 +537,8 @@ static int rtsx_pci_ms_set_param(struct memstick_host *msh,
 		if (value == MEMSTICK_SERIAL) {
 			clock = 19000000;
 			ssc_depth = RTSX_SSC_DEPTH_500K;
-
-			err = rtsx_pci_write_register(pcr, MS_CFG,
-					0x18, MS_BUS_WIDTH_1);
+			err = rtsx_pci_write_register(pcr, MS_CFG, 0x58,
+					MS_BUS_WIDTH_1 | PUSH_TIME_DEFAULT);
 			if (err < 0)
 				return err;
 		} else if (value == MEMSTICK_PAR4) {
-- 
1.7.9.5

Re: [PATCH] memstick: rtsx: fix ms card data transfer bug

[Andrew Morton]

On Wed, 30 Oct 2013 14:40:16 +0800 <micky_ching@realsil.com.cn> wrote:

> unlike mspro card, ms card use normal read/write mode for DMA
> data transfer.

What are the user-visible effects of this bug?

Please always include this information when fixing bugs so that others
can decide whether they (or their customers) need the patch.

Re: [PATCH] memstick: rtsx: fix ms card data transfer bug

[micky]

MS card can not use auto read/write mode, so it will fail at
initialize and long data transfer. This patch is used to add
support for ms card.

Shall I re-send this patch to add more info?

On 11/06/2013 05:10 AM, Andrew Morton wrote:
> On Wed, 30 Oct 2013 14:40:16 +0800 <micky_ching@realsil.com.cn> wrote:
>
>> unlike mspro card, ms card use normal read/write mode for DMA
>> data transfer.
> What are the user-visible effects of this bug?
>
> Please always include this information when fixing bugs so that others
> can decide whether they (or their customers) need the patch.
>
> .
>


-- 
Best Regards
Micky.

Re: [PATCH] memstick: rtsx: fix ms card data transfer bug

[Andrew Morton]

On Wed, 6 Nov 2013 09:14:59 +0800 micky <micky_ching@realsil.com.cn> wrote:

> On 11/06/2013 05:10 AM, Andrew Morton wrote:
> > On Wed, 30 Oct 2013 14:40:16 +0800 <micky_ching@realsil.com.cn> wrote:
> >
> >> unlike mspro card, ms card use normal read/write mode for DMA
> >> data transfer.
> > What are the user-visible effects of this bug?
> >
> > Please always include this information when fixing bugs so that others
> > can decide whether they (or their customers) need the patch.
> >
>

(top-posting repaired - please don't top-post!)

> MS card can not use auto read/write mode, so it will fail at
> initialize and long data transfer. This patch is used to add
> support for ms card.
> 
> Shall I re-send this patch to add more info?

That's OK - I updated the changelog in-place and added cc:stable so it
gets backported.  But then I dropped the patch ;)

>From this info I assume that use of ms cards is very rare, otherwise
people would have complained.  What is the difference between an "ms
card" and an "mspro card"?  How common are each type and what is their
availability?

ms_transfer_data() and mspro_transfer_data() are very similar.  I think
it would be more maintainable if they were integrated into a single
function?

trans_done and timeleft could be made local to the code block where
they used.  This would be neater and more maintainable.

This code is troublesome:

: 	if (pcr->trans_result == TRANS_NOT_READY) {
: 		init_completion(&trans_done);
: 		timeleft = wait_for_completion_interruptible_timeout(
: 			&trans_done, 1000);
: 		if (timeleft < 0) {
: 			dev_dbg(ms_dev(host),
: 				"%s: timeout wait for ok interrupt.\n",
: 				__func__);
: 			return -ETIMEDOUT;
: 		}
: 	}

- Why does it exist?  Needs a comment explaining what it is trying to
  achieve.

- It should use DECLARE_COMPLETION_ONSTACK() for trans_done

- It uses wait_for_completion() but nothing ever calls complete() on
  the object!  That's just bizarre and more appropriate primitives
  should be used.

- The debug message is hard to understand and appears to be wrong. 
  Should be "interrupt received while waiting for <something?>".

- The code appears to be terminating a kernel IO transaction when the
  user hits ^C.  That's just not viable - the ^C could have been
  entered for other reasons and the IO will complete just fine.  Also
  no -EINTR is returned callers don't appear to be set up to handle it.

So shudder.  I'll drop the patch.  Please explain very carefully what
you're trying to achieve here and perhaps we can suggest a suitable
implementation approach.

[PATCH] memstick: rtsx: fix ms card data transfer bug

[micky_ching]

From: Micky Ching <micky_ching@realsil.com.cn>

This patch is used to add support for ms card. The main difference
between ms card and mspro card is long data transfer mode. mspro card
can use auto mode DMA for long data transfer, but ms can not use this
mode, it should use normal mode DMA.

The memstick core add support for ms card, but the original driver will
make ms card fail at initialize, because it use auto mode DMA. This
patch can make ms card work properly.

Signed-off-by: Micky Ching <micky_ching@realsil.com.cn>
---
 drivers/memstick/host/rtsx_pci_ms.c |   30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/memstick/host/rtsx_pci_ms.c b/drivers/memstick/host/rtsx_pci_ms.c
index 25f8f93..2a635b6 100644
--- a/drivers/memstick/host/rtsx_pci_ms.c
+++ b/drivers/memstick/host/rtsx_pci_ms.c
@@ -145,6 +145,8 @@ static int ms_transfer_data(struct realtek_pci_ms *host, unsigned char data_dir,
 	unsigned int length = sg->length;
 	u16 sec_cnt = (u16)(length / 512);
 	u8 val, trans_mode, dma_dir;
+	struct memstick_dev *card = host->msh->card;
+	bool pro_card = card->id.type == MEMSTICK_TYPE_PRO;
 
 	dev_dbg(ms_dev(host), "%s: tpc = 0x%02x, data_dir = %s, length = %d\n",
 			__func__, tpc, (data_dir == READ) ? "READ" : "WRITE",
@@ -152,19 +154,21 @@ static int ms_transfer_data(struct realtek_pci_ms *host, unsigned char data_dir,
 
 	if (data_dir == READ) {
 		dma_dir = DMA_DIR_FROM_CARD;
-		trans_mode = MS_TM_AUTO_READ;
+		trans_mode = pro_card ? MS_TM_AUTO_READ : MS_TM_NORMAL_READ;
 	} else {
 		dma_dir = DMA_DIR_TO_CARD;
-		trans_mode = MS_TM_AUTO_WRITE;
+		trans_mode = pro_card ? MS_TM_AUTO_WRITE : MS_TM_NORMAL_WRITE;
 	}
 
 	rtsx_pci_init_cmd(pcr);
 
 	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, MS_TPC, 0xFF, tpc);
-	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, MS_SECTOR_CNT_H,
-			0xFF, (u8)(sec_cnt >> 8));
-	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, MS_SECTOR_CNT_L,
-			0xFF, (u8)sec_cnt);
+	if (pro_card) {
+		rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, MS_SECTOR_CNT_H,
+				0xFF, (u8)(sec_cnt >> 8));
+		rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, MS_SECTOR_CNT_L,
+				0xFF, (u8)sec_cnt);
+	}
 	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, MS_TRANS_CFG, 0xFF, cfg);
 
 	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, IRQSTAT0,
@@ -192,8 +196,14 @@ static int ms_transfer_data(struct realtek_pci_ms *host, unsigned char data_dir,
 	}
 
 	rtsx_pci_read_register(pcr, MS_TRANS_CFG, &val);
-	if (val & (MS_INT_CMDNK | MS_INT_ERR | MS_CRC16_ERR | MS_RDY_TIMEOUT))
-		return -EIO;
+	if (pro_card) {
+		if (val & (MS_INT_CMDNK | MS_INT_ERR |
+				MS_CRC16_ERR | MS_RDY_TIMEOUT))
+			return -EIO;
+	} else {
+		if (val & (MS_CRC16_ERR | MS_RDY_TIMEOUT))
+			return -EIO;
+	}
 
 	return 0;
 }
@@ -462,8 +472,8 @@ static int rtsx_pci_ms_set_param(struct memstick_host *msh,
 			clock = 19000000;
 			ssc_depth = RTSX_SSC_DEPTH_500K;
 
-			err = rtsx_pci_write_register(pcr, MS_CFG,
-					0x18, MS_BUS_WIDTH_1);
+			err = rtsx_pci_write_register(pcr, MS_CFG, 0x58,
+					MS_BUS_WIDTH_1 | PUSH_TIME_DEFAULT);
 			if (err < 0)
 				return err;
 		} else if (value == MEMSTICK_PAR4) {
-- 
1.7.9.5