Re: [Qemu-arm] [PATCH] watchdog: wdt_aspeed: Add support for the reset width register

[Andrew Jeffery]

Hi Phil,

On Tue, 2017-08-01 at 00:23 -0300, Philippe Mathieu-Daudé wrote:
> Hi Andrew,
> 
> On 07/31/2017 10:04 PM, Andrew Jeffery wrote:
> > The reset width register controls how the pulse on the SoC's WDTRST{1,2}
> > pins behaves. A pulse is emitted if the external reset bit is set in
> > WDT_CTRL. WDT_RESET_WIDTH requires magic bit patterns to configure both
> > push-pull/open-drain and active-high/active-low behaviours and thus
> > needs some special handling in the write path.
> 
> I wanted to verify the datashit but it seems to unavailable, looking there:
> https://www.verical.com/datasheet/aspeed-technology-inc-interface-misc-ast2050a3-gp-4078885.pdf
> 
> Can you point out which cpu model you are modeling and where to get this 
> watchdog datashit please? You might also add this to the header, for the 
> next one looking at this file :)

The watchdog model is common to at least the Aspeed AST2400- and
AST2500- SoC families, which I have datasheets for. However, they are
not available to the public and therefore (unfortunately) I can't point
you to them. You may be able to access them if you have appropriate
arrangements with Aspeed.

Regarding the features exposed by the patch, configuration of the
electrical properties of the  external reset signal is only provided in
the AST2500's watchdog IP, but the bits used in the reset width
register are marked as reserved on the AST2400. Therefore I thought it
was reasonable not to guard them behind some kind of revision test.

> 
> > 
> > > > Signed-off-by: Andrew Jeffery <address@hidden>
> > ---
> > I understand that we're in stabilisation mode, but I thought I'd send this 
> > out
> > to provoke any feedback. Happy to resend after the 2.10 release if required.
> 
> you can subject it "PATCH for 2.11" so ppl testing/closing 2.10 can keep 
> focused but still queue your mail for when 2.10 release is out.

Thanks for the tip.

> 
> > 
> >   hw/watchdog/wdt_aspeed.c | 47 
> > +++++++++++++++++++++++++++++++++++++----------
> >   1 file changed, 37 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> > index 8bbe579b6b66..4ef1412e99fc 100644
> > --- a/hw/watchdog/wdt_aspeed.c
> > +++ b/hw/watchdog/wdt_aspeed.c
> > @@ -14,10 +14,10 @@
> >   #include "qemu/timer.h"
> >   #include "hw/watchdog/wdt_aspeed.h"
> >   
> > -#define WDT_STATUS              (0x00 / 4)
> > -#define WDT_RELOAD_VALUE        (0x04 / 4)
> > -#define WDT_RESTART             (0x08 / 4)
> > -#define WDT_CTRL                (0x0C / 4)
> > +#define WDT_STATUS                      (0x00 / 4)
> > +#define WDT_RELOAD_VALUE                (0x04 / 4)
> > +#define WDT_RESTART                     (0x08 / 4)
> > +#define WDT_CTRL                        (0x0C / 4)
> >   #define   WDT_CTRL_RESET_MODE_SOC       (0x00 << 5)
> >   #define   WDT_CTRL_RESET_MODE_FULL_CHIP (0x01 << 5)
> >   #define   WDT_CTRL_1MHZ_CLK             BIT(4)
> > @@ -25,12 +25,21 @@
> >   #define   WDT_CTRL_WDT_INTR             BIT(2)
> >   #define   WDT_CTRL_RESET_SYSTEM         BIT(1)
> >   #define   WDT_CTRL_ENABLE               BIT(0)
> > +#define WDT_RESET_WIDTH                 (0x18 / 4)
> > +#define   WDT_RESET_WIDTH_ACTIVE_HIGH   BIT(31)
> > +#define     WDT_POLARITY_MASK           (0xFF << 24)
> > +#define     WDT_ACTIVE_HIGH_MAGIC       (0xA5 << 24)
> > +#define     WDT_ACTIVE_LOW_MAGIC        (0x5A << 24)
> > +#define   WDT_RESET_WIDTH_PUSH_PULL     BIT(30)
> > +#define     WDT_DRIVE_TYPE_MASK         (0xFF << 24)
> > +#define     WDT_PUSH_PULL_MAGIC         (0xA8 << 24)
> > +#define     WDT_OPEN_DRAIN_MAGIC        (0x8A << 24)
> > +#define   WDT_RESET_WIDTH_DURATION      0xFFF;
> 
> Which model? the AST2050 seems to be 0xff.

Ah, good catch, this is also the case in the AST2400. The AST2500
extends this field. Host code for the AST2400 and AST2050 *shouldn't*
set any greater values; is it worth enforcing?

> 
> >   
> > -#define WDT_TIMEOUT_STATUS      (0x10 / 4)
> > -#define WDT_TIMEOUT_CLEAR       (0x14 / 4)
> > -#define WDT_RESET_WDITH         (0x18 / 4)
> > +#define WDT_TIMEOUT_STATUS              (0x10 / 4)
> > +#define WDT_TIMEOUT_CLEAR               (0x14 / 4)
> >   
> > -#define WDT_RESTART_MAGIC       0x4755
> > +#define WDT_RESTART_MAGIC               0x4755
> >   
> >   static bool aspeed_wdt_is_enabled(const AspeedWDTState *s)
> >   {
> > @@ -55,9 +64,10 @@ static uint64_t aspeed_wdt_read(void *opaque, hwaddr 
> > offset, unsigned size)
> >           return 0;
> >       case WDT_CTRL:
> >           return s->regs[WDT_CTRL];
> > +    case WDT_RESET_WIDTH:
> > +        return s->regs[WDT_RESET_WIDTH];
> >       case WDT_TIMEOUT_STATUS:
> >       case WDT_TIMEOUT_CLEAR:
> > -    case WDT_RESET_WDITH:
> >           qemu_log_mask(LOG_UNIMP,
> >                         "%s: uninmplemented read at offset 0x%" HWADDR_PRIx 
> > "\n",
> >                         __func__, offset);
> > @@ -119,9 +129,25 @@ static void aspeed_wdt_write(void *opaque, hwaddr 
> > offset, uint64_t data,
> >               timer_del(s->timer);
> >           }
> >           break;
> > +    case WDT_RESET_WIDTH:
> > +    {
> > +        uint32_t property = data & WDT_POLARITY_MASK;
> > +
> > +        if (property == WDT_ACTIVE_HIGH_MAGIC) {
> > +            s->regs[WDT_RESET_WIDTH] |= WDT_RESET_WIDTH_ACTIVE_HIGH;
> > +        } else if (property == WDT_ACTIVE_LOW_MAGIC) {
> > +            s->regs[WDT_RESET_WIDTH] &= ~WDT_RESET_WIDTH_ACTIVE_HIGH;
> > +        } else if (property == WDT_PUSH_PULL_MAGIC) {
> > +            s->regs[WDT_RESET_WIDTH] |= WDT_RESET_WIDTH_PUSH_PULL;
> > +        } else if (property == WDT_OPEN_DRAIN_MAGIC) {
> > +            s->regs[WDT_RESET_WIDTH] &= ~WDT_RESET_WIDTH_PUSH_PULL;
> 
> } else {
>      qemu_log_mask(LOG_GUEST_ERROR, ...
> 
> > +        }

From the datasheet: "For others value, this bit will keep old value
without change." So it is not a guest error to write a value not
matching the cases above.

> 
> Anyway I'm not sure about this if().
> Usually watchdogs have a state machine, if you don't do all unlock steps 
> ordered, the SM get reset. This is why magic is involved, else you could 
> use it as a regular register.
> I'd expect a guest writing ACTIVE_HIGH_MAGIC then PUSH_PULL_MAGIC to not 
> modify the RESET_WIDTH register, since the correct behavior would be to 
> write ordered RESTART_MAGIC, then HIGH_MAGIC, then LOW_MAGIC and finally 
> the PULL/DRAIN change, but I'm just trying to model this wdg in my head 
> without having study the DS so you can't rely on my comments :)

Well, given I have it handy, I think the behaviour of the hardware
resolves this query:

address@hidden:~# devmem 0x1e785018
0x000000FF
address@hidden:~# devmem 0x1e785018 32 0xa80000ff
address@hidden:~# devmem 0x1e785018
0x400000FF
address@hidden:~# devmem 0x1e785018 32 0x8a0000ff
address@hidden:~# devmem 0x1e785018
0x000000FF
address@hidden:~# devmem 0x1e785018 32 0xa50000ff
address@hidden:~# devmem 0x1e785018
0x800000FF
address@hidden:~# devmem 0x1e785018 32 0x5a0000ff
address@hidden:~# devmem 0x1e785018
0x000000FF
address@hidden:~# devmem 0x1e785018 32 0xa80000ff
address@hidden:~# devmem
0x1e785018 32 0xa50000ff
address@hidden:~# devmem 0x1e785018
0xC00000FF
root
@romulus:~# devmem 0x1e785018 32 0x5a0000ff
address@hidden:~# devmem
0x1e785018 32 0x8a0000ff
address@hidden:~# devmem 0x1e785018
0x000000FF
address@hidden:~# devmem 0x1e785018 32 0xa50000ff
address@hidden:~# devmem 0x1e785018
0x800000FF
address@hidden:~# devmem 0x1e785018 32 0x000a5a5a
address@hidden:~# devmem
0x1e785018
0x800A5A5A
address@hidden:~# devmem 0x1e785018 32 0x5a0000ff
address@hidden:~# devmem 0x1e785018
0x000000FF

Bear in mind the driver is loaded, so maybe it's messing with the test,
but I haven't seen any indication to the contrary.

> 
> Also it seems unsafe to modify that kind of property while the watchdog 
> is running, usually you disable it before modifying it, else while 
> running changes are automagically ignored.

But that's a host-code problem, right? I don't see any hardware
restriction noted in the datasheet along these lines.

> 
> > +        s->regs[WDT_RESET_WIDTH] &= ~WDT_RESET_WIDTH_DURATION;
> > +        s->regs[WDT_RESET_WIDTH] |= data & WDT_RESET_WIDTH_DURATION;
> > +        break;
> > +    }
> >       case WDT_TIMEOUT_STATUS:
> >       case WDT_TIMEOUT_CLEAR:
> > -    case WDT_RESET_WDITH:
> >           qemu_log_mask(LOG_UNIMP,
> >                         "%s: uninmplemented write at offset 0x%" 
> > HWADDR_PRIx "\n",
> >                         __func__, offset);
> > @@ -167,6 +193,7 @@ static void aspeed_wdt_reset(DeviceState *dev)
> >       s->regs[WDT_RELOAD_VALUE] = 0x03EF1480;
> >       s->regs[WDT_RESTART] = 0;
> >       s->regs[WDT_CTRL] = 0;
> > +    s->regs[WDT_RESET_WIDTH] = 0XFF;
> 
> why different than your define WDT_RESET_WIDTH_DURATION?

Maybe WDT_RESET_WIDTH_DURATION is poorly named, but it's a mask, not a
value. The 0xFF here (somehow I got an upper-case X in there) is the
default value for the register as documented in both the AST2400 and
AST2500 datasheets. However, this *has* lead me to discover that I've
defined the WDT_RESET_WIDTH_DURATION mask too narrow for the AST2500,
it should be 0xFFFFF (19:0). The AST2400 datasheet documents the field
as 7:0 (0xFF).

I'll respin, taking into account the 2.11 note above, unless you have
any more comments.

Thanks for taking a look!

Andrew

> >   
> >       timer_del(s->timer);
> >   }
> > 
> 
> Regards,
> 
> Phil.

signature.asc
Description: This is a digitally signed message part