Re: [PATCH v2 2/5] hw/net:ftgmac100: support 64 bits dma dram address for AST2700

[Cédric Le Goater]

Hello Jamin,

> > I think that, first, we should introduce a container region. In this container
> > region would be mapped a sub region for the current set of registers. This
> > container region would be the one that the SoC maps as it is done today.
> >
> > Then, in a second patch, we should introduce a extra sub region for the set of
> > new registers and map it at 0x100 in the container region.
> >
> > It is close to what you did but it lacks an overall container region.
> > This container region should be sized as specified in the datasheet.
> Do you mean to change as following?
>
> ftgmac100.h
> #define FTGMAC100_MEM_SIZE 0x200

I would use the total size of the MMIO aperture, 0x1000, because
this region is reserved for the MAC unit logic, which means it
could grow. That's minor.

> #define FTGMAC100_NR_REGS 0x100

Value is fine.

However, the NR_REGS suffix is confusing. It is not a number of
registers but a MMIO aperture width. I would use a _MEM_SIZE suffix
instead. Could be FTGMAC100_REG_MEM_SIZE.


> #define FTGMAC100_REGS_HIGH_OFFSET 0x100
> #define FTGMAC100_NR_REGS_HIGH 0x100

Same here.

> struct FTGMAC100State {
>      MemoryRegion iomem_container;
>      MemoryRegion iomem;
>      MemoryRegion iomem_high;
> }
>
> Ftgmac100.c
> static void ftgmac100_realize(DeviceState *dev, Error **errp)
> {
>      memory_region_init(&s->iomem_container, OBJECT(s),
>                       TYPE_FTGMAC100 ".container", FTGMAC100_MEM_SIZE);  --> 
> container size 0x200
>      sysbus_init_mmio(sbd, &s->iomem_container);
>
>      memory_region_init_io(&s->iomem, OBJECT(s), &ftgmac100_ops, s,
>                            TYPE_FTGMAC100 ".regs", FTGMAC100_NR_REGS); --> 
> current register 0x0-0xff
>      memory_region_add_subregion(&s->iomem_container, 0x0, &s->iomem);
>
>      if (s-> dma64) {
>          memory_region_init_io(&s->iomem_high, OBJECT(s), &ftgmac100_high_ops,
>                                s, TYPE_FTGMAC100 ".regs.high", 
> FTGMAC100_NR_REGS_HIGH); --> high register 0x100-0x1ff
>          memory_region_add_subregion(&s->iomem_container, 
> FTGMAC100_REGS_HIGH_OFFSET,
>                                      &s->iomem_high);
>      }
> }

Looks good.

Thanks,

C.