From: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr> To: Guochun Mao <guochun.mao@mediatek.com>, Boris Brezillon <boris.brezillon@free-electrons.com>, Marek Vasut <marek.vasut@gmail.com> Cc: Mark Rutland <mark.rutland@arm.com>, devicetree@vger.kernel.org, Richard Weinberger <richard@nod.at>, Russell King <linux@armlinux.org.uk>, linux-kernel@vger.kernel.org, Rob Herring <robh+dt@kernel.org>, linux-mtd@lists.infradead.org, Matthias Brugger <matthias.bgg@gmail.com>, linux-mediatek@lists.infradead.org, Cyrille Pitchen <cyrille.pitchen@atmel.com>, David Woodhouse <dwmw2@infradead.org>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH V1 1/1] mtd: mtk-nor: set controller to 4B mode with large capacity flash Date: Thu, 30 Mar 2017 19:40:09 +0200 [thread overview] Message-ID: <24d60072-4124-7ec4-6cd3-fd70f4f6b10a@wedev4u.fr> (raw) In-Reply-To: <1490862222-723-2-git-send-email-guochun.mao@mediatek.com> Hi Guochun, Le 30/03/2017 à 10:23, Guochun Mao a écrit : > when nor's size larger than 16MByte, nor and controller should > enter 4Byte mode simultaneously. > > Signed-off-by: Guochun Mao <guochun.mao@mediatek.com> > --- > drivers/mtd/spi-nor/mtk-quadspi.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/mtd/spi-nor/mtk-quadspi.c b/drivers/mtd/spi-nor/mtk-quadspi.c > index e661877..05cd8a8 100644 > --- a/drivers/mtd/spi-nor/mtk-quadspi.c > +++ b/drivers/mtd/spi-nor/mtk-quadspi.c > @@ -369,6 +369,13 @@ static int mt8173_nor_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, > /* We only handle 1 byte */ > ret = mt8173_nor_wr_sr(mt8173_nor, *buf); > break; > + case SPINOR_OP_EN4B: > + /* Set nor controller to 4-byte address mode, > + * and simultaneously set nor flash. > + * This case should cooperate with default operation. > + */ > + writeb(readb(mt8173_nor->base + MTK_NOR_DUAL_REG) | 0x10, > + mt8173_nor->base + MTK_NOR_DUAL_REG); This is not good: you should check in both mt8173_nor_read() and mt8173_nor_write() whether nor->addr_width is either 3 or 4. from include/linux/mtd/spi-nor.h: * @addr_width: number of address bytes Besides SPI commands using an op code from 4-byte address instruction set always carry a 4-byte address. They can be used directly, without sending the SPINOR_OP_EN4B before. So you cannot assume that addresses will be 4-byte long only if your SPI controller driver has seen a SPINOR_OP_EN4B command before. This assumption is wrong. SPI controller driver should never check SPINOR_OP_* op codes like this. Then, testing SPINOR_OP_RDSR from mt8173_nor_read_reg() or SPINOR_OP_WRSR from mt8173_nor_write_reg() is not a good practice too: op codes may change depending on the memory manufacturer. So testing op code values like you do can work with some memories but maybe not all. Finally, don't use 0x10, please define a macro instead. Best regards, Cyrille > default: > ret = mt8173_nor_do_tx_rx(mt8173_nor, opcode, buf, len, NULL, 0); > if (ret) >
WARNING: multiple messages have this Message-ID (diff)
From: cyrille.pitchen@wedev4u.fr (Cyrille Pitchen) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH V1 1/1] mtd: mtk-nor: set controller to 4B mode with large capacity flash Date: Thu, 30 Mar 2017 19:40:09 +0200 [thread overview] Message-ID: <24d60072-4124-7ec4-6cd3-fd70f4f6b10a@wedev4u.fr> (raw) In-Reply-To: <1490862222-723-2-git-send-email-guochun.mao@mediatek.com> Hi Guochun, Le 30/03/2017 ? 10:23, Guochun Mao a ?crit : > when nor's size larger than 16MByte, nor and controller should > enter 4Byte mode simultaneously. > > Signed-off-by: Guochun Mao <guochun.mao@mediatek.com> > --- > drivers/mtd/spi-nor/mtk-quadspi.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/mtd/spi-nor/mtk-quadspi.c b/drivers/mtd/spi-nor/mtk-quadspi.c > index e661877..05cd8a8 100644 > --- a/drivers/mtd/spi-nor/mtk-quadspi.c > +++ b/drivers/mtd/spi-nor/mtk-quadspi.c > @@ -369,6 +369,13 @@ static int mt8173_nor_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, > /* We only handle 1 byte */ > ret = mt8173_nor_wr_sr(mt8173_nor, *buf); > break; > + case SPINOR_OP_EN4B: > + /* Set nor controller to 4-byte address mode, > + * and simultaneously set nor flash. > + * This case should cooperate with default operation. > + */ > + writeb(readb(mt8173_nor->base + MTK_NOR_DUAL_REG) | 0x10, > + mt8173_nor->base + MTK_NOR_DUAL_REG); This is not good: you should check in both mt8173_nor_read() and mt8173_nor_write() whether nor->addr_width is either 3 or 4. from include/linux/mtd/spi-nor.h: * @addr_width: number of address bytes Besides SPI commands using an op code from 4-byte address instruction set always carry a 4-byte address. They can be used directly, without sending the SPINOR_OP_EN4B before. So you cannot assume that addresses will be 4-byte long only if your SPI controller driver has seen a SPINOR_OP_EN4B command before. This assumption is wrong. SPI controller driver should never check SPINOR_OP_* op codes like this. Then, testing SPINOR_OP_RDSR from mt8173_nor_read_reg() or SPINOR_OP_WRSR from mt8173_nor_write_reg() is not a good practice too: op codes may change depending on the memory manufacturer. So testing op code values like you do can work with some memories but maybe not all. Finally, don't use 0x10, please define a macro instead. Best regards, Cyrille > default: > ret = mt8173_nor_do_tx_rx(mt8173_nor, opcode, buf, len, NULL, 0); > if (ret) >
next prev parent reply other threads:[~2017-03-30 18:59 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-03-30 8:23 [PATCH V1] mtd: mtk-nor: set controller to 4B mode with large capacity flash Guochun Mao 2017-03-30 8:23 ` Guochun Mao 2017-03-30 8:23 ` Guochun Mao 2017-03-30 8:23 ` [PATCH V1 1/1] " Guochun Mao 2017-03-30 8:23 ` Guochun Mao 2017-03-30 8:23 ` Guochun Mao 2017-03-30 10:00 ` Marek Vasut 2017-03-30 10:00 ` Marek Vasut 2017-03-30 17:40 ` Cyrille Pitchen [this message] 2017-03-30 17:40 ` Cyrille Pitchen 2017-03-31 2:26 ` Guochun Mao 2017-03-31 2:26 ` Guochun Mao 2017-03-31 2:26 ` Guochun Mao 2017-03-31 8:56 ` Cyrille Pitchen 2017-03-31 8:56 ` Cyrille Pitchen 2017-04-05 3:34 ` Guochun Mao 2017-04-05 3:34 ` Guochun Mao 2017-04-05 3:34 ` Guochun Mao
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=24d60072-4124-7ec4-6cd3-fd70f4f6b10a@wedev4u.fr \ --to=cyrille.pitchen@wedev4u.fr \ --cc=boris.brezillon@free-electrons.com \ --cc=cyrille.pitchen@atmel.com \ --cc=devicetree@vger.kernel.org \ --cc=dwmw2@infradead.org \ --cc=guochun.mao@mediatek.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mediatek@lists.infradead.org \ --cc=linux-mtd@lists.infradead.org \ --cc=linux@armlinux.org.uk \ --cc=marek.vasut@gmail.com \ --cc=mark.rutland@arm.com \ --cc=matthias.bgg@gmail.com \ --cc=richard@nod.at \ --cc=robh+dt@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.