* Re: [PATCH v3] hw: m25p80: allow write_enable latch get/set
2022-05-13 5:50 ` [PATCH v3] hw: m25p80: allow write_enable latch get/set Iris Chen via
@ 2022-05-13 13:54 ` Cédric Le Goater
2022-05-13 14:37 ` Thomas Huth
2022-05-13 22:47 ` Francisco Iglesias
1 sibling, 1 reply; 5+ messages in thread
From: Cédric Le Goater @ 2022-05-13 13:54 UTC (permalink / raw)
To: Iris Chen
Cc: pdel, qemu-devel, qemu-arm, patrick, alistair, kwolf, hreitz,
peter.maydell, andrew, joel, thuth, lvivier, pbonzini,
qemu-block
Thomas, and others,
Are you ok with the qtest extension proposed by Iris ?
If so, I can take them through an aspeed PR.
Thanks,
C.
On 5/13/22 07:50, Iris Chen via wrote:
> The write_enable latch property is not currently exposed.
> This commit makes it a modifiable property.
>
> Signed-off-by: Iris Chen <irischenlj@fb.com>
> ---
> v3: Addressed comments by Peter and Cedric.
> v2: Ran ./scripts/checkpatch.pl on the patch and added a description. Fixed comments regarding DEFINE_PROP_BOOL.
>
> hw/block/m25p80.c | 1 +
> tests/qtest/aspeed_gpio-test.c | 40 +++++++------------------------
> tests/qtest/aspeed_smc-test.c | 43 ++++++++++++++++++++++++++++++++++
> tests/qtest/libqtest.c | 24 +++++++++++++++++++
> tests/qtest/libqtest.h | 22 +++++++++++++++++
> 5 files changed, 98 insertions(+), 32 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 430d1298a8..4283b990af 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -1558,6 +1558,7 @@ static int m25p80_pre_save(void *opaque)
>
> static Property m25p80_properties[] = {
> /* This is default value for Micron flash */
> + DEFINE_PROP_BOOL("write-enable", Flash, write_enable, false),
> DEFINE_PROP_UINT32("nonvolatile-cfg", Flash, nonvolatile_cfg, 0x8FFF),
> DEFINE_PROP_UINT8("spansion-cr1nv", Flash, spansion_cr1nv, 0x0),
> DEFINE_PROP_UINT8("spansion-cr2nv", Flash, spansion_cr2nv, 0x8),
> diff --git a/tests/qtest/aspeed_gpio-test.c b/tests/qtest/aspeed_gpio-test.c
> index c1003f2d1b..bac63e8742 100644
> --- a/tests/qtest/aspeed_gpio-test.c
> +++ b/tests/qtest/aspeed_gpio-test.c
> @@ -28,30 +28,6 @@
> #include "qapi/qmp/qdict.h"
> #include "libqtest-single.h"
>
> -static bool qom_get_bool(QTestState *s, const char *path, const char *property)
> -{
> - QDict *r;
> - bool b;
> -
> - r = qtest_qmp(s, "{ 'execute': 'qom-get', 'arguments': "
> - "{ 'path': %s, 'property': %s } }", path, property);
> - b = qdict_get_bool(r, "return");
> - qobject_unref(r);
> -
> - return b;
> -}
> -
> -static void qom_set_bool(QTestState *s, const char *path, const char *property,
> - bool value)
> -{
> - QDict *r;
> -
> - r = qtest_qmp(s, "{ 'execute': 'qom-set', 'arguments': "
> - "{ 'path': %s, 'property': %s, 'value': %i } }",
> - path, property, value);
> - qobject_unref(r);
> -}
> -
> static void test_set_colocated_pins(const void *data)
> {
> QTestState *s = (QTestState *)data;
> @@ -60,14 +36,14 @@ static void test_set_colocated_pins(const void *data)
> * gpioV4-7 occupy bits within a single 32-bit value, so we want to make
> * sure that modifying one doesn't affect the other.
> */
> - qom_set_bool(s, "/machine/soc/gpio", "gpioV4", true);
> - qom_set_bool(s, "/machine/soc/gpio", "gpioV5", false);
> - qom_set_bool(s, "/machine/soc/gpio", "gpioV6", true);
> - qom_set_bool(s, "/machine/soc/gpio", "gpioV7", false);
> - g_assert(qom_get_bool(s, "/machine/soc/gpio", "gpioV4"));
> - g_assert(!qom_get_bool(s, "/machine/soc/gpio", "gpioV5"));
> - g_assert(qom_get_bool(s, "/machine/soc/gpio", "gpioV6"));
> - g_assert(!qom_get_bool(s, "/machine/soc/gpio", "gpioV7"));
> + qtest_qom_set_bool(s, "/machine/soc/gpio", "gpioV4", true);
> + qtest_qom_set_bool(s, "/machine/soc/gpio", "gpioV5", false);
> + qtest_qom_set_bool(s, "/machine/soc/gpio", "gpioV6", true);
> + qtest_qom_set_bool(s, "/machine/soc/gpio", "gpioV7", false);
> + g_assert(qtest_qom_get_bool(s, "/machine/soc/gpio", "gpioV4"));
> + g_assert(!qtest_qom_get_bool(s, "/machine/soc/gpio", "gpioV5"));
> + g_assert(qtest_qom_get_bool(s, "/machine/soc/gpio", "gpioV6"));
> + g_assert(!qtest_qom_get_bool(s, "/machine/soc/gpio", "gpioV7"));
> }
>
> int main(int argc, char **argv)
> diff --git a/tests/qtest/aspeed_smc-test.c b/tests/qtest/aspeed_smc-test.c
> index 87b40a0ef1..ec233315e6 100644
> --- a/tests/qtest/aspeed_smc-test.c
> +++ b/tests/qtest/aspeed_smc-test.c
> @@ -26,6 +26,7 @@
> #include "qemu/osdep.h"
> #include "qemu/bswap.h"
> #include "libqtest-single.h"
> +#include "qemu/bitops.h"
>
> /*
> * ASPEED SPI Controller registers
> @@ -40,6 +41,7 @@
> #define CTRL_FREADMODE 0x1
> #define CTRL_WRITEMODE 0x2
> #define CTRL_USERMODE 0x3
> +#define SR_WEL BIT(1)
>
> #define ASPEED_FMC_BASE 0x1E620000
> #define ASPEED_FLASH_BASE 0x20000000
> @@ -49,6 +51,8 @@
> */
> enum {
> JEDEC_READ = 0x9f,
> + RDSR = 0x5,
> + WRDI = 0x4,
> BULK_ERASE = 0xc7,
> READ = 0x03,
> PP = 0x02,
> @@ -348,6 +352,44 @@ static void test_write_page_mem(void)
> flash_reset();
> }
>
> +static void test_read_status_reg(void)
> +{
> + uint8_t r;
> +
> + spi_conf(CONF_ENABLE_W0);
> +
> + spi_ctrl_start_user();
> + writeb(ASPEED_FLASH_BASE, RDSR);
> + r = readb(ASPEED_FLASH_BASE);
> + spi_ctrl_stop_user();
> +
> + g_assert_cmphex(r & SR_WEL, ==, 0);
> + g_assert(!qtest_qom_get_bool
> + (global_qtest, "/machine/soc/fmc/ssi.0/child[0]", "write-enable"));
> +
> + spi_ctrl_start_user();
> + writeb(ASPEED_FLASH_BASE, WREN);
> + writeb(ASPEED_FLASH_BASE, RDSR);
> + r = readb(ASPEED_FLASH_BASE);
> + spi_ctrl_stop_user();
> +
> + g_assert_cmphex(r & SR_WEL, ==, SR_WEL);
> + g_assert(qtest_qom_get_bool
> + (global_qtest, "/machine/soc/fmc/ssi.0/child[0]", "write-enable"));
> +
> + spi_ctrl_start_user();
> + writeb(ASPEED_FLASH_BASE, WRDI);
> + writeb(ASPEED_FLASH_BASE, RDSR);
> + r = readb(ASPEED_FLASH_BASE);
> + spi_ctrl_stop_user();
> +
> + g_assert_cmphex(r & SR_WEL, ==, 0);
> + g_assert(!qtest_qom_get_bool
> + (global_qtest, "/machine/soc/fmc/ssi.0/child[0]", "write-enable"));
> +
> + flash_reset();
> +}
> +
> static char tmp_path[] = "/tmp/qtest.m25p80.XXXXXX";
>
> int main(int argc, char **argv)
> @@ -373,6 +415,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();
>
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 228357f1ea..a9904eba82 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -1423,3 +1423,27 @@ void qtest_client_inproc_recv(void *opaque, const char *str)
> g_string_append(qts->rx, str);
> return;
> }
> +
> +void qtest_qom_set_bool(QTestState *s, const char *path, const char *property,
> + bool value)
> +{
> + QDict *r;
> +
> + r = qtest_qmp(s, "{ 'execute': 'qom-set', 'arguments': "
> + "{ 'path': %s, 'property': %s, 'value': %i } }",
> + path, property, value);
> + qobject_unref(r);
> +}
> +
> +bool qtest_qom_get_bool(QTestState *s, const char *path, const char *property)
> +{
> + QDict *r;
> + bool b;
> +
> + r = qtest_qmp(s, "{ 'execute': 'qom-get', 'arguments': "
> + "{ 'path': %s, 'property': %s } }", path, property);
> + b = qdict_get_bool(r, "return");
> + qobject_unref(r);
> +
> + return b;
> +}
> diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
> index 4ab0cad326..94b187837d 100644
> --- a/tests/qtest/libqtest.h
> +++ b/tests/qtest/libqtest.h
> @@ -783,4 +783,26 @@ QTestState *qtest_inproc_init(QTestState **s, bool log, const char* arch,
> void (*send)(void*, const char*));
>
> void qtest_client_inproc_recv(void *opaque, const char *str);
> +
> +/**
> + * qtest_qom_set_bool:
> + * @s: QTestState instance to operate on.
> + * @path: Path to the property being set.
> + * @property: Property being set.
> + * @value: Value to set the property.
> + *
> + * Set the property with passed in value.
> + */
> +void qtest_qom_set_bool(QTestState *s, const char *path, const char *property,
> + bool value);
> +
> +/**
> + * qtest_qom_get_bool:
> + * @s: QTestState instance to operate on.
> + * @path: Path to the property being retrieved.
> + * @property: Property from where the value is being retrieved.
> + *
> + * Returns: Value retrieved from property.
> + */
> +bool qtest_qom_get_bool(QTestState *s, const char *path, const char *property);
> #endif
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3] hw: m25p80: allow write_enable latch get/set
2022-05-13 5:50 ` [PATCH v3] hw: m25p80: allow write_enable latch get/set Iris Chen via
2022-05-13 13:54 ` Cédric Le Goater
@ 2022-05-13 22:47 ` Francisco Iglesias
1 sibling, 0 replies; 5+ messages in thread
From: Francisco Iglesias @ 2022-05-13 22:47 UTC (permalink / raw)
To: Iris Chen
Cc: pdel, qemu-devel, qemu-arm, clg, patrick, alistair, kwolf,
hreitz, peter.maydell, andrew, joel, thuth, lvivier, pbonzini,
qemu-block
On [2022 May 12] Thu 22:50:22, Iris Chen via wrote:
> The write_enable latch property is not currently exposed.
> This commit makes it a modifiable property.
>
> Signed-off-by: Iris Chen <irischenlj@fb.com>
> ---
> v3: Addressed comments by Peter and Cedric.
> v2: Ran ./scripts/checkpatch.pl on the patch and added a description. Fixed comments regarding DEFINE_PROP_BOOL.
>
> hw/block/m25p80.c | 1 +
> tests/qtest/aspeed_gpio-test.c | 40 +++++++------------------------
> tests/qtest/aspeed_smc-test.c | 43 ++++++++++++++++++++++++++++++++++
> tests/qtest/libqtest.c | 24 +++++++++++++++++++
> tests/qtest/libqtest.h | 22 +++++++++++++++++
> 5 files changed, 98 insertions(+), 32 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 430d1298a8..4283b990af 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -1558,6 +1558,7 @@ static int m25p80_pre_save(void *opaque)
>
> static Property m25p80_properties[] = {
> /* This is default value for Micron flash */
> + DEFINE_PROP_BOOL("write-enable", Flash, write_enable, false),
Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
> DEFINE_PROP_UINT32("nonvolatile-cfg", Flash, nonvolatile_cfg, 0x8FFF),
> DEFINE_PROP_UINT8("spansion-cr1nv", Flash, spansion_cr1nv, 0x0),
> DEFINE_PROP_UINT8("spansion-cr2nv", Flash, spansion_cr2nv, 0x8),
> diff --git a/tests/qtest/aspeed_gpio-test.c b/tests/qtest/aspeed_gpio-test.c
> index c1003f2d1b..bac63e8742 100644
> --- a/tests/qtest/aspeed_gpio-test.c
> +++ b/tests/qtest/aspeed_gpio-test.c
> @@ -28,30 +28,6 @@
> #include "qapi/qmp/qdict.h"
> #include "libqtest-single.h"
>
> -static bool qom_get_bool(QTestState *s, const char *path, const char *property)
> -{
> - QDict *r;
> - bool b;
> -
> - r = qtest_qmp(s, "{ 'execute': 'qom-get', 'arguments': "
> - "{ 'path': %s, 'property': %s } }", path, property);
> - b = qdict_get_bool(r, "return");
> - qobject_unref(r);
> -
> - return b;
> -}
> -
> -static void qom_set_bool(QTestState *s, const char *path, const char *property,
> - bool value)
> -{
> - QDict *r;
> -
> - r = qtest_qmp(s, "{ 'execute': 'qom-set', 'arguments': "
> - "{ 'path': %s, 'property': %s, 'value': %i } }",
> - path, property, value);
> - qobject_unref(r);
> -}
> -
> static void test_set_colocated_pins(const void *data)
> {
> QTestState *s = (QTestState *)data;
> @@ -60,14 +36,14 @@ static void test_set_colocated_pins(const void *data)
> * gpioV4-7 occupy bits within a single 32-bit value, so we want to make
> * sure that modifying one doesn't affect the other.
> */
> - qom_set_bool(s, "/machine/soc/gpio", "gpioV4", true);
> - qom_set_bool(s, "/machine/soc/gpio", "gpioV5", false);
> - qom_set_bool(s, "/machine/soc/gpio", "gpioV6", true);
> - qom_set_bool(s, "/machine/soc/gpio", "gpioV7", false);
> - g_assert(qom_get_bool(s, "/machine/soc/gpio", "gpioV4"));
> - g_assert(!qom_get_bool(s, "/machine/soc/gpio", "gpioV5"));
> - g_assert(qom_get_bool(s, "/machine/soc/gpio", "gpioV6"));
> - g_assert(!qom_get_bool(s, "/machine/soc/gpio", "gpioV7"));
> + qtest_qom_set_bool(s, "/machine/soc/gpio", "gpioV4", true);
> + qtest_qom_set_bool(s, "/machine/soc/gpio", "gpioV5", false);
> + qtest_qom_set_bool(s, "/machine/soc/gpio", "gpioV6", true);
> + qtest_qom_set_bool(s, "/machine/soc/gpio", "gpioV7", false);
> + g_assert(qtest_qom_get_bool(s, "/machine/soc/gpio", "gpioV4"));
> + g_assert(!qtest_qom_get_bool(s, "/machine/soc/gpio", "gpioV5"));
> + g_assert(qtest_qom_get_bool(s, "/machine/soc/gpio", "gpioV6"));
> + g_assert(!qtest_qom_get_bool(s, "/machine/soc/gpio", "gpioV7"));
> }
>
> int main(int argc, char **argv)
> diff --git a/tests/qtest/aspeed_smc-test.c b/tests/qtest/aspeed_smc-test.c
> index 87b40a0ef1..ec233315e6 100644
> --- a/tests/qtest/aspeed_smc-test.c
> +++ b/tests/qtest/aspeed_smc-test.c
> @@ -26,6 +26,7 @@
> #include "qemu/osdep.h"
> #include "qemu/bswap.h"
> #include "libqtest-single.h"
> +#include "qemu/bitops.h"
>
> /*
> * ASPEED SPI Controller registers
> @@ -40,6 +41,7 @@
> #define CTRL_FREADMODE 0x1
> #define CTRL_WRITEMODE 0x2
> #define CTRL_USERMODE 0x3
> +#define SR_WEL BIT(1)
>
> #define ASPEED_FMC_BASE 0x1E620000
> #define ASPEED_FLASH_BASE 0x20000000
> @@ -49,6 +51,8 @@
> */
> enum {
> JEDEC_READ = 0x9f,
> + RDSR = 0x5,
> + WRDI = 0x4,
> BULK_ERASE = 0xc7,
> READ = 0x03,
> PP = 0x02,
> @@ -348,6 +352,44 @@ static void test_write_page_mem(void)
> flash_reset();
> }
>
> +static void test_read_status_reg(void)
> +{
> + uint8_t r;
> +
> + spi_conf(CONF_ENABLE_W0);
> +
> + spi_ctrl_start_user();
> + writeb(ASPEED_FLASH_BASE, RDSR);
> + r = readb(ASPEED_FLASH_BASE);
> + spi_ctrl_stop_user();
> +
> + g_assert_cmphex(r & SR_WEL, ==, 0);
> + g_assert(!qtest_qom_get_bool
> + (global_qtest, "/machine/soc/fmc/ssi.0/child[0]", "write-enable"));
> +
> + spi_ctrl_start_user();
> + writeb(ASPEED_FLASH_BASE, WREN);
> + writeb(ASPEED_FLASH_BASE, RDSR);
> + r = readb(ASPEED_FLASH_BASE);
> + spi_ctrl_stop_user();
> +
> + g_assert_cmphex(r & SR_WEL, ==, SR_WEL);
> + g_assert(qtest_qom_get_bool
> + (global_qtest, "/machine/soc/fmc/ssi.0/child[0]", "write-enable"));
> +
> + spi_ctrl_start_user();
> + writeb(ASPEED_FLASH_BASE, WRDI);
> + writeb(ASPEED_FLASH_BASE, RDSR);
> + r = readb(ASPEED_FLASH_BASE);
> + spi_ctrl_stop_user();
> +
> + g_assert_cmphex(r & SR_WEL, ==, 0);
> + g_assert(!qtest_qom_get_bool
> + (global_qtest, "/machine/soc/fmc/ssi.0/child[0]", "write-enable"));
> +
> + flash_reset();
> +}
> +
> static char tmp_path[] = "/tmp/qtest.m25p80.XXXXXX";
>
> int main(int argc, char **argv)
> @@ -373,6 +415,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();
>
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 228357f1ea..a9904eba82 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -1423,3 +1423,27 @@ void qtest_client_inproc_recv(void *opaque, const char *str)
> g_string_append(qts->rx, str);
> return;
> }
> +
> +void qtest_qom_set_bool(QTestState *s, const char *path, const char *property,
> + bool value)
> +{
> + QDict *r;
> +
> + r = qtest_qmp(s, "{ 'execute': 'qom-set', 'arguments': "
> + "{ 'path': %s, 'property': %s, 'value': %i } }",
> + path, property, value);
> + qobject_unref(r);
> +}
> +
> +bool qtest_qom_get_bool(QTestState *s, const char *path, const char *property)
> +{
> + QDict *r;
> + bool b;
> +
> + r = qtest_qmp(s, "{ 'execute': 'qom-get', 'arguments': "
> + "{ 'path': %s, 'property': %s } }", path, property);
> + b = qdict_get_bool(r, "return");
> + qobject_unref(r);
> +
> + return b;
> +}
> diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
> index 4ab0cad326..94b187837d 100644
> --- a/tests/qtest/libqtest.h
> +++ b/tests/qtest/libqtest.h
> @@ -783,4 +783,26 @@ QTestState *qtest_inproc_init(QTestState **s, bool log, const char* arch,
> void (*send)(void*, const char*));
>
> void qtest_client_inproc_recv(void *opaque, const char *str);
> +
> +/**
> + * qtest_qom_set_bool:
> + * @s: QTestState instance to operate on.
> + * @path: Path to the property being set.
> + * @property: Property being set.
> + * @value: Value to set the property.
> + *
> + * Set the property with passed in value.
> + */
> +void qtest_qom_set_bool(QTestState *s, const char *path, const char *property,
> + bool value);
> +
> +/**
> + * qtest_qom_get_bool:
> + * @s: QTestState instance to operate on.
> + * @path: Path to the property being retrieved.
> + * @property: Property from where the value is being retrieved.
> + *
> + * Returns: Value retrieved from property.
> + */
> +bool qtest_qom_get_bool(QTestState *s, const char *path, const char *property);
> #endif
> --
> 2.30.2
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread