Re: [PATCH 01/22] hw/block: m25p80: Add ISSI SPI flash support

[Bin Meng]

Hi Francisco,

On Tue, Jan 5, 2021 at 12:00 AM Francisco Iglesias
<frasse.iglesias@gmail.com> wrote:
>
> Hi Bin,
>
> On [2020 Dec 31] Thu 19:29:49, Bin Meng wrote:
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > This adds the ISSI SPI flash support. The number of dummy cycles in
> > fast read, fast read dual output and fast read quad output commands
> > is currently using the default 8. Per the datasheet [1], the number
> > of dummy cycles configurable, but this is not modeled.
> >
> > For flash whose size is larger than 16 MiB, the sequence of 3-byte
> > address along with EXTADD bit in the bank address register (BAR) is
> > not supported. Currently we assume that guest software will alawys
> > use op codes with 4-byte address sequence. Fortunately this is the
> > case for both U-Boot and Linux.
> >
> > [1] http://www.issi.com/WW/pdf/25LP-WP256.pdf
> >
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > ---
> >
> >  hw/block/m25p80.c | 38 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > index 844cabea21..8a62bc4bc4 100644
> > --- a/hw/block/m25p80.c
> > +++ b/hw/block/m25p80.c
> > @@ -411,6 +411,7 @@ typedef enum {
> >      MAN_NUMONYX,
> >      MAN_WINBOND,
> >      MAN_SST,
> > +    MAN_ISSI,
> >      MAN_GENERIC,
> >  } Manufacturer;
> >
> > @@ -486,6 +487,8 @@ static inline Manufacturer get_man(Flash *s)
> >          return MAN_MACRONIX;
> >      case 0xBF:
> >          return MAN_SST;
> > +    case 0x9D:
> > +        return MAN_ISSI;
> >      default:
> >          return MAN_GENERIC;
> >      }
> > @@ -705,6 +708,9 @@ static void complete_collecting_data(Flash *s)
> >          case MAN_SPANSION:
> >              s->quad_enable = !!(s->data[1] & 0x02);
> >              break;
> > +        case MAN_ISSI:
> > +            s->quad_enable = extract32(s->data[0], 6, 1);
> > +            break;
> >          case MAN_MACRONIX:
> >              s->quad_enable = extract32(s->data[0], 6, 1);
> >              if (s->len > 1) {
> > @@ -897,6 +903,16 @@ static void decode_fast_read_cmd(Flash *s)
> >                                      SPANSION_DUMMY_CLK_LEN
> >                                      );
> >          break;
> > +    case MAN_ISSI:
> > +        /*
> > +         * The fast read instruction code is followed by address bytes and
> > +         * dummy cycles, transmitted via the SI line.
> > +         *
> > +         * The number of dummy cycles are configurable but this is 
> > currently
> > +         * unmodeled, hence the default value 8 is used.
> > +         */
> > +        s->needed_bytes += ((8 * 1) / 8);
>
> According to how m25p80 models dummy clock cycles above
> means that the command is being modeled with 1 dummy clock cycle (and below is
> modeling the dio/qio commands with 1 and 3 dummy clock cycles). To model
> the command with 8 dummy clock cycles you only add +8 above (+4 and +6
> would be the values to add below). One can look into how one of the other
> flashes model the commands for examples. This might also mean that the
> controller will need a change and do the opposite what above calculation
> does, and convert the dummy bytes into dummy clock cycles (when
> transmitting on 1 line it generates 8 dummy clock cycles for each dummy
> byte, when it uses 2 lines it generates 4 etc..).
>

I now am more inclined to say something is wrong in the Xilinx SPI
models and I just don't have time to check what is wrong there. As I
mentioned here [1] for SPI controllers that use fifo to stuff the
dummy bits on the line, there is no way for hardware to tell what is
dummy bits in the fifo or not. The hardware just sends whatever in the
fifo which is prepared by the software. Hence the only way to get such
hardware to work is to use bytes, instead of bits in QEMU.

[1] 
http://patchwork.ozlabs.org/project/qemu-devel/patch/1606704602-59435-1-git-send-email-bmeng.cn@gmail.com/

Regards,
Bin