On 16/08/2019 09:40, Rashmica Gupta wrote: > Cédric, this is how I thought changes to the SOC for your aspeed-4.1 > branch would look
Some comments, > From 13a07834476fa266c352d9a075b341c483b2edf9 Mon Sep 17 00:00:00 2001 > From: Rashmica Gupta <rashmic...@gmail.com> > Date: Fri, 16 Aug 2019 15:18:22 +1000 > Subject: [PATCH] Aspeed SOC changes > > --- > include/hw/arm/aspeed_soc.h | 4 +++- > hw/arm/aspeed_soc.c | 32 ++++++++++++++++++++++---------- > 2 files changed, 25 insertions(+), 11 deletions(-) > > diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h > index 8673661de8..f375271d5a 100644 > --- a/include/hw/arm/aspeed_soc.h > +++ b/include/hw/arm/aspeed_soc.h > @@ -28,6 +28,7 @@ > #define ASPEED_WDTS_NUM 3 > #define ASPEED_CPUS_NUM 2 > #define ASPEED_MACS_NUM 2 > +#define ASPEED_GPIOS_NUM 2 > > > typedef struct AspeedSoCState { > /*< private >*/ > @@ -48,7 +49,7 @@ typedef struct AspeedSoCState { > AspeedSDMCState sdmc; > AspeedWDTState wdt[ASPEED_WDTS_NUM]; > FTGMAC100State ftgmac100[ASPEED_MACS_NUM]; > - AspeedGPIOState gpio; > + AspeedGPIOState gpio[ASPEED_GPIOS_NUM]; Even if they look the same, I think these are two different controllers and not multiple instances of the same. So I would rather introduce a new field 'gpio_1_8v' for the AST2600. > } AspeedSoCState; > > #define TYPE_ASPEED_SOC "aspeed-soc" > @@ -61,6 +62,7 @@ typedef struct AspeedSoCInfo { > uint64_t sram_size; > int spis_num; > int wdts_num; > + int gpios_num; > const int *irqmap; > const hwaddr *memmap; > uint32_t num_cpus; > diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c > index 7d47647016..414b99c4f3 100644 > --- a/hw/arm/aspeed_soc.c > +++ b/hw/arm/aspeed_soc.c > @@ -119,6 +119,7 @@ static const AspeedSoCInfo aspeed_socs[] = { > .sram_size = 0x8000, > .spis_num = 1, > .wdts_num = 2, > + .gpios_num = 1, > .irqmap = aspeed_soc_ast2400_irqmap, > .memmap = aspeed_soc_ast2400_memmap, > .num_cpus = 1, > @@ -132,6 +133,7 @@ static const AspeedSoCInfo aspeed_socs[] = { > .irqmap = aspeed_soc_ast2500_irqmap, > .memmap = aspeed_soc_ast2500_memmap, > .num_cpus = 1, > + .gpios_num = 1, > }, > }; > > @@ -226,9 +228,15 @@ static void aspeed_soc_init(Object *obj) > sysbus_init_child_obj(obj, "xdma", OBJECT(&s->xdma), sizeof(s- >> xdma), > TYPE_ASPEED_XDMA); > > - snprintf(typename, sizeof(typename), "aspeed.gpio-%s", socname); > - sysbus_init_child_obj(obj, "gpio", OBJECT(&s->gpio), sizeof(s- >> gpio), > - typename); > + for (i = 0; i < sc->info->gpios_num; i++) { > + if (ASPEED_IS_AST2600(sc->info->silicon_rev)) { > + snprintf(typename, sizeof(typename), "aspeed.gpio%d-%s", > i, socname); > + } else { > + snprintf(typename, sizeof(typename), "aspeed.gpio-%s", > socname); > + } > + sysbus_init_child_obj(obj, "gpio[*]", OBJECT(&s->gpio[i]), > sizeof(s->gpio[i]), > + typename); > + } > } > > static void aspeed_soc_realize(DeviceState *dev, Error **errp) > @@ -410,15 +418,19 @@ static void aspeed_soc_realize(DeviceState *dev, > Error **errp) > aspeed_soc_get_irq(s, ASPEED_XDMA)); > > /* GPIO */ > - object_property_set_bool(OBJECT(&s->gpio), true, "realized", > &err); > - if (err) { > - error_propagate(errp, err); > - return; > + for (i = 0; i < sc->info->gpios_num; i++) { > + hwaddr addr = sc->info->memmap[ASPEED_GPIO] + i * 0x800; I would introduce ASPEED_GPIO_V1_8V instead. > + object_property_set_bool(OBJECT(&s->gpio[i]), true, > "realized", &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->gpio[i]), 0, addr); > + sysbus_connect_irq(SYS_BUS_DEVICE(&s->gpio[i]), 0, > + aspeed_soc_get_irq(s, ASPEED_GPIO)); The interrupt is different. C. > } > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->gpio), 0, sc->info- >> memmap[ASPEED_GPIO]); > - sysbus_connect_irq(SYS_BUS_DEVICE(&s->gpio), 0, > - aspeed_soc_get_irq(s, ASPEED_GPIO)); > } > + > static Property aspeed_soc_properties[] = { > DEFINE_PROP_UINT32("num-cpus", AspeedSoCState, num_cpus, 0), > DEFINE_PROP_END_OF_LIST(), >