Re: [PATCH v1 03/15] hw/i2c/aspeed: support to set the different memory size

[Cédric Le Goater]

On 7/18/24 11:42, Jamin Lin wrote:
> Hi Cedric,
>
> > Subject: Re: [PATCH v1 03/15] hw/i2c/aspeed: support to set the different
> > memory size
> >
> > On 7/18/24 08:49, Jamin Lin wrote:
> > > According to the datasheet of ASPEED SOCs, an I2C controller owns 8KB
> > > of register space for AST2700, owns 4KB of register space for AST2600,
> > > AST2500 and AST2400, and owns 64KB of register space for AST1030.
> > >
> > > It set the memory region size 4KB by default and it does not
> > > compatible register space for AST2700.
> > >
> > > Introduce a new class attribute to set the I2C controller memory size
> > > for different ASPEED SOCs.
> > >
> > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > > ---
> > >    hw/i2c/aspeed_i2c.c         | 6 +++++-
> > >    include/hw/i2c/aspeed_i2c.h | 2 +-
> > >    2 files changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index
> > > b43afd250d..7d5a53c4c0 100644
> > > --- a/hw/i2c/aspeed_i2c.c
> > > +++ b/hw/i2c/aspeed_i2c.c
> > > @@ -1014,7 +1014,7 @@ static void aspeed_i2c_realize(DeviceState *dev,
> > > Error **errp)
> > >
> > >        sysbus_init_irq(sbd, &s->irq);
> > >        memory_region_init_io(&s->iomem, OBJECT(s),
> > &aspeed_i2c_ctrl_ops, s,
> > > -                          "aspeed.i2c", 0x1000);
> > > +                          "aspeed.i2c", aic->mem_size);
> > >        sysbus_init_mmio(sbd, &s->iomem);
> > >
> > >        for (i = 0; i < aic->num_busses; i++) { @@ -1286,6 +1286,7 @@
> > > static void aspeed_2400_i2c_class_init(ObjectClass *klass, void *data)
> > >        aic->pool_size = 0x800;
> > >        aic->pool_base = 0x800;
> > >        aic->bus_pool_base = aspeed_2400_i2c_bus_pool_base;
> > > +    aic->mem_size = 0x1000;
> > >    }
> > >
> > >    static const TypeInfo aspeed_2400_i2c_info = { @@ -1320,6 +1321,7 @@
> > > static void aspeed_2500_i2c_class_init(ObjectClass *klass, void *data)
> > >        aic->bus_pool_base = aspeed_2500_i2c_bus_pool_base;
> > >        aic->check_sram = true;
> > >        aic->has_dma = true;
> > > +    aic->mem_size = 0x1000;
> > >    }
> > >
> > >    static const TypeInfo aspeed_2500_i2c_info = { @@ -1353,6 +1355,7 @@
> > > static void aspeed_2600_i2c_class_init(ObjectClass *klass, void *data)
> > >        aic->pool_base = 0xC00;
> > >        aic->bus_pool_base = aspeed_2600_i2c_bus_pool_base;
> > >        aic->has_dma = true;
> > > +    aic->mem_size = 0x1000;
> > >    }
> > >
> > >    static const TypeInfo aspeed_2600_i2c_info = { @@ -1376,6 +1379,7 @@
> > > static void aspeed_1030_i2c_class_init(ObjectClass *klass, void *data)
> > >        aic->pool_base = 0xC00;
> > >        aic->bus_pool_base = aspeed_2600_i2c_bus_pool_base;
> > >        aic->has_dma = true;
> > > +    aic->mem_size = 0x10000;
> Thanks for review.
>
> According to the datasheet of AST1030 in chapter 7 (Memory Space Allocation 
> Table)
> , the register address space of I2C Controller range is start from 7E7B0000 to
> 7E7BFFFF and its register address space is 64KB(0x10000).

OK.
 
> The firmware only use 4KB address space. We can change mem_size either 4KB or 
> 64KB.
> Could you tell me which size you prefer?

I would keep the larger value for the model and let FW decide to resize or not.

Thanks,

C.