[U-Boot] [PATCH] mtd: nand: mxs: fix PIO_WORD number

[Marek Vasut]

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 at 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 at 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!