On Mon, Dec 01, 2014 at 09:08:42PM +0800, Zhou Wang wrote:
> On 2014年11月30日 17:35, Brian Norris wrote:
> >On Tue, Nov 04, 2014 at 08:47:00PM +0800, Zhou Wang wrote:
> >>Signed-off-by: Zhou Wang <wangzhou.bry@xxxxxxxxx>
[...]
> >>+static void hisi_nfc_dma_transfer(struct hinfc_host *host, int todev)
> >>+{
> >>+ struct mtd_info *mtd = &host->mtd;
> >>+ struct nand_chip *chip = mtd->priv;
> >>+ unsigned long val;
> >>+ int ret;
> >>+
> >>+ hinfc_write(host, host->dma_buffer, HINFC504_DMA_ADDR_DATA);
> >>+ hinfc_write(host, host->dma_oob, HINFC504_DMA_ADDR_OOB);
> >>+
> >>+ if (chip->ecc.mode == NAND_ECC_NONE) {
> >>+ hinfc_write(host, ((mtd->oobsize & HINFC504_DMA_LEN_OOB_MASK)
> >>+ << HINFC504_DMA_LEN_OOB_SHIFT), HINFC504_DMA_LEN);
> >>+
> >>+ hinfc_write(host, HINFC504_DMA_PARA_DATA_RW_EN
> >>+ | HINFC504_DMA_PARA_OOB_RW_EN, HINFC504_DMA_PARA);
> >>+ } else {
> >>+ hinfc_write(host, HINFC504_DMA_PARA_DATA_RW_EN
> >>+ | HINFC504_DMA_PARA_OOB_RW_EN | HINFC504_DMA_PARA_DATA_EDC_EN
> >>+ | HINFC504_DMA_PARA_OOB_EDC_EN | HINFC504_DMA_PARA_DATA_ECC_EN
> >>+ | HINFC504_DMA_PARA_OOB_ECC_EN, HINFC504_DMA_PARA);
> >>+ }
> >>+
> >>+ val = (HINFC504_DMA_CTRL_DMA_START | HINFC504_DMA_CTRL_BURST4_EN
> >>+ | HINFC504_DMA_CTRL_BURST8_EN | HINFC504_DMA_CTRL_BURST16_EN
> >>+ | HINFC504_DMA_CTRL_DATA_AREA_EN | HINFC504_DMA_CTRL_OOB_AREA_EN
> >>+ | ((host->addr_cycle == 4 ? 1 : 0)
> >>+ << HINFC504_DMA_CTRL_ADDR_NUM_SHIFT)
> >>+ | ((host->chipselect & HINFC504_DMA_CTRL_CS_MASK)
> >>+ << HINFC504_DMA_CTRL_CS_SHIFT));
> >>+
> >>+ if (todev)
> >>+ val |= HINFC504_DMA_CTRL_WE;
> >>+
> >>+ hinfc_write(host, val, HINFC504_DMA_CTRL);
> >>+
> >>+ init_completion(&host->cmd_complete);
> >
> >You need to run init_completion() *before* you kick off your I/O.
> >Otherwise, your interrupt handler is racing with this function.
> >
>
> Do you mean that it needs put init_completion() before hinfc_write()?
> If not so, interrupt handler maybe execute before init_completion()
> which will cause something wrong.
Yes.
init_completion(&host->cmd_complete);
should come sometime before
hinfc_write(host, val, HINFC504_DMA_CTRL);
> >>+ ret = wait_for_completion_timeout(&host->cmd_complete,
> >>+ HINFC504_NFC_DMA_TIMEOUT);
[...]
> >>+static void set_addr(struct mtd_info *mtd, int column, int page_addr)
> >>+{
> >>+ struct nand_chip *chip = mtd->priv;
> >>+ struct hinfc_host *host = chip->priv;
> >>+
> >>+ host->addr_cycle = 0;
> >>+ host->addr_value[0] = 0;
> >>+ host->addr_value[1] = 0;
> >>+
> >>+ /* Serially input address */
> >>+ if (column != -1) {
> >>+ /* Adjust columns for 16 bit buswidth */
> >>+ if (chip->options & NAND_BUSWIDTH_16)
> >>+ column >>= 1;
> >
> >It doesn't look like you're supporting ONFI parameter pages yet, but you
> >might consider checking for !nand_opcode_8bits(opcode) before adjusting the
> >address. We had to fix some other drivers for this recently.
> >
>
> sorry, could you give me more clue about this? Do you mean that we
> should first make sure it is not 8 bits bus width?
Look for use of nand_opcode_8bits() in nand_base.c.
I'm suggesting that you don't necessarily want to "adjust the column"
for all commands; there are some opcodes that take their address only on
the lower x8 bits, even if the flash has an x16 buswidth.
> >>+
> >>+ host->addr_value[0] = column & 0xffff;
> >>+ host->addr_cycle = 2;
> >>+ }
[...]
> >>+ switch (mtd->writesize) {
> >>+ case 2048:
> >>+ flag |= (0x001 << HINFC504_CON_PAGEISZE_SHIFT);
> >>+ /* add more pagesize support
> >>+ * default pagesize has been set in hisi_nfc_host_init
> >>+ */
> >
> >Does this mean you can't handle any other page size? You might want to
> >put the words 'TODO' or 'FIXME' in the comment, and you might want to
> >print an error and exit if you see some other value for mtd->writesize.
> >
>
> Yes, this NAND controller can handle not only 2048 page size. But
> the NAND controller on hip04-d01 board which I used to test this
> driver connects with a NAND flash chip which is just 2048 page size.
> So here I
> just listed 'case 2048', Maybe I need indicate this by 'TODO:...'?
Not just a TODO; make sure to also error out, so that users don't get
unexplained failures if they find themselves with a non-2KB page size
NAND.
Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
