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

[Peter Delevoryas]

> On May 11, 2022, at 10:25 PM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> Hello Iris,
> 
> [ Fixing Thomas email ]
> 
> On 5/12/22 02:54, Iris Chen via wrote:
>> The write_enable latch property is not currently exposed.
>> This commit makes it a modifiable property using get/set methods.
>> Signed-off-by: Iris Chen <irischenlj@fb.com>
>> ---
>> Ran ./scripts/checkpatch.pl on the patch and added a description. Fixed 
>> comments regarding DEFINE_PROP_BOOL.
> 
> You should run 'make check' also.
> 
> With the removal of "qapi/visitor.h" and the property name change
> in the test,

Actually, I realized that this test will still not pass even with the
WEL => write-enable fix. The test is assuming that we can modifying
the property at runtime (after object realization), we would need
to specify the property with realized_set_allowed=true.

I would assume that Iris probably shouldn’t use realized_set_allowed=true,
so maybe the test should just verify reading the property
after it has been set by the WRITE ENABLE command, e.g. use qom-get
after writeb(ASPEED_FLASH_BASE, WREN).

> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> One comment below,
> 
>> hw/block/m25p80.c | 2 ++
>> tests/qtest/aspeed_smc-test.c | 23 +++++++++++++++++++++++
>> 2 files changed, 25 insertions(+)
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 430d1298a8..019beb5b78 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 */
>> @@ -1558,6 +1559,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_smc-test.c b/tests/qtest/aspeed_smc-test.c
>> index 87b40a0ef1..fcc156bc00 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,7 @@
>> */
>> enum {
>> JEDEC_READ = 0x9f,
>> + RDSR = 0x5,
>> BULK_ERASE = 0xc7,
>> READ = 0x03,
>> PP = 0x02,
>> @@ -348,6 +351,25 @@ 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}}");
> 
> Peter added qom_get_bool() and qom_set_bool() helpers in
> aspeed_gpio-test.c, it might be interesting to reuse.
> 
> There are similar ones in the npcm7xx tests, btw.
> 
> Thanks,
> 
> C.
> 
> 
> 
>> +
>> + 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, ==, SR_WEL);
>> +
>> + flash_reset();
>> +}
>> +
>> static char tmp_path[] = "/tmp/qtest.m25p80.XXXXXX";
>> int main(int argc, char **argv)
>> @@ -373,6 +395,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();