Re: [PATCH v4 14/19] hw/arm/aspeed: attach I2C device to AST1700 model

[Cédric Le Goater]

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);