Re: [PATCH v2 02/11] hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers
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
0x03f1)
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
Signed-off-by: Philippe Mathieu-Daudé
---
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(>iomem, OBJECT(s), _wdt_ops, s,
- TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
+ TYPE_ASPEED_WDT, awc->iosize);
sysbus_init_mmio(sbd, >iomem);
}
--
2.38.1
Re: [PATCH v2 02/11] hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers
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
> > > > > 0x03f1)
> > > > >
> > > > > 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_REG0x000C
> > > > > #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.
> > >
> > > #regsiosize
> > >
> > > 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
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
0x03f1)
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_REG0x000C
#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.
#regsiosize
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
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
> > > 0x03f1)
> > >
> > > 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_REG0x000C
> > >#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.
>
>#regsiosize
>
> 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
> > > Signed-off-by: Philippe Mathieu-Daudé
> > > ---
> > > 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(>iomem, OBJECT(s), _wdt_ops, s,
> > > - TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
> > > + TYPE_ASPEED_WDT, awc->iosize);
> > > sysbus_init_mmio(sbd, >iomem);
> > > }
> > >
> > > --
> > > 2.38.1
> > >
> >
>
Re: [PATCH v2 02/11] hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers
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
0x03f1)
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_REG0x000C
#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.
#regsiosize
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
Signed-off-by: Philippe Mathieu-Daudé
---
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(>iomem, OBJECT(s), _wdt_ops, s,
- TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
+ TYPE_ASPEED_WDT, awc->iosize);
sysbus_init_mmio(sbd, >iomem);
}
--
2.38.1
Re: [PATCH v2 02/11] hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers
On 31/12/22 23:52, Dong, Eddie wrote: #define WDT_RELOAD_VAL_REG 0x0004 #define WDT_RESTART_REG 0x0008 #define WDT_CTRL_REG0x000C #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
> 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
> 0x03f1)
>
> 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_REG0x000C
> #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
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> 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(>iomem, OBJECT(s), _wdt_ops, s,
> - TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
> + TYPE_ASPEED_WDT, awc->iosize);
> sysbus_init_mmio(sbd, >iomem);
> }
>
> --
> 2.38.1
>
Re: [PATCH v2 02/11] hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers
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
> > 0x03f1)
> >
> > 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_REG0x000C
> >#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.
A 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
> ...
> 7e785000-7e78501f (prio 0, i/o): aspeed.wdt
> 7e785020-7e78507f (prio -1000, i/o): aspeed.io
> @00185020
> 7e785080-7e78509f (prio 0, i/o): aspeed.wdt
> 7e7850a0-7e7850ff (prio -1000, i/o): aspeed.io
> @001850a0
> 7e785100-7e78511f (prio 0, i/o): aspeed.wdt
> 7e785120-7e78517f (prio -1000, i/o): aspeed.io
> @00185120
> 7e785180-7e78519f (prio 0, i/o): aspeed.wdt
> 7e7851a0-7e788fff (prio -1000, i/o): aspeed.io
> @001851a0
>
> After:
>
> (qemu) info mtree -f
> ...
> 7e785000-7e78507f (prio 0, i/o): aspeed.wdt
> 7e785080-7e7850ff (prio 0, i/o): aspeed.wdt
> 7e785100-7e78517f (prio 0, i/o): aspeed.wdt
> 7e785180-7e7851ff (prio 0, i/o): aspeed.wdt
> 7e785200-7e788fff (prio -1000, i/o): aspeed.io
> @00185200
> ---
>
> > [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
> > Signed-off-by: Philippe Mathieu-Daudé
> > ---
> > 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(>iomem, OBJECT(s), _wdt_ops, s,
> > - TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
> > + TYPE_ASPEED_WDT, awc->iosize);
> > sysbus_init_mmio(sbd, >iomem);
> > }
>
Re: [PATCH v2 02/11] hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers
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
0x03f1)
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_REG0x000C
#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
...
7e785000-7e78501f (prio 0, i/o): aspeed.wdt
7e785020-7e78507f (prio -1000, i/o): aspeed.io
@00185020
7e785080-7e78509f (prio 0, i/o): aspeed.wdt
7e7850a0-7e7850ff (prio -1000, i/o): aspeed.io
@001850a0
7e785100-7e78511f (prio 0, i/o): aspeed.wdt
7e785120-7e78517f (prio -1000, i/o): aspeed.io
@00185120
7e785180-7e78519f (prio 0, i/o): aspeed.wdt
7e7851a0-7e788fff (prio -1000, i/o): aspeed.io
@001851a0
After:
(qemu) info mtree -f
...
7e785000-7e78507f (prio 0, i/o): aspeed.wdt
7e785080-7e7850ff (prio 0, i/o): aspeed.wdt
7e785100-7e78517f (prio 0, i/o): aspeed.wdt
7e785180-7e7851ff (prio 0, i/o): aspeed.wdt
7e785200-7e788fff (prio -1000, i/o): aspeed.io
@00185200
---
[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
Signed-off-by: Philippe Mathieu-Daudé
---
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(>iomem, OBJECT(s), _wdt_ops, s,
- TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
+ TYPE_ASPEED_WDT, awc->iosize);
sysbus_init_mmio(sbd, >iomem);
}
[PATCH v2 02/11] hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers
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
0x03f1)
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_REG0x000C
#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
Signed-off-by: Philippe Mathieu-Daudé
---
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(>iomem, OBJECT(s), _wdt_ops, s,
- TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
+ TYPE_ASPEED_WDT, awc->iosize);
sysbus_init_mmio(sbd, >iomem);
}
--
2.38.1
