RE: [PATCH for-7.1] hw/misc/grlib_ahb_apb_pnp: Support 8 and 16 bit accesses

[Konrad, Frederic]

Hi Peter,

CC'ing Philippe.

> -----Original Message-----
> From: Qemu-devel <qemu-devel-
> bounces+fkonrad=amd.com@nongnu.org> On Behalf Of Peter Maydell
> Sent: 02 August 2022 14:19
> To: qemu-devel@nongnu.org
> Cc: Fabien Chouteau <chouteau@adacore.com>; Frederic Konrad
> <konrad.frederic@yahoo.fr>
> Subject: [PATCH for-7.1] hw/misc/grlib_ahb_apb_pnp: Support 8 and 16 bit
> accesses
> 
> In real hardware, the APB and AHB PNP data tables can be accessed
> with byte and halfword reads as well as word reads.  Our
> implementation currently only handles word reads.  Add support for
> the 8 and 16 bit accesses.  Note that we only need to handle aligned
> accesses -- unaligned accesses should continue to trap, as happens on
> hardware.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1132
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> It would be nice if we could just set the .valid.min_access_size in
> the MemoryRegionOps to 1 and have the memory system core synthesize
> the 1 and 2 byte accesses from a 4 byte read, but currently that
> doesn't work (see various past mailing list threads).

That looks good to me but I thought this was fixed by 1a5a5570 and 0fbe394a
because RTEMS do bytes accesses?

Did that break at some point?

Regards,
Fred

> ---
>  hw/misc/grlib_ahb_apb_pnp.c | 10 ++++++----
>  hw/misc/trace-events        |  4 ++--
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/misc/grlib_ahb_apb_pnp.c b/hw/misc/grlib_ahb_apb_pnp.c
> index 43e001c3c7b..5b05f158592 100644
> --- a/hw/misc/grlib_ahb_apb_pnp.c
> +++ b/hw/misc/grlib_ahb_apb_pnp.c
> @@ -136,7 +136,8 @@ static uint64_t grlib_ahb_pnp_read(void *opaque,
> hwaddr offset, unsigned size)
>      uint32_t val;
> 
>      val = ahb_pnp->regs[offset >> 2];
> -    trace_grlib_ahb_pnp_read(offset, val);
> +    val = extract32(val, (4 - (offset & 3) - size) * 8, size * 8);
> +    trace_grlib_ahb_pnp_read(offset, size, val);
> 
>      return val;
>  }
> @@ -152,7 +153,7 @@ static const MemoryRegionOps grlib_ahb_pnp_ops =
> {
>      .write      = grlib_ahb_pnp_write,
>      .endianness = DEVICE_BIG_ENDIAN,
>      .impl = {
> -        .min_access_size = 4,
> +        .min_access_size = 1,
>          .max_access_size = 4,
>      },
>  };
> @@ -247,7 +248,8 @@ static uint64_t grlib_apb_pnp_read(void *opaque,
> hwaddr offset, unsigned size)
>      uint32_t val;
> 
>      val = apb_pnp->regs[offset >> 2];
> -    trace_grlib_apb_pnp_read(offset, val);
> +    val = extract32(val, (4 - (offset & 3) - size) * 8, size * 8);
> +    trace_grlib_apb_pnp_read(offset, size, val);
> 
>      return val;
>  }
> @@ -263,7 +265,7 @@ static const MemoryRegionOps grlib_apb_pnp_ops =
> {
>      .write      = grlib_apb_pnp_write,
>      .endianness = DEVICE_BIG_ENDIAN,
>      .impl = {
> -        .min_access_size = 4,
> +        .min_access_size = 1,
>          .max_access_size = 4,
>      },
>  };
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index 4d51a80de1d..c18bc0605e8 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -247,8 +247,8 @@ via1_adb_poll(uint8_t data, const char *vadbint, int
> status, int index, int size
>  via1_auxmode(int mode) "setting auxmode to %d"
> 
>  # grlib_ahb_apb_pnp.c
> -grlib_ahb_pnp_read(uint64_t addr, uint32_t value) "AHB PnP read
> addr:0x%03"PRIx64" data:0x%08x"
> -grlib_apb_pnp_read(uint64_t addr, uint32_t value) "APB PnP read
> addr:0x%03"PRIx64" data:0x%08x"
> +grlib_ahb_pnp_read(uint64_t addr, unsigned size, uint32_t value) "AHB PnP
> read addr:0x%03"PRIx64" size:%u data:0x%08x"
> +grlib_apb_pnp_read(uint64_t addr, unsigned size, uint32_t value) "APB PnP
> read addr:0x%03"PRIx64" size:%u data:0x%08x"
> 
>  # led.c
>  led_set_intensity(const char *color, const char *desc, uint8_t
> intensity_percent) "LED desc:'%s' color:%s intensity: %u%%"
> --
> 2.25.1
>