On Mon, 5 Aug 2024 at 21:17, Octavian Purdila <ta...@google.com> wrote:
>
> Add register access utility functions for device models, like checking
> aligned access and reading and writing to a register backstore.
> Signed-off-by: Octavian Purdila <ta...@google.com>
> ---
> include/hw/regs.h | 89 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 89 insertions(+)
> create mode 100644 include/hw/regs.h
>
> diff --git a/include/hw/regs.h b/include/hw/regs.h
> new file mode 100644
> index 0000000000..8d0da0629d
> --- /dev/null
> +++ b/include/hw/regs.h
> @@ -0,0 +1,89 @@
> +/*
> + * Useful macros/functions for register handling.
> + *
> + * Copyright (c) 2021 Google LLC
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef HW_REGS_H
> +#define HW_REGS_H
> +
> +#include "exec/hwaddr.h"
> +#include "exec/memattrs.h"
> +
> +#define BITS(hi, lo) (BIT(hi + 1) - BIT(lo))
> +#define BIT_IS_SET(v, b) (((v) & BIT(b)) != 0)
To the extent we need these we should be putting them in
bits.h with the other bit-related operations. (But
prefer the existing MAKE_64BIT_MASK over adding a
second macro that evaluates to a mask with a given
run of bits set).
> +
> +/*
> + * reg32_aligned_access
> + * @addr: address to check
> + * @size: size of access
> + *
> + * Check if access to a hardware address is 32bit aligned.
> + *
> + * Returns: true if access is 32bit aligned, false otherwise
> + */
> +static inline bool reg32_aligned_access(hwaddr addr, unsigned size)
> +{
> + if (size != 4 || addr % 4 != 0) {
> + return false;
> + }
> + return true;
> +}
> +
> +/*
> + * reg32_write
> + * @base: base address
> + * @addr: register offset in bytes
> + * @val: value to write
> + * @wr_bits_array: RW bitmask array
> + *
> + * Update the RW/WO bits of a 32bit register backstore with a given value
> + * (discarding updats to the RO bits). The RW/WO bits are encoded in the
> + * @wr_bits_array with bits set being RW and bits unset being RO.
> + *
> + * Usage example:
> + *
> + * wr_bits_array[] = {
> + * [REG1_ADDR/4] = 0xFF000000, // MSB byte writable
> + * [REG2_ADDR/4] = 0xFF, // LSB byte writable
> + * // all other registers are read-only
> + * };
> + *
> + * // backstore is updated to 0x12000000
> + * reg32_write(&backstore, REG1_ADDR, 0x12345678, wr_bits_array);
> + * // backstore is updated to 0x78
> + * reg32_write(&backstore, REG2_ADDR, 0x12345678, wr_bits_array);
> + */
This seems like it's reimplementing include/hw/register.h
functionality. I'm not personally a super-fan of that API,
but I definitely would prefer it if we didn't have more than
one way to do this.
> +static inline uint32_t reg32_write(void *base, uint32_t off, uint32_t val,
> + const uint32_t *rw_bits_array)
> +{
> + uint32_t *ptr = base + addr;
> + uint32_t old_value = *ptr;
> + uint32_t mask = rw_bits_array ? rw_bits_array[addr / 4] : 0xFFFFFFFF;
> +
> + /* set WO/RW bits */
> + *ptr |= val & mask;
> + /* clear RO bits */
> + *ptr &= val | ~mask;
> +
> + return old_value;
> +}
> +
> +/*
> + * reg32_read
> + * @base: base address
> + * @addr: register offset in bytes
> + *
> + * Returns: 32bit value from register backstore
> + */
> +static inline uint32_t reg32_read(void *base, uint32_t addr)
> +{
> + return *(uint32_t *)(base + addr);
> +}
Pointer type handling looks suspicious here -- if
the thing we're accessing is really a uint32_t* then
we should take that; if it isn't then casting it to
one and dereferencing might be reading unaligned memory.
thanks
-- PMM
