Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752100AbbKJDBW (ORCPT ); Mon, 9 Nov 2015 22:01:22 -0500 Received: from mail-pa0-f52.google.com ([209.85.220.52]:33108 "EHLO mail-pa0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751476AbbKJDBS (ORCPT ); Mon, 9 Nov 2015 22:01:18 -0500 Date: Mon, 9 Nov 2015 19:01:15 -0800 From: Brian Norris To: Bayi Cheng Cc: David Woodhouse , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Matthias Brugger , Daniel Kurtz , Sascha Hauer , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org, srv_heupstream@mediatek.com, jteki@openedev.com, ezequiel@vanguardiasur.com.ar Subject: Re: [PATCH v6 2/3] mtd: mtk-nor: mtk serial flash controller driver Message-ID: <20151110030115.GM12143@google.com> References: <1446824889-16144-1-git-send-email-bayi.cheng@mediatek.com> <1446824889-16144-3-git-send-email-bayi.cheng@mediatek.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1446824889-16144-3-git-send-email-bayi.cheng@mediatek.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4419 Lines: 159 Hi Bayi, A few small comments. On Fri, Nov 06, 2015 at 11:48:08PM +0800, Bayi Cheng wrote: > add spi nor flash driver for mediatek controller > > Signed-off-by: Bayi Cheng > --- > drivers/mtd/spi-nor/Kconfig | 7 + > drivers/mtd/spi-nor/Makefile | 1 + > drivers/mtd/spi-nor/mtk-quadspi.c | 475 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 483 insertions(+) > create mode 100644 drivers/mtd/spi-nor/mtk-quadspi.c > diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile > index e53333e..0bf3a7f8 100644 > --- a/drivers/mtd/spi-nor/Makefile > +++ b/drivers/mtd/spi-nor/Makefile > @@ -1,3 +1,4 @@ > obj-$(CONFIG_MTD_SPI_NOR) += spi-nor.o > obj-$(CONFIG_SPI_FSL_QUADSPI) += fsl-quadspi.o > +obj-$(CONFIG_MTD_MT81xx_NOR) += mtk-quadspi.o > obj-$(CONFIG_SPI_NXP_SPIFI) += nxp-spifi.o > diff --git a/drivers/mtd/spi-nor/mtk-quadspi.c b/drivers/mtd/spi-nor/mtk-quadspi.c > new file mode 100644 > index 0000000..6582811 > --- /dev/null > +++ b/drivers/mtd/spi-nor/mtk-quadspi.c > @@ -0,0 +1,475 @@ ... > +/* Can shift up to 48 bits (6 bytes) of TX/RX */ > +#define MTK_NOR_MAX_SHIFT 6 ... > +static int mt8173_nor_do_tx_rx(struct mt8173_nor *mt8173_nor, u8 op, > + u8 *tx, int txlen, u8 *rx, int rxlen) > +{ > + int len = 1 + txlen + rxlen; > + int i, ret, idx; > + > + if (len > MTK_NOR_MAX_SHIFT + 1) > + return -EINVAL; So I see you adjusted these bounds to add 1, which inspired one of my questions on the cover letter. Why do you allow len=7, which means you'd program 7*8 to the COUNT register, when the SoC manual says it has a max of 48? Is the manual wrong? I notice you added the '+ 1' to your driver, so it allows: do_tx_rx( txlen = 0 , rxlen = 6) /* e.g., for READ ID */ but that means your driver also allows: do_tx_rx( txlen = 6, rxlen = 0) /* ERROR: this will allow out of bounds write on PRGDATA register -1 */ If I understand correctly, the following constraints are more correct: /* Can only transmit 1 opcode and 5 other bytes */ if (1 + txlen > MTK_NOR_MAX_SHIFT) return -EINVAL; /* Can only receive 6 bytes after the opcode */ if (rxlen > MTK_NOR_MAX_SHIFT) return -EINVAL; /* Can only handle XXX bytes total */ /* How many bytes is the max for register MTK_NOR_CNT_REG ? */ if (len > XXX) return -EINVAL; > + > + writeb(len * 8, mt8173_nor->base + MTK_NOR_CNT_REG); > + > + /* start at PRGDATA5, go down to PRGDATA0 */ > + idx = MTK_NOR_MAX_SHIFT - 1; > + > + /* opcode */ > + writeb(op, mt8173_nor->base + MTK_NOR_PRG_REG(idx)); > + idx--; > + > + /* program TX data */ > + for (i = 0; i < txlen; i++, idx--) > + writeb(tx[i], mt8173_nor->base + MTK_NOR_PRG_REG(idx)); > + > + /* clear out rest of TX registers */ > + while (idx >= 0) { > + writeb(0, mt8173_nor->base + MTK_NOR_PRG_REG(idx)); > + idx--; > + } > + > + ret = mt8173_nor_execute_cmd(mt8173_nor, MTK_NOR_PRG_CMD); > + if (ret) > + return ret; > + > + /* restart at first RX byte */ > + idx = rxlen - 1; > + > + /* read out RX data */ > + for (i = 0; i < rxlen; i++, idx--) > + rx[i] = readb(mt8173_nor->base + MTK_NOR_SHREG(idx)); > + > + return 0; > +} > + ... > +static int __init mtk_nor_init(struct mt8173_nor *mt8173_nor, > + struct device_node *flash_node) > +{ > + struct mtd_part_parser_data ppdata = { > + .of_node = flash_node, > + }; > + int ret; > + struct spi_nor *nor; Normally we'd have a blank line here. > + /* initialize controller to accept commands */ > + writel(MTK_NOR_ENABLE_SF_CMD, mt8173_nor->base + MTK_NOR_WRPROT_REG); > + > + nor = &mt8173_nor->nor; > + nor->dev = mt8173_nor->dev; > + nor->priv = mt8173_nor; > + nor->flash_node = flash_node; > + > + /* fill the hooks to spi nor */ > + nor->read = mt8173_nor_read; > + nor->read_reg = mt8173_nor_read_reg; > + nor->write = mt8173_nor_write; > + nor->write_reg = mt8173_nor_write_reg; > + nor->mtd.owner = THIS_MODULE; > + nor->mtd.name = "mtk_nor"; > + /* initialized with NULL */ > + ret = spi_nor_scan(nor, NULL, SPI_NOR_DUAL); > + if (ret) > + return ret; > + > + return mtd_device_parse_register(&nor->mtd, NULL, &ppdata, NULL, 0); > +} ... Brian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/