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

[Luca Ellero]

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.
Regards

-- 
Luca Ellero

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