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

[Jamin Lin]

Hi Cedric,

> 
> On 7/3/24 10:16 AM, 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 "Normal Priority Transmit Ring Base Address Register
> > High(0x17C)", "High Priority Transmit Ring Base Address Register
> > High(0x184)" and "Receive Ring Base Address Register High(0x18C)" to
> > save the high part physical address of descriptor manager.
> > Ex: TX descriptor manager address [34:0] The "Normal Priority Transmit
> > Ring Base Address Register High(0x17C)"
> > bits [2:0] which corresponds the bits [34:32] of the 64 bits address
> > of the TX ring buffer address.
> > The "Normal Priority Transmit Ring Base Address Register(0x20)" bits
> > [31:0] which corresponds the bits [31:0] of the 64 bits address of the
> > TX ring buffer address.
> >
> > Besides, it have "TXDES 2" and "RXDES 2"
> > to save the high part physical address of packet buffer.
> > Ex: TX packet buffer address [34:0]
> > The "TXDES 2" bits [18:16] which corresponds the bits [34:32] of the
> > 64 bits address of the TX packet buffer address and "TXDES 3" bits
> > [31:0] which corresponds the bits [31:0] of the 64 bits address of the
> > TX packet buffer address.
> >
> > Update TX/RX ring and descriptor data type to uint64_t and supports
> > TX/RX ring, descriptor and packet buffers
> > 64 bits address for all ASPEED SOCs models.
> >
> > Incrementing the version of vmstate to 2.
> >
> > Introduce a new class(ftgmac100_high), class attribute and memop
> > handlers, the realize handler instantiate a new memory region and map
> > it on top of the current one to read/write FTGMAC100_*_HIGH regs.
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> >   hw/net/ftgmac100.c         | 156
> ++++++++++++++++++++++++++++++++-----
> >   include/hw/net/ftgmac100.h |  24 ++++--
> >   2 files changed, 151 insertions(+), 29 deletions(-)
> >
> > diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c index
> > 4e88430b2f..3d13f54efc 100644
> > --- a/hw/net/ftgmac100.c
> > +++ b/hw/net/ftgmac100.c
> > @@ -56,6 +56,16 @@
> >   #define FTGMAC100_PHYDATA         0x64
> >   #define FTGMAC100_FCR             0x68
> >
> > +/*
> > + * FTGMAC100 High registers
> > + *
> > + * values below are offset by - FTGMAC100_HIGH_OFFSET from datasheet
> > + * because its memory region is start at FTGMAC100_HIGH_OFFSET  */
> > +#define FTGMAC100_NPTXR_BADR_HIGH   (0x17C -
> FTGMAC100_HIGH_OFFSET)
> > +#define FTGMAC100_HPTXR_BADR_HIGH   (0x184 -
> FTGMAC100_HIGH_OFFSET)
> > +#define FTGMAC100_RXR_BADR_HIGH     (0x18C -
> FTGMAC100_HIGH_OFFSET)
> > +
> >   /*
> >    * Interrupt status register & interrupt enable register
> >    */
> > @@ -165,6 +175,8 @@
> >   #define FTGMAC100_TXDES1_TX2FIC          (1 << 30)
> >   #define FTGMAC100_TXDES1_TXIC            (1 << 31)
> >
> > +#define FTGMAC100_TXDES2_TXBUF_BADR_HI(x)   (((x) >> 16) & 0x7)
> > +
> >   /*
> >    * Receive descriptor
> >    */
> > @@ -198,13 +210,15 @@
> >   #define FTGMAC100_RXDES1_UDP_CHKSUM_ERR  (1 << 26)
> >   #define FTGMAC100_RXDES1_IP_CHKSUM_ERR   (1 << 27)
> >
> > +#define FTGMAC100_RXDES2_RXBUF_BADR_HI(x)   (((x) >> 16) & 0x7)
> > +
> >   /*
> >    * Receive and transmit Buffer Descriptor
> >    */
> >   typedef struct {
> >       uint32_t        des0;
> >       uint32_t        des1;
> > -    uint32_t        des2;        /* not used by HW */
> > +    uint32_t        des2;        /* used by HW high address */
> >       uint32_t        des3;
> >   } FTGMAC100Desc;
> >
> > @@ -515,12 +529,14 @@ out:
> >       return frame_size;
> >   }
> >
> > -static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
> > -                            uint32_t tx_descriptor)
> > +static void ftgmac100_do_tx(FTGMAC100State *s, uint64_t tx_ring,
> > +                            uint64_t tx_descriptor)
> >   {
> > +    FTGMAC100Class *fc = FTGMAC100_GET_CLASS(s);
> >       int frame_size = 0;
> >       uint8_t *ptr = s->frame;
> > -    uint32_t addr = tx_descriptor;
> > +    uint64_t addr = tx_descriptor;
> > +    uint64_t buf_addr = 0;
> 
> To ease reading, I would extract in a standalone patch the part converting
> addresses to 64 bits.
Will create a new patch
> 
> >       uint32_t flags = 0;
> >
> >       while (1) {
> > @@ -559,7 +575,12 @@ static void ftgmac100_do_tx(FTGMAC100State *s,
> uint32_t tx_ring,
> >               len =  sizeof(s->frame) - frame_size;
> >           }
> >
> > -        if (dma_memory_read(&address_space_memory, bd.des3,
> > +        buf_addr = bd.des3;
> > +        if (fc->is_dma64) {
> > +            buf_addr = deposit64(buf_addr, 32, 32,
> > +
> FTGMAC100_TXDES2_TXBUF_BADR_HI(bd.des2));
> > +        }
> > +        if (dma_memory_read(&address_space_memory, buf_addr,
> >                               ptr, len, MEMTXATTRS_UNSPECIFIED)) {
> >               qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to read
> packet @ 0x%x\n",
> >                             __func__, bd.des3); @@ -726,9 +747,9
> @@
> > static uint64_t ftgmac100_read(void *opaque, hwaddr addr, unsigned size)
> >       case FTGMAC100_MATH1:
> >           return s->math[1];
> >       case FTGMAC100_RXR_BADR:
> > -        return s->rx_ring;
> > +        return extract64(s->rx_ring, 0, 32);
> >       case FTGMAC100_NPTXR_BADR:
> > -        return s->tx_ring;
> > +        return extract64(s->tx_ring, 0, 32);
> >       case FTGMAC100_ITC:
> >           return s->itc;
> >       case FTGMAC100_DBLAC:
> > @@ -799,9 +820,8 @@ static void ftgmac100_write(void *opaque, hwaddr
> addr,
> >                             HWADDR_PRIx "\n", __func__, value);
> >               return;
> >           }
> > -
> > -        s->rx_ring = value;
> > -        s->rx_descriptor = s->rx_ring;
> > +        s->rx_ring = deposit64(s->rx_ring, 0, 32, value);
> > +        s->rx_descriptor = deposit64(s->rx_descriptor, 0, 32, value);
> >           break;
> >
> >       case FTGMAC100_RBSR: /* DMA buffer size */ @@ -814,8 +834,8
> @@
> > static void ftgmac100_write(void *opaque, hwaddr addr,
> >                             HWADDR_PRIx "\n", __func__, value);
> >               return;
> >           }
> > -        s->tx_ring = value;
> > -        s->tx_descriptor = s->tx_ring;
> > +        s->tx_ring = deposit64(s->tx_ring, 0, 32, value);
> > +        s->tx_descriptor = deposit64(s->tx_descriptor, 0, 32, value);
> >           break;
> >
> >       case FTGMAC100_NPTXPD: /* Trigger transmit */ @@ -914,6
> +934,60
> > @@ static void ftgmac100_write(void *opaque, hwaddr addr,
> >       ftgmac100_update_irq(s);
> >   }
> >
> > +static uint64_t ftgmac100_high_read(void *opaque, hwaddr addr,
> > +unsigned size) {
> > +    FTGMAC100State *s = FTGMAC100(opaque);
> > +    uint64_t val = 0;
> > +
> > +    switch (addr) {
> > +    case FTGMAC100_NPTXR_BADR_HIGH:
> > +        val = extract64(s->tx_ring, 32, 32);
> > +        break;
> > +    case FTGMAC100_HPTXR_BADR_HIGH:
> > +        /* High Priority Transmit Ring Base High Address */
> > +        qemu_log_mask(LOG_UNIMP, "%s: read to unimplemented
> register 0x%"
> > +                      HWADDR_PRIx "\n", __func__, addr);
> > +        break;
> > +    case FTGMAC100_RXR_BADR_HIGH:
> > +        val = extract64(s->rx_ring, 32, 32);
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad address at offset
> 0x%"
> > +                      HWADDR_PRIx "\n", __func__, addr);
> > +        break;
> > +    }
> > +
> > +    return val;
> > +}
> > +
> > +static void ftgmac100_high_write(void *opaque, hwaddr addr,
> > +                          uint64_t value, unsigned size) {
> > +    FTGMAC100State *s = FTGMAC100(opaque);
> > +
> > +    switch (addr) {
> > +    case FTGMAC100_NPTXR_BADR_HIGH:
> > +        s->tx_ring = deposit64(s->tx_ring, 32, 32, value);
> > +        s->tx_descriptor = deposit64(s->tx_descriptor, 32, 32, value);
> > +        break;
> > +    case FTGMAC100_HPTXR_BADR_HIGH:
> > +        /* High Priority Transmit Ring Base High Address */
> > +        qemu_log_mask(LOG_UNIMP, "%s: write to unimplemented
> register 0x%"
> > +                      HWADDR_PRIx "\n", __func__, addr);
> > +        break;
> > +    case FTGMAC100_RXR_BADR_HIGH:
> > +        s->rx_ring = deposit64(s->rx_ring, 32, 32, value);
> > +        s->rx_descriptor = deposit64(s->rx_descriptor, 32, 32, value);
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad address at offset
> 0x%"
> > +                      HWADDR_PRIx "\n", __func__, addr);
> > +        break;
> > +    }
> > +
> > +    ftgmac100_update_irq(s);
> > +}
> > +
> >   static int ftgmac100_filter(FTGMAC100State *s, const uint8_t *buf, size_t
> len)
> >   {
> >       unsigned mcast_idx;
> > @@ -955,11 +1029,12 @@ static ssize_t ftgmac100_receive(NetClientState
> *nc, const uint8_t *buf,
> >                                    size_t len)
> >   {
> >       FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc));
> > +    FTGMAC100Class *fc = FTGMAC100_GET_CLASS(s);
> >       FTGMAC100Desc bd;
> >       uint32_t flags = 0;
> > -    uint32_t addr;
> > +    uint64_t addr;
> >       uint32_t crc;
> > -    uint32_t buf_addr;
> > +    uint64_t buf_addr = 0;
> >       uint8_t *crc_ptr;
> >       uint32_t buf_len;
> >       size_t size = len;
> > @@ -1024,7 +1099,12 @@ static ssize_t ftgmac100_receive(NetClientState
> *nc, const uint8_t *buf,
> >           if (size < 4) {
> >               buf_len += size - 4;
> >           }
> > +
> >           buf_addr = bd.des3;
> > +        if (fc->is_dma64) {
> > +            buf_addr = deposit64(buf_addr, 32, 32,
> > +
> FTGMAC100_RXDES2_RXBUF_BADR_HI(bd.des2));
> > +        }
> >           if (first && proto == ETH_P_VLAN && buf_len >= 18) {
> >               bd.des1 = lduw_be_p(buf + 14) |
> > FTGMAC100_RXDES1_VLANTAG_AVAIL;
> >
> > @@ -1078,6 +1158,14 @@ static const MemoryRegionOps ftgmac100_ops =
> {
> >       .endianness = DEVICE_LITTLE_ENDIAN,
> >   };
> >
> > +static const MemoryRegionOps ftgmac100_high_ops = {
> > +    .read = ftgmac100_high_read,
> > +    .write = ftgmac100_high_write,
> > +    .valid.min_access_size = 4,
> > +    .valid.max_access_size = 4,
> > +    .endianness = DEVICE_LITTLE_ENDIAN, };
> > +
> >   static void ftgmac100_cleanup(NetClientState *nc)
> >   {
> >       FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc));
> > @@ -1097,6 +1185,7 @@ static NetClientInfo net_ftgmac100_info = {
> >   static void ftgmac100_realize(DeviceState *dev, Error **errp)
> >   {
> >       FTGMAC100State *s = FTGMAC100(dev);
> > +    FTGMAC100Class *fc = FTGMAC100_GET_CLASS(s);
> >       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> >
> >       if (s->aspeed) {
> > @@ -1107,9 +1196,17 @@ static void ftgmac100_realize(DeviceState *dev,
> Error **errp)
> >           s->rxdes0_edorr = FTGMAC100_RXDES0_EDORR;
> >       }
> >
> > -    memory_region_init_io(&s->iomem, OBJECT(dev), &ftgmac100_ops, s,
> > +    memory_region_init_io(&s->iomem, OBJECT(s), &ftgmac100_ops, s,
> >                             TYPE_FTGMAC100,
> FTGMAC100_NR_REGS);
> >       sysbus_init_mmio(sbd, &s->iomem);
> > +
> > +    if (fc->is_dma64) {
> > +        memory_region_init_io(&s->iomem_high, OBJECT(s),
> &ftgmac100_high_ops,
> > +                              s, TYPE_FTGMAC100_HIGH,
> FTGMAC100_HIGH_NR_REGS);
> > +        memory_region_add_subregion(&s->iomem,
> FTGMAC100_HIGH_OFFSET,
> > +                                    &s->iomem_high);
> > +    }
> 
> 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
#define FTGMAC100_NR_REGS 0x100
#define FTGMAC100_REGS_HIGH_OFFSET 0x100
#define FTGMAC100_NR_REGS_HIGH 0x100

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);
    }
}

> >       sysbus_init_irq(sbd, &s->irq);
> >       qemu_macaddr_default_if_unset(&s->conf.macaddr);
> >
> > @@ -1121,18 +1218,14 @@ static void ftgmac100_realize(DeviceState
> > *dev, Error **errp)
> >
> >   static const VMStateDescription vmstate_ftgmac100 = {
> >       .name = TYPE_FTGMAC100,
> > -    .version_id = 1,
> > -    .minimum_version_id = 1,
> > +    .version_id = 2,
> > +    .minimum_version_id = 2,
> >       .fields = (const VMStateField[]) {
> >           VMSTATE_UINT32(irq_state, FTGMAC100State),
> >           VMSTATE_UINT32(isr, FTGMAC100State),
> >           VMSTATE_UINT32(ier, FTGMAC100State),
> >           VMSTATE_UINT32(rx_enabled, FTGMAC100State),
> > -        VMSTATE_UINT32(rx_ring, FTGMAC100State),
> >           VMSTATE_UINT32(rbsr, FTGMAC100State),
> > -        VMSTATE_UINT32(tx_ring, FTGMAC100State),
> > -        VMSTATE_UINT32(rx_descriptor, FTGMAC100State),
> > -        VMSTATE_UINT32(tx_descriptor, FTGMAC100State),
> >           VMSTATE_UINT32_ARRAY(math, FTGMAC100State, 2),
> >           VMSTATE_UINT32(itc, FTGMAC100State),
> >           VMSTATE_UINT32(aptcr, FTGMAC100State), @@ -1151,6
> +1244,10
> > @@ static const VMStateDescription vmstate_ftgmac100 = {
> >           VMSTATE_UINT32(phy_int_mask, FTGMAC100State),
> >           VMSTATE_UINT32(txdes0_edotr, FTGMAC100State),
> >           VMSTATE_UINT32(rxdes0_edorr, FTGMAC100State),
> > +        VMSTATE_UINT64(rx_ring, FTGMAC100State),
> > +        VMSTATE_UINT64(tx_ring, FTGMAC100State),
> > +        VMSTATE_UINT64(rx_descriptor, FTGMAC100State),
> > +        VMSTATE_UINT64(tx_descriptor, FTGMAC100State),
> >           VMSTATE_END_OF_LIST()
> >       }
> >   };
> > @@ -1180,6 +1277,22 @@ static const TypeInfo ftgmac100_info = {
> >       .class_init = ftgmac100_class_init,
> >   };
> >
> > +static void ftgmac100_high_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    FTGMAC100Class *fc = FTGMAC100_CLASS(klass);
> > +
> > +    dc->desc = "Faraday FTGMAC100 Gigabit Ethernet High emulation";
> > +    fc->is_dma64 = true;
> > +}
> 
> Using a property to activate the region for new registers, instead of a class,
> would simplify the changes IMO. Let's call the property "dma64" ?
> 
Thanks for suggestion.
Will change to use object attribute and property instead of a class

Thanks-Jamin

> Thanks,
> 
> C.
> 
> 
> 
> > +
> > +static const TypeInfo ftgmac100_high_info = {
> > +    .name = TYPE_FTGMAC100_HIGH,
> > +    .parent = TYPE_FTGMAC100,
> > +    .instance_size = sizeof(FTGMAC100State),
> > +    .class_init = ftgmac100_high_class_init, };
> > +
> >   /*
> >    * AST2600 MII controller
> >    */
> > @@ -1342,6 +1455,7 @@ static const TypeInfo aspeed_mii_info = {
> >   static void ftgmac100_register_types(void)
> >   {
> >       type_register_static(&ftgmac100_info);
> > +    type_register_static(&ftgmac100_high_info);
> >       type_register_static(&aspeed_mii_info);
> >   }
> >
> > diff --git a/include/hw/net/ftgmac100.h b/include/hw/net/ftgmac100.h
> > index 5a970676da..3e44ed97c9 100644
> > --- a/include/hw/net/ftgmac100.h
> > +++ b/include/hw/net/ftgmac100.h
> > @@ -12,13 +12,15 @@
> >   #include "qom/object.h"
> >
> >   #define TYPE_FTGMAC100 "ftgmac100"
> > -OBJECT_DECLARE_SIMPLE_TYPE(FTGMAC100State, FTGMAC100)
> > +#define TYPE_FTGMAC100_HIGH "ftgmac100_high"
> > +OBJECT_DECLARE_TYPE(FTGMAC100State, FTGMAC100Class, FTGMAC100)
> >
> > -#define FTGMAC100_NR_REGS   0x200
> > +#define FTGMAC100_NR_REGS 0x200
> > +#define FTGMAC100_HIGH_OFFSET 0x100
> > +#define FTGMAC100_HIGH_NR_REGS 0x100
> >
> >   #include "hw/sysbus.h"
> >   #include "net/net.h"
> > -
> >   /*
> >    * Max frame size for the receiving buffer
> >    */
> > @@ -33,6 +35,7 @@ struct FTGMAC100State {
> >       NICConf conf;
> >       qemu_irq irq;
> >       MemoryRegion iomem;
> > +    MemoryRegion iomem_high;
> >
> >       uint8_t frame[FTGMAC100_MAX_FRAME_SIZE];
> >
> > @@ -40,10 +43,6 @@ struct FTGMAC100State {
> >       uint32_t isr;
> >       uint32_t ier;
> >       uint32_t rx_enabled;
> > -    uint32_t rx_ring;
> > -    uint32_t rx_descriptor;
> > -    uint32_t tx_ring;
> > -    uint32_t tx_descriptor;
> >       uint32_t math[2];
> >       uint32_t rbsr;
> >       uint32_t itc;
> > @@ -56,7 +55,10 @@ struct FTGMAC100State {
> >       uint32_t phycr;
> >       uint32_t phydata;
> >       uint32_t fcr;
> > -
> > +    uint64_t rx_ring;
> > +    uint64_t rx_descriptor;
> > +    uint64_t tx_ring;
> > +    uint64_t tx_descriptor;
> >
> >       uint32_t phy_status;
> >       uint32_t phy_control;
> > @@ -69,6 +71,12 @@ struct FTGMAC100State {
> >       uint32_t rxdes0_edorr;
> >   };
> >
> > +struct FTGMAC100Class {
> > +    SysBusDeviceClass parent_class;
> > +
> > +    bool is_dma64;
> > +};
> > +
> >   #define TYPE_ASPEED_MII "aspeed-mmi"
> >   OBJECT_DECLARE_SIMPLE_TYPE(AspeedMiiState, ASPEED_MII)
> >