Hi Conor, On Wed, Sep 30, 2026 at 4:37 PM Conor Dooley <[email protected]> wrote: > > On Fri, Sep 04, 2026 at 11:57:31PM +0800, Bin Meng wrote: > > U-Boot requests the PolarFire SoC device serial number during board > > late initialization. The IOSCB previously rejected every request, so > > This is kinda crazy actually, we probably should change u-boot to not do > this since the reason for acquiring the serial number is populating the > mac address. > I guess it just never actually ever is problematic in hardware, but it > does seem a bit mad to reject boot over.
Will prepare a U-Boot patch to correct this upstream. > > > board setup could not complete. > > > > Model the System Services control registers and byte-addressable > > mailbox. Obtain the 128-bit serial number from an optional device > > property, using a deterministic value when it is not configured. > > Return an explicit failure for unsupported services and reset the new > > runtime state with the rest of the device. > > > > Signed-off-by: Bin Meng <[email protected]> > > --- > > > > (no changes since v1) > > > > include/hw/misc/mchp_pfsoc_ioscb.h | 6 ++ > > hw/misc/mchp_pfsoc_ioscb.c | 141 +++++++++++++++++++++++++---- > > 2 files changed, 130 insertions(+), 17 deletions(-) > > > > diff --git a/include/hw/misc/mchp_pfsoc_ioscb.h > > b/include/hw/misc/mchp_pfsoc_ioscb.h > > index 9687ea25b1..fd31427304 100644 > > --- a/include/hw/misc/mchp_pfsoc_ioscb.h > > +++ b/include/hw/misc/mchp_pfsoc_ioscb.h > > @@ -25,6 +25,8 @@ > > > > #include "hw/core/sysbus.h" > > > > +#define MCHP_PFSOC_IOSCB_MAILBOX_SIZE 0x1000 > > + > > typedef struct MchpPfSoCIoscbState { > > SysBusDevice parent; > > MemoryRegion container; > > @@ -47,6 +49,10 @@ typedef struct MchpPfSoCIoscbState { > > MemoryRegion cfm_sgmii; > > MemoryRegion bc_sgmii; > > MemoryRegion io_calib_sgmii; > > + uint32_t services_cr; > > + uint32_t services_sr; > > + uint8_t mailbox_data[MCHP_PFSOC_IOSCB_MAILBOX_SIZE]; > > + char *serial_number; > > qemu_irq irq; > > } MchpPfSoCIoscbState; > > > > diff --git a/hw/misc/mchp_pfsoc_ioscb.c b/hw/misc/mchp_pfsoc_ioscb.c > > index 09b702e520..7c82b55986 100644 > > --- a/hw/misc/mchp_pfsoc_ioscb.c > > +++ b/hw/misc/mchp_pfsoc_ioscb.c > > @@ -25,6 +25,7 @@ > > #include "qemu/log.h" > > #include "qapi/error.h" > > #include "hw/core/irq.h" > > +#include "hw/core/qdev-properties.h" > > #include "hw/core/sysbus.h" > > #include "hw/misc/mchp_pfsoc_ioscb.h" > > > > @@ -37,6 +38,9 @@ > > #define IOSCB_CCC_REG_SIZE 0x2000000 > > #define IOSCB_CTRL_REG_SIZE 0x800 > > #define IOSCB_QSPIXIP_REG_SIZE 0x200 > > +#define IOSCB_SERIAL_NUMBER_SIZE 16U > > +#define IOSCB_DEFAULT_SERIAL_NUMBER "0123456789abcdef" > > +#define IOSCB_PROP_SERIAL_NUMBER "serial-number" > > > > > > /* > > @@ -186,33 +190,71 @@ static const MemoryRegionOps > > mchp_pfsoc_io_calib_ddr_ops = { > > .endianness = DEVICE_LITTLE_ENDIAN, > > }; > > > > -#define SERVICES_CR 0x50 > > -#define SERVICES_SR 0x54 > > -#define SERVICES_STATUS_SHIFT 16 > > +#define SERVICES_CR 0x50 > > +#define SERVICES_CR_REQUEST BIT(0) > > +#define SERVICES_CR_COMMAND_SHIFT 16 > > +#define SERVICES_CR_COMMAND_WIDTH 8 > > This I think should actually be 7. Will fix in v3. > > > +#define SERVICES_CR_COMMAND_MASK \ > > + MAKE_64BIT_MASK(SERVICES_CR_COMMAND_SHIFT, > > SERVICES_CR_COMMAND_WIDTH) > > +#define SERVICES_CR_MASK \ > > + (SERVICES_CR_REQUEST | SERVICES_CR_COMMAND_MASK) > > +#define SERVICES_SR 0x54 > > +#define SERVICES_SR_STATUS_SHIFT 16 > > +#define SERVICES_COMMAND_SERIAL_NUMBER 0 > > +#define SERVICES_STATUS_SUCCESS 0 > > +#define SERVICES_STATUS_FAILED 1 > > +#define SERVICES_MAILBOX_RESPONSE_OFFSET 0 > > + > > +static void services_cr_write(MchpPfSoCIoscbState *s, uint32_t value) > > +{ > > + uint32_t command; > > + uint32_t status = SERVICES_STATUS_FAILED; > > + > > + if (device_is_in_reset(DEVICE(s)) || > > + !(value & SERVICES_CR_REQUEST)) { > > + return; > > + } > > + > > + /* > > + * System services complete synchronously in this model, so clear the > > + * request bit before exposing the response to the guest. > > + */ > > + s->services_cr &= ~SERVICES_CR_REQUEST; > > + > > + command = (value & SERVICES_CR_COMMAND_MASK) >> > > + SERVICES_CR_COMMAND_SHIFT; > > + if (command == SERVICES_COMMAND_SERIAL_NUMBER) { > > + /* > > + * The serial-number service returns a 128-bit response starting at > > + * the beginning of the mailbox. > > That's not actually strictly true, bits 15:7 of the command actually > determine the location in the mailbox. I'm not actually aware of any users > of this feature, Linux and U-Boot both leave this at 0, so it doesn't really > matter for functionality, but the comment is wrong. > See section 3 of the document in the comment you're removing below for > how this works. Will fix the comment in v3. > > > + */ > > + memset(&s->mailbox_data[SERVICES_MAILBOX_RESPONSE_OFFSET], 0, > > + IOSCB_SERIAL_NUMBER_SIZE); > > + memcpy(&s->mailbox_data[SERVICES_MAILBOX_RESPONSE_OFFSET], > > + s->serial_number, strlen(s->serial_number)); > > + status = SERVICES_STATUS_SUCCESS; > > + } > > + > > + s->services_sr = status << SERVICES_SR_STATUS_SHIFT; > > + qemu_irq_raise(s->irq); > > +} > > > > static uint64_t mchp_pfsoc_ctrl_read(void *opaque, hwaddr offset, > > unsigned size) > > { > > - uint32_t val = 0; > > + MchpPfSoCIoscbState *s = opaque; > > > > switch (offset) { > > + case SERVICES_CR: > > + return s->services_cr; > > case SERVICES_SR: > > - /* > > - * Although some services have no error codes, most do. All > > services > > - * that do implement errors, begin their error codes at 1. Treat > > all > > - * service requests as failures & return 1. > > - * See the "PolarFire® FPGA and PolarFire SoC FPGA System Services" > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > - * user guide for more information on service error codes. > > - */ > > - val = 1u << SERVICES_STATUS_SHIFT; > > - break; > > + return s->services_sr; > > default: > > qemu_log_mask(LOG_UNIMP, "%s: unimplemented device read " > > "(size %d, offset 0x%" HWADDR_PRIx ")\n", > > __func__, size, offset); > > + return 0; > > } > > - > > - return val; > > } > > > > static void mchp_pfsoc_ctrl_write(void *opaque, hwaddr offset, > > @@ -222,13 +264,17 @@ static void mchp_pfsoc_ctrl_write(void *opaque, > > hwaddr offset, > > > > switch (offset) { > > case SERVICES_CR: > > - qemu_irq_raise(s->irq); > > + s->services_cr = value & SERVICES_CR_MASK; > > + services_cr_write(s, value); > > + break; > > + case SERVICES_SR: > > break; > > default: > > qemu_log_mask(LOG_UNIMP, "%s: unimplemented device write " > > "(size %d, value 0x%" PRIx64 > > ", offset 0x%" HWADDR_PRIx ")\n", > > __func__, size, value, offset); > > + break; > > } > > } > > > > @@ -236,6 +282,55 @@ static const MemoryRegionOps mchp_pfsoc_ctrl_ops = { > > .read = mchp_pfsoc_ctrl_read, > > .write = mchp_pfsoc_ctrl_write, > > .endianness = DEVICE_LITTLE_ENDIAN, > > + .valid = { > > + .min_access_size = sizeof(uint32_t), > > I'm not au fait enough with qemu to know, but does this mean that > accessing these registers with an 8 or 16 bit accessor will fail > somehow? > If so, that's not how the hardware behaves, and below... This was added during debugging. Checked the doc and found no clear clue about the access limitation. Testing showed that dropping these does not prevent the firmware from booting. Will drop this in v3. > > > + .max_access_size = sizeof(uint32_t), > > + }, > > +}; > > + > > +/* > > + * The System Controller uses the mailbox as a byte-addressable shared > > buffer > > + * for service command payloads and responses. > > + */ > > +static uint64_t mchp_pfsoc_mailbox_read(void *opaque, hwaddr offset, > > + unsigned size) > > +{ > > + MchpPfSoCIoscbState *s = opaque; > > + > > + return ldn_le_p(&s->mailbox_data[offset], size); > > +} > > + > > +static void mchp_pfsoc_mailbox_write(void *opaque, hwaddr offset, > > + uint64_t value, unsigned size) > > +{ > > + MchpPfSoCIoscbState *s = opaque; > > + > > + stn_le_p(&s->mailbox_data[offset], size, value); > > +} > > + > > +static const MemoryRegionOps mchp_pfsoc_mailbox_ops = { > > + .read = mchp_pfsoc_mailbox_read, > > + .write = mchp_pfsoc_mailbox_write, > > + .endianness = DEVICE_LITTLE_ENDIAN, > > + .valid = { > > + .min_access_size = sizeof(uint8_t), > > ...you have the more expected 8. > > > + .max_access_size = sizeof(uint32_t), > > + }, > > +}; > > + > > +static void mchp_pfsoc_ioscb_reset(DeviceState *dev) > > +{ > > + MchpPfSoCIoscbState *s = MCHP_PFSOC_IOSCB(dev); > > + > > + s->services_cr = 0; > > + s->services_sr = 0; > > + memset(s->mailbox_data, 0, sizeof(s->mailbox_data)); > > + qemu_irq_lower(s->irq); > > +} > > + > > +static const Property mchp_pfsoc_ioscb_properties[] = { > > + DEFINE_PROP_STRING(IOSCB_PROP_SERIAL_NUMBER, > > + MchpPfSoCIoscbState, serial_number), > > }; > > > > static void mchp_pfsoc_ioscb_realize(DeviceState *dev, Error **errp) > > @@ -243,6 +338,16 @@ static void mchp_pfsoc_ioscb_realize(DeviceState *dev, > > Error **errp) > > MchpPfSoCIoscbState *s = MCHP_PFSOC_IOSCB(dev); > > SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > > > > + /* Use a deterministic identity when no serial number is configured */ > > + if (!s->serial_number) { > > + s->serial_number = g_strdup(IOSCB_DEFAULT_SERIAL_NUMBER); > > + } > > + if (strlen(s->serial_number) > IOSCB_SERIAL_NUMBER_SIZE) { > > + error_setg(errp, "The serial number can't be longer than %u bytes", > > + IOSCB_SERIAL_NUMBER_SIZE); > > + return; > > + } > > + > > memory_region_init(&s->container, OBJECT(s), > > "mchp.pfsoc.ioscb", IOSCB_WHOLE_REG_SIZE); > > sysbus_init_mmio(sbd, &s->container); > > @@ -265,7 +370,7 @@ static void mchp_pfsoc_ioscb_realize(DeviceState *dev, > > Error **errp) > > "mchp.pfsoc.ioscb.qspixip", > > IOSCB_QSPIXIP_REG_SIZE); > > memory_region_add_subregion(&s->container, IOSCB_QSPIXIP_BASE, > > &s->qspixip); > > > > - memory_region_init_io(&s->mailbox, OBJECT(s), &mchp_pfsoc_dummy_ops, s, > > + memory_region_init_io(&s->mailbox, OBJECT(s), &mchp_pfsoc_mailbox_ops, > > s, > > "mchp.pfsoc.ioscb.mailbox", > > IOSCB_SUBMOD_REG_SIZE); > > memory_region_add_subregion(&s->container, IOSCB_MAILBOX_BASE, > > &s->mailbox); > > > > @@ -343,6 +448,8 @@ static void mchp_pfsoc_ioscb_class_init(ObjectClass > > *klass, const void *data) > > > > dc->desc = "Microchip PolarFire SoC IOSCB modules"; > > dc->realize = mchp_pfsoc_ioscb_realize; > > + device_class_set_legacy_reset(dc, mchp_pfsoc_ioscb_reset); > > + device_class_set_props(dc, mchp_pfsoc_ioscb_properties); > > } > > > > static const TypeInfo mchp_pfsoc_ioscb_info = { > > -- Regards, Bin
