Message ID | 1466063287-19350-2-git-send-email-andrew@aj.id.au (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 16 June 2016 at 08:48, Andrew Jeffery <andrew@aj.id.au> wrote: > The SCU is a collection of chip-level control registers that manage the > various functions supported by the AST2400. Typically the bits control > interactions with clocks, external hardware or reset behaviour, and we > can largly take a hands-off approach to reads and writes. > > Firmware makes heavy use of the state to determine how to boot, but the > reset values vary from SoC to SoC. qdev properties are exposed so that > the integrating SoC model can configure the appropriate reset values. > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > Reviewed-by: Cédric Le Goater <clg@kaod.org> > Reviewed-by: Joel Stanley <joel@jms.id.au> > --- > +static Property aspeed_scu_properties[] = { > + DEFINE_PROP_ARRAY("reset", AspeedSCUState, num_resets, reset, > + qdev_prop_uint32, uint32_t), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > +#define ASPEED_SCU_NR_REGS (0x1A8 >> 2) This seems like a very unwieldy way of specifying the reset values for this device. Are they really all fully configurable in the hardware? It seems unlikely. I'd much rather see something that looks more like what you might plausibly be configuring when wiring up the SoC, which might be some version/revision numbers and/or some particular tweakable parameters. thanks -- PMM
On Fri, 2016-06-17 at 15:22 +0100, Peter Maydell wrote: On 16 June 2016 at 08:48, Andrew Jeffery <andrew@aj.id.au> wrote: The SCU is a collection of chip-level control registers that manage the various functions supported by the AST2400. Typically the bits control interactions with clocks, external hardware or reset behaviour, and we can largly take a hands-off approach to reads and writes. Firmware makes heavy use of the state to determine how to boot, but the reset values vary from SoC to SoC. qdev properties are exposed so that the integrating SoC model can configure the appropriate reset values. Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Reviewed-by: Cédric Le Goater <clg@kaod.org> Reviewed-by: Joel Stanley <joel@jms.id.au> --- +static Property aspeed_scu_properties[] = { + DEFINE_PROP_ARRAY("reset", AspeedSCUState, num_resets, reset, + qdev_prop_uint32, uint32_t), + DEFINE_PROP_END_OF_LIST(), +}; + +#define ASPEED_SCU_NR_REGS (0x1A8 >> 2) This seems like a very unwieldy way of specifying the reset values for this device. Are they really all fully configurable in the hardware? It seems unlikely. I'd much rather see something that looks more like what you might plausibly be configuring when wiring up the SoC, which might be some version/revision numbers and/or some particular tweakable parameters. Right. I left out some context which may clear things up: We are working with two SoCs at the moment, the AST2400 and AST2500 (hopefully the AST2500 patches will be sent to the list soon). I wanted to abstract the configuration to cater for the differences in register values between the SoCs, less so for wiring the one SoC up in a different fashion. For what it's worth, out of 86 registers defined in the IO space between the two SoCs, 37 take the same value and 49 differ. I can rework the commit message to say as much. Separately, the qdev array approach was lifted from your commit 9c7d489379c2 hw/vexpress: Set reset values for daughterboard oscillators. Another approach would be to set the members of the SCU's reset array directly as we know the type. That seems less convoluted but my gut instinct was that wasn't how we should be configuring the devices. Is qdev the right way to go in the SCU's case? Cheers, Andrew
On 20 June 2016 at 04:44, Andrew Jeffery <andrew@aj.id.au> wrote: > On Fri, 2016-06-17 at 15:22 +0100, Peter Maydell wrote: >> +static Property aspeed_scu_properties[] = { >> + DEFINE_PROP_ARRAY("reset", AspeedSCUState, num_resets, reset, >> + qdev_prop_uint32, uint32_t), >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + >> +#define ASPEED_SCU_NR_REGS (0x1A8 >> 2) >> This seems like a very unwieldy way of specifying the reset values >> for this device. Are they really all fully configurable in the >> hardware? It seems unlikely. I'd much rather see something that >> looks more like what you might plausibly be configuring when wiring >> up the SoC, which might be some version/revision numbers and/or >> some particular tweakable parameters. > > Right. I left out some context which may clear things up: We are > working with two SoCs at the moment, the AST2400 and AST2500 (hopefully > the AST2500 patches will be sent to the list soon). I wanted to > abstract the configuration to cater for the differences in register > values between the SoCs, less so for wiring the one SoC up in a > different fashion. For what it's worth, out of 86 registers defined in > the IO space between the two SoCs, 37 take the same value and 49 > differ. I think there are a couple of plausible ways you might model this: (a) just have a single property for "revision" which corresponds to the revision of this bit of silicon IP within the SoC; the model of the device itself then knows what the reset state is for this revision of the device. (b) ditto, but also have some configurable flags where relevant (ie approximately where it's the same IP rev within the SoC but it's been configured by tying down different config lines; for instance hw/dma/pl330.c has a collection of properties which match the configurable knobs for the hardware.) You might or might not have enough visibility into the thing to know which of these is closest to what the real hardware is doing; if not then it's a matter of taste, looking at what is varying between the two and what isn't, etc. But "board level specifies all the register reset values" is definitely far too broad and generalised an API, I think. > Separately, the qdev array approach was lifted from your commit > 9c7d489379c2 hw/vexpress: Set reset values for daughterboard > oscillators. You'll notice that we only configure the specific things that need configuring with interfaces specific to those things (eg "daughterboard clocks" and "daughterboard voltages" are separate), not a single "have a complete set of register values" API. > Another approach would be to set the members of the SCU's > reset array directly as we know the type. That seems less convoluted > but my gut instinct was that wasn't how we should be configuring the > devices. Is qdev the right way to go in the SCU's case? We should definitely use qdev rather than poking directly at the device's internal state struct. thanks -- PMM
On Mon, 2016-06-20 at 14:57 +0100, Peter Maydell wrote: > On 20 June 2016 at 04:44, Andrew Jeffery <andrew@aj.id.au> wrote: > > > > On Fri, 2016-06-17 at 15:22 +0100, Peter Maydell wrote: > > > > > > +static Property aspeed_scu_properties[] = { > > > + DEFINE_PROP_ARRAY("reset", AspeedSCUState, num_resets, reset, > > > + qdev_prop_uint32, uint32_t), > > > + DEFINE_PROP_END_OF_LIST(), > > > +}; > > > + > > > +#define ASPEED_SCU_NR_REGS (0x1A8 >> 2) > > > This seems like a very unwieldy way of specifying the reset values > > > for this device. Are they really all fully configurable in the > > > hardware? It seems unlikely. I'd much rather see something that > > > looks more like what you might plausibly be configuring when wiring > > > up the SoC, which might be some version/revision numbers and/or > > > some particular tweakable parameters. > > Right. I left out some context which may clear things up: We are > > working with two SoCs at the moment, the AST2400 and AST2500 (hopefully > > the AST2500 patches will be sent to the list soon). I wanted to > > abstract the configuration to cater for the differences in register > > values between the SoCs, less so for wiring the one SoC up in a > > different fashion. For what it's worth, out of 86 registers defined in > > the IO space between the two SoCs, 37 take the same value and 49 > > differ. > I think there are a couple of plausible ways you might model this: > > (a) just have a single property for "revision" which corresponds > to the revision of this bit of silicon IP within the SoC; the > model of the device itself then knows what the reset state is > for this revision of the device. > (b) ditto, but also have some configurable flags where relevant > (ie approximately where it's the same IP rev within the SoC > but it's been configured by tying down different config lines; > for instance hw/dma/pl330.c has a collection of properties > which match the configurable knobs for the hardware.) Okay. I think (b) is the most appropriate. The board-controllable bits are primarily in the hardware strapping register. The register is composed of fields of mostly unrelated bits, so we could go two ways here: (1) expose the register through a single 32bit property (2) break out a property for each bitfield Do you have a preference? grepping the tree suggests there isn't a precedent for "large" numbers of properties* so maybe (2) is overkill, but (1) feels like it might fit into the overly-general-interface problem that we're trying to eliminate. * Seems the microblaze CPU defines the most with 9 properties? Approach (2) will leave us with 21 properties for the SCU. $ git grep -c DEFINE_PROP | sort -t: -k2 -r | head -n1 target-microblaze/cpu.c:9 > > You might or might not have enough visibility into the thing to > know which of these is closest to what the real hardware is doing; > if not then it's a matter of taste, looking at what is varying > between the two and what isn't, etc. But "board level specifies > all the register reset values" is definitely far too broad > and generalised an API, I think. > > > > > Separately, the qdev array approach was lifted from your commit > > 9c7d489379c2 hw/vexpress: Set reset values for daughterboard > > oscillators. > You'll notice that we only configure the specific things > that need configuring with interfaces specific to those things > (eg "daughterboard clocks" and "daughterboard voltages" are > separate), not a single "have a complete set of register values" API. Yes, I appreciate that now. Thanks. Andrew
On 21 June 2016 at 04:49, Andrew Jeffery <andrew@aj.id.au> wrote: > On Mon, 2016-06-20 at 14:57 +0100, Peter Maydell wrote: >> I think there are a couple of plausible ways you might model this: >> >> (a) just have a single property for "revision" which corresponds >> to the revision of this bit of silicon IP within the SoC; the >> model of the device itself then knows what the reset state is >> for this revision of the device. >> (b) ditto, but also have some configurable flags where relevant >> (ie approximately where it's the same IP rev within the SoC >> but it's been configured by tying down different config lines; >> for instance hw/dma/pl330.c has a collection of properties >> which match the configurable knobs for the hardware.) > > Okay. I think (b) is the most appropriate. The board-controllable bits > are primarily in the hardware strapping register. The register is > composed of fields of mostly unrelated bits, so we could go two ways > here: > > (1) expose the register through a single 32bit property > (2) break out a property for each bitfield > > Do you have a preference? grepping the tree suggests there isn't a > precedent for "large" numbers of properties* so maybe (2) is overkill, > but (1) feels like it might fit into the overly-general-interface > problem that we're trying to eliminate. > > * Seems the microblaze CPU defines the most with 9 properties? (You forgot '-n' on your sort rune.) hw/dma/pl330.c has 15. > Approach (2) will leave us with 21 properties for the SCU. What would the 21 properties be in this case? But yes, a single 32 bit property would probably be better. thanks -- PMM
On Tue, 2016-06-21 at 07:56 +0100, Peter Maydell wrote: > On 21 June 2016 at 04:49, Andrew Jeffery <andrew@aj.id.au> wrote: > > > > On Mon, 2016-06-20 at 14:57 +0100, Peter Maydell wrote: > > > > > > I think there are a couple of plausible ways you might model this: > > > > > > (a) just have a single property for "revision" which corresponds > > > to the revision of this bit of silicon IP within the SoC; the > > > model of the device itself then knows what the reset state is > > > for this revision of the device. > > > (b) ditto, but also have some configurable flags where relevant > > > (ie approximately where it's the same IP rev within the SoC > > > but it's been configured by tying down different config lines; > > > for instance hw/dma/pl330.c has a collection of properties > > > which match the configurable knobs for the hardware.) > > Okay. I think (b) is the most appropriate. The board-controllable bits > > are primarily in the hardware strapping register. The register is > > composed of fields of mostly unrelated bits, so we could go two ways > > here: > > > > (1) expose the register through a single 32bit property > > (2) break out a property for each bitfield > > > > Do you have a preference? grepping the tree suggests there isn't a > > precedent for "large" numbers of properties* so maybe (2) is overkill, > > but (1) feels like it might fit into the overly-general-interface > > problem that we're trying to eliminate. > > > > * Seems the microblaze CPU defines the most with 9 properties? > (You forgot '-n' on your sort rune.) > hw/dma/pl330.c has 15. Ugh. > > > > > Approach (2) will leave us with 21 properties for the SCU. > What would the 21 properties be in this case? > > But yes, a single 32 bit property would probably be better. Alright, I'll go ahead with that. Thanks for the feedback. Andrew
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs index ffb49c11aca6..54020aa06c00 100644 --- a/hw/misc/Makefile.objs +++ b/hw/misc/Makefile.objs @@ -52,3 +52,4 @@ obj-$(CONFIG_PVPANIC) += pvpanic.o obj-$(CONFIG_EDU) += edu.o obj-$(CONFIG_HYPERV_TESTDEV) += hyperv_testdev.o obj-$(CONFIG_AUX) += aux.o +obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c new file mode 100644 index 000000000000..492cf82af425 --- /dev/null +++ b/hw/misc/aspeed_scu.c @@ -0,0 +1,147 @@ +/* + * ASPEED System Control Unit + * + * Andrew Jeffery <andrew@aj.id.au> + * + * Copyright 2016 IBM Corp. + * + * This code is licensed under the GPL version 2 or later. See + * the COPYING file in the top-level directory. + */ + +#include "qemu/osdep.h" +#include <inttypes.h> +#include "hw/misc/aspeed_scu.h" +#include "hw/qdev-properties.h" +#include "qapi/error.h" +#include "qapi/visitor.h" +#include "qemu/bitops.h" +#include "trace.h" + +#define SCU_KEY 0x1688A8A8 +#define SCU_IO_REGION_SIZE 0x20000 + +static uint64_t aspeed_scu_read(void *opaque, hwaddr offset, unsigned size) +{ + AspeedSCUState *s = ASPEED_SCU(opaque); + + if (ASPEED_SCU_TO_REG(offset) >= ARRAY_SIZE(s->regs)) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n", + __func__, offset); + return 0; + } + + switch (offset) { + case ASPEED_SCU_WAKEUP_EN: + qemu_log_mask(LOG_GUEST_ERROR, + "%s: Read of write-only offset 0x%" HWADDR_PRIx "\n", + __func__, offset); + break; + } + + return s->regs[ASPEED_SCU_TO_REG(offset)]; +} + +static void aspeed_scu_write(void *opaque, hwaddr offset, uint64_t data, + unsigned size) +{ + AspeedSCUState *s = ASPEED_SCU(opaque); + + if (ASPEED_SCU_TO_REG(offset) >= ARRAY_SIZE(s->regs)) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx "\n", + __func__, offset); + return; + } + + if (offset > ASPEED_SCU_PROT_KEY && offset < ASPEED_SCU_CPU2_BASE_SEG1 && + s->regs[ASPEED_SCU_TO_REG(ASPEED_SCU_PROT_KEY)] != SCU_KEY) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: SCU is locked!\n", __func__); + return; + } + + trace_aspeed_scu_write(offset, size, data); + + switch (offset) { + case ASPEED_SCU_FREQ_CNTR_EVAL: + case ASPEED_SCU_VGA_SCRATCH1 ... ASPEED_SCU_VGA_SCRATCH8: + case ASPEED_SCU_RNG_DATA: + case ASPEED_SCU_REV_ID: + case ASPEED_SCU_FREE_CNTR4: + case ASPEED_SCU_FREE_CNTR4_EXT: + qemu_log_mask(LOG_GUEST_ERROR, + "%s: Write to read-only offset 0x%" HWADDR_PRIx "\n", + __func__, offset); + return; + } + + s->regs[ASPEED_SCU_TO_REG(offset)] = (uint32_t) data; +} + +static const MemoryRegionOps aspeed_scu_ops = { + .read = aspeed_scu_read, + .write = aspeed_scu_write, + .endianness = DEVICE_LITTLE_ENDIAN, + .valid.min_access_size = 4, + .valid.max_access_size = 4, + .valid.unaligned = false, +}; + +static void aspeed_scu_reset(DeviceState *dev) +{ + AspeedSCUState *s = ASPEED_SCU(dev); + + memcpy(s->regs, s->reset, sizeof(s->regs)); +} + +static void aspeed_scu_realize(DeviceState *dev, Error **errp) +{ + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); + AspeedSCUState *s = ASPEED_SCU(dev); + + memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_scu_ops, s, + TYPE_ASPEED_SCU, SCU_IO_REGION_SIZE); + + sysbus_init_mmio(sbd, &s->iomem); +} + +static const VMStateDescription vmstate_aspeed_scu = { + .name = "aspeed.new-vic", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT32_ARRAY(regs, AspeedSCUState, ASPEED_SCU_NR_REGS), + VMSTATE_END_OF_LIST() + } +}; + +static Property aspeed_scu_properties[] = { + DEFINE_PROP_ARRAY("reset", AspeedSCUState, num_resets, reset, + qdev_prop_uint32, uint32_t), + DEFINE_PROP_END_OF_LIST(), +}; + +static void aspeed_scu_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + dc->realize = aspeed_scu_realize; + dc->reset = aspeed_scu_reset; + dc->desc = "ASPEED System Control Unit"; + dc->vmsd = &vmstate_aspeed_scu; + dc->props = aspeed_scu_properties; +} + +static const TypeInfo aspeed_scu_info = { + .name = TYPE_ASPEED_SCU, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(AspeedSCUState), + .class_init = aspeed_scu_class_init, +}; + +static void aspeed_scu_register_types(void) +{ + type_register_static(&aspeed_scu_info); +} + +type_init(aspeed_scu_register_types); diff --git a/include/hw/misc/aspeed_scu.h b/include/hw/misc/aspeed_scu.h new file mode 100644 index 000000000000..f95c5c8a7d74 --- /dev/null +++ b/include/hw/misc/aspeed_scu.h @@ -0,0 +1,108 @@ +/* + * ASPEED System Control Unit + * + * Andrew Jeffery <andrew@aj.id.au> + * + * Copyright 2016 IBM Corp. + * + * This code is licensed under the GPL version 2 or later. See + * the COPYING file in the top-level directory. + */ +#ifndef ASPEED_SCU_H +#define ASPEED_SCU_H + +#include "hw/sysbus.h" + +#define TYPE_ASPEED_SCU "aspeed.scu" +#define ASPEED_SCU(obj) OBJECT_CHECK(AspeedSCUState, (obj), TYPE_ASPEED_SCU) + +#define ASPEED_SCU_NR_REGS (0x1A8 >> 2) + +#define ASPEED_SCU_PROT_KEY 0x00 +#define ASPEED_SCU_SYS_RST_CTRL 0x04 +#define ASPEED_SCU_CLK_SEL 0x08 +#define ASPEED_SCU_CLK_STOP_CTRL 0x0C +#define ASPEED_SCU_FREQ_CNTR_CTRL 0x10 +#define ASPEED_SCU_FREQ_CNTR_EVAL 0x14 +#define ASPEED_SCU_IRQ_CTRL 0x18 +#define ASPEED_SCU_D2PLL_PARAM 0x1C +#define ASPEED_SCU_MPLL_PARAM 0x20 +#define ASPEED_SCU_HPLL_PARAM 0x24 +#define ASPEED_SCU_FREQ_CNTR_RANGE 0x28 +#define ASPEED_SCU_MISC_CTRL1 0x2C +#define ASPEED_SCU_PCI_CTRL1 0x30 +#define ASPEED_SCU_PCI_CTRL2 0x34 +#define ASPEED_SCU_PCI_CTRL3 0x38 +#define ASPEED_SCU_SYS_RST_STATUS 0x3C +#define ASPEED_SCU_SOC_SCRATCH1 0x40 +#define ASPEED_SCU_SOC_SCRATCH2 0x44 +#define ASPEED_SCU_MAC_CLK_DELAY 0x48 +#define ASPEED_SCU_MISC_CTRL2 0x4C +#define ASPEED_SCU_VGA_SCRATCH1 0x50 +#define ASPEED_SCU_VGA_SCRATCH2 0x54 +#define ASPEED_SCU_VGA_SCRATCH3 0x58 +#define ASPEED_SCU_VGA_SCRATCH4 0x5C +#define ASPEED_SCU_VGA_SCRATCH5 0x60 +#define ASPEED_SCU_VGA_SCRATCH6 0x64 +#define ASPEED_SCU_VGA_SCRATCH7 0x68 +#define ASPEED_SCU_VGA_SCRATCH8 0x6C +#define ASPEED_SCU_HW_STRAP1 0x70 +#define ASPEED_SCU_RNG_CTRL 0x74 +#define ASPEED_SCU_RNG_DATA 0x78 +#define ASPEED_SCU_REV_ID 0x7C +#define ASPEED_SCU_PINMUX_CTRL1 0x80 +#define ASPEED_SCU_PINMUX_CTRL2 0x84 +#define ASPEED_SCU_PINMUX_CTRL3 0x88 +#define ASPEED_SCU_PINMUX_CTRL4 0x8C +#define ASPEED_SCU_PINMUX_CTRL5 0x90 +#define ASPEED_SCU_PINMUX_CTRL6 0x94 +#define ASPEED_SCU_WDT_RST_CTRL 0x9C +#define ASPEED_SCU_PINMUX_CTRL7 0xA0 +#define ASPEED_SCU_PINMUX_CTRL8 0xA4 +#define ASPEED_SCU_PINMUX_CTRL9 0xA8 +#define ASPEED_SCU_WAKEUP_EN 0xC0 +#define ASPEED_SCU_WAKEUP_CTRL 0xC4 +#define ASPEED_SCU_HW_STRAP2 0xD0 +#define ASPEED_SCU_FREE_CNTR4 0xE0 +#define ASPEED_SCU_FREE_CNTR4_EXT 0xE4 +#define ASPEED_SCU_CPU2_CTRL 0x100 +#define ASPEED_SCU_CPU2_BASE_SEG1 0x104 +#define ASPEED_SCU_CPU2_BASE_SEG2 0x108 +#define ASPEED_SCU_CPU2_BASE_SEG3 0x10C +#define ASPEED_SCU_CPU2_BASE_SEG4 0x110 +#define ASPEED_SCU_CPU2_BASE_SEG5 0x114 +#define ASPEED_SCU_CPU2_CACHE_CTRL 0x118 +#define ASPEED_SCU_UART_HPLL_CLK 0x160 +#define ASPEED_SCU_PCIE_CTRL 0x180 +#define ASPEED_SCU_BMC_MMIO_CTRL 0x184 +#define ASPEED_SCU_RELOC_DECODE_BASE1 0x188 +#define ASPEED_SCU_RELOC_DECODE_BASE2 0x18C +#define ASPEED_SCU_MAILBOX_DECODE_BASE 0x190 +#define ASPEED_SCU_SRAM_DECODE_BASE1 0x194 +#define ASPEED_SCU_SRAM_DECODE_BASE2 0x198 +#define ASPEED_SCU_BMC_REV_ID 0x19C +#define ASPEED_SCU_BMC_DEV_ID 0x1A4 + +#define ASPEED_SCU_TO_REG(offset) ((offset) >> 2) + +typedef struct AspeedSCUState { + /*< private >*/ + SysBusDevice parent_obj; + + /*< public >*/ + MemoryRegion iomem; + + uint32_t regs[ASPEED_SCU_NR_REGS]; + uint32_t *reset; + uint32_t num_resets; +} AspeedSCUState; + +typedef struct AspeedSCUResetCfg { + uint32_t offset; + uint32_t val; +} AspeedSCUResetCfg; + +void aspeed_scu_configure_reset(AspeedSCUState *scu, + const struct AspeedSCUResetCfg vals[], int n, Error **errp); + +#endif /* ASPEED_SCU_H */ diff --git a/trace-events b/trace-events index 2f14205de047..24749c414ee7 100644 --- a/trace-events +++ b/trace-events @@ -2164,3 +2164,6 @@ e1000e_cfg_support_virtio(bool support) "Virtio header supported: %d" e1000e_vm_state_running(void) "VM state is running" e1000e_vm_state_stopped(void) "VM state is stopped" + +# hw/misc/aspeed_scu.c +aspeed_scu_write(uint64_t offset, unsigned size, uint32_t data) "To 0x%" PRIx64 " of size %u: 0x%" PRIx32