Hi Iris,
On [2022 Jun 17] Fri 15:02:45, Iris Chen wrote:
> Signed-off-by: Iris Chen <irische...@fb.com>
> ---
> Thanks everyone for your comments. This is a v3 patch that addresses all
> suggestions (moving write_enable to decode_new_cmd).
> I am waiting on some feedback from Dan's (dz4l...@gmail.com) patch
> regarding adding a STATE_STANDBY state.
>
> Currently, all tests are passing.
>
> hw/block/m25p80.c | 77 ++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 62 insertions(+), 15 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 81ba3da4df..12a59ca57c 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -27,12 +27,14 @@
> #include "hw/qdev-properties.h"
> #include "hw/qdev-properties-system.h"
> #include "hw/ssi/ssi.h"
> +#include "hw/irq.h"
> #include "migration/vmstate.h"
> #include "qemu/bitops.h"
> #include "qemu/log.h"
> #include "qemu/module.h"
> #include "qemu/error-report.h"
> #include "qapi/error.h"
> +#include "qapi/visitor.h"
We can remove above two new header includes (sorry for missing this last time).
> #include "trace.h"
> #include "qom/object.h"
>
> @@ -472,11 +474,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 +727,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 +1171,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 +1213,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 +1504,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 +1543,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 +1621,18 @@ static const VMStateDescription
> vmstate_m25p80_aai_enable = {
> }
> };
>
> +static const VMStateDescription vmstate_m25p80_write_protect = {
> + .name = "m25p80/write_protect",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = false,
Above is a function, something like:
static bool m25p80_wp_level_srwd_needed(void *opaque)
{
Flash *s = (Flash *)opaque;
return !s->wp_level || s->status_register_write_disabled;
}
.needed = m25p80_wp_level_srwd_needed,
Looks good otherwise!
Thanks,
Best regards,
Francisco
> + .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 +1664,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
>
>
