[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v2 2/5] hw/net:ftgmac100: support 64 bits dma dram address fo
|
From: |
Jamin Lin |
|
Subject: |
RE: [PATCH v2 2/5] hw/net:ftgmac100: support 64 bits dma dram address for AST2700 |
|
Date: |
Thu, 4 Jul 2024 02:57:07 +0000 |
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)
> >