On Wed, 7 Jul 2021 at 02:25, Richard Henderson
<richard.hender...@linaro.org> wrote:
>
> On 7/2/21 4:45 AM, Peter Maydell wrote:
> > On Fri, 2 Jul 2021 at 12:02, Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
> >> On 7/2/21 12:40 PM, Peter Maydell wrote:
> >>> static const VMStateDescription vmstate_pl061 = {
> >>> @@ -151,16 +150,9 @@ static uint64_t pl061_read(void *opaque, hwaddr
> >>> offset,
> >>> {
> >>> PL061State *s = (PL061State *)opaque;
> >>>
> >>> - if (offset < 0x400) {
> >>> - return s->data & (offset >> 2);
> >>> - }
> >>> - if (offset >= s->rsvd_start && offset <= 0xfcc) {
> >>> - goto err_out;
> >>> - }
> >>> - if (offset >= 0xfd0 && offset < 0x1000) {
> >>> - return s->id[(offset - 0xfd0) >> 2];
> >>> - }
> >>> switch (offset) {
> >>> + case 0x0 ... 0x3fc: /* Data */
> >>> + return s->data & (offset >> 2);
> >>
> >> Don't we need to set pl061_ops.impl.min/max_access_size = 4
> >> to keep the same logic?
> >
> > I think the hardware intends to permit accesses of any width, but only
> > at 4-byte boundaries. There is a slight behaviour change here:
> > accesses to 0x3fd, 0x3fe, 0x3ff now fall into the default case (ie error)
> > rather than being treated like 0x3fc, and similarly accesses to 0xfdd,
> > 0xfde, 0xfdf are errors rather than treated like 0xfdc. But I think
> > that it's probably more correct to consider those to be errors.
> >
> > (We could explicitly check and goto err_out if (offset & 3)
> > right at the top, I suppose.)
>
> Perhaps just better to retain current behaviour with this patch by extending
> the case to
> the ends.
Makes sense. I propose to squash this diff into this patch, which
should make it a no-behaviour-change refactor, and then queue the
series to target-arm.next, since it's now all reviewed except for
patch 11 which is a comment-only change. (If you think this is a bit
fast I'm happy to post a v2 instead -- as a bugfix patchset this is
still OK post-softfreeze, so it's just a matter of my personal convenience
to be able to put it into the arm pull I'm going to do either this
afternoon or tomorrow.)
diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index 0f5d12e6d5a..b21b230402f 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -151,7 +151,7 @@ static uint64_t pl061_read(void *opaque, hwaddr offset,
PL061State *s = (PL061State *)opaque;
switch (offset) {
- case 0x0 ... 0x3fc: /* Data */
+ case 0x0 ... 0x3ff: /* Data */
return s->data & (offset >> 2);
case 0x400: /* Direction */
return s->dir;
@@ -224,9 +224,7 @@ static uint64_t pl061_read(void *opaque, hwaddr offset,
goto bad_offset;
}
return s->amsel;
- case 0x52c ... 0xfcc: /* Reserved */
- goto bad_offset;
- case 0xfd0 ... 0xffc: /* ID registers */
+ case 0xfd0 ... 0xfff: /* ID registers */
return s->id[(offset - 0xfd0) >> 2];
default:
bad_offset:
@@ -244,7 +242,7 @@ static void pl061_write(void *opaque, hwaddr offset,
uint8_t mask;
switch (offset) {
- case 0 ... 0x3fc:
+ case 0 ... 0x3ff:
mask = (offset >> 2) & s->dir;
s->data = (s->data & ~mask) | (value & mask);
pl061_update(s);
thanks
-- PMM
