| Message ID | 20221230113504.37032-3-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/arm/aspeed_ast10x0: Map more peripherals & few more fixes | expand |
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); > } >
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); > > } >
> 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 >
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.
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 >> >
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 > > > > > >
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.
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. >
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 >>> >> >
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); }
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(-)