| Message ID | 20240527080231.1576609-15-jamin_lin@aspeedtech.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add AST2700 support | expand |
Hello Jamin On 5/27/24 10:02, Jamin Lin wrote: > AST2700 dram size calculation is not back compatible AST2600. > According to the DDR capacity hardware behavior, > if users write the data to address which is beyond the ram size, > it would write the data to address 0. > For example: > a. sdram base address "0x4 00000000" > b. sdram size is 1 GiB > The available address range is from "0x4 00000000" to "0x4 40000000". > If users write 0xdeadbeef to address "0x6 00000000", > the value of DRAM address 0 (base address 0x4 00000000) should be 0xdeadbeef. > > Add aspeed_soc_ast2700_dram_init to calculate the dram size and add > memory I/O whose address range is from max_ram_size - ram_size to max_ram_size > and its read/write handler to emulate DDR capacity hardware behavior. > > Signed-off-by: Troy Lee <troy_lee@aspeedtech.com> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> > --- > hw/arm/aspeed_ast27x0.c | 94 ++++++++++++++++++++++++++++++++++++- > include/hw/arm/aspeed_soc.h | 1 + > 2 files changed, 94 insertions(+), 1 deletion(-) > > diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c > index a3a03fc1ca..19380087fa 100644 > --- a/hw/arm/aspeed_ast27x0.c > +++ b/hw/arm/aspeed_ast27x0.c > @@ -20,6 +20,7 @@ > #include "sysemu/sysemu.h" > #include "hw/intc/arm_gicv3.h" > #include "qapi/qmp/qlist.h" > +#include "qemu/log.h" > > static const hwaddr aspeed_soc_ast2700_memmap[] = { > [ASPEED_DEV_SPI_BOOT] = 0x400000000, > @@ -191,6 +192,97 @@ static qemu_irq aspeed_soc_ast2700_get_irq(AspeedSoCState *s, int dev) > return qdev_get_gpio_in(a->intc.gic, sc->irqmap[dev]); > } > > +static uint64_t aspeed_ram_capacity_read(void *opaque, hwaddr addr, > + unsigned int size) > +{ > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: read @%" PRIx64 " out of ram size\n", > + __func__, addr); > + return 0; > +} > + > +static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, uint64_t data, > + unsigned int size) > +{ > + AspeedSoCState *s = ASPEED_SOC(opaque); > + uint32_t test_pattern = 0xdeadbeef; > + bool invalid_pattern = true; > + uint32_t *ram_ptr; > + int sz; > + > + ram_ptr = memory_region_get_ram_ptr(s->dram_mr); > + > + /* > + * Emulate ddr capacity hardware behavior. > + * If writes the test_pattern to address which is beyond the ram size, > + * it would write the test_pattern to address 0. > + */ > + for (sz = 4; sz > 0 ; sz--) { > + test_pattern = (test_pattern << 4) + sz; > + if (data == test_pattern) { > + ram_ptr[0] = test_pattern; > + invalid_pattern = false; > + break; > + } > + } > + > + if (invalid_pattern) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: write invalid pattern @%" PRIx64 > + " to addr @%" HWADDR_PRIx "]\n", > + __func__, data, addr); > + } > +} I would simplify with write transaction on the DRAM memory region of the SoC. For that, initialize a 'dram_as' on top of 'dram_mr' in aspeed_soc_ast2700_dram_init(): address_space_init(&s->dram_as, s->dram_mr, "dram"); Then, in aspeed_ram_capacity_write(), add : address_space_write(&s->dram_as, addr % ram_size, MEMTXATTRS_UNSPECIFIED, &data, size); and check returned error. It should be enough to detect the RAM size from FW. Thanks, C. > +static const MemoryRegionOps aspeed_ram_capacity_ops = { > + .read = aspeed_ram_capacity_read, > + .write = aspeed_ram_capacity_write, > + .endianness = DEVICE_LITTLE_ENDIAN, > + .valid = { > + .min_access_size = 1, > + .max_access_size = 8, > + }, > +}; > + > +/* > + * SDMC should be realized first to get correct RAM size and max size > + * values > + */ > +static bool aspeed_soc_ast2700_dram_init(DeviceState *dev, Error **errp) > +{ > + ram_addr_t ram_size, max_ram_size; > + Aspeed27x0SoCState *a = ASPEED27X0_SOC(dev); > + AspeedSoCState *s = ASPEED_SOC(dev); > + AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); > + > + ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size", > + &error_abort); > + max_ram_size = object_property_get_uint(OBJECT(&s->sdmc), "max-ram-size", > + &error_abort); > + > + memory_region_init(&s->dram_container, OBJECT(s), "ram-container", > + ram_size); > + memory_region_add_subregion(&s->dram_container, 0, s->dram_mr); > + > + /* > + * Add a memory region beyond the RAM region to emulate > + * ddr capacity hardware behavior. > + */ > + if (ram_size < max_ram_size) { > + memory_region_init_io(&a->dram_empty, OBJECT(s), > + &aspeed_ram_capacity_ops, s, > + "ram-empty", max_ram_size - ram_size); > + > + memory_region_add_subregion(s->memory, > + sc->memmap[ASPEED_DEV_SDRAM] + ram_size, > + &a->dram_empty); > + } > + > + memory_region_add_subregion(s->memory, > + sc->memmap[ASPEED_DEV_SDRAM], &s->dram_container); > + return true; > +} > + > static void aspeed_soc_ast2700_init(Object *obj) > { > Aspeed27x0SoCState *a = ASPEED27X0_SOC(obj); > @@ -461,7 +553,7 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp) > sc->memmap[ASPEED_DEV_SDMC]); > > /* RAM */ > - if (!aspeed_soc_dram_init(s, errp)) { > + if (!aspeed_soc_ast2700_dram_init(dev, errp)) { > return; > } > > diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h > index 9f177b6037..9dbf48f873 100644 > --- a/include/hw/arm/aspeed_soc.h > +++ b/include/hw/arm/aspeed_soc.h > @@ -127,6 +127,7 @@ struct Aspeed27x0SoCState { > > ARMCPU cpu[ASPEED_CPUS_NUM]; > AspeedINTCState intc; > + MemoryRegion dram_empty; > }; > > #define TYPE_ASPEED27X0_SOC "aspeed27x0-soc"
Hi Cedric, > From: Cédric Le Goater <clg@kaod.org>> > Hello Jamin > > On 5/27/24 10:02, Jamin Lin wrote: > > AST2700 dram size calculation is not back compatible AST2600. > > According to the DDR capacity hardware behavior, if users write the > > data to address which is beyond the ram size, it would write the data > > to address 0. > > For example: > > a. sdram base address "0x4 00000000" > > b. sdram size is 1 GiB > > The available address range is from "0x4 00000000" to "0x4 40000000". > > If users write 0xdeadbeef to address "0x6 00000000", the value of DRAM > > address 0 (base address 0x4 00000000) should be 0xdeadbeef. > > > > Add aspeed_soc_ast2700_dram_init to calculate the dram size and add > > memory I/O whose address range is from max_ram_size - ram_size to > > max_ram_size and its read/write handler to emulate DDR capacity hardware > behavior. > > > > Signed-off-by: Troy Lee <troy_lee@aspeedtech.com> > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> > > --- > > hw/arm/aspeed_ast27x0.c | 94 > ++++++++++++++++++++++++++++++++++++- > > include/hw/arm/aspeed_soc.h | 1 + > > 2 files changed, 94 insertions(+), 1 deletion(-) > > > > diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index > > a3a03fc1ca..19380087fa 100644 > > --- a/hw/arm/aspeed_ast27x0.c > > +++ b/hw/arm/aspeed_ast27x0.c > > @@ -20,6 +20,7 @@ > > #include "sysemu/sysemu.h" > > #include "hw/intc/arm_gicv3.h" > > #include "qapi/qmp/qlist.h" > > +#include "qemu/log.h" > > > > static const hwaddr aspeed_soc_ast2700_memmap[] = { > > [ASPEED_DEV_SPI_BOOT] = 0x400000000, @@ -191,6 +192,97 > @@ > > static qemu_irq aspeed_soc_ast2700_get_irq(AspeedSoCState *s, int dev) > > return qdev_get_gpio_in(a->intc.gic, sc->irqmap[dev]); > > } > > > > +static uint64_t aspeed_ram_capacity_read(void *opaque, hwaddr addr, > > + unsigned > int > > +size) { > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: read @%" PRIx64 " out of ram size\n", > > + __func__, addr); > > + return 0; > > +} > > + > > +static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, > uint64_t data, > > + unsigned int size) > { > > + AspeedSoCState *s = ASPEED_SOC(opaque); > > + uint32_t test_pattern = 0xdeadbeef; > > + bool invalid_pattern = true; > > + uint32_t *ram_ptr; > > + int sz; > > + > > + ram_ptr = memory_region_get_ram_ptr(s->dram_mr); > > + > > + /* > > + * Emulate ddr capacity hardware behavior. > > + * If writes the test_pattern to address which is beyond the ram size, > > + * it would write the test_pattern to address 0. > > + */ > > + for (sz = 4; sz > 0 ; sz--) { > > + test_pattern = (test_pattern << 4) + sz; > > + if (data == test_pattern) { > > + ram_ptr[0] = test_pattern; > > + invalid_pattern = false; > > + break; > > + } > > + } > > + > > + if (invalid_pattern) { > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: write invalid pattern @%" PRIx64 > > + " to addr @%" HWADDR_PRIx "]\n", > > + __func__, data, addr); > > + } > > +} > > > I would simplify with write transaction on the DRAM memory region of the > SoC. > > For that, initialize a 'dram_as' on top of 'dram_mr' in > aspeed_soc_ast2700_dram_init(): > > address_space_init(&s->dram_as, s->dram_mr, "dram"); > > Then, in aspeed_ram_capacity_write(), add : > > address_space_write(&s->dram_as, addr % ram_size, > MEMTXATTRS_UNSPECIFIED, > &data, size); > > and check returned error. > > It should be enough to detect the RAM size from FW. > > > Thanks, > > C. > Thanks for your suggestion and review. I changed to use address space APIs to write DRAM memory region(s->dram_mr). I have a question about aspeed_ram_capacity_write function implementation. Could you tell me which solution you prefer? Do you want to use solution 1? Thanks-Jamin Solution 1: static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, uint64_t data, unsigned int size) { AspeedSoCState *s = ASPEED_SOC(opaque); ram_addr_t ram_size; MemTxResult result; ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size", &error_abort); /* * Emulate ddr capacity hardware behavior. * If writes the data to the address which is beyond the ram size, * it would write the data to the "address % ram_size". */ result = address_space_write(&s->dram_as, addr % ram_size, MEMTXATTRS_UNSPECIFIED, &data, 4); if (result != MEMTX_OK) { qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM write failed, addr:0x%" HWADDR_PRIx ", data :0x%" PRIx64 "\n", __func__, addr % ram_size, data); } } We don't care the test pattern. If users write the data to the invalid address, the date will be written into the DRAM memory region at "addr % dram_size". Ex: dram size is 1G and the available address range is from "0x4 00000000" to "0x4 3FFFFFFF" Users write data(0x12345678) at invalid address "0x5 00000000" and the data would be written at address "0x4 00000000" => md 400000000 1 400000000: dbeef432 => mw 500000000 12345678 => md 400000000 1 400000000: 12345678 Solution 2: static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, uint64_t data, unsigned int size) { AspeedSoCState *s = ASPEED_SOC(opaque); uint32_t test_pattern = 0xdeadbeef; bool invalid_pattern = true; ram_addr_t ram_size; MemTxResult result; int sz; ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size", &error_abort); /* * Emulate ddr capacity hardware behavior. * If writes the test_pattern to the address which is beyond the ram size, * it would write the test_pattern to the "address % ram_size". */ for (sz = 4; sz > 0 ; sz--) { test_pattern = (test_pattern << 4) + sz; if (data == test_pattern) { result = address_space_write(&s->dram_as, addr % ram_size, MEMTXATTRS_UNSPECIFIED, &data, 4); if (result != MEMTX_OK) { qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM write failed, pattern:0x%" PRIx64 ", addr:0x%" HWADDR_PRIx "\n", __func__, data, addr % ram_size); return; } invalid_pattern = false; break; } } if (invalid_pattern) { qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM write invalid pattern:0x%" PRIx64 ", addr:0x%" HWADDR_PRIx "\n", __func__, data, addr); } } It check test patterns. If users write the invalid test pattern to the invalid address, the date will NOT be written into the DRAM memory region at "addr % dram_size". Ex: dram size is 1G and the available address range is from "0x4 00000000" to "0x4 3FFFFFFF" Users write invalid test pattern (0x12345678) at invalid address "0x5 00000000" and the data would not be written at address "0x4 00000000" Invalid test pattern => md 400000000 1 400000000: dbeef432 => mw 500000000 12345678 => md 400000000 1 400000000: dbeef432 Only valid pattern would be written at address "0x4 00000000" Pattern --> (0xdeadbeef << 4) + 4 => md 400000000 1 400000000: dbeef432 => mw 500000000 deadbeef4 => md 400000000 1 400000000: eadbeef4 > > > > > +static const MemoryRegionOps aspeed_ram_capacity_ops = { > > + .read = aspeed_ram_capacity_read, > > + .write = aspeed_ram_capacity_write, > > + .endianness = DEVICE_LITTLE_ENDIAN, > > + .valid = { > > + .min_access_size = 1, > > + .max_access_size = 8, > > + }, > > +}; > > + > > +/* > > + * SDMC should be realized first to get correct RAM size and max size > > + * values > > + */ > > +static bool aspeed_soc_ast2700_dram_init(DeviceState *dev, Error > > +**errp) { > > + ram_addr_t ram_size, max_ram_size; > > + Aspeed27x0SoCState *a = ASPEED27X0_SOC(dev); > > + AspeedSoCState *s = ASPEED_SOC(dev); > > + AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); > > + > > + ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size", > > + &error_abort); > > + max_ram_size = object_property_get_uint(OBJECT(&s->sdmc), > "max-ram-size", > > + &error_abort); > > + > > + memory_region_init(&s->dram_container, OBJECT(s), "ram-container", > > + ram_size); > > + memory_region_add_subregion(&s->dram_container, 0, s->dram_mr); > > + > > + /* > > + * Add a memory region beyond the RAM region to emulate > > + * ddr capacity hardware behavior. > > + */ > > + if (ram_size < max_ram_size) { > > + memory_region_init_io(&a->dram_empty, OBJECT(s), > > + &aspeed_ram_capacity_ops, s, > > + "ram-empty", max_ram_size - > ram_size); > > + > > + memory_region_add_subregion(s->memory, > > + > sc->memmap[ASPEED_DEV_SDRAM] + ram_size, > > + &a->dram_empty); > > + } > > + > > + memory_region_add_subregion(s->memory, > > + sc->memmap[ASPEED_DEV_SDRAM], > &s->dram_container); > > + return true; > > +} > > + > > static void aspeed_soc_ast2700_init(Object *obj) > > { > > Aspeed27x0SoCState *a = ASPEED27X0_SOC(obj); @@ -461,7 +553,7 > @@ > > static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp) > > sc->memmap[ASPEED_DEV_SDMC]); > > > > /* RAM */ > > - if (!aspeed_soc_dram_init(s, errp)) { > > + if (!aspeed_soc_ast2700_dram_init(dev, errp)) { > > return; > > } > > > > diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h > > index 9f177b6037..9dbf48f873 100644 > > --- a/include/hw/arm/aspeed_soc.h > > +++ b/include/hw/arm/aspeed_soc.h > > @@ -127,6 +127,7 @@ struct Aspeed27x0SoCState { > > > > ARMCPU cpu[ASPEED_CPUS_NUM]; > > AspeedINTCState intc; > > + MemoryRegion dram_empty; > > }; > > > > #define TYPE_ASPEED27X0_SOC "aspeed27x0-soc"
On 5/30/24 09:42, Jamin Lin wrote: > Hi Cedric, >> From: Cédric Le Goater <clg@kaod.org>> >> Hello Jamin >> >> On 5/27/24 10:02, Jamin Lin wrote: >>> AST2700 dram size calculation is not back compatible AST2600. >>> According to the DDR capacity hardware behavior, if users write the >>> data to address which is beyond the ram size, it would write the data >>> to address 0. >>> For example: >>> a. sdram base address "0x4 00000000" >>> b. sdram size is 1 GiB >>> The available address range is from "0x4 00000000" to "0x4 40000000". >>> If users write 0xdeadbeef to address "0x6 00000000", the value of DRAM >>> address 0 (base address 0x4 00000000) should be 0xdeadbeef. >>> >>> Add aspeed_soc_ast2700_dram_init to calculate the dram size and add >>> memory I/O whose address range is from max_ram_size - ram_size to >>> max_ram_size and its read/write handler to emulate DDR capacity hardware >> behavior. >>> >>> Signed-off-by: Troy Lee <troy_lee@aspeedtech.com> >>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> >>> --- >>> hw/arm/aspeed_ast27x0.c | 94 >> ++++++++++++++++++++++++++++++++++++- >>> include/hw/arm/aspeed_soc.h | 1 + >>> 2 files changed, 94 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index >>> a3a03fc1ca..19380087fa 100644 >>> --- a/hw/arm/aspeed_ast27x0.c >>> +++ b/hw/arm/aspeed_ast27x0.c >>> @@ -20,6 +20,7 @@ >>> #include "sysemu/sysemu.h" >>> #include "hw/intc/arm_gicv3.h" >>> #include "qapi/qmp/qlist.h" >>> +#include "qemu/log.h" >>> >>> static const hwaddr aspeed_soc_ast2700_memmap[] = { >>> [ASPEED_DEV_SPI_BOOT] = 0x400000000, @@ -191,6 +192,97 >> @@ >>> static qemu_irq aspeed_soc_ast2700_get_irq(AspeedSoCState *s, int dev) >>> return qdev_get_gpio_in(a->intc.gic, sc->irqmap[dev]); >>> } >>> >>> +static uint64_t aspeed_ram_capacity_read(void *opaque, hwaddr addr, >>> + unsigned >> int >>> +size) { >>> + qemu_log_mask(LOG_GUEST_ERROR, >>> + "%s: read @%" PRIx64 " out of ram size\n", >>> + __func__, addr); >>> + return 0; >>> +} >>> + >>> +static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, >> uint64_t data, >>> + unsigned int size) >> { >>> + AspeedSoCState *s = ASPEED_SOC(opaque); >>> + uint32_t test_pattern = 0xdeadbeef; >>> + bool invalid_pattern = true; >>> + uint32_t *ram_ptr; >>> + int sz; >>> + >>> + ram_ptr = memory_region_get_ram_ptr(s->dram_mr); >>> + >>> + /* >>> + * Emulate ddr capacity hardware behavior. >>> + * If writes the test_pattern to address which is beyond the ram size, >>> + * it would write the test_pattern to address 0. >>> + */ >>> + for (sz = 4; sz > 0 ; sz--) { >>> + test_pattern = (test_pattern << 4) + sz; >>> + if (data == test_pattern) { >>> + ram_ptr[0] = test_pattern; >>> + invalid_pattern = false; >>> + break; >>> + } >>> + } >>> + >>> + if (invalid_pattern) { >>> + qemu_log_mask(LOG_GUEST_ERROR, >>> + "%s: write invalid pattern @%" PRIx64 >>> + " to addr @%" HWADDR_PRIx "]\n", >>> + __func__, data, addr); >>> + } >>> +} >> >> >> I would simplify with write transaction on the DRAM memory region of the >> SoC. >> >> For that, initialize a 'dram_as' on top of 'dram_mr' in >> aspeed_soc_ast2700_dram_init(): >> >> address_space_init(&s->dram_as, s->dram_mr, "dram"); >> >> Then, in aspeed_ram_capacity_write(), add : >> >> address_space_write(&s->dram_as, addr % ram_size, >> MEMTXATTRS_UNSPECIFIED, >> &data, size); >> >> and check returned error. >> >> It should be enough to detect the RAM size from FW. >> >> >> Thanks, >> >> C. >> > Thanks for your suggestion and review. > I changed to use address space APIs to write DRAM memory region(s->dram_mr). > I have a question about aspeed_ram_capacity_write function implementation. > Could you tell me which solution you prefer? Do you want to use solution 1? I prefer solution 1 because no assumption is made on what software does. It simply implements the wraparound HW does on RAM accesses. Thanks, C. > Thanks-Jamin > > Solution 1: > static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, uint64_t data, > unsigned int size) > { > AspeedSoCState *s = ASPEED_SOC(opaque); > ram_addr_t ram_size; > MemTxResult result; > > ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size", > &error_abort); > > /* > * Emulate ddr capacity hardware behavior. > * If writes the data to the address which is beyond the ram size, > * it would write the data to the "address % ram_size". > */ > result = address_space_write(&s->dram_as, addr % ram_size, > MEMTXATTRS_UNSPECIFIED, &data, 4); > if (result != MEMTX_OK) { > qemu_log_mask(LOG_GUEST_ERROR, > "%s: DRAM write failed, addr:0x%" HWADDR_PRIx > ", data :0x%" PRIx64 "\n", > __func__, addr % ram_size, data); > } > } > We don't care the test pattern. If users write the data to the invalid address, the date will be written into the DRAM memory region at "addr % dram_size". > Ex: dram size is 1G and the available address range is from "0x4 00000000" to "0x4 3FFFFFFF" > > Users write data(0x12345678) at invalid address "0x5 00000000" and the data would be written at address "0x4 00000000" > => md 400000000 1 > 400000000: dbeef432 > => mw 500000000 12345678 > => md 400000000 1 > 400000000: 12345678 > > Solution 2: > static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, uint64_t data, > unsigned int size) > { > AspeedSoCState *s = ASPEED_SOC(opaque); > uint32_t test_pattern = 0xdeadbeef; > bool invalid_pattern = true; > ram_addr_t ram_size; > MemTxResult result; > int sz; > > ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size", > &error_abort); > > /* > * Emulate ddr capacity hardware behavior. > * If writes the test_pattern to the address which is beyond the ram size, > * it would write the test_pattern to the "address % ram_size". > */ > for (sz = 4; sz > 0 ; sz--) { > test_pattern = (test_pattern << 4) + sz; > if (data == test_pattern) { > result = address_space_write(&s->dram_as, addr % ram_size, > MEMTXATTRS_UNSPECIFIED, &data, 4); > if (result != MEMTX_OK) { > qemu_log_mask(LOG_GUEST_ERROR, > "%s: DRAM write failed, pattern:0x%" PRIx64 > ", addr:0x%" HWADDR_PRIx "\n", > __func__, data, addr % ram_size); > return; > } > invalid_pattern = false; > break; > } > } > > if (invalid_pattern) { > qemu_log_mask(LOG_GUEST_ERROR, > "%s: DRAM write invalid pattern:0x%" PRIx64 > ", addr:0x%" HWADDR_PRIx "\n", > __func__, data, addr); > } > } > It check test patterns. If users write the invalid test pattern to the invalid address, the date will NOT be written into the DRAM memory region at "addr % dram_size". > Ex: dram size is 1G and the available address range is from "0x4 00000000" to "0x4 3FFFFFFF" > > Users write invalid test pattern (0x12345678) at invalid address "0x5 00000000" and the data would not be written at address "0x4 00000000" > > Invalid test pattern > => md 400000000 1 > 400000000: dbeef432 > => mw 500000000 12345678 > => md 400000000 1 > 400000000: dbeef432 > > Only valid pattern would be written at address "0x4 00000000" > Pattern --> (0xdeadbeef << 4) + 4 > => md 400000000 1 > 400000000: dbeef432 > => mw 500000000 deadbeef4 > => md 400000000 1 > 400000000: eadbeef4 > >> >> >> >>> +static const MemoryRegionOps aspeed_ram_capacity_ops = { >>> + .read = aspeed_ram_capacity_read, >>> + .write = aspeed_ram_capacity_write, >>> + .endianness = DEVICE_LITTLE_ENDIAN, >>> + .valid = { >>> + .min_access_size = 1, >>> + .max_access_size = 8, >>> + }, >>> +}; >>> + >>> +/* >>> + * SDMC should be realized first to get correct RAM size and max size >>> + * values >>> + */ >>> +static bool aspeed_soc_ast2700_dram_init(DeviceState *dev, Error >>> +**errp) { >>> + ram_addr_t ram_size, max_ram_size; >>> + Aspeed27x0SoCState *a = ASPEED27X0_SOC(dev); >>> + AspeedSoCState *s = ASPEED_SOC(dev); >>> + AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); >>> + >>> + ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size", >>> + &error_abort); >>> + max_ram_size = object_property_get_uint(OBJECT(&s->sdmc), >> "max-ram-size", >>> + &error_abort); >>> + >>> + memory_region_init(&s->dram_container, OBJECT(s), "ram-container", >>> + ram_size); >>> + memory_region_add_subregion(&s->dram_container, 0, s->dram_mr); >>> + >>> + /* >>> + * Add a memory region beyond the RAM region to emulate >>> + * ddr capacity hardware behavior. >>> + */ >>> + if (ram_size < max_ram_size) { >>> + memory_region_init_io(&a->dram_empty, OBJECT(s), >>> + &aspeed_ram_capacity_ops, s, >>> + "ram-empty", max_ram_size - >> ram_size); >>> + >>> + memory_region_add_subregion(s->memory, >>> + >> sc->memmap[ASPEED_DEV_SDRAM] + ram_size, >>> + &a->dram_empty); >>> + } >>> + >>> + memory_region_add_subregion(s->memory, >>> + sc->memmap[ASPEED_DEV_SDRAM], >> &s->dram_container); >>> + return true; >>> +} >>> + >>> static void aspeed_soc_ast2700_init(Object *obj) >>> { >>> Aspeed27x0SoCState *a = ASPEED27X0_SOC(obj); @@ -461,7 +553,7 >> @@ >>> static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp) >>> sc->memmap[ASPEED_DEV_SDMC]); >>> >>> /* RAM */ >>> - if (!aspeed_soc_dram_init(s, errp)) { >>> + if (!aspeed_soc_ast2700_dram_init(dev, errp)) { >>> return; >>> } >>> >>> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h >>> index 9f177b6037..9dbf48f873 100644 >>> --- a/include/hw/arm/aspeed_soc.h >>> +++ b/include/hw/arm/aspeed_soc.h >>> @@ -127,6 +127,7 @@ struct Aspeed27x0SoCState { >>> >>> ARMCPU cpu[ASPEED_CPUS_NUM]; >>> AspeedINTCState intc; >>> + MemoryRegion dram_empty; >>> }; >>> >>> #define TYPE_ASPEED27X0_SOC "aspeed27x0-soc" >
Hi Cedric, > From: Cédric Le Goater <clg@kaod.org> > On 5/30/24 09:42, Jamin Lin wrote: > > Hi Cedric, > >> From: Cédric Le Goater <clg@kaod.org>> Hello Jamin > >> > >> On 5/27/24 10:02, Jamin Lin wrote: > >>> AST2700 dram size calculation is not back compatible AST2600. > >>> According to the DDR capacity hardware behavior, if users write the > >>> data to address which is beyond the ram size, it would write the > >>> data to address 0. > >>> For example: > >>> a. sdram base address "0x4 00000000" > >>> b. sdram size is 1 GiB > >>> The available address range is from "0x4 00000000" to "0x4 40000000". > >>> If users write 0xdeadbeef to address "0x6 00000000", the value of > >>> DRAM address 0 (base address 0x4 00000000) should be 0xdeadbeef. > >>> > >>> Add aspeed_soc_ast2700_dram_init to calculate the dram size and add > >>> memory I/O whose address range is from max_ram_size - ram_size to > >>> max_ram_size and its read/write handler to emulate DDR capacity > >>> hardware > >> behavior. > >>> > >>> Signed-off-by: Troy Lee <troy_lee@aspeedtech.com> > >>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> > >>> --- > >>> hw/arm/aspeed_ast27x0.c | 94 > >> ++++++++++++++++++++++++++++++++++++- > >>> include/hw/arm/aspeed_soc.h | 1 + > >>> 2 files changed, 94 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index > >>> a3a03fc1ca..19380087fa 100644 > >>> --- a/hw/arm/aspeed_ast27x0.c > >>> +++ b/hw/arm/aspeed_ast27x0.c > >>> @@ -20,6 +20,7 @@ > >>> #include "sysemu/sysemu.h" > >>> #include "hw/intc/arm_gicv3.h" > >>> #include "qapi/qmp/qlist.h" > >>> +#include "qemu/log.h" > >>> > >>> static const hwaddr aspeed_soc_ast2700_memmap[] = { > >>> [ASPEED_DEV_SPI_BOOT] = 0x400000000, @@ -191,6 > +192,97 > >> @@ > >>> static qemu_irq aspeed_soc_ast2700_get_irq(AspeedSoCState *s, int dev) > >>> return qdev_get_gpio_in(a->intc.gic, sc->irqmap[dev]); > >>> } > >>> > >>> +static uint64_t aspeed_ram_capacity_read(void *opaque, hwaddr addr, > >>> + unsigned > >> int > >>> +size) { > >>> + qemu_log_mask(LOG_GUEST_ERROR, > >>> + "%s: read @%" PRIx64 " out of ram size\n", > >>> + __func__, addr); > >>> + return 0; > >>> +} > >>> + > >>> +static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, > >> uint64_t data, > >>> + unsigned int > size) > >> { > >>> + AspeedSoCState *s = ASPEED_SOC(opaque); > >>> + uint32_t test_pattern = 0xdeadbeef; > >>> + bool invalid_pattern = true; > >>> + uint32_t *ram_ptr; > >>> + int sz; > >>> + > >>> + ram_ptr = memory_region_get_ram_ptr(s->dram_mr); > >>> + > >>> + /* > >>> + * Emulate ddr capacity hardware behavior. > >>> + * If writes the test_pattern to address which is beyond the ram size, > >>> + * it would write the test_pattern to address 0. > >>> + */ > >>> + for (sz = 4; sz > 0 ; sz--) { > >>> + test_pattern = (test_pattern << 4) + sz; > >>> + if (data == test_pattern) { > >>> + ram_ptr[0] = test_pattern; > >>> + invalid_pattern = false; > >>> + break; > >>> + } > >>> + } > >>> + > >>> + if (invalid_pattern) { > >>> + qemu_log_mask(LOG_GUEST_ERROR, > >>> + "%s: write invalid pattern @%" PRIx64 > >>> + " to addr @%" HWADDR_PRIx "]\n", > >>> + __func__, data, addr); > >>> + } > >>> +} > >> > >> > >> I would simplify with write transaction on the DRAM memory region of > >> the SoC. > >> > >> For that, initialize a 'dram_as' on top of 'dram_mr' in > >> aspeed_soc_ast2700_dram_init(): > >> > >> address_space_init(&s->dram_as, s->dram_mr, "dram"); > >> > >> Then, in aspeed_ram_capacity_write(), add : > >> > >> address_space_write(&s->dram_as, addr % ram_size, > >> MEMTXATTRS_UNSPECIFIED, > >> &data, size); > >> > >> and check returned error. > >> > >> It should be enough to detect the RAM size from FW. > >> > >> > >> Thanks, > >> > >> C. > >> > > Thanks for your suggestion and review. > > I changed to use address space APIs to write DRAM memory > region(s->dram_mr). > > I have a question about aspeed_ram_capacity_write function > implementation. > > Could you tell me which solution you prefer? Do you want to use solution 1? > > I prefer solution 1 because no assumption is made on what software does. > It simply implements the wraparound HW does on RAM accesses. > > Thanks, > > C. Got it. I will update it in v5 patch (solution 1) Thanks for your help and suggestion. Jamin > > > > > Thanks-Jamin > > > > Solution 1: > > static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, uint64_t > data, > > unsigned int > size) { > > AspeedSoCState *s = ASPEED_SOC(opaque); > > ram_addr_t ram_size; > > MemTxResult result; > > > > ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size", > > &error_abort); > > > > /* > > * Emulate ddr capacity hardware behavior. > > * If writes the data to the address which is beyond the ram size, > > * it would write the data to the "address % ram_size". > > */ > > result = address_space_write(&s->dram_as, addr % ram_size, > > MEMTXATTRS_UNSPECIFIED, > &data, 4); > > if (result != MEMTX_OK) { > > qemu_log_mask(LOG_GUEST_ERROR, > > "%s: DRAM write failed, addr:0x%" > HWADDR_PRIx > > ", data :0x%" PRIx64 "\n", > > __func__, addr % ram_size, data); > > } > > } > > We don't care the test pattern. If users write the data to the invalid address, > the date will be written into the DRAM memory region at "addr % dram_size". > > Ex: dram size is 1G and the available address range is from "0x4 00000000" > to "0x4 3FFFFFFF" > > > > Users write data(0x12345678) at invalid address "0x5 00000000" and the > data would be written at address "0x4 00000000" > > => md 400000000 1 > > 400000000: dbeef432 > > => mw 500000000 12345678 > > => md 400000000 1 > > 400000000: 12345678 > > > > Solution 2: > > static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, uint64_t > data, > > unsigned int > size) { > > AspeedSoCState *s = ASPEED_SOC(opaque); > > uint32_t test_pattern = 0xdeadbeef; > > bool invalid_pattern = true; > > ram_addr_t ram_size; > > MemTxResult result; > > int sz; > > > > ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size", > > &error_abort); > > > > /* > > * Emulate ddr capacity hardware behavior. > > * If writes the test_pattern to the address which is beyond the ram > size, > > * it would write the test_pattern to the "address % ram_size". > > */ > > for (sz = 4; sz > 0 ; sz--) { > > test_pattern = (test_pattern << 4) + sz; > > if (data == test_pattern) { > > result = address_space_write(&s->dram_as, addr % > ram_size, > > > MEMTXATTRS_UNSPECIFIED, &data, 4); > > if (result != MEMTX_OK) { > > qemu_log_mask(LOG_GUEST_ERROR, > > "%s: DRAM write failed, pattern:0x%" > PRIx64 > > ", addr:0x%" HWADDR_PRIx "\n", > > __func__, data, addr % ram_size); > > return; > > } > > invalid_pattern = false; > > break; > > } > > } > > > > if (invalid_pattern) { > > qemu_log_mask(LOG_GUEST_ERROR, > > "%s: DRAM write invalid pattern:0x%" PRIx64 > > ", addr:0x%" HWADDR_PRIx "\n", > > __func__, data, addr); > > } > > } > > It check test patterns. If users write the invalid test pattern to the invalid > address, the date will NOT be written into the DRAM memory region at "addr > % dram_size". > > Ex: dram size is 1G and the available address range is from "0x4 00000000" > to "0x4 3FFFFFFF" > > > > Users write invalid test pattern (0x12345678) at invalid address "0x5 > 00000000" and the data would not be written at address "0x4 00000000" > > > > Invalid test pattern > > => md 400000000 1 > > 400000000: dbeef432 > > => mw 500000000 12345678 > > => md 400000000 1 > > 400000000: dbeef432 > > > > Only valid pattern would be written at address "0x4 00000000" > > Pattern --> (0xdeadbeef << 4) + 4 > > => md 400000000 1 > > 400000000: dbeef432 > > => mw 500000000 deadbeef4 > > => md 400000000 1 > > 400000000: eadbeef4 > > > >> > >> > >> > >>> +static const MemoryRegionOps aspeed_ram_capacity_ops = { > >>> + .read = aspeed_ram_capacity_read, > >>> + .write = aspeed_ram_capacity_write, > >>> + .endianness = DEVICE_LITTLE_ENDIAN, > >>> + .valid = { > >>> + .min_access_size = 1, > >>> + .max_access_size = 8, > >>> + }, > >>> +}; > >>> + > >>> +/* > >>> + * SDMC should be realized first to get correct RAM size and max > >>> +size > >>> + * values > >>> + */ > >>> +static bool aspeed_soc_ast2700_dram_init(DeviceState *dev, Error > >>> +**errp) { > >>> + ram_addr_t ram_size, max_ram_size; > >>> + Aspeed27x0SoCState *a = ASPEED27X0_SOC(dev); > >>> + AspeedSoCState *s = ASPEED_SOC(dev); > >>> + AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); > >>> + > >>> + ram_size = object_property_get_uint(OBJECT(&s->sdmc), > "ram-size", > >>> + &error_abort); > >>> + max_ram_size = object_property_get_uint(OBJECT(&s->sdmc), > >> "max-ram-size", > >>> + &error_abort); > >>> + > >>> + memory_region_init(&s->dram_container, OBJECT(s), > "ram-container", > >>> + ram_size); > >>> + memory_region_add_subregion(&s->dram_container, 0, > s->dram_mr); > >>> + > >>> + /* > >>> + * Add a memory region beyond the RAM region to emulate > >>> + * ddr capacity hardware behavior. > >>> + */ > >>> + if (ram_size < max_ram_size) { > >>> + memory_region_init_io(&a->dram_empty, OBJECT(s), > >>> + &aspeed_ram_capacity_ops, s, > >>> + "ram-empty", max_ram_size - > >> ram_size); > >>> + > >>> + memory_region_add_subregion(s->memory, > >>> + > >> sc->memmap[ASPEED_DEV_SDRAM] + ram_size, > >>> + &a->dram_empty); > >>> + } > >>> + > >>> + memory_region_add_subregion(s->memory, > >>> + sc->memmap[ASPEED_DEV_SDRAM], > >> &s->dram_container); > >>> + return true; > >>> +} > >>> + > >>> static void aspeed_soc_ast2700_init(Object *obj) > >>> { > >>> Aspeed27x0SoCState *a = ASPEED27X0_SOC(obj); @@ -461,7 > +553,7 > >> @@ > >>> static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp) > >>> sc->memmap[ASPEED_DEV_SDMC]); > >>> > >>> /* RAM */ > >>> - if (!aspeed_soc_dram_init(s, errp)) { > >>> + if (!aspeed_soc_ast2700_dram_init(dev, errp)) { > >>> return; > >>> } > >>> > >>> diff --git a/include/hw/arm/aspeed_soc.h > >>> b/include/hw/arm/aspeed_soc.h index 9f177b6037..9dbf48f873 100644 > >>> --- a/include/hw/arm/aspeed_soc.h > >>> +++ b/include/hw/arm/aspeed_soc.h > >>> @@ -127,6 +127,7 @@ struct Aspeed27x0SoCState { > >>> > >>> ARMCPU cpu[ASPEED_CPUS_NUM]; > >>> AspeedINTCState intc; > >>> + MemoryRegion dram_empty; > >>> }; > >>> > >>> #define TYPE_ASPEED27X0_SOC "aspeed27x0-soc" > >
diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index a3a03fc1ca..19380087fa 100644 --- a/hw/arm/aspeed_ast27x0.c +++ b/hw/arm/aspeed_ast27x0.c @@ -20,6 +20,7 @@ #include "sysemu/sysemu.h" #include "hw/intc/arm_gicv3.h" #include "qapi/qmp/qlist.h" +#include "qemu/log.h" static const hwaddr aspeed_soc_ast2700_memmap[] = { [ASPEED_DEV_SPI_BOOT] = 0x400000000, @@ -191,6 +192,97 @@ static qemu_irq aspeed_soc_ast2700_get_irq(AspeedSoCState *s, int dev) return qdev_get_gpio_in(a->intc.gic, sc->irqmap[dev]); } +static uint64_t aspeed_ram_capacity_read(void *opaque, hwaddr addr, + unsigned int size) +{ + qemu_log_mask(LOG_GUEST_ERROR, + "%s: read @%" PRIx64 " out of ram size\n", + __func__, addr); + return 0; +} + +static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, uint64_t data, + unsigned int size) +{ + AspeedSoCState *s = ASPEED_SOC(opaque); + uint32_t test_pattern = 0xdeadbeef; + bool invalid_pattern = true; + uint32_t *ram_ptr; + int sz; + + ram_ptr = memory_region_get_ram_ptr(s->dram_mr); + + /* + * Emulate ddr capacity hardware behavior. + * If writes the test_pattern to address which is beyond the ram size, + * it would write the test_pattern to address 0. + */ + for (sz = 4; sz > 0 ; sz--) { + test_pattern = (test_pattern << 4) + sz; + if (data == test_pattern) { + ram_ptr[0] = test_pattern; + invalid_pattern = false; + break; + } + } + + if (invalid_pattern) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: write invalid pattern @%" PRIx64 + " to addr @%" HWADDR_PRIx "]\n", + __func__, data, addr); + } +} + +static const MemoryRegionOps aspeed_ram_capacity_ops = { + .read = aspeed_ram_capacity_read, + .write = aspeed_ram_capacity_write, + .endianness = DEVICE_LITTLE_ENDIAN, + .valid = { + .min_access_size = 1, + .max_access_size = 8, + }, +}; + +/* + * SDMC should be realized first to get correct RAM size and max size + * values + */ +static bool aspeed_soc_ast2700_dram_init(DeviceState *dev, Error **errp) +{ + ram_addr_t ram_size, max_ram_size; + Aspeed27x0SoCState *a = ASPEED27X0_SOC(dev); + AspeedSoCState *s = ASPEED_SOC(dev); + AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); + + ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size", + &error_abort); + max_ram_size = object_property_get_uint(OBJECT(&s->sdmc), "max-ram-size", + &error_abort); + + memory_region_init(&s->dram_container, OBJECT(s), "ram-container", + ram_size); + memory_region_add_subregion(&s->dram_container, 0, s->dram_mr); + + /* + * Add a memory region beyond the RAM region to emulate + * ddr capacity hardware behavior. + */ + if (ram_size < max_ram_size) { + memory_region_init_io(&a->dram_empty, OBJECT(s), + &aspeed_ram_capacity_ops, s, + "ram-empty", max_ram_size - ram_size); + + memory_region_add_subregion(s->memory, + sc->memmap[ASPEED_DEV_SDRAM] + ram_size, + &a->dram_empty); + } + + memory_region_add_subregion(s->memory, + sc->memmap[ASPEED_DEV_SDRAM], &s->dram_container); + return true; +} + static void aspeed_soc_ast2700_init(Object *obj) { Aspeed27x0SoCState *a = ASPEED27X0_SOC(obj); @@ -461,7 +553,7 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp) sc->memmap[ASPEED_DEV_SDMC]); /* RAM */ - if (!aspeed_soc_dram_init(s, errp)) { + if (!aspeed_soc_ast2700_dram_init(dev, errp)) { return; } diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h index 9f177b6037..9dbf48f873 100644 --- a/include/hw/arm/aspeed_soc.h +++ b/include/hw/arm/aspeed_soc.h @@ -127,6 +127,7 @@ struct Aspeed27x0SoCState { ARMCPU cpu[ASPEED_CPUS_NUM]; AspeedINTCState intc; + MemoryRegion dram_empty; }; #define TYPE_ASPEED27X0_SOC "aspeed27x0-soc"