Re: [PATCH 13/21] aspeed: Add I2C buses to AST1030 model

From: "Cédric Le Goater" <clg@kaod.org>
To: Joel Stanley <joel@jms.id.au>
Cc: qemu-arm <qemu-arm@nongnu.org>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"Joe Komlodi" <komlodi@google.com>,
	"Troy Lee" <troy_lee@aspeedtech.com>,
	"Jamin Lin" <jamin_lin@aspeedtech.com>,
	"Steven Lee" <steven_lee@aspeedtech.com>,
	"Klaus Jensen" <k.jensen@samsung.com>,
	"Peter Delevoryas" <pdel@fb.com>,
	"Corey Minyard" <cminyard@mvista.com>,
	"Jonathan Cameron" <Jonathan.Cameron@huawei.com>,
	"Damien Hedde" <damien.hedde@greensocs.com>,
	"Andrew Jeffery" <andrew@aj.id.au>,
	"Cleber Rosa" <crosa@redhat.com>,
	"Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	"Wainer dos Santos Moschetta" <wainersm@redhat.com>,
	"Beraldo Leal" <bleal@redhat.com>
Subject: Re: [PATCH 13/21] aspeed: Add I2C buses to AST1030 model
Date: Tue, 7 Jun 2022 12:43:53 +0200	[thread overview]
Message-ID: <e20b78c1-02c0-0e27-e02d-98cb3038757a@kaod.org> (raw)
In-Reply-To: <CACPK8XeqgDE+oNSJS8PHQW8hxx25atYW6ww1_0d-nsJuc1vQBg@mail.gmail.com>

On 6/7/22 01:18, Joel Stanley wrote:
> On Mon, 6 Jun 2022 at 15:09, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> From: Troy Lee <troy_lee@aspeedtech.com>
>>
>> Instantiate the I2C buses in AST1030 model and create two slave device
>> for ast1030-evb.
>>
>> Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
>> Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
>> [ clg : - adapted to current ast1030 upstream models
>>          - fixed typo in commit log ]
>> Message-Id: <20220324100439.478317-3-troy_lee@aspeedtech.com>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> 
> one question about a comment below.
> 
>> ---
>>   hw/arm/aspeed.c         | 13 +++++++++++++
>>   hw/arm/aspeed_ast10x0.c | 18 ++++++++++++++++++
>>   2 files changed, 31 insertions(+)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 98dc185acd9a..5c3802308e80 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -1401,6 +1401,18 @@ static void aspeed_minibmc_machine_init(MachineState *machine)
>>                          AST1030_INTERNAL_FLASH_SIZE);
>>   }
>>
>> +static void ast1030_evb_i2c_init(AspeedMachineState *bmc)
>> +{
>> +    AspeedSoCState *soc = &bmc->soc;
>> +
>> +    /* U10 24C08 connects to SDA/SCL Groupt 1 by default */
>> +    uint8_t *eeprom_buf = g_malloc0(32 * 1024);
>> +    smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 0), 0x50, eeprom_buf);
>> +
>> +    /* U11 LM75 connects to SDA/SCL Group 2 by default */
>> +    i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 1), "tmp105", 0x4d);
>> +}
>> +
>>   static void aspeed_minibmc_machine_ast1030_evb_class_init(ObjectClass *oc,
>>                                                             void *data)
>>   {
>> @@ -1412,6 +1424,7 @@ static void aspeed_minibmc_machine_ast1030_evb_class_init(ObjectClass *oc,
>>       amc->hw_strap1 = 0;
>>       amc->hw_strap2 = 0;
>>       mc->init = aspeed_minibmc_machine_init;
>> +    amc->i2c_init = ast1030_evb_i2c_init;
>>       mc->default_ram_size = 0;
>>       mc->default_cpus = mc->min_cpus = mc->max_cpus = 1;
>>       amc->fmc_model = "sst25vf032b";
>> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
>> index d53454168403..a2ed275712fb 100644
>> --- a/hw/arm/aspeed_ast10x0.c
>> +++ b/hw/arm/aspeed_ast10x0.c
>> @@ -114,6 +114,9 @@ static void aspeed_soc_ast1030_init(Object *obj)
>>       object_property_add_alias(obj, "hw-strap1", OBJECT(&s->scu), "hw-strap1");
>>       object_property_add_alias(obj, "hw-strap2", OBJECT(&s->scu), "hw-strap2");
>>
>> +    snprintf(typename, sizeof(typename), "aspeed.i2c-%s", socname);
>> +    object_initialize_child(obj, "i2c", &s->i2c, typename);
>> +
>>       snprintf(typename, sizeof(typename), "aspeed.timer-%s", socname);
>>       object_initialize_child(obj, "timerctrl", &s->timerctrl, typename);
>>
>> @@ -188,6 +191,21 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
>>       }
>>       sysbus_mmio_map(SYS_BUS_DEVICE(&s->scu), 0, sc->memmap[ASPEED_DEV_SCU]);
>>
>> +    /* I2C */
>> +
>> +    object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(&s->sram),
>> +                             &error_abort);
>> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->i2c), errp)) {
>> +        return;
>> +    }
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->i2c), 0, sc->memmap[ASPEED_DEV_I2C]);
>> +    for (i = 0; i < ASPEED_I2C_GET_CLASS(&s->i2c)->num_busses; i++) {
>> +        qemu_irq irq = qdev_get_gpio_in(DEVICE(&s->armv7m),
>> +                                        sc->irqmap[ASPEED_DEV_I2C] + i);
>> +        /* The AST2600 I2C controller has one IRQ per bus. */
> 
> I know it's the same hardware, but is the "AST2600" part of the comment correct?

I will take care of it.

Thanks,

C.

> 
>> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c.busses[i]), 0, irq);
>> +    }
>> +
>>       /* LPC */
>>       if (!sysbus_realize(SYS_BUS_DEVICE(&s->lpc), errp)) {
>>           return;
>> --
>> 2.35.3
>>