From mboxrd@z Thu Jan 1 00:00:00 1970 From: Luca Ellero Date: Mon, 15 Dec 2014 09:45:13 +0100 Subject: [U-Boot] [PATCH] mtd: nand: mxs: fix PIO_WORD number In-Reply-To: <201412131412.43236.marex@denx.de> References: <1418199884-2430-1-git-send-email-luca.ellero@brickedbrain.com> <201412121358.29505.marex@denx.de> <548B03B2.3070305@brickedbrain.com> <201412131412.43236.marex@denx.de> Message-ID: <548E9F99.3070606@brickedbrain.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>>>>> --- >>>>>>> >>>>>>> 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 >>> >>> 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