linux-kernel - Re: [PATCH v6 0/3] Mediatek SPI-NOR flash driver

Open Source and information security mailing list archives

Date:	Fri, 13 Nov 2015 14:45:07 +0800
From:	bayi cheng <bayi.cheng@...iatek.com>
To:	Brian Norris <computersforpeace@...il.com>
CC:	Mark Rutland <mark.rutland@....com>, <devicetree@...r.kernel.org>,
	<srv_heupstream@...iatek.com>, Pawel Moll <pawel.moll@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	<linux-kernel@...r.kernel.org>, <jteki@...nedev.com>,
	Rob Herring <robh+dt@...nel.org>,
	<linux-mediatek@...ts.infradead.org>,
	<ezequiel@...guardiasur.com.ar>, Kumar Gala <galak@...eaurora.org>,
	"Matthias Brugger" <matthias.bgg@...il.com>,
	<linux-mtd@...ts.infradead.org>,
	"David Woodhouse" <dwmw2@...radead.org>,
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v6 0/3] Mediatek SPI-NOR flash driver

On Wed, 2015-11-11 at 12:26 -0800, Brian Norris wrote:
> On Wed, Nov 11, 2015 at 10:04:14PM +0800, Bayi Cheng wrote:
> > On Mon, 2015-11-09 at 18:46 -0800, Brian Norris wrote:
> > > I believe you didn't completely answer all my questions from v5 though.
> > > I'll repeat a bit here. Particularly, refer to [1].
> > > I'll summarize; I understand that your common transmit/receive operation
> > > works something like this:
> > > 
> > > Quoting from [1]:
> > > > (1) total number of bits to send/receive goes in the COUNT register (so
> > > >     far, as many as 7*8=56?)
> > > > (2) opcode is written to PRGDATA5
> > > > (3) other "transmit" data (like addresses), if any, are placed on PRGDATA4..0
> > > > (4) command is sent (execute_cmd())
> > > > (5) data is read back in SHREG{X..0}, if needed
> > > 
> > > My questions were:
> > > 
> > > (a) Why does mt8173_nor_set_read_mode() use PRGDATA3? That's not
> > >     mentioned in the SoC manual, and it doesn't really match any of the
> > >     steps above. Perhaps it's just a quirk of the controller's
> > >     programming model?
> > > 
> > yes, for this question, I have done some testes, If I change the
> > PRGDATA3 to PRGDATA5 for mt8173_nor_set_read_mode() like others
> > functions, then the controller will be hanged, and I have asked our
> > designer for double confirm.
> 
> I wasn't suggesting to change this to PRGDATA5. I just was wondering why
> the difference. It's not documented. (I suppose an acceptable answer is
> just "because that's how the HW works.")
> 
I have synced with our designer, and just as you said, that's how the HW
works, and the read operation is different is different from other
operation. By the way, I have double confirm our driver code, and I have
made a mistake for quad read operation, the quad command need use
PRGDATA4 Instead PRGDATA3.
PS: write PRGDATA4 0xEB(4bit I/O read mode) or 0x6B(4 bit output mode),
So I will modify mt8173_nor_set_read_mode function in next patch.

> > > (b) How do you determine X from step (5)?
> > > 
> > > Right now, your code seems to answer that X is "rxlen - 1". Correct?
> > > 
> > yes, I have used "rxlen -1", because the first of nor flash output is
> > located at SHREG[0], in the other words, the output data starts at
> > SHREG[0] and go up to SHREG[relen -1]
> 
> But, we aren't reading from SHREG[0] first; we're reading backwards from
> SHREG[rxlen - 1] down to SHREG[0]. It seems that's correct, right?

Yes, You are right!
> 
> > > If that's correct and if I put all of my understanding together
> > > correctly, this means that you can actually shift out (in PRGDATA) up to
> > > 6 bytes (that is, 1 opcode and 5 tx bytes) and shift in (in SHREG) up to
> > > 7 bytes, except that the first byte is received during the opcode cycle,
> > > and so it is discarded, and we effectively receive only 6 bytes.
> > > 
> > > Is that all correct? If so, then I think you still need to adjust the
> > > boundary conditions in your do_tx_rx() function. (I'll comment on the
> > > driver to point out the specifics.)
> > 
> > Yes, you are right! and I will adjust the boundary conditions in
> > do_tx_rx() function.
> 
> OK, good. BTW, can you make sure to rewrite the appropriate MAX macro(s)
> to reflect the right values? It seems like maybe you'll want separate
> macros for the maximum TX and RX -- and total (?), or is this just the
> same as RX? -- since they seem to have different limits.
> 

OK, I will use two MAX macros, one is for TX&RX, and the other is for 
the total.

> > By the way, could you tell me whether I need to publish a new patch? or
> > you can fix them up directly?
> 
> I think there are a few more adjustments to make, so please just post a
> new version of the driver only. The DT binding and DTS changes look good
> to go now.

Yes, I will publish a new patch next week, and Thanks again for your
grate help and support!

> 
> Regards,
> Brian
> 
> > > [1] http://lists.infradead.org/pipermail/linux-mtd/2015-October/062951.html
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives