Re: [PATCH v4] hw: m25p80: add WP# pin and SRWD bit for write protection

[Francisco Iglesias]

On [2022 Jun 28] Tue 17:52:50, Cédric Le Goater wrote:
> Alistair, Francisco,
> 
> On 6/22/22 11:45, Francisco Iglesias wrote:
> > On [2022 Jun 21] Tue 13:24:27, Iris Chen wrote:
> > > From: Iris Chen <irischenlj@gmail.com>
> > > 
> > > Signed-off-by: Iris Chen <irischenlj@gmail.com>
> > 
> > Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
> 
> I am planning to include this patch in the next aspeed PR if that's
> OK with you.

Sounds good to me Cédric!

Best regards,
Francisco

> 
> Thanks,
> 
> C.
> 
> > 
> > > ---
> > > Fixed .needed for subsection and suggestions from Francisco
> > > 
> > >   hw/block/m25p80.c | 82 ++++++++++++++++++++++++++++++++++++++---------
> > >   1 file changed, 67 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > > index 81ba3da4df..3045dda53b 100644
> > > --- a/hw/block/m25p80.c
> > > +++ b/hw/block/m25p80.c
> > > @@ -472,11 +472,13 @@ struct Flash {
> > >       uint8_t spansion_cr2v;
> > >       uint8_t spansion_cr3v;
> > >       uint8_t spansion_cr4v;
> > > +    bool wp_level;
> > >       bool write_enable;
> > >       bool four_bytes_address_mode;
> > >       bool reset_enable;
> > >       bool quad_enable;
> > >       bool aai_enable;
> > > +    bool status_register_write_disabled;
> > >       uint8_t ear;
> > >       int64_t dirty_page;
> > > @@ -723,6 +725,8 @@ static void complete_collecting_data(Flash *s)
> > >           flash_erase(s, s->cur_addr, s->cmd_in_progress);
> > >           break;
> > >       case WRSR:
> > > +        s->status_register_write_disabled = extract32(s->data[0], 7, 1);
> > > +
> > >           switch (get_man(s)) {
> > >           case MAN_SPANSION:
> > >               s->quad_enable = !!(s->data[1] & 0x02);
> > > @@ -1165,22 +1169,34 @@ static void decode_new_cmd(Flash *s, uint32_t 
> > > value)
> > >           break;
> > >       case WRSR:
> > > -        if (s->write_enable) {
> > > -            switch (get_man(s)) {
> > > -            case MAN_SPANSION:
> > > -                s->needed_bytes = 2;
> > > -                s->state = STATE_COLLECTING_DATA;
> > > -                break;
> > > -            case MAN_MACRONIX:
> > > -                s->needed_bytes = 2;
> > > -                s->state = STATE_COLLECTING_VAR_LEN_DATA;
> > > -                break;
> > > -            default:
> > > -                s->needed_bytes = 1;
> > > -                s->state = STATE_COLLECTING_DATA;
> > > -            }
> > > -            s->pos = 0;
> > > +        /*
> > > +         * If WP# is low and status_register_write_disabled is high,
> > > +         * status register writes are disabled.
> > > +         * This is also called "hardware protected mode" (HPM). All other
> > > +         * combinations of the two states are called "software protected 
> > > mode"
> > > +         * (SPM), and status register writes are permitted.
> > > +         */
> > > +        if ((s->wp_level == 0 && s->status_register_write_disabled)
> > > +            || !s->write_enable) {
> > > +            qemu_log_mask(LOG_GUEST_ERROR,
> > > +                          "M25P80: Status register write is 
> > > disabled!\n");
> > > +            break;
> > > +        }
> > > +
> > > +        switch (get_man(s)) {
> > > +        case MAN_SPANSION:
> > > +            s->needed_bytes = 2;
> > > +            s->state = STATE_COLLECTING_DATA;
> > > +            break;
> > > +        case MAN_MACRONIX:
> > > +            s->needed_bytes = 2;
> > > +            s->state = STATE_COLLECTING_VAR_LEN_DATA;
> > > +            break;
> > > +        default:
> > > +            s->needed_bytes = 1;
> > > +            s->state = STATE_COLLECTING_DATA;
> > >           }
> > > +        s->pos = 0;
> > >           break;
> > >       case WRDI:
> > > @@ -1195,6 +1211,8 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> > >       case RDSR:
> > >           s->data[0] = (!!s->write_enable) << 1;
> > > +        s->data[0] |= (!!s->status_register_write_disabled) << 7;
> > > +
> > >           if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) {
> > >               s->data[0] |= (!!s->quad_enable) << 6;
> > >           }
> > > @@ -1484,6 +1502,14 @@ static uint32_t m25p80_transfer8(SSIPeripheral 
> > > *ss, uint32_t tx)
> > >       return r;
> > >   }
> > > +static void m25p80_write_protect_pin_irq_handler(void *opaque, int n, 
> > > int level)
> > > +{
> > > +    Flash *s = M25P80(opaque);
> > > +    /* WP# is just a single pin. */
> > > +    assert(n == 0);
> > > +    s->wp_level = !!level;
> > > +}
> > > +
> > >   static void m25p80_realize(SSIPeripheral *ss, Error **errp)
> > >   {
> > >       Flash *s = M25P80(ss);
> > > @@ -1515,12 +1541,18 @@ static void m25p80_realize(SSIPeripheral *ss, 
> > > Error **errp)
> > >           s->storage = blk_blockalign(NULL, s->size);
> > >           memset(s->storage, 0xFF, s->size);
> > >       }
> > > +
> > > +    qdev_init_gpio_in_named(DEVICE(s),
> > > +                            m25p80_write_protect_pin_irq_handler, "WP#", 
> > > 1);
> > >   }
> > >   static void m25p80_reset(DeviceState *d)
> > >   {
> > >       Flash *s = M25P80(d);
> > > +    s->wp_level = true;
> > > +    s->status_register_write_disabled = false;
> > > +
> > >       reset_memory(s);
> > >   }
> > > @@ -1587,6 +1619,25 @@ static const VMStateDescription 
> > > vmstate_m25p80_aai_enable = {
> > >       }
> > >   };
> > > +static bool m25p80_wp_level_srwd_needed(void *opaque)
> > > +{
> > > +    Flash *s = (Flash *)opaque;
> > > +
> > > +    return !s->wp_level || s->status_register_write_disabled;
> > > +}
> > > +
> > > +static const VMStateDescription vmstate_m25p80_write_protect = {
> > > +    .name = "m25p80/write_protect",
> > > +    .version_id = 1,
> > > +    .minimum_version_id = 1,
> > > +    .needed = m25p80_wp_level_srwd_needed,
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_BOOL(wp_level, Flash),
> > > +        VMSTATE_BOOL(status_register_write_disabled, Flash),
> > > +        VMSTATE_END_OF_LIST()
> > > +    }
> > > +};
> > > +
> > >   static const VMStateDescription vmstate_m25p80 = {
> > >       .name = "m25p80",
> > >       .version_id = 0,
> > > @@ -1618,6 +1669,7 @@ static const VMStateDescription vmstate_m25p80 = {
> > >       .subsections = (const VMStateDescription * []) {
> > >           &vmstate_m25p80_data_read_loop,
> > >           &vmstate_m25p80_aai_enable,
> > > +        &vmstate_m25p80_write_protect,
> > >           NULL
> > >       }
> > >   };
> > > -- 
> > > 2.30.2
> > > 
> > > 
>