[PATCH v2 00/11] hw/arm/aspeed_ast10x0: Map more peripherals & few more fixes

[PATCH v2 00/11] hw/arm/aspeed_ast10x0: Map more peripherals & few more fixes

[Philippe Mathieu-Daudé]

Missing review: #3

Since v1:
- Fixed typo (Peter)
- Link HACE with sram insted of secsram
- Split patch #2 in 2, reworded it
- Cover more WDT registers (new patch)

---

Trying to fix some bugs triggered running Zephyr.

Still 2 bugs:

1/
uart:~$ sensor get SYSCLK
[00:00:23.592,000] <err> os: ***** USAGE FAULT *****
[00:00:23.593,000] <err> os:   Illegal use of the EPSR
[00:00:23.593,000] <err> os: r0/a1:  0x00033448  r1/a2:  0x00000000  r2/a3:  0x00047f50
[00:00:23.593,000] <err> os: r3/a4:  0x00000000 r12/ip:  0x00000000 r14/lr:  0x00000fbd
[00:00:23.593,000] <err> os:  xpsr:  0x60000000
[00:00:23.593,000] <err> os: Faulting instruction address (r15/pc): 0x00000000
[00:00:23.593,000] <err> os: >>> ZEPHYR FATAL ERROR 0: CPU exception on CPU 0
[00:00:23.594,000] <err> os: Current thread: 0x38248 (shell_uart)
[00:00:23.601,000] <err> os: Halting system

2/
uart:~$ mcuboot
[00:01:04.990,000] <err> os: ***** BUS FAULT *****
[00:01:04.990,000] <err> os:   Instruction bus error
[00:01:04.991,000] <err> os: r0/a1:  0x00000000  r1/a2:  0x000ffff0  r2/a3:  0x00047ef0
[00:01:04.991,000] <err> os: r3/a4:  0x00000010 r12/ip:  0x6df7ecb5 r14/lr:  0x000188ed
[00:01:04.991,000] <err> os:  xpsr:  0x61000000
[00:01:04.991,000] <err> os: Faulting instruction address (r15/pc): 0x6df7ecb4
[00:01:04.991,000] <err> os: >>> ZEPHYR FATAL ERROR 0: CPU exception on CPU 0
[00:01:04.991,000] <err> os: Current thread: 0x38248 (shell_uart)
[00:01:04.994,000] <err> os: Halting system

----------------
IN:
PMSA MPU lookup for reading at 0x0001d400 mmu_idx 65 -> Hit (prot rwx)
0x0001d5a2:  6869       ldr      r1, [r5, #4]
0x0001d5a4:  4421       add      r1, r4
0x0001d5a6:  6883       ldr      r3, [r0, #8]
0x0001d5a8:  681c       ldr      r4, [r3]
0x0001d5aa:  463a       mov      r2, r7
0x0001d5ac:  4633       mov      r3, r6
0x0001d5ae:  46a4       mov      ip, r4
0x0001d5b0:  e8bd 41f0  pop.w    {r4, r5, r6, r7, r8, lr}
0x0001d5b4:  4760       bx       ip

PMSA MPU lookup for reading at 0x00000008 mmu_idx 65 -> Hit (prot rwx)
PMSA MPU lookup for execute at 0x6df7ecb4 mmu_idx 65 -> Hit (prot rwx)
Taking exception 3 [Prefetch Abort] on CPU 0
...at fault address 0x6df7ecb4
...with CFSR.IBUSERR
PMSA MPU lookup for writing at 0x00047ec8 mmu_idx 65 -> Hit (prot rwx)
PMSA MPU lookup for writing at 0x00047ecc mmu_idx 65 -> Hit (prot rwx)
PMSA MPU lookup for writing at 0x00047ed0 mmu_idx 65 -> Hit (prot rwx)
PMSA MPU lookup for writing at 0x00047ed4 mmu_idx 65 -> Hit (prot rwx)
PMSA MPU lookup for writing at 0x00047ed8 mmu_idx 65 -> Hit (prot rwx)
PMSA MPU lookup for writing at 0x00047edc mmu_idx 65 -> Hit (prot rwx)
PMSA MPU lookup for writing at 0x00047ee0 mmu_idx 65 -> Hit (prot rwx)
PMSA MPU lookup for writing at 0x00047ee4 mmu_idx 65 -> Hit (prot rwx)
...taking pending nonsecure exception 5
...loading from element 5 of non-secure vector table at 0x14
...loaded new PC 0xa0cd
----------------

Philippe Mathieu-Daudé (11):
  hw/watchdog/wdt_aspeed: Rename MMIO region size as 'iosize'
  hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers
  hw/watchdog/wdt_aspeed: Log unimplemented registers as UNIMP level
  hw/arm/aspeed: Use the IEC binary prefix definitions
  hw/misc/aspeed_hace: Do not crash if address_space_map() failed
  hw/arm/aspeed_ast10x0: Add various unimplemented peripherals
  hw/arm/aspeed_ast10x0: Map I3C peripheral
  hw/arm/aspeed_ast10x0: Map the secure SRAM
  hw/arm/aspeed_ast10x0: Map HACE peripheral
  hw/arm/aspeed_ast10x0: Add TODO comment to use Cortex-M4F
  tests/avocado: Test Aspeed Zephyr SDK v00.01.08 on AST1030 board

 hw/arm/aspeed_ast10x0.c          | 84 ++++++++++++++++++++++++++++++--
 hw/arm/aspeed_ast2600.c          |  5 +-
 hw/arm/aspeed_soc.c              |  6 +--
 hw/misc/aspeed_hace.c            | 21 +++++---
 hw/watchdog/wdt_aspeed.c         | 24 +++++++--
 include/hw/arm/aspeed_soc.h      | 14 ++++++
 include/hw/watchdog/wdt_aspeed.h |  4 +-
 tests/avocado/machine_aspeed.py  | 41 +++++++++++++++-
 8 files changed, 176 insertions(+), 23 deletions(-)

-- 
2.38.1

[PATCH v2 01/11] hw/watchdog/wdt_aspeed: Rename MMIO region size as 'iosize'

[Philippe Mathieu-Daudé]

Avoid confusing two different things:
- the WDT I/O region size ('iosize')
- at which offset the SoC map the WDT ('offset')
While it is often the same, we can map smaller region sizes
at larger offsets.

Here we are interested in the I/O region size, so rename as
'iosize'.

Reviewed-by: Peter Delevoryas <peter@pjd.dev>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/aspeed_ast10x0.c          | 2 +-
 hw/arm/aspeed_ast2600.c          | 2 +-
 hw/arm/aspeed_soc.c              | 2 +-
 hw/watchdog/wdt_aspeed.c         | 8 ++++----
 include/hw/watchdog/wdt_aspeed.h | 2 +-
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
index 4d0b9b115f..122b3fd3f3 100644
--- a/hw/arm/aspeed_ast10x0.c
+++ b/hw/arm/aspeed_ast10x0.c
@@ -325,7 +325,7 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
             return;
         }
         aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->wdt[i]), 0,
-                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
+                        sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);
     }
 
     /* GPIO */
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index cd75465c2b..a79e05ddbd 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -472,7 +472,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
             return;
         }
         aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->wdt[i]), 0,
-                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
+                        sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);
     }
 
     /* RAM */
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index b05b9dd416..2c0924d311 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -393,7 +393,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
             return;
         }
         aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->wdt[i]), 0,
-                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
+                        sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);
     }
 
     /* RAM  */
diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
index d753693a2e..958725a1b5 100644
--- a/hw/watchdog/wdt_aspeed.c
+++ b/hw/watchdog/wdt_aspeed.c
@@ -309,7 +309,7 @@ static void aspeed_2400_wdt_class_init(ObjectClass *klass, void *data)
     AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
 
     dc->desc = "ASPEED 2400 Watchdog Controller";
-    awc->offset = 0x20;
+    awc->iosize = 0x20;
     awc->ext_pulse_width_mask = 0xff;
     awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
     awc->wdt_reload = aspeed_wdt_reload;
@@ -346,7 +346,7 @@ static void aspeed_2500_wdt_class_init(ObjectClass *klass, void *data)
     AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
 
     dc->desc = "ASPEED 2500 Watchdog Controller";
-    awc->offset = 0x20;
+    awc->iosize = 0x20;
     awc->ext_pulse_width_mask = 0xfffff;
     awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
     awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
@@ -369,7 +369,7 @@ static void aspeed_2600_wdt_class_init(ObjectClass *klass, void *data)
     AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
 
     dc->desc = "ASPEED 2600 Watchdog Controller";
-    awc->offset = 0x40;
+    awc->iosize = 0x40;
     awc->ext_pulse_width_mask = 0xfffff; /* TODO */
     awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
     awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
@@ -392,7 +392,7 @@ static void aspeed_1030_wdt_class_init(ObjectClass *klass, void *data)
     AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
 
     dc->desc = "ASPEED 1030 Watchdog Controller";
-    awc->offset = 0x80;
+    awc->iosize = 0x80;
     awc->ext_pulse_width_mask = 0xfffff; /* TODO */
     awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
     awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
index dfa5dfa424..db91ee6b51 100644
--- a/include/hw/watchdog/wdt_aspeed.h
+++ b/include/hw/watchdog/wdt_aspeed.h
@@ -40,7 +40,7 @@ struct AspeedWDTState {
 struct AspeedWDTClass {
     SysBusDeviceClass parent_class;
 
-    uint32_t offset;
+    uint32_t iosize;
     uint32_t ext_pulse_width_mask;
     uint32_t reset_ctrl_reg;
     void (*reset_pulse)(AspeedWDTState *s, uint32_t property);
-- 
2.38.1

RE: [PATCH v2 01/11] hw/watchdog/wdt_aspeed: Rename MMIO region size as 'iosize'

[Dong, Eddie]

> 
> Avoid confusing two different things:
> - the WDT I/O region size ('iosize')
> - at which offset the SoC map the WDT ('offset') While it is often the same, we
> can map smaller region sizes at larger offsets.
> 
> Here we are interested in the I/O region size, so rename as 'iosize'.
> 
> Reviewed-by: Peter Delevoryas <peter@pjd.dev>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/aspeed_ast10x0.c          | 2 +-
>  hw/arm/aspeed_ast2600.c          | 2 +-
>  hw/arm/aspeed_soc.c              | 2 +-
>  hw/watchdog/wdt_aspeed.c         | 8 ++++----
>  include/hw/watchdog/wdt_aspeed.h | 2 +-
>  5 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c index
> 4d0b9b115f..122b3fd3f3 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -325,7 +325,7 @@ static void aspeed_soc_ast1030_realize(DeviceState
> *dev_soc, Error **errp)
>              return;
>          }
>          aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->wdt[i]), 0,
> -                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
> +                        sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);


How does the specification say here (I didn't find it)?

I read this is for a case where multiple WDTs are implemented. 
And offset means the distance io registers of WDT[1] are located from WDT[0].
Or does the spec explicitly says the io registers of WDT[1] is located right after
WDT[0] without any gaps in this case, iosize is better)?

>      }
> 
>      /* GPIO */
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c index
> cd75465c2b..a79e05ddbd 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -472,7 +472,7 @@ static void aspeed_soc_ast2600_realize(DeviceState
> *dev, Error **errp)
>              return;
>          }
>          aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->wdt[i]), 0,
> -                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
> +                        sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);
>      }
> 
>      /* RAM */
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c index
> b05b9dd416..2c0924d311 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -393,7 +393,7 @@ static void aspeed_soc_realize(DeviceState *dev,
> Error **errp)
>              return;
>          }
>          aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->wdt[i]), 0,
> -                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
> +                        sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);
>      }
> 
>      /* RAM  */
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c index
> d753693a2e..958725a1b5 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -309,7 +309,7 @@ static void aspeed_2400_wdt_class_init(ObjectClass
> *klass, void *data)
>      AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
> 
>      dc->desc = "ASPEED 2400 Watchdog Controller";
> -    awc->offset = 0x20;
> +    awc->iosize = 0x20;
>      awc->ext_pulse_width_mask = 0xff;
>      awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
>      awc->wdt_reload = aspeed_wdt_reload; @@ -346,7 +346,7 @@ static void
> aspeed_2500_wdt_class_init(ObjectClass *klass, void *data)
>      AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
> 
>      dc->desc = "ASPEED 2500 Watchdog Controller";
> -    awc->offset = 0x20;
> +    awc->iosize = 0x20;
>      awc->ext_pulse_width_mask = 0xfffff;
>      awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
>      awc->reset_pulse = aspeed_2500_wdt_reset_pulse; @@ -369,7 +369,7
> @@ static void aspeed_2600_wdt_class_init(ObjectClass *klass, void *data)
>      AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
> 
>      dc->desc = "ASPEED 2600 Watchdog Controller";
> -    awc->offset = 0x40;
> +    awc->iosize = 0x40;
>      awc->ext_pulse_width_mask = 0xfffff; /* TODO */
>      awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
>      awc->reset_pulse = aspeed_2500_wdt_reset_pulse; @@ -392,7 +392,7
> @@ static void aspeed_1030_wdt_class_init(ObjectClass *klass, void *data)
>      AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
> 
>      dc->desc = "ASPEED 1030 Watchdog Controller";
> -    awc->offset = 0x80;
> +    awc->iosize = 0x80;
>      awc->ext_pulse_width_mask = 0xfffff; /* TODO */
>      awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
>      awc->reset_pulse = aspeed_2500_wdt_reset_pulse; diff --git
> a/include/hw/watchdog/wdt_aspeed.h
> b/include/hw/watchdog/wdt_aspeed.h
> index dfa5dfa424..db91ee6b51 100644
> --- a/include/hw/watchdog/wdt_aspeed.h
> +++ b/include/hw/watchdog/wdt_aspeed.h
> @@ -40,7 +40,7 @@ struct AspeedWDTState {  struct AspeedWDTClass {
>      SysBusDeviceClass parent_class;
> 
> -    uint32_t offset;
> +    uint32_t iosize;
>      uint32_t ext_pulse_width_mask;
>      uint32_t reset_ctrl_reg;
>      void (*reset_pulse)(AspeedWDTState *s, uint32_t property);
> --
> 2.38.1
> 

Re: [PATCH v2 01/11] hw/watchdog/wdt_aspeed: Rename MMIO region size as 'iosize'

[Cédric Le Goater]

On 12/31/22 23:43, Dong, Eddie wrote:
>>
>> Avoid confusing two different things:
>> - the WDT I/O region size ('iosize')
>> - at which offset the SoC map the WDT ('offset') While it is often the same, we
>> can map smaller region sizes at larger offsets.
>>
>> Here we are interested in the I/O region size, so rename as 'iosize'.
>>
>> Reviewed-by: Peter Delevoryas <peter@pjd.dev>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/arm/aspeed_ast10x0.c          | 2 +-
>>   hw/arm/aspeed_ast2600.c          | 2 +-
>>   hw/arm/aspeed_soc.c              | 2 +-
>>   hw/watchdog/wdt_aspeed.c         | 8 ++++----
>>   include/hw/watchdog/wdt_aspeed.h | 2 +-
>>   5 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c index
>> 4d0b9b115f..122b3fd3f3 100644
>> --- a/hw/arm/aspeed_ast10x0.c
>> +++ b/hw/arm/aspeed_ast10x0.c
>> @@ -325,7 +325,7 @@ static void aspeed_soc_ast1030_realize(DeviceState
>> *dev_soc, Error **errp)
>>               return;
>>           }
>>           aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->wdt[i]), 0,
>> -                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
>> +                        sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);
> 
> 
> How does the specification say here (I didn't find it)?
> I read this is for a case where multiple WDTs are implemented.
> And offset means the distance io registers of WDT[1] are located from WDT[0].

The specs say 'offset'

> Or does the spec explicitly says the io registers of WDT[1] is located right after
> WDT[0] without any gaps in this case, iosize is better)?

The IO regions are contiguous but size/width is larger than the register set.

That said, the model takes some shortcuts and Phil's proposal is a sign that
we need to clarify and distinguish the number of regs, the region width and
the offset where it is mapped in the overall MMIO region of the SoC.


> 
>>       }
>>
>>       /* GPIO */
>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c index
>> cd75465c2b..a79e05ddbd 100644
>> --- a/hw/arm/aspeed_ast2600.c
>> +++ b/hw/arm/aspeed_ast2600.c
>> @@ -472,7 +472,7 @@ static void aspeed_soc_ast2600_realize(DeviceState
>> *dev, Error **errp)
>>               return;
>>           }
>>           aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->wdt[i]), 0,
>> -                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
>> +                        sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);

May be, as a clarification, introduce :

   hwaddr wdt_offset = sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize



>>       }
>>
>>       /* RAM */
>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c index
>> b05b9dd416..2c0924d311 100644
>> --- a/hw/arm/aspeed_soc.c
>> +++ b/hw/arm/aspeed_soc.c
>> @@ -393,7 +393,7 @@ static void aspeed_soc_realize(DeviceState *dev,
>> Error **errp)
>>               return;
>>           }
>>           aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->wdt[i]), 0,
>> -                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
>> +                        sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);
>>       }
>>
>>       /* RAM  */
>> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c index
>> d753693a2e..958725a1b5 100644
>> --- a/hw/watchdog/wdt_aspeed.c
>> +++ b/hw/watchdog/wdt_aspeed.c
>> @@ -309,7 +309,7 @@ static void aspeed_2400_wdt_class_init(ObjectClass
>> *klass, void *data)
>>       AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
>>
>>       dc->desc = "ASPEED 2400 Watchdog Controller";
>> -    awc->offset = 0x20;
>> +    awc->iosize = 0x20;
>>       awc->ext_pulse_width_mask = 0xff;
>>       awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
>>       awc->wdt_reload = aspeed_wdt_reload; @@ -346,7 +346,7 @@ static void
>> aspeed_2500_wdt_class_init(ObjectClass *klass, void *data)
>>       AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
>>
>>       dc->desc = "ASPEED 2500 Watchdog Controller";
>> -    awc->offset = 0x20;
>> +    awc->iosize = 0x20;
>>       awc->ext_pulse_width_mask = 0xfffff;
>>       awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
>>       awc->reset_pulse = aspeed_2500_wdt_reset_pulse; @@ -369,7 +369,7
>> @@ static void aspeed_2600_wdt_class_init(ObjectClass *klass, void *data)
>>       AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
>>
>>       dc->desc = "ASPEED 2600 Watchdog Controller";
>> -    awc->offset = 0x40;
>> +    awc->iosize = 0x40;
>>       awc->ext_pulse_width_mask = 0xfffff; /* TODO */
>>       awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
>>       awc->reset_pulse = aspeed_2500_wdt_reset_pulse; @@ -392,7 +392,7
>> @@ static void aspeed_1030_wdt_class_init(ObjectClass *klass, void *data)
>>       AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
>>
>>       dc->desc = "ASPEED 1030 Watchdog Controller";
>> -    awc->offset = 0x80;
>> +    awc->iosize = 0x80;
>>       awc->ext_pulse_width_mask = 0xfffff; /* TODO */
>>       awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
>>       awc->reset_pulse = aspeed_2500_wdt_reset_pulse; diff --git
>> a/include/hw/watchdog/wdt_aspeed.h
>> b/include/hw/watchdog/wdt_aspeed.h
>> index dfa5dfa424..db91ee6b51 100644
>> --- a/include/hw/watchdog/wdt_aspeed.h
>> +++ b/include/hw/watchdog/wdt_aspeed.h
>> @@ -40,7 +40,7 @@ struct AspeedWDTState {  struct AspeedWDTClass {
>>       SysBusDeviceClass parent_class;
>>
>> -    uint32_t offset;
>> +    uint32_t iosize;
>>       uint32_t ext_pulse_width_mask;
>>       uint32_t reset_ctrl_reg;
>>       void (*reset_pulse)(AspeedWDTState *s, uint32_t property);
>> --
>> 2.38.1
>>
> 

Re: [PATCH v2 01/11] hw/watchdog/wdt_aspeed: Rename MMIO region size as 'iosize'

[Cédric Le Goater]

Philippe,

On 1/2/23 13:30, Cédric Le Goater wrote:
> On 12/31/22 23:43, Dong, Eddie wrote:
>>>
>>> Avoid confusing two different things:
>>> - the WDT I/O region size ('iosize')
>>> - at which offset the SoC map the WDT ('offset') While it is often the same, we
>>> can map smaller region sizes at larger offsets.
>>>
>>> Here we are interested in the I/O region size, so rename as 'iosize'.
>>>
>>> Reviewed-by: Peter Delevoryas <peter@pjd.dev>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/arm/aspeed_ast10x0.c          | 2 +-
>>>   hw/arm/aspeed_ast2600.c          | 2 +-
>>>   hw/arm/aspeed_soc.c              | 2 +-
>>>   hw/watchdog/wdt_aspeed.c         | 8 ++++----
>>>   include/hw/watchdog/wdt_aspeed.h | 2 +-
>>>   5 files changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c index
>>> 4d0b9b115f..122b3fd3f3 100644
>>> --- a/hw/arm/aspeed_ast10x0.c
>>> +++ b/hw/arm/aspeed_ast10x0.c
>>> @@ -325,7 +325,7 @@ static void aspeed_soc_ast1030_realize(DeviceState
>>> *dev_soc, Error **errp)
>>>               return;
>>>           }
>>>           aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->wdt[i]), 0,
>>> -                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
>>> +                        sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);
>>
>>
>> How does the specification say here (I didn't find it)?
>> I read this is for a case where multiple WDTs are implemented.
>> And offset means the distance io registers of WDT[1] are located from WDT[0].
> 
> The specs say 'offset'
> 
>> Or does the spec explicitly says the io registers of WDT[1] is located right after
>> WDT[0] without any gaps in this case, iosize is better)?
> 
> The IO regions are contiguous but size/width is larger than the register set.
> 
> That said, the model takes some shortcuts and Phil's proposal is a sign that
> we need to clarify and distinguish the number of regs, the region width and
> the offset where it is mapped in the overall MMIO region of the SoC.
> 
> 
>>
>>>       }
>>>
>>>       /* GPIO */
>>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c index
>>> cd75465c2b..a79e05ddbd 100644
>>> --- a/hw/arm/aspeed_ast2600.c
>>> +++ b/hw/arm/aspeed_ast2600.c
>>> @@ -472,7 +472,7 @@ static void aspeed_soc_ast2600_realize(DeviceState
>>> *dev, Error **errp)
>>>               return;
>>>           }
>>>           aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->wdt[i]), 0,
>>> -                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
>>> +                        sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);
> 
> May be, as a clarification, introduce :
> 
>    hwaddr wdt_offset = sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize

That would be nice to add since you are about to respin.

Thanks,

C.

> 
> 
> 
>>>       }
>>>
>>>       /* RAM */
>>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c index
>>> b05b9dd416..2c0924d311 100644
>>> --- a/hw/arm/aspeed_soc.c
>>> +++ b/hw/arm/aspeed_soc.c
>>> @@ -393,7 +393,7 @@ static void aspeed_soc_realize(DeviceState *dev,
>>> Error **errp)
>>>               return;
>>>           }
>>>           aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->wdt[i]), 0,
>>> -                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
>>> +                        sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);
>>>       }
>>>
>>>       /* RAM  */
>>> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c index
>>> d753693a2e..958725a1b5 100644
>>> --- a/hw/watchdog/wdt_aspeed.c
>>> +++ b/hw/watchdog/wdt_aspeed.c
>>> @@ -309,7 +309,7 @@ static void aspeed_2400_wdt_class_init(ObjectClass
>>> *klass, void *data)
>>>       AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
>>>
>>>       dc->desc = "ASPEED 2400 Watchdog Controller";
>>> -    awc->offset = 0x20;
>>> +    awc->iosize = 0x20;
>>>       awc->ext_pulse_width_mask = 0xff;
>>>       awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
>>>       awc->wdt_reload = aspeed_wdt_reload; @@ -346,7 +346,7 @@ static void
>>> aspeed_2500_wdt_class_init(ObjectClass *klass, void *data)
>>>       AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
>>>
>>>       dc->desc = "ASPEED 2500 Watchdog Controller";
>>> -    awc->offset = 0x20;
>>> +    awc->iosize = 0x20;
>>>       awc->ext_pulse_width_mask = 0xfffff;
>>>       awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
>>>       awc->reset_pulse = aspeed_2500_wdt_reset_pulse; @@ -369,7 +369,7
>>> @@ static void aspeed_2600_wdt_class_init(ObjectClass *klass, void *data)
>>>       AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
>>>
>>>       dc->desc = "ASPEED 2600 Watchdog Controller";
>>> -    awc->offset = 0x40;
>>> +    awc->iosize = 0x40;
>>>       awc->ext_pulse_width_mask = 0xfffff; /* TODO */
>>>       awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
>>>       awc->reset_pulse = aspeed_2500_wdt_reset_pulse; @@ -392,7 +392,7
>>> @@ static void aspeed_1030_wdt_class_init(ObjectClass *klass, void *data)
>>>       AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
>>>
>>>       dc->desc = "ASPEED 1030 Watchdog Controller";
>>> -    awc->offset = 0x80;
>>> +    awc->iosize = 0x80;
>>>       awc->ext_pulse_width_mask = 0xfffff; /* TODO */
>>>       awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
>>>       awc->reset_pulse = aspeed_2500_wdt_reset_pulse; diff --git
>>> a/include/hw/watchdog/wdt_aspeed.h
>>> b/include/hw/watchdog/wdt_aspeed.h
>>> index dfa5dfa424..db91ee6b51 100644
>>> --- a/include/hw/watchdog/wdt_aspeed.h
>>> +++ b/include/hw/watchdog/wdt_aspeed.h
>>> @@ -40,7 +40,7 @@ struct AspeedWDTState {  struct AspeedWDTClass {
>>>       SysBusDeviceClass parent_class;
>>>
>>> -    uint32_t offset;
>>> +    uint32_t iosize;
>>>       uint32_t ext_pulse_width_mask;
>>>       uint32_t reset_ctrl_reg;
>>>       void (*reset_pulse)(AspeedWDTState *s, uint32_t property);
>>> -- 
>>> 2.38.1
>>>
>>
> 

Re: [PATCH v2 01/11] hw/watchdog/wdt_aspeed: Rename MMIO region size as 'iosize'

[Peter Delevoryas]

On Fri, Dec 30, 2022 at 12:34:54PM +0100, Philippe Mathieu-Daudé wrote:
> Avoid confusing two different things:
> - the WDT I/O region size ('iosize')
> - at which offset the SoC map the WDT ('offset')
> While it is often the same, we can map smaller region sizes
> at larger offsets.
> 
> Here we are interested in the I/O region size, so rename as
> 'iosize'.
> 
> Reviewed-by: Peter Delevoryas <peter@pjd.dev>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/aspeed_ast10x0.c          | 2 +-
>  hw/arm/aspeed_ast2600.c          | 2 +-
>  hw/arm/aspeed_soc.c              | 2 +-
>  hw/watchdog/wdt_aspeed.c         | 8 ++++----
>  include/hw/watchdog/wdt_aspeed.h | 2 +-
>  5 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> index 4d0b9b115f..122b3fd3f3 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -325,7 +325,7 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
>              return;
>          }
>          aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->wdt[i]), 0,
> -                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
> +                        sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);
>      }
>  
>      /* GPIO */
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index cd75465c2b..a79e05ddbd 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -472,7 +472,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>              return;
>          }
>          aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->wdt[i]), 0,
> -                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
> +                        sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);
>      }
>  
>      /* RAM */
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index b05b9dd416..2c0924d311 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -393,7 +393,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>              return;
>          }
>          aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->wdt[i]), 0,
> -                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
> +                        sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);
>      }
>  
>      /* RAM  */
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> index d753693a2e..958725a1b5 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -309,7 +309,7 @@ static void aspeed_2400_wdt_class_init(ObjectClass *klass, void *data)
>      AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
>  
>      dc->desc = "ASPEED 2400 Watchdog Controller";
> -    awc->offset = 0x20;
> +    awc->iosize = 0x20;
>      awc->ext_pulse_width_mask = 0xff;
>      awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
>      awc->wdt_reload = aspeed_wdt_reload;
> @@ -346,7 +346,7 @@ static void aspeed_2500_wdt_class_init(ObjectClass *klass, void *data)
>      AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
>  
>      dc->desc = "ASPEED 2500 Watchdog Controller";
> -    awc->offset = 0x20;
> +    awc->iosize = 0x20;
>      awc->ext_pulse_width_mask = 0xfffff;
>      awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
>      awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
> @@ -369,7 +369,7 @@ static void aspeed_2600_wdt_class_init(ObjectClass *klass, void *data)
>      AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
>  
>      dc->desc = "ASPEED 2600 Watchdog Controller";
> -    awc->offset = 0x40;
> +    awc->iosize = 0x40;
>      awc->ext_pulse_width_mask = 0xfffff; /* TODO */
>      awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
>      awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
> @@ -392,7 +392,7 @@ static void aspeed_1030_wdt_class_init(ObjectClass *klass, void *data)
>      AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
>  
>      dc->desc = "ASPEED 1030 Watchdog Controller";
> -    awc->offset = 0x80;
> +    awc->iosize = 0x80;

Just noticed, the offset/iosize for the AST1030 seems to be incorrect. It
should be 0x40, not 0x80.

It should have the same value as the AST2600, since they should be the exact
same peripheral.

This is not your fault, just a bug report from me.

This seems to be an error in the original patch from Steven Lee, but also a bug in the AspeedSDK
Zephyr kernel driver?

e259e01ecb ("aspeed/wdt: Add AST1030 support")
https://github.com/AspeedTech-BMC/zephyr/blob/1b9764a854abbea8b38445f1d5de9f4441e29c3b/drivers/watchdog/wdt_aspeed.c#L21

The only other explanation is that the datasheet I have is old and wrong. I'm
referencing the AST1030 A0 v0.5 datasheet from February 2021.

Sorry for not noticing this earlier!

So to be clear:

Chip: Watchdog[N] Iosize
AST2400: 0x20
AST2500: 0x20
AST2600: 0x40
AST1030: 0x40

Chip: Watchdog[N] WDT_REGS_MAX
AST2400: 0x18
AST2500: 0x1C (Added "Reset Mask" register)
AST2600: 0x30 (Added even more reset control and mask registers)
AST1030: 0x30

Thanks,
Peter

>      awc->ext_pulse_width_mask = 0xfffff; /* TODO */
>      awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
>      awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
> diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
> index dfa5dfa424..db91ee6b51 100644
> --- a/include/hw/watchdog/wdt_aspeed.h
> +++ b/include/hw/watchdog/wdt_aspeed.h
> @@ -40,7 +40,7 @@ struct AspeedWDTState {
>  struct AspeedWDTClass {
>      SysBusDeviceClass parent_class;
>  
> -    uint32_t offset;
> +    uint32_t iosize;
>      uint32_t ext_pulse_width_mask;
>      uint32_t reset_ctrl_reg;
>      void (*reset_pulse)(AspeedWDTState *s, uint32_t property);
> -- 
> 2.38.1
> 
> 

[PATCH v2 02/11] hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers

[Philippe Mathieu-Daudé]

When booting the Zephyr demo in [1] we get:

  aspeed.io: unimplemented device write (size 4, offset 0x185128, value 0x030f1ff1) <--
  aspeed.io: unimplemented device write (size 4, offset 0x18512c, value 0x03fffff1)

This corresponds to this Zephyr code [2]:

  static int aspeed_wdt_init(const struct device *dev)
  {
    const struct aspeed_wdt_config *config = dev->config;
    struct aspeed_wdt_data *const data = dev->data;
    uint32_t reg_val;

    /* disable WDT by default */
    reg_val = sys_read32(config->ctrl_base + WDT_CTRL_REG);
    reg_val &= ~WDT_CTRL_ENABLE;
    sys_write32(reg_val, config->ctrl_base + WDT_CTRL_REG);

    sys_write32(data->rst_mask1,
                config->ctrl_base + WDT_SW_RESET_MASK1_REG);   <------
    sys_write32(data->rst_mask2,
                config->ctrl_base + WDT_SW_RESET_MASK2_REG);

    return 0;
  }

The register definitions are [3]:

  #define WDT_RELOAD_VAL_REG          0x0004
  #define WDT_RESTART_REG             0x0008
  #define WDT_CTRL_REG                0x000C
  #define WDT_TIMEOUT_STATUS_REG      0x0010
  #define WDT_TIMEOUT_STATUS_CLR_REG  0x0014
  #define WDT_RESET_MASK1_REG         0x001C
  #define WDT_RESET_MASK2_REG         0x0020
  #define WDT_SW_RESET_MASK1_REG      0x0028   <------
  #define WDT_SW_RESET_MASK2_REG      0x002C
  #define WDT_SW_RESET_CTRL_REG       0x0024

Currently QEMU only cover a MMIO region of size 0x20:

  #define ASPEED_WDT_REGS_MAX        (0x20 / 4)

Change to map the whole 'iosize' which might be bigger, covering
the other registers. The MemoryRegionOps read/write handlers will
report the accesses as out-of-bounds guest-errors, but the next
commit will report them as unimplemented.

[1] https://github.com/AspeedTech-BMC/zephyr/releases/tag/v00.01.07
[2] https://github.com/AspeedTech-BMC/zephyr/commit/2e99f10ac27b
[3] https://github.com/AspeedTech-BMC/zephyr/blob/v00.01.08/drivers/watchdog/wdt_aspeed.c#L31

Reviewed-by: Peter Delevoryas <peter@pjd.dev>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/watchdog/wdt_aspeed.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
index 958725a1b5..eefca31ae4 100644
--- a/hw/watchdog/wdt_aspeed.c
+++ b/hw/watchdog/wdt_aspeed.c
@@ -260,6 +260,7 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     AspeedWDTState *s = ASPEED_WDT(dev);
+    AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(dev);
 
     assert(s->scu);
 
@@ -271,7 +272,7 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
     s->pclk_freq = PCLK_HZ;
 
     memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_wdt_ops, s,
-                          TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
+                          TYPE_ASPEED_WDT, awc->iosize);
     sysbus_init_mmio(sbd, &s->iomem);
 }
 
-- 
2.38.1

Re: [PATCH v2 02/11] hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers

[Philippe Mathieu-Daudé]

On 30/12/22 12:34, Philippe Mathieu-Daudé wrote:
> When booting the Zephyr demo in [1] we get:
> 
>    aspeed.io: unimplemented device write (size 4, offset 0x185128, value 0x030f1ff1) <--
>    aspeed.io: unimplemented device write (size 4, offset 0x18512c, value 0x03fffff1)
> 
> This corresponds to this Zephyr code [2]:
> 
>    static int aspeed_wdt_init(const struct device *dev)
>    {
>      const struct aspeed_wdt_config *config = dev->config;
>      struct aspeed_wdt_data *const data = dev->data;
>      uint32_t reg_val;
> 
>      /* disable WDT by default */
>      reg_val = sys_read32(config->ctrl_base + WDT_CTRL_REG);
>      reg_val &= ~WDT_CTRL_ENABLE;
>      sys_write32(reg_val, config->ctrl_base + WDT_CTRL_REG);
> 
>      sys_write32(data->rst_mask1,
>                  config->ctrl_base + WDT_SW_RESET_MASK1_REG);   <------
>      sys_write32(data->rst_mask2,
>                  config->ctrl_base + WDT_SW_RESET_MASK2_REG);
> 
>      return 0;
>    }
> 
> The register definitions are [3]:
> 
>    #define WDT_RELOAD_VAL_REG          0x0004
>    #define WDT_RESTART_REG             0x0008
>    #define WDT_CTRL_REG                0x000C
>    #define WDT_TIMEOUT_STATUS_REG      0x0010
>    #define WDT_TIMEOUT_STATUS_CLR_REG  0x0014
>    #define WDT_RESET_MASK1_REG         0x001C
>    #define WDT_RESET_MASK2_REG         0x0020
>    #define WDT_SW_RESET_MASK1_REG      0x0028   <------
>    #define WDT_SW_RESET_MASK2_REG      0x002C
>    #define WDT_SW_RESET_CTRL_REG       0x0024
> 
> Currently QEMU only cover a MMIO region of size 0x20:
> 
>    #define ASPEED_WDT_REGS_MAX        (0x20 / 4)
> 
> Change to map the whole 'iosize' which might be bigger, covering
> the other registers. The MemoryRegionOps read/write handlers will
> report the accesses as out-of-bounds guest-errors, but the next
> commit will report them as unimplemented.

I'll amend here for clarity:

---

Memory layout before this change:

   (qemu) info mtree -f
     ...
     000000007e785000-000000007e78501f (prio 0, i/o): aspeed.wdt
     000000007e785020-000000007e78507f (prio -1000, i/o): aspeed.io 
@0000000000185020
     000000007e785080-000000007e78509f (prio 0, i/o): aspeed.wdt
     000000007e7850a0-000000007e7850ff (prio -1000, i/o): aspeed.io 
@00000000001850a0
     000000007e785100-000000007e78511f (prio 0, i/o): aspeed.wdt
     000000007e785120-000000007e78517f (prio -1000, i/o): aspeed.io 
@0000000000185120
     000000007e785180-000000007e78519f (prio 0, i/o): aspeed.wdt
     000000007e7851a0-000000007e788fff (prio -1000, i/o): aspeed.io 
@00000000001851a0

After:

   (qemu) info mtree -f
     ...
     000000007e785000-000000007e78507f (prio 0, i/o): aspeed.wdt
     000000007e785080-000000007e7850ff (prio 0, i/o): aspeed.wdt
     000000007e785100-000000007e78517f (prio 0, i/o): aspeed.wdt
     000000007e785180-000000007e7851ff (prio 0, i/o): aspeed.wdt
     000000007e785200-000000007e788fff (prio -1000, i/o): aspeed.io 
@0000000000185200
---

> [1] https://github.com/AspeedTech-BMC/zephyr/releases/tag/v00.01.07
> [2] https://github.com/AspeedTech-BMC/zephyr/commit/2e99f10ac27b
> [3] https://github.com/AspeedTech-BMC/zephyr/blob/v00.01.08/drivers/watchdog/wdt_aspeed.c#L31
> 
> Reviewed-by: Peter Delevoryas <peter@pjd.dev>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/watchdog/wdt_aspeed.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> index 958725a1b5..eefca31ae4 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -260,6 +260,7 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
>   {
>       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>       AspeedWDTState *s = ASPEED_WDT(dev);
> +    AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(dev);
>   
>       assert(s->scu);
>   
> @@ -271,7 +272,7 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
>       s->pclk_freq = PCLK_HZ;
>   
>       memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_wdt_ops, s,
> -                          TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
> +                          TYPE_ASPEED_WDT, awc->iosize);
>       sysbus_init_mmio(sbd, &s->iomem);
>   }
>   

Re: [PATCH v2 02/11] hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers

[Peter Delevoryas]

On Fri, Dec 30, 2022 at 01:31:35PM +0100, Philippe Mathieu-Daudé wrote:
> On 30/12/22 12:34, Philippe Mathieu-Daudé wrote:
> > When booting the Zephyr demo in [1] we get:
> > 
> >    aspeed.io: unimplemented device write (size 4, offset 0x185128, value 0x030f1ff1) <--
> >    aspeed.io: unimplemented device write (size 4, offset 0x18512c, value 0x03fffff1)
> > 
> > This corresponds to this Zephyr code [2]:
> > 
> >    static int aspeed_wdt_init(const struct device *dev)
> >    {
> >      const struct aspeed_wdt_config *config = dev->config;
> >      struct aspeed_wdt_data *const data = dev->data;
> >      uint32_t reg_val;
> > 
> >      /* disable WDT by default */
> >      reg_val = sys_read32(config->ctrl_base + WDT_CTRL_REG);
> >      reg_val &= ~WDT_CTRL_ENABLE;
> >      sys_write32(reg_val, config->ctrl_base + WDT_CTRL_REG);
> > 
> >      sys_write32(data->rst_mask1,
> >                  config->ctrl_base + WDT_SW_RESET_MASK1_REG);   <------
> >      sys_write32(data->rst_mask2,
> >                  config->ctrl_base + WDT_SW_RESET_MASK2_REG);
> > 
> >      return 0;
> >    }
> > 
> > The register definitions are [3]:
> > 
> >    #define WDT_RELOAD_VAL_REG          0x0004
> >    #define WDT_RESTART_REG             0x0008
> >    #define WDT_CTRL_REG                0x000C
> >    #define WDT_TIMEOUT_STATUS_REG      0x0010
> >    #define WDT_TIMEOUT_STATUS_CLR_REG  0x0014
> >    #define WDT_RESET_MASK1_REG         0x001C
> >    #define WDT_RESET_MASK2_REG         0x0020
> >    #define WDT_SW_RESET_MASK1_REG      0x0028   <------
> >    #define WDT_SW_RESET_MASK2_REG      0x002C
> >    #define WDT_SW_RESET_CTRL_REG       0x0024
> > 
> > Currently QEMU only cover a MMIO region of size 0x20:
> > 
> >    #define ASPEED_WDT_REGS_MAX        (0x20 / 4)
> > 
> > Change to map the whole 'iosize' which might be bigger, covering
> > the other registers. The MemoryRegionOps read/write handlers will
> > report the accesses as out-of-bounds guest-errors, but the next
> > commit will report them as unimplemented.

Ahhhh I see, this makes perfect sense now, thanks for the detail!

> 
> I'll amend here for clarity:
> 
> ---
> 
> Memory layout before this change:
> 
>   (qemu) info mtree -f
>     ...
>     000000007e785000-000000007e78501f (prio 0, i/o): aspeed.wdt
>     000000007e785020-000000007e78507f (prio -1000, i/o): aspeed.io
> @0000000000185020
>     000000007e785080-000000007e78509f (prio 0, i/o): aspeed.wdt
>     000000007e7850a0-000000007e7850ff (prio -1000, i/o): aspeed.io
> @00000000001850a0
>     000000007e785100-000000007e78511f (prio 0, i/o): aspeed.wdt
>     000000007e785120-000000007e78517f (prio -1000, i/o): aspeed.io
> @0000000000185120
>     000000007e785180-000000007e78519f (prio 0, i/o): aspeed.wdt
>     000000007e7851a0-000000007e788fff (prio -1000, i/o): aspeed.io
> @00000000001851a0
> 
> After:
> 
>   (qemu) info mtree -f
>     ...
>     000000007e785000-000000007e78507f (prio 0, i/o): aspeed.wdt
>     000000007e785080-000000007e7850ff (prio 0, i/o): aspeed.wdt
>     000000007e785100-000000007e78517f (prio 0, i/o): aspeed.wdt
>     000000007e785180-000000007e7851ff (prio 0, i/o): aspeed.wdt
>     000000007e785200-000000007e788fff (prio -1000, i/o): aspeed.io
> @0000000000185200
> ---
> 
> > [1] https://github.com/AspeedTech-BMC/zephyr/releases/tag/v00.01.07
> > [2] https://github.com/AspeedTech-BMC/zephyr/commit/2e99f10ac27b
> > [3] https://github.com/AspeedTech-BMC/zephyr/blob/v00.01.08/drivers/watchdog/wdt_aspeed.c#L31
> > 
> > Reviewed-by: Peter Delevoryas <peter@pjd.dev>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   hw/watchdog/wdt_aspeed.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> > index 958725a1b5..eefca31ae4 100644
> > --- a/hw/watchdog/wdt_aspeed.c
> > +++ b/hw/watchdog/wdt_aspeed.c
> > @@ -260,6 +260,7 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
> >   {
> >       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> >       AspeedWDTState *s = ASPEED_WDT(dev);
> > +    AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(dev);
> >       assert(s->scu);
> > @@ -271,7 +272,7 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
> >       s->pclk_freq = PCLK_HZ;
> >       memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_wdt_ops, s,
> > -                          TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
> > +                          TYPE_ASPEED_WDT, awc->iosize);
> >       sysbus_init_mmio(sbd, &s->iomem);
> >   }
> 

RE: [PATCH v2 02/11] hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers

[Dong, Eddie]

> When booting the Zephyr demo in [1] we get:
> 
>   aspeed.io: unimplemented device write (size 4, offset 0x185128, value
> 0x030f1ff1) <--
>   aspeed.io: unimplemented device write (size 4, offset 0x18512c, value
> 0x03fffff1)
> 
> This corresponds to this Zephyr code [2]:
> 
>   static int aspeed_wdt_init(const struct device *dev)
>   {
>     const struct aspeed_wdt_config *config = dev->config;
>     struct aspeed_wdt_data *const data = dev->data;
>     uint32_t reg_val;
> 
>     /* disable WDT by default */
>     reg_val = sys_read32(config->ctrl_base + WDT_CTRL_REG);
>     reg_val &= ~WDT_CTRL_ENABLE;
>     sys_write32(reg_val, config->ctrl_base + WDT_CTRL_REG);
> 
>     sys_write32(data->rst_mask1,
>                 config->ctrl_base + WDT_SW_RESET_MASK1_REG);   <------
>     sys_write32(data->rst_mask2,
>                 config->ctrl_base + WDT_SW_RESET_MASK2_REG);
> 
>     return 0;
>   }
> 
> The register definitions are [3]:
> 
>   #define WDT_RELOAD_VAL_REG          0x0004
>   #define WDT_RESTART_REG             0x0008
>   #define WDT_CTRL_REG                0x000C
>   #define WDT_TIMEOUT_STATUS_REG      0x0010
>   #define WDT_TIMEOUT_STATUS_CLR_REG  0x0014
>   #define WDT_RESET_MASK1_REG         0x001C
>   #define WDT_RESET_MASK2_REG         0x0020
>   #define WDT_SW_RESET_MASK1_REG      0x0028   <------
>   #define WDT_SW_RESET_MASK2_REG      0x002C
>   #define WDT_SW_RESET_CTRL_REG       0x0024
> 
> Currently QEMU only cover a MMIO region of size 0x20:
> 
>   #define ASPEED_WDT_REGS_MAX        (0x20 / 4)
> 
> Change to map the whole 'iosize' which might be bigger, covering the other

The root cause is that ASPEED_WDT_REGS_MAX is too small, right?
Probably the Qemu is emulating an old version of the hardware.

Given the meaning of ASPEED_WDT_REGS_MAX, it should be larger than iosize, not?
Probably ASPEED_WDT_REGS_MAX should be per device type (aspeed_2400/2500),
while iosize is for all devices, and its initial value comes from the per device type REGS_MAX.

> registers. The MemoryRegionOps read/write handlers will report the accesses
> as out-of-bounds guest-errors, but the next commit will report them as
> unimplemented.
> 
> [1] https://github.com/AspeedTech-BMC/zephyr/releases/tag/v00.01.07
> [2] https://github.com/AspeedTech-BMC/zephyr/commit/2e99f10ac27b
> [3] https://github.com/AspeedTech-
> BMC/zephyr/blob/v00.01.08/drivers/watchdog/wdt_aspeed.c#L31
> 
> Reviewed-by: Peter Delevoryas <peter@pjd.dev>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/watchdog/wdt_aspeed.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c index
> 958725a1b5..eefca31ae4 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -260,6 +260,7 @@ static void aspeed_wdt_realize(DeviceState *dev,
> Error **errp)  {
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>      AspeedWDTState *s = ASPEED_WDT(dev);
> +    AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(dev);
> 
>      assert(s->scu);
> 
> @@ -271,7 +272,7 @@ static void aspeed_wdt_realize(DeviceState *dev,
> Error **errp)
>      s->pclk_freq = PCLK_HZ;
> 
>      memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_wdt_ops, s,
> -                          TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
> +                          TYPE_ASPEED_WDT, awc->iosize);
>      sysbus_init_mmio(sbd, &s->iomem);
>  }
> 
> --
> 2.38.1
> 

Re: [PATCH v2 02/11] hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers

[Philippe Mathieu-Daudé]

On 31/12/22 23:52, Dong, Eddie wrote:

>>    #define WDT_RELOAD_VAL_REG          0x0004
>>    #define WDT_RESTART_REG             0x0008
>>    #define WDT_CTRL_REG                0x000C
>>    #define WDT_TIMEOUT_STATUS_REG      0x0010
>>    #define WDT_TIMEOUT_STATUS_CLR_REG  0x0014
>>    #define WDT_RESET_MASK1_REG         0x001C
>>    #define WDT_RESET_MASK2_REG         0x0020
>>    #define WDT_SW_RESET_MASK1_REG      0x0028   <------
>>    #define WDT_SW_RESET_MASK2_REG      0x002C
>>    #define WDT_SW_RESET_CTRL_REG       0x0024
>>
>> Currently QEMU only cover a MMIO region of size 0x20:
>>
>>    #define ASPEED_WDT_REGS_MAX        (0x20 / 4)
>>
>> Change to map the whole 'iosize' which might be bigger, covering the other
> 
> The root cause is that ASPEED_WDT_REGS_MAX is too small, right?
> Probably the Qemu is emulating an old version of the hardware.
> 
> Given the meaning of ASPEED_WDT_REGS_MAX, it should be larger than iosize, not?
> Probably ASPEED_WDT_REGS_MAX should be per device type (aspeed_2400/2500),
> while iosize is for all devices, and its initial value comes from the per device type REGS_MAX.

You are correct. I don't have access to the specifications neither,
but I suspect you are right, the current watchdog is modelling an
older version that what the AST1030 has.

I started these WDT patches to understand the unimplemented accesses
done by Zephyr, but eventually they resulted irrelevant to the fixes
(see following patches) so I'll simply drop them.

Thanks,

Phil.

Re: [PATCH v2 02/11] hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers

[Cédric Le Goater]

On 12/31/22 23:52, Dong, Eddie wrote:
>> When booting the Zephyr demo in [1] we get:
>>
>>    aspeed.io: unimplemented device write (size 4, offset 0x185128, value
>> 0x030f1ff1) <--
>>    aspeed.io: unimplemented device write (size 4, offset 0x18512c, value
>> 0x03fffff1)
>>
>> This corresponds to this Zephyr code [2]:
>>
>>    static int aspeed_wdt_init(const struct device *dev)
>>    {
>>      const struct aspeed_wdt_config *config = dev->config;
>>      struct aspeed_wdt_data *const data = dev->data;
>>      uint32_t reg_val;
>>
>>      /* disable WDT by default */
>>      reg_val = sys_read32(config->ctrl_base + WDT_CTRL_REG);
>>      reg_val &= ~WDT_CTRL_ENABLE;
>>      sys_write32(reg_val, config->ctrl_base + WDT_CTRL_REG);
>>
>>      sys_write32(data->rst_mask1,
>>                  config->ctrl_base + WDT_SW_RESET_MASK1_REG);   <------
>>      sys_write32(data->rst_mask2,
>>                  config->ctrl_base + WDT_SW_RESET_MASK2_REG);
>>
>>      return 0;
>>    }
>>
>> The register definitions are [3]:
>>
>>    #define WDT_RELOAD_VAL_REG          0x0004
>>    #define WDT_RESTART_REG             0x0008
>>    #define WDT_CTRL_REG                0x000C
>>    #define WDT_TIMEOUT_STATUS_REG      0x0010
>>    #define WDT_TIMEOUT_STATUS_CLR_REG  0x0014
>>    #define WDT_RESET_MASK1_REG         0x001C
>>    #define WDT_RESET_MASK2_REG         0x0020
>>    #define WDT_SW_RESET_MASK1_REG      0x0028   <------
>>    #define WDT_SW_RESET_MASK2_REG      0x002C
>>    #define WDT_SW_RESET_CTRL_REG       0x0024
>>
>> Currently QEMU only cover a MMIO region of size 0x20:
>>
>>    #define ASPEED_WDT_REGS_MAX        (0x20 / 4)
>>
>> Change to map the whole 'iosize' which might be bigger, covering the other
> 
> The root cause is that ASPEED_WDT_REGS_MAX is too small, right?
> Probably the Qemu is emulating an old version of the hardware.
> 
> Given the meaning of ASPEED_WDT_REGS_MAX, it should be larger than iosize, not?
> Probably ASPEED_WDT_REGS_MAX should be per device type (aspeed_2400/2500),

yes. We would need a new class attribute for it. Please use these values, they
should be correct.

            #regs    iosize

AST2400   0x18/4      0x20
AST2500   0x20/4      0x20
AST2600   0x30/4      0x40
AST1030   0x4C/4      0x80


AFAICT, the WDT logic was changed in a compatible way with the previous generation.

Thanks

C.

> while iosize is for all devices, and its initial value comes from the per device type REGS_MAX.
> 
>> registers. The MemoryRegionOps read/write handlers will report the accesses
>> as out-of-bounds guest-errors, but the next commit will report them as
>> unimplemented.
>>
>> [1] https://github.com/AspeedTech-BMC/zephyr/releases/tag/v00.01.07
>> [2] https://github.com/AspeedTech-BMC/zephyr/commit/2e99f10ac27b
>> [3] https://github.com/AspeedTech-
>> BMC/zephyr/blob/v00.01.08/drivers/watchdog/wdt_aspeed.c#L31
>>
>> Reviewed-by: Peter Delevoryas <peter@pjd.dev>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/watchdog/wdt_aspeed.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c index
>> 958725a1b5..eefca31ae4 100644
>> --- a/hw/watchdog/wdt_aspeed.c
>> +++ b/hw/watchdog/wdt_aspeed.c
>> @@ -260,6 +260,7 @@ static void aspeed_wdt_realize(DeviceState *dev,
>> Error **errp)  {
>>       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>       AspeedWDTState *s = ASPEED_WDT(dev);
>> +    AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(dev);
>>
>>       assert(s->scu);
>>
>> @@ -271,7 +272,7 @@ static void aspeed_wdt_realize(DeviceState *dev,
>> Error **errp)
>>       s->pclk_freq = PCLK_HZ;
>>
>>       memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_wdt_ops, s,
>> -                          TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
>> +                          TYPE_ASPEED_WDT, awc->iosize);
>>       sysbus_init_mmio(sbd, &s->iomem);
>>   }
>>
>> --
>> 2.38.1
>>
> 

Re: [PATCH v2 02/11] hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers

[Peter Delevoryas]

On Mon, Jan 02, 2023 at 02:31:31PM +0100, Cédric Le Goater wrote:
> On 12/31/22 23:52, Dong, Eddie wrote:
> > > When booting the Zephyr demo in [1] we get:
> > > 
> > >    aspeed.io: unimplemented device write (size 4, offset 0x185128, value
> > > 0x030f1ff1) <--
> > >    aspeed.io: unimplemented device write (size 4, offset 0x18512c, value
> > > 0x03fffff1)
> > > 
> > > This corresponds to this Zephyr code [2]:
> > > 
> > >    static int aspeed_wdt_init(const struct device *dev)
> > >    {
> > >      const struct aspeed_wdt_config *config = dev->config;
> > >      struct aspeed_wdt_data *const data = dev->data;
> > >      uint32_t reg_val;
> > > 
> > >      /* disable WDT by default */
> > >      reg_val = sys_read32(config->ctrl_base + WDT_CTRL_REG);
> > >      reg_val &= ~WDT_CTRL_ENABLE;
> > >      sys_write32(reg_val, config->ctrl_base + WDT_CTRL_REG);
> > > 
> > >      sys_write32(data->rst_mask1,
> > >                  config->ctrl_base + WDT_SW_RESET_MASK1_REG);   <------
> > >      sys_write32(data->rst_mask2,
> > >                  config->ctrl_base + WDT_SW_RESET_MASK2_REG);
> > > 
> > >      return 0;
> > >    }
> > > 
> > > The register definitions are [3]:
> > > 
> > >    #define WDT_RELOAD_VAL_REG          0x0004
> > >    #define WDT_RESTART_REG             0x0008
> > >    #define WDT_CTRL_REG                0x000C
> > >    #define WDT_TIMEOUT_STATUS_REG      0x0010
> > >    #define WDT_TIMEOUT_STATUS_CLR_REG  0x0014
> > >    #define WDT_RESET_MASK1_REG         0x001C
> > >    #define WDT_RESET_MASK2_REG         0x0020
> > >    #define WDT_SW_RESET_MASK1_REG      0x0028   <------
> > >    #define WDT_SW_RESET_MASK2_REG      0x002C
> > >    #define WDT_SW_RESET_CTRL_REG       0x0024
> > > 
> > > Currently QEMU only cover a MMIO region of size 0x20:
> > > 
> > >    #define ASPEED_WDT_REGS_MAX        (0x20 / 4)
> > > 
> > > Change to map the whole 'iosize' which might be bigger, covering the other
> > 
> > The root cause is that ASPEED_WDT_REGS_MAX is too small, right?
> > Probably the Qemu is emulating an old version of the hardware.
> > 
> > Given the meaning of ASPEED_WDT_REGS_MAX, it should be larger than iosize, not?
> > Probably ASPEED_WDT_REGS_MAX should be per device type (aspeed_2400/2500),
> 
> yes. We would need a new class attribute for it. Please use these values, they
> should be correct.
> 
>            #regs    iosize
> 
> AST2400   0x18/4      0x20
> AST2500   0x20/4      0x20

I think only one additional register was added in the AST2500, bringing it to 0x1C.

> AST2600   0x30/4      0x40
> AST1030   0x4C/4      0x80

I know the Zephyr driver for the AST1030 directly from Aspeed is claiming that
the iosize is 0x80, but the datasheet I have says it's only 0x40. And, that the
#regs would still just be 0x30/4. Afaik the AST2600 and AST1030 should have the
exact same peripheral.

Peter

> 
> 
> AFAICT, the WDT logic was changed in a compatible way with the previous generation.
> 
> Thanks
> 
> C.
> 
> > while iosize is for all devices, and its initial value comes from the per device type REGS_MAX.
> > 
> > > registers. The MemoryRegionOps read/write handlers will report the accesses
> > > as out-of-bounds guest-errors, but the next commit will report them as
> > > unimplemented.
> > > 
> > > [1] https://github.com/AspeedTech-BMC/zephyr/releases/tag/v00.01.07
> > > [2] https://github.com/AspeedTech-BMC/zephyr/commit/2e99f10ac27b
> > > [3] https://github.com/AspeedTech-
> > > BMC/zephyr/blob/v00.01.08/drivers/watchdog/wdt_aspeed.c#L31
> > > 
> > > Reviewed-by: Peter Delevoryas <peter@pjd.dev>
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > >   hw/watchdog/wdt_aspeed.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c index
> > > 958725a1b5..eefca31ae4 100644
> > > --- a/hw/watchdog/wdt_aspeed.c
> > > +++ b/hw/watchdog/wdt_aspeed.c
> > > @@ -260,6 +260,7 @@ static void aspeed_wdt_realize(DeviceState *dev,
> > > Error **errp)  {
> > >       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> > >       AspeedWDTState *s = ASPEED_WDT(dev);
> > > +    AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(dev);
> > > 
> > >       assert(s->scu);
> > > 
> > > @@ -271,7 +272,7 @@ static void aspeed_wdt_realize(DeviceState *dev,
> > > Error **errp)
> > >       s->pclk_freq = PCLK_HZ;
> > > 
> > >       memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_wdt_ops, s,
> > > -                          TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
> > > +                          TYPE_ASPEED_WDT, awc->iosize);
> > >       sysbus_init_mmio(sbd, &s->iomem);
> > >   }
> > > 
> > > --
> > > 2.38.1
> > > 
> > 
> 

Re: [PATCH v2 02/11] hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers

[Cédric Le Goater]

On 1/3/23 16:31, Peter Delevoryas wrote:
> On Mon, Jan 02, 2023 at 02:31:31PM +0100, Cédric Le Goater wrote:
>> On 12/31/22 23:52, Dong, Eddie wrote:
>>>> When booting the Zephyr demo in [1] we get:
>>>>
>>>>     aspeed.io: unimplemented device write (size 4, offset 0x185128, value
>>>> 0x030f1ff1) <--
>>>>     aspeed.io: unimplemented device write (size 4, offset 0x18512c, value
>>>> 0x03fffff1)
>>>>
>>>> This corresponds to this Zephyr code [2]:
>>>>
>>>>     static int aspeed_wdt_init(const struct device *dev)
>>>>     {
>>>>       const struct aspeed_wdt_config *config = dev->config;
>>>>       struct aspeed_wdt_data *const data = dev->data;
>>>>       uint32_t reg_val;
>>>>
>>>>       /* disable WDT by default */
>>>>       reg_val = sys_read32(config->ctrl_base + WDT_CTRL_REG);
>>>>       reg_val &= ~WDT_CTRL_ENABLE;
>>>>       sys_write32(reg_val, config->ctrl_base + WDT_CTRL_REG);
>>>>
>>>>       sys_write32(data->rst_mask1,
>>>>                   config->ctrl_base + WDT_SW_RESET_MASK1_REG);   <------
>>>>       sys_write32(data->rst_mask2,
>>>>                   config->ctrl_base + WDT_SW_RESET_MASK2_REG);
>>>>
>>>>       return 0;
>>>>     }
>>>>
>>>> The register definitions are [3]:
>>>>
>>>>     #define WDT_RELOAD_VAL_REG          0x0004
>>>>     #define WDT_RESTART_REG             0x0008
>>>>     #define WDT_CTRL_REG                0x000C
>>>>     #define WDT_TIMEOUT_STATUS_REG      0x0010
>>>>     #define WDT_TIMEOUT_STATUS_CLR_REG  0x0014
>>>>     #define WDT_RESET_MASK1_REG         0x001C
>>>>     #define WDT_RESET_MASK2_REG         0x0020
>>>>     #define WDT_SW_RESET_MASK1_REG      0x0028   <------
>>>>     #define WDT_SW_RESET_MASK2_REG      0x002C
>>>>     #define WDT_SW_RESET_CTRL_REG       0x0024
>>>>
>>>> Currently QEMU only cover a MMIO region of size 0x20:
>>>>
>>>>     #define ASPEED_WDT_REGS_MAX        (0x20 / 4)
>>>>
>>>> Change to map the whole 'iosize' which might be bigger, covering the other
>>>
>>> The root cause is that ASPEED_WDT_REGS_MAX is too small, right?
>>> Probably the Qemu is emulating an old version of the hardware.
>>>
>>> Given the meaning of ASPEED_WDT_REGS_MAX, it should be larger than iosize, not?
>>> Probably ASPEED_WDT_REGS_MAX should be per device type (aspeed_2400/2500),
>>
>> yes. We would need a new class attribute for it. Please use these values, they
>> should be correct.
>>
>>             #regs    iosize
>>
>> AST2400   0x18/4      0x20
>> AST2500   0x20/4      0x20
> 
> I think only one additional register was added in the AST2500, bringing it to 0x1C.

yes.

> 
>> AST2600   0x30/4      0x40
>> AST1030   0x4C/4      0x80
> 
> I know the Zephyr driver for the AST1030 directly from Aspeed is claiming that
> the iosize is 0x80, but the datasheet I have says it's only 0x40. And, that the
> #regs would still just be 0x30/4. Afaik the AST2600 and AST1030 should have the
> exact same peripheral.

Hmm, I see 5 extra registers in the AST1030 SoC compared to the AST2600 SoC. All
related to write protection.

C.

Re: [PATCH v2 02/11] hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers

[Peter Delevoryas]

On Tue, Jan 03, 2023 at 04:48:14PM +0100, Cédric Le Goater wrote:
> On 1/3/23 16:31, Peter Delevoryas wrote:
> > On Mon, Jan 02, 2023 at 02:31:31PM +0100, Cédric Le Goater wrote:
> > > On 12/31/22 23:52, Dong, Eddie wrote:
> > > > > When booting the Zephyr demo in [1] we get:
> > > > > 
> > > > >     aspeed.io: unimplemented device write (size 4, offset 0x185128, value
> > > > > 0x030f1ff1) <--
> > > > >     aspeed.io: unimplemented device write (size 4, offset 0x18512c, value
> > > > > 0x03fffff1)
> > > > > 
> > > > > This corresponds to this Zephyr code [2]:
> > > > > 
> > > > >     static int aspeed_wdt_init(const struct device *dev)
> > > > >     {
> > > > >       const struct aspeed_wdt_config *config = dev->config;
> > > > >       struct aspeed_wdt_data *const data = dev->data;
> > > > >       uint32_t reg_val;
> > > > > 
> > > > >       /* disable WDT by default */
> > > > >       reg_val = sys_read32(config->ctrl_base + WDT_CTRL_REG);
> > > > >       reg_val &= ~WDT_CTRL_ENABLE;
> > > > >       sys_write32(reg_val, config->ctrl_base + WDT_CTRL_REG);
> > > > > 
> > > > >       sys_write32(data->rst_mask1,
> > > > >                   config->ctrl_base + WDT_SW_RESET_MASK1_REG);   <------
> > > > >       sys_write32(data->rst_mask2,
> > > > >                   config->ctrl_base + WDT_SW_RESET_MASK2_REG);
> > > > > 
> > > > >       return 0;
> > > > >     }
> > > > > 
> > > > > The register definitions are [3]:
> > > > > 
> > > > >     #define WDT_RELOAD_VAL_REG          0x0004
> > > > >     #define WDT_RESTART_REG             0x0008
> > > > >     #define WDT_CTRL_REG                0x000C
> > > > >     #define WDT_TIMEOUT_STATUS_REG      0x0010
> > > > >     #define WDT_TIMEOUT_STATUS_CLR_REG  0x0014
> > > > >     #define WDT_RESET_MASK1_REG         0x001C
> > > > >     #define WDT_RESET_MASK2_REG         0x0020
> > > > >     #define WDT_SW_RESET_MASK1_REG      0x0028   <------
> > > > >     #define WDT_SW_RESET_MASK2_REG      0x002C
> > > > >     #define WDT_SW_RESET_CTRL_REG       0x0024
> > > > > 
> > > > > Currently QEMU only cover a MMIO region of size 0x20:
> > > > > 
> > > > >     #define ASPEED_WDT_REGS_MAX        (0x20 / 4)
> > > > > 
> > > > > Change to map the whole 'iosize' which might be bigger, covering the other
> > > > 
> > > > The root cause is that ASPEED_WDT_REGS_MAX is too small, right?
> > > > Probably the Qemu is emulating an old version of the hardware.
> > > > 
> > > > Given the meaning of ASPEED_WDT_REGS_MAX, it should be larger than iosize, not?
> > > > Probably ASPEED_WDT_REGS_MAX should be per device type (aspeed_2400/2500),
> > > 
> > > yes. We would need a new class attribute for it. Please use these values, they
> > > should be correct.
> > > 
> > >             #regs    iosize
> > > 
> > > AST2400   0x18/4      0x20
> > > AST2500   0x20/4      0x20
> > 
> > I think only one additional register was added in the AST2500, bringing it to 0x1C.
> 
> yes.
> 
> > 
> > > AST2600   0x30/4      0x40
> > > AST1030   0x4C/4      0x80
> > 
> > I know the Zephyr driver for the AST1030 directly from Aspeed is claiming that
> > the iosize is 0x80, but the datasheet I have says it's only 0x40. And, that the
> > #regs would still just be 0x30/4. Afaik the AST2600 and AST1030 should have the
> > exact same peripheral.
> 
> Hmm, I see 5 extra registers in the AST1030 SoC compared to the AST2600 SoC. All
> related to write protection.

Oh really? Hmmm ok, perhaps my datasheet is outdated then, I'm referencing
AST1030 A0 v0.5 from Feb 2021, which might be outdated.

Thanks, sorry for the confusion. Erg.
Peter

> 
> C.
> 

Re: [PATCH v2 02/11] hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers

[Cédric Le Goater]

Philippe,

On 1/2/23 14:31, Cédric Le Goater wrote:
> On 12/31/22 23:52, Dong, Eddie wrote:
>>> When booting the Zephyr demo in [1] we get:
>>>
>>>    aspeed.io: unimplemented device write (size 4, offset 0x185128, value
>>> 0x030f1ff1) <--
>>>    aspeed.io: unimplemented device write (size 4, offset 0x18512c, value
>>> 0x03fffff1)
>>>
>>> This corresponds to this Zephyr code [2]:
>>>
>>>    static int aspeed_wdt_init(const struct device *dev)
>>>    {
>>>      const struct aspeed_wdt_config *config = dev->config;
>>>      struct aspeed_wdt_data *const data = dev->data;
>>>      uint32_t reg_val;
>>>
>>>      /* disable WDT by default */
>>>      reg_val = sys_read32(config->ctrl_base + WDT_CTRL_REG);
>>>      reg_val &= ~WDT_CTRL_ENABLE;
>>>      sys_write32(reg_val, config->ctrl_base + WDT_CTRL_REG);
>>>
>>>      sys_write32(data->rst_mask1,
>>>                  config->ctrl_base + WDT_SW_RESET_MASK1_REG);   <------
>>>      sys_write32(data->rst_mask2,
>>>                  config->ctrl_base + WDT_SW_RESET_MASK2_REG);
>>>
>>>      return 0;
>>>    }
>>>
>>> The register definitions are [3]:
>>>
>>>    #define WDT_RELOAD_VAL_REG          0x0004
>>>    #define WDT_RESTART_REG             0x0008
>>>    #define WDT_CTRL_REG                0x000C
>>>    #define WDT_TIMEOUT_STATUS_REG      0x0010
>>>    #define WDT_TIMEOUT_STATUS_CLR_REG  0x0014
>>>    #define WDT_RESET_MASK1_REG         0x001C
>>>    #define WDT_RESET_MASK2_REG         0x0020
>>>    #define WDT_SW_RESET_MASK1_REG      0x0028   <------
>>>    #define WDT_SW_RESET_MASK2_REG      0x002C
>>>    #define WDT_SW_RESET_CTRL_REG       0x0024
>>>
>>> Currently QEMU only cover a MMIO region of size 0x20:
>>>
>>>    #define ASPEED_WDT_REGS_MAX        (0x20 / 4)
>>>
>>> Change to map the whole 'iosize' which might be bigger, covering the other
>>
>> The root cause is that ASPEED_WDT_REGS_MAX is too small, right?
>> Probably the Qemu is emulating an old version of the hardware.
>>
>> Given the meaning of ASPEED_WDT_REGS_MAX, it should be larger than iosize, not?
>> Probably ASPEED_WDT_REGS_MAX should be per device type (aspeed_2400/2500),
> 
> yes. We would need a new class attribute for it. Please use these values, they
> should be correct.
> 
>             #regs    iosize
> 
> AST2400   0x18/4      0x20
> AST2500   0x20/4      0x20
> AST2600   0x30/4      0x40
> AST1030   0x4C/4      0x80
> 

That might be a big change for the next respin. If you don't have time, we can
make adjustments later. May be add a TODO with the above values ?

Thanks,

C.

  


> AFAICT, the WDT logic was changed in a compatible way with the previous generation.
> 
> Thanks
> 
> C.
> 
>> while iosize is for all devices, and its initial value comes from the per device type REGS_MAX.
>>
>>> registers. The MemoryRegionOps read/write handlers will report the accesses
>>> as out-of-bounds guest-errors, but the next commit will report them as
>>> unimplemented.
>>>
>>> [1] https://github.com/AspeedTech-BMC/zephyr/releases/tag/v00.01.07
>>> [2] https://github.com/AspeedTech-BMC/zephyr/commit/2e99f10ac27b
>>> [3] https://github.com/AspeedTech-
>>> BMC/zephyr/blob/v00.01.08/drivers/watchdog/wdt_aspeed.c#L31
>>>
>>> Reviewed-by: Peter Delevoryas <peter@pjd.dev>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/watchdog/wdt_aspeed.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c index
>>> 958725a1b5..eefca31ae4 100644
>>> --- a/hw/watchdog/wdt_aspeed.c
>>> +++ b/hw/watchdog/wdt_aspeed.c
>>> @@ -260,6 +260,7 @@ static void aspeed_wdt_realize(DeviceState *dev,
>>> Error **errp)  {
>>>       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>>       AspeedWDTState *s = ASPEED_WDT(dev);
>>> +    AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(dev);
>>>
>>>       assert(s->scu);
>>>
>>> @@ -271,7 +272,7 @@ static void aspeed_wdt_realize(DeviceState *dev,
>>> Error **errp)
>>>       s->pclk_freq = PCLK_HZ;
>>>
>>>       memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_wdt_ops, s,
>>> -                          TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
>>> +                          TYPE_ASPEED_WDT, awc->iosize);
>>>       sysbus_init_mmio(sbd, &s->iomem);
>>>   }
>>>
>>> -- 
>>> 2.38.1
>>>
>>
> 

[PATCH v2 03/11] hw/watchdog/wdt_aspeed: Log unimplemented registers as UNIMP level

[Philippe Mathieu-Daudé]

Add more Aspeed watchdog registers from [*].

Since guests can righteously access them, log the access at
'unimplemented' level instead of 'guest-errors'.

[*] https://github.com/AspeedTech-BMC/zephyr/blob/v00.01.08/drivers/watchdog/wdt_aspeed.c#L31

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/watchdog/wdt_aspeed.c         | 13 +++++++++++++
 include/hw/watchdog/wdt_aspeed.h |  2 +-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
index eefca31ae4..d267aa185c 100644
--- a/hw/watchdog/wdt_aspeed.c
+++ b/hw/watchdog/wdt_aspeed.c
@@ -42,6 +42,11 @@
 #define     WDT_PUSH_PULL_MAGIC         (0xA8 << 24)
 #define     WDT_OPEN_DRAIN_MAGIC        (0x8A << 24)
 #define WDT_RESET_MASK1                 (0x1c / 4)
+#define WDT_RESET_MASK2                 (0x20 / 4)
+
+#define WDT_SW_RESET_CTRL               (0x24 / 4)
+#define WDT_SW_RESET_MASK1              (0x28 / 4)
+#define WDT_SW_RESET_MASK2              (0x2c / 4)
 
 #define WDT_TIMEOUT_STATUS              (0x10 / 4)
 #define WDT_TIMEOUT_CLEAR               (0x14 / 4)
@@ -83,6 +88,10 @@ static uint64_t aspeed_wdt_read(void *opaque, hwaddr offset, unsigned size)
         return s->regs[WDT_RESET_MASK1];
     case WDT_TIMEOUT_STATUS:
     case WDT_TIMEOUT_CLEAR:
+    case WDT_RESET_MASK2:
+    case WDT_SW_RESET_CTRL:
+    case WDT_SW_RESET_MASK1:
+    case WDT_SW_RESET_MASK2:
         qemu_log_mask(LOG_UNIMP,
                       "%s: uninmplemented read at offset 0x%" HWADDR_PRIx "\n",
                       __func__, offset);
@@ -190,6 +199,10 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
 
     case WDT_TIMEOUT_STATUS:
     case WDT_TIMEOUT_CLEAR:
+    case WDT_RESET_MASK2:
+    case WDT_SW_RESET_CTRL:
+    case WDT_SW_RESET_MASK1:
+    case WDT_SW_RESET_MASK2:
         qemu_log_mask(LOG_UNIMP,
                       "%s: uninmplemented write at offset 0x%" HWADDR_PRIx "\n",
                       __func__, offset);
diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
index db91ee6b51..e90ef86651 100644
--- a/include/hw/watchdog/wdt_aspeed.h
+++ b/include/hw/watchdog/wdt_aspeed.h
@@ -21,7 +21,7 @@ OBJECT_DECLARE_TYPE(AspeedWDTState, AspeedWDTClass, ASPEED_WDT)
 #define TYPE_ASPEED_2600_WDT TYPE_ASPEED_WDT "-ast2600"
 #define TYPE_ASPEED_1030_WDT TYPE_ASPEED_WDT "-ast1030"
 
-#define ASPEED_WDT_REGS_MAX        (0x20 / 4)
+#define ASPEED_WDT_REGS_MAX        (0x30 / 4)
 
 struct AspeedWDTState {
     /*< private >*/
-- 
2.38.1

Re: [PATCH v2 03/11] hw/watchdog/wdt_aspeed: Log unimplemented registers as UNIMP level

[Peter Delevoryas]

On Fri, Dec 30, 2022 at 12:34:56PM +0100, Philippe Mathieu-Daudé wrote:
> Add more Aspeed watchdog registers from [*].
> 
> Since guests can righteously access them, log the access at
> 'unimplemented' level instead of 'guest-errors'.
> 
> [*] https://github.com/AspeedTech-BMC/zephyr/blob/v00.01.08/drivers/watchdog/wdt_aspeed.c#L31
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Peter Delevoryas <peter@pjd.dev>

> ---
>  hw/watchdog/wdt_aspeed.c         | 13 +++++++++++++
>  include/hw/watchdog/wdt_aspeed.h |  2 +-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> index eefca31ae4..d267aa185c 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -42,6 +42,11 @@
>  #define     WDT_PUSH_PULL_MAGIC         (0xA8 << 24)
>  #define     WDT_OPEN_DRAIN_MAGIC        (0x8A << 24)
>  #define WDT_RESET_MASK1                 (0x1c / 4)
> +#define WDT_RESET_MASK2                 (0x20 / 4)
> +
> +#define WDT_SW_RESET_CTRL               (0x24 / 4)
> +#define WDT_SW_RESET_MASK1              (0x28 / 4)
> +#define WDT_SW_RESET_MASK2              (0x2c / 4)
>  
>  #define WDT_TIMEOUT_STATUS              (0x10 / 4)
>  #define WDT_TIMEOUT_CLEAR               (0x14 / 4)
> @@ -83,6 +88,10 @@ static uint64_t aspeed_wdt_read(void *opaque, hwaddr offset, unsigned size)
>          return s->regs[WDT_RESET_MASK1];
>      case WDT_TIMEOUT_STATUS:
>      case WDT_TIMEOUT_CLEAR:
> +    case WDT_RESET_MASK2:
> +    case WDT_SW_RESET_CTRL:
> +    case WDT_SW_RESET_MASK1:
> +    case WDT_SW_RESET_MASK2:
>          qemu_log_mask(LOG_UNIMP,
>                        "%s: uninmplemented read at offset 0x%" HWADDR_PRIx "\n",
>                        __func__, offset);
> @@ -190,6 +199,10 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
>  
>      case WDT_TIMEOUT_STATUS:
>      case WDT_TIMEOUT_CLEAR:
> +    case WDT_RESET_MASK2:
> +    case WDT_SW_RESET_CTRL:
> +    case WDT_SW_RESET_MASK1:
> +    case WDT_SW_RESET_MASK2:
>          qemu_log_mask(LOG_UNIMP,
>                        "%s: uninmplemented write at offset 0x%" HWADDR_PRIx "\n",
>                        __func__, offset);
> diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
> index db91ee6b51..e90ef86651 100644
> --- a/include/hw/watchdog/wdt_aspeed.h
> +++ b/include/hw/watchdog/wdt_aspeed.h
> @@ -21,7 +21,7 @@ OBJECT_DECLARE_TYPE(AspeedWDTState, AspeedWDTClass, ASPEED_WDT)
>  #define TYPE_ASPEED_2600_WDT TYPE_ASPEED_WDT "-ast2600"
>  #define TYPE_ASPEED_1030_WDT TYPE_ASPEED_WDT "-ast1030"
>  
> -#define ASPEED_WDT_REGS_MAX        (0x20 / 4)
> +#define ASPEED_WDT_REGS_MAX        (0x30 / 4)
>  
>  struct AspeedWDTState {
>      /*< private >*/
> -- 
> 2.38.1
> 

Re: [PATCH v2 03/11] hw/watchdog/wdt_aspeed: Log unimplemented registers as UNIMP level

[Cédric Le Goater]

On 12/30/22 12:34, Philippe Mathieu-Daudé wrote:
> Add more Aspeed watchdog registers from [*].
> 
> Since guests can righteously access them, log the access at
> 'unimplemented' level instead of 'guest-errors'.
> 
> [*] https://github.com/AspeedTech-BMC/zephyr/blob/v00.01.08/drivers/watchdog/wdt_aspeed.c#L31
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

LGTM.

We need to decide how to address the #regs per soc. I would introduce a class
attribute and define ASPEED_WDT_REGS_MAX as the maximum of all, or possibly
allocate the regs array in the realize routine. This is a little more work.

Thanks,

C.



> ---
>   hw/watchdog/wdt_aspeed.c         | 13 +++++++++++++
>   include/hw/watchdog/wdt_aspeed.h |  2 +-
>   2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> index eefca31ae4..d267aa185c 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -42,6 +42,11 @@
>   #define     WDT_PUSH_PULL_MAGIC         (0xA8 << 24)
>   #define     WDT_OPEN_DRAIN_MAGIC        (0x8A << 24)
>   #define WDT_RESET_MASK1                 (0x1c / 4)
> +#define WDT_RESET_MASK2                 (0x20 / 4)
> +
> +#define WDT_SW_RESET_CTRL               (0x24 / 4)
> +#define WDT_SW_RESET_MASK1              (0x28 / 4)
> +#define WDT_SW_RESET_MASK2              (0x2c / 4)
>   
>   #define WDT_TIMEOUT_STATUS              (0x10 / 4)
>   #define WDT_TIMEOUT_CLEAR               (0x14 / 4)
> @@ -83,6 +88,10 @@ static uint64_t aspeed_wdt_read(void *opaque, hwaddr offset, unsigned size)
>           return s->regs[WDT_RESET_MASK1];
>       case WDT_TIMEOUT_STATUS:
>       case WDT_TIMEOUT_CLEAR:
> +    case WDT_RESET_MASK2:
> +    case WDT_SW_RESET_CTRL:
> +    case WDT_SW_RESET_MASK1:
> +    case WDT_SW_RESET_MASK2:
>           qemu_log_mask(LOG_UNIMP,
>                         "%s: uninmplemented read at offset 0x%" HWADDR_PRIx "\n",
>                         __func__, offset);
> @@ -190,6 +199,10 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
>   
>       case WDT_TIMEOUT_STATUS:
>       case WDT_TIMEOUT_CLEAR:
> +    case WDT_RESET_MASK2:
> +    case WDT_SW_RESET_CTRL:
> +    case WDT_SW_RESET_MASK1:
> +    case WDT_SW_RESET_MASK2:
>           qemu_log_mask(LOG_UNIMP,
>                         "%s: uninmplemented write at offset 0x%" HWADDR_PRIx "\n",
>                         __func__, offset);
> diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
> index db91ee6b51..e90ef86651 100644
> --- a/include/hw/watchdog/wdt_aspeed.h
> +++ b/include/hw/watchdog/wdt_aspeed.h
> @@ -21,7 +21,7 @@ OBJECT_DECLARE_TYPE(AspeedWDTState, AspeedWDTClass, ASPEED_WDT)
>   #define TYPE_ASPEED_2600_WDT TYPE_ASPEED_WDT "-ast2600"
>   #define TYPE_ASPEED_1030_WDT TYPE_ASPEED_WDT "-ast1030"
>   
> -#define ASPEED_WDT_REGS_MAX        (0x20 / 4)
> +#define ASPEED_WDT_REGS_MAX        (0x30 / 4)
>   
>   struct AspeedWDTState {
>       /*< private >*/

[PATCH v2 04/11] hw/arm/aspeed: Use the IEC binary prefix definitions

[Philippe Mathieu-Daudé]

IEC binary prefixes ease code review: the unit is explicit.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Delevoryas <peter@pjd.dev>
---
 hw/arm/aspeed_ast10x0.c | 3 ++-
 hw/arm/aspeed_ast2600.c | 3 ++-
 hw/arm/aspeed_soc.c     | 4 ++--
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
index 122b3fd3f3..3500294df7 100644
--- a/hw/arm/aspeed_ast10x0.c
+++ b/hw/arm/aspeed_ast10x0.c
@@ -10,6 +10,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/units.h"
 #include "qapi/error.h"
 #include "exec/address-spaces.h"
 #include "sysemu/sysemu.h"
@@ -348,7 +349,7 @@ static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data)
     sc->name = "ast1030-a1";
     sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-m4");
     sc->silicon_rev = AST1030_A1_SILICON_REV;
-    sc->sram_size = 0xc0000;
+    sc->sram_size = 768 * KiB;
     sc->spis_num = 2;
     sc->ehcis_num = 0;
     sc->wdts_num = 4;
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index a79e05ddbd..72df72a540 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -8,6 +8,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/units.h"
 #include "qapi/error.h"
 #include "hw/misc/unimp.h"
 #include "hw/arm/aspeed_soc.h"
@@ -619,7 +620,7 @@ static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
     sc->name         = "ast2600-a3";
     sc->cpu_type     = ARM_CPU_TYPE_NAME("cortex-a7");
     sc->silicon_rev  = AST2600_A3_SILICON_REV;
-    sc->sram_size    = 0x16400;
+    sc->sram_size    = 89 * KiB;
     sc->spis_num     = 2;
     sc->ehcis_num    = 2;
     sc->wdts_num     = 4;
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 2c0924d311..677342c9ed 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -517,7 +517,7 @@ static void aspeed_soc_ast2400_class_init(ObjectClass *oc, void *data)
     sc->name         = "ast2400-a1";
     sc->cpu_type     = ARM_CPU_TYPE_NAME("arm926");
     sc->silicon_rev  = AST2400_A1_SILICON_REV;
-    sc->sram_size    = 0x8000;
+    sc->sram_size    = 32 * KiB;
     sc->spis_num     = 1;
     sc->ehcis_num    = 1;
     sc->wdts_num     = 2;
@@ -544,7 +544,7 @@ static void aspeed_soc_ast2500_class_init(ObjectClass *oc, void *data)
     sc->name         = "ast2500-a1";
     sc->cpu_type     = ARM_CPU_TYPE_NAME("arm1176");
     sc->silicon_rev  = AST2500_A1_SILICON_REV;
-    sc->sram_size    = 0x9000;
+    sc->sram_size    = 36 * KiB;
     sc->spis_num     = 2;
     sc->ehcis_num    = 2;
     sc->wdts_num     = 3;
-- 
2.38.1

Re: [PATCH v2 04/11] hw/arm/aspeed: Use the IEC binary prefix definitions

[Cédric Le Goater]

On 12/30/22 12:34, Philippe Mathieu-Daudé wrote:
> IEC binary prefixes ease code review: the unit is explicit.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Peter Delevoryas <peter@pjd.dev>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>   hw/arm/aspeed_ast10x0.c | 3 ++-
>   hw/arm/aspeed_ast2600.c | 3 ++-
>   hw/arm/aspeed_soc.c     | 4 ++--
>   3 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> index 122b3fd3f3..3500294df7 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -10,6 +10,7 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qemu/units.h"
>   #include "qapi/error.h"
>   #include "exec/address-spaces.h"
>   #include "sysemu/sysemu.h"
> @@ -348,7 +349,7 @@ static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data)
>       sc->name = "ast1030-a1";
>       sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-m4");
>       sc->silicon_rev = AST1030_A1_SILICON_REV;
> -    sc->sram_size = 0xc0000;
> +    sc->sram_size = 768 * KiB;
>       sc->spis_num = 2;
>       sc->ehcis_num = 0;
>       sc->wdts_num = 4;
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index a79e05ddbd..72df72a540 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -8,6 +8,7 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qemu/units.h"
>   #include "qapi/error.h"
>   #include "hw/misc/unimp.h"
>   #include "hw/arm/aspeed_soc.h"
> @@ -619,7 +620,7 @@ static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
>       sc->name         = "ast2600-a3";
>       sc->cpu_type     = ARM_CPU_TYPE_NAME("cortex-a7");
>       sc->silicon_rev  = AST2600_A3_SILICON_REV;
> -    sc->sram_size    = 0x16400;
> +    sc->sram_size    = 89 * KiB;
>       sc->spis_num     = 2;
>       sc->ehcis_num    = 2;
>       sc->wdts_num     = 4;
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 2c0924d311..677342c9ed 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -517,7 +517,7 @@ static void aspeed_soc_ast2400_class_init(ObjectClass *oc, void *data)
>       sc->name         = "ast2400-a1";
>       sc->cpu_type     = ARM_CPU_TYPE_NAME("arm926");
>       sc->silicon_rev  = AST2400_A1_SILICON_REV;
> -    sc->sram_size    = 0x8000;
> +    sc->sram_size    = 32 * KiB;
>       sc->spis_num     = 1;
>       sc->ehcis_num    = 1;
>       sc->wdts_num     = 2;
> @@ -544,7 +544,7 @@ static void aspeed_soc_ast2500_class_init(ObjectClass *oc, void *data)
>       sc->name         = "ast2500-a1";
>       sc->cpu_type     = ARM_CPU_TYPE_NAME("arm1176");
>       sc->silicon_rev  = AST2500_A1_SILICON_REV;
> -    sc->sram_size    = 0x9000;
> +    sc->sram_size    = 36 * KiB;
>       sc->spis_num     = 2;
>       sc->ehcis_num    = 2;
>       sc->wdts_num     = 3;

Re: [PATCH v2 04/11] hw/arm/aspeed: Use the IEC binary prefix definitions

[Joel Stanley]

On Fri, 30 Dec 2022 at 11:35, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> IEC binary prefixes ease code review: the unit is explicit.

I strongly prefer the existing code; it tells you the size without
having to do maths.

>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Peter Delevoryas <peter@pjd.dev>
> ---
>  hw/arm/aspeed_ast10x0.c | 3 ++-
>  hw/arm/aspeed_ast2600.c | 3 ++-
>  hw/arm/aspeed_soc.c     | 4 ++--
>  3 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> index 122b3fd3f3..3500294df7 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -10,6 +10,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/units.h"
>  #include "qapi/error.h"
>  #include "exec/address-spaces.h"
>  #include "sysemu/sysemu.h"
> @@ -348,7 +349,7 @@ static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data)
>      sc->name = "ast1030-a1";
>      sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-m4");
>      sc->silicon_rev = AST1030_A1_SILICON_REV;
> -    sc->sram_size = 0xc0000;
> +    sc->sram_size = 768 * KiB;
>      sc->spis_num = 2;
>      sc->ehcis_num = 0;
>      sc->wdts_num = 4;
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index a79e05ddbd..72df72a540 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -8,6 +8,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/units.h"
>  #include "qapi/error.h"
>  #include "hw/misc/unimp.h"
>  #include "hw/arm/aspeed_soc.h"
> @@ -619,7 +620,7 @@ static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
>      sc->name         = "ast2600-a3";
>      sc->cpu_type     = ARM_CPU_TYPE_NAME("cortex-a7");
>      sc->silicon_rev  = AST2600_A3_SILICON_REV;
> -    sc->sram_size    = 0x16400;
> +    sc->sram_size    = 89 * KiB;
>      sc->spis_num     = 2;
>      sc->ehcis_num    = 2;
>      sc->wdts_num     = 4;
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 2c0924d311..677342c9ed 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -517,7 +517,7 @@ static void aspeed_soc_ast2400_class_init(ObjectClass *oc, void *data)
>      sc->name         = "ast2400-a1";
>      sc->cpu_type     = ARM_CPU_TYPE_NAME("arm926");
>      sc->silicon_rev  = AST2400_A1_SILICON_REV;
> -    sc->sram_size    = 0x8000;
> +    sc->sram_size    = 32 * KiB;
>      sc->spis_num     = 1;
>      sc->ehcis_num    = 1;
>      sc->wdts_num     = 2;
> @@ -544,7 +544,7 @@ static void aspeed_soc_ast2500_class_init(ObjectClass *oc, void *data)
>      sc->name         = "ast2500-a1";
>      sc->cpu_type     = ARM_CPU_TYPE_NAME("arm1176");
>      sc->silicon_rev  = AST2500_A1_SILICON_REV;
> -    sc->sram_size    = 0x9000;
> +    sc->sram_size    = 36 * KiB;
>      sc->spis_num     = 2;
>      sc->ehcis_num    = 2;
>      sc->wdts_num     = 3;
> --
> 2.38.1
>

Re: [PATCH v2 04/11] hw/arm/aspeed: Use the IEC binary prefix definitions

[Cédric Le Goater]

On 1/18/23 07:53, Joel Stanley wrote:
> On Fri, 30 Dec 2022 at 11:35, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> IEC binary prefixes ease code review: the unit is explicit.
> 
> I strongly prefer the existing code; it tells you the size without
> having to do maths.

you mean that it matches better with the address space representation
in the code and the 'info mtree' output ? If so, I agree. We can keep
this patch out, it is not fundamental.

The hex representation of values has its advantages compared to the
macros because hex is generally what you get in debug outputs and
it is easier to compare and manipulate.  Some Linux dev feel the
same.

C.

> 
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Peter Delevoryas <peter@pjd.dev>
>> ---
>>   hw/arm/aspeed_ast10x0.c | 3 ++-
>>   hw/arm/aspeed_ast2600.c | 3 ++-
>>   hw/arm/aspeed_soc.c     | 4 ++--
>>   3 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
>> index 122b3fd3f3..3500294df7 100644
>> --- a/hw/arm/aspeed_ast10x0.c
>> +++ b/hw/arm/aspeed_ast10x0.c
>> @@ -10,6 +10,7 @@
>>    */
>>
>>   #include "qemu/osdep.h"
>> +#include "qemu/units.h"
>>   #include "qapi/error.h"
>>   #include "exec/address-spaces.h"
>>   #include "sysemu/sysemu.h"
>> @@ -348,7 +349,7 @@ static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data)
>>       sc->name = "ast1030-a1";
>>       sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-m4");
>>       sc->silicon_rev = AST1030_A1_SILICON_REV;
>> -    sc->sram_size = 0xc0000;
>> +    sc->sram_size = 768 * KiB;
>>       sc->spis_num = 2;
>>       sc->ehcis_num = 0;
>>       sc->wdts_num = 4;
>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
>> index a79e05ddbd..72df72a540 100644
>> --- a/hw/arm/aspeed_ast2600.c
>> +++ b/hw/arm/aspeed_ast2600.c
>> @@ -8,6 +8,7 @@
>>    */
>>
>>   #include "qemu/osdep.h"
>> +#include "qemu/units.h"
>>   #include "qapi/error.h"
>>   #include "hw/misc/unimp.h"
>>   #include "hw/arm/aspeed_soc.h"
>> @@ -619,7 +620,7 @@ static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
>>       sc->name         = "ast2600-a3";
>>       sc->cpu_type     = ARM_CPU_TYPE_NAME("cortex-a7");
>>       sc->silicon_rev  = AST2600_A3_SILICON_REV;
>> -    sc->sram_size    = 0x16400;
>> +    sc->sram_size    = 89 * KiB;
>>       sc->spis_num     = 2;
>>       sc->ehcis_num    = 2;
>>       sc->wdts_num     = 4;
>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
>> index 2c0924d311..677342c9ed 100644
>> --- a/hw/arm/aspeed_soc.c
>> +++ b/hw/arm/aspeed_soc.c
>> @@ -517,7 +517,7 @@ static void aspeed_soc_ast2400_class_init(ObjectClass *oc, void *data)
>>       sc->name         = "ast2400-a1";
>>       sc->cpu_type     = ARM_CPU_TYPE_NAME("arm926");
>>       sc->silicon_rev  = AST2400_A1_SILICON_REV;
>> -    sc->sram_size    = 0x8000;
>> +    sc->sram_size    = 32 * KiB;
>>       sc->spis_num     = 1;
>>       sc->ehcis_num    = 1;
>>       sc->wdts_num     = 2;
>> @@ -544,7 +544,7 @@ static void aspeed_soc_ast2500_class_init(ObjectClass *oc, void *data)
>>       sc->name         = "ast2500-a1";
>>       sc->cpu_type     = ARM_CPU_TYPE_NAME("arm1176");
>>       sc->silicon_rev  = AST2500_A1_SILICON_REV;
>> -    sc->sram_size    = 0x9000;
>> +    sc->sram_size    = 36 * KiB;
>>       sc->spis_num     = 2;
>>       sc->ehcis_num    = 2;
>>       sc->wdts_num     = 3;
>> --
>> 2.38.1
>>

Re: [PATCH v2 04/11] hw/arm/aspeed: Use the IEC binary prefix definitions

[Philippe Mathieu-Daudé]

On 18/1/23 08:32, Cédric Le Goater wrote:
> On 1/18/23 07:53, Joel Stanley wrote:
>> On Fri, 30 Dec 2022 at 11:35, Philippe Mathieu-Daudé 
>> <philmd@linaro.org> wrote:
>>>
>>> IEC binary prefixes ease code review: the unit is explicit.
>>
>> I strongly prefer the existing code; it tells you the size without
>> having to do maths.

I guess it depends on one's mindset and culture / education,
maybe I'm too young for the full hexadecimal world and am more
custom to decimal notation with binary prefixes.

0x16400 is just another magic number for me, while 89 * KiB is a size.

> you mean that it matches better with the address space representation
> in the code and the 'info mtree' output ? If so, I agree. We can keep
> this patch out, it is not fundamental.
> 
> The hex representation of values has its advantages compared to the
> macros because hex is generally what you get in debug outputs and
> it is easier to compare and manipulate.  Some Linux dev feel the
> same.

>>> aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
>>>       sc->name         = "ast2600-a3";
>>>       sc->cpu_type     = ARM_CPU_TYPE_NAME("cortex-a7");
>>>       sc->silicon_rev  = AST2600_A3_SILICON_REV;
>>> -    sc->sram_size    = 0x16400;
>>> +    sc->sram_size    = 89 * KiB;
To keep everybody happy I'll respin using:

          sc->sram_size    = 0x16400; /* 89 * KiB */

Thanks,

Phil.

Re: [PATCH v2 04/11] hw/arm/aspeed: Use the IEC binary prefix definitions

[Cédric Le Goater]

On 1/18/23 09:37, Philippe Mathieu-Daudé wrote:
> On 18/1/23 08:32, Cédric Le Goater wrote:
>> On 1/18/23 07:53, Joel Stanley wrote:
>>> On Fri, 30 Dec 2022 at 11:35, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>>>
>>>> IEC binary prefixes ease code review: the unit is explicit.
>>>
>>> I strongly prefer the existing code; it tells you the size without
>>> having to do maths.
> 
> I guess it depends on one's mindset and culture / education,
> maybe I'm too young for the full hexadecimal world and am more
> custom to decimal notation with binary prefixes.
> 
> 0x16400 is just another magic number for me, while 89 * KiB is a size.
> 
>> you mean that it matches better with the address space representation
>> in the code and the 'info mtree' output ? If so, I agree. We can keep
>> this patch out, it is not fundamental.
>>
>> The hex representation of values has its advantages compared to the
>> macros because hex is generally what you get in debug outputs and
>> it is easier to compare and manipulate.  Some Linux dev feel the
>> same.
> 
>>>> aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
>>>>       sc->name         = "ast2600-a3";
>>>>       sc->cpu_type     = ARM_CPU_TYPE_NAME("cortex-a7");
>>>>       sc->silicon_rev  = AST2600_A3_SILICON_REV;
>>>> -    sc->sram_size    = 0x16400;
>>>> +    sc->sram_size    = 89 * KiB;
> To keep everybody happy I'll respin using:
> 
>           sc->sram_size    = 0x16400; /* 89 * KiB */

I had some minor/aesthetic comments. If you could also address them please, I will
include this series in the aspeed-8.0 PR.

Thanks,

C.

[PATCH v2 05/11] hw/misc/aspeed_hace: Do not crash if address_space_map() failed

[Philippe Mathieu-Daudé]

address_space_map() can fail:

  uart:~$ hash test
  sha256_test
  tv[0]:
  Segmentation fault: 11
  Thread 3 "qemu-system-arm" received signal SIGSEGV, Segmentation fault.
  gen_acc_mode_iov (req_len=0x7ffff18b7778, id=<optimized out>, iov=0x7ffff18b7780, s=0x555556ce0bd0)
      at ../hw/misc/aspeed_hace.c:171
  171         if (has_padding(s, &iov[id], *req_len, &total_msg_len, &pad_offset)) {
  (gdb) bt
  #0  gen_acc_mode_iov (req_len=0x7ffff18b7778, id=<optimized out>, iov=0x7ffff18b7780, s=0x555556ce0bd0)
      at ../hw/misc/aspeed_hace.c:171
  #1  do_hash_operation (s=s@entry=0x555556ce0bd0, algo=3, sg_mode=sg_mode@entry=true, acc_mode=acc_mode@entry=true)
      at ../hw/misc/aspeed_hace.c:224
  #2  0x00005555559bdbb8 in aspeed_hace_write (opaque=<optimized out>, addr=12, data=262488, size=<optimized out>)
      at ../hw/misc/aspeed_hace.c:358

This change doesn't fix much, but at least the guest
can't crash QEMU anymore. Instead it is still usable:

  uart:~$ hash test
  sha256_test
  tv[0]:hash_final error
  sha384_test
  tv[0]:hash_final error
  sha512_test
  tv[0]:hash_final error
  [00:00:06.278,000] <err> hace_global: HACE poll timeout
  [00:00:09.324,000] <err> hace_global: HACE poll timeout
  [00:00:12.261,000] <err> hace_global: HACE poll timeout
  uart:~$

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Delevoryas <peter@pjd.dev>
---
 hw/misc/aspeed_hace.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
index ac21be306c..12a761f1f5 100644
--- a/hw/misc/aspeed_hace.c
+++ b/hw/misc/aspeed_hace.c
@@ -193,6 +193,7 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
     size_t digest_len = 0;
     int niov = 0;
     int i;
+    void *haddr;
 
     if (sg_mode) {
         uint32_t len = 0;
@@ -217,9 +218,13 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
             addr &= SG_LIST_ADDR_MASK;
 
             plen = len & SG_LIST_LEN_MASK;
-            iov[i].iov_base = address_space_map(&s->dram_as, addr, &plen, false,
-                                                MEMTXATTRS_UNSPECIFIED);
-
+            haddr = address_space_map(&s->dram_as, addr, &plen, false,
+                                      MEMTXATTRS_UNSPECIFIED);
+            if (haddr == NULL) {
+                qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n", __func__);
+                return;
+            }
+            iov[i].iov_base = haddr;
             if (acc_mode) {
                 niov = gen_acc_mode_iov(s, iov, i, &plen);
 
@@ -230,10 +235,14 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
     } else {
         hwaddr len = s->regs[R_HASH_SRC_LEN];
 
+        haddr = address_space_map(&s->dram_as, s->regs[R_HASH_SRC],
+                                  &len, false, MEMTXATTRS_UNSPECIFIED);
+        if (haddr == NULL) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n", __func__);
+            return;
+        }
+        iov[0].iov_base = haddr;
         iov[0].iov_len = len;
-        iov[0].iov_base = address_space_map(&s->dram_as, s->regs[R_HASH_SRC],
-                                            &len, false,
-                                            MEMTXATTRS_UNSPECIFIED);
         i = 1;
 
         if (s->iov_count) {
-- 
2.38.1

Re: [PATCH v2 05/11] hw/misc/aspeed_hace: Do not crash if address_space_map() failed

[Cédric Le Goater]

On 12/30/22 12:34, Philippe Mathieu-Daudé wrote:
> address_space_map() can fail:
> 
>    uart:~$ hash test
>    sha256_test
>    tv[0]:
>    Segmentation fault: 11
>    Thread 3 "qemu-system-arm" received signal SIGSEGV, Segmentation fault.
>    gen_acc_mode_iov (req_len=0x7ffff18b7778, id=<optimized out>, iov=0x7ffff18b7780, s=0x555556ce0bd0)
>        at ../hw/misc/aspeed_hace.c:171
>    171         if (has_padding(s, &iov[id], *req_len, &total_msg_len, &pad_offset)) {
>    (gdb) bt
>    #0  gen_acc_mode_iov (req_len=0x7ffff18b7778, id=<optimized out>, iov=0x7ffff18b7780, s=0x555556ce0bd0)
>        at ../hw/misc/aspeed_hace.c:171
>    #1  do_hash_operation (s=s@entry=0x555556ce0bd0, algo=3, sg_mode=sg_mode@entry=true, acc_mode=acc_mode@entry=true)
>        at ../hw/misc/aspeed_hace.c:224
>    #2  0x00005555559bdbb8 in aspeed_hace_write (opaque=<optimized out>, addr=12, data=262488, size=<optimized out>)
>        at ../hw/misc/aspeed_hace.c:358
> 
> This change doesn't fix much, but at least the guest
> can't crash QEMU anymore. Instead it is still usable:
> 
>    uart:~$ hash test
>    sha256_test
>    tv[0]:hash_final error
>    sha384_test
>    tv[0]:hash_final error
>    sha512_test
>    tv[0]:hash_final error
>    [00:00:06.278,000] <err> hace_global: HACE poll timeout
>    [00:00:09.324,000] <err> hace_global: HACE poll timeout
>    [00:00:12.261,000] <err> hace_global: HACE poll timeout
>    uart:~$
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Peter Delevoryas <peter@pjd.dev>

This is a tough model. I am glad you are taking a look at it.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>   hw/misc/aspeed_hace.c | 21 +++++++++++++++------
>   1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
> index ac21be306c..12a761f1f5 100644
> --- a/hw/misc/aspeed_hace.c
> +++ b/hw/misc/aspeed_hace.c
> @@ -193,6 +193,7 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
>       size_t digest_len = 0;
>       int niov = 0;
>       int i;
> +    void *haddr;
>   
>       if (sg_mode) {
>           uint32_t len = 0;
> @@ -217,9 +218,13 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
>               addr &= SG_LIST_ADDR_MASK;
>   
>               plen = len & SG_LIST_LEN_MASK;
> -            iov[i].iov_base = address_space_map(&s->dram_as, addr, &plen, false,
> -                                                MEMTXATTRS_UNSPECIFIED);
> -
> +            haddr = address_space_map(&s->dram_as, addr, &plen, false,
> +                                      MEMTXATTRS_UNSPECIFIED);
> +            if (haddr == NULL) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n", __func__);
> +                return;
> +            }
> +            iov[i].iov_base = haddr;
>               if (acc_mode) {
>                   niov = gen_acc_mode_iov(s, iov, i, &plen);
>   
> @@ -230,10 +235,14 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
>       } else {
>           hwaddr len = s->regs[R_HASH_SRC_LEN];
>   
> +        haddr = address_space_map(&s->dram_as, s->regs[R_HASH_SRC],
> +                                  &len, false, MEMTXATTRS_UNSPECIFIED);
> +        if (haddr == NULL) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n", __func__);
> +            return;
> +        }
> +        iov[0].iov_base = haddr;
>           iov[0].iov_len = len;
> -        iov[0].iov_base = address_space_map(&s->dram_as, s->regs[R_HASH_SRC],
> -                                            &len, false,
> -                                            MEMTXATTRS_UNSPECIFIED);
>           i = 1;
>   
>           if (s->iov_count) {

[PATCH v2 06/11] hw/arm/aspeed_ast10x0: Add various unimplemented peripherals

[Philippe Mathieu-Daudé]

Based on booting Zephyr demo from [1] running QEMU with
'-d unimp' and checking missing devices in [2].

[1] https://github.com/AspeedTech-BMC/zephyr/releases/tag/v00.01.07
[2] https://github.com/AspeedTech-BMC/zephyr/blob/v00.01.08/dts/arm/aspeed/ast10x0.dtsi

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Delevoryas <peter@pjd.dev>
---
 hw/arm/aspeed_ast10x0.c     | 35 +++++++++++++++++++++++++++++++++++
 include/hw/arm/aspeed_soc.h | 11 +++++++++++
 2 files changed, 46 insertions(+)

diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
index 3500294df7..d7dbc1a801 100644
--- a/hw/arm/aspeed_ast10x0.c
+++ b/hw/arm/aspeed_ast10x0.c
@@ -28,10 +28,15 @@ static const hwaddr aspeed_soc_ast1030_memmap[] = {
     [ASPEED_DEV_FMC]       = 0x7E620000,
     [ASPEED_DEV_SPI1]      = 0x7E630000,
     [ASPEED_DEV_SPI2]      = 0x7E640000,
+    [ASPEED_DEV_UDC]       = 0x7E6A2000,
     [ASPEED_DEV_SCU]       = 0x7E6E2000,
+    [ASPEED_DEV_JTAG0]     = 0x7E6E4000,
+    [ASPEED_DEV_JTAG1]     = 0x7E6E4100,
     [ASPEED_DEV_ADC]       = 0x7E6E9000,
+    [ASPEED_DEV_ESPI]      = 0x7E6EE000,
     [ASPEED_DEV_SBC]       = 0x7E6F2000,
     [ASPEED_DEV_GPIO]      = 0x7E780000,
+    [ASPEED_DEV_SGPIOM]    = 0x7E780500,
     [ASPEED_DEV_TIMER1]    = 0x7E782000,
     [ASPEED_DEV_UART1]     = 0x7E783000,
     [ASPEED_DEV_UART2]     = 0x7E78D000,
@@ -79,12 +84,17 @@ static const int aspeed_soc_ast1030_irqmap[] = {
     [ASPEED_DEV_LPC]       = 35,
     [ASPEED_DEV_PECI]      = 38,
     [ASPEED_DEV_FMC]       = 39,
+    [ASPEED_DEV_ESPI]      = 42,
     [ASPEED_DEV_PWM]       = 44,
     [ASPEED_DEV_ADC]       = 46,
     [ASPEED_DEV_SPI1]      = 65,
     [ASPEED_DEV_SPI2]      = 66,
     [ASPEED_DEV_I2C]       = 110, /* 110 ~ 123 */
     [ASPEED_DEV_KCS]       = 138, /* 138 -> 142 */
+    [ASPEED_DEV_UDC]       = 9,
+    [ASPEED_DEV_SGPIOM]    = 51,
+    [ASPEED_DEV_JTAG0]     = 27,
+    [ASPEED_DEV_JTAG1]     = 53,
 };
 
 static qemu_irq aspeed_soc_ast1030_get_irq(AspeedSoCState *s, int dev)
@@ -155,6 +165,15 @@ static void aspeed_soc_ast1030_init(Object *obj)
     object_initialize_child(obj, "iomem", &s->iomem, TYPE_UNIMPLEMENTED_DEVICE);
     object_initialize_child(obj, "sbc-unimplemented", &s->sbc_unimplemented,
                             TYPE_UNIMPLEMENTED_DEVICE);
+    object_initialize_child(obj, "pwm", &s->pwm, TYPE_UNIMPLEMENTED_DEVICE);
+    object_initialize_child(obj, "espi", &s->espi, TYPE_UNIMPLEMENTED_DEVICE);
+    object_initialize_child(obj, "udc", &s->udc, TYPE_UNIMPLEMENTED_DEVICE);
+    object_initialize_child(obj, "sgpiom", &s->sgpiom,
+                            TYPE_UNIMPLEMENTED_DEVICE);
+    object_initialize_child(obj, "jtag[0]", &s->jtag[0],
+                            TYPE_UNIMPLEMENTED_DEVICE);
+    object_initialize_child(obj, "jtag[1]", &s->jtag[1],
+                            TYPE_UNIMPLEMENTED_DEVICE);
 }
 
 static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
@@ -337,6 +356,22 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
                     sc->memmap[ASPEED_DEV_GPIO]);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->gpio), 0,
                        aspeed_soc_get_irq(s, ASPEED_DEV_GPIO));
+
+    aspeed_mmio_map_unimplemented(s, SYS_BUS_DEVICE(&s->pwm), "aspeed.pwm",
+                                  sc->memmap[ASPEED_DEV_PWM], 0x100);
+
+    aspeed_mmio_map_unimplemented(s, SYS_BUS_DEVICE(&s->espi), "aspeed.espi",
+                                  sc->memmap[ASPEED_DEV_ESPI], 0x800);
+
+    aspeed_mmio_map_unimplemented(s, SYS_BUS_DEVICE(&s->udc), "aspeed.udc",
+                                  sc->memmap[ASPEED_DEV_UDC], 0x1000);
+    aspeed_mmio_map_unimplemented(s, SYS_BUS_DEVICE(&s->sgpiom), "aspeed.sgpiom",
+                                  sc->memmap[ASPEED_DEV_SGPIOM], 0x100);
+
+    aspeed_mmio_map_unimplemented(s, SYS_BUS_DEVICE(&s->jtag[0]), "aspeed.jtag",
+                                  sc->memmap[ASPEED_DEV_JTAG0], 0x20);
+    aspeed_mmio_map_unimplemented(s, SYS_BUS_DEVICE(&s->jtag[1]), "aspeed.jtag",
+                                  sc->memmap[ASPEED_DEV_JTAG1], 0x20);
 }
 
 static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 8389200b2d..9a5e3c0bac 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -44,6 +44,7 @@
 #define ASPEED_CPUS_NUM  2
 #define ASPEED_MACS_NUM  4
 #define ASPEED_UARTS_NUM 13
+#define ASPEED_JTAG_NUM  2
 
 struct AspeedSoCState {
     /*< private >*/
@@ -87,6 +88,11 @@ struct AspeedSoCState {
     UnimplementedDeviceState video;
     UnimplementedDeviceState emmc_boot_controller;
     UnimplementedDeviceState dpmcu;
+    UnimplementedDeviceState pwm;
+    UnimplementedDeviceState espi;
+    UnimplementedDeviceState udc;
+    UnimplementedDeviceState sgpiom;
+    UnimplementedDeviceState jtag[ASPEED_JTAG_NUM];
 };
 
 #define TYPE_ASPEED_SOC "aspeed-soc"
@@ -174,6 +180,11 @@ enum {
     ASPEED_DEV_DPMCU,
     ASPEED_DEV_DP,
     ASPEED_DEV_I3C,
+    ASPEED_DEV_ESPI,
+    ASPEED_DEV_UDC,
+    ASPEED_DEV_SGPIOM,
+    ASPEED_DEV_JTAG0,
+    ASPEED_DEV_JTAG1,
 };
 
 qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int dev);
-- 
2.38.1

Re: [PATCH v2 06/11] hw/arm/aspeed_ast10x0: Add various unimplemented peripherals

[Cédric Le Goater]

On 12/30/22 12:34, Philippe Mathieu-Daudé wrote:
> Based on booting Zephyr demo from [1] running QEMU with
> '-d unimp' and checking missing devices in [2].
> 
> [1] https://github.com/AspeedTech-BMC/zephyr/releases/tag/v00.01.07
> [2] https://github.com/AspeedTech-BMC/zephyr/blob/v00.01.08/dts/arm/aspeed/ast10x0.dtsi
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Peter Delevoryas <peter@pjd.dev>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> ---
>   hw/arm/aspeed_ast10x0.c     | 35 +++++++++++++++++++++++++++++++++++
>   include/hw/arm/aspeed_soc.h | 11 +++++++++++
>   2 files changed, 46 insertions(+)
> 
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> index 3500294df7..d7dbc1a801 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -28,10 +28,15 @@ static const hwaddr aspeed_soc_ast1030_memmap[] = {
>       [ASPEED_DEV_FMC]       = 0x7E620000,
>       [ASPEED_DEV_SPI1]      = 0x7E630000,
>       [ASPEED_DEV_SPI2]      = 0x7E640000,
> +    [ASPEED_DEV_UDC]       = 0x7E6A2000,
>       [ASPEED_DEV_SCU]       = 0x7E6E2000,
> +    [ASPEED_DEV_JTAG0]     = 0x7E6E4000,
> +    [ASPEED_DEV_JTAG1]     = 0x7E6E4100,
>       [ASPEED_DEV_ADC]       = 0x7E6E9000,
> +    [ASPEED_DEV_ESPI]      = 0x7E6EE000,
>       [ASPEED_DEV_SBC]       = 0x7E6F2000,
>       [ASPEED_DEV_GPIO]      = 0x7E780000,
> +    [ASPEED_DEV_SGPIOM]    = 0x7E780500,
>       [ASPEED_DEV_TIMER1]    = 0x7E782000,
>       [ASPEED_DEV_UART1]     = 0x7E783000,
>       [ASPEED_DEV_UART2]     = 0x7E78D000,
> @@ -79,12 +84,17 @@ static const int aspeed_soc_ast1030_irqmap[] = {
>       [ASPEED_DEV_LPC]       = 35,
>       [ASPEED_DEV_PECI]      = 38,
>       [ASPEED_DEV_FMC]       = 39,
> +    [ASPEED_DEV_ESPI]      = 42,
>       [ASPEED_DEV_PWM]       = 44,
>       [ASPEED_DEV_ADC]       = 46,
>       [ASPEED_DEV_SPI1]      = 65,
>       [ASPEED_DEV_SPI2]      = 66,
>       [ASPEED_DEV_I2C]       = 110, /* 110 ~ 123 */
>       [ASPEED_DEV_KCS]       = 138, /* 138 -> 142 */
> +    [ASPEED_DEV_UDC]       = 9,
> +    [ASPEED_DEV_SGPIOM]    = 51,
> +    [ASPEED_DEV_JTAG0]     = 27,
> +    [ASPEED_DEV_JTAG1]     = 53,
>   };
>   
>   static qemu_irq aspeed_soc_ast1030_get_irq(AspeedSoCState *s, int dev)
> @@ -155,6 +165,15 @@ static void aspeed_soc_ast1030_init(Object *obj)
>       object_initialize_child(obj, "iomem", &s->iomem, TYPE_UNIMPLEMENTED_DEVICE);
>       object_initialize_child(obj, "sbc-unimplemented", &s->sbc_unimplemented,
>                               TYPE_UNIMPLEMENTED_DEVICE);
> +    object_initialize_child(obj, "pwm", &s->pwm, TYPE_UNIMPLEMENTED_DEVICE);
> +    object_initialize_child(obj, "espi", &s->espi, TYPE_UNIMPLEMENTED_DEVICE);
> +    object_initialize_child(obj, "udc", &s->udc, TYPE_UNIMPLEMENTED_DEVICE);
> +    object_initialize_child(obj, "sgpiom", &s->sgpiom,
> +                            TYPE_UNIMPLEMENTED_DEVICE);
> +    object_initialize_child(obj, "jtag[0]", &s->jtag[0],
> +                            TYPE_UNIMPLEMENTED_DEVICE);
> +    object_initialize_child(obj, "jtag[1]", &s->jtag[1],
> +                            TYPE_UNIMPLEMENTED_DEVICE);
>   }
>   
>   static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
> @@ -337,6 +356,22 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
>                       sc->memmap[ASPEED_DEV_GPIO]);
>       sysbus_connect_irq(SYS_BUS_DEVICE(&s->gpio), 0,
>                          aspeed_soc_get_irq(s, ASPEED_DEV_GPIO));
> +
> +    aspeed_mmio_map_unimplemented(s, SYS_BUS_DEVICE(&s->pwm), "aspeed.pwm",
> +                                  sc->memmap[ASPEED_DEV_PWM], 0x100);
> +
> +    aspeed_mmio_map_unimplemented(s, SYS_BUS_DEVICE(&s->espi), "aspeed.espi",
> +                                  sc->memmap[ASPEED_DEV_ESPI], 0x800);
> +
> +    aspeed_mmio_map_unimplemented(s, SYS_BUS_DEVICE(&s->udc), "aspeed.udc",
> +                                  sc->memmap[ASPEED_DEV_UDC], 0x1000);
> +    aspeed_mmio_map_unimplemented(s, SYS_BUS_DEVICE(&s->sgpiom), "aspeed.sgpiom",
> +                                  sc->memmap[ASPEED_DEV_SGPIOM], 0x100);
> +
> +    aspeed_mmio_map_unimplemented(s, SYS_BUS_DEVICE(&s->jtag[0]), "aspeed.jtag",
> +                                  sc->memmap[ASPEED_DEV_JTAG0], 0x20);
> +    aspeed_mmio_map_unimplemented(s, SYS_BUS_DEVICE(&s->jtag[1]), "aspeed.jtag",
> +                                  sc->memmap[ASPEED_DEV_JTAG1], 0x20);
>   }
>   
>   static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data)
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index 8389200b2d..9a5e3c0bac 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -44,6 +44,7 @@
>   #define ASPEED_CPUS_NUM  2
>   #define ASPEED_MACS_NUM  4
>   #define ASPEED_UARTS_NUM 13
> +#define ASPEED_JTAG_NUM  2
>   
>   struct AspeedSoCState {
>       /*< private >*/
> @@ -87,6 +88,11 @@ struct AspeedSoCState {
>       UnimplementedDeviceState video;
>       UnimplementedDeviceState emmc_boot_controller;
>       UnimplementedDeviceState dpmcu;
> +    UnimplementedDeviceState pwm;
> +    UnimplementedDeviceState espi;
> +    UnimplementedDeviceState udc;
> +    UnimplementedDeviceState sgpiom;
> +    UnimplementedDeviceState jtag[ASPEED_JTAG_NUM];
>   };
>   
>   #define TYPE_ASPEED_SOC "aspeed-soc"
> @@ -174,6 +180,11 @@ enum {
>       ASPEED_DEV_DPMCU,
>       ASPEED_DEV_DP,
>       ASPEED_DEV_I3C,
> +    ASPEED_DEV_ESPI,
> +    ASPEED_DEV_UDC,
> +    ASPEED_DEV_SGPIOM,
> +    ASPEED_DEV_JTAG0,
> +    ASPEED_DEV_JTAG1,
>   };
>   
>   qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int dev);

[PATCH v2 07/11] hw/arm/aspeed_ast10x0: Map I3C peripheral

[Philippe Mathieu-Daudé]

Since I don't have access to the datasheet, the relevant
values were found in:
https://github.com/AspeedTech-BMC/zephyr/blob/v00.01.08/dts/arm/aspeed/ast10x0.dtsi

Reviewed-by: Peter Delevoryas <peter@pjd.dev>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/aspeed_ast10x0.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
index d7dbc1a801..6c6d33d4a0 100644
--- a/hw/arm/aspeed_ast10x0.c
+++ b/hw/arm/aspeed_ast10x0.c
@@ -54,6 +54,7 @@ static const hwaddr aspeed_soc_ast1030_memmap[] = {
     [ASPEED_DEV_WDT]       = 0x7E785000,
     [ASPEED_DEV_LPC]       = 0x7E789000,
     [ASPEED_DEV_PECI]      = 0x7E78B000,
+    [ASPEED_DEV_I3C]       = 0x7E7A0000,
     [ASPEED_DEV_I2C]       = 0x7E7B0000,
 };
 
@@ -89,6 +90,7 @@ static const int aspeed_soc_ast1030_irqmap[] = {
     [ASPEED_DEV_ADC]       = 46,
     [ASPEED_DEV_SPI1]      = 65,
     [ASPEED_DEV_SPI2]      = 66,
+    [ASPEED_DEV_I3C]       = 102, /* 102 -> 105 */
     [ASPEED_DEV_I2C]       = 110, /* 110 ~ 123 */
     [ASPEED_DEV_KCS]       = 138, /* 138 -> 142 */
     [ASPEED_DEV_UDC]       = 9,
@@ -130,6 +132,8 @@ static void aspeed_soc_ast1030_init(Object *obj)
     snprintf(typename, sizeof(typename), "aspeed.i2c-%s", socname);
     object_initialize_child(obj, "i2c", &s->i2c, typename);
 
+    object_initialize_child(obj, "i3c", &s->i3c, TYPE_ASPEED_I3C);
+
     snprintf(typename, sizeof(typename), "aspeed.timer-%s", socname);
     object_initialize_child(obj, "timerctrl", &s->timerctrl, typename);
 
@@ -240,6 +244,18 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c.busses[i]), 0, irq);
     }
 
+    /* I3C */
+    if (!sysbus_realize(SYS_BUS_DEVICE(&s->i3c), errp)) {
+        return;
+    }
+    aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->i3c), 0, sc->memmap[ASPEED_DEV_I3C]);
+    for (i = 0; i < ASPEED_I3C_NR_DEVICES; i++) {
+        qemu_irq irq = qdev_get_gpio_in(DEVICE(&s->armv7m),
+                                        sc->irqmap[ASPEED_DEV_I3C] + i);
+        /* The AST1030 I3C controller has one IRQ per bus. */
+        sysbus_connect_irq(SYS_BUS_DEVICE(&s->i3c.devices[i]), 0, irq);
+    }
+
     /* PECI */
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->peci), errp)) {
         return;
-- 
2.38.1

Re: [PATCH v2 07/11] hw/arm/aspeed_ast10x0: Map I3C peripheral

[Cédric Le Goater]

On 12/30/22 12:35, Philippe Mathieu-Daudé wrote:
> Since I don't have access to the datasheet, the relevant
> values were found in:
> https://github.com/AspeedTech-BMC/zephyr/blob/v00.01.08/dts/arm/aspeed/ast10x0.dtsi
> 
> Reviewed-by: Peter Delevoryas <peter@pjd.dev>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
I3C is really a dummy model. I hope we can grow the modeling with time and add some
device models.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>   hw/arm/aspeed_ast10x0.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> index d7dbc1a801..6c6d33d4a0 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -54,6 +54,7 @@ static const hwaddr aspeed_soc_ast1030_memmap[] = {
>       [ASPEED_DEV_WDT]       = 0x7E785000,
>       [ASPEED_DEV_LPC]       = 0x7E789000,
>       [ASPEED_DEV_PECI]      = 0x7E78B000,
> +    [ASPEED_DEV_I3C]       = 0x7E7A0000,
>       [ASPEED_DEV_I2C]       = 0x7E7B0000,
>   };
>   
> @@ -89,6 +90,7 @@ static const int aspeed_soc_ast1030_irqmap[] = {
>       [ASPEED_DEV_ADC]       = 46,
>       [ASPEED_DEV_SPI1]      = 65,
>       [ASPEED_DEV_SPI2]      = 66,
> +    [ASPEED_DEV_I3C]       = 102, /* 102 -> 105 */
>       [ASPEED_DEV_I2C]       = 110, /* 110 ~ 123 */
>       [ASPEED_DEV_KCS]       = 138, /* 138 -> 142 */
>       [ASPEED_DEV_UDC]       = 9,
> @@ -130,6 +132,8 @@ static void aspeed_soc_ast1030_init(Object *obj)
>       snprintf(typename, sizeof(typename), "aspeed.i2c-%s", socname);
>       object_initialize_child(obj, "i2c", &s->i2c, typename);
>   
> +    object_initialize_child(obj, "i3c", &s->i3c, TYPE_ASPEED_I3C);
> +
>       snprintf(typename, sizeof(typename), "aspeed.timer-%s", socname);
>       object_initialize_child(obj, "timerctrl", &s->timerctrl, typename);
>   
> @@ -240,6 +244,18 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
>           sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c.busses[i]), 0, irq);
>       }
>   
> +    /* I3C */
> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->i3c), errp)) {
> +        return;
> +    }
> +    aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->i3c), 0, sc->memmap[ASPEED_DEV_I3C]);
> +    for (i = 0; i < ASPEED_I3C_NR_DEVICES; i++) {
> +        qemu_irq irq = qdev_get_gpio_in(DEVICE(&s->armv7m),
> +                                        sc->irqmap[ASPEED_DEV_I3C] + i);
> +        /* The AST1030 I3C controller has one IRQ per bus. */
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->i3c.devices[i]), 0, irq);
> +    }
> +
>       /* PECI */
>       if (!sysbus_realize(SYS_BUS_DEVICE(&s->peci), errp)) {
>           return;

[PATCH v2 08/11] hw/arm/aspeed_ast10x0: Map the secure SRAM

[Philippe Mathieu-Daudé]

Some SRAM appears to be used by the Secure Boot unit and
crypto accelerators. Name it 'secure sram'.

Note, the SRAM base address was already present but unused
(the 'SBC' index is used for the MMIO peripheral).

Interestingly using CFLAGS=-Winitializer-overrides reports:

  ../hw/arm/aspeed_ast10x0.c:32:30: warning: initializer overrides prior initialization of this subobject [-Winitializer-overrides]
    [ASPEED_DEV_SBC]       = 0x7E6F2000,
                             ^~~~~~~~~~
  ../hw/arm/aspeed_ast10x0.c:24:30: note: previous initialization is here
    [ASPEED_DEV_SBC]       = 0x79000000,
                             ^~~~~~~~~~
This fixes with Zephyr:

  uart:~$ rsa test
  rsa test vector[0]:
  [00:00:26.156,000] <err> os: ***** BUS FAULT *****
  [00:00:26.157,000] <err> os:   Precise data bus error
  [00:00:26.157,000] <err> os:   BFAR Address: 0x79000000
  [00:00:26.158,000] <err> os: r0/a1:  0x79000000  r1/a2:  0x00000000  r2/a3:  0x00001800
  [00:00:26.158,000] <err> os: r3/a4:  0x79001800 r12/ip:  0x00000800 r14/lr:  0x0001098d
  [00:00:26.158,000] <err> os:  xpsr:  0x81000000
  [00:00:26.158,000] <err> os: Faulting instruction address (r15/pc): 0x0001e1bc
  [00:00:26.158,000] <err> os: >>> ZEPHYR FATAL ERROR 0: CPU exception on CPU 0
  [00:00:26.158,000] <err> os: Current thread: 0x38248 (shell_uart)
  [00:00:26.165,000] <err> os: Halting system

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Delevoryas <peter@pjd.dev>
---
 hw/arm/aspeed_ast10x0.c     | 11 ++++++++++-
 include/hw/arm/aspeed_soc.h |  3 +++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
index 6c6d33d4a0..e74e2660ce 100644
--- a/hw/arm/aspeed_ast10x0.c
+++ b/hw/arm/aspeed_ast10x0.c
@@ -22,7 +22,7 @@
 
 static const hwaddr aspeed_soc_ast1030_memmap[] = {
     [ASPEED_DEV_SRAM]      = 0x00000000,
-    [ASPEED_DEV_SBC]       = 0x79000000,
+    [ASPEED_DEV_SECSRAM]   = 0x79000000,
     [ASPEED_DEV_IOMEM]     = 0x7E600000,
     [ASPEED_DEV_PWM]       = 0x7E610000,
     [ASPEED_DEV_FMC]       = 0x7E620000,
@@ -222,6 +222,14 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
     memory_region_add_subregion(s->memory,
                                 sc->memmap[ASPEED_DEV_SRAM],
                                 &s->sram);
+    memory_region_init_ram(&s->secsram, OBJECT(s), "sec.sram",
+                           sc->secsram_size, &err);
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return;
+    }
+    memory_region_add_subregion(s->memory, sc->memmap[ASPEED_DEV_SECSRAM],
+                                &s->secsram);
 
     /* SCU */
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->scu), errp)) {
@@ -401,6 +409,7 @@ static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data)
     sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-m4");
     sc->silicon_rev = AST1030_A1_SILICON_REV;
     sc->sram_size = 768 * KiB;
+    sc->secsram_size = 9 * KiB;
     sc->spis_num = 2;
     sc->ehcis_num = 0;
     sc->wdts_num = 4;
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 9a5e3c0bac..bd1e03e78a 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -71,6 +71,7 @@ struct AspeedSoCState {
     AspeedSMCState spi[ASPEED_SPIS_NUM];
     EHCISysBusState ehci[ASPEED_EHCIS_NUM];
     AspeedSBCState sbc;
+    MemoryRegion secsram;
     UnimplementedDeviceState sbc_unimplemented;
     AspeedSDMCState sdmc;
     AspeedWDTState wdt[ASPEED_WDTS_NUM];
@@ -105,6 +106,7 @@ struct AspeedSoCClass {
     const char *cpu_type;
     uint32_t silicon_rev;
     uint64_t sram_size;
+    uint64_t secsram_size;
     int spis_num;
     int ehcis_num;
     int wdts_num;
@@ -143,6 +145,7 @@ enum {
     ASPEED_DEV_SCU,
     ASPEED_DEV_ADC,
     ASPEED_DEV_SBC,
+    ASPEED_DEV_SECSRAM,
     ASPEED_DEV_EMMC_BC,
     ASPEED_DEV_VIDEO,
     ASPEED_DEV_SRAM,
-- 
2.38.1

Re: [PATCH v2 08/11] hw/arm/aspeed_ast10x0: Map the secure SRAM

[Cédric Le Goater]

On 12/30/22 12:35, Philippe Mathieu-Daudé wrote:
> Some SRAM appears to be used by the Secure Boot unit and
> crypto accelerators. Name it 'secure sram'.
> 
> Note, the SRAM base address was already present but unused
> (the 'SBC' index is used for the MMIO peripheral).
> 
> Interestingly using CFLAGS=-Winitializer-overrides reports:
> 
>    ../hw/arm/aspeed_ast10x0.c:32:30: warning: initializer overrides prior initialization of this subobject [-Winitializer-overrides]
>      [ASPEED_DEV_SBC]       = 0x7E6F2000,
>                               ^~~~~~~~~~
>    ../hw/arm/aspeed_ast10x0.c:24:30: note: previous initialization is here
>      [ASPEED_DEV_SBC]       = 0x79000000,
>                               ^~~~~~~~~~
> This fixes with Zephyr:
> 
>    uart:~$ rsa test
>    rsa test vector[0]:
>    [00:00:26.156,000] <err> os: ***** BUS FAULT *****
>    [00:00:26.157,000] <err> os:   Precise data bus error
>    [00:00:26.157,000] <err> os:   BFAR Address: 0x79000000
>    [00:00:26.158,000] <err> os: r0/a1:  0x79000000  r1/a2:  0x00000000  r2/a3:  0x00001800
>    [00:00:26.158,000] <err> os: r3/a4:  0x79001800 r12/ip:  0x00000800 r14/lr:  0x0001098d
>    [00:00:26.158,000] <err> os:  xpsr:  0x81000000
>    [00:00:26.158,000] <err> os: Faulting instruction address (r15/pc): 0x0001e1bc
>    [00:00:26.158,000] <err> os: >>> ZEPHYR FATAL ERROR 0: CPU exception on CPU 0
>    [00:00:26.158,000] <err> os: Current thread: 0x38248 (shell_uart)
>    [00:00:26.165,000] <err> os: Halting system
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Peter Delevoryas <peter@pjd.dev>
> ---
>   hw/arm/aspeed_ast10x0.c     | 11 ++++++++++-
>   include/hw/arm/aspeed_soc.h |  3 +++
>   2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> index 6c6d33d4a0..e74e2660ce 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -22,7 +22,7 @@
>   
>   static const hwaddr aspeed_soc_ast1030_memmap[] = {
>       [ASPEED_DEV_SRAM]      = 0x00000000,
> -    [ASPEED_DEV_SBC]       = 0x79000000,
> +    [ASPEED_DEV_SECSRAM]   = 0x79000000,
>       [ASPEED_DEV_IOMEM]     = 0x7E600000,
>       [ASPEED_DEV_PWM]       = 0x7E610000,
>       [ASPEED_DEV_FMC]       = 0x7E620000,
> @@ -222,6 +222,14 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
>       memory_region_add_subregion(s->memory,
>                                   sc->memmap[ASPEED_DEV_SRAM],
>                                   &s->sram);
> +    memory_region_init_ram(&s->secsram, OBJECT(s), "sec.sram",
> +                           sc->secsram_size, &err);
> +    if (err != NULL) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    memory_region_add_subregion(s->memory, sc->memmap[ASPEED_DEV_SECSRAM],
> +                                &s->secsram);
>   
>       /* SCU */
>       if (!sysbus_realize(SYS_BUS_DEVICE(&s->scu), errp)) {
> @@ -401,6 +409,7 @@ static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data)
>       sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-m4");
>       sc->silicon_rev = AST1030_A1_SILICON_REV;
>       sc->sram_size = 768 * KiB;
> +    sc->secsram_size = 9 * KiB;

256  * KiB

>       sc->spis_num = 2;
>       sc->ehcis_num = 0;
>       sc->wdts_num = 4;
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index 9a5e3c0bac..bd1e03e78a 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -71,6 +71,7 @@ struct AspeedSoCState {
>       AspeedSMCState spi[ASPEED_SPIS_NUM];
>       EHCISysBusState ehci[ASPEED_EHCIS_NUM];
>       AspeedSBCState sbc;
> +    MemoryRegion secsram;
>       UnimplementedDeviceState sbc_unimplemented;
>       AspeedSDMCState sdmc;
>       AspeedWDTState wdt[ASPEED_WDTS_NUM];
> @@ -105,6 +106,7 @@ struct AspeedSoCClass {
>       const char *cpu_type;
>       uint32_t silicon_rev;
>       uint64_t sram_size;
> +    uint64_t secsram_size;
>       int spis_num;
>       int ehcis_num;
>       int wdts_num;
> @@ -143,6 +145,7 @@ enum {
>       ASPEED_DEV_SCU,
>       ASPEED_DEV_ADC,
>       ASPEED_DEV_SBC,
> +    ASPEED_DEV_SECSRAM,

May be keep the ASPEED_DEV_SBC_ prefix ?

Thanks,

C.

>       ASPEED_DEV_EMMC_BC,
>       ASPEED_DEV_VIDEO,
>       ASPEED_DEV_SRAM,

[PATCH v2 09/11] hw/arm/aspeed_ast10x0: Map HACE peripheral

[Philippe Mathieu-Daudé]

Since I don't have access to the datasheet, the relevant
values were found in:
https://github.com/AspeedTech-BMC/zephyr/blob/v00.01.08/dts/arm/aspeed/ast10x0.dtsi

Before on Zephyr:

  uart:~$ hash test
  sha256_test
  tv[0]:hash_final error
  sha384_test
  tv[0]:hash_final error
  sha512_test
  tv[0]:hash_final error
  [00:00:06.278,000] <err> hace_global: HACE poll timeout
  [00:00:09.324,000] <err> hace_global: HACE poll timeout
  [00:00:12.261,000] <err> hace_global: HACE poll timeout

  uart:~$ crypto aes256_cbc_vault
  aes256_cbc vault key 1
  [00:00:06.699,000] <inf> hace_global: aspeed_crypto_session_setup
  [00:00:06.699,000] <inf> hace_global: data->cmd: 1c2098
  [00:00:06.699,000] <inf> hace_global: crypto_data_src: 93340
  [00:00:06.699,000] <inf> hace_global: crypto_data_dst: 93348
  [00:00:06.699,000] <inf> hace_global: crypto_ctx_base: 93300
  [00:00:06.699,000] <inf> hace_global: crypto_data_len: 80000040
  [00:00:06.699,000] <inf> hace_global: crypto_cmd_reg:  11c2098
  [00:00:09.743,000] <inf> hace_global: HACE_STS: 0
  [00:00:09.743,000] <err> hace_global: HACE poll timeout
  [00:00:09.743,000] <err> crypto: CBC mode ENCRYPT - Failed
  [00:00:09.743,000] <inf> hace_global: aspeed_crypto_session_free
  uart:~$

After:

  uart:~$ hash test
  sha256_test
  tv[0]:PASS
  tv[1]:PASS
  tv[2]:PASS
  tv[3]:PASS
  tv[4]:PASS
  sha384_test
  tv[0]:PASS
  tv[1]:PASS
  tv[2]:PASS
  tv[3]:PASS
  tv[4]:PASS
  tv[5]:PASS
  sha512_test
  tv[0]:PASS
  tv[1]:PASS
  tv[2]:PASS
  tv[3]:PASS
  tv[4]:PASS
  tv[5]:PASS

  uart:~$ crypto aes256_cbc_vault
  aes256_cbc vault key 1
  Was waiting for:
  6b c1 be e2 2e 40 9f 96 e9 3d 7e 11 73 93 17 2a
  ae 2d 8a 57 1e 03 ac 9c 9e b7 6f ac 45 af 8e 51
  30 c8 1c 46 a3 5c e4 11 e5 fb c1 19 1a 0a 52 ef
  f6 9f 24 45 df 4f 9b 17 ad 2b 41 7b e6 6c 37 10

   But got:
  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

  [00:00:05.771,000] <inf> hace_global: aspeed_crypto_session_setup
  [00:00:05.772,000] <inf> hace_global: data->cmd: 1c2098
  [00:00:05.772,000] <inf> hace_global: crypto_data_src: 93340
  [00:00:05.772,000] <inf> hace_global: crypto_data_dst: 93348
  [00:00:05.772,000] <inf> hace_global: crypto_ctx_base: 93300
  [00:00:05.772,000] <inf> hace_global: crypto_data_len: 80000040
  [00:00:05.772,000] <inf> hace_global: crypto_cmd_reg:  11c2098
  [00:00:05.772,000] <inf> hace_global: HACE_STS: 1000
  [00:00:05.772,000] <inf> crypto: Output length (encryption): 80
  [00:00:05.772,000] <inf> hace_global: aspeed_crypto_session_free
  [00:00:05.772,000] <inf> hace_global: aspeed_crypto_session_setup
  [00:00:05.772,000] <inf> hace_global: data->cmd: 1c2018
  [00:00:05.772,000] <inf> hace_global: crypto_data_src: 93340
  [00:00:05.772,000] <inf> hace_global: crypto_data_dst: 93348
  [00:00:05.772,000] <inf> hace_global: crypto_ctx_base: 93300
  [00:00:05.772,000] <inf> hace_global: crypto_data_len: 80000040
  [00:00:05.772,000] <inf> hace_global: crypto_cmd_reg:  11c2018
  [00:00:05.772,000] <inf> hace_global: HACE_STS: 1000
  [00:00:05.772,000] <inf> crypto: Output length (decryption): 64
  [00:00:05.772,000] <err> crypto: CBC mode DECRYPT - Mismatch between plaintext and decrypted cipher text
  [00:00:05.774,000] <inf> hace_global: aspeed_crypto_session_free
  uart:~$

Reviewed-by: Peter Delevoryas <peter@pjd.dev>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/aspeed_ast10x0.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
index e74e2660ce..7a7443a95b 100644
--- a/hw/arm/aspeed_ast10x0.c
+++ b/hw/arm/aspeed_ast10x0.c
@@ -29,6 +29,7 @@ static const hwaddr aspeed_soc_ast1030_memmap[] = {
     [ASPEED_DEV_SPI1]      = 0x7E630000,
     [ASPEED_DEV_SPI2]      = 0x7E640000,
     [ASPEED_DEV_UDC]       = 0x7E6A2000,
+    [ASPEED_DEV_HACE]      = 0x7E6D0000,
     [ASPEED_DEV_SCU]       = 0x7E6E2000,
     [ASPEED_DEV_JTAG0]     = 0x7E6E4000,
     [ASPEED_DEV_JTAG1]     = 0x7E6E4100,
@@ -166,6 +167,9 @@ static void aspeed_soc_ast1030_init(Object *obj)
     snprintf(typename, sizeof(typename), "aspeed.gpio-%s", socname);
     object_initialize_child(obj, "gpio", &s->gpio, typename);
 
+    snprintf(typename, sizeof(typename), "aspeed.hace-%s", socname);
+    object_initialize_child(obj, "hace", &s->hace, typename);
+
     object_initialize_child(obj, "iomem", &s->iomem, TYPE_UNIMPLEMENTED_DEVICE);
     object_initialize_child(obj, "sbc-unimplemented", &s->sbc_unimplemented,
                             TYPE_UNIMPLEMENTED_DEVICE);
@@ -359,6 +363,17 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
     }
     aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->sbc), 0, sc->memmap[ASPEED_DEV_SBC]);
 
+    /* HACE */
+    object_property_set_link(OBJECT(&s->hace), "dram", OBJECT(&s->sram),
+                             &error_abort);
+    if (!sysbus_realize(SYS_BUS_DEVICE(&s->hace), errp)) {
+        return;
+    }
+    aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->hace), 0,
+                    sc->memmap[ASPEED_DEV_HACE]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->hace), 0,
+                       aspeed_soc_get_irq(s, ASPEED_DEV_HACE));
+
     /* Watch dog */
     for (i = 0; i < sc->wdts_num; i++) {
         AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(&s->wdt[i]);
-- 
2.38.1

Re: [PATCH v2 09/11] hw/arm/aspeed_ast10x0: Map HACE peripheral

[Cédric Le Goater]

On 12/30/22 12:35, Philippe Mathieu-Daudé wrote:
> Since I don't have access to the datasheet, the relevant
> values were found in:
> https://github.com/AspeedTech-BMC/zephyr/blob/v00.01.08/dts/arm/aspeed/ast10x0.dtsi
> 
> Before on Zephyr:
> 
>    uart:~$ hash test
>    sha256_test
>    tv[0]:hash_final error
>    sha384_test
>    tv[0]:hash_final error
>    sha512_test
>    tv[0]:hash_final error
>    [00:00:06.278,000] <err> hace_global: HACE poll timeout
>    [00:00:09.324,000] <err> hace_global: HACE poll timeout
>    [00:00:12.261,000] <err> hace_global: HACE poll timeout
> 
>    uart:~$ crypto aes256_cbc_vault
>    aes256_cbc vault key 1
>    [00:00:06.699,000] <inf> hace_global: aspeed_crypto_session_setup
>    [00:00:06.699,000] <inf> hace_global: data->cmd: 1c2098
>    [00:00:06.699,000] <inf> hace_global: crypto_data_src: 93340
>    [00:00:06.699,000] <inf> hace_global: crypto_data_dst: 93348
>    [00:00:06.699,000] <inf> hace_global: crypto_ctx_base: 93300
>    [00:00:06.699,000] <inf> hace_global: crypto_data_len: 80000040
>    [00:00:06.699,000] <inf> hace_global: crypto_cmd_reg:  11c2098
>    [00:00:09.743,000] <inf> hace_global: HACE_STS: 0
>    [00:00:09.743,000] <err> hace_global: HACE poll timeout
>    [00:00:09.743,000] <err> crypto: CBC mode ENCRYPT - Failed
>    [00:00:09.743,000] <inf> hace_global: aspeed_crypto_session_free
>    uart:~$
> 
> After:
> 
>    uart:~$ hash test
>    sha256_test
>    tv[0]:PASS
>    tv[1]:PASS
>    tv[2]:PASS
>    tv[3]:PASS
>    tv[4]:PASS
>    sha384_test
>    tv[0]:PASS
>    tv[1]:PASS
>    tv[2]:PASS
>    tv[3]:PASS
>    tv[4]:PASS
>    tv[5]:PASS
>    sha512_test
>    tv[0]:PASS
>    tv[1]:PASS
>    tv[2]:PASS
>    tv[3]:PASS
>    tv[4]:PASS
>    tv[5]:PASS
> 
>    uart:~$ crypto aes256_cbc_vault
>    aes256_cbc vault key 1
>    Was waiting for:
>    6b c1 be e2 2e 40 9f 96 e9 3d 7e 11 73 93 17 2a
>    ae 2d 8a 57 1e 03 ac 9c 9e b7 6f ac 45 af 8e 51
>    30 c8 1c 46 a3 5c e4 11 e5 fb c1 19 1a 0a 52 ef
>    f6 9f 24 45 df 4f 9b 17 ad 2b 41 7b e6 6c 37 10
> 
>     But got:
>    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

I think the HACE model only supports hash for now.


> 
>    [00:00:05.771,000] <inf> hace_global: aspeed_crypto_session_setup
>    [00:00:05.772,000] <inf> hace_global: data->cmd: 1c2098
>    [00:00:05.772,000] <inf> hace_global: crypto_data_src: 93340
>    [00:00:05.772,000] <inf> hace_global: crypto_data_dst: 93348
>    [00:00:05.772,000] <inf> hace_global: crypto_ctx_base: 93300
>    [00:00:05.772,000] <inf> hace_global: crypto_data_len: 80000040
>    [00:00:05.772,000] <inf> hace_global: crypto_cmd_reg:  11c2098
>    [00:00:05.772,000] <inf> hace_global: HACE_STS: 1000
>    [00:00:05.772,000] <inf> crypto: Output length (encryption): 80
>    [00:00:05.772,000] <inf> hace_global: aspeed_crypto_session_free
>    [00:00:05.772,000] <inf> hace_global: aspeed_crypto_session_setup
>    [00:00:05.772,000] <inf> hace_global: data->cmd: 1c2018
>    [00:00:05.772,000] <inf> hace_global: crypto_data_src: 93340
>    [00:00:05.772,000] <inf> hace_global: crypto_data_dst: 93348
>    [00:00:05.772,000] <inf> hace_global: crypto_ctx_base: 93300
>    [00:00:05.772,000] <inf> hace_global: crypto_data_len: 80000040
>    [00:00:05.772,000] <inf> hace_global: crypto_cmd_reg:  11c2018
>    [00:00:05.772,000] <inf> hace_global: HACE_STS: 1000
>    [00:00:05.772,000] <inf> crypto: Output length (decryption): 64
>    [00:00:05.772,000] <err> crypto: CBC mode DECRYPT - Mismatch between plaintext and decrypted cipher text
>    [00:00:05.774,000] <inf> hace_global: aspeed_crypto_session_free
>    uart:~$
> 
> Reviewed-by: Peter Delevoryas <peter@pjd.dev>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>   hw/arm/aspeed_ast10x0.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> index e74e2660ce..7a7443a95b 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -29,6 +29,7 @@ static const hwaddr aspeed_soc_ast1030_memmap[] = {
>       [ASPEED_DEV_SPI1]      = 0x7E630000,
>       [ASPEED_DEV_SPI2]      = 0x7E640000,
>       [ASPEED_DEV_UDC]       = 0x7E6A2000,
> +    [ASPEED_DEV_HACE]      = 0x7E6D0000,
>       [ASPEED_DEV_SCU]       = 0x7E6E2000,
>       [ASPEED_DEV_JTAG0]     = 0x7E6E4000,
>       [ASPEED_DEV_JTAG1]     = 0x7E6E4100,
> @@ -166,6 +167,9 @@ static void aspeed_soc_ast1030_init(Object *obj)
>       snprintf(typename, sizeof(typename), "aspeed.gpio-%s", socname);
>       object_initialize_child(obj, "gpio", &s->gpio, typename);
>   
> +    snprintf(typename, sizeof(typename), "aspeed.hace-%s", socname);
> +    object_initialize_child(obj, "hace", &s->hace, typename);
> +
>       object_initialize_child(obj, "iomem", &s->iomem, TYPE_UNIMPLEMENTED_DEVICE);
>       object_initialize_child(obj, "sbc-unimplemented", &s->sbc_unimplemented,
>                               TYPE_UNIMPLEMENTED_DEVICE);
> @@ -359,6 +363,17 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
>       }
>       aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->sbc), 0, sc->memmap[ASPEED_DEV_SBC]);
>   
> +    /* HACE */
> +    object_property_set_link(OBJECT(&s->hace), "dram", OBJECT(&s->sram),
> +                             &error_abort);
> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->hace), errp)) {
> +        return;
> +    }
> +    aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->hace), 0,
> +                    sc->memmap[ASPEED_DEV_HACE]);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->hace), 0,
> +                       aspeed_soc_get_irq(s, ASPEED_DEV_HACE));
> +
>       /* Watch dog */
>       for (i = 0; i < sc->wdts_num; i++) {
>           AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(&s->wdt[i]);

[PATCH v2 10/11] hw/arm/aspeed_ast10x0: Add TODO comment to use Cortex-M4F

[Philippe Mathieu-Daudé]

This SoC uses a Cortex-M4F. QEMU only implements a M4,
which is good enough. Add a TODO note in case the M4F
is added.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Delevoryas <peter@pjd.dev>
---
 hw/arm/aspeed_ast10x0.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
index 7a7443a95b..a3bcbef24a 100644
--- a/hw/arm/aspeed_ast10x0.c
+++ b/hw/arm/aspeed_ast10x0.c
@@ -421,7 +421,7 @@ static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data)
     dc->realize = aspeed_soc_ast1030_realize;
 
     sc->name = "ast1030-a1";
-    sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-m4");
+    sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-m4"); /* TODO cortex-m4f */
     sc->silicon_rev = AST1030_A1_SILICON_REV;
     sc->sram_size = 768 * KiB;
     sc->secsram_size = 9 * KiB;
-- 
2.38.1

Re: [PATCH v2 10/11] hw/arm/aspeed_ast10x0: Add TODO comment to use Cortex-M4F

[Cédric Le Goater]

On 12/30/22 12:35, Philippe Mathieu-Daudé wrote:
> This SoC uses a Cortex-M4F. QEMU only implements a M4,
> which is good enough. Add a TODO note in case the M4F
> is added.

How complex would it be to add the FPU version of the M4 ? I suppose we have
all the instructions already implemented ?
  
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Peter Delevoryas <peter@pjd.dev>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.



> ---
>   hw/arm/aspeed_ast10x0.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> index 7a7443a95b..a3bcbef24a 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -421,7 +421,7 @@ static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data)
>       dc->realize = aspeed_soc_ast1030_realize;
>   
>       sc->name = "ast1030-a1";
> -    sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-m4");
> +    sc->cpu_type = ARM_CPU_TYPE_NAME("cortex-m4"); /* TODO cortex-m4f */
>       sc->silicon_rev = AST1030_A1_SILICON_REV;
>       sc->sram_size = 768 * KiB;
>       sc->secsram_size = 9 * KiB;

[PATCH v2 11/11] tests/avocado: Test Aspeed Zephyr SDK v00.01.08 on AST1030 board

[Philippe Mathieu-Daudé]

Add a very quick test that runs some commands in a Zephyr shell:

  $ tests/venv/bin/avocado --show=app,console run -t os:zephyr tests/avocado
  (2/2) tests/avocado/machine_aspeed.py:AST1030Machine.test_ast1030_zephyros_1_07:
  console: *** Booting Zephyr OS build v00.01.07  ***
  console: ast1030_evb demo
  console: SOC: AST1030-A1
  console: uart:~$ kernel stacks
  console: 0x36910 wdt_background (real size 1024):	unused 988	usage 36 / 1024 (3 %)
  console: 0x36ad8 shell_uart (real size 4096):	unused 3084	usage 1012 / 4096 (24 %)
  console: 0x2edb8 ADC0       (real size 400):	unused 260	usage 140 / 400 (35 %)
  console: 0x2f0f0 ADC1       (real size 400):	unused 260	usage 140 / 400 (35 %)
  console: 0x3b098 sysworkq   (real size 1024):	unused 860	usage 164 / 1024 (16 %)
  console: 0x36cc0 usbdworkq  (real size 1024):	unused 860	usage 164 / 1024 (16 %)
  console: 0x36bd8 usbworkq   (real size 1024):	unused 860	usage 164 / 1024 (16 %)
  console: 0x36a10 logging    (real size 768):	unused 548	usage 220 / 768 (28 %)
  console: 0x36ef8 idle 00    (real size 320):	unused 268	usage 52 / 320 (16 %)
  console: 0x47800 IRQ 00     (real size 2048):	unused 1504	usage 544 / 2048 (26 %)
  console: uart:~$ otp info scu
  console: SCU     BIT   reg_protect     Description
  console: ____________________________________________________________________
  console: 0x500   0x0   0x0             Disable ARM CM4 CPU boot (TXD5)
  console: 0x500   0x1   0x0            /Reserved
  console: 0x500   0x2   0x0            \ "
  console: 0x500   0x3   0x0             Address offset of single chip ABR mode
  console: 0x500   0x4   0x0            /Reserved
  console: 0x500   0x5   0x0            | "
  console: 0x500   0x6   0x0            | "
  console: 0x500   0x7   0x0            | "
  console: 0x500   0x8   0x0            | "
  console: 0x500   0x9   0x0            | "
  console: 0x500   0xA   0x0            | "
  console: 0x500   0xB   0x0            | "
  console: 0x500   0xC   0x0            | "
  console: 0x500   0xD   0x0            | "
  console: 0x500   0xE   0x0            | "
  console: 0x500   0xF   0x0            | "
  console: 0x500   0x10  0x0            \ "
  console: 0x500   0x11  0x0             Disabl3 ARM JTAG debug
  console: 0x500   0x12  0x0            /Reserved
  console: 0x500   0x13  0x0            | "
  console: 0x500   0x14  0x0            | "
  console: 0x500   0x15  0x0            | "
  console: 0x500   0x16  0x0            | "
  console: 0x500   0x17  0x0            | "
  console: 0x500   0x18  0x0            | "
  console: 0x500   0x19  0x0            | "
  console: 0x500   0x1A  0x0            | "
  console: 0x500   0x1B  0x0            | "
  console: 0x500   0x1C  0x0            | "
  console: 0x500   0x1D  0x0            | "
  console: 0x500   0x1E  0x0            | "
  console: 0x500   0x1F  0x0            \ "
  console: 0x510   0x0   0x0            /Reserved
  console: 0x510   0x1   0x0            | "
  console: 0x510   0x2   0x0            | "
  console: 0x510   0x3   0x0            \ "
  console: 0x510   0x4   0x0             Disable debug interfaces
  console: 0x510   0x5   0x0            /Reserved
  console: 0x510   0x6   0x0            | "
  console: 0x510   0x7   0x0            \ "
  console: 0x510   0x8   0x0             Enable boot from Uart5 by Pin Strap
  console: 0x510   0x9   0x0            /Reserved
  console: 0x510   0xA   0x0            \ "
  console: 0x510   0xB   0x0             Enable boot SPI ABR
  console: 0x510   0xC   0x0             Boot SPI ABR Mode
  console: 0x510   0xD   0x0            /Boot SPI flash size
  console: 0x510   0xE   0x0            | "
  console: 0x510   0xF   0x0            \ "
  console: 0x510   0x10  0x0            /Reserved
  console: 0x510   0x11  0x0            | "
  console: 0x510   0x12  0x0            | "
  console: 0x510   0x13  0x0            | "
  console: 0x510   0x14  0x0            | "
  console: 0x510   0x15  0x0            \ "
  console: 0x510   0x16  0x0             Enable boot SPI auxiliary control pins
  console: 0x510   0x19  0x0            /Reserved
  console: 0x510   0x1A  0x0            | "
  console: 0x510   0x1B  0x0            | "
  console: 0x510   0x1C  0x0            | "
  console: 0x510   0x1D  0x0            | "
  console: 0x510   0x1E  0x0            | "
  console: 0x510   0x1F  0x0            \ "
  console: 0x510   0x1E  0x0             Enable dedicate GPIO strap pins
  console: 0x510   0x1F  0x0             Enable Secure Boot by Pin Strap
  console: uart:~$ hwinfo devid
  console: Length: 8
  console: ID: 0x0000018000000180
  console: uart:~$ crypto aes256_cbc_vault
  console: aes256_cbc vault key 1
  console: Was waiting for:
  console: 6b c1 be e2 2e 40 9f 96 e9 3d 7e 11 73 93 17 2a
  console: ae 2d 8a 57 1e 03 ac 9c 9e b7 6f ac 45 af 8e 51
  console: 30 c8 1c 46 a3 5c e4 11 e5 fb c1 19 1a 0a 52 ef
  console: f6 9f 24 45 df 4f 9b 17 ad 2b 41 7b e6 6c 37 10
  console: But got:
  console: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  console: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  console: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  console: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  console: uart:~$ random get
  console: 0x862460d
  console: uart:~$ i2c scan I2C_0
  console: 0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
  console: 00:             -- -- -- -- -- -- -- -- -- -- -- --
  console: 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
  console: 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
  console: 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
  console: 40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
  console: 50: 50 -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
  console: 60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
  console: 70: -- -- -- -- -- -- -- --
  console: 1 devices found on I2C_0
  console: uart:~$ kernel uptime
  console: Uptime: 9897 ms
  console: uart:~$ kernel reboot warm
  console: *** Booting Zephyr OS build v00.01.07  ***
  PASS (1.08 s)

Ref: https://github.com/AspeedTech-BMC/zephyr/releases/download/v00.01.07/Aspeed_Zephy_SDK_User_Guide_v00.01.07.pdf

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Delevoryas <peter@pjd.dev>
---
 tests/avocado/machine_aspeed.py | 41 ++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/tests/avocado/machine_aspeed.py b/tests/avocado/machine_aspeed.py
index 1fc385e1c8..11f5b17eb9 100644
--- a/tests/avocado/machine_aspeed.py
+++ b/tests/avocado/machine_aspeed.py
@@ -22,10 +22,11 @@ class AST1030Machine(QemuSystemTest):
 
     timeout = 10
 
-    def test_ast1030_zephyros(self):
+    def test_ast1030_zephyros_1_04(self):
         """
         :avocado: tags=arch:arm
         :avocado: tags=machine:ast1030-evb
+        :avocado: tags=os:zephyr
         """
         tar_url = ('https://github.com/AspeedTech-BMC'
                    '/zephyr/releases/download/v00.01.04/ast1030-evb-demo.zip')
@@ -41,6 +42,44 @@ def test_ast1030_zephyros(self):
         exec_command_and_wait_for_pattern(self, "help",
                                           "Available commands")
 
+    def test_ast1030_zephyros_1_07(self):
+        """
+        :avocado: tags=arch:arm
+        :avocado: tags=machine:ast1030-evb
+        :avocado: tags=os:zephyr
+        """
+        tar_url = ('https://github.com/AspeedTech-BMC'
+                   '/zephyr/releases/download/v00.01.07/ast1030-evb-demo.zip')
+        tar_hash = '40ac87eabdcd3b3454ce5aad11fedc72a33ecda2'
+        tar_path = self.fetch_asset(tar_url, asset_hash=tar_hash)
+        archive.extract(tar_path, self.workdir)
+        kernel_file = self.workdir + "/ast1030-evb-demo/zephyr.bin"
+        self.vm.set_console()
+        self.vm.add_args('-kernel', kernel_file,
+                         '-nographic')
+        self.vm.launch()
+        wait_for_console_pattern(self, "Booting Zephyr OS")
+        for shell_cmd in [
+                'kernel stacks',
+                'otp info conf',
+                'otp info scu',
+                'hwinfo devid',
+                'crypto aes256_cbc_vault',
+                'random get',
+                'jtag JTAG1 sw_xfer high TMS',
+                'adc ADC0 resolution 12',
+                'adc ADC0 read 42',
+                'adc ADC1 read 69',
+                'i2c scan I2C_0',
+                'i3c attach I3C_0',
+                'hash test',
+                'kernel uptime',
+                'kernel reboot warm',
+                'kernel uptime',
+                'kernel reboot cold',
+                'kernel uptime',
+        ]: exec_command_and_wait_for_pattern(self, shell_cmd, "uart:~$")
+
 class AST2x00Machine(QemuSystemTest):
 
     timeout = 90
-- 
2.38.1

Re: [PATCH v2 11/11] tests/avocado: Test Aspeed Zephyr SDK v00.01.08 on AST1030 board

[Cédric Le Goater]

On 12/30/22 12:35, Philippe Mathieu-Daudé wrote:
> Add a very quick test that runs some commands in a Zephyr shell:
> 
>    $ tests/venv/bin/avocado --show=app,console run -t os:zephyr tests/avocado
>    (2/2) tests/avocado/machine_aspeed.py:AST1030Machine.test_ast1030_zephyros_1_07:
>    console: *** Booting Zephyr OS build v00.01.07  ***
>    console: ast1030_evb demo
>    console: SOC: AST1030-A1
>    console: uart:~$ kernel stacks
>    console: 0x36910 wdt_background (real size 1024):	unused 988	usage 36 / 1024 (3 %)
>    console: 0x36ad8 shell_uart (real size 4096):	unused 3084	usage 1012 / 4096 (24 %)
>    console: 0x2edb8 ADC0       (real size 400):	unused 260	usage 140 / 400 (35 %)
>    console: 0x2f0f0 ADC1       (real size 400):	unused 260	usage 140 / 400 (35 %)
>    console: 0x3b098 sysworkq   (real size 1024):	unused 860	usage 164 / 1024 (16 %)
>    console: 0x36cc0 usbdworkq  (real size 1024):	unused 860	usage 164 / 1024 (16 %)
>    console: 0x36bd8 usbworkq   (real size 1024):	unused 860	usage 164 / 1024 (16 %)
>    console: 0x36a10 logging    (real size 768):	unused 548	usage 220 / 768 (28 %)
>    console: 0x36ef8 idle 00    (real size 320):	unused 268	usage 52 / 320 (16 %)
>    console: 0x47800 IRQ 00     (real size 2048):	unused 1504	usage 544 / 2048 (26 %)
>    console: uart:~$ otp info scu
>    console: SCU     BIT   reg_protect     Description
>    console: ____________________________________________________________________
>    console: 0x500   0x0   0x0             Disable ARM CM4 CPU boot (TXD5)
>    console: 0x500   0x1   0x0            /Reserved
>    console: 0x500   0x2   0x0            \ "
>    console: 0x500   0x3   0x0             Address offset of single chip ABR mode
>    console: 0x500   0x4   0x0            /Reserved
>    console: 0x500   0x5   0x0            | "
>    console: 0x500   0x6   0x0            | "
>    console: 0x500   0x7   0x0            | "
>    console: 0x500   0x8   0x0            | "
>    console: 0x500   0x9   0x0            | "
>    console: 0x500   0xA   0x0            | "
>    console: 0x500   0xB   0x0            | "
>    console: 0x500   0xC   0x0            | "
>    console: 0x500   0xD   0x0            | "
>    console: 0x500   0xE   0x0            | "
>    console: 0x500   0xF   0x0            | "
>    console: 0x500   0x10  0x0            \ "
>    console: 0x500   0x11  0x0             Disabl3 ARM JTAG debug
>    console: 0x500   0x12  0x0            /Reserved
>    console: 0x500   0x13  0x0            | "
>    console: 0x500   0x14  0x0            | "
>    console: 0x500   0x15  0x0            | "
>    console: 0x500   0x16  0x0            | "
>    console: 0x500   0x17  0x0            | "
>    console: 0x500   0x18  0x0            | "
>    console: 0x500   0x19  0x0            | "
>    console: 0x500   0x1A  0x0            | "
>    console: 0x500   0x1B  0x0            | "
>    console: 0x500   0x1C  0x0            | "
>    console: 0x500   0x1D  0x0            | "
>    console: 0x500   0x1E  0x0            | "
>    console: 0x500   0x1F  0x0            \ "
>    console: 0x510   0x0   0x0            /Reserved
>    console: 0x510   0x1   0x0            | "
>    console: 0x510   0x2   0x0            | "
>    console: 0x510   0x3   0x0            \ "
>    console: 0x510   0x4   0x0             Disable debug interfaces
>    console: 0x510   0x5   0x0            /Reserved
>    console: 0x510   0x6   0x0            | "
>    console: 0x510   0x7   0x0            \ "
>    console: 0x510   0x8   0x0             Enable boot from Uart5 by Pin Strap
>    console: 0x510   0x9   0x0            /Reserved
>    console: 0x510   0xA   0x0            \ "
>    console: 0x510   0xB   0x0             Enable boot SPI ABR
>    console: 0x510   0xC   0x0             Boot SPI ABR Mode
>    console: 0x510   0xD   0x0            /Boot SPI flash size
>    console: 0x510   0xE   0x0            | "
>    console: 0x510   0xF   0x0            \ "
>    console: 0x510   0x10  0x0            /Reserved
>    console: 0x510   0x11  0x0            | "
>    console: 0x510   0x12  0x0            | "
>    console: 0x510   0x13  0x0            | "
>    console: 0x510   0x14  0x0            | "
>    console: 0x510   0x15  0x0            \ "
>    console: 0x510   0x16  0x0             Enable boot SPI auxiliary control pins
>    console: 0x510   0x19  0x0            /Reserved
>    console: 0x510   0x1A  0x0            | "
>    console: 0x510   0x1B  0x0            | "
>    console: 0x510   0x1C  0x0            | "
>    console: 0x510   0x1D  0x0            | "
>    console: 0x510   0x1E  0x0            | "
>    console: 0x510   0x1F  0x0            \ "
>    console: 0x510   0x1E  0x0             Enable dedicate GPIO strap pins
>    console: 0x510   0x1F  0x0             Enable Secure Boot by Pin Strap
>    console: uart:~$ hwinfo devid
>    console: Length: 8
>    console: ID: 0x0000018000000180
>    console: uart:~$ crypto aes256_cbc_vault
>    console: aes256_cbc vault key 1
>    console: Was waiting for:
>    console: 6b c1 be e2 2e 40 9f 96 e9 3d 7e 11 73 93 17 2a
>    console: ae 2d 8a 57 1e 03 ac 9c 9e b7 6f ac 45 af 8e 51
>    console: 30 c8 1c 46 a3 5c e4 11 e5 fb c1 19 1a 0a 52 ef
>    console: f6 9f 24 45 df 4f 9b 17 ad 2b 41 7b e6 6c 37 10
>    console: But got:
>    console: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>    console: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>    console: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>    console: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>    console: uart:~$ random get
>    console: 0x862460d
>    console: uart:~$ i2c scan I2C_0
>    console: 0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
>    console: 00:             -- -- -- -- -- -- -- -- -- -- -- --
>    console: 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>    console: 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>    console: 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>    console: 40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>    console: 50: 50 -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>    console: 60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>    console: 70: -- -- -- -- -- -- -- --
>    console: 1 devices found on I2C_0
>    console: uart:~$ kernel uptime
>    console: Uptime: 9897 ms
>    console: uart:~$ kernel reboot warm
>    console: *** Booting Zephyr OS build v00.01.07  ***
>    PASS (1.08 s)
> 
> Ref: https://github.com/AspeedTech-BMC/zephyr/releases/download/v00.01.07/Aspeed_Zephy_SDK_User_Guide_v00.01.07.pdf
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Peter Delevoryas <peter@pjd.dev>



Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> ---
>   tests/avocado/machine_aspeed.py | 41 ++++++++++++++++++++++++++++++++-
>   1 file changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/avocado/machine_aspeed.py b/tests/avocado/machine_aspeed.py
> index 1fc385e1c8..11f5b17eb9 100644
> --- a/tests/avocado/machine_aspeed.py
> +++ b/tests/avocado/machine_aspeed.py
> @@ -22,10 +22,11 @@ class AST1030Machine(QemuSystemTest):
>   
>       timeout = 10
>   
> -    def test_ast1030_zephyros(self):
> +    def test_ast1030_zephyros_1_04(self):
>           """
>           :avocado: tags=arch:arm
>           :avocado: tags=machine:ast1030-evb
> +        :avocado: tags=os:zephyr
>           """
>           tar_url = ('https://github.com/AspeedTech-BMC'
>                      '/zephyr/releases/download/v00.01.04/ast1030-evb-demo.zip')
> @@ -41,6 +42,44 @@ def test_ast1030_zephyros(self):
>           exec_command_and_wait_for_pattern(self, "help",
>                                             "Available commands")
>   
> +    def test_ast1030_zephyros_1_07(self):
> +        """
> +        :avocado: tags=arch:arm
> +        :avocado: tags=machine:ast1030-evb
> +        :avocado: tags=os:zephyr
> +        """
> +        tar_url = ('https://github.com/AspeedTech-BMC'
> +                   '/zephyr/releases/download/v00.01.07/ast1030-evb-demo.zip')
> +        tar_hash = '40ac87eabdcd3b3454ce5aad11fedc72a33ecda2'
> +        tar_path = self.fetch_asset(tar_url, asset_hash=tar_hash)
> +        archive.extract(tar_path, self.workdir)
> +        kernel_file = self.workdir + "/ast1030-evb-demo/zephyr.bin"
> +        self.vm.set_console()
> +        self.vm.add_args('-kernel', kernel_file,
> +                         '-nographic')
> +        self.vm.launch()
> +        wait_for_console_pattern(self, "Booting Zephyr OS")
> +        for shell_cmd in [
> +                'kernel stacks',
> +                'otp info conf',
> +                'otp info scu',
> +                'hwinfo devid',
> +                'crypto aes256_cbc_vault',
> +                'random get',
> +                'jtag JTAG1 sw_xfer high TMS',
> +                'adc ADC0 resolution 12',
> +                'adc ADC0 read 42',
> +                'adc ADC1 read 69',
> +                'i2c scan I2C_0',
> +                'i3c attach I3C_0',
> +                'hash test',
> +                'kernel uptime',
> +                'kernel reboot warm',
> +                'kernel uptime',
> +                'kernel reboot cold',
> +                'kernel uptime',
> +        ]: exec_command_and_wait_for_pattern(self, shell_cmd, "uart:~$")
> +
>   class AST2x00Machine(QemuSystemTest):
>   
>       timeout = 90