Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1258525 > unrolled thread

Re: [PATCH v4 2/3] mtd: mtk-nor: mtk serial flash controller driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 2/3] mtd: mtk-nor: mtk serial flash controller driver Brian Norris <computersforpeace@gmail.com> - 2015-10-29 03:00 +0100
    Re: [PATCH v4 2/3] mtd: mtk-nor: mtk serial flash controller driver Brian Norris <computersforpeace@gmail.com> - 2015-10-29 17:10 +0100
      Re: [PATCH v4 2/3] mtd: mtk-nor: mtk serial flash controller driver Brian Norris <computersforpeace@gmail.com> - 2015-10-30 06:10 +0100

#1258525 — Re: [PATCH v4 2/3] mtd: mtk-nor: mtk serial flash controller driver

Hi Bayi,

I'm looking over your v5, and I still have a lot of comments. I'll send
those soon. But I still question one of your responses here:

On Sun, Oct 18, 2015 at 10:20:35PM +0800, bayi.cheng wrote:
> On Fri, 2015-10-16 at 00:39 -0700, Brian Norris wrote:
> > On Tue, Oct 13, 2015 at 05:39:19PM +0800, Bayi Cheng wrote:

> > > diff --git a/drivers/mtd/spi-nor/mtk-quadspi.c b/drivers/mtd/spi-nor/mtk-quadspi.c
> > > new file mode 100644
> > > index 0000000..c6ac366
> > > --- /dev/null
> > > +++ b/drivers/mtd/spi-nor/mtk-quadspi.c
> > > @@ -0,0 +1,486 @@
[...]
> > > +#define MTK_NOR_CMD_REG			0x00
> > > +#define MTK_NOR_CNT_REG			0x04
> > > +#define MTK_NOR_RDSR_REG		0x08
> > > +#define MTK_NOR_RDATA_REG		0x0c
> > > +#define MTK_NOR_RADR0_REG		0x10
> > > +#define MTK_NOR_RADR1_REG		0x14
> > > +#define MTK_NOR_RADR2_REG		0x18
> > > +#define MTK_NOR_WDATA_REG		0x1c
> > > +#define MTK_NOR_PRGDATA0_REG		0x20
> > > +#define MTK_NOR_PRGDATA1_REG		0x24
> > > +#define MTK_NOR_PRGDATA2_REG		0x28
> > > +#define MTK_NOR_PRGDATA3_REG		0x2c
> > > +#define MTK_NOR_PRGDATA4_REG		0x30
> > > +#define MTK_NOR_PRGDATA5_REG		0x34

^^ so you have 6 "TX" registers.

> > > +#define MTK_NOR_SHREG0_REG		0x38
> > > +#define MTK_NOR_SHREG1_REG		0x3c
> > > +#define MTK_NOR_SHREG2_REG		0x40
> > > +#define MTK_NOR_SHREG3_REG		0x44
> > > +#define MTK_NOR_SHREG4_REG		0x48
> > > +#define MTK_NOR_SHREG5_REG		0x4c

^^ and you have at least 6 "RX" registers (looking at the mt8173 manual,
I'm not sure what the SHREG{6..9} are for). So I don't see why you can't
program MTK_NOR_CNT_REG to 48 (6 bytes * 8 bits/byte) to get 6 bytes
TX/RX, or IOW, 1 byte of opcode and 5 bytes of either RX or TX data. So
I'm still not convinced about your claim below.

I was able to rewrite your driver to do the above, and I can read out 5
bytes no problem, but unfortunately, the flash device I have has only 3
bytes of ID, so the last two bytes are zero, which doesn't tell me too
much if this is working right.

> > > +#define MTK_NOR_SHREG6_REG		0x50
> > > +#define MTK_NOR_SHREG7_REG		0x54
> > > +#define MTK_NOR_SHREG8_REG		0x58
> > > +#define MTK_NOR_SHREG9_REG		0x5c
> > > +#define MTK_NOR_CFG1_REG		0x60
> > > +#define MTK_NOR_CFG2_REG		0x64
> > > +#define MTK_NOR_CFG3_REG		0x68
> > > +#define MTK_NOR_STATUS0_REG		0x70
> > > +#define MTK_NOR_STATUS1_REG		0x74
> > > +#define MTK_NOR_STATUS2_REG		0x78
> > > +#define MTK_NOR_STATUS3_REG		0x7c
> > > +#define MTK_NOR_FLHCFG_REG		0x84
> > > +#define MTK_NOR_TIME_REG		0x94
> > > +#define MTK_NOR_PP_DATA_REG		0x98
> > > +#define MTK_NOR_PREBUF_STUS_REG		0x9c
> > > +#define MTK_NOR_DELSEL0_REG		0xa0
> > > +#define MTK_NOR_DELSEL1_REG		0xa4
> > > +#define MTK_NOR_INTRSTUS_REG		0xa8
> > > +#define MTK_NOR_INTREN_REG		0xac
> > > +#define MTK_NOR_CHKSUM_CTL_REG		0xb8
> > > +#define MTK_NOR_CHKSUM_REG		0xbc
> > > +#define MTK_NOR_CMD2_REG		0xc0
> > > +#define MTK_NOR_WRPROT_REG		0xc4
> > > +#define MTK_NOR_RADR3_REG		0xc8
> > > +#define MTK_NOR_DUAL_REG		0xcc
> > > +#define MTK_NOR_DELSEL2_REG		0xd0
> > > +#define MTK_NOR_DELSEL3_REG		0xd4
> > > +#define MTK_NOR_DELSEL4_REG		0xd8

...

> > > +static int mt8173_nor_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
> > > +{
> > > +	int ret;
> > > +	struct mt8173_nor *mt8173_nor = nor->priv;
> > > +
> > > +	/* mtk nor controller doesn't supoort SPINOR_OP_RDCR */
> > > +	switch (opcode) {
> > > +	case SPINOR_OP_RDID:
> > > +		/* read JEDEC ID need 4 bytes commands */
> > > +		ret = mt8173_nor_set_cmd(mt8173_nor, 0, 32, SPINOR_OP_RDID);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > > +		/* mtk nor flash controller only support 3 bytes IDs */
> > 
> > Are you absolutely sure of this? That would be highly unfortunate, but
> > I also don't believe it's true.
> > 
> Yes, for this issue I have asked our designer of nor flash controller,
> unfortunately, it is true, and I have tried to read more IDs, but our
> controller just accept 3 IDs from nor flash, and our next generation IC
> may solve this problem.

How exactly did you try? Did you do what I suggested above? Are the
"shift registers" not all actually functional?

> > > +		buf[2] = readb(mt8173_nor->base + MTK_NOR_SHREG0_REG);
> > > +		buf[1] = readb(mt8173_nor->base + MTK_NOR_SHREG1_REG);
> > > +		buf[0] = readb(mt8173_nor->base + MTK_NOR_SHREG2_REG);
> > > +		break;

[...]

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/

[toc] | [next] | [standalone]

#1258886

On Thu, Oct 29, 2015 at 09:56:31PM +0800, bayi.cheng wrote:
> On Thu, 2015-10-29 at 11:28 +0800, bayi.cheng wrote:
> > On Wed, 2015-10-28 at 18:52 -0700, Brian Norris wrote:
> > > On Sun, Oct 18, 2015 at 10:20:35PM +0800, bayi.cheng wrote:
> > > > On Fri, 2015-10-16 at 00:39 -0700, Brian Norris wrote:
> > > > > On Tue, Oct 13, 2015 at 05:39:19PM +0800, Bayi Cheng wrote:

> > > > > > +	/* mtk nor controller doesn't supoort SPINOR_OP_RDCR */
> > > > > > +	switch (opcode) {
> > > > > > +	case SPINOR_OP_RDID:
> > > > > > +		/* read JEDEC ID need 4 bytes commands */
> > > > > > +		ret = mt8173_nor_set_cmd(mt8173_nor, 0, 32, SPINOR_OP_RDID);
> > > > > > +		if (ret < 0)
> > > > > > +			return ret;
> > > > > > +
> > > > > > +		/* mtk nor flash controller only support 3 bytes IDs */
> > > > > 
> > > > > Are you absolutely sure of this? That would be highly unfortunate, but
> > > > > I also don't believe it's true.
> > > > > 
> > > > Yes, for this issue I have asked our designer of nor flash controller,
> > > > unfortunately, it is true, and I have tried to read more IDs, but our
> > > > controller just accept 3 IDs from nor flash, and our next generation IC
> > > > may solve this problem.
> > > 
> > > How exactly did you try? Did you do what I suggested above? Are the
> > > "shift registers" not all actually functional?
> > > 
> > Hi Brian, For this problem, I have asked our nor controller designer to
> > double confirm again, and he promise to do some simulation testes, On
> > the other hand, I have got some Spansion nor flash which has 5 IDs, and
> > after our designer make sure our controller can support 5 or 6 IDs, then
> > I will rework a special platform with S25FL256SA. If we have any
> > progress, I will inform you immediately!  Thanks!!
> > 
> Hi Brian, Thanks very much for your perseverance!!
> Actually, You idea is correct, and our designer has completed some
> simulation test on spansion nor flash witch has more than 3 IDs, and the
> results has prove that we can support to read 5 IDs. I also double

Awesome!

> confirmed with spansion nor flash. The only pity is that our nor
> controller can't support 6 IDs, we just support 5 IDs.

That's not quite as awesome. But that's still much more workable than
only 3 bytes. Perhaps we'll want a way to communicate that to the
spi-nor layer, so it doesn't think it can match a full 6 bytes?

> I will submit my changes to V6 version. Thanks again!!

OK, but let me take another look at v5 and send you comments on that one
first.

Regards,
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/

[toc] | [prev] | [next] | [standalone]

#1259203

Hi Bayi,

On Fri, Oct 30, 2015 at 10:12:39AM +0800, bayi.cheng wrote:
> Hi Brian,  The current station is as follows.
> 
> 1: put 0x9F to MTK_NOR_PRGDATA5_REG, and five 0x0 to
> MTK_NOR_PRGDATA4_REG ~ MTK_NOR_PRGDATA0_REG, then set (1 + 5) * 8 
> to MTK_NOR_CNT_REG, for this way, we can read five IDs.
> 
> 2: put 0x9F to MTK_NOR_PRGDATA5_REG, and five 0x0 to
> MTK_NOR_PRGDATA4_REG ~ MTK_NOR_PRGDATA0_REG, then set (1 + 5 + 1) * 8
> to MTK_NOR_CNT_REG, for this way, we can read six IDs.
> In this case, nor flash IDs can be read from MTK_NOR_SHREG5_REG to
> MTK_NOR_SHREG0_REG . Thanks!

Thanks for the update. Glad to hear you can read more bytes there; the
extra number of Shift Registers *looked* to me like you should be able
to read even more than 5...

So in my other email, I showed you how I generalized the TX/RX function,
so it can handle most of both the write_reg() and read_reg() functions.
I'm not 100% sure now that it has all of the RX path correct; it worked
for the cases I could test, but it's confusing when reading the manual
to figure out which SHREG register I should start from. But anyway, I
think it should only take some small adjustments to my patch to make it
handle things properly.

I'd really appreciate it if you could incorporate my feedback and
review/improve the ..._do_tx_rx() function I wrote, to make sure it
handles reading any arbitrary number of bytes (at least up to 6). So,
you might, for example, run some tests where you have spi-nor.c call

	nor->read_reg(nor, SPINOR_OP_RDID, id, idlen)

with varying values for 'idlen', and make sure they all work properly.

Let me know if you have any questions about comments either here, or on
v5.

Regards,
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web