* [U-Boot] [PATCH] mtd: nand: mxs: fix PIO_WORD number @ 2014-12-10 8:24 Luca Ellero 2014-12-12 8:43 ` Stefan Roese 0 siblings, 1 reply; 14+ messages in thread From: Luca Ellero @ 2014-12-10 8:24 UTC (permalink / raw) To: u-boot There is only one pio_word in this DMA transaction so data field must be 1. Signed-off-by: Luca Ellero <luca.ellero@brickedbrain.com> --- drivers/mtd/nand/mxs_nand.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c index 7a064ab..616c9ca 100644 --- a/drivers/mtd/nand/mxs_nand.c +++ b/drivers/mtd/nand/mxs_nand.c @@ -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; -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [U-Boot] [PATCH] mtd: nand: mxs: fix PIO_WORD number 2014-12-10 8:24 [U-Boot] [PATCH] mtd: nand: mxs: fix PIO_WORD number Luca Ellero @ 2014-12-12 8:43 ` Stefan Roese 2014-12-12 12:40 ` Luca Ellero 0 siblings, 1 reply; 14+ messages in thread From: Stefan Roese @ 2014-12-12 8:43 UTC (permalink / raw) To: u-boot Hi Luca, On 10.12.2014 09:24, Luca Ellero wrote: > There is only one pio_word in this DMA transaction so data field must be 1. > > Signed-off-by: Luca Ellero <luca.ellero@brickedbrain.com> > --- > drivers/mtd/nand/mxs_nand.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c > index 7a064ab..616c9ca 100644 > --- a/drivers/mtd/nand/mxs_nand.c > +++ b/drivers/mtd/nand/mxs_nand.c > @@ -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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot] [PATCH] mtd: nand: mxs: fix PIO_WORD number 2014-12-12 8:43 ` Stefan Roese @ 2014-12-12 12:40 ` Luca Ellero 2014-12-12 12:43 ` Stefan Roese 0 siblings, 1 reply; 14+ messages in thread From: Luca Ellero @ 2014-12-12 12:40 UTC (permalink / raw) To: u-boot Hi Stefan, On 12/12/2014 09:43, Stefan Roese wrote: > Hi Luca, > > On 10.12.2014 09:24, Luca Ellero wrote: >> There is only one pio_word in this DMA transaction so data field must >> be 1. >> >> Signed-off-by: Luca Ellero <luca.ellero@brickedbrain.com> >> --- >> drivers/mtd/nand/mxs_nand.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c >> index 7a064ab..616c9ca 100644 >> --- a/drivers/mtd/nand/mxs_nand.c >> +++ b/drivers/mtd/nand/mxs_nand.c >> @@ -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. -- Luca Ellero E-mail: luca.ellero at brickedbrain.com Internet: www.brickedbrain.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot] [PATCH] mtd: nand: mxs: fix PIO_WORD number 2014-12-12 12:40 ` Luca Ellero @ 2014-12-12 12:43 ` Stefan Roese 2014-12-12 12:58 ` Marek Vasut 0 siblings, 1 reply; 14+ messages in thread From: Stefan Roese @ 2014-12-12 12:43 UTC (permalink / raw) To: u-boot Hi Luca, On 12.12.2014 13:40, Luca Ellero wrote: >> On 10.12.2014 09:24, Luca Ellero wrote: >>> There is only one pio_word in this DMA transaction so data field must >>> be 1. >>> >>> Signed-off-by: Luca Ellero <luca.ellero@brickedbrain.com> >>> --- >>> drivers/mtd/nand/mxs_nand.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c >>> index 7a064ab..616c9ca 100644 >>> --- a/drivers/mtd/nand/mxs_nand.c >>> +++ b/drivers/mtd/nand/mxs_nand.c >>> @@ -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. 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 <sr@denx.de> Thanks, Stefan ^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot] [PATCH] mtd: nand: mxs: fix PIO_WORD number 2014-12-12 12:43 ` Stefan Roese @ 2014-12-12 12:58 ` Marek Vasut 2014-12-12 15:03 ` Luca Ellero 0 siblings, 1 reply; 14+ messages in thread From: Marek Vasut @ 2014-12-12 12:58 UTC (permalink / raw) To: u-boot On Friday, December 12, 2014 at 01:43:22 PM, Stefan Roese wrote: > Hi Luca, > > On 12.12.2014 13:40, Luca Ellero wrote: > >> On 10.12.2014 09:24, Luca Ellero wrote: > >>> There is only one pio_word in this DMA transaction so data field must > >>> be 1. > >>> > >>> Signed-off-by: Luca Ellero <luca.ellero@brickedbrain.com> > >>> --- > >>> > >>> drivers/mtd/nand/mxs_nand.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c > >>> index 7a064ab..616c9ca 100644 > >>> --- a/drivers/mtd/nand/mxs_nand.c > >>> +++ b/drivers/mtd/nand/mxs_nand.c > >>> @@ -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. > > 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 <sr@denx.de> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot] [PATCH] mtd: nand: mxs: fix PIO_WORD number 2014-12-12 12:58 ` Marek Vasut @ 2014-12-12 15:03 ` Luca Ellero 2014-12-13 13:12 ` Marek Vasut 0 siblings, 1 reply; 14+ messages in thread From: Luca Ellero @ 2014-12-12 15:03 UTC (permalink / raw) To: u-boot Hi Marek, On 12/12/2014 13:58, Marek Vasut wrote: > On Friday, December 12, 2014 at 01:43:22 PM, Stefan Roese wrote: >> Hi Luca, >> >> On 12.12.2014 13:40, Luca Ellero wrote: >>>> On 10.12.2014 09:24, Luca Ellero wrote: >>>>> There is only one pio_word in this DMA transaction so data field must >>>>> be 1. >>>>> >>>>> Signed-off-by: Luca Ellero <luca.ellero@brickedbrain.com> >>>>> --- >>>>> >>>>> drivers/mtd/nand/mxs_nand.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c >>>>> index 7a064ab..616c9ca 100644 >>>>> --- a/drivers/mtd/nand/mxs_nand.c >>>>> +++ b/drivers/mtd/nand/mxs_nand.c >>>>> @@ -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. >> >> 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 <sr@denx.de> > > 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 -- Luca Ellero E-mail: luca.ellero at brickedbrain.com Internet: www.brickedbrain.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot] [PATCH] mtd: nand: mxs: fix PIO_WORD number 2014-12-12 15:03 ` Luca Ellero @ 2014-12-13 13:12 ` Marek Vasut 2014-12-15 8:45 ` Luca Ellero 0 siblings, 1 reply; 14+ messages in thread From: Marek Vasut @ 2014-12-13 13:12 UTC (permalink / raw) To: u-boot On Friday, December 12, 2014 at 04:03:14 PM, Luca Ellero wrote: > Hi Marek, > > On 12/12/2014 13:58, Marek Vasut wrote: > > On Friday, December 12, 2014 at 01:43:22 PM, Stefan Roese wrote: > >> Hi Luca, > >> > >> On 12.12.2014 13:40, Luca Ellero wrote: > >>>> On 10.12.2014 09:24, Luca Ellero wrote: > >>>>> There is only one pio_word in this DMA transaction so data field must > >>>>> be 1. > >>>>> > >>>>> Signed-off-by: Luca Ellero <luca.ellero@brickedbrain.com> > >>>>> --- > >>>>> > >>>>> drivers/mtd/nand/mxs_nand.c | 2 +- > >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/drivers/mtd/nand/mxs_nand.c > >>>>> b/drivers/mtd/nand/mxs_nand.c index 7a064ab..616c9ca 100644 > >>>>> --- a/drivers/mtd/nand/mxs_nand.c > >>>>> +++ b/drivers/mtd/nand/mxs_nand.c > >>>>> @@ -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. > >> > >> 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 <sr@denx.de> > > > > 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? 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot] [PATCH] mtd: nand: mxs: fix PIO_WORD number 2014-12-13 13:12 ` Marek Vasut @ 2014-12-15 8:45 ` Luca Ellero 2014-12-15 11:14 ` Marek Vasut 0 siblings, 1 reply; 14+ messages in thread From: Luca Ellero @ 2014-12-15 8:45 UTC (permalink / raw) To: u-boot Hi Marek, On 13/12/2014 14:12, Marek Vasut wrote: > On Friday, December 12, 2014 at 04:03:14 PM, Luca Ellero wrote: >> Hi Marek, >> >> On 12/12/2014 13:58, Marek Vasut wrote: >>> On Friday, December 12, 2014 at 01:43:22 PM, Stefan Roese wrote: >>>> Hi Luca, >>>> >>>> On 12.12.2014 13:40, Luca Ellero wrote: >>>>>> On 10.12.2014 09:24, Luca Ellero wrote: >>>>>>> There is only one pio_word in this DMA transaction so data field must >>>>>>> be 1. >>>>>>> >>>>>>> Signed-off-by: Luca Ellero <luca.ellero@brickedbrain.com> >>>>>>> --- >>>>>>> >>>>>>> drivers/mtd/nand/mxs_nand.c | 2 +- >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/mtd/nand/mxs_nand.c >>>>>>> b/drivers/mtd/nand/mxs_nand.c index 7a064ab..616c9ca 100644 >>>>>>> --- a/drivers/mtd/nand/mxs_nand.c >>>>>>> +++ b/drivers/mtd/nand/mxs_nand.c >>>>>>> @@ -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. >>>> >>>> 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 <sr@denx.de> >>> >>> 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? > > 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 -- Luca Ellero E-mail: luca.ellero at brickedbrain.com Internet: www.brickedbrain.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot] [PATCH] mtd: nand: mxs: fix PIO_WORD number 2014-12-15 8:45 ` Luca Ellero @ 2014-12-15 11:14 ` Marek Vasut 2014-12-16 8:06 ` Luca Ellero 0 siblings, 1 reply; 14+ messages in thread From: Marek Vasut @ 2014-12-15 11:14 UTC (permalink / raw) To: u-boot On Monday, December 15, 2014 at 09:45:13 AM, Luca Ellero wrote: > Hi Marek, > > On 13/12/2014 14:12, Marek Vasut wrote: > > On Friday, December 12, 2014 at 04:03:14 PM, Luca Ellero wrote: > >> Hi Marek, > >> > >> On 12/12/2014 13:58, Marek Vasut wrote: > >>> On Friday, December 12, 2014 at 01:43:22 PM, Stefan Roese wrote: > >>>> Hi Luca, > >>>> > >>>> On 12.12.2014 13:40, Luca Ellero wrote: > >>>>>> On 10.12.2014 09:24, Luca Ellero wrote: > >>>>>>> There is only one pio_word in this DMA transaction so data field > >>>>>>> must be 1. > >>>>>>> > >>>>>>> Signed-off-by: Luca Ellero <luca.ellero@brickedbrain.com> > >>>>>>> --- > >>>>>>> > >>>>>>> drivers/mtd/nand/mxs_nand.c | 2 +- > >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>>>> > >>>>>>> diff --git a/drivers/mtd/nand/mxs_nand.c > >>>>>>> b/drivers/mtd/nand/mxs_nand.c index 7a064ab..616c9ca 100644 > >>>>>>> --- a/drivers/mtd/nand/mxs_nand.c > >>>>>>> +++ b/drivers/mtd/nand/mxs_nand.c > >>>>>>> @@ -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. > >>>> > >>>> 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 <sr@denx.de> > >>> > >>> 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? > > > > 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. 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! ^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot] [PATCH] mtd: nand: mxs: fix PIO_WORD number 2014-12-15 11:14 ` Marek Vasut @ 2014-12-16 8:06 ` Luca Ellero 2014-12-16 11:45 ` Marek Vasut 2015-02-10 13:14 ` Stefano Babic 0 siblings, 2 replies; 14+ messages in thread From: Luca Ellero @ 2014-12-16 8:06 UTC (permalink / raw) To: u-boot On 15/12/2014 12:14, Marek Vasut wrote: > On Monday, December 15, 2014 at 09:45:13 AM, Luca Ellero wrote: >> Hi Marek, >> >> On 13/12/2014 14:12, Marek Vasut wrote: >>> On Friday, December 12, 2014 at 04:03:14 PM, Luca Ellero wrote: >>>> Hi Marek, >>>> >>>> On 12/12/2014 13:58, Marek Vasut wrote: >>>>> On Friday, December 12, 2014 at 01:43:22 PM, Stefan Roese wrote: >>>>>> Hi Luca, >>>>>> >>>>>> On 12.12.2014 13:40, Luca Ellero wrote: >>>>>>>> On 10.12.2014 09:24, Luca Ellero wrote: >>>>>>>>> There is only one pio_word in this DMA transaction so data field >>>>>>>>> must be 1. >>>>>>>>> >>>>>>>>> Signed-off-by: Luca Ellero <luca.ellero@brickedbrain.com> >>>>>>>>> --- >>>>>>>>> >>>>>>>>> drivers/mtd/nand/mxs_nand.c | 2 +- >>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/mtd/nand/mxs_nand.c >>>>>>>>> b/drivers/mtd/nand/mxs_nand.c index 7a064ab..616c9ca 100644 >>>>>>>>> --- a/drivers/mtd/nand/mxs_nand.c >>>>>>>>> +++ b/drivers/mtd/nand/mxs_nand.c >>>>>>>>> @@ -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. >>>>>> >>>>>> 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 <sr@denx.de> >>>>> >>>>> 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? >>> >>> 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. > > 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 -- Luca Ellero E-mail: luca.ellero at brickedbrain.com Internet: www.brickedbrain.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot] [PATCH] mtd: nand: mxs: fix PIO_WORD number 2014-12-16 8:06 ` Luca Ellero @ 2014-12-16 11:45 ` Marek Vasut 2015-02-10 13:14 ` Stefano Babic 1 sibling, 0 replies; 14+ messages in thread From: Marek Vasut @ 2014-12-16 11:45 UTC (permalink / raw) To: u-boot On Tuesday, December 16, 2014 at 09:06:34 AM, Luca Ellero wrote: > On 15/12/2014 12:14, Marek Vasut wrote: > > On Monday, December 15, 2014 at 09:45:13 AM, Luca Ellero wrote: > >> Hi Marek, > >> > >> On 13/12/2014 14:12, Marek Vasut wrote: > >>> On Friday, December 12, 2014 at 04:03:14 PM, Luca Ellero wrote: > >>>> Hi Marek, > >>>> > >>>> On 12/12/2014 13:58, Marek Vasut wrote: > >>>>> On Friday, December 12, 2014 at 01:43:22 PM, Stefan Roese wrote: > >>>>>> Hi Luca, > >>>>>> > >>>>>> On 12.12.2014 13:40, Luca Ellero wrote: > >>>>>>>> On 10.12.2014 09:24, Luca Ellero wrote: > >>>>>>>>> There is only one pio_word in this DMA transaction so data field > >>>>>>>>> must be 1. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Luca Ellero <luca.ellero@brickedbrain.com> > >>>>>>>>> --- > >>>>>>>>> > >>>>>>>>> drivers/mtd/nand/mxs_nand.c | 2 +- > >>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>>>>>> > >>>>>>>>> diff --git a/drivers/mtd/nand/mxs_nand.c > >>>>>>>>> b/drivers/mtd/nand/mxs_nand.c index 7a064ab..616c9ca 100644 > >>>>>>>>> --- a/drivers/mtd/nand/mxs_nand.c > >>>>>>>>> +++ b/drivers/mtd/nand/mxs_nand.c > >>>>>>>>> @@ -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. > >>>>>> > >>>>>> 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 <sr@denx.de> > >>>>> > >>>>> 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? > >>> > >>> 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. > > > > 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! ^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot] [PATCH] mtd: nand: mxs: fix PIO_WORD number 2014-12-16 8:06 ` Luca Ellero 2014-12-16 11:45 ` Marek Vasut @ 2015-02-10 13:14 ` Stefano Babic 2015-02-10 15:18 ` Luca Ellero 1 sibling, 1 reply; 14+ messages in thread From: Stefano Babic @ 2015-02-10 13:14 UTC (permalink / raw) To: u-boot Hi Luca, On 16/12/2014 09:06, Luca Ellero wrote: >>> 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! >> > > 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 -- ===================================================================== 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 ===================================================================== ^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot] [PATCH] mtd: nand: mxs: fix PIO_WORD number 2015-02-10 13:14 ` Stefano Babic @ 2015-02-10 15:18 ` Luca Ellero 2015-02-10 15:47 ` Marek Vasut 0 siblings, 1 reply; 14+ messages in thread From: Luca Ellero @ 2015-02-10 15:18 UTC (permalink / raw) To: u-boot On 10/02/2015 14:14, Stefano Babic wrote: > Hi Luca, > > On 16/12/2014 09:06, Luca Ellero wrote: > >>>> 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! >>> >> >> 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 -- Luca Ellero E-mail: luca.ellero at brickedbrain.com Internet: www.brickedbrain.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot] [PATCH] mtd: nand: mxs: fix PIO_WORD number 2015-02-10 15:18 ` Luca Ellero @ 2015-02-10 15:47 ` Marek Vasut 0 siblings, 0 replies; 14+ messages in thread From: Marek Vasut @ 2015-02-10 15:47 UTC (permalink / raw) To: u-boot On Tuesday, February 10, 2015 at 04:18:12 PM, Luca Ellero wrote: > On 10/02/2015 14:14, Stefano Babic wrote: > > Hi Luca, > > > > On 16/12/2014 09:06, Luca Ellero wrote: > >>>> 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! > >> > >> 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. Thank you :) Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-02-10 15:47 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-12-10 8:24 [U-Boot] [PATCH] mtd: nand: mxs: fix PIO_WORD number Luca Ellero 2014-12-12 8:43 ` Stefan Roese 2014-12-12 12:40 ` Luca Ellero 2014-12-12 12:43 ` Stefan Roese 2014-12-12 12:58 ` Marek Vasut 2014-12-12 15:03 ` Luca Ellero 2014-12-13 13:12 ` Marek Vasut 2014-12-15 8:45 ` Luca Ellero 2014-12-15 11:14 ` Marek Vasut 2014-12-16 8:06 ` Luca Ellero 2014-12-16 11:45 ` Marek Vasut 2015-02-10 13:14 ` Stefano Babic 2015-02-10 15:18 ` Luca Ellero 2015-02-10 15:47 ` Marek Vasut
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox