diff mbox series

[RFC,05/23] hw: add register access utility functions

Message ID 20240805201719.2345596-6-tavip@google.com (mailing list archive)
State New, archived
Headers show
Series NXP i.MX RT595, ARM SVD and device model unit tests | expand

Commit Message

Octavian Purdila Aug. 5, 2024, 8:17 p.m. UTC 
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 <tavip@google.com>
---
 include/hw/regs.h | 89 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)
 create mode 100644 include/hw/regs.h

Comments

Peter Maydell Aug. 12, 2024, 3:32 p.m. UTC | #1 
On Mon, 5 Aug 2024 at 21:17, Octavian Purdila <tavip@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 <tavip@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
Octavian Purdila Aug. 12, 2024, 9:14 p.m. UTC | #2 
On Mon, Aug 12, 2024 at 8:33 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 5 Aug 2024 at 21:17, Octavian Purdila <tavip@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 <tavip@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).
>

BITS is useful when defining masks from datasheets. Specifically, in
this patch set they are used to define writable bits. While it is
possible to use MAKE_64BIT_MASK, I find it less error prone to use
BITS.

However, it just occurred to me that we might be able to use the SVD
information to derive this. If possible I'll just remove it in v2
(otherwise I will move it to include/qemu/bitops.h).

> > +
> > +/*
> > + * 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.
>

Interesting, I was not aware of this thanks for pointing it out!

It seems a bit heavy to be used just for the write masking part, but
I'll take a closer look at it.

> > +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.
>

It is used for performing generic accesses to generated structs (patch
3/23) which should be aligned in the way that are used in the patch
set. If we decide to keep it, I'll add a note regarding alignment.

> thanks
> -- PMM
Richard Henderson Aug. 12, 2024, 10:35 p.m. UTC | #3 
On 8/13/24 07:14, Octavian Purdila wrote:
>>> +#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).
>>
> 
> BITS is useful when defining masks from datasheets. Specifically, in
> this patch set they are used to define writable bits. While it is
> possible to use MAKE_64BIT_MASK, I find it less error prone to use
> BITS.

The formulation of BITS is less than perfect: BITS(63, 62) cannot be evaluated because 
BIT(64) is undefined.


r~
Philippe Mathieu-Daudé Aug. 13, 2024, 8:28 a.m. UTC | #4 
On 12/8/24 23:14, Octavian Purdila wrote:
> On Mon, Aug 12, 2024 at 8:33 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Mon, 5 Aug 2024 at 21:17, Octavian Purdila <tavip@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 <tavip@google.com>
>>> ---
>>>   include/hw/regs.h | 89 +++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 89 insertions(+)
>>>   create mode 100644 include/hw/regs.h


>>> +/*
>>> + * 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.
>>
> 
> It is used for performing generic accesses to generated structs (patch
> 3/23) which should be aligned in the way that are used in the patch
> set. If we decide to keep it, I'll add a note regarding alignment.

Could we use ldl_he_p() instead?
diff mbox series

Patch

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)
+
+/*
+ * 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);
+ */
+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);
+}
+
+#endif /* HW_REGS_H */