> -----Original Message----- > From: Cédric Le Goater <[email protected]> > Sent: Wednesday, January 7, 2026 11:35 PM > To: Kane Chen <[email protected]>; Peter Maydell > <[email protected]>; Steven Lee <[email protected]>; Troy > Lee <[email protected]>; Jamin Lin <[email protected]>; Andrew > Jeffery <[email protected]>; Joel Stanley <[email protected]>; > open list:ASPEED BMCs <[email protected]>; open list:All patches CC > here <[email protected]> > Cc: Troy Lee <[email protected]>; [email protected] > Subject: Re: [PATCH v4 14/19] hw/arm/aspeed: attach I2C device to AST1700 > model > > On 1/7/26 15:53, Cédric Le Goater wrote: > > Hello Kane, > > > > I agree we need a way to distinguish the I2C buses belonging to the > > Aspeed Soc from those of the Aspeed IO Expander, so devices can be > > attached on the correct bus. > > > > Let's proceed with your proposal. Below are some refinements. > > > > [ ... ] > > > >> --- a/hw/i2c/aspeed_i2c.c > >> +++ b/hw/i2c/aspeed_i2c.c > >> @@ -1261,6 +1261,7 @@ static void aspeed_i2c_realize(DeviceState > >> *dev, Error **errp) > >> static const Property aspeed_i2c_properties[] = { > >> DEFINE_PROP_LINK("dram", AspeedI2CState, dram_mr, > >> TYPE_MEMORY_REGION, > MemoryRegion *), > >> + DEFINE_PROP_STRING("bus-label", AspeedI2CState, bus_label), > >> }; > >> static void aspeed_i2c_class_init(ObjectClass *klass, const void > >> *data) @@ -1421,14 +1422,28 @@ static void > >> aspeed_i2c_bus_realize(DeviceState *dev, Error **errp) > >> { > >> AspeedI2CBus *s = ASPEED_I2C_BUS(dev); > >> AspeedI2CClass *aic; > >> - g_autofree char *name = g_strdup_printf(TYPE_ASPEED_I2C_BUS > >> ".%d", s->id); > > > > > > I would prefer if you added a new property to AspeedI2CBus : > > > > DEFINE_PROP_STRING("name", AspeedI2CBus, name), > > > > which would be set in aspeed_i2c_realize() : > > > > /* default value */ > > if (!s->bus_label) { > > s->bus_label = g_strdup(TYPE_ASPEED_I2C_BUS); > > } > > > > g_autofree char *name = g_strdup_printf("%s.%d", s->bus_label, > > i); > > > > if (!object_property_set_str(bus, "bus-name", name, errp)) { > > return; > > } > > The above changes may be included in a separate prerequisite patch. > > Thanks, > > C. > > > > The naming should be a higher level decision, made at the SoC level. > > So, aspeed_ast1700_realize() should be adjusted when the "bus-label" > > property is set. > > > > I don't think we need to keep the "aspeed.%s.i2c.bus" prefix. How > > about removing "aspeed" (which is redudant anyhow on an Aspeed > machine) ? > > > > "ioexp%d.i2c.bus" > > > > Also, in aspeed_i2c_bus_realize(), please make sure AspeedI2CBus::name > > is set, like s->controller. This could be an assert too. > > > > Thanks, > > > > C. > > > > > >> - g_autofree char *pool_name = g_strdup_printf("%s.pool", name); > >> + g_autofree char *name = NULL; > >> + g_autofree char *pool_name = NULL; > >> if (!s->controller) { > >> error_setg(errp, TYPE_ASPEED_I2C_BUS ": 'controller' link > >> not set"); > >> return; > >> } > >> + /* > >> + * I2C bus naming: > >> + * - Empty bus_label -> BMC internal controller, use default > name. > >> + * - Non-empty bus_label -> external/addon controller, prefix > >> +with label > >> + * to avoid conflicts and show bus origin. > >> + */ > >> + if (!s->controller->bus_label || > >> +(strlen(s->controller->bus_label) == 0)) { > >> + name = g_strdup_printf(TYPE_ASPEED_I2C_BUS ".%d", s->id); > >> + } else { > >> + name = g_strdup_printf("aspeed.%s.i2c.bus.%d", > >> + s->controller->bus_label, > s->id); > >> + } > >> + pool_name = g_strdup_printf("%s.pool", name); > >> + > >> aic = ASPEED_I2C_GET_CLASS(s->controller); > >> sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq); > >
Hi Cédric, Thanks for your suggestions. I have refactored the bus naming logic to align with your comments. The decision making for the bus name has been moved up to the SoC level, and the redundant "aspeed" prefix has been removed. Here is a summary of the changes: 1. Added a bus-label property to AspeedAST1700SoCState. This allows the top-level SoC (e.g., AST2700) to define the label during its initialization or realize phase. 2. The bus-label is passed from aspeed_ast1700_realize to the I2C controller (AspeedI2CState). 3. In aspeed_i2c_realize, the controller generates unique names using the bus-label. These names are passed to the AspeedI2CBus through a new bus-name property during the initialization of the buses. With these changes, the new object hierarchies and bus names are as follows: BMC: /i2c/bus[0]/aspeed.i2c.bus.0 IOEXP0 (LTPI0): /ioexp[0]/ioexp-i2c[0]/bus[0]/ioexp0.0 IOEXP1 (LTPI1): /ioexp[1]/ioexp-i2c[0]/bus[0]/ioexp1.0 I have also verified that this naming convention does not require changes to existing test scripts, and all functional tests passed successfully. If you have no further concerns regarding this approach, I will submit the updated patch series. Best Regards, Kane
