mtd: nand: mxs: fix PIO_WORD number

PATCH New Luca Ellero 4 participants 14 messages Dec 10, 2014 – Feb 10, 2015

Subsystems (3)

Arm i.MX and Layerscape Memory technology devices (MTD) NAND flash

lore

Share & tools

lore

Luca Ellero

author

2014-12-10 08:24 UTC #0 lore raw

PATCH 1/1 mtd: nand: mxs: fix PIO_WORD number

There is only one pio_word in this DMA transaction so data field must be 1.

Signed-off-by Luca Ellero <[email protected]>

---
 drivers/mtd/nand/mxs_nand.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mtd/nand/mxs_nand.c +1 -1

index 7a064ab..616c9ca 100644
@@ -305,7 +305,7 @@ static void mxs_nand_cmd_ctrl(struct mtd_info *mtd, int data, unsigned int ctrl)
 	d->cmd.data =
 		MXS_DMA_DESC_COMMAND_DMA_READ | MXS_DMA_DESC_IRQ |
 		MXS_DMA_DESC_CHAIN | MXS_DMA_DESC_DEC_SEM |
-		MXS_DMA_DESC_WAIT4END | (3 << MXS_DMA_DESC_PIO_WORDS_OFFSET) |
+		MXS_DMA_DESC_WAIT4END | (1 << MXS_DMA_DESC_PIO_WORDS_OFFSET) |
 		(nand_info->cmd_queue_len << MXS_DMA_DESC_BYTES_OFFSET);
 
 	d->cmd.address = (dma_addr_t)nand_info->cmd_buf;

Signature · 1 line

1.7.10.4

Stefan Roese

2014-12-12 08:43 UTC #1 lore raw

PATCH 1/1 mtd: nand: mxs: fix PIO_WORD number

Hi Luca,

Quoted 12 lines · Luca Ellero

There is only one pio_word in this DMA transaction so data field must be 1.
Signed-off-by Luca Ellero <[email protected]>
---
  drivers/mtd/nand/mxs_nand.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mtd/nand/mxs_nand.c +0 -0
index 7a064ab..616c9ca 100644

drivers/mtd/nand/mxs_nand.c static void mxs_nand_cmd_ctrl(struct mtd_info *mtd, int data, unsigned int ctrl) · line 309

Quoted 6 lines

drivers/mtd/nand/mxs_nand.c +1 -1

@@ -305,7 +305,7 @@ static void mxs_nand_cmd_ctrl(struct mtd_info *mtd, int data, unsigned int ctrl)
  	d->cmd.data =
  		MXS_DMA_DESC_COMMAND_DMA_READ | MXS_DMA_DESC_IRQ |
  		MXS_DMA_DESC_CHAIN | MXS_DMA_DESC_DEC_SEM |
-		MXS_DMA_DESC_WAIT4END | (3 << MXS_DMA_DESC_PIO_WORDS_OFFSET) |
+		MXS_DMA_DESC_WAIT4END | (1 << MXS_DMA_DESC_PIO_WORDS_OFFSET) |
  		(nand_info->cmd_queue_len << MXS_DMA_DESC_BYTES_OFFSET);

What error or problem does this incorrect setup cause in your case? I'm asking since I'm also using this driver in some mx6 system and have not seen any issues.

Thanks,
Stefan

Luca Ellero

author

2014-12-12 12:40 UTC #2 lore raw

PATCH 1/1 mtd: nand: mxs: fix PIO_WORD number

Hi Stefan,

Quoted 11 lines · Stefan Roese

Hi Luca,
Quoted 8 lines · Luca Ellero
There is only one pio_word in this DMA transaction so data field must be 1.
Signed-off-by Luca Ellero <[email protected]>
---
  drivers/mtd/nand/mxs_nand.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 16 lines

drivers/mtd/nand/mxs_nand.c static void mxs_nand_cmd_ctrl(struct mtd_info · line 310
Quoted 12 lines
drivers/mtd/nand/mxs_nand.c +1 -1
index 7a064ab..616c9ca 100644
@@ -305,7 +305,7 @@ static void mxs_nand_cmd_ctrl(struct mtd_info
 *mtd, int data, unsigned int ctrl)
      d->cmd.data =
          MXS_DMA_DESC_COMMAND_DMA_READ | MXS_DMA_DESC_IRQ |
          MXS_DMA_DESC_CHAIN | MXS_DMA_DESC_DEC_SEM |
-        MXS_DMA_DESC_WAIT4END | (3 << MXS_DMA_DESC_PIO_WORDS_OFFSET) |
+        MXS_DMA_DESC_WAIT4END | (1 << MXS_DMA_DESC_PIO_WORDS_OFFSET) |
          (nand_info->cmd_queue_len << MXS_DMA_DESC_BYTES_OFFSET);
What error or problem does this incorrect setup cause in your case? I'm asking since I'm also using this driver in some mx6 system and have not seen any issues.

As far as I can see, it doesn't seem to cause any issue. But, if you read the iMX6 Reference Manual (chapter 14.2) this field should reflect the number of PIO_WORDS appended to the DMA command, in this case 1.

Signature · 4 lines

Luca Ellero

E-mail: luca.ellero at brickedbrain.com
Internet: www.brickedbrain.com

Stefan Roese

2014-12-12 12:43 UTC #3 lore raw

PATCH 1/1 mtd: nand: mxs: fix PIO_WORD number

Hi Luca,

Quoted 26 lines · Luca Ellero

Quoted 9 lines · Luca Ellero
Quoted 8 lines
There is only one pio_word in this DMA transaction so data field must be 1.
Signed-off-by Luca Ellero <[email protected]>
---
  drivers/mtd/nand/mxs_nand.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 16 lines
drivers/mtd/nand/mxs_nand.c static void mxs_nand_cmd_ctrl(struct mtd_info · line 310
Quoted 12 lines
drivers/mtd/nand/mxs_nand.c +1 -1
index 7a064ab..616c9ca 100644
@@ -305,7 +305,7 @@ static void mxs_nand_cmd_ctrl(struct mtd_info
 *mtd, int data, unsigned int ctrl)
      d->cmd.data =
          MXS_DMA_DESC_COMMAND_DMA_READ | MXS_DMA_DESC_IRQ |
          MXS_DMA_DESC_CHAIN | MXS_DMA_DESC_DEC_SEM |
-        MXS_DMA_DESC_WAIT4END | (3 << MXS_DMA_DESC_PIO_WORDS_OFFSET) |
+        MXS_DMA_DESC_WAIT4END | (1 << MXS_DMA_DESC_PIO_WORDS_OFFSET) |
          (nand_info->cmd_queue_len << MXS_DMA_DESC_BYTES_OFFSET);
What error or problem does this incorrect setup cause in your case? I'm asking since I'm also using this driver in some mx6 system and have not seen any issues.

Quoted 3 lines

As far as I can see, it doesn't seem to cause any issue. But, if you read the iMX6 Reference Manual (chapter 14.2) this field should reflect the number of PIO_WORDS appended to the DMA command, in this case 1.

Okay. I just wanted to check if this patch fixes a real problem that you have experienced. Thanks for the explanation.

Reviewed-by Stefan Roese <[email protected]>

Thanks,
Stefan

Marek Vasut

2014-12-12 12:58 UTC #4 lore raw

PATCH 1/1 mtd: nand: mxs: fix PIO_WORD number

Quoted 34 lines · Stefan Roese

Hi Luca,
Quoted 31 lines · Luca Ellero
Quoted 30 lines · Luca Ellero
Quoted 15 lines
There is only one pio_word in this DMA transaction so data field must be 1.
Signed-off-by Luca Ellero <[email protected]>
---
  drivers/mtd/nand/mxs_nand.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mtd/nand/mxs_nand.c +0 -0
index 7a064ab..616c9ca 100644
@@ -305,7 +305,7 @@ static void mxs_nand_cmd_ctrl(struct mtd_info
 *mtd, int data, unsigned int ctrl)
drivers/mtd/nand/mxs_nand.c static void mxs_nand_cmd_ctrl(struct mtd_info
Quoted 10 lines
drivers/mtd/nand/mxs_nand.c +1 -1
 
      d->cmd.data =
 
          MXS_DMA_DESC_COMMAND_DMA_READ | MXS_DMA_DESC_IRQ |
          MXS_DMA_DESC_CHAIN | MXS_DMA_DESC_DEC_SEM |
 
-        MXS_DMA_DESC_WAIT4END | (3 << MXS_DMA_DESC_PIO_WORDS_OFFSET) |
+        MXS_DMA_DESC_WAIT4END | (1 << MXS_DMA_DESC_PIO_WORDS_OFFSET) |
 
          (nand_info->cmd_queue_len << MXS_DMA_DESC_BYTES_OFFSET);
What error or problem does this incorrect setup cause in your case? I'm asking since I'm also using this driver in some mx6 system and have not seen any issues.

Quoted 8 lines

Quoted 3 lines
As far as I can see, it doesn't seem to cause any issue. But, if you read the iMX6 Reference Manual (chapter 14.2) this field should reflect the number of PIO_WORDS appended to the DMA command, in this case 1.
Okay. I just wanted to check if this patch fixes a real problem that you have experienced. Thanks for the explanation.
Reviewed-by Stefan Roese <[email protected]>

The patch does in fact change the behavior such that it no longer clears the ECCCTRL and COMPARE registers both on MX28 and on MX6 . Could this have some impact?

Best regards,
Marek Vasut

Luca Ellero

author

2014-12-12 15:03 UTC #5 lore raw

PATCH 1/1 mtd: nand: mxs: fix PIO_WORD number

Hi Marek,

Quoted 39 lines · Marek Vasut

Quoted 38 lines · Stefan Roese
Hi Luca,
Quoted 34 lines · Luca Ellero
Quoted 22 lines · Luca Ellero
Quoted 21 lines
There is only one pio_word in this DMA transaction so data field must be 1.
Signed-off-by Luca Ellero <[email protected]>
---
   drivers/mtd/nand/mxs_nand.c |    2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mtd/nand/mxs_nand.c +0 -0
index 7a064ab..616c9ca 100644
@@ -305,7 +305,7 @@ static void mxs_nand_cmd_ctrl(struct mtd_info
 *mtd, int data, unsigned int ctrl)
 
       d->cmd.data =
 
           MXS_DMA_DESC_COMMAND_DMA_READ | MXS_DMA_DESC_IRQ |
           MXS_DMA_DESC_CHAIN | MXS_DMA_DESC_DEC_SEM |
 
Quoted 8 lines
drivers/mtd/nand/mxs_nand.c static void mxs_nand_cmd_ctrl(struct mtd_info *mtd, int data, unsigned int ctrl)
Quoted 4 lines
drivers/mtd/nand/mxs_nand.c +1 -1
-        MXS_DMA_DESC_WAIT4END | (3 << MXS_DMA_DESC_PIO_WORDS_OFFSET) |
+        MXS_DMA_DESC_WAIT4END | (1 << MXS_DMA_DESC_PIO_WORDS_OFFSET) |
 
           (nand_info->cmd_queue_len << MXS_DMA_DESC_BYTES_OFFSET);
What error or problem does this incorrect setup cause in your case? I'm asking since I'm also using this driver in some mx6 system and have not seen any issues.
As far as I can see, it doesn't seem to cause any issue. But, if you read the iMX6 Reference Manual (chapter 14.2) this field should reflect the number of PIO_WORDS appended to the DMA command, in this case 1.

Quoted 8 lines

Quoted 4 lines
Okay. I just wanted to check if this patch fixes a real problem that you have experienced. Thanks for the explanation.
Reviewed-by Stefan Roese <[email protected]>
The patch does in fact change the behavior such that it no longer clears the ECCCTRL and COMPARE registers both on MX28 and on MX6 . Could this have some impact?

I'm not sure. The manual doesn't tell much about it. Anyway if we want to clear COMPARE and ECCCTRL register, we should at least ensure that pio_words 1 and 2 are 0 before executing the DMA chain.

Something like this:

	d->cmd.pio_words[1] = 0;
	d->cmd.pio_words[2] = 0;

What do you think?

regards

Signature · 4 lines

Luca Ellero

E-mail: luca.ellero at brickedbrain.com
Internet: www.brickedbrain.com

Marek Vasut

2014-12-13 13:12 UTC #6 lore raw

PATCH 1/1 mtd: nand: mxs: fix PIO_WORD number

Quoted 56 lines · Luca Ellero

Hi Marek,
Quoted 40 lines · Marek Vasut
Quoted 39 lines · Stefan Roese
Hi Luca,
Quoted 35 lines · Luca Ellero
Quoted 26 lines · Luca Ellero
Quoted 25 lines
There is only one pio_word in this DMA transaction so data field must be 1.
Signed-off-by Luca Ellero <[email protected]>
---
   drivers/mtd/nand/mxs_nand.c |    2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/mtd/nand/mxs_nand.c index 7a064ab..616c9ca 100644
drivers/mtd/nand/mxs_nand.c +0 -1
@@ -305,7 +305,7 @@ static void mxs_nand_cmd_ctrl(struct mtd_info
 *mtd, int data, unsigned int ctrl)
 
       d->cmd.data =
 
           MXS_DMA_DESC_COMMAND_DMA_READ | MXS_DMA_DESC_IRQ |
           MXS_DMA_DESC_CHAIN | MXS_DMA_DESC_DEC_SEM |
 
-        MXS_DMA_DESC_WAIT4END | (3 << MXS_DMA_DESC_PIO_WORDS_OFFSET)
| +        MXS_DMA_DESC_WAIT4END | (1 <<
MXS_DMA_DESC_PIO_WORDS_OFFSET) |
Quoted 5 lines
Quoted 1 line
           (nand_info->cmd_queue_len << MXS_DMA_DESC_BYTES_OFFSET);
What error or problem does this incorrect setup cause in your case? I'm asking since I'm also using this driver in some mx6 system and have not seen any issues.
As far as I can see, it doesn't seem to cause any issue. But, if you read the iMX6 Reference Manual (chapter 14.2) this field should reflect the number of PIO_WORDS appended to the DMA command, in this case 1.
Quoted 8 lines
Quoted 4 lines
Okay. I just wanted to check if this patch fixes a real problem that you have experienced. Thanks for the explanation.
Reviewed-by Stefan Roese <[email protected]>
The patch does in fact change the behavior such that it no longer clears the ECCCTRL and COMPARE registers both on MX28 and on MX6 . Could this have some impact?
I'm not sure. The manual doesn't tell much about it. Anyway if we want to clear COMPARE and ECCCTRL register, we should at least ensure that pio_words 1 and 2 are 0 before executing the DMA chain.

Quoted 6 lines

Something like this:
	d->cmd.pio_words[1] = 0;
	d->cmd.pio_words[2] = 0;
What do you think?

I believe the descriptor is zeroed out in mxs_nand_return_dma_descs(), though I admit depending on such behavior is pretty iffy.

The question is, does your patch introduce a side-effect ? My proposal would be to schedule the patch for -next and see what happens. I believe the patch would be just fine and won't break anything.

What do you think ?

Best regards,
Marek Vasut

Luca Ellero

author

2014-12-15 08:45 UTC #7 lore raw

PATCH 1/1 mtd: nand: mxs: fix PIO_WORD number

Hi Marek,

Quoted 64 lines · Marek Vasut

Quoted 56 lines · Luca Ellero
Hi Marek,
Quoted 40 lines · Marek Vasut
Quoted 39 lines · Stefan Roese
Hi Luca,
Quoted 35 lines · Luca Ellero
Quoted 26 lines · Luca Ellero
Quoted 25 lines
There is only one pio_word in this DMA transaction so data field must be 1.
Signed-off-by Luca Ellero <[email protected]>
---
    drivers/mtd/nand/mxs_nand.c |    2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/mtd/nand/mxs_nand.c index 7a064ab..616c9ca 100644
drivers/mtd/nand/mxs_nand.c +0 -1
@@ -305,7 +305,7 @@ static void mxs_nand_cmd_ctrl(struct mtd_info
 *mtd, int data, unsigned int ctrl)
 
        d->cmd.data =
 
            MXS_DMA_DESC_COMMAND_DMA_READ | MXS_DMA_DESC_IRQ |
            MXS_DMA_DESC_CHAIN | MXS_DMA_DESC_DEC_SEM |
 
-        MXS_DMA_DESC_WAIT4END | (3 << MXS_DMA_DESC_PIO_WORDS_OFFSET)
| +        MXS_DMA_DESC_WAIT4END | (1 <<
MXS_DMA_DESC_PIO_WORDS_OFFSET) |
Quoted 5 lines
Quoted 1 line
            (nand_info->cmd_queue_len << MXS_DMA_DESC_BYTES_OFFSET);
What error or problem does this incorrect setup cause in your case? I'm asking since I'm also using this driver in some mx6 system and have not seen any issues.
As far as I can see, it doesn't seem to cause any issue. But, if you read the iMX6 Reference Manual (chapter 14.2) this field should reflect the number of PIO_WORDS appended to the DMA command, in this case 1.
Quoted 8 lines
Quoted 4 lines
Okay. I just wanted to check if this patch fixes a real problem that you have experienced. Thanks for the explanation.
Reviewed-by Stefan Roese <[email protected]>
The patch does in fact change the behavior such that it no longer clears the ECCCTRL and COMPARE registers both on MX28 and on MX6 . Could this have some impact?
I'm not sure. The manual doesn't tell much about it. Anyway if we want to clear COMPARE and ECCCTRL register, we should at least ensure that pio_words 1 and 2 are 0 before executing the DMA chain.
Quoted 6 lines
Something like this:
	d->cmd.pio_words[1] = 0;
	d->cmd.pio_words[2] = 0;
What do you think?

Quoted 8 lines

I believe the descriptor is zeroed out in mxs_nand_return_dma_descs(), though I admit depending on such behavior is pretty iffy.
The question is, does your patch introduce a side-effect ? My proposal would be to schedule the patch for -next and see what happens. I believe the patch would be just fine and won't break anything.
What do you think ?

Scheduling the patch for -next it's ok for me.
However there are other two points where pio_words number doesn't reflect the pio_words really initiated, one is in mxs_nand_read_buf() and one is in mxs_nand_write_buf(). Each one declares 4 pio_words but only one is initiated.
I wonder what we should do in this cases.
Regards

Signature · 4 lines

Luca Ellero

E-mail: luca.ellero at brickedbrain.com
Internet: www.brickedbrain.com

Marek Vasut

2014-12-15 11:14 UTC #8 lore raw

PATCH 1/1 mtd: nand: mxs: fix PIO_WORD number

Quoted 78 lines · Luca Ellero

Hi Marek,
Quoted 75 lines · Marek Vasut
Quoted 61 lines · Luca Ellero
Hi Marek,
Quoted 45 lines · Marek Vasut
Quoted 44 lines · Stefan Roese
Hi Luca,
Quoted 40 lines · Luca Ellero
Quoted 27 lines · Luca Ellero
Quoted 26 lines
There is only one pio_word in this DMA transaction so data field must be 1.
Signed-off-by Luca Ellero <[email protected]>
---
    drivers/mtd/nand/mxs_nand.c |    2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/mtd/nand/mxs_nand.c index 7a064ab..616c9ca 100644
drivers/mtd/nand/mxs_nand.c +0 -1
@@ -305,7 +305,7 @@ static void mxs_nand_cmd_ctrl(struct mtd_info
 *mtd, int data, unsigned int ctrl)
 
        d->cmd.data =
 
            MXS_DMA_DESC_COMMAND_DMA_READ | MXS_DMA_DESC_IRQ |
            MXS_DMA_DESC_CHAIN | MXS_DMA_DESC_DEC_SEM |
 
-        MXS_DMA_DESC_WAIT4END | (3 <<
MXS_DMA_DESC_PIO_WORDS_OFFSET)
| + MXS_DMA_DESC_WAIT4END | (1 <<
Quoted 8 lines
Quoted 4 lines
MXS_DMA_DESC_PIO_WORDS_OFFSET) |
            (nand_info->cmd_queue_len <<
            MXS_DMA_DESC_BYTES_OFFSET);
What error or problem does this incorrect setup cause in your case? I'm asking since I'm also using this driver in some mx6 system and have not seen any issues.
As far as I can see, it doesn't seem to cause any issue. But, if you read the iMX6 Reference Manual (chapter 14.2) this field should reflect the number of PIO_WORDS appended to the DMA command, in this case 1.
Quoted 8 lines
Quoted 4 lines
Okay. I just wanted to check if this patch fixes a real problem that you have experienced. Thanks for the explanation.
Reviewed-by Stefan Roese <[email protected]>
The patch does in fact change the behavior such that it no longer clears the ECCCTRL and COMPARE registers both on MX28 and on MX6 . Could this have some impact?
I'm not sure. The manual doesn't tell much about it. Anyway if we want to clear COMPARE and ECCCTRL register, we should at least ensure that pio_words 1 and 2 are 0 before executing the DMA chain.
Quoted 5 lines
Something like this:
	d->cmd.pio_words[1] = 0;
	d->cmd.pio_words[2] = 0;
What do you think?
I believe the descriptor is zeroed out in mxs_nand_return_dma_descs(), though I admit depending on such behavior is pretty iffy.
The question is, does your patch introduce a side-effect ? My proposal would be to schedule the patch for -next and see what happens. I believe the patch would be just fine and won't break anything.

Quoted 8 lines

Quoted 1 line
What do you think ?
Scheduling the patch for -next it's ok for me.
However there are other two points where pio_words number doesn't reflect the pio_words really initiated, one is in mxs_nand_read_buf() and one is in mxs_nand_write_buf(). Each one declares 4 pio_words but only one is initiated.
I wonder what we should do in this cases.

You can fix those as well. I recall that all this goop came from the original (2.6.35) GPMI NAND driver, which is likely where all those bugs came from as well.

Thank you!

Luca Ellero

author

2014-12-16 08:06 UTC #9 lore raw

PATCH 1/1 mtd: nand: mxs: fix PIO_WORD number

Quoted 88 lines · Marek Vasut

Quoted 78 lines · Luca Ellero
Hi Marek,
Quoted 75 lines · Marek Vasut
Quoted 61 lines · Luca Ellero
Hi Marek,
Quoted 45 lines · Marek Vasut
Quoted 44 lines · Stefan Roese
Hi Luca,
Quoted 40 lines · Luca Ellero
Quoted 27 lines · Luca Ellero
Quoted 26 lines
There is only one pio_word in this DMA transaction so data field must be 1.
Signed-off-by Luca Ellero <[email protected]>
---
     drivers/mtd/nand/mxs_nand.c |    2 +-
     1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/mtd/nand/mxs_nand.c index 7a064ab..616c9ca 100644
drivers/mtd/nand/mxs_nand.c +0 -1
@@ -305,7 +305,7 @@ static void mxs_nand_cmd_ctrl(struct mtd_info
 *mtd, int data, unsigned int ctrl)
 
         d->cmd.data =
 
             MXS_DMA_DESC_COMMAND_DMA_READ | MXS_DMA_DESC_IRQ |
             MXS_DMA_DESC_CHAIN | MXS_DMA_DESC_DEC_SEM |
 
-        MXS_DMA_DESC_WAIT4END | (3 <<
MXS_DMA_DESC_PIO_WORDS_OFFSET)
| + MXS_DMA_DESC_WAIT4END | (1 <<
Quoted 8 lines
Quoted 4 lines
MXS_DMA_DESC_PIO_WORDS_OFFSET) |
             (nand_info->cmd_queue_len <<
             MXS_DMA_DESC_BYTES_OFFSET);
What error or problem does this incorrect setup cause in your case? I'm asking since I'm also using this driver in some mx6 system and have not seen any issues.
As far as I can see, it doesn't seem to cause any issue. But, if you read the iMX6 Reference Manual (chapter 14.2) this field should reflect the number of PIO_WORDS appended to the DMA command, in this case 1.
Quoted 8 lines
Quoted 4 lines
Okay. I just wanted to check if this patch fixes a real problem that you have experienced. Thanks for the explanation.
Reviewed-by Stefan Roese <[email protected]>
The patch does in fact change the behavior such that it no longer clears the ECCCTRL and COMPARE registers both on MX28 and on MX6 . Could this have some impact?
I'm not sure. The manual doesn't tell much about it. Anyway if we want to clear COMPARE and ECCCTRL register, we should at least ensure that pio_words 1 and 2 are 0 before executing the DMA chain.
Quoted 5 lines
Something like this:
	d->cmd.pio_words[1] = 0;
	d->cmd.pio_words[2] = 0;
What do you think?
I believe the descriptor is zeroed out in mxs_nand_return_dma_descs(), though I admit depending on such behavior is pretty iffy.
The question is, does your patch introduce a side-effect ? My proposal would be to schedule the patch for -next and see what happens. I believe the patch would be just fine and won't break anything.
Quoted 8 lines
Quoted 1 line
What do you think ?
Scheduling the patch for -next it's ok for me.
However there are other two points where pio_words number doesn't reflect the pio_words really initiated, one is in mxs_nand_read_buf() and one is in mxs_nand_write_buf(). Each one declares 4 pio_words but only one is initiated.
I wonder what we should do in this cases.

Quoted 6 lines

You can fix those as well. I recall that all this goop came from the original (2.6.35) GPMI NAND driver, which is likely where all those bugs came from as well.
Thank you!

OK, I will send a patch to fix them.
Regards

Signature · 4 lines

Luca Ellero

E-mail: luca.ellero at brickedbrain.com
Internet: www.brickedbrain.com

Marek Vasut

2014-12-16 11:45 UTC #10 lore raw

PATCH 1/1 mtd: nand: mxs: fix PIO_WORD number

Quoted 89 lines · Luca Ellero

Quoted 88 lines · Marek Vasut
Quoted 86 lines · Luca Ellero
Hi Marek,
Quoted 68 lines · Marek Vasut
Quoted 61 lines · Luca Ellero
Hi Marek,
Quoted 45 lines · Marek Vasut
Quoted 44 lines · Stefan Roese
Hi Luca,
Quoted 40 lines · Luca Ellero
Quoted 27 lines · Luca Ellero
Quoted 26 lines
There is only one pio_word in this DMA transaction so data field must be 1.
Signed-off-by Luca Ellero <[email protected]>
---
     drivers/mtd/nand/mxs_nand.c |    2 +-
     1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/mtd/nand/mxs_nand.c index 7a064ab..616c9ca 100644
drivers/mtd/nand/mxs_nand.c +0 -1
@@ -305,7 +305,7 @@ static void mxs_nand_cmd_ctrl(struct mtd_info
 *mtd, int data, unsigned int ctrl)
 
         d->cmd.data =
 
             MXS_DMA_DESC_COMMAND_DMA_READ | MXS_DMA_DESC_IRQ |
             MXS_DMA_DESC_CHAIN | MXS_DMA_DESC_DEC_SEM |
 
-        MXS_DMA_DESC_WAIT4END | (3 <<
MXS_DMA_DESC_PIO_WORDS_OFFSET)
| + MXS_DMA_DESC_WAIT4END | (1 <<
Quoted 8 lines
Quoted 4 lines
MXS_DMA_DESC_PIO_WORDS_OFFSET) |
             (nand_info->cmd_queue_len <<
             MXS_DMA_DESC_BYTES_OFFSET);
What error or problem does this incorrect setup cause in your case? I'm asking since I'm also using this driver in some mx6 system and have not seen any issues.
As far as I can see, it doesn't seem to cause any issue. But, if you read the iMX6 Reference Manual (chapter 14.2) this field should reflect the number of PIO_WORDS appended to the DMA command, in this case 1.
Quoted 8 lines
Quoted 4 lines
Okay. I just wanted to check if this patch fixes a real problem that you have experienced. Thanks for the explanation.
Reviewed-by Stefan Roese <[email protected]>
The patch does in fact change the behavior such that it no longer clears the ECCCTRL and COMPARE registers both on MX28 and on MX6 . Could this have some impact?
I'm not sure. The manual doesn't tell much about it. Anyway if we want to clear COMPARE and ECCCTRL register, we should at least ensure that pio_words 1 and 2 are 0 before executing the DMA chain.
Quoted 5 lines
Something like this:
	d->cmd.pio_words[1] = 0;
	d->cmd.pio_words[2] = 0;
What do you think?
Quoted 8 lines
I believe the descriptor is zeroed out in mxs_nand_return_dma_descs(), though I admit depending on such behavior is pretty iffy.
The question is, does your patch introduce a side-effect ? My proposal would be to schedule the patch for -next and see what happens. I believe the patch would be just fine and won't break anything.
What do you think ?
Scheduling the patch for -next it's ok for me.
However there are other two points where pio_words number doesn't reflect the pio_words really initiated, one is in mxs_nand_read_buf() and one is in mxs_nand_write_buf(). Each one declares 4 pio_words but only one is initiated.
I wonder what we should do in this cases.

Quoted 7 lines

Quoted 5 lines
You can fix those as well. I recall that all this goop came from the original (2.6.35) GPMI NAND driver, which is likely where all those bugs came from as well.
Thank you!
OK, I will send a patch to fix them.

Thanks!

Stefano Babic

2015-02-10 13:14 UTC #11 lore raw

PATCH 1/1 mtd: nand: mxs: fix PIO_WORD number

In reply to Luca Ellero

Hi Luca,

Quoted 13 lines · Luca Ellero

Quoted 13 lines
Quoted 6 lines
Scheduling the patch for -next it's ok for me.
However there are other two points where pio_words number doesn't reflect the pio_words really initiated, one is in mxs_nand_read_buf() and one is in mxs_nand_write_buf(). Each one declares 4 pio_words but only one is initiated.
I wonder what we should do in this cases.
You can fix those as well. I recall that all this goop came from the original
(2.6.35) GPMI NAND driver, which is likely where all those bugs came from as
well.

Quoted 4 lines

Quoted 2 lines
Thank you!
OK, I will send a patch to fix them.

Do you plan to send a V2 patch for fixing all the issues at once ?

Best regards,
Stefano Babic

Signature · 5 lines

=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

Luca Ellero

author

2015-02-10 15:18 UTC #12 lore raw

PATCH 1/1 mtd: nand: mxs: fix PIO_WORD number

Quoted 20 lines · Stefano Babic

Hi Luca,
Quoted 16 lines · Luca Ellero
Quoted 15 lines
Quoted 6 lines
Scheduling the patch for -next it's ok for me.
However there are other two points where pio_words number doesn't reflect the pio_words really initiated, one is in mxs_nand_read_buf() and one is in mxs_nand_write_buf(). Each one declares 4 pio_words but only one is initiated.
I wonder what we should do in this cases.
You can fix those as well. I recall that all this goop came from the original
(2.6.35) GPMI NAND driver, which is likely where all those bugs came from as
well.
Thank you!

Quoted 7 lines

Quoted 1 line
OK, I will send a patch to fix them.
Do you plan to send a V2 patch for fixing all the issues at once ?
Best regards,
Stefano Babic

Hi Stefano,
yes, I can send a patch with all the fixes, that's fine. I plan to send it tomorrow.
Regards
Luca

Signature · 4 lines

Luca Ellero

E-mail: luca.ellero at brickedbrain.com
Internet: www.brickedbrain.com

Marek Vasut

2015-02-10 15:47 UTC #13 lore raw

PATCH 1/1 mtd: nand: mxs: fix PIO_WORD number

Quoted 21 lines · Luca Ellero

Quoted 20 lines · Stefano Babic
Hi Luca,
Quoted 16 lines · Luca Ellero
Quoted 7 lines
Quoted 6 lines
Scheduling the patch for -next it's ok for me.
However there are other two points where pio_words number doesn't reflect the pio_words really initiated, one is in mxs_nand_read_buf() and one is in mxs_nand_write_buf(). Each one declares 4 pio_words but only one is initiated.
I wonder what we should do in this cases.
Quoted 7 lines
You can fix those as well. I recall that all this goop came from the original
(2.6.35) GPMI NAND driver, which is likely where all those bugs came from as
well.
Thank you!
OK, I will send a patch to fix them.

Quoted 8 lines

Quoted 4 lines
Do you plan to send a V2 patch for fixing all the issues at once ?
Best regards,
Stefano Babic
Hi Stefano,
yes, I can send a patch with all the fixes, that's fine. I plan to send it tomorrow.

Thank you :)

Best regards,
Marek Vasut