[PATCH v1 00/15] support ADC and I2C for AST2700

[PATCH v1 00/15] support ADC and I2C for AST2700

[Jamin Lin via]

v1: 
1. support ADC for AST2700
2. support I2C for AST2700

Jamin Lin (15):
  aspeed/adc: Add AST2700 support
  aspeed/soc: support ADC for AST2700
  hw/i2c/aspeed: support to set the different memory size
  hw/i2c/aspeed: support discontinuous register memory region of I2C bus
  hw/i2c/aspeed: rename the I2C class pool attribute to share_pool
  hw/i2c/aspeed: introduce a new bus pool buffer attribute in
    AspeedI2Cbus
  hw/i2c/aspeed: support discontinuous poll buffer memory region of I2C
    bus
  hw/i2c/aspeed: introduce a new dma_dram_offset attribute in
    AspeedI2Cbus
  hw/i2c/aspeed: Add AST2700 support
  hw/i2c/aspeed: support Tx/Rx buffer 64 bits address
  hw/i2c/aspeed: support high part dram offset for DMA 64 bits
  aspeed/soc: introduce a new API to get the INTC orgate information
  aspeed/soc: support I2C for AST2700
  aspeed: fix coding style
  aspeed: add tmp105 in i2c bus 0 for AST2700

 hw/adc/aspeed_adc.c         |  16 ++
 hw/arm/aspeed.c             |  31 +++-
 hw/arm/aspeed_ast27x0.c     |  65 +++++++
 hw/i2c/aspeed_i2c.c         | 340 ++++++++++++++++++++++++++++++------
 include/hw/adc/aspeed_adc.h |   1 +
 include/hw/i2c/aspeed_i2c.h |  34 ++--
 6 files changed, 418 insertions(+), 69 deletions(-)

-- 
2.34.1

[PATCH v1 01/15] aspeed/adc: Add AST2700 support

[Jamin Lin via]

AST2700 and AST2600 ADC controllers are identical.
Introduce ast2700 class and set 2 engines.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/adc/aspeed_adc.c         | 16 ++++++++++++++++
 include/hw/adc/aspeed_adc.h |  1 +
 2 files changed, 17 insertions(+)

diff --git a/hw/adc/aspeed_adc.c b/hw/adc/aspeed_adc.c
index 68bdbc73b0..48328ef891 100644
--- a/hw/adc/aspeed_adc.c
+++ b/hw/adc/aspeed_adc.c
@@ -398,6 +398,15 @@ static void aspeed_1030_adc_class_init(ObjectClass *klass, void *data)
     aac->nr_engines = 2;
 }
 
+static void aspeed_2700_adc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedADCClass *aac = ASPEED_ADC_CLASS(klass);
+
+    dc->desc = "ASPEED 2700 ADC Controller";
+    aac->nr_engines = 2;
+}
+
 static const TypeInfo aspeed_adc_info = {
     .name = TYPE_ASPEED_ADC,
     .parent = TYPE_SYS_BUS_DEVICE,
@@ -430,6 +439,12 @@ static const TypeInfo aspeed_1030_adc_info = {
     .class_init = aspeed_1030_adc_class_init, /* No change since AST2600 */
 };
 
+static const TypeInfo aspeed_2700_adc_info = {
+    .name = TYPE_ASPEED_2700_ADC,
+    .parent = TYPE_ASPEED_ADC,
+    .class_init = aspeed_2700_adc_class_init,
+};
+
 static void aspeed_adc_register_types(void)
 {
     type_register_static(&aspeed_adc_engine_info);
@@ -438,6 +453,7 @@ static void aspeed_adc_register_types(void)
     type_register_static(&aspeed_2500_adc_info);
     type_register_static(&aspeed_2600_adc_info);
     type_register_static(&aspeed_1030_adc_info);
+    type_register_static(&aspeed_2700_adc_info);
 }
 
 type_init(aspeed_adc_register_types);
diff --git a/include/hw/adc/aspeed_adc.h b/include/hw/adc/aspeed_adc.h
index ff1d06ea91..f502f197ac 100644
--- a/include/hw/adc/aspeed_adc.h
+++ b/include/hw/adc/aspeed_adc.h
@@ -18,6 +18,7 @@
 #define TYPE_ASPEED_2500_ADC TYPE_ASPEED_ADC "-ast2500"
 #define TYPE_ASPEED_2600_ADC TYPE_ASPEED_ADC "-ast2600"
 #define TYPE_ASPEED_1030_ADC TYPE_ASPEED_ADC "-ast1030"
+#define TYPE_ASPEED_2700_ADC TYPE_ASPEED_ADC "-ast2700"
 OBJECT_DECLARE_TYPE(AspeedADCState, AspeedADCClass, ASPEED_ADC)
 
 #define TYPE_ASPEED_ADC_ENGINE "aspeed.adc.engine"
-- 
2.34.1

Re: [PATCH v1 01/15] aspeed/adc: Add AST2700 support

[Cédric Le Goater]

On 7/18/24 08:49, Jamin Lin wrote:
> AST2700 and AST2600 ADC controllers are identical.
> Introduce ast2700 class and set 2 engines.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/adc/aspeed_adc.c         | 16 ++++++++++++++++
>   include/hw/adc/aspeed_adc.h |  1 +
>   2 files changed, 17 insertions(+)
> 
> diff --git a/hw/adc/aspeed_adc.c b/hw/adc/aspeed_adc.c
> index 68bdbc73b0..48328ef891 100644
> --- a/hw/adc/aspeed_adc.c
> +++ b/hw/adc/aspeed_adc.c
> @@ -398,6 +398,15 @@ static void aspeed_1030_adc_class_init(ObjectClass *klass, void *data)
>       aac->nr_engines = 2;
>   }
>   
> +static void aspeed_2700_adc_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    AspeedADCClass *aac = ASPEED_ADC_CLASS(klass);
> +
> +    dc->desc = "ASPEED 2700 ADC Controller";
> +    aac->nr_engines = 2;
> +}
> +
>   static const TypeInfo aspeed_adc_info = {
>       .name = TYPE_ASPEED_ADC,
>       .parent = TYPE_SYS_BUS_DEVICE,
> @@ -430,6 +439,12 @@ static const TypeInfo aspeed_1030_adc_info = {
>       .class_init = aspeed_1030_adc_class_init, /* No change since AST2600 */
>   };
>   
> +static const TypeInfo aspeed_2700_adc_info = {
> +    .name = TYPE_ASPEED_2700_ADC,
> +    .parent = TYPE_ASPEED_ADC,
> +    .class_init = aspeed_2700_adc_class_init,
> +};
> +
>   static void aspeed_adc_register_types(void)
>   {
>       type_register_static(&aspeed_adc_engine_info);
> @@ -438,6 +453,7 @@ static void aspeed_adc_register_types(void)
>       type_register_static(&aspeed_2500_adc_info);
>       type_register_static(&aspeed_2600_adc_info);
>       type_register_static(&aspeed_1030_adc_info);
> +    type_register_static(&aspeed_2700_adc_info);
>   }
>   
>   type_init(aspeed_adc_register_types);
> diff --git a/include/hw/adc/aspeed_adc.h b/include/hw/adc/aspeed_adc.h
> index ff1d06ea91..f502f197ac 100644
> --- a/include/hw/adc/aspeed_adc.h
> +++ b/include/hw/adc/aspeed_adc.h
> @@ -18,6 +18,7 @@
>   #define TYPE_ASPEED_2500_ADC TYPE_ASPEED_ADC "-ast2500"
>   #define TYPE_ASPEED_2600_ADC TYPE_ASPEED_ADC "-ast2600"
>   #define TYPE_ASPEED_1030_ADC TYPE_ASPEED_ADC "-ast1030"
> +#define TYPE_ASPEED_2700_ADC TYPE_ASPEED_ADC "-ast2700"
>   OBJECT_DECLARE_TYPE(AspeedADCState, AspeedADCClass, ASPEED_ADC)
>   
>   #define TYPE_ASPEED_ADC_ENGINE "aspeed.adc.engine"

[PATCH v1 02/15] aspeed/soc: support ADC for AST2700

[Jamin Lin via]

Add ADC model for AST2700 ADC support.
The ADC controller registers base address is start at
0x14C0_0000 and its address space is 0x1000.
The ADC controller interrupt is connected to
GICINT130_INTC group at bit 16. The GIC IRQ is 130.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/arm/aspeed_ast27x0.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index a9fb0d4b88..4257b5e8af 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -60,6 +60,7 @@ static const hwaddr aspeed_soc_ast2700_memmap[] = {
     [ASPEED_DEV_SLIIO]     =  0x14C1E000,
     [ASPEED_GIC_DIST]      =  0x12200000,
     [ASPEED_GIC_REDIST]    =  0x12280000,
+    [ASPEED_DEV_ADC]       =  0x14C00000,
 };
 
 #define AST2700_MAX_IRQ 288
@@ -344,6 +345,9 @@ static void aspeed_soc_ast2700_init(Object *obj)
     object_initialize_child(obj, "sli", &s->sli, TYPE_ASPEED_2700_SLI);
     object_initialize_child(obj, "sliio", &s->sliio, TYPE_ASPEED_2700_SLIIO);
     object_initialize_child(obj, "intc", &a->intc, TYPE_ASPEED_2700_INTC);
+
+    snprintf(typename, sizeof(typename), "aspeed.adc-%s", socname);
+    object_initialize_child(obj, "adc", &s->adc, typename);
 }
 
 /*
@@ -601,6 +605,14 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
     aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->sliio), 0,
                     sc->memmap[ASPEED_DEV_SLIIO]);
 
+    /* ADC */
+    if (!sysbus_realize(SYS_BUS_DEVICE(&s->adc), errp)) {
+        return;
+    }
+    aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->adc), 0, sc->memmap[ASPEED_DEV_ADC]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
+                       aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
+
     create_unimplemented_device("ast2700.dpmcu", 0x11000000, 0x40000);
     create_unimplemented_device("ast2700.iomem0", 0x12000000, 0x01000000);
     create_unimplemented_device("ast2700.iomem1", 0x14000000, 0x01000000);
-- 
2.34.1

Re: [PATCH v1 02/15] aspeed/soc: support ADC for AST2700

[Cédric Le Goater]

On 7/18/24 08:49, Jamin Lin wrote:
> Add ADC model for AST2700 ADC support.
> The ADC controller registers base address is start at
> 0x14C0_0000 and its address space is 0x1000.
> The ADC controller interrupt is connected to
> GICINT130_INTC group at bit 16. The GIC IRQ is 130.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/arm/aspeed_ast27x0.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
> index a9fb0d4b88..4257b5e8af 100644
> --- a/hw/arm/aspeed_ast27x0.c
> +++ b/hw/arm/aspeed_ast27x0.c
> @@ -60,6 +60,7 @@ static const hwaddr aspeed_soc_ast2700_memmap[] = {
>       [ASPEED_DEV_SLIIO]     =  0x14C1E000,
>       [ASPEED_GIC_DIST]      =  0x12200000,
>       [ASPEED_GIC_REDIST]    =  0x12280000,
> +    [ASPEED_DEV_ADC]       =  0x14C00000,
>   };
>   
>   #define AST2700_MAX_IRQ 288
> @@ -344,6 +345,9 @@ static void aspeed_soc_ast2700_init(Object *obj)
>       object_initialize_child(obj, "sli", &s->sli, TYPE_ASPEED_2700_SLI);
>       object_initialize_child(obj, "sliio", &s->sliio, TYPE_ASPEED_2700_SLIIO);
>       object_initialize_child(obj, "intc", &a->intc, TYPE_ASPEED_2700_INTC);
> +
> +    snprintf(typename, sizeof(typename), "aspeed.adc-%s", socname);
> +    object_initialize_child(obj, "adc", &s->adc, typename);
>   }
>   
>   /*
> @@ -601,6 +605,14 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
>       aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->sliio), 0,
>                       sc->memmap[ASPEED_DEV_SLIIO]);
>   
> +    /* ADC */
> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->adc), errp)) {
> +        return;
> +    }
> +    aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->adc), 0, sc->memmap[ASPEED_DEV_ADC]);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
> +                       aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
> +
>       create_unimplemented_device("ast2700.dpmcu", 0x11000000, 0x40000);
>       create_unimplemented_device("ast2700.iomem0", 0x12000000, 0x01000000);
>       create_unimplemented_device("ast2700.iomem1", 0x14000000, 0x01000000);

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

[Jamin Lin via]

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;
 }
 
 static const TypeInfo aspeed_1030_i2c_info = {
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index a064479e59..065b636d29 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -283,7 +283,7 @@ struct AspeedI2CClass {
     uint8_t *(*bus_pool_base)(AspeedI2CBus *);
     bool check_sram;
     bool has_dma;
-
+    uint64_t mem_size;
 };
 
 static inline bool aspeed_i2c_is_new_mode(AspeedI2CState *s)
-- 
2.34.1

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

[Cédric Le Goater]

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;

Are you sure this value is correct ?


Thanks,

C.


>   }
>   
>   static const TypeInfo aspeed_1030_i2c_info = {
> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> index a064479e59..065b636d29 100644
> --- a/include/hw/i2c/aspeed_i2c.h
> +++ b/include/hw/i2c/aspeed_i2c.h
> @@ -283,7 +283,7 @@ struct AspeedI2CClass {
>       uint8_t *(*bus_pool_base)(AspeedI2CBus *);
>       bool check_sram;
>       bool has_dma;
> -
> +    uint64_t mem_size;
>   };
>   
>   static inline bool aspeed_i2c_is_new_mode(AspeedI2CState *s)

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

[Jamin Lin]

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

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

> Are you sure this value is correct ?
>
>
> Thanks,
>
> C.
>
>
> >   }
> >
> >   static const TypeInfo aspeed_1030_i2c_info = { diff --git
> > a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h index
> > a064479e59..065b636d29 100644
> > --- a/include/hw/i2c/aspeed_i2c.h
> > +++ b/include/hw/i2c/aspeed_i2c.h
> > @@ -283,7 +283,7 @@ struct AspeedI2CClass {
> >       uint8_t *(*bus_pool_base)(AspeedI2CBus *);
> >       bool check_sram;
> >       bool has_dma;
> > -
> > +    uint64_t mem_size;
> >   };
> >
> >   static inline bool aspeed_i2c_is_new_mode(AspeedI2CState *s)

************* Email Confidentiality Notice ********************
免責聲明:
本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!

DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.

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.

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

[Jamin Lin]

Hi Cedric,

> Subject: Re: [PATCH v1 03/15] hw/i2c/aspeed: support to set the different
> memory size
>
> 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 for suggestion.
Got it.

> Thanks,
>
> C.
>
>
>
>
>

************* Email Confidentiality Notice ********************
免責聲明:
本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!

DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.

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

[Cédric Le Goater]

On 7/19/24 03:11, 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 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 for suggestion.
> Got it.

and so,


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> 
>> Thanks,
>>
>> C.
>>
>>
>>
>>
>>
> 
> ************* Email Confidentiality Notice ********************
> 免責聲明:
> 本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!
> 
> DISCLAIMER:
> This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.

[PATCH v1 04/15] hw/i2c/aspeed: support discontinuous register memory region of I2C bus

[Jamin Lin via]

It only support continuous register memory region for all I2C bus.
However, the register address of all I2c bus are discontinuous
for AST2700.

Ex: the register address of I2C bus for ast2700 as following.
0x100 - 0x17F: Device 0
0x200 - 0x27F: Device 1
0x300 - 0x37F: Device 2
0x400 - 0x47F: Device 3
0x500 - 0x57F: Device 4
0x600 - 0x67F: Device 5
0x700 - 0x77F: Device 6
0x800 - 0x87F: Device 7
0x900 - 0x97F: Device 8
0xA00 - 0xA7F: Device 9
0xB00 - 0xB7F: Device 10
0xC00 - 0xC7F: Device 11
0xD00 - 0xD7F: Device 12
0xE00 - 0xE7F: Device 13
0xF00 – 0xF7F: Device 14
0x1000 – 0x107F: Device 15

Introduce a new class attribute to make user set each I2C bus gap size.
Update formula to create all I2C bus register memory regions.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/i2c/aspeed_i2c.c         | 3 ++-
 include/hw/i2c/aspeed_i2c.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 7d5a53c4c0..462ad78a13 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -1011,6 +1011,7 @@ static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     AspeedI2CState *s = ASPEED_I2C(dev);
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s);
+    uint32_t reg_offset = aic->reg_size + aic->reg_gap_size;
 
     sysbus_init_irq(sbd, &s->irq);
     memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_i2c_ctrl_ops, s,
@@ -1033,7 +1034,7 @@ static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
             return;
         }
 
-        memory_region_add_subregion(&s->iomem, aic->reg_size * (i + offset),
+        memory_region_add_subregion(&s->iomem, reg_offset * (i + offset),
                                     &s->busses[i].mr);
     }
 
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 065b636d29..422ee0e298 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -275,6 +275,7 @@ struct AspeedI2CClass {
 
     uint8_t num_busses;
     uint8_t reg_size;
+    uint32_t reg_gap_size;
     uint8_t gap;
     qemu_irq (*bus_get_irq)(AspeedI2CBus *);
 
-- 
2.34.1

Re: [PATCH v1 04/15] hw/i2c/aspeed: support discontinuous register memory region of I2C bus

[Cédric Le Goater]

On 7/18/24 08:49, Jamin Lin wrote:
> It only support continuous register memory region for all I2C bus.
> However, the register address of all I2c bus are discontinuous
> for AST2700.
> 
> Ex: the register address of I2C bus for ast2700 as following.
> 0x100 - 0x17F: Device 0
> 0x200 - 0x27F: Device 1
> 0x300 - 0x37F: Device 2
> 0x400 - 0x47F: Device 3
> 0x500 - 0x57F: Device 4
> 0x600 - 0x67F: Device 5
> 0x700 - 0x77F: Device 6
> 0x800 - 0x87F: Device 7
> 0x900 - 0x97F: Device 8
> 0xA00 - 0xA7F: Device 9
> 0xB00 - 0xB7F: Device 10
> 0xC00 - 0xC7F: Device 11
> 0xD00 - 0xD7F: Device 12
> 0xE00 - 0xE7F: Device 13
> 0xF00 – 0xF7F: Device 14
> 0x1000 – 0x107F: Device 15
> 
> Introduce a new class attribute to make user set each I2C bus gap size.
> Update formula to create all I2C bus register memory regions.

I don't think this is necessary to model. Could we simply increase
tge register MMIO size for the AST2700 bus model and rely on the
memops to catch invalid register offsets ?


Thanks,

C.



> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/i2c/aspeed_i2c.c         | 3 ++-
>   include/hw/i2c/aspeed_i2c.h | 1 +
>   2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 7d5a53c4c0..462ad78a13 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -1011,6 +1011,7 @@ static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
>       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>       AspeedI2CState *s = ASPEED_I2C(dev);
>       AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s);
> +    uint32_t reg_offset = aic->reg_size + aic->reg_gap_size;
>   
>       sysbus_init_irq(sbd, &s->irq);
>       memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_i2c_ctrl_ops, s,
> @@ -1033,7 +1034,7 @@ static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
>               return;
>           }
>   
> -        memory_region_add_subregion(&s->iomem, aic->reg_size * (i + offset),
> +        memory_region_add_subregion(&s->iomem, reg_offset * (i + offset),
>                                       &s->busses[i].mr);
>       }
>   
> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> index 065b636d29..422ee0e298 100644
> --- a/include/hw/i2c/aspeed_i2c.h
> +++ b/include/hw/i2c/aspeed_i2c.h
> @@ -275,6 +275,7 @@ struct AspeedI2CClass {
>   
>       uint8_t num_busses;
>       uint8_t reg_size;
> +    uint32_t reg_gap_size;
>       uint8_t gap;
>       qemu_irq (*bus_get_irq)(AspeedI2CBus *);
>   

RE: [PATCH v1 04/15] hw/i2c/aspeed: support discontinuous register memory region of I2C bus

[Jamin Lin]

Hi Cedric,

> Subject: Re: [PATCH v1 04/15] hw/i2c/aspeed: support discontinuous register
> memory region of I2C bus
>
> On 7/18/24 08:49, Jamin Lin wrote:
> > It only support continuous register memory region for all I2C bus.
> > However, the register address of all I2c bus are discontinuous for
> > AST2700.
> >
> > Ex: the register address of I2C bus for ast2700 as following.
> > 0x100 - 0x17F: Device 0
> > 0x200 - 0x27F: Device 1
> > 0x300 - 0x37F: Device 2
> > 0x400 - 0x47F: Device 3
> > 0x500 - 0x57F: Device 4
> > 0x600 - 0x67F: Device 5
> > 0x700 - 0x77F: Device 6
> > 0x800 - 0x87F: Device 7
> > 0x900 - 0x97F: Device 8
> > 0xA00 - 0xA7F: Device 9
> > 0xB00 - 0xB7F: Device 10
> > 0xC00 - 0xC7F: Device 11
> > 0xD00 - 0xD7F: Device 12
> > 0xE00 - 0xE7F: Device 13
> > 0xF00 – 0xF7F: Device 14
> > 0x1000 – 0x107F: Device 15
> >
> > Introduce a new class attribute to make user set each I2C bus gap size.
> > Update formula to create all I2C bus register memory regions.
>
> I don't think this is necessary to model. Could we simply increase tge register
> MMIO size for the AST2700 bus model and rely on the memops to catch
> invalid register offsets ?
>
Thanks for your review and suggestion.

Sorry, I am not very clearly understand your comments.
Could you please describe it more detail?
Thanks-Jamin

>
> Thanks,
>
> C.
>
>
>
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> >   hw/i2c/aspeed_i2c.c         | 3 ++-
> >   include/hw/i2c/aspeed_i2c.h | 1 +
> >   2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index
> > 7d5a53c4c0..462ad78a13 100644
> > --- a/hw/i2c/aspeed_i2c.c
> > +++ b/hw/i2c/aspeed_i2c.c
> > @@ -1011,6 +1011,7 @@ static void aspeed_i2c_realize(DeviceState *dev,
> Error **errp)
> >       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> >       AspeedI2CState *s = ASPEED_I2C(dev);
> >       AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s);
> > +    uint32_t reg_offset = aic->reg_size + aic->reg_gap_size;
> >
> >       sysbus_init_irq(sbd, &s->irq);
> >       memory_region_init_io(&s->iomem, OBJECT(s),
> > &aspeed_i2c_ctrl_ops, s, @@ -1033,7 +1034,7 @@ static void
> aspeed_i2c_realize(DeviceState *dev, Error **errp)
> >               return;
> >           }
> >
> > -        memory_region_add_subregion(&s->iomem, aic->reg_size * (i +
> offset),
> > +        memory_region_add_subregion(&s->iomem, reg_offset * (i +
> > + offset),
> >                                       &s->busses[i].mr);
> >       }
> >
> > diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> > index 065b636d29..422ee0e298 100644
> > --- a/include/hw/i2c/aspeed_i2c.h
> > +++ b/include/hw/i2c/aspeed_i2c.h
> > @@ -275,6 +275,7 @@ struct AspeedI2CClass {
> >
> >       uint8_t num_busses;
> >       uint8_t reg_size;
> > +    uint32_t reg_gap_size;
> >       uint8_t gap;
> >       qemu_irq (*bus_get_irq)(AspeedI2CBus *);
> >

************* Email Confidentiality Notice ********************
免責聲明:
本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!

DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.

Re: [PATCH v1 04/15] hw/i2c/aspeed: support discontinuous register memory region of I2C bus

[Cédric Le Goater]

On 7/18/24 11:44, Jamin Lin wrote:
> Hi Cedric,
> 
>> Subject: Re: [PATCH v1 04/15] hw/i2c/aspeed: support discontinuous register
>> memory region of I2C bus
>>
>> On 7/18/24 08:49, Jamin Lin wrote:
>>> It only support continuous register memory region for all I2C bus.
>>> However, the register address of all I2c bus are discontinuous for
>>> AST2700.
>>>
>>> Ex: the register address of I2C bus for ast2700 as following.
>>> 0x100 - 0x17F: Device 0
>>> 0x200 - 0x27F: Device 1
>>> 0x300 - 0x37F: Device 2
>>> 0x400 - 0x47F: Device 3
>>> 0x500 - 0x57F: Device 4
>>> 0x600 - 0x67F: Device 5
>>> 0x700 - 0x77F: Device 6
>>> 0x800 - 0x87F: Device 7
>>> 0x900 - 0x97F: Device 8
>>> 0xA00 - 0xA7F: Device 9
>>> 0xB00 - 0xB7F: Device 10
>>> 0xC00 - 0xC7F: Device 11
>>> 0xD00 - 0xD7F: Device 12
>>> 0xE00 - 0xE7F: Device 13
>>> 0xF00 – 0xF7F: Device 14
>>> 0x1000 – 0x107F: Device 15
>>>
>>> Introduce a new class attribute to make user set each I2C bus gap size.
>>> Update formula to create all I2C bus register memory regions.
>>
>> I don't think this is necessary to model. Could we simply increase tge register
>> MMIO size for the AST2700 bus model and rely on the memops to catch
>> invalid register offsets ?
>>
> Thanks for your review and suggestion.
> 
> Sorry, I am not very clearly understand your comments.
> Could you please describe it more detail?
> Thanks-Jamin

I don't think you need to introduce a gap size class attribute.

Setting :

     aic->reg_size = 0x100; /* size + gap */

in aspeed_2700_i2c_class_init() should be enough.

Thanks,

C.

RE: [PATCH v1 04/15] hw/i2c/aspeed: support discontinuous register memory region of I2C bus

[Jamin Lin]

Hi Cedric,

> Subject: Re: [PATCH v1 04/15] hw/i2c/aspeed: support discontinuous register
> memory region of I2C bus
>
> On 7/18/24 11:44, Jamin Lin wrote:
> > Hi Cedric,
> >
> >> Subject: Re: [PATCH v1 04/15] hw/i2c/aspeed: support discontinuous
> >> register memory region of I2C bus
> >>
> >> On 7/18/24 08:49, Jamin Lin wrote:
> >>> It only support continuous register memory region for all I2C bus.
> >>> However, the register address of all I2c bus are discontinuous for
> >>> AST2700.
> >>>
> >>> Ex: the register address of I2C bus for ast2700 as following.
> >>> 0x100 - 0x17F: Device 0
> >>> 0x200 - 0x27F: Device 1
> >>> 0x300 - 0x37F: Device 2
> >>> 0x400 - 0x47F: Device 3
> >>> 0x500 - 0x57F: Device 4
> >>> 0x600 - 0x67F: Device 5
> >>> 0x700 - 0x77F: Device 6
> >>> 0x800 - 0x87F: Device 7
> >>> 0x900 - 0x97F: Device 8
> >>> 0xA00 - 0xA7F: Device 9
> >>> 0xB00 - 0xB7F: Device 10
> >>> 0xC00 - 0xC7F: Device 11
> >>> 0xD00 - 0xD7F: Device 12
> >>> 0xE00 - 0xE7F: Device 13
> >>> 0xF00 – 0xF7F: Device 14
> >>> 0x1000 – 0x107F: Device 15
> >>>
> >>> Introduce a new class attribute to make user set each I2C bus gap size.
> >>> Update formula to create all I2C bus register memory regions.
> >>
> >> I don't think this is necessary to model. Could we simply increase
> >> tge register MMIO size for the AST2700 bus model and rely on the
> >> memops to catch invalid register offsets ?
> >>
> > Thanks for your review and suggestion.
> >
> > Sorry, I am not very clearly understand your comments.
> > Could you please describe it more detail?
> > Thanks-Jamin
>
> I don't think you need to introduce a gap size class attribute.
>
> Setting :
>
>      aic->reg_size = 0x100; /* size + gap */
>
> in aspeed_2700_i2c_class_init() should be enough.
>
Sorry reply you late.

The address space of I2C bus and pool buffer are as following
0x100 - 0x17F: device1_reg  0x1a0 - 0x1bf: device1_buf
0x200 - 0x27F: device2_reg  0x2a0 - 0x2bf:device2_buf
0x300 - 0x37F: device3_reg  0x3a0 -0x3bf: device3_buf

I tried to set the aic->reg_size 0x100 and aic->pool_size 0x100.
And the memory regions of I2c bus became as following.

0x100-0x1ff aspeed.i2c.bus.0  0x1a0-0x29f aspeed.i2c.bus.0.pool
0x200-0x2ff aspeed.i2c.bus.1  0x2a0-0x39f aspeed.i2c.bus.1.pool
0x300-0x3ff aspeed.i2c.bus.2  0x3a0-0x49f aspeed.i2c.bus.2.pool

0000000014c0f000-0000000014c10fff (prio 0, i/o): aspeed.i2c
    0000000014c0f100-0000000014c0f1ff (prio 0, i/o): aspeed.i2c.bus.0
    0000000014c0f1a0-0000000014c0f29f (prio 0, i/o): aspeed.i2c.bus.0.pool
    0000000014c0f200-0000000014c0f2ff (prio 0, i/o): aspeed.i2c.bus.1
    0000000014c0f2a0-0000000014c0f39f (prio 0, i/o): aspeed.i2c.bus.1.pool
    0000000014c0f300-0000000014c0f3ff (prio 0, i/o): aspeed.i2c.bus.2
    0000000014c0f3a0-0000000014c0f49f (prio 0, i/o): aspeed.i2c.bus.2.pool

The memory region of aspeed.i2c.bus.0 (0x100-0x1ff) occupied aspeed.i2c.bus.0.pool address space(0x1a0-0x1bf).
And the memory region of aspeed.i2c.bus.0.pool (0x1a0-0x29f) occupied aspeed.i2c.bus.1 address space(0x200-0x27F)
That was why I created reg_gap_size and pool_gap_size to fix this issue.
Do you have any suggestion?

Thanks-Jamin

> Thanks,
>
> C.
>

************* Email Confidentiality Notice ********************
免責聲明:
本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!

DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.

Re: [PATCH v1 04/15] hw/i2c/aspeed: support discontinuous register memory region of I2C bus

[Cédric Le Goater]

Hello Jamin,

On 7/26/24 08:00, Jamin Lin wrote:
> Hi Cedric,

I will looked at v2. Sorry for the late reply, I was on PTO.

Thanks,

C.

  


>> Subject: Re: [PATCH v1 04/15] hw/i2c/aspeed: support discontinuous register
>> memory region of I2C bus
>>
>> On 7/18/24 11:44, Jamin Lin wrote:
>>> Hi Cedric,
>>>
>>>> Subject: Re: [PATCH v1 04/15] hw/i2c/aspeed: support discontinuous
>>>> register memory region of I2C bus
>>>>
>>>> On 7/18/24 08:49, Jamin Lin wrote:
>>>>> It only support continuous register memory region for all I2C bus.
>>>>> However, the register address of all I2c bus are discontinuous for
>>>>> AST2700.
>>>>>
>>>>> Ex: the register address of I2C bus for ast2700 as following.
>>>>> 0x100 - 0x17F: Device 0
>>>>> 0x200 - 0x27F: Device 1
>>>>> 0x300 - 0x37F: Device 2
>>>>> 0x400 - 0x47F: Device 3
>>>>> 0x500 - 0x57F: Device 4
>>>>> 0x600 - 0x67F: Device 5
>>>>> 0x700 - 0x77F: Device 6
>>>>> 0x800 - 0x87F: Device 7
>>>>> 0x900 - 0x97F: Device 8
>>>>> 0xA00 - 0xA7F: Device 9
>>>>> 0xB00 - 0xB7F: Device 10
>>>>> 0xC00 - 0xC7F: Device 11
>>>>> 0xD00 - 0xD7F: Device 12
>>>>> 0xE00 - 0xE7F: Device 13
>>>>> 0xF00 – 0xF7F: Device 14
>>>>> 0x1000 – 0x107F: Device 15
>>>>>
>>>>> Introduce a new class attribute to make user set each I2C bus gap size.
>>>>> Update formula to create all I2C bus register memory regions.
>>>>
>>>> I don't think this is necessary to model. Could we simply increase
>>>> tge register MMIO size for the AST2700 bus model and rely on the
>>>> memops to catch invalid register offsets ?
>>>>
>>> Thanks for your review and suggestion.
>>>
>>> Sorry, I am not very clearly understand your comments.
>>> Could you please describe it more detail?
>>> Thanks-Jamin
>>
>> I don't think you need to introduce a gap size class attribute.
>>
>> Setting :
>>
>>       aic->reg_size = 0x100; /* size + gap */
>>
>> in aspeed_2700_i2c_class_init() should be enough.
>>
> Sorry reply you late.
> 
> The address space of I2C bus and pool buffer are as following
> 0x100 - 0x17F: device1_reg  0x1a0 - 0x1bf: device1_buf
> 0x200 - 0x27F: device2_reg  0x2a0 - 0x2bf:device2_buf
> 0x300 - 0x37F: device3_reg  0x3a0 -0x3bf: device3_buf
> 
> I tried to set the aic->reg_size 0x100 and aic->pool_size 0x100.
> And the memory regions of I2c bus became as following.
> 
> 0x100-0x1ff aspeed.i2c.bus.0  0x1a0-0x29f aspeed.i2c.bus.0.pool
> 0x200-0x2ff aspeed.i2c.bus.1  0x2a0-0x39f aspeed.i2c.bus.1.pool
> 0x300-0x3ff aspeed.i2c.bus.2  0x3a0-0x49f aspeed.i2c.bus.2.pool
> 
> 0000000014c0f000-0000000014c10fff (prio 0, i/o): aspeed.i2c
>      0000000014c0f100-0000000014c0f1ff (prio 0, i/o): aspeed.i2c.bus.0
>      0000000014c0f1a0-0000000014c0f29f (prio 0, i/o): aspeed.i2c.bus.0.pool
>      0000000014c0f200-0000000014c0f2ff (prio 0, i/o): aspeed.i2c.bus.1
>      0000000014c0f2a0-0000000014c0f39f (prio 0, i/o): aspeed.i2c.bus.1.pool
>      0000000014c0f300-0000000014c0f3ff (prio 0, i/o): aspeed.i2c.bus.2
>      0000000014c0f3a0-0000000014c0f49f (prio 0, i/o): aspeed.i2c.bus.2.pool
> 
> The memory region of aspeed.i2c.bus.0 (0x100-0x1ff) occupied aspeed.i2c.bus.0.pool address space(0x1a0-0x1bf).
> And the memory region of aspeed.i2c.bus.0.pool (0x1a0-0x29f) occupied aspeed.i2c.bus.1 address space(0x200-0x27F)
> That was why I created reg_gap_size and pool_gap_size to fix this issue.
> Do you have any suggestion?
> 
> Thanks-Jamin
> 
>> Thanks,
>>
>> C.
>>
> 
> ************* Email Confidentiality Notice ********************
> 免責聲明:
> 本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!
> 
> DISCLAIMER:
> This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.

Re: [PATCH v1 04/15] hw/i2c/aspeed: support discontinuous register memory region of I2C bus

[林建明]

Hi Cedric,

Cédric Le Goater <clg@kaod.org> 於 2024年8月26日 週一 下午7:49寫道：
>
> Hello Jamin,
>
> On 7/26/24 08:00, Jamin Lin wrote:
> > Hi Cedric,
>
> I will looked at v2. Sorry for the late reply, I was on PTO.
>
> Thanks,
>
> C.
>
Thanks for help

Due to my company internal policy, it will automatically add the
following statement if I reply the reviewers questions via my outlook
with my aspeed account.
Therefore, I will change to use my personal gmail to reply reviewers question.
My Chinese name is "林建明"  gmail: jaminlin1207@gmail.com


************* Email Confidentiality Notice ********************
 免責聲明:
 本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者,
並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!

 DISCLAIMER:
 This message (and any attachments) may contain legally privileged
and/or other confidential information. If you have received it in
error, please notify the sender by reply e-mail and immediately delete
the e-mail and any attachments without copying or disclosing the
contents. Thank you.

Thanks-Jamin
>
>
>
>
> >> Subject: Re: [PATCH v1 04/15] hw/i2c/aspeed: support discontinuous register
> >> memory region of I2C bus
> >>
> >> On 7/18/24 11:44, Jamin Lin wrote:
> >>> Hi Cedric,
> >>>
> >>>> Subject: Re: [PATCH v1 04/15] hw/i2c/aspeed: support discontinuous
> >>>> register memory region of I2C bus
> >>>>
> >>>> On 7/18/24 08:49, Jamin Lin wrote:
> >>>>> It only support continuous register memory region for all I2C bus.
> >>>>> However, the register address of all I2c bus are discontinuous for
> >>>>> AST2700.
> >>>>>
> >>>>> Ex: the register address of I2C bus for ast2700 as following.
> >>>>> 0x100 - 0x17F: Device 0
> >>>>> 0x200 - 0x27F: Device 1
> >>>>> 0x300 - 0x37F: Device 2
> >>>>> 0x400 - 0x47F: Device 3
> >>>>> 0x500 - 0x57F: Device 4
> >>>>> 0x600 - 0x67F: Device 5
> >>>>> 0x700 - 0x77F: Device 6
> >>>>> 0x800 - 0x87F: Device 7
> >>>>> 0x900 - 0x97F: Device 8
> >>>>> 0xA00 - 0xA7F: Device 9
> >>>>> 0xB00 - 0xB7F: Device 10
> >>>>> 0xC00 - 0xC7F: Device 11
> >>>>> 0xD00 - 0xD7F: Device 12
> >>>>> 0xE00 - 0xE7F: Device 13
> >>>>> 0xF00 – 0xF7F: Device 14
> >>>>> 0x1000 – 0x107F: Device 15
> >>>>>
> >>>>> Introduce a new class attribute to make user set each I2C bus gap size.
> >>>>> Update formula to create all I2C bus register memory regions.
> >>>>
> >>>> I don't think this is necessary to model. Could we simply increase
> >>>> tge register MMIO size for the AST2700 bus model and rely on the
> >>>> memops to catch invalid register offsets ?
> >>>>
> >>> Thanks for your review and suggestion.
> >>>
> >>> Sorry, I am not very clearly understand your comments.
> >>> Could you please describe it more detail?
> >>> Thanks-Jamin
> >>
> >> I don't think you need to introduce a gap size class attribute.
> >>
> >> Setting :
> >>
> >>       aic->reg_size = 0x100; /* size + gap */
> >>
> >> in aspeed_2700_i2c_class_init() should be enough.
> >>
> > Sorry reply you late.
> >
> > The address space of I2C bus and pool buffer are as following
> > 0x100 - 0x17F: device1_reg  0x1a0 - 0x1bf: device1_buf
> > 0x200 - 0x27F: device2_reg  0x2a0 - 0x2bf:device2_buf
> > 0x300 - 0x37F: device3_reg  0x3a0 -0x3bf: device3_buf
> >
> > I tried to set the aic->reg_size 0x100 and aic->pool_size 0x100.
> > And the memory regions of I2c bus became as following.
> >
> > 0x100-0x1ff aspeed.i2c.bus.0  0x1a0-0x29f aspeed.i2c.bus.0.pool
> > 0x200-0x2ff aspeed.i2c.bus.1  0x2a0-0x39f aspeed.i2c.bus.1.pool
> > 0x300-0x3ff aspeed.i2c.bus.2  0x3a0-0x49f aspeed.i2c.bus.2.pool
> >
> > 0000000014c0f000-0000000014c10fff (prio 0, i/o): aspeed.i2c
> >      0000000014c0f100-0000000014c0f1ff (prio 0, i/o): aspeed.i2c.bus.0
> >      0000000014c0f1a0-0000000014c0f29f (prio 0, i/o): aspeed.i2c.bus.0.pool
> >      0000000014c0f200-0000000014c0f2ff (prio 0, i/o): aspeed.i2c.bus.1
> >      0000000014c0f2a0-0000000014c0f39f (prio 0, i/o): aspeed.i2c.bus.1.pool
> >      0000000014c0f300-0000000014c0f3ff (prio 0, i/o): aspeed.i2c.bus.2
> >      0000000014c0f3a0-0000000014c0f49f (prio 0, i/o): aspeed.i2c.bus.2.pool
> >
> > The memory region of aspeed.i2c.bus.0 (0x100-0x1ff) occupied aspeed.i2c.bus.0.pool address space(0x1a0-0x1bf).
> > And the memory region of aspeed.i2c.bus.0.pool (0x1a0-0x29f) occupied aspeed.i2c.bus.1 address space(0x200-0x27F)
> > That was why I created reg_gap_size and pool_gap_size to fix this issue.
> > Do you have any suggestion?
> >
> > Thanks-Jamin
> >
> >> Thanks,
> >>
> >> C.
> >>
> >
> > ************* Email Confidentiality Notice ********************
> > 免責聲明:
> > 本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!
> >
> > DISCLAIMER:
> > This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.
>
>

[PATCH v1 05/15] hw/i2c/aspeed: rename the I2C class pool attribute to share_pool

[Jamin Lin via]

According to the datasheet of ASPEED SOCs,
each I2C bus has their own pool buffer since AST2500.

Only AST2400 utilized a pool buffer share to all I2C bus.
And firmware required to set the offset of pool buffer
by writing "Function Control Register(I2CD 00)"

To make this model more readable, will change to introduce
a new bus pool buffer attribute in AspeedI2Cbus.
So, it does not need to calculate the pool buffer offset
for different I2C bus.

This patch rename the I2C class pool attribute to share_pool.
It make user more understand share pool and bus pool
are different.

Incrementing the version of aspeed_i2c_vmstate to 3.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/i2c/aspeed_i2c.c         | 39 ++++++++++++++++++++-----------------
 include/hw/i2c/aspeed_i2c.h |  4 ++--
 2 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 462ad78a13..9c222a02fe 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -906,7 +906,7 @@ static const MemoryRegionOps aspeed_i2c_ctrl_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-static uint64_t aspeed_i2c_pool_read(void *opaque, hwaddr offset,
+static uint64_t aspeed_i2c_share_pool_read(void *opaque, hwaddr offset,
                                      unsigned size)
 {
     AspeedI2CState *s = opaque;
@@ -914,26 +914,26 @@ static uint64_t aspeed_i2c_pool_read(void *opaque, hwaddr offset,
     int i;
 
     for (i = 0; i < size; i++) {
-        ret |= (uint64_t) s->pool[offset + i] << (8 * i);
+        ret |= (uint64_t) s->share_pool[offset + i] << (8 * i);
     }
 
     return ret;
 }
 
-static void aspeed_i2c_pool_write(void *opaque, hwaddr offset,
+static void aspeed_i2c_share_pool_write(void *opaque, hwaddr offset,
                                   uint64_t value, unsigned size)
 {
     AspeedI2CState *s = opaque;
     int i;
 
     for (i = 0; i < size; i++) {
-        s->pool[offset + i] = (value >> (8 * i)) & 0xFF;
+        s->share_pool[offset + i] = (value >> (8 * i)) & 0xFF;
     }
 }
 
-static const MemoryRegionOps aspeed_i2c_pool_ops = {
-    .read = aspeed_i2c_pool_read,
-    .write = aspeed_i2c_pool_write,
+static const MemoryRegionOps aspeed_i2c_share_pool_ops = {
+    .read = aspeed_i2c_share_pool_read,
+    .write = aspeed_i2c_share_pool_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .valid = {
         .min_access_size = 1,
@@ -953,14 +953,15 @@ static const VMStateDescription aspeed_i2c_bus_vmstate = {
 
 static const VMStateDescription aspeed_i2c_vmstate = {
     .name = TYPE_ASPEED_I2C,
-    .version_id = 2,
-    .minimum_version_id = 2,
+    .version_id = 3,
+    .minimum_version_id = 3,
     .fields = (const VMStateField[]) {
         VMSTATE_UINT32(intr_status, AspeedI2CState),
         VMSTATE_STRUCT_ARRAY(busses, AspeedI2CState,
                              ASPEED_I2C_NR_BUSSES, 1, aspeed_i2c_bus_vmstate,
                              AspeedI2CBus),
-        VMSTATE_UINT8_ARRAY(pool, AspeedI2CState, ASPEED_I2C_MAX_POOL_SIZE),
+        VMSTATE_UINT8_ARRAY(share_pool, AspeedI2CState,
+                            ASPEED_I2C_SHARE_POOL_SIZE),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -995,7 +996,7 @@ static void aspeed_i2c_instance_init(Object *obj)
  *   0x140 ... 0x17F: Device 5
  *   0x180 ... 0x1BF: Device 6
  *   0x1C0 ... 0x1FF: Device 7
- *   0x200 ... 0x2FF: Buffer Pool  (unused in linux driver)
+ *   0x200 ... 0x2FF: Buffer Pool (AST2500 unused in linux driver)
  *   0x300 ... 0x33F: Device 8
  *   0x340 ... 0x37F: Device 9
  *   0x380 ... 0x3BF: Device 10
@@ -1003,7 +1004,7 @@ static void aspeed_i2c_instance_init(Object *obj)
  *   0x400 ... 0x43F: Device 12
  *   0x440 ... 0x47F: Device 13
  *   0x480 ... 0x4BF: Device 14
- *   0x800 ... 0xFFF: Buffer Pool  (unused in linux driver)
+ *   0x800 ... 0xFFF: Buffer Pool (AST2400 unused in linux driver)
  */
 static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
 {
@@ -1038,8 +1039,9 @@ static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
                                     &s->busses[i].mr);
     }
 
-    memory_region_init_io(&s->pool_iomem, OBJECT(s), &aspeed_i2c_pool_ops, s,
-                          "aspeed.i2c-pool", aic->pool_size);
+    memory_region_init_io(&s->pool_iomem, OBJECT(s),
+                          &aspeed_i2c_share_pool_ops, s,
+                          "aspeed.i2c-share-pool", aic->pool_size);
     memory_region_add_subregion(&s->iomem, aic->pool_base, &s->pool_iomem);
 
     if (aic->has_dma) {
@@ -1267,8 +1269,9 @@ static qemu_irq aspeed_2400_i2c_bus_get_irq(AspeedI2CBus *bus)
 static uint8_t *aspeed_2400_i2c_bus_pool_base(AspeedI2CBus *bus)
 {
     uint8_t *pool_page =
-        &bus->controller->pool[ARRAY_FIELD_EX32(bus->regs, I2CD_FUN_CTRL,
-                                                POOL_PAGE_SEL) * 0x100];
+        &bus->controller->share_pool[ARRAY_FIELD_EX32(bus->regs,
+                                                      I2CD_FUN_CTRL,
+                                                      POOL_PAGE_SEL) * 0x100];
 
     return &pool_page[ARRAY_FIELD_EX32(bus->regs, I2CD_POOL_CTRL, OFFSET)];
 }
@@ -1303,7 +1306,7 @@ static qemu_irq aspeed_2500_i2c_bus_get_irq(AspeedI2CBus *bus)
 
 static uint8_t *aspeed_2500_i2c_bus_pool_base(AspeedI2CBus *bus)
 {
-    return &bus->controller->pool[bus->id * 0x10];
+    return &bus->controller->share_pool[bus->id * 0x10];
 }
 
 static void aspeed_2500_i2c_class_init(ObjectClass *klass, void *data)
@@ -1338,7 +1341,7 @@ static qemu_irq aspeed_2600_i2c_bus_get_irq(AspeedI2CBus *bus)
 
 static uint8_t *aspeed_2600_i2c_bus_pool_base(AspeedI2CBus *bus)
 {
-   return &bus->controller->pool[bus->id * 0x20];
+   return &bus->controller->share_pool[bus->id * 0x20];
 }
 
 static void aspeed_2600_i2c_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 422ee0e298..02ede85906 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -34,7 +34,7 @@
 OBJECT_DECLARE_TYPE(AspeedI2CState, AspeedI2CClass, ASPEED_I2C)
 
 #define ASPEED_I2C_NR_BUSSES 16
-#define ASPEED_I2C_MAX_POOL_SIZE 0x800
+#define ASPEED_I2C_SHARE_POOL_SIZE 0x800
 #define ASPEED_I2C_OLD_NUM_REG 11
 #define ASPEED_I2C_NEW_NUM_REG 22
 
@@ -257,7 +257,7 @@ struct AspeedI2CState {
     uint32_t ctrl_global;
     uint32_t new_clk_divider;
     MemoryRegion pool_iomem;
-    uint8_t pool[ASPEED_I2C_MAX_POOL_SIZE];
+    uint8_t share_pool[ASPEED_I2C_SHARE_POOL_SIZE];
 
     AspeedI2CBus busses[ASPEED_I2C_NR_BUSSES];
     MemoryRegion *dram_mr;
-- 
2.34.1

Re: [PATCH v1 05/15] hw/i2c/aspeed: rename the I2C class pool attribute to share_pool

[Cédric Le Goater]

On 7/18/24 08:49, Jamin Lin wrote:
> According to the datasheet of ASPEED SOCs,
> each I2C bus has their own pool buffer since AST2500.
> 
> Only AST2400 utilized a pool buffer share to all I2C bus.
> And firmware required to set the offset of pool buffer
> by writing "Function Control Register(I2CD 00)"
> 
> To make this model more readable, will change to introduce
> a new bus pool buffer attribute in AspeedI2Cbus.
> So, it does not need to calculate the pool buffer offset
> for different I2C bus.
> 
> This patch rename the I2C class pool attribute to share_pool.
> It make user more understand share pool and bus pool
> are different.
> 
> Incrementing the version of aspeed_i2c_vmstate to 3.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/i2c/aspeed_i2c.c         | 39 ++++++++++++++++++++-----------------
>   include/hw/i2c/aspeed_i2c.h |  4 ++--
>   2 files changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 462ad78a13..9c222a02fe 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -906,7 +906,7 @@ static const MemoryRegionOps aspeed_i2c_ctrl_ops = {
>       .endianness = DEVICE_LITTLE_ENDIAN,
>   };
>   
> -static uint64_t aspeed_i2c_pool_read(void *opaque, hwaddr offset,
> +static uint64_t aspeed_i2c_share_pool_read(void *opaque, hwaddr offset,
>                                        unsigned size)
>   {
>       AspeedI2CState *s = opaque;
> @@ -914,26 +914,26 @@ static uint64_t aspeed_i2c_pool_read(void *opaque, hwaddr offset,
>       int i;
>   
>       for (i = 0; i < size; i++) {
> -        ret |= (uint64_t) s->pool[offset + i] << (8 * i);
> +        ret |= (uint64_t) s->share_pool[offset + i] << (8 * i);
>       }
>   
>       return ret;
>   }
>   
> -static void aspeed_i2c_pool_write(void *opaque, hwaddr offset,
> +static void aspeed_i2c_share_pool_write(void *opaque, hwaddr offset,
>                                     uint64_t value, unsigned size)
>   {
>       AspeedI2CState *s = opaque;
>       int i;
>   
>       for (i = 0; i < size; i++) {
> -        s->pool[offset + i] = (value >> (8 * i)) & 0xFF;
> +        s->share_pool[offset + i] = (value >> (8 * i)) & 0xFF;
>       }
>   }
>   
> -static const MemoryRegionOps aspeed_i2c_pool_ops = {
> -    .read = aspeed_i2c_pool_read,
> -    .write = aspeed_i2c_pool_write,
> +static const MemoryRegionOps aspeed_i2c_share_pool_ops = {
> +    .read = aspeed_i2c_share_pool_read,
> +    .write = aspeed_i2c_share_pool_write,
>       .endianness = DEVICE_LITTLE_ENDIAN,
>       .valid = {
>           .min_access_size = 1,
> @@ -953,14 +953,15 @@ static const VMStateDescription aspeed_i2c_bus_vmstate = {
>   
>   static const VMStateDescription aspeed_i2c_vmstate = {
>       .name = TYPE_ASPEED_I2C,
> -    .version_id = 2,
> -    .minimum_version_id = 2,
> +    .version_id = 3,
> +    .minimum_version_id = 3,
>       .fields = (const VMStateField[]) {
>           VMSTATE_UINT32(intr_status, AspeedI2CState),
>           VMSTATE_STRUCT_ARRAY(busses, AspeedI2CState,
>                                ASPEED_I2C_NR_BUSSES, 1, aspeed_i2c_bus_vmstate,
>                                AspeedI2CBus),
> -        VMSTATE_UINT8_ARRAY(pool, AspeedI2CState, ASPEED_I2C_MAX_POOL_SIZE),
> +        VMSTATE_UINT8_ARRAY(share_pool, AspeedI2CState,
> +                            ASPEED_I2C_SHARE_POOL_SIZE),
>           VMSTATE_END_OF_LIST()
>       }
>   };
> @@ -995,7 +996,7 @@ static void aspeed_i2c_instance_init(Object *obj)
>    *   0x140 ... 0x17F: Device 5
>    *   0x180 ... 0x1BF: Device 6
>    *   0x1C0 ... 0x1FF: Device 7
> - *   0x200 ... 0x2FF: Buffer Pool  (unused in linux driver)
> + *   0x200 ... 0x2FF: Buffer Pool (AST2500 unused in linux driver)
>    *   0x300 ... 0x33F: Device 8
>    *   0x340 ... 0x37F: Device 9
>    *   0x380 ... 0x3BF: Device 10
> @@ -1003,7 +1004,7 @@ static void aspeed_i2c_instance_init(Object *obj)
>    *   0x400 ... 0x43F: Device 12
>    *   0x440 ... 0x47F: Device 13
>    *   0x480 ... 0x4BF: Device 14
> - *   0x800 ... 0xFFF: Buffer Pool  (unused in linux driver)
> + *   0x800 ... 0xFFF: Buffer Pool (AST2400 unused in linux driver)
>    */
>   static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
>   {
> @@ -1038,8 +1039,9 @@ static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
>                                       &s->busses[i].mr);
>       }
>   
> -    memory_region_init_io(&s->pool_iomem, OBJECT(s), &aspeed_i2c_pool_ops, s,
> -                          "aspeed.i2c-pool", aic->pool_size);
> +    memory_region_init_io(&s->pool_iomem, OBJECT(s),
> +                          &aspeed_i2c_share_pool_ops, s,
> +                          "aspeed.i2c-share-pool", aic->pool_size);
>       memory_region_add_subregion(&s->iomem, aic->pool_base, &s->pool_iomem);
>   
>       if (aic->has_dma) {
> @@ -1267,8 +1269,9 @@ static qemu_irq aspeed_2400_i2c_bus_get_irq(AspeedI2CBus *bus)
>   static uint8_t *aspeed_2400_i2c_bus_pool_base(AspeedI2CBus *bus)
>   {
>       uint8_t *pool_page =
> -        &bus->controller->pool[ARRAY_FIELD_EX32(bus->regs, I2CD_FUN_CTRL,
> -                                                POOL_PAGE_SEL) * 0x100];
> +        &bus->controller->share_pool[ARRAY_FIELD_EX32(bus->regs,
> +                                                      I2CD_FUN_CTRL,
> +                                                      POOL_PAGE_SEL) * 0x100];
>   
>       return &pool_page[ARRAY_FIELD_EX32(bus->regs, I2CD_POOL_CTRL, OFFSET)];
>   }
> @@ -1303,7 +1306,7 @@ static qemu_irq aspeed_2500_i2c_bus_get_irq(AspeedI2CBus *bus)
>   
>   static uint8_t *aspeed_2500_i2c_bus_pool_base(AspeedI2CBus *bus)
>   {
> -    return &bus->controller->pool[bus->id * 0x10];
> +    return &bus->controller->share_pool[bus->id * 0x10];
>   }
>   
>   static void aspeed_2500_i2c_class_init(ObjectClass *klass, void *data)
> @@ -1338,7 +1341,7 @@ static qemu_irq aspeed_2600_i2c_bus_get_irq(AspeedI2CBus *bus)
>   
>   static uint8_t *aspeed_2600_i2c_bus_pool_base(AspeedI2CBus *bus)
>   {
> -   return &bus->controller->pool[bus->id * 0x20];
> +   return &bus->controller->share_pool[bus->id * 0x20];
>   }
>   
>   static void aspeed_2600_i2c_class_init(ObjectClass *klass, void *data)
> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> index 422ee0e298..02ede85906 100644
> --- a/include/hw/i2c/aspeed_i2c.h
> +++ b/include/hw/i2c/aspeed_i2c.h
> @@ -34,7 +34,7 @@
>   OBJECT_DECLARE_TYPE(AspeedI2CState, AspeedI2CClass, ASPEED_I2C)
>   
>   #define ASPEED_I2C_NR_BUSSES 16
> -#define ASPEED_I2C_MAX_POOL_SIZE 0x800
> +#define ASPEED_I2C_SHARE_POOL_SIZE 0x800
>   #define ASPEED_I2C_OLD_NUM_REG 11
>   #define ASPEED_I2C_NEW_NUM_REG 22
>   
> @@ -257,7 +257,7 @@ struct AspeedI2CState {
>       uint32_t ctrl_global;
>       uint32_t new_clk_divider;
>       MemoryRegion pool_iomem;
> -    uint8_t pool[ASPEED_I2C_MAX_POOL_SIZE];
> +    uint8_t share_pool[ASPEED_I2C_SHARE_POOL_SIZE];
>   
>       AspeedI2CBus busses[ASPEED_I2C_NR_BUSSES];
>       MemoryRegion *dram_mr;

[PATCH v1 06/15] hw/i2c/aspeed: introduce a new bus pool buffer attribute in AspeedI2Cbus

[Jamin Lin via]

According to the datasheet of ASPEED SOCs,
each I2C bus has their own pool buffer since AST2500.
Only AST2400 utilized a pool buffer share to all I2C bus.
Besides, using a share pool buffer only support
pool buffer memory regions are continuous for all I2C bus.

To make this model more readable and support discontinuous
bus pool buffer memory regions, changes to introduce
a new bus pool buffer attribute in AspeedI2Cbus and
new memops. So, it does not need to calculate
the pool buffer offset for different I2C bus.

Introduce a new has_share_pool class attribute in AspeedI2CClass and
use it to create either a share pool buffer or bus pool buffers
in aspeed_i2c_realize. Update each pull buffer size to 0x10 for AST2500
and 0x20 for AST2600 and AST1030.

Incrementing the version of aspeed_i2c_bus_vmstate to 6.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/i2c/aspeed_i2c.c         | 131 +++++++++++++++++++++++++++++++-----
 include/hw/i2c/aspeed_i2c.h |   4 ++
 2 files changed, 117 insertions(+), 18 deletions(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 9c222a02fe..d3d49340ea 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -941,12 +941,48 @@ static const MemoryRegionOps aspeed_i2c_share_pool_ops = {
     },
 };
 
+static uint64_t aspeed_i2c_bus_pool_read(void *opaque, hwaddr offset,
+                                     unsigned size)
+{
+    AspeedI2CBus *s = opaque;
+    uint64_t ret = 0;
+    int i;
+
+    for (i = 0; i < size; i++) {
+        ret |= (uint64_t) s->pool[offset + i] << (8 * i);
+    }
+
+    return ret;
+}
+
+static void aspeed_i2c_bus_pool_write(void *opaque, hwaddr offset,
+                                  uint64_t value, unsigned size)
+{
+    AspeedI2CBus *s = opaque;
+    int i;
+
+    for (i = 0; i < size; i++) {
+        s->pool[offset + i] = (value >> (8 * i)) & 0xFF;
+    }
+}
+
+static const MemoryRegionOps aspeed_i2c_bus_pool_ops = {
+    .read = aspeed_i2c_bus_pool_read,
+    .write = aspeed_i2c_bus_pool_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 4,
+    },
+};
+
 static const VMStateDescription aspeed_i2c_bus_vmstate = {
     .name = TYPE_ASPEED_I2C,
-    .version_id = 5,
-    .minimum_version_id = 5,
+    .version_id = 6,
+    .minimum_version_id = 6,
     .fields = (const VMStateField[]) {
         VMSTATE_UINT32_ARRAY(regs, AspeedI2CBus, ASPEED_I2C_NEW_NUM_REG),
+        VMSTATE_UINT8_ARRAY(pool, AspeedI2CBus, ASPEED_I2C_BUS_POOL_SIZE),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -996,7 +1032,21 @@ static void aspeed_i2c_instance_init(Object *obj)
  *   0x140 ... 0x17F: Device 5
  *   0x180 ... 0x1BF: Device 6
  *   0x1C0 ... 0x1FF: Device 7
- *   0x200 ... 0x2FF: Buffer Pool (AST2500 unused in linux driver)
+ *   0x200 ... 0x20F: Device 1 buffer (AST2500 unused in linux driver)
+ *   0x210 ... 0x21F: Device 2 buffer
+ *   0x220 ... 0x22F: Device 3 buffer
+ *   0x230 ... 0x23F: Device 4 buffer
+ *   0x240 ... 0x24F: Device 5 buffer
+ *   0x250 ... 0x25F: Device 6 buffer
+ *   0x260 ... 0x26F: Device 7 buffer
+ *   0x270 ... 0x27F: Device 8 buffer
+ *   0x280 ... 0x28F: Device 9 buffer
+ *   0x290 ... 0x29F: Device 10 buffer
+ *   0x2A0 ... 0x2AF: Device 11 buffer
+ *   0x2B0 ... 0x2BF: Device 12 buffer
+ *   0x2C0 ... 0x2CF: Device 13 buffer
+ *   0x2D0 ... 0x2DF: Device 14 buffer
+ *   0x2E0 ... 0x2FF: Reserved
  *   0x300 ... 0x33F: Device 8
  *   0x340 ... 0x37F: Device 9
  *   0x380 ... 0x3BF: Device 10
@@ -1005,6 +1055,41 @@ static void aspeed_i2c_instance_init(Object *obj)
  *   0x440 ... 0x47F: Device 13
  *   0x480 ... 0x4BF: Device 14
  *   0x800 ... 0xFFF: Buffer Pool (AST2400 unused in linux driver)
+ *
+ * Address Definitions (AST2600 and AST1030)
+ *   0x000 ... 0x07F: Global Register
+ *   0x080 ... 0x0FF: Device 1
+ *   0x100 ... 0x17F: Device 2
+ *   0x180 ... 0x1FF: Device 3
+ *   0x200 ... 0x27F: Device 4
+ *   0x280 ... 0x2FF: Device 5
+ *   0x300 ... 0x37F: Device 6
+ *   0x380 ... 0x3FF: Device 7
+ *   0x400 ... 0x47F: Device 8
+ *   0x480 ... 0x4FF: Device 9
+ *   0x500 ... 0x57F: Device 10
+ *   0x580 ... 0x5FF: Device 11
+ *   0x600 ... 0x67F: Device 12
+ *   0x680 ... 0x6FF: Device 13
+ *   0x700 ... 0x77F: Device 14
+ *   0x780 ... 0x7FF: Device 15 (15 and 16 unused in AST1030)
+ *   0x800 ... 0x87F: Device 16
+ *   0xC00 ... 0xC1F: Device 1 buffer
+ *   0xC20 ... 0xC3F: Device 2 buffer
+ *   0xC40 ... 0xC5F: Device 3 buffer
+ *   0xC60 ... 0xC7F: Device 4 buffer
+ *   0xC80 ... 0xC9F: Device 5 buffer
+ *   0xCA0 ... 0xCBF: Device 6 buffer
+ *   0xCC0 ... 0xCDF: Device 7 buffer
+ *   0xCE0 ... 0xCFF: Device 8 buffer
+ *   0xD00 ... 0xD1F: Device 9 buffer
+ *   0xD20 ... 0xD3F: Device 10 buffer
+ *   0xD40 ... 0xD5F: Device 11 buffer
+ *   0xD60 ... 0xD7F: Device 12 buffer
+ *   0xD80 ... 0xD9F: Device 13 buffer
+ *   0xDA0 ... 0xDBF: Device 14 buffer
+ *   0xDC0 ... 0xDDF: Device 15 buffer (15 and 16 unused in AST1030)
+ *   0xDE0 ... 0xDFF: Device 16 buffer
  */
 static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
 {
@@ -1039,10 +1124,19 @@ static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
                                     &s->busses[i].mr);
     }
 
-    memory_region_init_io(&s->pool_iomem, OBJECT(s),
-                          &aspeed_i2c_share_pool_ops, s,
-                          "aspeed.i2c-share-pool", aic->pool_size);
-    memory_region_add_subregion(&s->iomem, aic->pool_base, &s->pool_iomem);
+    if (aic->has_share_pool) {
+        memory_region_init_io(&s->pool_iomem, OBJECT(s),
+                              &aspeed_i2c_share_pool_ops, s,
+                              "aspeed.i2c-share-pool", aic->pool_size);
+        memory_region_add_subregion(&s->iomem, aic->pool_base,
+                                    &s->pool_iomem);
+    } else {
+        for (i = 0; i < aic->num_busses; i++) {
+            memory_region_add_subregion(&s->iomem,
+                                        aic->pool_base + (aic->pool_size * i),
+                                        &s->busses[i].mr_pool);
+        }
+    }
 
     if (aic->has_dma) {
         if (!s->dram_mr) {
@@ -1218,6 +1312,7 @@ 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);
+    g_autofree char *pool_name = g_strdup_printf("%s.pool", name);
 
     if (!s->controller) {
         error_setg(errp, TYPE_ASPEED_I2C_BUS ": 'controller' link not set");
@@ -1235,6 +1330,10 @@ static void aspeed_i2c_bus_realize(DeviceState *dev, Error **errp)
     memory_region_init_io(&s->mr, OBJECT(s), &aspeed_i2c_bus_ops,
                           s, name, aic->reg_size);
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr);
+
+    memory_region_init_io(&s->mr_pool, OBJECT(s), &aspeed_i2c_bus_pool_ops,
+                          s, pool_name, aic->pool_size);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr_pool);
 }
 
 static Property aspeed_i2c_bus_properties[] = {
@@ -1287,6 +1386,7 @@ static void aspeed_2400_i2c_class_init(ObjectClass *klass, void *data)
     aic->reg_size = 0x40;
     aic->gap = 7;
     aic->bus_get_irq = aspeed_2400_i2c_bus_get_irq;
+    aic->has_share_pool = true;
     aic->pool_size = 0x800;
     aic->pool_base = 0x800;
     aic->bus_pool_base = aspeed_2400_i2c_bus_pool_base;
@@ -1306,7 +1406,7 @@ static qemu_irq aspeed_2500_i2c_bus_get_irq(AspeedI2CBus *bus)
 
 static uint8_t *aspeed_2500_i2c_bus_pool_base(AspeedI2CBus *bus)
 {
-    return &bus->controller->share_pool[bus->id * 0x10];
+    return bus->pool;
 }
 
 static void aspeed_2500_i2c_class_init(ObjectClass *klass, void *data)
@@ -1320,7 +1420,7 @@ static void aspeed_2500_i2c_class_init(ObjectClass *klass, void *data)
     aic->reg_size = 0x40;
     aic->gap = 7;
     aic->bus_get_irq = aspeed_2500_i2c_bus_get_irq;
-    aic->pool_size = 0x100;
+    aic->pool_size = 0x10;
     aic->pool_base = 0x200;
     aic->bus_pool_base = aspeed_2500_i2c_bus_pool_base;
     aic->check_sram = true;
@@ -1339,11 +1439,6 @@ static qemu_irq aspeed_2600_i2c_bus_get_irq(AspeedI2CBus *bus)
     return bus->irq;
 }
 
-static uint8_t *aspeed_2600_i2c_bus_pool_base(AspeedI2CBus *bus)
-{
-   return &bus->controller->share_pool[bus->id * 0x20];
-}
-
 static void aspeed_2600_i2c_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -1355,9 +1450,9 @@ static void aspeed_2600_i2c_class_init(ObjectClass *klass, void *data)
     aic->reg_size = 0x80;
     aic->gap = -1; /* no gap */
     aic->bus_get_irq = aspeed_2600_i2c_bus_get_irq;
-    aic->pool_size = 0x200;
+    aic->pool_size = 0x20;
     aic->pool_base = 0xC00;
-    aic->bus_pool_base = aspeed_2600_i2c_bus_pool_base;
+    aic->bus_pool_base = aspeed_2500_i2c_bus_pool_base;
     aic->has_dma = true;
     aic->mem_size = 0x1000;
 }
@@ -1379,9 +1474,9 @@ static void aspeed_1030_i2c_class_init(ObjectClass *klass, void *data)
     aic->reg_size = 0x80;
     aic->gap = -1; /* no gap */
     aic->bus_get_irq = aspeed_2600_i2c_bus_get_irq;
-    aic->pool_size = 0x200;
+    aic->pool_size = 0x20;
     aic->pool_base = 0xC00;
-    aic->bus_pool_base = aspeed_2600_i2c_bus_pool_base;
+    aic->bus_pool_base = aspeed_2500_i2c_bus_pool_base;
     aic->has_dma = true;
     aic->mem_size = 0x10000;
 }
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 02ede85906..8e62ec64f8 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -35,6 +35,7 @@ OBJECT_DECLARE_TYPE(AspeedI2CState, AspeedI2CClass, ASPEED_I2C)
 
 #define ASPEED_I2C_NR_BUSSES 16
 #define ASPEED_I2C_SHARE_POOL_SIZE 0x800
+#define ASPEED_I2C_BUS_POOL_SIZE 0x20
 #define ASPEED_I2C_OLD_NUM_REG 11
 #define ASPEED_I2C_NEW_NUM_REG 22
 
@@ -239,12 +240,14 @@ struct AspeedI2CBus {
     I2CSlave *slave;
 
     MemoryRegion mr;
+    MemoryRegion mr_pool;
 
     I2CBus *bus;
     uint8_t id;
     qemu_irq irq;
 
     uint32_t regs[ASPEED_I2C_NEW_NUM_REG];
+    uint8_t pool[ASPEED_I2C_BUS_POOL_SIZE];
 };
 
 struct AspeedI2CState {
@@ -284,6 +287,7 @@ struct AspeedI2CClass {
     uint8_t *(*bus_pool_base)(AspeedI2CBus *);
     bool check_sram;
     bool has_dma;
+    bool has_share_pool;
     uint64_t mem_size;
 };
 
-- 
2.34.1

Re: [PATCH v1 06/15] hw/i2c/aspeed: introduce a new bus pool buffer attribute in AspeedI2Cbus

[Cédric Le Goater]

On 7/18/24 08:49, Jamin Lin wrote:
> According to the datasheet of ASPEED SOCs,
> each I2C bus has their own pool buffer since AST2500.
> Only AST2400 utilized a pool buffer share to all I2C bus.
> Besides, using a share pool buffer only support
> pool buffer memory regions are continuous for all I2C bus.
> 
> To make this model more readable and support discontinuous
> bus pool buffer memory regions, changes to introduce
> a new bus pool buffer attribute in AspeedI2Cbus and
> new memops. So, it does not need to calculate
> the pool buffer offset for different I2C bus.
> 
> Introduce a new has_share_pool class attribute in AspeedI2CClass and
> use it to create either a share pool buffer or bus pool buffers
> in aspeed_i2c_realize. Update each pull buffer size to 0x10 for AST2500
> and 0x20 for AST2600 and AST1030.
> 
> Incrementing the version of aspeed_i2c_bus_vmstate to 6.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/i2c/aspeed_i2c.c         | 131 +++++++++++++++++++++++++++++++-----
>   include/hw/i2c/aspeed_i2c.h |   4 ++
>   2 files changed, 117 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 9c222a02fe..d3d49340ea 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -941,12 +941,48 @@ static const MemoryRegionOps aspeed_i2c_share_pool_ops = {
>       },
>   };
>   
> +static uint64_t aspeed_i2c_bus_pool_read(void *opaque, hwaddr offset,
> +                                     unsigned size)
> +{
> +    AspeedI2CBus *s = opaque;
> +    uint64_t ret = 0;
> +    int i;
> +
> +    for (i = 0; i < size; i++) {
> +        ret |= (uint64_t) s->pool[offset + i] << (8 * i);
> +    }
> +
> +    return ret;
> +}
> +
> +static void aspeed_i2c_bus_pool_write(void *opaque, hwaddr offset,
> +                                  uint64_t value, unsigned size)
> +{
> +    AspeedI2CBus *s = opaque;
> +    int i;
> +
> +    for (i = 0; i < size; i++) {
> +        s->pool[offset + i] = (value >> (8 * i)) & 0xFF;
> +    }
> +}
> +
> +static const MemoryRegionOps aspeed_i2c_bus_pool_ops = {
> +    .read = aspeed_i2c_bus_pool_read,
> +    .write = aspeed_i2c_bus_pool_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 4,
> +    },
> +};
> +
>   static const VMStateDescription aspeed_i2c_bus_vmstate = {
>       .name = TYPE_ASPEED_I2C,
> -    .version_id = 5,
> -    .minimum_version_id = 5,
> +    .version_id = 6,
> +    .minimum_version_id = 6,
>       .fields = (const VMStateField[]) {
>           VMSTATE_UINT32_ARRAY(regs, AspeedI2CBus, ASPEED_I2C_NEW_NUM_REG),
> +        VMSTATE_UINT8_ARRAY(pool, AspeedI2CBus, ASPEED_I2C_BUS_POOL_SIZE),
>           VMSTATE_END_OF_LIST()
>       }
>   };
> @@ -996,7 +1032,21 @@ static void aspeed_i2c_instance_init(Object *obj)
>    *   0x140 ... 0x17F: Device 5
>    *   0x180 ... 0x1BF: Device 6
>    *   0x1C0 ... 0x1FF: Device 7
> - *   0x200 ... 0x2FF: Buffer Pool (AST2500 unused in linux driver)
> + *   0x200 ... 0x20F: Device 1 buffer (AST2500 unused in linux driver)
> + *   0x210 ... 0x21F: Device 2 buffer
> + *   0x220 ... 0x22F: Device 3 buffer
> + *   0x230 ... 0x23F: Device 4 buffer
> + *   0x240 ... 0x24F: Device 5 buffer
> + *   0x250 ... 0x25F: Device 6 buffer
> + *   0x260 ... 0x26F: Device 7 buffer
> + *   0x270 ... 0x27F: Device 8 buffer
> + *   0x280 ... 0x28F: Device 9 buffer
> + *   0x290 ... 0x29F: Device 10 buffer
> + *   0x2A0 ... 0x2AF: Device 11 buffer
> + *   0x2B0 ... 0x2BF: Device 12 buffer
> + *   0x2C0 ... 0x2CF: Device 13 buffer
> + *   0x2D0 ... 0x2DF: Device 14 buffer
> + *   0x2E0 ... 0x2FF: Reserved
>    *   0x300 ... 0x33F: Device 8
>    *   0x340 ... 0x37F: Device 9
>    *   0x380 ... 0x3BF: Device 10
> @@ -1005,6 +1055,41 @@ static void aspeed_i2c_instance_init(Object *obj)
>    *   0x440 ... 0x47F: Device 13
>    *   0x480 ... 0x4BF: Device 14
>    *   0x800 ... 0xFFF: Buffer Pool (AST2400 unused in linux driver)
> + *
> + * Address Definitions (AST2600 and AST1030)
> + *   0x000 ... 0x07F: Global Register
> + *   0x080 ... 0x0FF: Device 1
> + *   0x100 ... 0x17F: Device 2
> + *   0x180 ... 0x1FF: Device 3
> + *   0x200 ... 0x27F: Device 4
> + *   0x280 ... 0x2FF: Device 5
> + *   0x300 ... 0x37F: Device 6
> + *   0x380 ... 0x3FF: Device 7
> + *   0x400 ... 0x47F: Device 8
> + *   0x480 ... 0x4FF: Device 9
> + *   0x500 ... 0x57F: Device 10
> + *   0x580 ... 0x5FF: Device 11
> + *   0x600 ... 0x67F: Device 12
> + *   0x680 ... 0x6FF: Device 13
> + *   0x700 ... 0x77F: Device 14
> + *   0x780 ... 0x7FF: Device 15 (15 and 16 unused in AST1030)
> + *   0x800 ... 0x87F: Device 16
> + *   0xC00 ... 0xC1F: Device 1 buffer
> + *   0xC20 ... 0xC3F: Device 2 buffer
> + *   0xC40 ... 0xC5F: Device 3 buffer
> + *   0xC60 ... 0xC7F: Device 4 buffer
> + *   0xC80 ... 0xC9F: Device 5 buffer
> + *   0xCA0 ... 0xCBF: Device 6 buffer
> + *   0xCC0 ... 0xCDF: Device 7 buffer
> + *   0xCE0 ... 0xCFF: Device 8 buffer
> + *   0xD00 ... 0xD1F: Device 9 buffer
> + *   0xD20 ... 0xD3F: Device 10 buffer
> + *   0xD40 ... 0xD5F: Device 11 buffer
> + *   0xD60 ... 0xD7F: Device 12 buffer
> + *   0xD80 ... 0xD9F: Device 13 buffer
> + *   0xDA0 ... 0xDBF: Device 14 buffer
> + *   0xDC0 ... 0xDDF: Device 15 buffer (15 and 16 unused in AST1030)
> + *   0xDE0 ... 0xDFF: Device 16 buffer
>    */
>   static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
>   {
> @@ -1039,10 +1124,19 @@ static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
>                                       &s->busses[i].mr);
>       }
>   
> -    memory_region_init_io(&s->pool_iomem, OBJECT(s),
> -                          &aspeed_i2c_share_pool_ops, s,
> -                          "aspeed.i2c-share-pool", aic->pool_size);
> -    memory_region_add_subregion(&s->iomem, aic->pool_base, &s->pool_iomem);
> +    if (aic->has_share_pool) {
> +        memory_region_init_io(&s->pool_iomem, OBJECT(s),
> +                              &aspeed_i2c_share_pool_ops, s,
> +                              "aspeed.i2c-share-pool", aic->pool_size);
> +        memory_region_add_subregion(&s->iomem, aic->pool_base,
> +                                    &s->pool_iomem);
> +    } else {
> +        for (i = 0; i < aic->num_busses; i++) {
> +            memory_region_add_subregion(&s->iomem,
> +                                        aic->pool_base + (aic->pool_size * i),
> +                                        &s->busses[i].mr_pool);
> +        }
> +    }
>   
>       if (aic->has_dma) {
>           if (!s->dram_mr) {
> @@ -1218,6 +1312,7 @@ 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);
> +    g_autofree char *pool_name = g_strdup_printf("%s.pool", name);
>   
>       if (!s->controller) {
>           error_setg(errp, TYPE_ASPEED_I2C_BUS ": 'controller' link not set");
> @@ -1235,6 +1330,10 @@ static void aspeed_i2c_bus_realize(DeviceState *dev, Error **errp)
>       memory_region_init_io(&s->mr, OBJECT(s), &aspeed_i2c_bus_ops,
>                             s, name, aic->reg_size);
>       sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr);
> +
> +    memory_region_init_io(&s->mr_pool, OBJECT(s), &aspeed_i2c_bus_pool_ops,
> +                          s, pool_name, aic->pool_size);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr_pool);
>   }
>   
>   static Property aspeed_i2c_bus_properties[] = {
> @@ -1287,6 +1386,7 @@ static void aspeed_2400_i2c_class_init(ObjectClass *klass, void *data)
>       aic->reg_size = 0x40;
>       aic->gap = 7;
>       aic->bus_get_irq = aspeed_2400_i2c_bus_get_irq;
> +    aic->has_share_pool = true;
>       aic->pool_size = 0x800;
>       aic->pool_base = 0x800;
>       aic->bus_pool_base = aspeed_2400_i2c_bus_pool_base;
> @@ -1306,7 +1406,7 @@ static qemu_irq aspeed_2500_i2c_bus_get_irq(AspeedI2CBus *bus)
>   
>   static uint8_t *aspeed_2500_i2c_bus_pool_base(AspeedI2CBus *bus)
>   {
> -    return &bus->controller->share_pool[bus->id * 0x10];
> +    return bus->pool;
>   }
>   
>   static void aspeed_2500_i2c_class_init(ObjectClass *klass, void *data)
> @@ -1320,7 +1420,7 @@ static void aspeed_2500_i2c_class_init(ObjectClass *klass, void *data)
>       aic->reg_size = 0x40;
>       aic->gap = 7;
>       aic->bus_get_irq = aspeed_2500_i2c_bus_get_irq;
> -    aic->pool_size = 0x100;
> +    aic->pool_size = 0x10;
>       aic->pool_base = 0x200;
>       aic->bus_pool_base = aspeed_2500_i2c_bus_pool_base;
>       aic->check_sram = true;
> @@ -1339,11 +1439,6 @@ static qemu_irq aspeed_2600_i2c_bus_get_irq(AspeedI2CBus *bus)
>       return bus->irq;
>   }
>   
> -static uint8_t *aspeed_2600_i2c_bus_pool_base(AspeedI2CBus *bus)
> -{
> -   return &bus->controller->share_pool[bus->id * 0x20];
> -}
> -
>   static void aspeed_2600_i2c_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -1355,9 +1450,9 @@ static void aspeed_2600_i2c_class_init(ObjectClass *klass, void *data)
>       aic->reg_size = 0x80;
>       aic->gap = -1; /* no gap */
>       aic->bus_get_irq = aspeed_2600_i2c_bus_get_irq;
> -    aic->pool_size = 0x200;
> +    aic->pool_size = 0x20;
>       aic->pool_base = 0xC00;
> -    aic->bus_pool_base = aspeed_2600_i2c_bus_pool_base;
> +    aic->bus_pool_base = aspeed_2500_i2c_bus_pool_base;
>       aic->has_dma = true;
>       aic->mem_size = 0x1000;
>   }
> @@ -1379,9 +1474,9 @@ static void aspeed_1030_i2c_class_init(ObjectClass *klass, void *data)
>       aic->reg_size = 0x80;
>       aic->gap = -1; /* no gap */
>       aic->bus_get_irq = aspeed_2600_i2c_bus_get_irq;
> -    aic->pool_size = 0x200;
> +    aic->pool_size = 0x20;
>       aic->pool_base = 0xC00;
> -    aic->bus_pool_base = aspeed_2600_i2c_bus_pool_base;
> +    aic->bus_pool_base = aspeed_2500_i2c_bus_pool_base;
>       aic->has_dma = true;
>       aic->mem_size = 0x10000;
>   }
> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> index 02ede85906..8e62ec64f8 100644
> --- a/include/hw/i2c/aspeed_i2c.h
> +++ b/include/hw/i2c/aspeed_i2c.h
> @@ -35,6 +35,7 @@ OBJECT_DECLARE_TYPE(AspeedI2CState, AspeedI2CClass, ASPEED_I2C)
>   
>   #define ASPEED_I2C_NR_BUSSES 16
>   #define ASPEED_I2C_SHARE_POOL_SIZE 0x800
> +#define ASPEED_I2C_BUS_POOL_SIZE 0x20
>   #define ASPEED_I2C_OLD_NUM_REG 11
>   #define ASPEED_I2C_NEW_NUM_REG 22
>   
> @@ -239,12 +240,14 @@ struct AspeedI2CBus {
>       I2CSlave *slave;
>   
>       MemoryRegion mr;
> +    MemoryRegion mr_pool;
>   
>       I2CBus *bus;
>       uint8_t id;
>       qemu_irq irq;
>   
>       uint32_t regs[ASPEED_I2C_NEW_NUM_REG];
> +    uint8_t pool[ASPEED_I2C_BUS_POOL_SIZE];
>   };
>   
>   struct AspeedI2CState {
> @@ -284,6 +287,7 @@ struct AspeedI2CClass {
>       uint8_t *(*bus_pool_base)(AspeedI2CBus *);
>       bool check_sram;
>       bool has_dma;
> +    bool has_share_pool;
>       uint64_t mem_size;
>   };
>   

[PATCH v1 07/15] hw/i2c/aspeed: support discontinuous poll buffer memory region of I2C bus

[Jamin Lin via]

It only support continuous pool buffer memory region for all I2C bus.
However, the pool buffer address of all I2c bus are discontinuous
for AST2700.

Ex: the pool buffer address of I2C bus for ast2700 as following.
0x1A0 - 0x1BF: Device 0 buffer
0x2A0 - 0x2BF: Device 1 buffer
0x3A0 - 0x3BF: Device 2 buffer
0x4A0 - 0x4BF: Device 3 buffer
0x5A0 - 0x5BF: Device 4 buffer
0x6A0 - 0x6BF: Device 5 buffer
0x7A0 - 0x7BF: Device 6 buffer
0x8A0 - 0x8BF: Device 7 buffer
0x9A0 - 0x9BF: Device 8 buffer
0xAA0 - 0xABF: Device 9 buffer
0xBA0 - 0xBBF: Device 10 buffer
0xCA0 - 0xCBF: Device 11 buffer
0xDA0 - 0xDBF: Device 12 buffer
0xEA0 - 0xEBF: Device 13 buffer
0xFA0 – 0xFBF: Device 14 buffer
0x10A0 – 0x10BF: Device 15 buffer

Introduce a new class attribute to make user set each I2C bus
pool buffer gap size. Update formula to create all I2C bus
pool buffer memory regions.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/i2c/aspeed_i2c.c         | 3 ++-
 include/hw/i2c/aspeed_i2c.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index d3d49340ea..abcb1d5330 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -1098,6 +1098,7 @@ static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
     AspeedI2CState *s = ASPEED_I2C(dev);
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s);
     uint32_t reg_offset = aic->reg_size + aic->reg_gap_size;
+    uint32_t pool_offset = aic->pool_size + aic->pool_gap_size;
 
     sysbus_init_irq(sbd, &s->irq);
     memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_i2c_ctrl_ops, s,
@@ -1133,7 +1134,7 @@ static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
     } else {
         for (i = 0; i < aic->num_busses; i++) {
             memory_region_add_subregion(&s->iomem,
-                                        aic->pool_base + (aic->pool_size * i),
+                                        aic->pool_base + (pool_offset * i),
                                         &s->busses[i].mr_pool);
         }
     }
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 8e62ec64f8..b42c4dc584 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -284,6 +284,7 @@ struct AspeedI2CClass {
 
     uint64_t pool_size;
     hwaddr pool_base;
+    uint32_t pool_gap_size;
     uint8_t *(*bus_pool_base)(AspeedI2CBus *);
     bool check_sram;
     bool has_dma;
-- 
2.34.1

Re: [PATCH v1 07/15] hw/i2c/aspeed: support discontinuous poll buffer memory region of I2C bus

[Cédric Le Goater]

On 7/18/24 08:49, Jamin Lin wrote:
> It only support continuous pool buffer memory region for all I2C bus.
> However, the pool buffer address of all I2c bus are discontinuous
> for AST2700.
> 
> Ex: the pool buffer address of I2C bus for ast2700 as following.
> 0x1A0 - 0x1BF: Device 0 buffer
> 0x2A0 - 0x2BF: Device 1 buffer
> 0x3A0 - 0x3BF: Device 2 buffer
> 0x4A0 - 0x4BF: Device 3 buffer
> 0x5A0 - 0x5BF: Device 4 buffer
> 0x6A0 - 0x6BF: Device 5 buffer
> 0x7A0 - 0x7BF: Device 6 buffer
> 0x8A0 - 0x8BF: Device 7 buffer
> 0x9A0 - 0x9BF: Device 8 buffer
> 0xAA0 - 0xABF: Device 9 buffer
> 0xBA0 - 0xBBF: Device 10 buffer
> 0xCA0 - 0xCBF: Device 11 buffer
> 0xDA0 - 0xDBF: Device 12 buffer
> 0xEA0 - 0xEBF: Device 13 buffer
> 0xFA0 – 0xFBF: Device 14 buffer
> 0x10A0 – 0x10BF: Device 15 buffer
> 
> Introduce a new class attribute to make user set each I2C bus
> pool buffer gap size. Update formula to create all I2C bus
> pool buffer memory regions.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>

Same comment than on patch 4.


Thanks,

C.


> ---
>   hw/i2c/aspeed_i2c.c         | 3 ++-
>   include/hw/i2c/aspeed_i2c.h | 1 +
>   2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index d3d49340ea..abcb1d5330 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -1098,6 +1098,7 @@ static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
>       AspeedI2CState *s = ASPEED_I2C(dev);
>       AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s);
>       uint32_t reg_offset = aic->reg_size + aic->reg_gap_size;
> +    uint32_t pool_offset = aic->pool_size + aic->pool_gap_size;
>   
>       sysbus_init_irq(sbd, &s->irq);
>       memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_i2c_ctrl_ops, s,
> @@ -1133,7 +1134,7 @@ static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
>       } else {
>           for (i = 0; i < aic->num_busses; i++) {
>               memory_region_add_subregion(&s->iomem,
> -                                        aic->pool_base + (aic->pool_size * i),
> +                                        aic->pool_base + (pool_offset * i),
>                                           &s->busses[i].mr_pool);
>           }
>       }
> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> index 8e62ec64f8..b42c4dc584 100644
> --- a/include/hw/i2c/aspeed_i2c.h
> +++ b/include/hw/i2c/aspeed_i2c.h
> @@ -284,6 +284,7 @@ struct AspeedI2CClass {
>   
>       uint64_t pool_size;
>       hwaddr pool_base;
> +    uint32_t pool_gap_size;
>       uint8_t *(*bus_pool_base)(AspeedI2CBus *);
>       bool check_sram;
>       bool has_dma;

[PATCH v1 08/15] hw/i2c/aspeed: introduce a new dma_dram_offset attribute in AspeedI2Cbus

[Jamin Lin via]

The "Current DMA Operating Address Status(0x50)" register of
I2C new mode has been removed in AST2700.
This register is used for debugging and it is a read only register.

To support AST2700 DMA mode, introduce a new
dma_dram_offset class attribute in AspeedI2Cbus to save the
current DMA operating address.

ASPEED AST2700 SOC is a 64 bits quad core CPUs (Cortex-a35)
And the base address of dram is "0x4 00000000" which
is 64bits address.

Set the dma_dram_offset data type to uint64_t for
64 bits dram address DMA support.

Both "DMA Mode Buffer Address Register(I2CD24 old mode)" and
"DMA Operating Address Status (I2CC50 new mode)" are used for showing the
low part dram offset bits [31:0], so change to read/write both register bits [31:0] in
bus register read/write functions.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/i2c/aspeed_i2c.c         | 50 +++++++++++++++++++++++--------------
 include/hw/i2c/aspeed_i2c.h |  9 +------
 2 files changed, 32 insertions(+), 27 deletions(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index abcb1d5330..c0d3ac3867 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -114,7 +114,10 @@ static uint64_t aspeed_i2c_bus_old_read(AspeedI2CBus *bus, hwaddr offset,
         if (!aic->has_dma) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n",  __func__);
             value = -1;
+            break;
         }
+
+        value = extract64(bus->dma_dram_offset, 0, 32);
         break;
     case A_I2CD_DMA_LEN:
         if (!aic->has_dma) {
@@ -150,9 +153,7 @@ static uint64_t aspeed_i2c_bus_new_read(AspeedI2CBus *bus, hwaddr offset,
     case A_I2CM_DMA_TX_ADDR:
     case A_I2CM_DMA_RX_ADDR:
     case A_I2CM_DMA_LEN_STS:
-    case A_I2CC_DMA_ADDR:
     case A_I2CC_DMA_LEN:
-
     case A_I2CS_DEV_ADDR:
     case A_I2CS_DMA_RX_ADDR:
     case A_I2CS_DMA_LEN:
@@ -161,6 +162,9 @@ static uint64_t aspeed_i2c_bus_new_read(AspeedI2CBus *bus, hwaddr offset,
     case A_I2CS_DMA_LEN_STS:
         /* Value is already set, don't do anything. */
         break;
+    case A_I2CC_DMA_ADDR:
+        value = extract64(bus->dma_dram_offset, 0, 32);
+        break;
     case A_I2CS_INTR_STS:
         break;
     case A_I2CM_CMD:
@@ -210,18 +214,18 @@ static int aspeed_i2c_dma_read(AspeedI2CBus *bus, uint8_t *data)
 {
     MemTxResult result;
     AspeedI2CState *s = bus->controller;
-    uint32_t reg_dma_addr = aspeed_i2c_bus_dma_addr_offset(bus);
     uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
 
-    result = address_space_read(&s->dram_as, bus->regs[reg_dma_addr],
+    result = address_space_read(&s->dram_as, bus->dma_dram_offset,
                                 MEMTXATTRS_UNSPECIFIED, data, 1);
     if (result != MEMTX_OK) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM read failed @%08x\n",
-                      __func__, bus->regs[reg_dma_addr]);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: DRAM read failed @%" PRIx64 "\n",
+                      __func__, bus->dma_dram_offset);
         return -1;
     }
 
-    bus->regs[reg_dma_addr]++;
+    bus->dma_dram_offset++;
     bus->regs[reg_dma_len]--;
     return 0;
 }
@@ -291,7 +295,6 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
     uint32_t reg_pool_ctrl = aspeed_i2c_bus_pool_ctrl_offset(bus);
     uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
     uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
-    uint32_t reg_dma_addr = aspeed_i2c_bus_dma_addr_offset(bus);
     int pool_rx_count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl,
                                                 RX_SIZE) + 1;
 
@@ -323,14 +326,17 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
             data = i2c_recv(bus->bus);
             trace_aspeed_i2c_bus_recv("DMA", bus->regs[reg_dma_len],
                                       bus->regs[reg_dma_len], data);
-            result = address_space_write(&s->dram_as, bus->regs[reg_dma_addr],
+
+            result = address_space_write(&s->dram_as, bus->dma_dram_offset,
                                          MEMTXATTRS_UNSPECIFIED, &data, 1);
             if (result != MEMTX_OK) {
-                qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM write failed @%08x\n",
-                              __func__, bus->regs[reg_dma_addr]);
+                qemu_log_mask(LOG_GUEST_ERROR,
+                              "%s: DRAM write failed @%" PRIx64 "\n",
+                              __func__, bus->dma_dram_offset);
                 return;
             }
-            bus->regs[reg_dma_addr]++;
+
+            bus->dma_dram_offset++;
             bus->regs[reg_dma_len]--;
             /* In new mode, keep track of how many bytes we RXed */
             if (aspeed_i2c_is_new_mode(bus->controller)) {
@@ -636,14 +642,18 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
     case A_I2CM_DMA_TX_ADDR:
         bus->regs[R_I2CM_DMA_TX_ADDR] = FIELD_EX32(value, I2CM_DMA_TX_ADDR,
                                                    ADDR);
-        bus->regs[R_I2CC_DMA_ADDR] = FIELD_EX32(value, I2CM_DMA_TX_ADDR, ADDR);
+        bus->dma_dram_offset =
+            deposit64(bus->dma_dram_offset, 0, 32,
+                      FIELD_EX32(value, I2CM_DMA_TX_ADDR, ADDR));
         bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN,
                                                      TX_BUF_LEN) + 1;
         break;
     case A_I2CM_DMA_RX_ADDR:
         bus->regs[R_I2CM_DMA_RX_ADDR] = FIELD_EX32(value, I2CM_DMA_RX_ADDR,
                                                    ADDR);
-        bus->regs[R_I2CC_DMA_ADDR] = FIELD_EX32(value, I2CM_DMA_RX_ADDR, ADDR);
+        bus->dma_dram_offset =
+            deposit64(bus->dma_dram_offset, 0, 32,
+                      FIELD_EX32(value, I2CM_DMA_RX_ADDR, ADDR));
         bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN,
                                                      RX_BUF_LEN) + 1;
         break;
@@ -811,7 +821,8 @@ static void aspeed_i2c_bus_old_write(AspeedI2CBus *bus, hwaddr offset,
             break;
         }
 
-        bus->regs[R_I2CD_DMA_ADDR] = value & 0x3ffffffc;
+        bus->dma_dram_offset = deposit64(bus->dma_dram_offset, 0, 32,
+                                         value & 0x3ffffffc);
         break;
 
     case A_I2CD_DMA_LEN:
@@ -1188,8 +1199,9 @@ static int aspeed_i2c_bus_new_slave_event(AspeedI2CBus *bus,
             return -1;
         }
         ARRAY_FIELD_DP32(bus->regs, I2CS_DMA_LEN_STS, RX_LEN, 0);
-        bus->regs[R_I2CC_DMA_ADDR] =
-            ARRAY_FIELD_EX32(bus->regs, I2CS_DMA_RX_ADDR, ADDR);
+        bus->dma_dram_offset =
+            deposit64(bus->dma_dram_offset, 0, 32,
+                      ARRAY_FIELD_EX32(bus->regs, I2CS_DMA_RX_ADDR, ADDR));
         bus->regs[R_I2CC_DMA_LEN] =
             ARRAY_FIELD_EX32(bus->regs, I2CS_DMA_LEN, RX_BUF_LEN) + 1;
         i2c_ack(bus->bus);
@@ -1255,10 +1267,10 @@ static int aspeed_i2c_bus_slave_event(I2CSlave *slave, enum i2c_event event)
 static void aspeed_i2c_bus_new_slave_send_async(AspeedI2CBus *bus, uint8_t data)
 {
     assert(address_space_write(&bus->controller->dram_as,
-                               bus->regs[R_I2CC_DMA_ADDR],
+                               bus->dma_dram_offset,
                                MEMTXATTRS_UNSPECIFIED, &data, 1) == MEMTX_OK);
 
-    bus->regs[R_I2CC_DMA_ADDR]++;
+    bus->dma_dram_offset++;
     bus->regs[R_I2CC_DMA_LEN]--;
     ARRAY_FIELD_DP32(bus->regs, I2CS_DMA_LEN_STS, RX_LEN,
                      ARRAY_FIELD_EX32(bus->regs, I2CS_DMA_LEN_STS, RX_LEN) + 1);
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index b42c4dc584..bdaea3207d 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -248,6 +248,7 @@ struct AspeedI2CBus {
 
     uint32_t regs[ASPEED_I2C_NEW_NUM_REG];
     uint8_t pool[ASPEED_I2C_BUS_POOL_SIZE];
+    uint64_t dma_dram_offset;
 };
 
 struct AspeedI2CState {
@@ -369,14 +370,6 @@ static inline uint32_t aspeed_i2c_bus_dma_len_offset(AspeedI2CBus *bus)
     return R_I2CD_DMA_LEN;
 }
 
-static inline uint32_t aspeed_i2c_bus_dma_addr_offset(AspeedI2CBus *bus)
-{
-    if (aspeed_i2c_is_new_mode(bus->controller)) {
-        return R_I2CC_DMA_ADDR;
-    }
-    return R_I2CD_DMA_ADDR;
-}
-
 static inline bool aspeed_i2c_bus_is_master(AspeedI2CBus *bus)
 {
     return SHARED_ARRAY_FIELD_EX32(bus->regs, aspeed_i2c_bus_ctrl_offset(bus),
-- 
2.34.1

Re: [PATCH v1 08/15] hw/i2c/aspeed: introduce a new dma_dram_offset attribute in AspeedI2Cbus

[Cédric Le Goater]

On 7/18/24 08:49, Jamin Lin wrote:
> The "Current DMA Operating Address Status(0x50)" register of
> I2C new mode has been removed in AST2700.
> This register is used for debugging and it is a read only register.
> 
> To support AST2700 DMA mode, introduce a new
> dma_dram_offset class attribute in AspeedI2Cbus to save the
> current DMA operating address.
> 
> ASPEED AST2700 SOC is a 64 bits quad core CPUs (Cortex-a35)
> And the base address of dram is "0x4 00000000" which
> is 64bits address.
> 
> Set the dma_dram_offset data type to uint64_t for
> 64 bits dram address DMA support.
> 
> Both "DMA Mode Buffer Address Register(I2CD24 old mode)" and
> "DMA Operating Address Status (I2CC50 new mode)" are used for showing the
> low part dram offset bits [31:0], so change to read/write both register bits [31:0] in
> bus register read/write functions.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/i2c/aspeed_i2c.c         | 50 +++++++++++++++++++++++--------------
>   include/hw/i2c/aspeed_i2c.h |  9 +------
>   2 files changed, 32 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index abcb1d5330..c0d3ac3867 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -114,7 +114,10 @@ static uint64_t aspeed_i2c_bus_old_read(AspeedI2CBus *bus, hwaddr offset,
>           if (!aic->has_dma) {
>               qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n",  __func__);
>               value = -1;
> +            break;
>           }
> +
> +        value = extract64(bus->dma_dram_offset, 0, 32);
>           break;
>       case A_I2CD_DMA_LEN:
>           if (!aic->has_dma) {
> @@ -150,9 +153,7 @@ static uint64_t aspeed_i2c_bus_new_read(AspeedI2CBus *bus, hwaddr offset,
>       case A_I2CM_DMA_TX_ADDR:
>       case A_I2CM_DMA_RX_ADDR:
>       case A_I2CM_DMA_LEN_STS:
> -    case A_I2CC_DMA_ADDR:
>       case A_I2CC_DMA_LEN:
> -
>       case A_I2CS_DEV_ADDR:
>       case A_I2CS_DMA_RX_ADDR:
>       case A_I2CS_DMA_LEN:
> @@ -161,6 +162,9 @@ static uint64_t aspeed_i2c_bus_new_read(AspeedI2CBus *bus, hwaddr offset,
>       case A_I2CS_DMA_LEN_STS:
>           /* Value is already set, don't do anything. */
>           break;
> +    case A_I2CC_DMA_ADDR:
> +        value = extract64(bus->dma_dram_offset, 0, 32);
> +        break;
>       case A_I2CS_INTR_STS:
>           break;
>       case A_I2CM_CMD:
> @@ -210,18 +214,18 @@ static int aspeed_i2c_dma_read(AspeedI2CBus *bus, uint8_t *data)
>   {
>       MemTxResult result;
>       AspeedI2CState *s = bus->controller;
> -    uint32_t reg_dma_addr = aspeed_i2c_bus_dma_addr_offset(bus);
>       uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
>   
> -    result = address_space_read(&s->dram_as, bus->regs[reg_dma_addr],
> +    result = address_space_read(&s->dram_as, bus->dma_dram_offset,
>                                   MEMTXATTRS_UNSPECIFIED, data, 1);
>       if (result != MEMTX_OK) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM read failed @%08x\n",
> -                      __func__, bus->regs[reg_dma_addr]);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: DRAM read failed @%" PRIx64 "\n",
> +                      __func__, bus->dma_dram_offset);
>           return -1;
>       }
>   
> -    bus->regs[reg_dma_addr]++;
> +    bus->dma_dram_offset++;
>       bus->regs[reg_dma_len]--;
>       return 0;
>   }
> @@ -291,7 +295,6 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
>       uint32_t reg_pool_ctrl = aspeed_i2c_bus_pool_ctrl_offset(bus);
>       uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
>       uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
> -    uint32_t reg_dma_addr = aspeed_i2c_bus_dma_addr_offset(bus);
>       int pool_rx_count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl,
>                                                   RX_SIZE) + 1;
>   
> @@ -323,14 +326,17 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
>               data = i2c_recv(bus->bus);
>               trace_aspeed_i2c_bus_recv("DMA", bus->regs[reg_dma_len],
>                                         bus->regs[reg_dma_len], data);
> -            result = address_space_write(&s->dram_as, bus->regs[reg_dma_addr],
> +
> +            result = address_space_write(&s->dram_as, bus->dma_dram_offset,
>                                            MEMTXATTRS_UNSPECIFIED, &data, 1);
>               if (result != MEMTX_OK) {
> -                qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM write failed @%08x\n",
> -                              __func__, bus->regs[reg_dma_addr]);
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                              "%s: DRAM write failed @%" PRIx64 "\n",
> +                              __func__, bus->dma_dram_offset);
>                   return;
>               }
> -            bus->regs[reg_dma_addr]++;
> +
> +            bus->dma_dram_offset++;
>               bus->regs[reg_dma_len]--;
>               /* In new mode, keep track of how many bytes we RXed */
>               if (aspeed_i2c_is_new_mode(bus->controller)) {
> @@ -636,14 +642,18 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
>       case A_I2CM_DMA_TX_ADDR:
>           bus->regs[R_I2CM_DMA_TX_ADDR] = FIELD_EX32(value, I2CM_DMA_TX_ADDR,
>                                                      ADDR);
> -        bus->regs[R_I2CC_DMA_ADDR] = FIELD_EX32(value, I2CM_DMA_TX_ADDR, ADDR);
> +        bus->dma_dram_offset =
> +            deposit64(bus->dma_dram_offset, 0, 32,
> +                      FIELD_EX32(value, I2CM_DMA_TX_ADDR, ADDR));
>           bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN,
>                                                        TX_BUF_LEN) + 1;
>           break;
>       case A_I2CM_DMA_RX_ADDR:
>           bus->regs[R_I2CM_DMA_RX_ADDR] = FIELD_EX32(value, I2CM_DMA_RX_ADDR,
>                                                      ADDR);
> -        bus->regs[R_I2CC_DMA_ADDR] = FIELD_EX32(value, I2CM_DMA_RX_ADDR, ADDR);
> +        bus->dma_dram_offset =
> +            deposit64(bus->dma_dram_offset, 0, 32,
> +                      FIELD_EX32(value, I2CM_DMA_RX_ADDR, ADDR));
>           bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN,
>                                                        RX_BUF_LEN) + 1;
>           break;
> @@ -811,7 +821,8 @@ static void aspeed_i2c_bus_old_write(AspeedI2CBus *bus, hwaddr offset,
>               break;
>           }
>   
> -        bus->regs[R_I2CD_DMA_ADDR] = value & 0x3ffffffc;
> +        bus->dma_dram_offset = deposit64(bus->dma_dram_offset, 0, 32,
> +                                         value & 0x3ffffffc);
>           break;
>   
>       case A_I2CD_DMA_LEN:
> @@ -1188,8 +1199,9 @@ static int aspeed_i2c_bus_new_slave_event(AspeedI2CBus *bus,
>               return -1;
>           }
>           ARRAY_FIELD_DP32(bus->regs, I2CS_DMA_LEN_STS, RX_LEN, 0);
> -        bus->regs[R_I2CC_DMA_ADDR] =
> -            ARRAY_FIELD_EX32(bus->regs, I2CS_DMA_RX_ADDR, ADDR);
> +        bus->dma_dram_offset =
> +            deposit64(bus->dma_dram_offset, 0, 32,
> +                      ARRAY_FIELD_EX32(bus->regs, I2CS_DMA_RX_ADDR, ADDR));
>           bus->regs[R_I2CC_DMA_LEN] =
>               ARRAY_FIELD_EX32(bus->regs, I2CS_DMA_LEN, RX_BUF_LEN) + 1;
>           i2c_ack(bus->bus);
> @@ -1255,10 +1267,10 @@ static int aspeed_i2c_bus_slave_event(I2CSlave *slave, enum i2c_event event)
>   static void aspeed_i2c_bus_new_slave_send_async(AspeedI2CBus *bus, uint8_t data)
>   {
>       assert(address_space_write(&bus->controller->dram_as,
> -                               bus->regs[R_I2CC_DMA_ADDR],
> +                               bus->dma_dram_offset,
>                                  MEMTXATTRS_UNSPECIFIED, &data, 1) == MEMTX_OK);
>   
> -    bus->regs[R_I2CC_DMA_ADDR]++;
> +    bus->dma_dram_offset++;
>       bus->regs[R_I2CC_DMA_LEN]--;
>       ARRAY_FIELD_DP32(bus->regs, I2CS_DMA_LEN_STS, RX_LEN,
>                        ARRAY_FIELD_EX32(bus->regs, I2CS_DMA_LEN_STS, RX_LEN) + 1);
> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> index b42c4dc584..bdaea3207d 100644
> --- a/include/hw/i2c/aspeed_i2c.h
> +++ b/include/hw/i2c/aspeed_i2c.h
> @@ -248,6 +248,7 @@ struct AspeedI2CBus {
>   
>       uint32_t regs[ASPEED_I2C_NEW_NUM_REG];
>       uint8_t pool[ASPEED_I2C_BUS_POOL_SIZE];
> +    uint64_t dma_dram_offset;


aspeed_i2c_bus_vmstate needs an update. No need to increment again the
vmstate version if all patches are merged in the same release cycle.

Thanks,

C.

>   };
>   
>   struct AspeedI2CState {
> @@ -369,14 +370,6 @@ static inline uint32_t aspeed_i2c_bus_dma_len_offset(AspeedI2CBus *bus)
>       return R_I2CD_DMA_LEN;
>   }
>   
> -static inline uint32_t aspeed_i2c_bus_dma_addr_offset(AspeedI2CBus *bus)
> -{
> -    if (aspeed_i2c_is_new_mode(bus->controller)) {
> -        return R_I2CC_DMA_ADDR;
> -    }
> -    return R_I2CD_DMA_ADDR;
> -}
> -
>   static inline bool aspeed_i2c_bus_is_master(AspeedI2CBus *bus)
>   {
>       return SHARED_ARRAY_FIELD_EX32(bus->regs, aspeed_i2c_bus_ctrl_offset(bus),

RE: [PATCH v1 08/15] hw/i2c/aspeed: introduce a new dma_dram_offset attribute in AspeedI2Cbus

[Jamin Lin]

Hi Cedric,

> Subject: Re: [PATCH v1 08/15] hw/i2c/aspeed: introduce a new
> dma_dram_offset attribute in AspeedI2Cbus
>
> On 7/18/24 08:49, Jamin Lin wrote:
> > The "Current DMA Operating Address Status(0x50)" register of I2C new
> > mode has been removed in AST2700.
> > This register is used for debugging and it is a read only register.
> >
> > To support AST2700 DMA mode, introduce a new dma_dram_offset class
> > attribute in AspeedI2Cbus to save the current DMA operating address.
> >
> > ASPEED AST2700 SOC is a 64 bits quad core CPUs (Cortex-a35) And the
> > base address of dram is "0x4 00000000" which is 64bits address.
> >
> > Set the dma_dram_offset data type to uint64_t for
> > 64 bits dram address DMA support.
> >
> > Both "DMA Mode Buffer Address Register(I2CD24 old mode)" and "DMA
> > Operating Address Status (I2CC50 new mode)" are used for showing the
> > low part dram offset bits [31:0], so change to read/write both
> > register bits [31:0] in bus register read/write functions.
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> >   hw/i2c/aspeed_i2c.c         | 50
> +++++++++++++++++++++++--------------
> >   include/hw/i2c/aspeed_i2c.h |  9 +------
> >   2 files changed, 32 insertions(+), 27 deletions(-)
> >
> > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index
> > abcb1d5330..c0d3ac3867 100644
> > --- a/hw/i2c/aspeed_i2c.c
> > +++ b/hw/i2c/aspeed_i2c.c
> > @@ -114,7 +114,10 @@ static uint64_t
> aspeed_i2c_bus_old_read(AspeedI2CBus *bus, hwaddr offset,
> >           if (!aic->has_dma) {
> >               qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA
> support\n",  __func__);
> >               value = -1;
> > +            break;
> >           }
> > +
> > +        value = extract64(bus->dma_dram_offset, 0, 32);
> >           break;
> >       case A_I2CD_DMA_LEN:
> >           if (!aic->has_dma) {
> > @@ -150,9 +153,7 @@ static uint64_t
> aspeed_i2c_bus_new_read(AspeedI2CBus *bus, hwaddr offset,
> >       case A_I2CM_DMA_TX_ADDR:
> >       case A_I2CM_DMA_RX_ADDR:
> >       case A_I2CM_DMA_LEN_STS:
> > -    case A_I2CC_DMA_ADDR:
> >       case A_I2CC_DMA_LEN:
> > -
> >       case A_I2CS_DEV_ADDR:
> >       case A_I2CS_DMA_RX_ADDR:
> >       case A_I2CS_DMA_LEN:
> > @@ -161,6 +162,9 @@ static uint64_t
> aspeed_i2c_bus_new_read(AspeedI2CBus *bus, hwaddr offset,
> >       case A_I2CS_DMA_LEN_STS:
> >           /* Value is already set, don't do anything. */
> >           break;
> > +    case A_I2CC_DMA_ADDR:
> > +        value = extract64(bus->dma_dram_offset, 0, 32);
> > +        break;
> >       case A_I2CS_INTR_STS:
> >           break;
> >       case A_I2CM_CMD:
> > @@ -210,18 +214,18 @@ static int aspeed_i2c_dma_read(AspeedI2CBus
> *bus, uint8_t *data)
> >   {
> >       MemTxResult result;
> >       AspeedI2CState *s = bus->controller;
> > -    uint32_t reg_dma_addr = aspeed_i2c_bus_dma_addr_offset(bus);
> >       uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
> >
> > -    result = address_space_read(&s->dram_as, bus->regs[reg_dma_addr],
> > +    result = address_space_read(&s->dram_as, bus->dma_dram_offset,
> >                                   MEMTXATTRS_UNSPECIFIED, data,
> 1);
> >       if (result != MEMTX_OK) {
> > -        qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM read failed
> @%08x\n",
> > -                      __func__, bus->regs[reg_dma_addr]);
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: DRAM read failed @%" PRIx64 "\n",
> > +                      __func__, bus->dma_dram_offset);
> >           return -1;
> >       }
> >
> > -    bus->regs[reg_dma_addr]++;
> > +    bus->dma_dram_offset++;
> >       bus->regs[reg_dma_len]--;
> >       return 0;
> >   }
> > @@ -291,7 +295,6 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
> >       uint32_t reg_pool_ctrl = aspeed_i2c_bus_pool_ctrl_offset(bus);
> >       uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
> >       uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
> > -    uint32_t reg_dma_addr = aspeed_i2c_bus_dma_addr_offset(bus);
> >       int pool_rx_count = SHARED_ARRAY_FIELD_EX32(bus->regs,
> reg_pool_ctrl,
> >                                                   RX_SIZE) + 1;
> >
> > @@ -323,14 +326,17 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus
> *bus)
> >               data = i2c_recv(bus->bus);
> >               trace_aspeed_i2c_bus_recv("DMA",
> bus->regs[reg_dma_len],
> >                                         bus->regs[reg_dma_len],
> data);
> > -            result = address_space_write(&s->dram_as,
> bus->regs[reg_dma_addr],
> > +
> > +            result = address_space_write(&s->dram_as,
> > + bus->dma_dram_offset,
> >
> MEMTXATTRS_UNSPECIFIED, &data, 1);
> >               if (result != MEMTX_OK) {
> > -                qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM write
> failed @%08x\n",
> > -                              __func__, bus->regs[reg_dma_addr]);
> > +                qemu_log_mask(LOG_GUEST_ERROR,
> > +                              "%s: DRAM write failed @%" PRIx64
> "\n",
> > +                              __func__, bus->dma_dram_offset);
> >                   return;
> >               }
> > -            bus->regs[reg_dma_addr]++;
> > +
> > +            bus->dma_dram_offset++;
> >               bus->regs[reg_dma_len]--;
> >               /* In new mode, keep track of how many bytes we RXed */
> >               if (aspeed_i2c_is_new_mode(bus->controller)) { @@
> > -636,14 +642,18 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus
> *bus, hwaddr offset,
> >       case A_I2CM_DMA_TX_ADDR:
> >           bus->regs[R_I2CM_DMA_TX_ADDR] = FIELD_EX32(value,
> I2CM_DMA_TX_ADDR,
> >                                                      ADDR);
> > -        bus->regs[R_I2CC_DMA_ADDR] = FIELD_EX32(value,
> I2CM_DMA_TX_ADDR, ADDR);
> > +        bus->dma_dram_offset =
> > +            deposit64(bus->dma_dram_offset, 0, 32,
> > +                      FIELD_EX32(value, I2CM_DMA_TX_ADDR,
> ADDR));
> >           bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs,
> I2CM_DMA_LEN,
> >
> TX_BUF_LEN) + 1;
> >           break;
> >       case A_I2CM_DMA_RX_ADDR:
> >           bus->regs[R_I2CM_DMA_RX_ADDR] = FIELD_EX32(value,
> I2CM_DMA_RX_ADDR,
> >                                                      ADDR);
> > -        bus->regs[R_I2CC_DMA_ADDR] = FIELD_EX32(value,
> I2CM_DMA_RX_ADDR, ADDR);
> > +        bus->dma_dram_offset =
> > +            deposit64(bus->dma_dram_offset, 0, 32,
> > +                      FIELD_EX32(value, I2CM_DMA_RX_ADDR,
> ADDR));
> >           bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs,
> I2CM_DMA_LEN,
> >
> RX_BUF_LEN) + 1;
> >           break;
> > @@ -811,7 +821,8 @@ static void aspeed_i2c_bus_old_write(AspeedI2CBus
> *bus, hwaddr offset,
> >               break;
> >           }
> >
> > -        bus->regs[R_I2CD_DMA_ADDR] = value & 0x3ffffffc;
> > +        bus->dma_dram_offset = deposit64(bus->dma_dram_offset, 0,
> 32,
> > +                                         value & 0x3ffffffc);
> >           break;
> >
> >       case A_I2CD_DMA_LEN:
> > @@ -1188,8 +1199,9 @@ static int
> aspeed_i2c_bus_new_slave_event(AspeedI2CBus *bus,
> >               return -1;
> >           }
> >           ARRAY_FIELD_DP32(bus->regs, I2CS_DMA_LEN_STS, RX_LEN,
> 0);
> > -        bus->regs[R_I2CC_DMA_ADDR] =
> > -            ARRAY_FIELD_EX32(bus->regs, I2CS_DMA_RX_ADDR, ADDR);
> > +        bus->dma_dram_offset =
> > +            deposit64(bus->dma_dram_offset, 0, 32,
> > +                      ARRAY_FIELD_EX32(bus->regs,
> I2CS_DMA_RX_ADDR,
> > + ADDR));
> >           bus->regs[R_I2CC_DMA_LEN] =
> >               ARRAY_FIELD_EX32(bus->regs, I2CS_DMA_LEN,
> RX_BUF_LEN) + 1;
> >           i2c_ack(bus->bus);
> > @@ -1255,10 +1267,10 @@ static int aspeed_i2c_bus_slave_event(I2CSlave
> *slave, enum i2c_event event)
> >   static void aspeed_i2c_bus_new_slave_send_async(AspeedI2CBus *bus,
> uint8_t data)
> >   {
> >       assert(address_space_write(&bus->controller->dram_as,
> > -                               bus->regs[R_I2CC_DMA_ADDR],
> > +                               bus->dma_dram_offset,
> >                                  MEMTXATTRS_UNSPECIFIED, &data,
> 1) ==
> > MEMTX_OK);
> >
> > -    bus->regs[R_I2CC_DMA_ADDR]++;
> > +    bus->dma_dram_offset++;
> >       bus->regs[R_I2CC_DMA_LEN]--;
> >       ARRAY_FIELD_DP32(bus->regs, I2CS_DMA_LEN_STS, RX_LEN,
> >                        ARRAY_FIELD_EX32(bus->regs,
> I2CS_DMA_LEN_STS,
> > RX_LEN) + 1); diff --git a/include/hw/i2c/aspeed_i2c.h
> > b/include/hw/i2c/aspeed_i2c.h index b42c4dc584..bdaea3207d 100644
> > --- a/include/hw/i2c/aspeed_i2c.h
> > +++ b/include/hw/i2c/aspeed_i2c.h
> > @@ -248,6 +248,7 @@ struct AspeedI2CBus {
> >
> >       uint32_t regs[ASPEED_I2C_NEW_NUM_REG];
> >       uint8_t pool[ASPEED_I2C_BUS_POOL_SIZE];
> > +    uint64_t dma_dram_offset;
>
>
> aspeed_i2c_bus_vmstate needs an update. No need to increment again the
> vmstate version if all patches are merged in the same release cycle.
>

Thanks for review.
Will update
Jamin


> Thanks,
>
> C.
>
> >   };
> >
> >   struct AspeedI2CState {
> > @@ -369,14 +370,6 @@ static inline uint32_t
> aspeed_i2c_bus_dma_len_offset(AspeedI2CBus *bus)
> >       return R_I2CD_DMA_LEN;
> >   }
> >
> > -static inline uint32_t aspeed_i2c_bus_dma_addr_offset(AspeedI2CBus
> > *bus) -{
> > -    if (aspeed_i2c_is_new_mode(bus->controller)) {
> > -        return R_I2CC_DMA_ADDR;
> > -    }
> > -    return R_I2CD_DMA_ADDR;
> > -}
> > -
> >   static inline bool aspeed_i2c_bus_is_master(AspeedI2CBus *bus)
> >   {
> >       return SHARED_ARRAY_FIELD_EX32(bus->regs,
> > aspeed_i2c_bus_ctrl_offset(bus),

************* Email Confidentiality Notice ********************
免責聲明:
本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!

DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.

[PATCH v1 09/15] hw/i2c/aspeed: Add AST2700 support

[Jamin Lin via]

Introduce a new ast2700 class to support AST2700.
The I2C bus register memory regions and
I2C bus pool buffer memory regions are discontinuous
and they do not back compatible AST2600.

Add a new ast2700 i2c class init function to match the
address of I2C bus register and pool buffer from the datasheet.

An I2C controller registers owns 8KB address space.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/i2c/aspeed_i2c.c         | 62 +++++++++++++++++++++++++++++++++++++
 include/hw/i2c/aspeed_i2c.h |  1 +
 2 files changed, 63 insertions(+)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index c0d3ac3867..29d400ac93 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -1101,6 +1101,41 @@ static void aspeed_i2c_instance_init(Object *obj)
  *   0xDA0 ... 0xDBF: Device 14 buffer
  *   0xDC0 ... 0xDDF: Device 15 buffer (15 and 16 unused in AST1030)
  *   0xDE0 ... 0xDFF: Device 16 buffer
+ *
+ * Address Definitions (AST2700)
+ *   0x000 ... 0x0FF: Global Register
+ *   0x100 ... 0x17F: Device 0
+ *   0x1A0 ... 0x1BF: Device 0 buffer
+ *   0x200 ... 0x27F: Device 1
+ *   0x2A0 ... 0x2BF: Device 1 buffer
+ *   0x300 ... 0x37F: Device 2
+ *   0x3A0 ... 0x3BF: Device 2 buffer
+ *   0x400 ... 0x47F: Device 3
+ *   0x4A0 ... 0x4BF: Device 3 buffer
+ *   0x500 ... 0x57F: Device 4
+ *   0x5A0 ... 0x5BF: Device 4 buffer
+ *   0x600 ... 0x67F: Device 5
+ *   0x6A0 ... 0x6BF: Device 5 buffer
+ *   0x700 ... 0x77F: Device 6
+ *   0x7A0 ... 0x7BF: Device 6 buffer
+ *   0x800 ... 0x87F: Device 7
+ *   0x8A0 ... 0x8BF: Device 7 buffer
+ *   0x900 ... 0x97F: Device 8
+ *   0x9A0 ... 0x9BF: Device 8 buffer
+ *   0xA00 ... 0xA7F: Device 9
+ *   0xAA0 ... 0xABF: Device 9 buffer
+ *   0xB00 ... 0xB7F: Device 10
+ *   0xBA0 ... 0xBBF: Device 10 buffer
+ *   0xC00 ... 0xC7F: Device 11
+ *   0xCA0 ... 0xCBF: Device 11 buffer
+ *   0xD00 ... 0xD7F: Device 12
+ *   0xDA0 ... 0xDBF: Device 12 buffer
+ *   0xE00 ... 0xE7F: Device 13
+ *   0xEA0 ... 0xEBF: Device 13 buffer
+ *   0xF00 ... 0xF7F: Device 14
+ *   0xFA0 ... 0xFBF: Device 14 buffer
+ *   0x1000 ... 0x107F: Device 15
+ *   0x10A0 ... 0x10BF: Device 15 buffer
  */
 static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
 {
@@ -1500,6 +1535,32 @@ static const TypeInfo aspeed_1030_i2c_info = {
     .class_init = aspeed_1030_i2c_class_init,
 };
 
+static void aspeed_2700_i2c_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedI2CClass *aic = ASPEED_I2C_CLASS(klass);
+
+    dc->desc = "ASPEED 2700 I2C Controller";
+
+    aic->num_busses = 16;
+    aic->reg_size = 0x80;
+    aic->reg_gap_size = 0x80;
+    aic->gap = -1; /* no gap */
+    aic->bus_get_irq = aspeed_2600_i2c_bus_get_irq;
+    aic->pool_size = 0x20;
+    aic->pool_gap_size = 0xe0;
+    aic->pool_base = 0x1a0;
+    aic->bus_pool_base = aspeed_2500_i2c_bus_pool_base;
+    aic->has_dma = true;
+    aic->mem_size = 0x10000;
+}
+
+static const TypeInfo aspeed_2700_i2c_info = {
+    .name = TYPE_ASPEED_2700_I2C,
+    .parent = TYPE_ASPEED_I2C,
+    .class_init = aspeed_2700_i2c_class_init,
+};
+
 static void aspeed_i2c_register_types(void)
 {
     type_register_static(&aspeed_i2c_bus_info);
@@ -1509,6 +1570,7 @@ static void aspeed_i2c_register_types(void)
     type_register_static(&aspeed_2500_i2c_info);
     type_register_static(&aspeed_2600_i2c_info);
     type_register_static(&aspeed_1030_i2c_info);
+    type_register_static(&aspeed_2700_i2c_info);
 }
 
 type_init(aspeed_i2c_register_types)
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index bdaea3207d..4f23dc10c3 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -31,6 +31,7 @@
 #define TYPE_ASPEED_2500_I2C TYPE_ASPEED_I2C "-ast2500"
 #define TYPE_ASPEED_2600_I2C TYPE_ASPEED_I2C "-ast2600"
 #define TYPE_ASPEED_1030_I2C TYPE_ASPEED_I2C "-ast1030"
+#define TYPE_ASPEED_2700_I2C TYPE_ASPEED_I2C "-ast2700"
 OBJECT_DECLARE_TYPE(AspeedI2CState, AspeedI2CClass, ASPEED_I2C)
 
 #define ASPEED_I2C_NR_BUSSES 16
-- 
2.34.1

Re: [PATCH v1 09/15] hw/i2c/aspeed: Add AST2700 support

[Cédric Le Goater]

On 7/18/24 08:49, Jamin Lin wrote:
> Introduce a new ast2700 class to support AST2700.
> The I2C bus register memory regions and
> I2C bus pool buffer memory regions are discontinuous
> and they do not back compatible AST2600.
> 
> Add a new ast2700 i2c class init function to match the
> address of I2C bus register and pool buffer from the datasheet.
> 
> An I2C controller registers owns 8KB address space.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/i2c/aspeed_i2c.c         | 62 +++++++++++++++++++++++++++++++++++++
>   include/hw/i2c/aspeed_i2c.h |  1 +
>   2 files changed, 63 insertions(+)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index c0d3ac3867..29d400ac93 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -1101,6 +1101,41 @@ static void aspeed_i2c_instance_init(Object *obj)
>    *   0xDA0 ... 0xDBF: Device 14 buffer
>    *   0xDC0 ... 0xDDF: Device 15 buffer (15 and 16 unused in AST1030)
>    *   0xDE0 ... 0xDFF: Device 16 buffer
> + *
> + * Address Definitions (AST2700)
> + *   0x000 ... 0x0FF: Global Register
> + *   0x100 ... 0x17F: Device 0
> + *   0x1A0 ... 0x1BF: Device 0 buffer
> + *   0x200 ... 0x27F: Device 1
> + *   0x2A0 ... 0x2BF: Device 1 buffer
> + *   0x300 ... 0x37F: Device 2
> + *   0x3A0 ... 0x3BF: Device 2 buffer
> + *   0x400 ... 0x47F: Device 3
> + *   0x4A0 ... 0x4BF: Device 3 buffer
> + *   0x500 ... 0x57F: Device 4
> + *   0x5A0 ... 0x5BF: Device 4 buffer
> + *   0x600 ... 0x67F: Device 5
> + *   0x6A0 ... 0x6BF: Device 5 buffer
> + *   0x700 ... 0x77F: Device 6
> + *   0x7A0 ... 0x7BF: Device 6 buffer
> + *   0x800 ... 0x87F: Device 7
> + *   0x8A0 ... 0x8BF: Device 7 buffer
> + *   0x900 ... 0x97F: Device 8
> + *   0x9A0 ... 0x9BF: Device 8 buffer
> + *   0xA00 ... 0xA7F: Device 9
> + *   0xAA0 ... 0xABF: Device 9 buffer
> + *   0xB00 ... 0xB7F: Device 10
> + *   0xBA0 ... 0xBBF: Device 10 buffer
> + *   0xC00 ... 0xC7F: Device 11
> + *   0xCA0 ... 0xCBF: Device 11 buffer
> + *   0xD00 ... 0xD7F: Device 12
> + *   0xDA0 ... 0xDBF: Device 12 buffer
> + *   0xE00 ... 0xE7F: Device 13
> + *   0xEA0 ... 0xEBF: Device 13 buffer
> + *   0xF00 ... 0xF7F: Device 14
> + *   0xFA0 ... 0xFBF: Device 14 buffer
> + *   0x1000 ... 0x107F: Device 15
> + *   0x10A0 ... 0x10BF: Device 15 buffer
>    */
>   static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
>   {
> @@ -1500,6 +1535,32 @@ static const TypeInfo aspeed_1030_i2c_info = {
>       .class_init = aspeed_1030_i2c_class_init,
>   };
>   
> +static void aspeed_2700_i2c_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    AspeedI2CClass *aic = ASPEED_I2C_CLASS(klass);
> +
> +    dc->desc = "ASPEED 2700 I2C Controller";
> +
> +    aic->num_busses = 16;
> +    aic->reg_size = 0x80;
> +    aic->reg_gap_size = 0x80;
> +    aic->gap = -1; /* no gap */
> +    aic->bus_get_irq = aspeed_2600_i2c_bus_get_irq;
> +    aic->pool_size = 0x20;
> +    aic->pool_gap_size = 0xe0;
> +    aic->pool_base = 0x1a0;
> +    aic->bus_pool_base = aspeed_2500_i2c_bus_pool_base;
> +    aic->has_dma = true;
> +    aic->mem_size = 0x10000;

I see an overlap the SLI model. Is this value correct ?

  

Thanks,

C.


  
> +}
> +
> +static const TypeInfo aspeed_2700_i2c_info = {
> +    .name = TYPE_ASPEED_2700_I2C,
> +    .parent = TYPE_ASPEED_I2C,
> +    .class_init = aspeed_2700_i2c_class_init,
> +};
> +
>   static void aspeed_i2c_register_types(void)
>   {
>       type_register_static(&aspeed_i2c_bus_info);
> @@ -1509,6 +1570,7 @@ static void aspeed_i2c_register_types(void)
>       type_register_static(&aspeed_2500_i2c_info);
>       type_register_static(&aspeed_2600_i2c_info);
>       type_register_static(&aspeed_1030_i2c_info);
> +    type_register_static(&aspeed_2700_i2c_info);
>   }
>   
>   type_init(aspeed_i2c_register_types)
> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> index bdaea3207d..4f23dc10c3 100644
> --- a/include/hw/i2c/aspeed_i2c.h
> +++ b/include/hw/i2c/aspeed_i2c.h
> @@ -31,6 +31,7 @@
>   #define TYPE_ASPEED_2500_I2C TYPE_ASPEED_I2C "-ast2500"
>   #define TYPE_ASPEED_2600_I2C TYPE_ASPEED_I2C "-ast2600"
>   #define TYPE_ASPEED_1030_I2C TYPE_ASPEED_I2C "-ast1030"
> +#define TYPE_ASPEED_2700_I2C TYPE_ASPEED_I2C "-ast2700"
>   OBJECT_DECLARE_TYPE(AspeedI2CState, AspeedI2CClass, ASPEED_I2C)
>   
>   #define ASPEED_I2C_NR_BUSSES 16

RE: [PATCH v1 09/15] hw/i2c/aspeed: Add AST2700 support

[Jamin Lin]

Hi Cedric,

> Subject: Re: [PATCH v1 09/15] hw/i2c/aspeed: Add AST2700 support
>
> On 7/18/24 08:49, Jamin Lin wrote:
> > Introduce a new ast2700 class to support AST2700.
> > The I2C bus register memory regions and I2C bus pool buffer memory
> > regions are discontinuous and they do not back compatible AST2600.
> >
> > Add a new ast2700 i2c class init function to match the address of I2C
> > bus register and pool buffer from the datasheet.
> >
> > An I2C controller registers owns 8KB address space.
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> >   hw/i2c/aspeed_i2c.c         | 62
> +++++++++++++++++++++++++++++++++++++
> >   include/hw/i2c/aspeed_i2c.h |  1 +
> >   2 files changed, 63 insertions(+)
> >
> > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index
> > c0d3ac3867..29d400ac93 100644
> > --- a/hw/i2c/aspeed_i2c.c
> > +++ b/hw/i2c/aspeed_i2c.c
> > @@ -1101,6 +1101,41 @@ static void aspeed_i2c_instance_init(Object *obj)
> >    *   0xDA0 ... 0xDBF: Device 14 buffer
> >    *   0xDC0 ... 0xDDF: Device 15 buffer (15 and 16 unused in AST1030)
> >    *   0xDE0 ... 0xDFF: Device 16 buffer
> > + *
> > + * Address Definitions (AST2700)
> > + *   0x000 ... 0x0FF: Global Register
> > + *   0x100 ... 0x17F: Device 0
> > + *   0x1A0 ... 0x1BF: Device 0 buffer
> > + *   0x200 ... 0x27F: Device 1
> > + *   0x2A0 ... 0x2BF: Device 1 buffer
> > + *   0x300 ... 0x37F: Device 2
> > + *   0x3A0 ... 0x3BF: Device 2 buffer
> > + *   0x400 ... 0x47F: Device 3
> > + *   0x4A0 ... 0x4BF: Device 3 buffer
> > + *   0x500 ... 0x57F: Device 4
> > + *   0x5A0 ... 0x5BF: Device 4 buffer
> > + *   0x600 ... 0x67F: Device 5
> > + *   0x6A0 ... 0x6BF: Device 5 buffer
> > + *   0x700 ... 0x77F: Device 6
> > + *   0x7A0 ... 0x7BF: Device 6 buffer
> > + *   0x800 ... 0x87F: Device 7
> > + *   0x8A0 ... 0x8BF: Device 7 buffer
> > + *   0x900 ... 0x97F: Device 8
> > + *   0x9A0 ... 0x9BF: Device 8 buffer
> > + *   0xA00 ... 0xA7F: Device 9
> > + *   0xAA0 ... 0xABF: Device 9 buffer
> > + *   0xB00 ... 0xB7F: Device 10
> > + *   0xBA0 ... 0xBBF: Device 10 buffer
> > + *   0xC00 ... 0xC7F: Device 11
> > + *   0xCA0 ... 0xCBF: Device 11 buffer
> > + *   0xD00 ... 0xD7F: Device 12
> > + *   0xDA0 ... 0xDBF: Device 12 buffer
> > + *   0xE00 ... 0xE7F: Device 13
> > + *   0xEA0 ... 0xEBF: Device 13 buffer
> > + *   0xF00 ... 0xF7F: Device 14
> > + *   0xFA0 ... 0xFBF: Device 14 buffer
> > + *   0x1000 ... 0x107F: Device 15
> > + *   0x10A0 ... 0x10BF: Device 15 buffer
> >    */
> >   static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
> >   {
> > @@ -1500,6 +1535,32 @@ static const TypeInfo aspeed_1030_i2c_info = {
> >       .class_init = aspeed_1030_i2c_class_init,
> >   };
> >
> > +static void aspeed_2700_i2c_class_init(ObjectClass *klass, void
> > +*data) {
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    AspeedI2CClass *aic = ASPEED_I2C_CLASS(klass);
> > +
> > +    dc->desc = "ASPEED 2700 I2C Controller";
> > +
> > +    aic->num_busses = 16;
> > +    aic->reg_size = 0x80;
> > +    aic->reg_gap_size = 0x80;
> > +    aic->gap = -1; /* no gap */
> > +    aic->bus_get_irq = aspeed_2600_i2c_bus_get_irq;
> > +    aic->pool_size = 0x20;
> > +    aic->pool_gap_size = 0xe0;
> > +    aic->pool_base = 0x1a0;
> > +    aic->bus_pool_base = aspeed_2500_i2c_bus_pool_base;
> > +    aic->has_dma = true;
> > +    aic->mem_size = 0x10000;
>
> I see an overlap the SLI model. Is this value correct ?
>
Yes, it is incorrect mem_size value.
Will update mem_size to 0x2000(8KB)
Thanks for review.

Jamin
>
>
> Thanks,
>
> C.
>
>
>
> > +}
> > +
> > +static const TypeInfo aspeed_2700_i2c_info = {
> > +    .name = TYPE_ASPEED_2700_I2C,
> > +    .parent = TYPE_ASPEED_I2C,
> > +    .class_init = aspeed_2700_i2c_class_init, };
> > +
> >   static void aspeed_i2c_register_types(void)
> >   {
> >       type_register_static(&aspeed_i2c_bus_info);
> > @@ -1509,6 +1570,7 @@ static void aspeed_i2c_register_types(void)
> >       type_register_static(&aspeed_2500_i2c_info);
> >       type_register_static(&aspeed_2600_i2c_info);
> >       type_register_static(&aspeed_1030_i2c_info);
> > +    type_register_static(&aspeed_2700_i2c_info);
> >   }
> >
> >   type_init(aspeed_i2c_register_types)
> > diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> > index bdaea3207d..4f23dc10c3 100644
> > --- a/include/hw/i2c/aspeed_i2c.h
> > +++ b/include/hw/i2c/aspeed_i2c.h
> > @@ -31,6 +31,7 @@
> >   #define TYPE_ASPEED_2500_I2C TYPE_ASPEED_I2C "-ast2500"
> >   #define TYPE_ASPEED_2600_I2C TYPE_ASPEED_I2C "-ast2600"
> >   #define TYPE_ASPEED_1030_I2C TYPE_ASPEED_I2C "-ast1030"
> > +#define TYPE_ASPEED_2700_I2C TYPE_ASPEED_I2C "-ast2700"
> >   OBJECT_DECLARE_TYPE(AspeedI2CState, AspeedI2CClass, ASPEED_I2C)
> >
> >   #define ASPEED_I2C_NR_BUSSES 16

************* Email Confidentiality Notice ********************
免責聲明:
本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!

DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.

[PATCH v1 10/15] hw/i2c/aspeed: support Tx/Rx buffer 64 bits address

[Jamin Lin via]

ASPEED AST2700 SOC is a 64 bits quad core CPUs (Cortex-a35)
And the base address of dram is "0x4 00000000" which
is 64bits address.

It have "Master DMA Mode Tx Buffer Base Address[39:32](0x60)"
and "Master DMA Mode Rx Buffer Base Address[39:32](0x64)"
to save the high part physical address of Tx/Rx buffer address
for master mode.

It have "Slave DMA Mode Tx Buffer Base Address[39:32](0x68)" and
"Slave DMA Mode Rx Buffer Base Address[39:32](0x6C)" to
save the high part physical address of Tx/Rx buffer address
for slave mode.

Ex: Tx buffer address for master mode [39:0]
The "Master DMA Mode Tx Buffer Base Address[39:32](0x60)"
bits [7:0] which corresponds the bits [39:32] of the 64 bits address of
the Tx buffer address.
The "Master DMA Mode Tx Buffer Base Address(0x30)" bits [31:0]
which corresponds the bits [31:0] of the 64 bits address
of the Tx buffer address.

Introduce a new has_dma64 class attribute and new registers of
new mode to support DMA 64 bits dram address.
Update new mode register number to 28.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/i2c/aspeed_i2c.c         | 48 +++++++++++++++++++++++++++++++++++++
 include/hw/i2c/aspeed_i2c.h | 12 +++++++++-
 2 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 29d400ac93..b48f250e08 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -140,6 +140,7 @@ static uint64_t aspeed_i2c_bus_old_read(AspeedI2CBus *bus, hwaddr offset,
 static uint64_t aspeed_i2c_bus_new_read(AspeedI2CBus *bus, hwaddr offset,
                                         unsigned size)
 {
+    AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
     uint64_t value = bus->regs[offset / sizeof(*bus->regs)];
 
     switch (offset) {
@@ -170,6 +171,16 @@ static uint64_t aspeed_i2c_bus_new_read(AspeedI2CBus *bus, hwaddr offset,
     case A_I2CM_CMD:
         value = SHARED_FIELD_DP32(value, BUS_BUSY_STS, i2c_bus_busy(bus->bus));
         break;
+    case A_I2CM_DMA_TX_ADDR_HI:
+    case A_I2CM_DMA_RX_ADDR_HI:
+    case A_I2CS_DMA_TX_ADDR_HI:
+    case A_I2CS_DMA_RX_ADDR_HI:
+        if (!aic->has_dma64) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA 64 bits support\n",
+            __func__);
+            value = -1;
+        }
+        break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: Bad offset 0x%" HWADDR_PRIx "\n", __func__, offset);
@@ -731,6 +742,42 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
         qemu_log_mask(LOG_UNIMP, "%s: Slave mode DMA TX is not implemented\n",
                       __func__);
         break;
+
+    case A_I2CM_DMA_TX_ADDR_HI:
+        if (!aic->has_dma64) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA 64 bits support\n",
+                          __func__);
+            break;
+        }
+        bus->regs[R_I2CM_DMA_TX_ADDR_HI] = FIELD_EX32(value,
+                                                      I2CM_DMA_TX_ADDR_HI,
+                                                      ADDR_HI);
+        break;
+    case A_I2CM_DMA_RX_ADDR_HI:
+        if (!aic->has_dma64) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA 64 bits support\n",
+                          __func__);
+            break;
+        }
+        bus->regs[R_I2CM_DMA_RX_ADDR_HI] = FIELD_EX32(value,
+                                                      I2CM_DMA_RX_ADDR_HI,
+                                                      ADDR_HI);
+        break;
+    case A_I2CS_DMA_TX_ADDR_HI:
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: Slave mode DMA TX Addr high is not implemented\n",
+                      __func__);
+        break;
+    case A_I2CS_DMA_RX_ADDR_HI:
+        if (!aic->has_dma64) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA 64 bits support\n",
+                          __func__);
+            break;
+        }
+        bus->regs[R_I2CS_DMA_RX_ADDR_HI] = FIELD_EX32(value,
+                                                      I2CS_DMA_RX_ADDR_HI,
+                                                      ADDR_HI);
+        break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
                       __func__, offset);
@@ -1553,6 +1600,7 @@ static void aspeed_2700_i2c_class_init(ObjectClass *klass, void *data)
     aic->bus_pool_base = aspeed_2500_i2c_bus_pool_base;
     aic->has_dma = true;
     aic->mem_size = 0x10000;
+    aic->has_dma64 = true;
 }
 
 static const TypeInfo aspeed_2700_i2c_info = {
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 4f23dc10c3..2c4c81bd20 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -38,7 +38,7 @@ OBJECT_DECLARE_TYPE(AspeedI2CState, AspeedI2CClass, ASPEED_I2C)
 #define ASPEED_I2C_SHARE_POOL_SIZE 0x800
 #define ASPEED_I2C_BUS_POOL_SIZE 0x20
 #define ASPEED_I2C_OLD_NUM_REG 11
-#define ASPEED_I2C_NEW_NUM_REG 22
+#define ASPEED_I2C_NEW_NUM_REG 28
 
 #define A_I2CD_M_STOP_CMD       BIT(5)
 #define A_I2CD_M_RX_CMD         BIT(3)
@@ -227,6 +227,15 @@ REG32(I2CS_DMA_LEN_STS, 0x4c)
     FIELD(I2CS_DMA_LEN_STS, TX_LEN, 0, 13)
 REG32(I2CC_DMA_ADDR, 0x50)
 REG32(I2CC_DMA_LEN, 0x54)
+/* DMA 64bits */
+REG32(I2CM_DMA_TX_ADDR_HI, 0x60)
+    FIELD(I2CM_DMA_TX_ADDR_HI, ADDR_HI, 0, 7)
+REG32(I2CM_DMA_RX_ADDR_HI, 0x64)
+    FIELD(I2CM_DMA_RX_ADDR_HI, ADDR_HI, 0, 7)
+REG32(I2CS_DMA_TX_ADDR_HI, 0x68)
+    FIELD(I2CS_DMA_TX_ADDR_HI, ADDR_HI, 0, 7)
+REG32(I2CS_DMA_RX_ADDR_HI, 0x6c)
+    FIELD(I2CS_DMA_RX_ADDR_HI, ADDR_HI, 0, 7)
 
 struct AspeedI2CState;
 
@@ -292,6 +301,7 @@ struct AspeedI2CClass {
     bool has_dma;
     bool has_share_pool;
     uint64_t mem_size;
+    bool has_dma64;
 };
 
 static inline bool aspeed_i2c_is_new_mode(AspeedI2CState *s)
-- 
2.34.1

Re: [PATCH v1 10/15] hw/i2c/aspeed: support Tx/Rx buffer 64 bits address

[Cédric Le Goater]

On 7/18/24 08:49, Jamin Lin wrote:
> ASPEED AST2700 SOC is a 64 bits quad core CPUs (Cortex-a35)
> And the base address of dram is "0x4 00000000" which
> is 64bits address.
> 
> It have "Master DMA Mode Tx Buffer Base Address[39:32](0x60)"
> and "Master DMA Mode Rx Buffer Base Address[39:32](0x64)"
> to save the high part physical address of Tx/Rx buffer address
> for master mode.
> 
> It have "Slave DMA Mode Tx Buffer Base Address[39:32](0x68)" and
> "Slave DMA Mode Rx Buffer Base Address[39:32](0x6C)" to
> save the high part physical address of Tx/Rx buffer address
> for slave mode.
> 
> Ex: Tx buffer address for master mode [39:0]
> The "Master DMA Mode Tx Buffer Base Address[39:32](0x60)"
> bits [7:0] which corresponds the bits [39:32] of the 64 bits address of
> the Tx buffer address.
> The "Master DMA Mode Tx Buffer Base Address(0x30)" bits [31:0]
> which corresponds the bits [31:0] of the 64 bits address
> of the Tx buffer address.
> 
> Introduce a new has_dma64 class attribute and new registers of
> new mode to support DMA 64 bits dram address.
> Update new mode register number to 28.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/i2c/aspeed_i2c.c         | 48 +++++++++++++++++++++++++++++++++++++
>   include/hw/i2c/aspeed_i2c.h | 12 +++++++++-
>   2 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 29d400ac93..b48f250e08 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -140,6 +140,7 @@ static uint64_t aspeed_i2c_bus_old_read(AspeedI2CBus *bus, hwaddr offset,
>   static uint64_t aspeed_i2c_bus_new_read(AspeedI2CBus *bus, hwaddr offset,
>                                           unsigned size)
>   {
> +    AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
>       uint64_t value = bus->regs[offset / sizeof(*bus->regs)];
>   
>       switch (offset) {
> @@ -170,6 +171,16 @@ static uint64_t aspeed_i2c_bus_new_read(AspeedI2CBus *bus, hwaddr offset,
>       case A_I2CM_CMD:
>           value = SHARED_FIELD_DP32(value, BUS_BUSY_STS, i2c_bus_busy(bus->bus));
>           break;
> +    case A_I2CM_DMA_TX_ADDR_HI:
> +    case A_I2CM_DMA_RX_ADDR_HI:
> +    case A_I2CS_DMA_TX_ADDR_HI:
> +    case A_I2CS_DMA_RX_ADDR_HI:
> +        if (!aic->has_dma64) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA 64 bits support\n",
> +            __func__);
> +            value = -1;
> +        }
> +        break;
>       default:
>           qemu_log_mask(LOG_GUEST_ERROR,
>                         "%s: Bad offset 0x%" HWADDR_PRIx "\n", __func__, offset);
> @@ -731,6 +742,42 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
>           qemu_log_mask(LOG_UNIMP, "%s: Slave mode DMA TX is not implemented\n",
>                         __func__);
>           break;
> +
> +    case A_I2CM_DMA_TX_ADDR_HI:
> +        if (!aic->has_dma64) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA 64 bits support\n",
> +                          __func__);
> +            break;
> +        }
> +        bus->regs[R_I2CM_DMA_TX_ADDR_HI] = FIELD_EX32(value,
> +                                                      I2CM_DMA_TX_ADDR_HI,
> +                                                      ADDR_HI);
> +        break;
> +    case A_I2CM_DMA_RX_ADDR_HI:
> +        if (!aic->has_dma64) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA 64 bits support\n",
> +                          __func__);
> +            break;
> +        }
> +        bus->regs[R_I2CM_DMA_RX_ADDR_HI] = FIELD_EX32(value,
> +                                                      I2CM_DMA_RX_ADDR_HI,
> +                                                      ADDR_HI);
> +        break;
> +    case A_I2CS_DMA_TX_ADDR_HI:
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: Slave mode DMA TX Addr high is not implemented\n",
> +                      __func__);
> +        break;
> +    case A_I2CS_DMA_RX_ADDR_HI:
> +        if (!aic->has_dma64) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA 64 bits support\n",
> +                          __func__);
> +            break;
> +        }
> +        bus->regs[R_I2CS_DMA_RX_ADDR_HI] = FIELD_EX32(value,
> +                                                      I2CS_DMA_RX_ADDR_HI,
> +                                                      ADDR_HI);
> +        break;
>       default:
>           qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
>                         __func__, offset);
> @@ -1553,6 +1600,7 @@ static void aspeed_2700_i2c_class_init(ObjectClass *klass, void *data)
>       aic->bus_pool_base = aspeed_2500_i2c_bus_pool_base;
>       aic->has_dma = true;
>       aic->mem_size = 0x10000;
> +    aic->has_dma64 = true;
>   }
>   
>   static const TypeInfo aspeed_2700_i2c_info = {
> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> index 4f23dc10c3..2c4c81bd20 100644
> --- a/include/hw/i2c/aspeed_i2c.h
> +++ b/include/hw/i2c/aspeed_i2c.h
> @@ -38,7 +38,7 @@ OBJECT_DECLARE_TYPE(AspeedI2CState, AspeedI2CClass, ASPEED_I2C)
>   #define ASPEED_I2C_SHARE_POOL_SIZE 0x800
>   #define ASPEED_I2C_BUS_POOL_SIZE 0x20
>   #define ASPEED_I2C_OLD_NUM_REG 11
> -#define ASPEED_I2C_NEW_NUM_REG 22
> +#define ASPEED_I2C_NEW_NUM_REG 28
>   
>   #define A_I2CD_M_STOP_CMD       BIT(5)
>   #define A_I2CD_M_RX_CMD         BIT(3)
> @@ -227,6 +227,15 @@ REG32(I2CS_DMA_LEN_STS, 0x4c)
>       FIELD(I2CS_DMA_LEN_STS, TX_LEN, 0, 13)
>   REG32(I2CC_DMA_ADDR, 0x50)
>   REG32(I2CC_DMA_LEN, 0x54)
> +/* DMA 64bits */
> +REG32(I2CM_DMA_TX_ADDR_HI, 0x60)
> +    FIELD(I2CM_DMA_TX_ADDR_HI, ADDR_HI, 0, 7)
> +REG32(I2CM_DMA_RX_ADDR_HI, 0x64)
> +    FIELD(I2CM_DMA_RX_ADDR_HI, ADDR_HI, 0, 7)
> +REG32(I2CS_DMA_TX_ADDR_HI, 0x68)
> +    FIELD(I2CS_DMA_TX_ADDR_HI, ADDR_HI, 0, 7)
> +REG32(I2CS_DMA_RX_ADDR_HI, 0x6c)
> +    FIELD(I2CS_DMA_RX_ADDR_HI, ADDR_HI, 0, 7)
>   
>   struct AspeedI2CState;
>   
> @@ -292,6 +301,7 @@ struct AspeedI2CClass {
>       bool has_dma;
>       bool has_share_pool;
>       uint64_t mem_size;
> +    bool has_dma64;
>   };
>   
>   static inline bool aspeed_i2c_is_new_mode(AspeedI2CState *s)

[PATCH v1 11/15] hw/i2c/aspeed: support high part dram offset for DMA 64 bits

[Jamin Lin via]

ASPEED AST2700 SOC is a 64 bits quad core CPUs (Cortex-a35)
And the base address of dram is "0x4 00000000" which
is 64bits address.

The AST2700 support the maximum DRAM size is 8 GB.
The DRAM physical address range is from "0x4_0000_0000" to
"0x5_FFFF_FFFF".

The DRAM offset range is from "0x0_0000_0000" to
"0x1_FFFF_FFFF" and it is enough to use bits [33:0]
saving the dram offset.

Therefore, save the high part physical address bit[1:0]
of Tx/Rx buffer address as dma_dram_offset bit[33:32].
It does not need to decrease the dram physical
high part address for DMA operation.
(high part physical address bit[7:0] – 4)

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/i2c/aspeed_i2c.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index b48f250e08..e28deadcfc 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -743,6 +743,14 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
                       __func__);
         break;
 
+    /*
+     * The AST2700 support the maximum DRAM size is 8 GB.
+     * The DRAM offset range is from 0x0_0000_0000 to
+     * 0x1_FFFF_FFFF and it is enough to use bits [33:0]
+     * saving the dram offset.
+     * Therefore, save the high part physical address bit[1:0]
+     * of Tx/Rx buffer address as dma_dram_offset bit[33:32].
+     */
     case A_I2CM_DMA_TX_ADDR_HI:
         if (!aic->has_dma64) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA 64 bits support\n",
@@ -752,6 +760,8 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
         bus->regs[R_I2CM_DMA_TX_ADDR_HI] = FIELD_EX32(value,
                                                       I2CM_DMA_TX_ADDR_HI,
                                                       ADDR_HI);
+        bus->dma_dram_offset = deposit64(bus->dma_dram_offset, 32, 32,
+                                         extract32(value, 0, 2));
         break;
     case A_I2CM_DMA_RX_ADDR_HI:
         if (!aic->has_dma64) {
@@ -762,6 +772,8 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
         bus->regs[R_I2CM_DMA_RX_ADDR_HI] = FIELD_EX32(value,
                                                       I2CM_DMA_RX_ADDR_HI,
                                                       ADDR_HI);
+        bus->dma_dram_offset = deposit64(bus->dma_dram_offset, 32, 32,
+                                         extract32(value, 0, 2));
         break;
     case A_I2CS_DMA_TX_ADDR_HI:
         qemu_log_mask(LOG_UNIMP,
@@ -777,6 +789,8 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
         bus->regs[R_I2CS_DMA_RX_ADDR_HI] = FIELD_EX32(value,
                                                       I2CS_DMA_RX_ADDR_HI,
                                                       ADDR_HI);
+        bus->dma_dram_offset = deposit64(bus->dma_dram_offset, 32, 32,
+                                         extract32(value, 0, 2));
         break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
-- 
2.34.1

Re: [PATCH v1 11/15] hw/i2c/aspeed: support high part dram offset for DMA 64 bits

[Cédric Le Goater]

On 7/18/24 08:49, Jamin Lin wrote:
> ASPEED AST2700 SOC is a 64 bits quad core CPUs (Cortex-a35)
> And the base address of dram is "0x4 00000000" which
> is 64bits address.
> 
> The AST2700 support the maximum DRAM size is 8 GB.
> The DRAM physical address range is from "0x4_0000_0000" to
> "0x5_FFFF_FFFF".
> 
> The DRAM offset range is from "0x0_0000_0000" to
> "0x1_FFFF_FFFF" and it is enough to use bits [33:0]
> saving the dram offset.
> 
> Therefore, save the high part physical address bit[1:0]
> of Tx/Rx buffer address as dma_dram_offset bit[33:32].
> It does not need to decrease the dram physical
> high part address for DMA operation.
> (high part physical address bit[7:0] – 4)
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/i2c/aspeed_i2c.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index b48f250e08..e28deadcfc 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -743,6 +743,14 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
>                         __func__);
>           break;
>   
> +    /*
> +     * The AST2700 support the maximum DRAM size is 8 GB.
> +     * The DRAM offset range is from 0x0_0000_0000 to
> +     * 0x1_FFFF_FFFF and it is enough to use bits [33:0]
> +     * saving the dram offset.
> +     * Therefore, save the high part physical address bit[1:0]
> +     * of Tx/Rx buffer address as dma_dram_offset bit[33:32].
> +     */
>       case A_I2CM_DMA_TX_ADDR_HI:
>           if (!aic->has_dma64) {
>               qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA 64 bits support\n",
> @@ -752,6 +760,8 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
>           bus->regs[R_I2CM_DMA_TX_ADDR_HI] = FIELD_EX32(value,
>                                                         I2CM_DMA_TX_ADDR_HI,
>                                                         ADDR_HI);
> +        bus->dma_dram_offset = deposit64(bus->dma_dram_offset, 32, 32,
> +                                         extract32(value, 0, 2));
>           break;
>       case A_I2CM_DMA_RX_ADDR_HI:
>           if (!aic->has_dma64) {
> @@ -762,6 +772,8 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
>           bus->regs[R_I2CM_DMA_RX_ADDR_HI] = FIELD_EX32(value,
>                                                         I2CM_DMA_RX_ADDR_HI,
>                                                         ADDR_HI);
> +        bus->dma_dram_offset = deposit64(bus->dma_dram_offset, 32, 32,
> +                                         extract32(value, 0, 2));
>           break;
>       case A_I2CS_DMA_TX_ADDR_HI:
>           qemu_log_mask(LOG_UNIMP,
> @@ -777,6 +789,8 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
>           bus->regs[R_I2CS_DMA_RX_ADDR_HI] = FIELD_EX32(value,
>                                                         I2CS_DMA_RX_ADDR_HI,
>                                                         ADDR_HI);
> +        bus->dma_dram_offset = deposit64(bus->dma_dram_offset, 32, 32,
> +                                         extract32(value, 0, 2));
>           break;
>       default:
>           qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",

[PATCH v1 12/15] aspeed/soc: introduce a new API to get the INTC orgate information

[Jamin Lin via]

Currently, users can set the intc mapping table with
enumerated device id and device irq to get the INTC orgate
input pins. However, some devices use the continuous bits number in the
same orgate. To reduce the enumerated device id definition,
create a new API to get the INTC orgate index and source bit number
if users only provide the start bus number of device.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/arm/aspeed_ast27x0.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index 4257b5e8af..0bbd66110b 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -164,6 +164,11 @@ struct gic_intc_irq_info {
     const int *ptr;
 };
 
+struct gic_intc_orgate_info {
+    int index;
+    int int_num;
+};
+
 static const struct gic_intc_irq_info aspeed_soc_ast2700_gic_intcmap[] = {
     {128,  aspeed_soc_ast2700_gic128_intcmap},
     {129,  NULL},
@@ -193,6 +198,27 @@ static qemu_irq aspeed_soc_ast2700_get_irq(AspeedSoCState *s, int dev)
     return qdev_get_gpio_in(DEVICE(&a->gic), sc->irqmap[dev]);
 }
 
+static void aspeed_soc_ast2700_get_intc_orgate(AspeedSoCState *s, int dev,
+    struct gic_intc_orgate_info *orgate_info)
+{
+    AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(aspeed_soc_ast2700_gic_intcmap); i++) {
+        if (sc->irqmap[dev] == aspeed_soc_ast2700_gic_intcmap[i].irq) {
+            assert(aspeed_soc_ast2700_gic_intcmap[i].ptr);
+            orgate_info->index = i;
+            orgate_info->int_num = aspeed_soc_ast2700_gic_intcmap[i].ptr[dev];
+            return;
+        }
+    }
+
+    /*
+     * Invalid orgate index, device irq should be 128 to 136.
+     */
+    g_assert_not_reached();
+}
+
 static uint64_t aspeed_ram_capacity_read(void *opaque, hwaddr addr,
                                                     unsigned int size)
 {
-- 
2.34.1

Re: [PATCH v1 12/15] aspeed/soc: introduce a new API to get the INTC orgate information

[Cédric Le Goater]

On 7/18/24 08:49, Jamin Lin wrote:
> Currently, users can set the intc mapping table with
> enumerated device id and device irq to get the INTC orgate
> input pins. However, some devices use the continuous bits number in the
> same orgate. To reduce the enumerated device id definition,
> create a new API to get the INTC orgate index and source bit number
> if users only provide the start bus number of device.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/arm/aspeed_ast27x0.c | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
> index 4257b5e8af..0bbd66110b 100644
> --- a/hw/arm/aspeed_ast27x0.c
> +++ b/hw/arm/aspeed_ast27x0.c
> @@ -164,6 +164,11 @@ struct gic_intc_irq_info {
>       const int *ptr;
>   };
>   
> +struct gic_intc_orgate_info {
> +    int index;
> +    int int_num;
> +};
> +
>   static const struct gic_intc_irq_info aspeed_soc_ast2700_gic_intcmap[] = {
>       {128,  aspeed_soc_ast2700_gic128_intcmap},
>       {129,  NULL},
> @@ -193,6 +198,27 @@ static qemu_irq aspeed_soc_ast2700_get_irq(AspeedSoCState *s, int dev)
>       return qdev_get_gpio_in(DEVICE(&a->gic), sc->irqmap[dev]);
>   }
>   
> +static void aspeed_soc_ast2700_get_intc_orgate(AspeedSoCState *s, int dev,
> +    struct gic_intc_orgate_info *orgate_info)
> +{
> +    AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(aspeed_soc_ast2700_gic_intcmap); i++) {
> +        if (sc->irqmap[dev] == aspeed_soc_ast2700_gic_intcmap[i].irq) {
> +            assert(aspeed_soc_ast2700_gic_intcmap[i].ptr);
> +            orgate_info->index = i;
> +            orgate_info->int_num = aspeed_soc_ast2700_gic_intcmap[i].ptr[dev];
> +            return;
> +        }
> +    }
> +
> +    /*
> +     * Invalid orgate index, device irq should be 128 to 136.
> +     */
> +    g_assert_not_reached();
> +}


This looks redundant. Couldn't we extend aspeed_soc_ast2700_get_irq() with an
index parameter instead ?

Thanks,

C.




>   static uint64_t aspeed_ram_capacity_read(void *opaque, hwaddr addr,
>                                                       unsigned int size)
>   {

RE: [PATCH v1 12/15] aspeed/soc: introduce a new API to get the INTC orgate information

[Jamin Lin]

Hi Cedric,

> Subject: Re: [PATCH v1 12/15] aspeed/soc: introduce a new API to get the INTC
> orgate information
>
> On 7/18/24 08:49, Jamin Lin wrote:
> > Currently, users can set the intc mapping table with enumerated device
> > id and device irq to get the INTC orgate input pins. However, some
> > devices use the continuous bits number in the same orgate. To reduce
> > the enumerated device id definition, create a new API to get the INTC
> > orgate index and source bit number if users only provide the start bus
> > number of device.
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> >   hw/arm/aspeed_ast27x0.c | 26 ++++++++++++++++++++++++++
> >   1 file changed, 26 insertions(+)
> >
> > diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index
> > 4257b5e8af..0bbd66110b 100644
> > --- a/hw/arm/aspeed_ast27x0.c
> > +++ b/hw/arm/aspeed_ast27x0.c
> > @@ -164,6 +164,11 @@ struct gic_intc_irq_info {
> >       const int *ptr;
> >   };
> >
> > +struct gic_intc_orgate_info {
> > +    int index;
> > +    int int_num;
> > +};
> > +
> >   static const struct gic_intc_irq_info aspeed_soc_ast2700_gic_intcmap[] =
> {
> >       {128,  aspeed_soc_ast2700_gic128_intcmap},
> >       {129,  NULL},
> > @@ -193,6 +198,27 @@ static qemu_irq
> aspeed_soc_ast2700_get_irq(AspeedSoCState *s, int dev)
> >       return qdev_get_gpio_in(DEVICE(&a->gic), sc->irqmap[dev]);
> >   }
> >
> > +static void aspeed_soc_ast2700_get_intc_orgate(AspeedSoCState *s, int
> dev,
> > +    struct gic_intc_orgate_info *orgate_info) {
> > +    AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
> > +    int i;
> > +
> > +    for (i = 0; i < ARRAY_SIZE(aspeed_soc_ast2700_gic_intcmap); i++) {
> > +        if (sc->irqmap[dev] == aspeed_soc_ast2700_gic_intcmap[i].irq) {
> > +            assert(aspeed_soc_ast2700_gic_intcmap[i].ptr);
> > +            orgate_info->index = i;
> > +            orgate_info->int_num =
> aspeed_soc_ast2700_gic_intcmap[i].ptr[dev];
> > +            return;
> > +        }
> > +    }
> > +
> > +    /*
> > +     * Invalid orgate index, device irq should be 128 to 136.
> > +     */
> > +    g_assert_not_reached();
> > +}
>
>
> This looks redundant. Couldn't we extend aspeed_soc_ast2700_get_irq() with
> an index parameter instead ?
>

Thanks for review and suggestion.

According to the current design of aspeed_soc_get_irq,
the function type of get_irq callback function should be XXX(AspeedSoCState *s, int dev)

qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int dev)
{
    return ASPEED_SOC_GET_CLASS(s)->get_irq(s, dev);
}

struct AspeedSoCClass {
    qemu_irq (*get_irq)(AspeedSoCState *s, int dev);
}

If we want to add a new index parameter in aspeed_soc_ast2700_get_irq, I will change as following.
1.
static qemu_irq aspeed_soc_ast2700_get_irq(AspeedSoCState *s, int dev, int index)
{
    Aspeed27x0SoCState *a = ASPEED27X0_SOC(s);
    AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
    int i;

    for (i = 0; i < ARRAY_SIZE(aspeed_soc_ast2700_gic_intcmap); i++) {
        if (sc->irqmap[dev] == aspeed_soc_ast2700_gic_intcmap[i].irq) {
            assert(aspeed_soc_ast2700_gic_intcmap[i].ptr);
            return qdev_get_gpio_in(DEVICE(&a->intc.orgates[i]),
                aspeed_soc_ast2700_gic_intcmap[i].ptr[dev] + index);
        }
    }

    return qdev_get_gpio_in(DEVICE(&a->gic), sc->irqmap[dev]);
}

2. introduce a new get_irq_index function pointer and the function type of this callback function as following.
struct AspeedSoCClass {
   qemu_irq_index (*get_irq)(AspeedSoCState *s, int dev, int index);
}

Do you have any concern or could you please give me any suggestion?
Thanks-Jamin


> Thanks,
>
> C.
>
>
>
>
> >   static uint64_t aspeed_ram_capacity_read(void *opaque, hwaddr addr,
> >                                                       unsigned
> int size)
> >   {

************* Email Confidentiality Notice ********************
免責聲明:
本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!

DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.

RE: [PATCH v1 12/15] aspeed/soc: introduce a new API to get the INTC orgate information

[Jamin Lin]

Sorry for typo

> Subject: RE: [PATCH v1 12/15] aspeed/soc: introduce a new API to get the INTC
> orgate information
>
> Hi Cedric,
>
> > Subject: Re: [PATCH v1 12/15] aspeed/soc: introduce a new API to get
> > the INTC orgate information
> >
> > On 7/18/24 08:49, Jamin Lin wrote:
> > > Currently, users can set the intc mapping table with enumerated
> > > device id and device irq to get the INTC orgate input pins. However,
> > > some devices use the continuous bits number in the same orgate. To
> > > reduce the enumerated device id definition, create a new API to get
> > > the INTC orgate index and source bit number if users only provide
> > > the start bus number of device.
> > >
> > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > > ---
> > >   hw/arm/aspeed_ast27x0.c | 26 ++++++++++++++++++++++++++
> > >   1 file changed, 26 insertions(+)
> > >
> > > diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index
> > > 4257b5e8af..0bbd66110b 100644
> > > --- a/hw/arm/aspeed_ast27x0.c
> > > +++ b/hw/arm/aspeed_ast27x0.c
> > > @@ -164,6 +164,11 @@ struct gic_intc_irq_info {
> > >       const int *ptr;
> > >   };
> > >
> > > +struct gic_intc_orgate_info {
> > > +    int index;
> > > +    int int_num;
> > > +};
> > > +
> > >   static const struct gic_intc_irq_info
> > > aspeed_soc_ast2700_gic_intcmap[] =
> > {
> > >       {128,  aspeed_soc_ast2700_gic128_intcmap},
> > >       {129,  NULL},
> > > @@ -193,6 +198,27 @@ static qemu_irq
> > aspeed_soc_ast2700_get_irq(AspeedSoCState *s, int dev)
> > >       return qdev_get_gpio_in(DEVICE(&a->gic), sc->irqmap[dev]);
> > >   }
> > >
> > > +static void aspeed_soc_ast2700_get_intc_orgate(AspeedSoCState *s,
> > > +int
> > dev,
> > > +    struct gic_intc_orgate_info *orgate_info) {
> > > +    AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
> > > +    int i;
> > > +
> > > +    for (i = 0; i < ARRAY_SIZE(aspeed_soc_ast2700_gic_intcmap); i++) {
> > > +        if (sc->irqmap[dev] == aspeed_soc_ast2700_gic_intcmap[i].irq)
> {
> > > +            assert(aspeed_soc_ast2700_gic_intcmap[i].ptr);
> > > +            orgate_info->index = i;
> > > +            orgate_info->int_num =
> > aspeed_soc_ast2700_gic_intcmap[i].ptr[dev];
> > > +            return;
> > > +        }
> > > +    }
> > > +
> > > +    /*
> > > +     * Invalid orgate index, device irq should be 128 to 136.
> > > +     */
> > > +    g_assert_not_reached();
> > > +}
> >
> >
> > This looks redundant. Couldn't we extend aspeed_soc_ast2700_get_irq()
> > with an index parameter instead ?
> >
>
> Thanks for review and suggestion.
>
> According to the current design of aspeed_soc_get_irq, the function type of
> get_irq callback function should be XXX(AspeedSoCState *s, int dev)
>
> qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int dev) {
>     return ASPEED_SOC_GET_CLASS(s)->get_irq(s, dev); }
>
> struct AspeedSoCClass {
>     qemu_irq (*get_irq)(AspeedSoCState *s, int dev); }
>
> If we want to add a new index parameter in aspeed_soc_ast2700_get_irq, I
> will change as following.
> 1.
> static qemu_irq aspeed_soc_ast2700_get_irq(AspeedSoCState *s, int dev, int
> index) {
>     Aspeed27x0SoCState *a = ASPEED27X0_SOC(s);
>     AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
>     int i;
>
>     for (i = 0; i < ARRAY_SIZE(aspeed_soc_ast2700_gic_intcmap); i++) {
>         if (sc->irqmap[dev] == aspeed_soc_ast2700_gic_intcmap[i].irq) {
>             assert(aspeed_soc_ast2700_gic_intcmap[i].ptr);
>             return qdev_get_gpio_in(DEVICE(&a->intc.orgates[i]),
>                 aspeed_soc_ast2700_gic_intcmap[i].ptr[dev] + index);
>         }
>     }
>
>     return qdev_get_gpio_in(DEVICE(&a->gic), sc->irqmap[dev]); }
>
> 2. introduce a new get_irq_index function pointer and the function type of this
> callback function as following.
> struct AspeedSoCClass {
>    qemu_irq_index (*get_irq)(AspeedSoCState *s, int dev, int index); }
>
qemu_irq (*get_irq_index)(AspeedSoCState *s, int dev, int index);


3. Add aspeed_soc_get_irq_index
qemu_irq aspeed_soc_get_irq_index(AspeedSoCState *s, int dev, int index)
{
    return ASPEED_SOC_GET_CLASS(s)->get_irq_index(s, dev, index);
}

4. I need to modify this function, too
bool aspeed_soc_uart_realize(AspeedSoCState *s, Error **errp)
{
 sysbus_connect_irq(SYS_BUS_DEVICE(smm), 0, aspeed_soc_get_irq(s, uart));
}

Thanks-Jamin
> Do you have any concern or could you please give me any suggestion?
> Thanks-Jamin
>
>
> > Thanks,
> >
> > C.
> >
> >
> >
> >
> > >   static uint64_t aspeed_ram_capacity_read(void *opaque, hwaddr addr,
> > >
> unsigned
> > int size)
> > >   {

************* Email Confidentiality Notice ********************
免責聲明:
本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!

DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.

[PATCH v1 13/15] aspeed/soc: support I2C for AST2700

[Jamin Lin via]

Add I2C model for AST2700 I2C support.
The I2C controller registers base address is start at
0x14C0_F000 and its address space is 0x2000.

The AST2700 I2C controller has one source INTC per bus.
I2C buses interrupt are connected to GICINT130_INTC
from bit 0 to bit 15.
I2C bus 0 is connected to GICINT130_INTC at bit 0.
I2C bus 15 is connected to GICINT130_INTC at bit 15.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/arm/aspeed_ast27x0.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index 0bbd66110b..e84141c13b 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -61,6 +61,7 @@ static const hwaddr aspeed_soc_ast2700_memmap[] = {
     [ASPEED_GIC_DIST]      =  0x12200000,
     [ASPEED_GIC_REDIST]    =  0x12280000,
     [ASPEED_DEV_ADC]       =  0x14C00000,
+    [ASPEED_DEV_I2C]       =  0x14C0F000,
 };
 
 #define AST2700_MAX_IRQ 288
@@ -374,6 +375,9 @@ static void aspeed_soc_ast2700_init(Object *obj)
 
     snprintf(typename, sizeof(typename), "aspeed.adc-%s", socname);
     object_initialize_child(obj, "adc", &s->adc, typename);
+
+    snprintf(typename, sizeof(typename), "aspeed.i2c-%s", socname);
+    object_initialize_child(obj, "i2c", &s->i2c, typename);
 }
 
 /*
@@ -457,6 +461,8 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
     AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
     AspeedINTCClass *ic = ASPEED_INTC_GET_CLASS(&a->intc);
     g_autofree char *sram_name = NULL;
+    qemu_irq irq;
+    struct gic_intc_orgate_info orgate_info;
 
     /* Default boot region (SPI memory or ROMs) */
     memory_region_init(&s->spi_boot_container, OBJECT(s),
@@ -639,6 +645,27 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
                        aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
 
+    /* I2C */
+    object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
+                             &error_abort);
+    if (!sysbus_realize(SYS_BUS_DEVICE(&s->i2c), errp)) {
+        return;
+    }
+    aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->i2c), 0, sc->memmap[ASPEED_DEV_I2C]);
+    aspeed_soc_ast2700_get_intc_orgate(s, ASPEED_DEV_I2C, &orgate_info);
+    for (i = 0; i < ASPEED_I2C_GET_CLASS(&s->i2c)->num_busses; i++) {
+        /*
+         * The AST2700 I2C controller has one source INTC per bus.
+         * I2C buses interrupt are connected to GICINT130_INTC
+         * from bit 0 to bit 15.
+         * I2C bus 0 is connected to GICINT130_INTC at bit 0.
+         * I2C bus 15 is connected to GICINT130_INTC at bit 15.
+         */
+        irq = qdev_get_gpio_in(DEVICE(&a->intc.orgates[orgate_info.index]),
+                               orgate_info.int_num + i);
+        sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c.busses[i]), 0, irq);
+    }
+
     create_unimplemented_device("ast2700.dpmcu", 0x11000000, 0x40000);
     create_unimplemented_device("ast2700.iomem0", 0x12000000, 0x01000000);
     create_unimplemented_device("ast2700.iomem1", 0x14000000, 0x01000000);
-- 
2.34.1

[PATCH v1 14/15] aspeed: fix coding style

[Jamin Lin via]

Fix coding style issues from checkpatch.pl

Test command:
./scripts/checkpatch.pl --no-tree -f hw/arm/aspeed.c

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/arm/aspeed.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 53a4f665d0..f8766ea462 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -265,7 +265,8 @@ static void write_boot_rom(BlockBackend *blk, hwaddr addr, size_t rom_size,
     g_autofree void *storage = NULL;
     int64_t size;
 
-    /* The block backend size should have already been 'validated' by
+    /*
+     * The block backend size should have already been 'validated' by
      * the creation of the m25p80 object.
      */
     size = blk_getlength(blk);
@@ -463,8 +464,10 @@ static void palmetto_bmc_i2c_init(AspeedMachineState *bmc)
     DeviceState *dev;
     uint8_t *eeprom_buf = g_malloc0(32 * 1024);
 
-    /* The palmetto platform expects a ds3231 RTC but a ds1338 is
-     * enough to provide basic RTC features. Alarms will be missing */
+    /*
+     * The palmetto platform expects a ds3231 RTC but a ds1338 is
+     * enough to provide basic RTC features. Alarms will be missing
+     */
     i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 0), "ds1338", 0x68);
 
     smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 0), 0x50,
@@ -555,8 +558,10 @@ static void romulus_bmc_i2c_init(AspeedMachineState *bmc)
 {
     AspeedSoCState *soc = bmc->soc;
 
-    /* The romulus board expects Epson RX8900 I2C RTC but a ds1338 is
-     * good enough */
+    /*
+     * The romulus board expects Epson RX8900 I2C RTC but a ds1338 is
+     * good enough
+     */
     i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
 }
 
@@ -664,8 +669,10 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
     i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 9), TYPE_TMP105,
                      0x4a);
 
-    /* The witherspoon board expects Epson RX8900 I2C RTC but a ds1338 is
-     * good enough */
+    /*
+     * The witherspoon board expects Epson RX8900 I2C RTC but a ds1338 is
+     * good enough
+     */
     i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
 
     smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 11), 0x51,
-- 
2.34.1

Re: [PATCH v1 14/15] aspeed: fix coding style

[Cédric Le Goater]

On 7/18/24 08:49, Jamin Lin wrote:
> Fix coding style issues from checkpatch.pl
> 
> Test command:
> ./scripts/checkpatch.pl --no-tree -f hw/arm/aspeed.c
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/arm/aspeed.c | 21 ++++++++++++++-------
>   1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 53a4f665d0..f8766ea462 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -265,7 +265,8 @@ static void write_boot_rom(BlockBackend *blk, hwaddr addr, size_t rom_size,
>       g_autofree void *storage = NULL;
>       int64_t size;
>   
> -    /* The block backend size should have already been 'validated' by
> +    /*
> +     * The block backend size should have already been 'validated' by
>        * the creation of the m25p80 object.
>        */
>       size = blk_getlength(blk);
> @@ -463,8 +464,10 @@ static void palmetto_bmc_i2c_init(AspeedMachineState *bmc)
>       DeviceState *dev;
>       uint8_t *eeprom_buf = g_malloc0(32 * 1024);
>   
> -    /* The palmetto platform expects a ds3231 RTC but a ds1338 is
> -     * enough to provide basic RTC features. Alarms will be missing */
> +    /*
> +     * The palmetto platform expects a ds3231 RTC but a ds1338 is
> +     * enough to provide basic RTC features. Alarms will be missing
> +     */
>       i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 0), "ds1338", 0x68);
>   
>       smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 0), 0x50,
> @@ -555,8 +558,10 @@ static void romulus_bmc_i2c_init(AspeedMachineState *bmc)
>   {
>       AspeedSoCState *soc = bmc->soc;
>   
> -    /* The romulus board expects Epson RX8900 I2C RTC but a ds1338 is
> -     * good enough */
> +    /*
> +     * The romulus board expects Epson RX8900 I2C RTC but a ds1338 is
> +     * good enough
> +     */
>       i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
>   }
>   
> @@ -664,8 +669,10 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState *bmc)
>       i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 9), TYPE_TMP105,
>                        0x4a);
>   
> -    /* The witherspoon board expects Epson RX8900 I2C RTC but a ds1338 is
> -     * good enough */
> +    /*
> +     * The witherspoon board expects Epson RX8900 I2C RTC but a ds1338 is
> +     * good enough
> +     */
>       i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 11), "ds1338", 0x32);
>   
>       smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 11), 0x51,

[PATCH v1 15/15] aspeed: add tmp105 in i2c bus 0 for AST2700

[Jamin Lin via]

ASPEED SDK add lm75 in i2c bus 0 for AST2700.
LM75 is compatible with TMP105 driver.

Introduce a new i2c init function and
add tmp105 device model in i2c bus 0.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/arm/aspeed.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index f8766ea462..ed98758708 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -1604,6 +1604,15 @@ static void aspeed_minibmc_machine_ast1030_evb_class_init(ObjectClass *oc,
 }
 
 #ifdef TARGET_AARCH64
+static void ast2700_evb_i2c_init(AspeedMachineState *bmc)
+{
+    AspeedSoCState *soc = bmc->soc;
+
+    /* LM75 is compatible with TMP105 driver */
+    i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 0),
+                            TYPE_TMP105, 0x4d);
+}
+
 static void aspeed_machine_ast2700_evb_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -1618,6 +1627,7 @@ static void aspeed_machine_ast2700_evb_class_init(ObjectClass *oc, void *data)
     amc->num_cs    = 2;
     amc->macs_mask = ASPEED_MAC0_ON | ASPEED_MAC1_ON | ASPEED_MAC2_ON;
     amc->uart_default = ASPEED_DEV_UART12;
+    amc->i2c_init  = ast2700_evb_i2c_init;
     mc->default_ram_size = 1 * GiB;
     aspeed_machine_class_init_cpus_defaults(mc);
 }
-- 
2.34.1

Re: [PATCH v1 15/15] aspeed: add tmp105 in i2c bus 0 for AST2700

[Cédric Le Goater]

On 7/18/24 08:49, Jamin Lin wrote:
> ASPEED SDK add lm75 in i2c bus 0 for AST2700.
> LM75 is compatible with TMP105 driver.
> 
> Introduce a new i2c init function and
> add tmp105 device model in i2c bus 0.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>

As a followup, you could modify test_aarch64_ast2700_evb_sdk_v09_02
to read the sensor value.

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/arm/aspeed.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index f8766ea462..ed98758708 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -1604,6 +1604,15 @@ static void aspeed_minibmc_machine_ast1030_evb_class_init(ObjectClass *oc,
>   }
>   
>   #ifdef TARGET_AARCH64
> +static void ast2700_evb_i2c_init(AspeedMachineState *bmc)
> +{
> +    AspeedSoCState *soc = bmc->soc;
> +
> +    /* LM75 is compatible with TMP105 driver */
> +    i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 0),
> +                            TYPE_TMP105, 0x4d);
> +}
> +
>   static void aspeed_machine_ast2700_evb_class_init(ObjectClass *oc, void *data)
>   {
>       MachineClass *mc = MACHINE_CLASS(oc);
> @@ -1618,6 +1627,7 @@ static void aspeed_machine_ast2700_evb_class_init(ObjectClass *oc, void *data)
>       amc->num_cs    = 2;
>       amc->macs_mask = ASPEED_MAC0_ON | ASPEED_MAC1_ON | ASPEED_MAC2_ON;
>       amc->uart_default = ASPEED_DEV_UART12;
> +    amc->i2c_init  = ast2700_evb_i2c_init;
>       mc->default_ram_size = 1 * GiB;
>       aspeed_machine_class_init_cpus_defaults(mc);
>   }

RE: [PATCH v1 15/15] aspeed: add tmp105 in i2c bus 0 for AST2700

[Jamin Lin]

Hi Cedric,

> Subject: Re: [PATCH v1 15/15] aspeed: add tmp105 in i2c bus 0 for AST2700
>
> On 7/18/24 08:49, Jamin Lin wrote:
> > ASPEED SDK add lm75 in i2c bus 0 for AST2700.
> > LM75 is compatible with TMP105 driver.
> >
> > Introduce a new i2c init function and
> > add tmp105 device model in i2c bus 0.
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
>
> As a followup, you could modify test_aarch64_ast2700_evb_sdk_v09_02
> to read the sensor value.
>
Thanks for review and suggestion.
Will update avocado test case.

Jamin
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
>
> Thanks,
>
> C.
>
>
> > ---
> >   hw/arm/aspeed.c | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> >
> > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c index
> > f8766ea462..ed98758708 100644
> > --- a/hw/arm/aspeed.c
> > +++ b/hw/arm/aspeed.c
> > @@ -1604,6 +1604,15 @@ static void
> aspeed_minibmc_machine_ast1030_evb_class_init(ObjectClass *oc,
> >   }
> >
> >   #ifdef TARGET_AARCH64
> > +static void ast2700_evb_i2c_init(AspeedMachineState *bmc) {
> > +    AspeedSoCState *soc = bmc->soc;
> > +
> > +    /* LM75 is compatible with TMP105 driver */
> > +    i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 0),
> > +                            TYPE_TMP105, 0x4d); }
> > +
> >   static void aspeed_machine_ast2700_evb_class_init(ObjectClass *oc, void
> *data)
> >   {
> >       MachineClass *mc = MACHINE_CLASS(oc); @@ -1618,6 +1627,7
> @@
> > static void aspeed_machine_ast2700_evb_class_init(ObjectClass *oc, void
> *data)
> >       amc->num_cs    = 2;
> >       amc->macs_mask = ASPEED_MAC0_ON | ASPEED_MAC1_ON |
> ASPEED_MAC2_ON;
> >       amc->uart_default = ASPEED_DEV_UART12;
> > +    amc->i2c_init  = ast2700_evb_i2c_init;
> >       mc->default_ram_size = 1 * GiB;
> >       aspeed_machine_class_init_cpus_defaults(mc);
> >   }

************* Email Confidentiality Notice ********************
免責聲明:
本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!

DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.

Re: [PATCH v1 00/15] support ADC and I2C for AST2700

[Cédric Le Goater]

On 7/18/24 08:49, Jamin Lin wrote:
> v1:
> 1. support ADC for AST2700
> 2. support I2C for AST2700
> 
> Jamin Lin (15):
>    aspeed/adc: Add AST2700 support
>    aspeed/soc: support ADC for AST2700
>    hw/i2c/aspeed: support to set the different memory size
>    hw/i2c/aspeed: support discontinuous register memory region of I2C bus
>    hw/i2c/aspeed: rename the I2C class pool attribute to share_pool
>    hw/i2c/aspeed: introduce a new bus pool buffer attribute in
>      AspeedI2Cbus
>    hw/i2c/aspeed: support discontinuous poll buffer memory region of I2C
>      bus
>    hw/i2c/aspeed: introduce a new dma_dram_offset attribute in
>      AspeedI2Cbus
>    hw/i2c/aspeed: Add AST2700 support
>    hw/i2c/aspeed: support Tx/Rx buffer 64 bits address
>    hw/i2c/aspeed: support high part dram offset for DMA 64 bits
>    aspeed/soc: introduce a new API to get the INTC orgate information
>    aspeed/soc: support I2C for AST2700
>    aspeed: fix coding style
>    aspeed: add tmp105 in i2c bus 0 for AST2700
> 
>   hw/adc/aspeed_adc.c         |  16 ++
>   hw/arm/aspeed.c             |  31 +++-
>   hw/arm/aspeed_ast27x0.c     |  65 +++++++
>   hw/i2c/aspeed_i2c.c         | 340 ++++++++++++++++++++++++++++++------
>   include/hw/adc/aspeed_adc.h |   1 +
>   include/hw/i2c/aspeed_i2c.h |  34 ++--
>   6 files changed, 418 insertions(+), 69 deletions(-)
> 



Applied 1-2 to aspeed-next.

Thanks,

C.

Re: [PATCH v1 00/15] support ADC and I2C for AST2700

[Cédric Le Goater]

On 7/18/24 18:18, Cédric Le Goater wrote:
> On 7/18/24 08:49, Jamin Lin wrote:
>> v1:
>> 1. support ADC for AST2700
>> 2. support I2C for AST2700
>>
>> Jamin Lin (15):
>>    aspeed/adc: Add AST2700 support
>>    aspeed/soc: support ADC for AST2700
>>    hw/i2c/aspeed: support to set the different memory size
>>    hw/i2c/aspeed: support discontinuous register memory region of I2C bus
>>    hw/i2c/aspeed: rename the I2C class pool attribute to share_pool
>>    hw/i2c/aspeed: introduce a new bus pool buffer attribute in
>>      AspeedI2Cbus
>>    hw/i2c/aspeed: support discontinuous poll buffer memory region of I2C
>>      bus
>>    hw/i2c/aspeed: introduce a new dma_dram_offset attribute in
>>      AspeedI2Cbus
>>    hw/i2c/aspeed: Add AST2700 support
>>    hw/i2c/aspeed: support Tx/Rx buffer 64 bits address
>>    hw/i2c/aspeed: support high part dram offset for DMA 64 bits
>>    aspeed/soc: introduce a new API to get the INTC orgate information
>>    aspeed/soc: support I2C for AST2700
>>    aspeed: fix coding style
>>    aspeed: add tmp105 in i2c bus 0 for AST2700
>>
>>   hw/adc/aspeed_adc.c         |  16 ++
>>   hw/arm/aspeed.c             |  31 +++-
>>   hw/arm/aspeed_ast27x0.c     |  65 +++++++
>>   hw/i2c/aspeed_i2c.c         | 340 ++++++++++++++++++++++++++++++------
>>   include/hw/adc/aspeed_adc.h |   1 +
>>   include/hw/i2c/aspeed_i2c.h |  34 ++--
>>   6 files changed, 418 insertions(+), 69 deletions(-)
>>
> 
> 
> 
> Applied 1-2 to aspeed-next.


Applied 3,5,6,14 to aspeed-next.

Thanks,

C.