Hi Iris,
Looks good some, a couple of comments below.
On [2022 Jun 08] Wed 20:13:19, Iris Chen wrote:
> From: Iris Chen <irische...@gmail.com>
>
> Signed-off-by: Iris Chen <irische...@gmail.com>
> ---
> Addressed all comments from V1. The biggest change: removed
> object_class_property_add.
>
> hw/block/m25p80.c | 37 +++++++++++++++++++++++++++++++++++
> tests/qtest/aspeed_smc-test.c | 2 ++
> 2 files changed, 39 insertions(+)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 81ba3da4df..1a20bd55d4 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"
> #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,21 @@ static void complete_collecting_data(Flash *s)
> flash_erase(s, s->cur_addr, s->cmd_in_progress);
> break;
> case WRSR:
> + /*
> + * 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) {
'write_enable' needs to be true in 'decode_new_cmd' when issueing the WRSR
command, otherwise the state machinery will not advance to this function
(meaning that above check for !s->write_enable will never hit as far as I can
tell). A suggestion is to move the check for wp_level and
status_reg_wr_disabled into 'decode_new_cmd' to for keeping it consistent.
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "M25P80: Status register write is disabled!\n");
> + break;
> + }
> + 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);
> @@ -1195,6 +1214,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 +1505,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 +1544,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);
> }
>
> @@ -1601,6 +1636,8 @@ static const VMStateDescription vmstate_m25p80 = {
> VMSTATE_UINT8(needed_bytes, Flash),
> VMSTATE_UINT8(cmd_in_progress, Flash),
> VMSTATE_UINT32(cur_addr, Flash),
> + VMSTATE_BOOL(wp_level, Flash),
> + VMSTATE_BOOL(status_register_write_disabled, Flash),
Above needs to be added through a subsection, you can see commit 465ef47abe3
for an example an also read about this in docs/devel/migration.rst.
Thank you,
Best regads,
Francisco Iglesias
> VMSTATE_BOOL(write_enable, Flash),
> VMSTATE_BOOL(reset_enable, Flash),
> VMSTATE_UINT8(ear, Flash),
> diff --git a/tests/qtest/aspeed_smc-test.c b/tests/qtest/aspeed_smc-test.c
> index ec233315e6..c5d97d4410 100644
> --- a/tests/qtest/aspeed_smc-test.c
> +++ b/tests/qtest/aspeed_smc-test.c
> @@ -56,7 +56,9 @@ enum {
> BULK_ERASE = 0xc7,
> READ = 0x03,
> PP = 0x02,
> + WRSR = 0x1,
> WREN = 0x6,
> + SRWD = 0x80,
> RESET_ENABLE = 0x66,
> RESET_MEMORY = 0x99,
> EN_4BYTE_ADDR = 0xB7,
> --
> 2.30.2
>
>
