Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
On Tue, Jan 12, 2021 at 9:10 AM Bin Meng wrote:
>
> On Wed, Jan 6, 2021 at 10:21 PM Bin Meng wrote:
> >
> > Hi Francisco,
> >
> > On Tue, Dec 22, 2020 at 9:40 AM Bin Meng wrote:
> > >
> > > Hi Francisco,
> > >
> > > On Wed, Dec 16, 2020 at 6:11 PM Bin Meng wrote:
> > > >
> > > > Hi Francisco,
> > > >
> > > > On Wed, Dec 16, 2020 at 12:40 AM Francisco Iglesias
> > > > wrote:
> > > > >
> > > > > Hello Bin,
> > > > >
> > > > > On [2020 Dec 12] Sat 17:44:27, Bin Meng wrote:
> > > > > > Hi Francisco,
> > > > > >
> > > > > > On Sat, Dec 12, 2020 at 5:24 PM Francisco Iglesias
> > > > > > wrote:
> > > > > > >
> > > > > > > Hi bin,
> > > > > > >
> > > > > > > On [2020 Dec 12] Sat 16:16:59, Bin Meng wrote:
> > > > > > > > Hi Francisco,
> > > > > > > >
> > > > > > > > On Sat, Dec 12, 2020 at 12:11 AM Francisco Iglesias
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > Hello Bin,
> > > > > > > > >
> > > > > > > > > On [2020 Dec 11] Fri 23:29:16, Bin Meng wrote:
> > > > > > > > > > Hi Francisco,
> > > > > > > > > >
> > > > > > > > > > On Fri, Dec 11, 2020 at 11:16 PM Francisco Iglesias
> > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hello Bin,
> > > > > > > > > > >
> > > > > > > > > > > On [2020 Dec 11] Fri 14:07:21, Bin Meng wrote:
> > > > > > > > > > > > Hi Francisco,
> > > > > > > > > > > >
> > > > > > > > > > > > On Fri, Dec 4, 2020 at 7:28 PM Francisco Iglesias
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hello Bin,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On [2020 Dec 04] Fri 18:52:50, Bin Meng wrote:
> > > > > > > > > > > > > > Hi Francisco,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Fri, Dec 4, 2020 at 6:46 PM Francisco Iglesias
> > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Hello Bin,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > On [2020 Dec 04] Fri 15:52:12, Bin Meng wrote:
> > > > > > > > > > > > > > > > Hi Francisco,
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > On Thu, Dec 3, 2020 at 4:38 PM Francisco
> > > > > > > > > > > > > > > > Iglesias
> > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Hi Bin and Alistair,
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > On [2020 Dec 02] Wed 11:40:11, Alistair
> > > > > > > > > > > > > > > > > Francis wrote:
> > > > > > > > > > > > > > > > > > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng
> > > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > From: Bin Meng
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > SST flashes require a dummy byte after
> > > > > > > > > > > > > > > > > > > the address bits.
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > Signed-off-by: Bin Meng
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > I couldn't find a datasheet that says
> > > > > > > > > > > > > > > > > > this... But the actual code
> > > > > > > > > > > > > > > > > > change looks fine, so:
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > Acked-by: Alistair Francis
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > Alistair
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > hw/block/m25p80.c | 3 +++
> > > > > > > > > > > > > > > > > > > 1 file changed, 3 insertions(+)
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > diff --git a/hw/block/m25p80.c
> > > > > > > > > > > > > > > > > > > b/hw/block/m25p80.c
> > > > > > > > > > > > > > > > > > > index 483925f..9b36762 100644
> > > > > > > > > > > > > > > > > > > --- a/hw/block/m25p80.c
> > > > > > > > > > > > > > > > > > > +++ b/hw/block/m25p80.c
> > > > > > > > > > > > > > > > > > > @@ -825,6 +825,9 @@ static void
> > > > > > > > > > > > > > > > > > > decode_fast_read_cmd(Flash *s)
> > > > > > > > > > > > > > > > > > > s->needed_bytes = get_addr_length(s);
> > > > > > > > > > > > > > > > > > > switch (get_man(s)) {
> > > > > > > > > > > > > > > > > > > /* Dummy cycles - modeled with bytes
> > > > > > > > > > > > > > > > > > > writes instead of bits */
> > > > > > > > > > > > > > > > > > > +case MAN_SST:
> > > > > > > > > > > > > > > > > > > +s->needed_bytes += 1;
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > 1 dummy clk cycle is modelled as 1 byte write
> > > > > > > > > > > > > > > > > (see the comment above), so 1
> > > > > > > > > > > > > > > > > dummy byte (8 dummy clk cycles) will need +8
> > > > > > > > > > > > > > > > > above.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > I think you were confused by the
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
On Wed, Jan 6, 2021 at 10:21 PM Bin Meng wrote:
>
> Hi Francisco,
>
> On Tue, Dec 22, 2020 at 9:40 AM Bin Meng wrote:
> >
> > Hi Francisco,
> >
> > On Wed, Dec 16, 2020 at 6:11 PM Bin Meng wrote:
> > >
> > > Hi Francisco,
> > >
> > > On Wed, Dec 16, 2020 at 12:40 AM Francisco Iglesias
> > > wrote:
> > > >
> > > > Hello Bin,
> > > >
> > > > On [2020 Dec 12] Sat 17:44:27, Bin Meng wrote:
> > > > > Hi Francisco,
> > > > >
> > > > > On Sat, Dec 12, 2020 at 5:24 PM Francisco Iglesias
> > > > > wrote:
> > > > > >
> > > > > > Hi bin,
> > > > > >
> > > > > > On [2020 Dec 12] Sat 16:16:59, Bin Meng wrote:
> > > > > > > Hi Francisco,
> > > > > > >
> > > > > > > On Sat, Dec 12, 2020 at 12:11 AM Francisco Iglesias
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > Hello Bin,
> > > > > > > >
> > > > > > > > On [2020 Dec 11] Fri 23:29:16, Bin Meng wrote:
> > > > > > > > > Hi Francisco,
> > > > > > > > >
> > > > > > > > > On Fri, Dec 11, 2020 at 11:16 PM Francisco Iglesias
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > Hello Bin,
> > > > > > > > > >
> > > > > > > > > > On [2020 Dec 11] Fri 14:07:21, Bin Meng wrote:
> > > > > > > > > > > Hi Francisco,
> > > > > > > > > > >
> > > > > > > > > > > On Fri, Dec 4, 2020 at 7:28 PM Francisco Iglesias
> > > > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Hello Bin,
> > > > > > > > > > > >
> > > > > > > > > > > > On [2020 Dec 04] Fri 18:52:50, Bin Meng wrote:
> > > > > > > > > > > > > Hi Francisco,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Fri, Dec 4, 2020 at 6:46 PM Francisco Iglesias
> > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Hello Bin,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On [2020 Dec 04] Fri 15:52:12, Bin Meng wrote:
> > > > > > > > > > > > > > > Hi Francisco,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > On Thu, Dec 3, 2020 at 4:38 PM Francisco Iglesias
> > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Hi Bin and Alistair,
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > On [2020 Dec 02] Wed 11:40:11, Alistair Francis
> > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng
> > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > From: Bin Meng
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > SST flashes require a dummy byte after the
> > > > > > > > > > > > > > > > > > address bits.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > Signed-off-by: Bin Meng
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > I couldn't find a datasheet that says this...
> > > > > > > > > > > > > > > > > But the actual code
> > > > > > > > > > > > > > > > > change looks fine, so:
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Acked-by: Alistair Francis
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Alistair
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > hw/block/m25p80.c | 3 +++
> > > > > > > > > > > > > > > > > > 1 file changed, 3 insertions(+)
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > diff --git a/hw/block/m25p80.c
> > > > > > > > > > > > > > > > > > b/hw/block/m25p80.c
> > > > > > > > > > > > > > > > > > index 483925f..9b36762 100644
> > > > > > > > > > > > > > > > > > --- a/hw/block/m25p80.c
> > > > > > > > > > > > > > > > > > +++ b/hw/block/m25p80.c
> > > > > > > > > > > > > > > > > > @@ -825,6 +825,9 @@ static void
> > > > > > > > > > > > > > > > > > decode_fast_read_cmd(Flash *s)
> > > > > > > > > > > > > > > > > > s->needed_bytes = get_addr_length(s);
> > > > > > > > > > > > > > > > > > switch (get_man(s)) {
> > > > > > > > > > > > > > > > > > /* Dummy cycles - modeled with bytes
> > > > > > > > > > > > > > > > > > writes instead of bits */
> > > > > > > > > > > > > > > > > > +case MAN_SST:
> > > > > > > > > > > > > > > > > > +s->needed_bytes += 1;
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > 1 dummy clk cycle is modelled as 1 byte write
> > > > > > > > > > > > > > > > (see the comment above), so 1
> > > > > > > > > > > > > > > > dummy byte (8 dummy clk cycles) will need +8
> > > > > > > > > > > > > > > > above.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > I think you were confused by the WINBOND codes.
> > > > > > > > > > > > > > > The comments are
> > > > > > > > > > > > > > > correct. It is modeled with bytes instead of
> > > > > > > > > > > > > > > bits, so we should +=1.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > What the comment says is (perhaps not superclear)
> > > > > >
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
Hi Francisco,
On Tue, Dec 22, 2020 at 9:40 AM Bin Meng wrote:
>
> Hi Francisco,
>
> On Wed, Dec 16, 2020 at 6:11 PM Bin Meng wrote:
> >
> > Hi Francisco,
> >
> > On Wed, Dec 16, 2020 at 12:40 AM Francisco Iglesias
> > wrote:
> > >
> > > Hello Bin,
> > >
> > > On [2020 Dec 12] Sat 17:44:27, Bin Meng wrote:
> > > > Hi Francisco,
> > > >
> > > > On Sat, Dec 12, 2020 at 5:24 PM Francisco Iglesias
> > > > wrote:
> > > > >
> > > > > Hi bin,
> > > > >
> > > > > On [2020 Dec 12] Sat 16:16:59, Bin Meng wrote:
> > > > > > Hi Francisco,
> > > > > >
> > > > > > On Sat, Dec 12, 2020 at 12:11 AM Francisco Iglesias
> > > > > > wrote:
> > > > > > >
> > > > > > > Hello Bin,
> > > > > > >
> > > > > > > On [2020 Dec 11] Fri 23:29:16, Bin Meng wrote:
> > > > > > > > Hi Francisco,
> > > > > > > >
> > > > > > > > On Fri, Dec 11, 2020 at 11:16 PM Francisco Iglesias
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > Hello Bin,
> > > > > > > > >
> > > > > > > > > On [2020 Dec 11] Fri 14:07:21, Bin Meng wrote:
> > > > > > > > > > Hi Francisco,
> > > > > > > > > >
> > > > > > > > > > On Fri, Dec 4, 2020 at 7:28 PM Francisco Iglesias
> > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hello Bin,
> > > > > > > > > > >
> > > > > > > > > > > On [2020 Dec 04] Fri 18:52:50, Bin Meng wrote:
> > > > > > > > > > > > Hi Francisco,
> > > > > > > > > > > >
> > > > > > > > > > > > On Fri, Dec 4, 2020 at 6:46 PM Francisco Iglesias
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hello Bin,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On [2020 Dec 04] Fri 15:52:12, Bin Meng wrote:
> > > > > > > > > > > > > > Hi Francisco,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Thu, Dec 3, 2020 at 4:38 PM Francisco Iglesias
> > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Hi Bin and Alistair,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > On [2020 Dec 02] Wed 11:40:11, Alistair Francis
> > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng
> > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > From: Bin Meng
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > SST flashes require a dummy byte after the
> > > > > > > > > > > > > > > > > address bits.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Signed-off-by: Bin Meng
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > I couldn't find a datasheet that says this...
> > > > > > > > > > > > > > > > But the actual code
> > > > > > > > > > > > > > > > change looks fine, so:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Acked-by: Alistair Francis
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Alistair
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > hw/block/m25p80.c | 3 +++
> > > > > > > > > > > > > > > > > 1 file changed, 3 insertions(+)
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > diff --git a/hw/block/m25p80.c
> > > > > > > > > > > > > > > > > b/hw/block/m25p80.c
> > > > > > > > > > > > > > > > > index 483925f..9b36762 100644
> > > > > > > > > > > > > > > > > --- a/hw/block/m25p80.c
> > > > > > > > > > > > > > > > > +++ b/hw/block/m25p80.c
> > > > > > > > > > > > > > > > > @@ -825,6 +825,9 @@ static void
> > > > > > > > > > > > > > > > > decode_fast_read_cmd(Flash *s)
> > > > > > > > > > > > > > > > > s->needed_bytes = get_addr_length(s);
> > > > > > > > > > > > > > > > > switch (get_man(s)) {
> > > > > > > > > > > > > > > > > /* Dummy cycles - modeled with bytes
> > > > > > > > > > > > > > > > > writes instead of bits */
> > > > > > > > > > > > > > > > > +case MAN_SST:
> > > > > > > > > > > > > > > > > +s->needed_bytes += 1;
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > 1 dummy clk cycle is modelled as 1 byte write
> > > > > > > > > > > > > > > (see the comment above), so 1
> > > > > > > > > > > > > > > dummy byte (8 dummy clk cycles) will need +8
> > > > > > > > > > > > > > > above.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I think you were confused by the WINBOND codes. The
> > > > > > > > > > > > > > comments are
> > > > > > > > > > > > > > correct. It is modeled with bytes instead of bits,
> > > > > > > > > > > > > > so we should +=1.
> > > > > > > > > > > > >
> > > > > > > > > > > > > What the comment says is (perhaps not superclear)
> > > > > > > > > > > > > that 1 dummy clock cycle
> > > > > > > > > > > > > is modeled as one 1 byte write into the flash
> > > > > > > > > > > > > (meaining that 8 byte writes
> > > > > > > > > > > > > are needed for 1 dummy byte). Perhaps it is easier to
> > > > > > > > > > > > >
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
Hi Francisco,
On Wed, Dec 16, 2020 at 6:11 PM Bin Meng wrote:
>
> Hi Francisco,
>
> On Wed, Dec 16, 2020 at 12:40 AM Francisco Iglesias
> wrote:
> >
> > Hello Bin,
> >
> > On [2020 Dec 12] Sat 17:44:27, Bin Meng wrote:
> > > Hi Francisco,
> > >
> > > On Sat, Dec 12, 2020 at 5:24 PM Francisco Iglesias
> > > wrote:
> > > >
> > > > Hi bin,
> > > >
> > > > On [2020 Dec 12] Sat 16:16:59, Bin Meng wrote:
> > > > > Hi Francisco,
> > > > >
> > > > > On Sat, Dec 12, 2020 at 12:11 AM Francisco Iglesias
> > > > > wrote:
> > > > > >
> > > > > > Hello Bin,
> > > > > >
> > > > > > On [2020 Dec 11] Fri 23:29:16, Bin Meng wrote:
> > > > > > > Hi Francisco,
> > > > > > >
> > > > > > > On Fri, Dec 11, 2020 at 11:16 PM Francisco Iglesias
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > Hello Bin,
> > > > > > > >
> > > > > > > > On [2020 Dec 11] Fri 14:07:21, Bin Meng wrote:
> > > > > > > > > Hi Francisco,
> > > > > > > > >
> > > > > > > > > On Fri, Dec 4, 2020 at 7:28 PM Francisco Iglesias
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > Hello Bin,
> > > > > > > > > >
> > > > > > > > > > On [2020 Dec 04] Fri 18:52:50, Bin Meng wrote:
> > > > > > > > > > > Hi Francisco,
> > > > > > > > > > >
> > > > > > > > > > > On Fri, Dec 4, 2020 at 6:46 PM Francisco Iglesias
> > > > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Hello Bin,
> > > > > > > > > > > >
> > > > > > > > > > > > On [2020 Dec 04] Fri 15:52:12, Bin Meng wrote:
> > > > > > > > > > > > > Hi Francisco,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Thu, Dec 3, 2020 at 4:38 PM Francisco Iglesias
> > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Hi Bin and Alistair,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On [2020 Dec 02] Wed 11:40:11, Alistair Francis
> > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng
> > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > From: Bin Meng
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > SST flashes require a dummy byte after the
> > > > > > > > > > > > > > > > address bits.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Signed-off-by: Bin Meng
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > I couldn't find a datasheet that says this... But
> > > > > > > > > > > > > > > the actual code
> > > > > > > > > > > > > > > change looks fine, so:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Acked-by: Alistair Francis
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Alistair
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > hw/block/m25p80.c | 3 +++
> > > > > > > > > > > > > > > > 1 file changed, 3 insertions(+)
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > diff --git a/hw/block/m25p80.c
> > > > > > > > > > > > > > > > b/hw/block/m25p80.c
> > > > > > > > > > > > > > > > index 483925f..9b36762 100644
> > > > > > > > > > > > > > > > --- a/hw/block/m25p80.c
> > > > > > > > > > > > > > > > +++ b/hw/block/m25p80.c
> > > > > > > > > > > > > > > > @@ -825,6 +825,9 @@ static void
> > > > > > > > > > > > > > > > decode_fast_read_cmd(Flash *s)
> > > > > > > > > > > > > > > > s->needed_bytes = get_addr_length(s);
> > > > > > > > > > > > > > > > switch (get_man(s)) {
> > > > > > > > > > > > > > > > /* Dummy cycles - modeled with bytes
> > > > > > > > > > > > > > > > writes instead of bits */
> > > > > > > > > > > > > > > > +case MAN_SST:
> > > > > > > > > > > > > > > > +s->needed_bytes += 1;
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > 1 dummy clk cycle is modelled as 1 byte write (see
> > > > > > > > > > > > > > the comment above), so 1
> > > > > > > > > > > > > > dummy byte (8 dummy clk cycles) will need +8 above.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I think you were confused by the WINBOND codes. The
> > > > > > > > > > > > > comments are
> > > > > > > > > > > > > correct. It is modeled with bytes instead of bits, so
> > > > > > > > > > > > > we should +=1.
> > > > > > > > > > > >
> > > > > > > > > > > > What the comment says is (perhaps not superclear) that
> > > > > > > > > > > > 1 dummy clock cycle
> > > > > > > > > > > > is modeled as one 1 byte write into the flash (meaining
> > > > > > > > > > > > that 8 byte writes
> > > > > > > > > > > > are needed for 1 dummy byte). Perhaps it is easier to
> > > > > > > > > > > > understand
> > > > > > > > > > > > looking into how the controllers issue the command
> > > > > > > > > > > > towards the flash model
> > > > > > > > > > > > (for example the xilinx_spips), the start of the
> > > > > > > > > > > > FAST_READ cmd is issued
> > > > > > > > > > > > as writing the following into the flash: 1 byte (cmd),
> > > > > > > > > > > > 3
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
Hi Francisco,
On Wed, Dec 16, 2020 at 12:40 AM Francisco Iglesias
wrote:
>
> Hello Bin,
>
> On [2020 Dec 12] Sat 17:44:27, Bin Meng wrote:
> > Hi Francisco,
> >
> > On Sat, Dec 12, 2020 at 5:24 PM Francisco Iglesias
> > wrote:
> > >
> > > Hi bin,
> > >
> > > On [2020 Dec 12] Sat 16:16:59, Bin Meng wrote:
> > > > Hi Francisco,
> > > >
> > > > On Sat, Dec 12, 2020 at 12:11 AM Francisco Iglesias
> > > > wrote:
> > > > >
> > > > > Hello Bin,
> > > > >
> > > > > On [2020 Dec 11] Fri 23:29:16, Bin Meng wrote:
> > > > > > Hi Francisco,
> > > > > >
> > > > > > On Fri, Dec 11, 2020 at 11:16 PM Francisco Iglesias
> > > > > > wrote:
> > > > > > >
> > > > > > > Hello Bin,
> > > > > > >
> > > > > > > On [2020 Dec 11] Fri 14:07:21, Bin Meng wrote:
> > > > > > > > Hi Francisco,
> > > > > > > >
> > > > > > > > On Fri, Dec 4, 2020 at 7:28 PM Francisco Iglesias
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > Hello Bin,
> > > > > > > > >
> > > > > > > > > On [2020 Dec 04] Fri 18:52:50, Bin Meng wrote:
> > > > > > > > > > Hi Francisco,
> > > > > > > > > >
> > > > > > > > > > On Fri, Dec 4, 2020 at 6:46 PM Francisco Iglesias
> > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hello Bin,
> > > > > > > > > > >
> > > > > > > > > > > On [2020 Dec 04] Fri 15:52:12, Bin Meng wrote:
> > > > > > > > > > > > Hi Francisco,
> > > > > > > > > > > >
> > > > > > > > > > > > On Thu, Dec 3, 2020 at 4:38 PM Francisco Iglesias
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hi Bin and Alistair,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On [2020 Dec 02] Wed 11:40:11, Alistair Francis wrote:
> > > > > > > > > > > > > > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng
> > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > From: Bin Meng
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > SST flashes require a dummy byte after the
> > > > > > > > > > > > > > > address bits.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Signed-off-by: Bin Meng
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I couldn't find a datasheet that says this... But
> > > > > > > > > > > > > > the actual code
> > > > > > > > > > > > > > change looks fine, so:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Acked-by: Alistair Francis
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Alistair
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > hw/block/m25p80.c | 3 +++
> > > > > > > > > > > > > > > 1 file changed, 3 insertions(+)
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > > > > > > > > > > > > > > index 483925f..9b36762 100644
> > > > > > > > > > > > > > > --- a/hw/block/m25p80.c
> > > > > > > > > > > > > > > +++ b/hw/block/m25p80.c
> > > > > > > > > > > > > > > @@ -825,6 +825,9 @@ static void
> > > > > > > > > > > > > > > decode_fast_read_cmd(Flash *s)
> > > > > > > > > > > > > > > s->needed_bytes = get_addr_length(s);
> > > > > > > > > > > > > > > switch (get_man(s)) {
> > > > > > > > > > > > > > > /* Dummy cycles - modeled with bytes writes
> > > > > > > > > > > > > > > instead of bits */
> > > > > > > > > > > > > > > +case MAN_SST:
> > > > > > > > > > > > > > > +s->needed_bytes += 1;
> > > > > > > > > > > > >
> > > > > > > > > > > > > 1 dummy clk cycle is modelled as 1 byte write (see
> > > > > > > > > > > > > the comment above), so 1
> > > > > > > > > > > > > dummy byte (8 dummy clk cycles) will need +8 above.
> > > > > > > > > > > >
> > > > > > > > > > > > I think you were confused by the WINBOND codes. The
> > > > > > > > > > > > comments are
> > > > > > > > > > > > correct. It is modeled with bytes instead of bits, so
> > > > > > > > > > > > we should +=1.
> > > > > > > > > > >
> > > > > > > > > > > What the comment says is (perhaps not superclear) that 1
> > > > > > > > > > > dummy clock cycle
> > > > > > > > > > > is modeled as one 1 byte write into the flash (meaining
> > > > > > > > > > > that 8 byte writes
> > > > > > > > > > > are needed for 1 dummy byte). Perhaps it is easier to
> > > > > > > > > > > understand
> > > > > > > > > > > looking into how the controllers issue the command
> > > > > > > > > > > towards the flash model
> > > > > > > > > > > (for example the xilinx_spips), the start of the
> > > > > > > > > > > FAST_READ cmd is issued
> > > > > > > > > > > as writing the following into the flash: 1 byte (cmd), 3
> > > > > > > > > > > bytes (address),
> > > > > > > > > > > 8 bytes (8 dummy cycles -> 1 dummy byte).
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > My interpretation of the comments are opposite: one cycle
> > > > > > > > > > is a bit,
> > > > > > > > > > but we are not using bits, instead we are using bytes.
> > > > > > > > >
> > > > > > > > > Yes, the
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
Hello Bin,
On [2020 Dec 12] Sat 17:44:27, Bin Meng wrote:
> Hi Francisco,
>
> On Sat, Dec 12, 2020 at 5:24 PM Francisco Iglesias
> wrote:
> >
> > Hi bin,
> >
> > On [2020 Dec 12] Sat 16:16:59, Bin Meng wrote:
> > > Hi Francisco,
> > >
> > > On Sat, Dec 12, 2020 at 12:11 AM Francisco Iglesias
> > > wrote:
> > > >
> > > > Hello Bin,
> > > >
> > > > On [2020 Dec 11] Fri 23:29:16, Bin Meng wrote:
> > > > > Hi Francisco,
> > > > >
> > > > > On Fri, Dec 11, 2020 at 11:16 PM Francisco Iglesias
> > > > > wrote:
> > > > > >
> > > > > > Hello Bin,
> > > > > >
> > > > > > On [2020 Dec 11] Fri 14:07:21, Bin Meng wrote:
> > > > > > > Hi Francisco,
> > > > > > >
> > > > > > > On Fri, Dec 4, 2020 at 7:28 PM Francisco Iglesias
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > Hello Bin,
> > > > > > > >
> > > > > > > > On [2020 Dec 04] Fri 18:52:50, Bin Meng wrote:
> > > > > > > > > Hi Francisco,
> > > > > > > > >
> > > > > > > > > On Fri, Dec 4, 2020 at 6:46 PM Francisco Iglesias
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > Hello Bin,
> > > > > > > > > >
> > > > > > > > > > On [2020 Dec 04] Fri 15:52:12, Bin Meng wrote:
> > > > > > > > > > > Hi Francisco,
> > > > > > > > > > >
> > > > > > > > > > > On Thu, Dec 3, 2020 at 4:38 PM Francisco Iglesias
> > > > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Hi Bin and Alistair,
> > > > > > > > > > > >
> > > > > > > > > > > > On [2020 Dec 02] Wed 11:40:11, Alistair Francis wrote:
> > > > > > > > > > > > > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng
> > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > From: Bin Meng
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > SST flashes require a dummy byte after the address
> > > > > > > > > > > > > > bits.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Signed-off-by: Bin Meng
> > > > > > > > > > > > >
> > > > > > > > > > > > > I couldn't find a datasheet that says this... But the
> > > > > > > > > > > > > actual code
> > > > > > > > > > > > > change looks fine, so:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Acked-by: Alistair Francis
> > > > > > > > > > > > >
> > > > > > > > > > > > > Alistair
> > > > > > > > > > > > >
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > hw/block/m25p80.c | 3 +++
> > > > > > > > > > > > > > 1 file changed, 3 insertions(+)
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > > > > > > > > > > > > > index 483925f..9b36762 100644
> > > > > > > > > > > > > > --- a/hw/block/m25p80.c
> > > > > > > > > > > > > > +++ b/hw/block/m25p80.c
> > > > > > > > > > > > > > @@ -825,6 +825,9 @@ static void
> > > > > > > > > > > > > > decode_fast_read_cmd(Flash *s)
> > > > > > > > > > > > > > s->needed_bytes = get_addr_length(s);
> > > > > > > > > > > > > > switch (get_man(s)) {
> > > > > > > > > > > > > > /* Dummy cycles - modeled with bytes writes
> > > > > > > > > > > > > > instead of bits */
> > > > > > > > > > > > > > +case MAN_SST:
> > > > > > > > > > > > > > +s->needed_bytes += 1;
> > > > > > > > > > > >
> > > > > > > > > > > > 1 dummy clk cycle is modelled as 1 byte write (see the
> > > > > > > > > > > > comment above), so 1
> > > > > > > > > > > > dummy byte (8 dummy clk cycles) will need +8 above.
> > > > > > > > > > >
> > > > > > > > > > > I think you were confused by the WINBOND codes. The
> > > > > > > > > > > comments are
> > > > > > > > > > > correct. It is modeled with bytes instead of bits, so we
> > > > > > > > > > > should +=1.
> > > > > > > > > >
> > > > > > > > > > What the comment says is (perhaps not superclear) that 1
> > > > > > > > > > dummy clock cycle
> > > > > > > > > > is modeled as one 1 byte write into the flash (meaining
> > > > > > > > > > that 8 byte writes
> > > > > > > > > > are needed for 1 dummy byte). Perhaps it is easier to
> > > > > > > > > > understand
> > > > > > > > > > looking into how the controllers issue the command towards
> > > > > > > > > > the flash model
> > > > > > > > > > (for example the xilinx_spips), the start of the FAST_READ
> > > > > > > > > > cmd is issued
> > > > > > > > > > as writing the following into the flash: 1 byte (cmd), 3
> > > > > > > > > > bytes (address),
> > > > > > > > > > 8 bytes (8 dummy cycles -> 1 dummy byte).
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > My interpretation of the comments are opposite: one cycle is
> > > > > > > > > a bit,
> > > > > > > > > but we are not using bits, instead we are using bytes.
> > > > > > > >
> > > > > > > > Yes, the mentioning of 'bits' in the comment makes it not very
> > > > > > > > clear at first read.
> > > > > > > > Maybe just bellow would have been better:
> > > > > > > >
> > > > > > > > /* Dummy clock cycles - modeled with bytes writes */
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Testing shows that +=1 is the correct way with the
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
Hi Francisco,
On Sat, Dec 12, 2020 at 5:24 PM Francisco Iglesias
wrote:
>
> Hi bin,
>
> On [2020 Dec 12] Sat 16:16:59, Bin Meng wrote:
> > Hi Francisco,
> >
> > On Sat, Dec 12, 2020 at 12:11 AM Francisco Iglesias
> > wrote:
> > >
> > > Hello Bin,
> > >
> > > On [2020 Dec 11] Fri 23:29:16, Bin Meng wrote:
> > > > Hi Francisco,
> > > >
> > > > On Fri, Dec 11, 2020 at 11:16 PM Francisco Iglesias
> > > > wrote:
> > > > >
> > > > > Hello Bin,
> > > > >
> > > > > On [2020 Dec 11] Fri 14:07:21, Bin Meng wrote:
> > > > > > Hi Francisco,
> > > > > >
> > > > > > On Fri, Dec 4, 2020 at 7:28 PM Francisco Iglesias
> > > > > > wrote:
> > > > > > >
> > > > > > > Hello Bin,
> > > > > > >
> > > > > > > On [2020 Dec 04] Fri 18:52:50, Bin Meng wrote:
> > > > > > > > Hi Francisco,
> > > > > > > >
> > > > > > > > On Fri, Dec 4, 2020 at 6:46 PM Francisco Iglesias
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > Hello Bin,
> > > > > > > > >
> > > > > > > > > On [2020 Dec 04] Fri 15:52:12, Bin Meng wrote:
> > > > > > > > > > Hi Francisco,
> > > > > > > > > >
> > > > > > > > > > On Thu, Dec 3, 2020 at 4:38 PM Francisco Iglesias
> > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Bin and Alistair,
> > > > > > > > > > >
> > > > > > > > > > > On [2020 Dec 02] Wed 11:40:11, Alistair Francis wrote:
> > > > > > > > > > > > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > From: Bin Meng
> > > > > > > > > > > > >
> > > > > > > > > > > > > SST flashes require a dummy byte after the address
> > > > > > > > > > > > > bits.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Bin Meng
> > > > > > > > > > > >
> > > > > > > > > > > > I couldn't find a datasheet that says this... But the
> > > > > > > > > > > > actual code
> > > > > > > > > > > > change looks fine, so:
> > > > > > > > > > > >
> > > > > > > > > > > > Acked-by: Alistair Francis
> > > > > > > > > > > >
> > > > > > > > > > > > Alistair
> > > > > > > > > > > >
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >
> > > > > > > > > > > > > hw/block/m25p80.c | 3 +++
> > > > > > > > > > > > > 1 file changed, 3 insertions(+)
> > > > > > > > > > > > >
> > > > > > > > > > > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > > > > > > > > > > > > index 483925f..9b36762 100644
> > > > > > > > > > > > > --- a/hw/block/m25p80.c
> > > > > > > > > > > > > +++ b/hw/block/m25p80.c
> > > > > > > > > > > > > @@ -825,6 +825,9 @@ static void
> > > > > > > > > > > > > decode_fast_read_cmd(Flash *s)
> > > > > > > > > > > > > s->needed_bytes = get_addr_length(s);
> > > > > > > > > > > > > switch (get_man(s)) {
> > > > > > > > > > > > > /* Dummy cycles - modeled with bytes writes
> > > > > > > > > > > > > instead of bits */
> > > > > > > > > > > > > +case MAN_SST:
> > > > > > > > > > > > > +s->needed_bytes += 1;
> > > > > > > > > > >
> > > > > > > > > > > 1 dummy clk cycle is modelled as 1 byte write (see the
> > > > > > > > > > > comment above), so 1
> > > > > > > > > > > dummy byte (8 dummy clk cycles) will need +8 above.
> > > > > > > > > >
> > > > > > > > > > I think you were confused by the WINBOND codes. The
> > > > > > > > > > comments are
> > > > > > > > > > correct. It is modeled with bytes instead of bits, so we
> > > > > > > > > > should +=1.
> > > > > > > > >
> > > > > > > > > What the comment says is (perhaps not superclear) that 1
> > > > > > > > > dummy clock cycle
> > > > > > > > > is modeled as one 1 byte write into the flash (meaining that
> > > > > > > > > 8 byte writes
> > > > > > > > > are needed for 1 dummy byte). Perhaps it is easier to
> > > > > > > > > understand
> > > > > > > > > looking into how the controllers issue the command towards
> > > > > > > > > the flash model
> > > > > > > > > (for example the xilinx_spips), the start of the FAST_READ
> > > > > > > > > cmd is issued
> > > > > > > > > as writing the following into the flash: 1 byte (cmd), 3
> > > > > > > > > bytes (address),
> > > > > > > > > 8 bytes (8 dummy cycles -> 1 dummy byte).
> > > > > > > > >
> > > > > > > >
> > > > > > > > My interpretation of the comments are opposite: one cycle is a
> > > > > > > > bit,
> > > > > > > > but we are not using bits, instead we are using bytes.
> > > > > > >
> > > > > > > Yes, the mentioning of 'bits' in the comment makes it not very
> > > > > > > clear at first read.
> > > > > > > Maybe just bellow would have been better:
> > > > > > >
> > > > > > > /* Dummy clock cycles - modeled with bytes writes */
> > > > > > >
> > > > > > > >
> > > > > > > > Testing shows that +=1 is the correct way with the imx_spi
> > > > > > > > controller,
> > > > > > > > and with my SiFive SPI model in my local tree (not upstreamed
> > > > > > > > yet)
> > > > > > >
> > > > > > > Perhaps an option could be to look into how the aspeed_smc,
> > > > > > > xilinx_spips or the
> > > > > > > npcm7xx_fiu generate dummy
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
Hi bin,
On [2020 Dec 12] Sat 16:16:59, Bin Meng wrote:
> Hi Francisco,
>
> On Sat, Dec 12, 2020 at 12:11 AM Francisco Iglesias
> wrote:
> >
> > Hello Bin,
> >
> > On [2020 Dec 11] Fri 23:29:16, Bin Meng wrote:
> > > Hi Francisco,
> > >
> > > On Fri, Dec 11, 2020 at 11:16 PM Francisco Iglesias
> > > wrote:
> > > >
> > > > Hello Bin,
> > > >
> > > > On [2020 Dec 11] Fri 14:07:21, Bin Meng wrote:
> > > > > Hi Francisco,
> > > > >
> > > > > On Fri, Dec 4, 2020 at 7:28 PM Francisco Iglesias
> > > > > wrote:
> > > > > >
> > > > > > Hello Bin,
> > > > > >
> > > > > > On [2020 Dec 04] Fri 18:52:50, Bin Meng wrote:
> > > > > > > Hi Francisco,
> > > > > > >
> > > > > > > On Fri, Dec 4, 2020 at 6:46 PM Francisco Iglesias
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > Hello Bin,
> > > > > > > >
> > > > > > > > On [2020 Dec 04] Fri 15:52:12, Bin Meng wrote:
> > > > > > > > > Hi Francisco,
> > > > > > > > >
> > > > > > > > > On Thu, Dec 3, 2020 at 4:38 PM Francisco Iglesias
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Bin and Alistair,
> > > > > > > > > >
> > > > > > > > > > On [2020 Dec 02] Wed 11:40:11, Alistair Francis wrote:
> > > > > > > > > > > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng
> > > > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > From: Bin Meng
> > > > > > > > > > > >
> > > > > > > > > > > > SST flashes require a dummy byte after the address bits.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Bin Meng
> > > > > > > > > > >
> > > > > > > > > > > I couldn't find a datasheet that says this... But the
> > > > > > > > > > > actual code
> > > > > > > > > > > change looks fine, so:
> > > > > > > > > > >
> > > > > > > > > > > Acked-by: Alistair Francis
> > > > > > > > > > >
> > > > > > > > > > > Alistair
> > > > > > > > > > >
> > > > > > > > > > > > ---
> > > > > > > > > > > >
> > > > > > > > > > > > hw/block/m25p80.c | 3 +++
> > > > > > > > > > > > 1 file changed, 3 insertions(+)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > > > > > > > > > > > index 483925f..9b36762 100644
> > > > > > > > > > > > --- a/hw/block/m25p80.c
> > > > > > > > > > > > +++ b/hw/block/m25p80.c
> > > > > > > > > > > > @@ -825,6 +825,9 @@ static void
> > > > > > > > > > > > decode_fast_read_cmd(Flash *s)
> > > > > > > > > > > > s->needed_bytes = get_addr_length(s);
> > > > > > > > > > > > switch (get_man(s)) {
> > > > > > > > > > > > /* Dummy cycles - modeled with bytes writes
> > > > > > > > > > > > instead of bits */
> > > > > > > > > > > > +case MAN_SST:
> > > > > > > > > > > > +s->needed_bytes += 1;
> > > > > > > > > >
> > > > > > > > > > 1 dummy clk cycle is modelled as 1 byte write (see the
> > > > > > > > > > comment above), so 1
> > > > > > > > > > dummy byte (8 dummy clk cycles) will need +8 above.
> > > > > > > > >
> > > > > > > > > I think you were confused by the WINBOND codes. The comments
> > > > > > > > > are
> > > > > > > > > correct. It is modeled with bytes instead of bits, so we
> > > > > > > > > should +=1.
> > > > > > > >
> > > > > > > > What the comment says is (perhaps not superclear) that 1 dummy
> > > > > > > > clock cycle
> > > > > > > > is modeled as one 1 byte write into the flash (meaining that 8
> > > > > > > > byte writes
> > > > > > > > are needed for 1 dummy byte). Perhaps it is easier to understand
> > > > > > > > looking into how the controllers issue the command towards the
> > > > > > > > flash model
> > > > > > > > (for example the xilinx_spips), the start of the FAST_READ cmd
> > > > > > > > is issued
> > > > > > > > as writing the following into the flash: 1 byte (cmd), 3 bytes
> > > > > > > > (address),
> > > > > > > > 8 bytes (8 dummy cycles -> 1 dummy byte).
> > > > > > > >
> > > > > > >
> > > > > > > My interpretation of the comments are opposite: one cycle is a
> > > > > > > bit,
> > > > > > > but we are not using bits, instead we are using bytes.
> > > > > >
> > > > > > Yes, the mentioning of 'bits' in the comment makes it not very
> > > > > > clear at first read.
> > > > > > Maybe just bellow would have been better:
> > > > > >
> > > > > > /* Dummy clock cycles - modeled with bytes writes */
> > > > > >
> > > > > > >
> > > > > > > Testing shows that +=1 is the correct way with the imx_spi
> > > > > > > controller,
> > > > > > > and with my SiFive SPI model in my local tree (not upstreamed yet)
> > > > > >
> > > > > > Perhaps an option could be to look into how the aspeed_smc,
> > > > > > xilinx_spips or the
> > > > > > npcm7xx_fiu generate dummy clock cycles and see if a similar
> > > > > > solution to one of
> > > > > > those could work aswell for the imx_spi?
> > > > > >
> > > > >
> > > > > Thanks for pointing this out. So there is some inconsistency among
> > > > > different SPI controller modeling.
> > > >
> > > > I'm not sure I understand you correctly but the controllers supporting
> > > > commands with
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
Hi Francisco,
On Sat, Dec 12, 2020 at 12:11 AM Francisco Iglesias
wrote:
>
> Hello Bin,
>
> On [2020 Dec 11] Fri 23:29:16, Bin Meng wrote:
> > Hi Francisco,
> >
> > On Fri, Dec 11, 2020 at 11:16 PM Francisco Iglesias
> > wrote:
> > >
> > > Hello Bin,
> > >
> > > On [2020 Dec 11] Fri 14:07:21, Bin Meng wrote:
> > > > Hi Francisco,
> > > >
> > > > On Fri, Dec 4, 2020 at 7:28 PM Francisco Iglesias
> > > > wrote:
> > > > >
> > > > > Hello Bin,
> > > > >
> > > > > On [2020 Dec 04] Fri 18:52:50, Bin Meng wrote:
> > > > > > Hi Francisco,
> > > > > >
> > > > > > On Fri, Dec 4, 2020 at 6:46 PM Francisco Iglesias
> > > > > > wrote:
> > > > > > >
> > > > > > > Hello Bin,
> > > > > > >
> > > > > > > On [2020 Dec 04] Fri 15:52:12, Bin Meng wrote:
> > > > > > > > Hi Francisco,
> > > > > > > >
> > > > > > > > On Thu, Dec 3, 2020 at 4:38 PM Francisco Iglesias
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > Hi Bin and Alistair,
> > > > > > > > >
> > > > > > > > > On [2020 Dec 02] Wed 11:40:11, Alistair Francis wrote:
> > > > > > > > > > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng
> > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > From: Bin Meng
> > > > > > > > > > >
> > > > > > > > > > > SST flashes require a dummy byte after the address bits.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Bin Meng
> > > > > > > > > >
> > > > > > > > > > I couldn't find a datasheet that says this... But the
> > > > > > > > > > actual code
> > > > > > > > > > change looks fine, so:
> > > > > > > > > >
> > > > > > > > > > Acked-by: Alistair Francis
> > > > > > > > > >
> > > > > > > > > > Alistair
> > > > > > > > > >
> > > > > > > > > > > ---
> > > > > > > > > > >
> > > > > > > > > > > hw/block/m25p80.c | 3 +++
> > > > > > > > > > > 1 file changed, 3 insertions(+)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > > > > > > > > > > index 483925f..9b36762 100644
> > > > > > > > > > > --- a/hw/block/m25p80.c
> > > > > > > > > > > +++ b/hw/block/m25p80.c
> > > > > > > > > > > @@ -825,6 +825,9 @@ static void
> > > > > > > > > > > decode_fast_read_cmd(Flash *s)
> > > > > > > > > > > s->needed_bytes = get_addr_length(s);
> > > > > > > > > > > switch (get_man(s)) {
> > > > > > > > > > > /* Dummy cycles - modeled with bytes writes instead
> > > > > > > > > > > of bits */
> > > > > > > > > > > +case MAN_SST:
> > > > > > > > > > > +s->needed_bytes += 1;
> > > > > > > > >
> > > > > > > > > 1 dummy clk cycle is modelled as 1 byte write (see the
> > > > > > > > > comment above), so 1
> > > > > > > > > dummy byte (8 dummy clk cycles) will need +8 above.
> > > > > > > >
> > > > > > > > I think you were confused by the WINBOND codes. The comments are
> > > > > > > > correct. It is modeled with bytes instead of bits, so we should
> > > > > > > > +=1.
> > > > > > >
> > > > > > > What the comment says is (perhaps not superclear) that 1 dummy
> > > > > > > clock cycle
> > > > > > > is modeled as one 1 byte write into the flash (meaining that 8
> > > > > > > byte writes
> > > > > > > are needed for 1 dummy byte). Perhaps it is easier to understand
> > > > > > > looking into how the controllers issue the command towards the
> > > > > > > flash model
> > > > > > > (for example the xilinx_spips), the start of the FAST_READ cmd is
> > > > > > > issued
> > > > > > > as writing the following into the flash: 1 byte (cmd), 3 bytes
> > > > > > > (address),
> > > > > > > 8 bytes (8 dummy cycles -> 1 dummy byte).
> > > > > > >
> > > > > >
> > > > > > My interpretation of the comments are opposite: one cycle is a bit,
> > > > > > but we are not using bits, instead we are using bytes.
> > > > >
> > > > > Yes, the mentioning of 'bits' in the comment makes it not very clear
> > > > > at first read.
> > > > > Maybe just bellow would have been better:
> > > > >
> > > > > /* Dummy clock cycles - modeled with bytes writes */
> > > > >
> > > > > >
> > > > > > Testing shows that +=1 is the correct way with the imx_spi
> > > > > > controller,
> > > > > > and with my SiFive SPI model in my local tree (not upstreamed yet)
> > > > >
> > > > > Perhaps an option could be to look into how the aspeed_smc,
> > > > > xilinx_spips or the
> > > > > npcm7xx_fiu generate dummy clock cycles and see if a similar solution
> > > > > to one of
> > > > > those could work aswell for the imx_spi?
> > > > >
> > > >
> > > > Thanks for pointing this out. So there is some inconsistency among
> > > > different SPI controller modeling.
> > >
> > > I'm not sure I understand you correctly but the controllers supporting
> > > commands with dummy clock cycles can only do it following the modeled
> > > approach, so I would rather say it is pretty consistent across the
> > > controllers (not all controllers support these commands though).
> >
> > I mean there are 2 approaches to emulate the dummy cycles for
>
> There is currently only 1 way of modeling dummy clock cycles. All
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
Hello Bin,
On [2020 Dec 11] Fri 23:29:16, Bin Meng wrote:
> Hi Francisco,
>
> On Fri, Dec 11, 2020 at 11:16 PM Francisco Iglesias
> wrote:
> >
> > Hello Bin,
> >
> > On [2020 Dec 11] Fri 14:07:21, Bin Meng wrote:
> > > Hi Francisco,
> > >
> > > On Fri, Dec 4, 2020 at 7:28 PM Francisco Iglesias
> > > wrote:
> > > >
> > > > Hello Bin,
> > > >
> > > > On [2020 Dec 04] Fri 18:52:50, Bin Meng wrote:
> > > > > Hi Francisco,
> > > > >
> > > > > On Fri, Dec 4, 2020 at 6:46 PM Francisco Iglesias
> > > > > wrote:
> > > > > >
> > > > > > Hello Bin,
> > > > > >
> > > > > > On [2020 Dec 04] Fri 15:52:12, Bin Meng wrote:
> > > > > > > Hi Francisco,
> > > > > > >
> > > > > > > On Thu, Dec 3, 2020 at 4:38 PM Francisco Iglesias
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > Hi Bin and Alistair,
> > > > > > > >
> > > > > > > > On [2020 Dec 02] Wed 11:40:11, Alistair Francis wrote:
> > > > > > > > > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > From: Bin Meng
> > > > > > > > > >
> > > > > > > > > > SST flashes require a dummy byte after the address bits.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Bin Meng
> > > > > > > > >
> > > > > > > > > I couldn't find a datasheet that says this... But the actual
> > > > > > > > > code
> > > > > > > > > change looks fine, so:
> > > > > > > > >
> > > > > > > > > Acked-by: Alistair Francis
> > > > > > > > >
> > > > > > > > > Alistair
> > > > > > > > >
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > hw/block/m25p80.c | 3 +++
> > > > > > > > > > 1 file changed, 3 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > > > > > > > > > index 483925f..9b36762 100644
> > > > > > > > > > --- a/hw/block/m25p80.c
> > > > > > > > > > +++ b/hw/block/m25p80.c
> > > > > > > > > > @@ -825,6 +825,9 @@ static void decode_fast_read_cmd(Flash
> > > > > > > > > > *s)
> > > > > > > > > > s->needed_bytes = get_addr_length(s);
> > > > > > > > > > switch (get_man(s)) {
> > > > > > > > > > /* Dummy cycles - modeled with bytes writes instead of
> > > > > > > > > > bits */
> > > > > > > > > > +case MAN_SST:
> > > > > > > > > > +s->needed_bytes += 1;
> > > > > > > >
> > > > > > > > 1 dummy clk cycle is modelled as 1 byte write (see the comment
> > > > > > > > above), so 1
> > > > > > > > dummy byte (8 dummy clk cycles) will need +8 above.
> > > > > > >
> > > > > > > I think you were confused by the WINBOND codes. The comments are
> > > > > > > correct. It is modeled with bytes instead of bits, so we should
> > > > > > > +=1.
> > > > > >
> > > > > > What the comment says is (perhaps not superclear) that 1 dummy
> > > > > > clock cycle
> > > > > > is modeled as one 1 byte write into the flash (meaining that 8 byte
> > > > > > writes
> > > > > > are needed for 1 dummy byte). Perhaps it is easier to understand
> > > > > > looking into how the controllers issue the command towards the
> > > > > > flash model
> > > > > > (for example the xilinx_spips), the start of the FAST_READ cmd is
> > > > > > issued
> > > > > > as writing the following into the flash: 1 byte (cmd), 3 bytes
> > > > > > (address),
> > > > > > 8 bytes (8 dummy cycles -> 1 dummy byte).
> > > > > >
> > > > >
> > > > > My interpretation of the comments are opposite: one cycle is a bit,
> > > > > but we are not using bits, instead we are using bytes.
> > > >
> > > > Yes, the mentioning of 'bits' in the comment makes it not very clear at
> > > > first read.
> > > > Maybe just bellow would have been better:
> > > >
> > > > /* Dummy clock cycles - modeled with bytes writes */
> > > >
> > > > >
> > > > > Testing shows that +=1 is the correct way with the imx_spi controller,
> > > > > and with my SiFive SPI model in my local tree (not upstreamed yet)
> > > >
> > > > Perhaps an option could be to look into how the aspeed_smc,
> > > > xilinx_spips or the
> > > > npcm7xx_fiu generate dummy clock cycles and see if a similar solution
> > > > to one of
> > > > those could work aswell for the imx_spi?
> > > >
> > >
> > > Thanks for pointing this out. So there is some inconsistency among
> > > different SPI controller modeling.
> >
> > I'm not sure I understand you correctly but the controllers supporting
> > commands with dummy clock cycles can only do it following the modeled
> > approach, so I would rather say it is pretty consistent across the
> > controllers (not all controllers support these commands though).
>
> I mean there are 2 approaches to emulate the dummy cycles for
There is currently only 1 way of modeling dummy clock cycles. All commands that
require / support them in m25p80 goes with that approach. An the controllers
that support dummy clock cycles uses that approach.
> different SPI controller models, yet we only have one m25p80 flash
> model to work with both of them. Some controllers may choose 1 byte to
> emulate 1 dummy clock cycle, but some
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
Hi Francisco,
On Fri, Dec 11, 2020 at 11:16 PM Francisco Iglesias
wrote:
>
> Hello Bin,
>
> On [2020 Dec 11] Fri 14:07:21, Bin Meng wrote:
> > Hi Francisco,
> >
> > On Fri, Dec 4, 2020 at 7:28 PM Francisco Iglesias
> > wrote:
> > >
> > > Hello Bin,
> > >
> > > On [2020 Dec 04] Fri 18:52:50, Bin Meng wrote:
> > > > Hi Francisco,
> > > >
> > > > On Fri, Dec 4, 2020 at 6:46 PM Francisco Iglesias
> > > > wrote:
> > > > >
> > > > > Hello Bin,
> > > > >
> > > > > On [2020 Dec 04] Fri 15:52:12, Bin Meng wrote:
> > > > > > Hi Francisco,
> > > > > >
> > > > > > On Thu, Dec 3, 2020 at 4:38 PM Francisco Iglesias
> > > > > > wrote:
> > > > > > >
> > > > > > > Hi Bin and Alistair,
> > > > > > >
> > > > > > > On [2020 Dec 02] Wed 11:40:11, Alistair Francis wrote:
> > > > > > > > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > From: Bin Meng
> > > > > > > > >
> > > > > > > > > SST flashes require a dummy byte after the address bits.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Bin Meng
> > > > > > > >
> > > > > > > > I couldn't find a datasheet that says this... But the actual
> > > > > > > > code
> > > > > > > > change looks fine, so:
> > > > > > > >
> > > > > > > > Acked-by: Alistair Francis
> > > > > > > >
> > > > > > > > Alistair
> > > > > > > >
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > hw/block/m25p80.c | 3 +++
> > > > > > > > > 1 file changed, 3 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > > > > > > > > index 483925f..9b36762 100644
> > > > > > > > > --- a/hw/block/m25p80.c
> > > > > > > > > +++ b/hw/block/m25p80.c
> > > > > > > > > @@ -825,6 +825,9 @@ static void decode_fast_read_cmd(Flash *s)
> > > > > > > > > s->needed_bytes = get_addr_length(s);
> > > > > > > > > switch (get_man(s)) {
> > > > > > > > > /* Dummy cycles - modeled with bytes writes instead of
> > > > > > > > > bits */
> > > > > > > > > +case MAN_SST:
> > > > > > > > > +s->needed_bytes += 1;
> > > > > > >
> > > > > > > 1 dummy clk cycle is modelled as 1 byte write (see the comment
> > > > > > > above), so 1
> > > > > > > dummy byte (8 dummy clk cycles) will need +8 above.
> > > > > >
> > > > > > I think you were confused by the WINBOND codes. The comments are
> > > > > > correct. It is modeled with bytes instead of bits, so we should +=1.
> > > > >
> > > > > What the comment says is (perhaps not superclear) that 1 dummy clock
> > > > > cycle
> > > > > is modeled as one 1 byte write into the flash (meaining that 8 byte
> > > > > writes
> > > > > are needed for 1 dummy byte). Perhaps it is easier to understand
> > > > > looking into how the controllers issue the command towards the flash
> > > > > model
> > > > > (for example the xilinx_spips), the start of the FAST_READ cmd is
> > > > > issued
> > > > > as writing the following into the flash: 1 byte (cmd), 3 bytes
> > > > > (address),
> > > > > 8 bytes (8 dummy cycles -> 1 dummy byte).
> > > > >
> > > >
> > > > My interpretation of the comments are opposite: one cycle is a bit,
> > > > but we are not using bits, instead we are using bytes.
> > >
> > > Yes, the mentioning of 'bits' in the comment makes it not very clear at
> > > first read.
> > > Maybe just bellow would have been better:
> > >
> > > /* Dummy clock cycles - modeled with bytes writes */
> > >
> > > >
> > > > Testing shows that +=1 is the correct way with the imx_spi controller,
> > > > and with my SiFive SPI model in my local tree (not upstreamed yet)
> > >
> > > Perhaps an option could be to look into how the aspeed_smc, xilinx_spips
> > > or the
> > > npcm7xx_fiu generate dummy clock cycles and see if a similar solution to
> > > one of
> > > those could work aswell for the imx_spi?
> > >
> >
> > Thanks for pointing this out. So there is some inconsistency among
> > different SPI controller modeling.
>
> I'm not sure I understand you correctly but the controllers supporting
> commands with dummy clock cycles can only do it following the modeled
> approach, so I would rather say it is pretty consistent across the
> controllers (not all controllers support these commands though).
I mean there are 2 approaches to emulate the dummy cycles for
different SPI controller models, yet we only have one m25p80 flash
model to work with both of them. Some controllers may choose 1 byte to
emulate 1 dummy clock cycle, but some others choose 1 bit to emulate 1
dummy cycle. This is inconsistent.
>
> >
> > Or maybe fixing aspeed_smc, xilinx_spips and npcm7xx_fiu to work like
> > imx_spi?
>
> For me I would say no to above (it makes more sense to let new controllers
> implement the currently modeled approach).
Yes, we can certainly make them consistent. But the question is which
one is the correct one? I tried to search in the doc but in vain.
>
> > Which one is the expected behavior for dummy cycles?
>
> Dummy clock cycles are modeled as 1 byte written to the flash
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
Hello Bin,
On [2020 Dec 11] Fri 14:07:21, Bin Meng wrote:
> Hi Francisco,
>
> On Fri, Dec 4, 2020 at 7:28 PM Francisco Iglesias
> wrote:
> >
> > Hello Bin,
> >
> > On [2020 Dec 04] Fri 18:52:50, Bin Meng wrote:
> > > Hi Francisco,
> > >
> > > On Fri, Dec 4, 2020 at 6:46 PM Francisco Iglesias
> > > wrote:
> > > >
> > > > Hello Bin,
> > > >
> > > > On [2020 Dec 04] Fri 15:52:12, Bin Meng wrote:
> > > > > Hi Francisco,
> > > > >
> > > > > On Thu, Dec 3, 2020 at 4:38 PM Francisco Iglesias
> > > > > wrote:
> > > > > >
> > > > > > Hi Bin and Alistair,
> > > > > >
> > > > > > On [2020 Dec 02] Wed 11:40:11, Alistair Francis wrote:
> > > > > > > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > From: Bin Meng
> > > > > > > >
> > > > > > > > SST flashes require a dummy byte after the address bits.
> > > > > > > >
> > > > > > > > Signed-off-by: Bin Meng
> > > > > > >
> > > > > > > I couldn't find a datasheet that says this... But the actual code
> > > > > > > change looks fine, so:
> > > > > > >
> > > > > > > Acked-by: Alistair Francis
> > > > > > >
> > > > > > > Alistair
> > > > > > >
> > > > > > > > ---
> > > > > > > >
> > > > > > > > hw/block/m25p80.c | 3 +++
> > > > > > > > 1 file changed, 3 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > > > > > > > index 483925f..9b36762 100644
> > > > > > > > --- a/hw/block/m25p80.c
> > > > > > > > +++ b/hw/block/m25p80.c
> > > > > > > > @@ -825,6 +825,9 @@ static void decode_fast_read_cmd(Flash *s)
> > > > > > > > s->needed_bytes = get_addr_length(s);
> > > > > > > > switch (get_man(s)) {
> > > > > > > > /* Dummy cycles - modeled with bytes writes instead of
> > > > > > > > bits */
> > > > > > > > +case MAN_SST:
> > > > > > > > +s->needed_bytes += 1;
> > > > > >
> > > > > > 1 dummy clk cycle is modelled as 1 byte write (see the comment
> > > > > > above), so 1
> > > > > > dummy byte (8 dummy clk cycles) will need +8 above.
> > > > >
> > > > > I think you were confused by the WINBOND codes. The comments are
> > > > > correct. It is modeled with bytes instead of bits, so we should +=1.
> > > >
> > > > What the comment says is (perhaps not superclear) that 1 dummy clock
> > > > cycle
> > > > is modeled as one 1 byte write into the flash (meaining that 8 byte
> > > > writes
> > > > are needed for 1 dummy byte). Perhaps it is easier to understand
> > > > looking into how the controllers issue the command towards the flash
> > > > model
> > > > (for example the xilinx_spips), the start of the FAST_READ cmd is issued
> > > > as writing the following into the flash: 1 byte (cmd), 3 bytes
> > > > (address),
> > > > 8 bytes (8 dummy cycles -> 1 dummy byte).
> > > >
> > >
> > > My interpretation of the comments are opposite: one cycle is a bit,
> > > but we are not using bits, instead we are using bytes.
> >
> > Yes, the mentioning of 'bits' in the comment makes it not very clear at
> > first read.
> > Maybe just bellow would have been better:
> >
> > /* Dummy clock cycles - modeled with bytes writes */
> >
> > >
> > > Testing shows that +=1 is the correct way with the imx_spi controller,
> > > and with my SiFive SPI model in my local tree (not upstreamed yet)
> >
> > Perhaps an option could be to look into how the aspeed_smc, xilinx_spips or
> > the
> > npcm7xx_fiu generate dummy clock cycles and see if a similar solution to
> > one of
> > those could work aswell for the imx_spi?
> >
>
> Thanks for pointing this out. So there is some inconsistency among
> different SPI controller modeling.
I'm not sure I understand you correctly but the controllers supporting
commands with dummy clock cycles can only do it following the modeled
approach, so I would rather say it is pretty consistent across the
controllers (not all controllers support these commands though).
>
> Or maybe fixing aspeed_smc, xilinx_spips and npcm7xx_fiu to work like
> imx_spi?
For me I would say no to above (it makes more sense to let new controllers
implement the currently modeled approach).
> Which one is the expected behavior for dummy cycles?
Dummy clock cycles are modeled as 1 byte written to the flash per dummy clock
cycle (expected behavior).
Best regards,
Francisco Iglesias
>
> > Regarding this patch, with += 8 it looks correct to me (and will work with
> > above controllers as far as I can see).
> >
>
> Regards,
> Bin
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
Hi Francisco,
On Fri, Dec 4, 2020 at 7:28 PM Francisco Iglesias
wrote:
>
> Hello Bin,
>
> On [2020 Dec 04] Fri 18:52:50, Bin Meng wrote:
> > Hi Francisco,
> >
> > On Fri, Dec 4, 2020 at 6:46 PM Francisco Iglesias
> > wrote:
> > >
> > > Hello Bin,
> > >
> > > On [2020 Dec 04] Fri 15:52:12, Bin Meng wrote:
> > > > Hi Francisco,
> > > >
> > > > On Thu, Dec 3, 2020 at 4:38 PM Francisco Iglesias
> > > > wrote:
> > > > >
> > > > > Hi Bin and Alistair,
> > > > >
> > > > > On [2020 Dec 02] Wed 11:40:11, Alistair Francis wrote:
> > > > > > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng wrote:
> > > > > > >
> > > > > > > From: Bin Meng
> > > > > > >
> > > > > > > SST flashes require a dummy byte after the address bits.
> > > > > > >
> > > > > > > Signed-off-by: Bin Meng
> > > > > >
> > > > > > I couldn't find a datasheet that says this... But the actual code
> > > > > > change looks fine, so:
> > > > > >
> > > > > > Acked-by: Alistair Francis
> > > > > >
> > > > > > Alistair
> > > > > >
> > > > > > > ---
> > > > > > >
> > > > > > > hw/block/m25p80.c | 3 +++
> > > > > > > 1 file changed, 3 insertions(+)
> > > > > > >
> > > > > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > > > > > > index 483925f..9b36762 100644
> > > > > > > --- a/hw/block/m25p80.c
> > > > > > > +++ b/hw/block/m25p80.c
> > > > > > > @@ -825,6 +825,9 @@ static void decode_fast_read_cmd(Flash *s)
> > > > > > > s->needed_bytes = get_addr_length(s);
> > > > > > > switch (get_man(s)) {
> > > > > > > /* Dummy cycles - modeled with bytes writes instead of bits
> > > > > > > */
> > > > > > > +case MAN_SST:
> > > > > > > +s->needed_bytes += 1;
> > > > >
> > > > > 1 dummy clk cycle is modelled as 1 byte write (see the comment
> > > > > above), so 1
> > > > > dummy byte (8 dummy clk cycles) will need +8 above.
> > > >
> > > > I think you were confused by the WINBOND codes. The comments are
> > > > correct. It is modeled with bytes instead of bits, so we should +=1.
> > >
> > > What the comment says is (perhaps not superclear) that 1 dummy clock cycle
> > > is modeled as one 1 byte write into the flash (meaining that 8 byte writes
> > > are needed for 1 dummy byte). Perhaps it is easier to understand
> > > looking into how the controllers issue the command towards the flash model
> > > (for example the xilinx_spips), the start of the FAST_READ cmd is issued
> > > as writing the following into the flash: 1 byte (cmd), 3 bytes (address),
> > > 8 bytes (8 dummy cycles -> 1 dummy byte).
> > >
> >
> > My interpretation of the comments are opposite: one cycle is a bit,
> > but we are not using bits, instead we are using bytes.
>
> Yes, the mentioning of 'bits' in the comment makes it not very clear at first
> read.
> Maybe just bellow would have been better:
>
> /* Dummy clock cycles - modeled with bytes writes */
>
> >
> > Testing shows that +=1 is the correct way with the imx_spi controller,
> > and with my SiFive SPI model in my local tree (not upstreamed yet)
>
> Perhaps an option could be to look into how the aspeed_smc, xilinx_spips or
> the
> npcm7xx_fiu generate dummy clock cycles and see if a similar solution to one
> of
> those could work aswell for the imx_spi?
>
Thanks for pointing this out. So there is some inconsistency among
different SPI controller modeling.
Or maybe fixing aspeed_smc, xilinx_spips and npcm7xx_fiu to work like
imx_spi? Which one is the expected behavior for dummy cycles?
> Regarding this patch, with += 8 it looks correct to me (and will work with
> above controllers as far as I can see).
>
Regards,
Bin
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
Hello Bin,
On [2020 Dec 04] Fri 18:52:50, Bin Meng wrote:
> Hi Francisco,
>
> On Fri, Dec 4, 2020 at 6:46 PM Francisco Iglesias
> wrote:
> >
> > Hello Bin,
> >
> > On [2020 Dec 04] Fri 15:52:12, Bin Meng wrote:
> > > Hi Francisco,
> > >
> > > On Thu, Dec 3, 2020 at 4:38 PM Francisco Iglesias
> > > wrote:
> > > >
> > > > Hi Bin and Alistair,
> > > >
> > > > On [2020 Dec 02] Wed 11:40:11, Alistair Francis wrote:
> > > > > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng wrote:
> > > > > >
> > > > > > From: Bin Meng
> > > > > >
> > > > > > SST flashes require a dummy byte after the address bits.
> > > > > >
> > > > > > Signed-off-by: Bin Meng
> > > > >
> > > > > I couldn't find a datasheet that says this... But the actual code
> > > > > change looks fine, so:
> > > > >
> > > > > Acked-by: Alistair Francis
> > > > >
> > > > > Alistair
> > > > >
> > > > > > ---
> > > > > >
> > > > > > hw/block/m25p80.c | 3 +++
> > > > > > 1 file changed, 3 insertions(+)
> > > > > >
> > > > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > > > > > index 483925f..9b36762 100644
> > > > > > --- a/hw/block/m25p80.c
> > > > > > +++ b/hw/block/m25p80.c
> > > > > > @@ -825,6 +825,9 @@ static void decode_fast_read_cmd(Flash *s)
> > > > > > s->needed_bytes = get_addr_length(s);
> > > > > > switch (get_man(s)) {
> > > > > > /* Dummy cycles - modeled with bytes writes instead of bits */
> > > > > > +case MAN_SST:
> > > > > > +s->needed_bytes += 1;
> > > >
> > > > 1 dummy clk cycle is modelled as 1 byte write (see the comment above),
> > > > so 1
> > > > dummy byte (8 dummy clk cycles) will need +8 above.
> > >
> > > I think you were confused by the WINBOND codes. The comments are
> > > correct. It is modeled with bytes instead of bits, so we should +=1.
> >
> > What the comment says is (perhaps not superclear) that 1 dummy clock cycle
> > is modeled as one 1 byte write into the flash (meaining that 8 byte writes
> > are needed for 1 dummy byte). Perhaps it is easier to understand
> > looking into how the controllers issue the command towards the flash model
> > (for example the xilinx_spips), the start of the FAST_READ cmd is issued
> > as writing the following into the flash: 1 byte (cmd), 3 bytes (address),
> > 8 bytes (8 dummy cycles -> 1 dummy byte).
> >
>
> My interpretation of the comments are opposite: one cycle is a bit,
> but we are not using bits, instead we are using bytes.
Yes, the mentioning of 'bits' in the comment makes it not very clear at first
read.
Maybe just bellow would have been better:
/* Dummy clock cycles - modeled with bytes writes */
>
> Testing shows that +=1 is the correct way with the imx_spi controller,
> and with my SiFive SPI model in my local tree (not upstreamed yet)
Perhaps an option could be to look into how the aspeed_smc, xilinx_spips or the
npcm7xx_fiu generate dummy clock cycles and see if a similar solution to one of
those could work aswell for the imx_spi?
Regarding this patch, with += 8 it looks correct to me (and will work with
above controllers as far as I can see).
Best regards,
Francisco Iglesias
>
> Regards,
> Bin
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
Hi Francisco,
On Fri, Dec 4, 2020 at 6:46 PM Francisco Iglesias
wrote:
>
> Hello Bin,
>
> On [2020 Dec 04] Fri 15:52:12, Bin Meng wrote:
> > Hi Francisco,
> >
> > On Thu, Dec 3, 2020 at 4:38 PM Francisco Iglesias
> > wrote:
> > >
> > > Hi Bin and Alistair,
> > >
> > > On [2020 Dec 02] Wed 11:40:11, Alistair Francis wrote:
> > > > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng wrote:
> > > > >
> > > > > From: Bin Meng
> > > > >
> > > > > SST flashes require a dummy byte after the address bits.
> > > > >
> > > > > Signed-off-by: Bin Meng
> > > >
> > > > I couldn't find a datasheet that says this... But the actual code
> > > > change looks fine, so:
> > > >
> > > > Acked-by: Alistair Francis
> > > >
> > > > Alistair
> > > >
> > > > > ---
> > > > >
> > > > > hw/block/m25p80.c | 3 +++
> > > > > 1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > > > > index 483925f..9b36762 100644
> > > > > --- a/hw/block/m25p80.c
> > > > > +++ b/hw/block/m25p80.c
> > > > > @@ -825,6 +825,9 @@ static void decode_fast_read_cmd(Flash *s)
> > > > > s->needed_bytes = get_addr_length(s);
> > > > > switch (get_man(s)) {
> > > > > /* Dummy cycles - modeled with bytes writes instead of bits */
> > > > > +case MAN_SST:
> > > > > +s->needed_bytes += 1;
> > >
> > > 1 dummy clk cycle is modelled as 1 byte write (see the comment above), so
> > > 1
> > > dummy byte (8 dummy clk cycles) will need +8 above.
> >
> > I think you were confused by the WINBOND codes. The comments are
> > correct. It is modeled with bytes instead of bits, so we should +=1.
>
> What the comment says is (perhaps not superclear) that 1 dummy clock cycle
> is modeled as one 1 byte write into the flash (meaining that 8 byte writes
> are needed for 1 dummy byte). Perhaps it is easier to understand
> looking into how the controllers issue the command towards the flash model
> (for example the xilinx_spips), the start of the FAST_READ cmd is issued
> as writing the following into the flash: 1 byte (cmd), 3 bytes (address),
> 8 bytes (8 dummy cycles -> 1 dummy byte).
>
My interpretation of the comments are opposite: one cycle is a bit,
but we are not using bits, instead we are using bytes.
Testing shows that +=1 is the correct way with the imx_spi controller,
and with my SiFive SPI model in my local tree (not upstreamed yet)
Regards,
Bin
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
Hello Bin,
On [2020 Dec 04] Fri 15:52:12, Bin Meng wrote:
> Hi Francisco,
>
> On Thu, Dec 3, 2020 at 4:38 PM Francisco Iglesias
> wrote:
> >
> > Hi Bin and Alistair,
> >
> > On [2020 Dec 02] Wed 11:40:11, Alistair Francis wrote:
> > > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng wrote:
> > > >
> > > > From: Bin Meng
> > > >
> > > > SST flashes require a dummy byte after the address bits.
> > > >
> > > > Signed-off-by: Bin Meng
> > >
> > > I couldn't find a datasheet that says this... But the actual code
> > > change looks fine, so:
> > >
> > > Acked-by: Alistair Francis
> > >
> > > Alistair
> > >
> > > > ---
> > > >
> > > > hw/block/m25p80.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > > > index 483925f..9b36762 100644
> > > > --- a/hw/block/m25p80.c
> > > > +++ b/hw/block/m25p80.c
> > > > @@ -825,6 +825,9 @@ static void decode_fast_read_cmd(Flash *s)
> > > > s->needed_bytes = get_addr_length(s);
> > > > switch (get_man(s)) {
> > > > /* Dummy cycles - modeled with bytes writes instead of bits */
> > > > +case MAN_SST:
> > > > +s->needed_bytes += 1;
> >
> > 1 dummy clk cycle is modelled as 1 byte write (see the comment above), so 1
> > dummy byte (8 dummy clk cycles) will need +8 above.
>
> I think you were confused by the WINBOND codes. The comments are
> correct. It is modeled with bytes instead of bits, so we should +=1.
What the comment says is (perhaps not superclear) that 1 dummy clock cycle
is modeled as one 1 byte write into the flash (meaining that 8 byte writes
are needed for 1 dummy byte). Perhaps it is easier to understand
looking into how the controllers issue the command towards the flash model
(for example the xilinx_spips), the start of the FAST_READ cmd is issued
as writing the following into the flash: 1 byte (cmd), 3 bytes (address),
8 bytes (8 dummy cycles -> 1 dummy byte).
Best regards,
Francisco Iglesias
>
> > An option could be to fall
> > through to the Windbond case below instead (since it seems to operate
> > likewise).
> >
> > Best regards,
> > Francisco Iglesias
> >
> >
> > > > +break;
> > > > case MAN_WINBOND:
> > > > s->needed_bytes += 8;
>
> Actually this is wrong. This should be corrected to +=1. I will
> prepare a patch for it.
>
> > > > break;
> > > > --
>
> Regards,
> Bin
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
Hi Francisco,
On Thu, Dec 3, 2020 at 4:38 PM Francisco Iglesias
wrote:
>
> Hi Bin and Alistair,
>
> On [2020 Dec 02] Wed 11:40:11, Alistair Francis wrote:
> > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng wrote:
> > >
> > > From: Bin Meng
> > >
> > > SST flashes require a dummy byte after the address bits.
> > >
> > > Signed-off-by: Bin Meng
> >
> > I couldn't find a datasheet that says this... But the actual code
> > change looks fine, so:
> >
> > Acked-by: Alistair Francis
> >
> > Alistair
> >
> > > ---
> > >
> > > hw/block/m25p80.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > > index 483925f..9b36762 100644
> > > --- a/hw/block/m25p80.c
> > > +++ b/hw/block/m25p80.c
> > > @@ -825,6 +825,9 @@ static void decode_fast_read_cmd(Flash *s)
> > > s->needed_bytes = get_addr_length(s);
> > > switch (get_man(s)) {
> > > /* Dummy cycles - modeled with bytes writes instead of bits */
> > > +case MAN_SST:
> > > +s->needed_bytes += 1;
>
> 1 dummy clk cycle is modelled as 1 byte write (see the comment above), so 1
> dummy byte (8 dummy clk cycles) will need +8 above.
I think you were confused by the WINBOND codes. The comments are
correct. It is modeled with bytes instead of bits, so we should +=1.
> An option could be to fall
> through to the Windbond case below instead (since it seems to operate
> likewise).
>
> Best regards,
> Francisco Iglesias
>
>
> > > +break;
> > > case MAN_WINBOND:
> > > s->needed_bytes += 8;
Actually this is wrong. This should be corrected to +=1. I will
prepare a patch for it.
> > > break;
> > > --
Regards,
Bin
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
Hi Bin and Alistair,
On [2020 Dec 02] Wed 11:40:11, Alistair Francis wrote:
> On Sun, Nov 29, 2020 at 6:55 PM Bin Meng wrote:
> >
> > From: Bin Meng
> >
> > SST flashes require a dummy byte after the address bits.
> >
> > Signed-off-by: Bin Meng
>
> I couldn't find a datasheet that says this... But the actual code
> change looks fine, so:
>
> Acked-by: Alistair Francis
>
> Alistair
>
> > ---
> >
> > hw/block/m25p80.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > index 483925f..9b36762 100644
> > --- a/hw/block/m25p80.c
> > +++ b/hw/block/m25p80.c
> > @@ -825,6 +825,9 @@ static void decode_fast_read_cmd(Flash *s)
> > s->needed_bytes = get_addr_length(s);
> > switch (get_man(s)) {
> > /* Dummy cycles - modeled with bytes writes instead of bits */
> > +case MAN_SST:
> > +s->needed_bytes += 1;
1 dummy clk cycle is modelled as 1 byte write (see the comment above), so 1
dummy byte (8 dummy clk cycles) will need +8 above. An option could be to fall
through to the Windbond case below instead (since it seems to operate
likewise).
Best regards,
Francisco Iglesias
> > +break;
> > case MAN_WINBOND:
> > s->needed_bytes += 8;
> > break;
> > --
> > 2.7.4
> >
> >
>
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
On Wed, Dec 2, 2020 at 3:09 PM Bin Meng wrote: > > Hi Alistair, > > On Thu, Dec 3, 2020 at 3:52 AM Alistair Francis wrote: > > > > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng wrote: > > > > > > From: Bin Meng > > > > > > SST flashes require a dummy byte after the address bits. > > > > > > Signed-off-by: Bin Meng > > > > I couldn't find a datasheet that says this... But the actual code > > change looks fine, so: > > > > Please find the SST25VF016B datasheet at > http://ww1.microchip.com/downloads/en/devicedoc/s71271_04.pdf. The > fast read sequence is on page 11. Ah cool. I thought it would be somewhere, I just couldn't find it. Alistair > > > Acked-by: Alistair Francis > > > > Thanks! > > Regards, > Bin
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
Hi Alistair, On Thu, Dec 3, 2020 at 3:52 AM Alistair Francis wrote: > > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng wrote: > > > > From: Bin Meng > > > > SST flashes require a dummy byte after the address bits. > > > > Signed-off-by: Bin Meng > > I couldn't find a datasheet that says this... But the actual code > change looks fine, so: > Please find the SST25VF016B datasheet at http://ww1.microchip.com/downloads/en/devicedoc/s71271_04.pdf. The fast read sequence is on page 11. > Acked-by: Alistair Francis > Thanks! Regards, Bin
Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
On Sun, Nov 29, 2020 at 6:55 PM Bin Meng wrote:
>
> From: Bin Meng
>
> SST flashes require a dummy byte after the address bits.
>
> Signed-off-by: Bin Meng
I couldn't find a datasheet that says this... But the actual code
change looks fine, so:
Acked-by: Alistair Francis
Alistair
> ---
>
> hw/block/m25p80.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 483925f..9b36762 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -825,6 +825,9 @@ static void decode_fast_read_cmd(Flash *s)
> s->needed_bytes = get_addr_length(s);
> switch (get_man(s)) {
> /* Dummy cycles - modeled with bytes writes instead of bits */
> +case MAN_SST:
> +s->needed_bytes += 1;
> +break;
> case MAN_WINBOND:
> s->needed_bytes += 8;
> break;
> --
> 2.7.4
>
>
[PATCH] hw/block: m25p80: Fix fast read for SST flashes
From: Bin Meng
SST flashes require a dummy byte after the address bits.
Signed-off-by: Bin Meng
---
hw/block/m25p80.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 483925f..9b36762 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -825,6 +825,9 @@ static void decode_fast_read_cmd(Flash *s)
s->needed_bytes = get_addr_length(s);
switch (get_man(s)) {
/* Dummy cycles - modeled with bytes writes instead of bits */
+case MAN_SST:
+s->needed_bytes += 1;
+break;
case MAN_WINBOND:
s->needed_bytes += 8;
break;
--
2.7.4
