Re: [PATCH 1/1] hw: allow write_enable latch get/set

[Peter Delevoryas]

> On May 11, 2022, at 11:45 AM, Iris Chen via <qemu-devel@nongnu.org> wrote:
> 
> ---
> hw/block/m25p80.c             | 30 ++++++++++++++++++++++++++++++
> tests/qtest/aspeed_smc-test.c | 20 ++++++++++++++++++++
> 2 files changed, 50 insertions(+)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 430d1298a8..fb72704e5a 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -35,6 +35,7 @@
> #include "qapi/error.h"
> #include "trace.h"
> #include "qom/object.h"
> +#include "qapi/visitor.h"
> 
> /* Fields for FlashPartInfo->flags */
> 
> @@ -1646,6 +1647,31 @@ static const VMStateDescription vmstate_m25p80 = {
>     }
> };
> 
> +static void m25p80_get_wel(Object *obj, Visitor *v, const char *name,
> +                            void *opaque, Error **errp)
> +{
> +    Flash *s = M25P80(obj);
> +
> +    assert(strcmp(name, "WEL") == 0);
> +
> +    visit_type_bool(v, name, &s->write_enable, errp);
> +}
> +
> +static void m25p80_set_wel(Object *obj, Visitor *v, const char *name,
> +                            void *opaque, Error **errp)
> +{
> +    Flash *s = M25P80(obj);
> +    bool value;
> +
> +    assert(strcmp(name, "WEL") == 0);
> +
> +    if (!visit_type_bool(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    s->write_enable = value;
> +}
> +
> static void m25p80_class_init(ObjectClass *klass, void *data)
> {
>     DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -1660,6 +1686,10 @@ static void m25p80_class_init(ObjectClass *klass, void 
> *data)
>     device_class_set_props(dc, m25p80_properties);
>     dc->reset = m25p80_reset;
>     mc->pi = data;
> +
> +    object_class_property_add(klass, "WEL", "bool",
> +                            m25p80_get_wel,
> +                            m25p80_set_wel, NULL, NULL);

I think the spacing here might be slightly off, try making the 2nd and 3rd lines
align with “klass” in the first line.

> }
> 
> static const TypeInfo m25p80_info = {
> diff --git a/tests/qtest/aspeed_smc-test.c b/tests/qtest/aspeed_smc-test.c
> index 87b40a0ef1..af885a9c9d 100644
> --- a/tests/qtest/aspeed_smc-test.c
> +++ b/tests/qtest/aspeed_smc-test.c
> @@ -49,6 +49,7 @@
>  */
> enum {
>     JEDEC_READ = 0x9f,
> +    RDSR = 0x5,
>     BULK_ERASE = 0xc7,
>     READ = 0x03,
>     PP = 0x02,
> @@ -348,6 +349,24 @@ static void test_write_page_mem(void)
>     flash_reset();
> }
> 
> +static void test_read_status_reg(void)
> +{
> +    uint8_t r;
> +
> +     qmp("{ 'execute': 'qom-set', 'arguments': "
> +       "{'path': '/machine/soc/fmc/ssi.0/child[0]', 'property': 'WEL', 
> 'value': true}}");

Spacing is off here, make sure the indentation is set to 4 spaces.

> +
> +    spi_conf(CONF_ENABLE_W0);
> +     spi_ctrl_start_user();
> +     writeb(ASPEED_FLASH_BASE, RDSR);
> +     r = readb(ASPEED_FLASH_BASE);
> +     spi_ctrl_stop_user();

Spacing is off here too.

> +
> +     g_assert_cmphex(r, ==, 0x2);

Maybe consider using a constant instead of 0x2, like:

...
#include “qemu/bitops.h" 
...
#define SR_WEL BIT(1)
…
g_assert_cmphex(r, ==, SR_WEL);

Additionally, since we only care about testing the write enable
latch state (and not the other bits in the status register),
it could be:

g_assert_cmphex(r & SR_WEL, ==, SR_WEL);

> +
> +     flash_reset();
> +}
> +
> static char tmp_path[] = "/tmp/qtest.m25p80.XXXXXX";
> 
> int main(int argc, char **argv)
> @@ -373,6 +392,7 @@ int main(int argc, char **argv)
>     qtest_add_func("/ast2400/smc/write_page", test_write_page);
>     qtest_add_func("/ast2400/smc/read_page_mem", test_read_page_mem);
>     qtest_add_func("/ast2400/smc/write_page_mem", test_write_page_mem);
> +    qtest_add_func("/ast2400/smc/read_status_reg", test_read_status_reg);
> 
>     ret = g_test_run();
> 
> -- 
> 2.30.2
> 
> 
>