From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51976) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b3a9V-0004N0-57 for qemu-devel@nongnu.org; Thu, 19 May 2016 22:27:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b3a9P-0007j4-4J for qemu-devel@nongnu.org; Thu, 19 May 2016 22:27:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37615) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b3a9O-0007j0-SL for qemu-devel@nongnu.org; Thu, 19 May 2016 22:26:55 -0400 References: <2340f45a659a3f9a17307eaba0f44f9ebfdc62c3.1463609668.git.jcd@tribudubois.net> <573D32C2.20609@redhat.com> <573D58DB.3050505@tribudubois.net> From: Jason Wang Message-ID: <573E75E8.6070800@redhat.com> Date: Fri, 20 May 2016 10:26:48 +0800 MIME-Version: 1.0 In-Reply-To: <573D58DB.3050505@tribudubois.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 6/8] i.MX: move FEC device to a register array structure. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jean-Christophe DUBOIS Cc: qemu-devel@nongnu.org, "peter.maydell@linaro.org >> Peter Maydell" On 2016=E5=B9=B405=E6=9C=8819=E6=97=A5 14:10, Jean-Christophe DUBOIS wrot= e: > Le 19/05/2016 05:28, Jason Wang a =C3=A9crit : >> >> >> On 2016=E5=B9=B405=E6=9C=8819=E6=97=A5 06:23, Jean-Christophe Dubois w= rote: >>> This is to prepare for the ENET Gb device of the i.MX6. >>> >>> Signed-off-by: Jean-Christophe Dubois >>> --- >>> >>> Changes since v1: >>> * Not present on v1. >>> >>> Changes since v2: >>> * The patch was split in 2 parts: >>> - a "port" to a reg array based device (this patch). >>> - the addition of the Gb support (next patch). >>> Changes since v3: >>> * Small fix patches were extracted from this patch (see previous 3=20 >>> patches) >>> * Reset logic through ECR was fixed. >>> * TDAR/RDAR behavior was fixed. >>> >>> hw/net/imx_fec.c | 396=20 >>> ++++++++++++++++++++++++++--------------------- >>> include/hw/net/imx_fec.h | 55 ++++--- >>> 2 files changed, 258 insertions(+), 193 deletions(-) >>> >>> [...] >>> - case 0x014: /* TDAR */ >>> - if (s->ecr & FEC_EN) { >>> + case ENET_TDAR: >>> + if (s->regs[ENET_ECR] & FEC_EN) { >>> + s->regs[index] =3D ENET_TDAR_TDAR; >>> imx_fec_do_tx(s); >>> } >>> + s->regs[index] =3D 0; >>> break; >>> - case 0x024: /* ECR */ >>> - s->ecr =3D value; >>> + case ENET_ECR: >>> if (value & FEC_RESET) { >>> - imx_fec_reset(DEVICE(s)); >>> + return imx_fec_reset(DEVICE(s)); >>> } >>> - if ((s->ecr & FEC_EN) =3D=3D 0) { >>> - s->rx_enabled =3D 0; >>> + s->regs[index] =3D value; >>> + if ((s->regs[index] & FEC_EN) =3D=3D 0) { >>> + s->regs[ENET_RDAR] =3D 0; >>> + s->rx_descriptor =3D s->regs[ENET_RDSR]; >>> + s->regs[ENET_TDAR] =3D 0; >>> + s->tx_descriptor =3D s->regs[ENET_TDSR]; >> >> This seems like a new behavior, is this a bug fix? If yes, better spli= t. > > It is a more correct behavior I think. Note however that our guest=20 > OSes (in particular Linux) do not trigger this bit. So it is mostly=20 > untested/unused. > Is this the real hardware or documented behavior? If yes, need a=20 separate patch for this. If not, we'd better not add this. >> >>> } >>> break; >>> - case 0x040: /* MMFR */ >>> - /* store the value */ >>> - s->mmfr =3D value; >>> + case ENET_MMFR: >>> + s->regs[index] =3D value; >>> if (extract32(value, 29, 1)) { >>> - s->mmfr =3D do_phy_read(s, extract32(value, 18, 10)); >>> + /* This is a read operation */ >>> + s->regs[ENET_MMFR] =3D deposit32(s->regs[ENET_MMFR], 0, = 16, >>> + do_phy_read(s, >>> + extract32(value, >>> + 18, 10))); >>> } else { >>> + /* This a write operation */ >>> do_phy_write(s, extract32(value, 18, 10),=20 >>> extract32(value, 0, 16)); >>> } >>> /* raise the interrupt as the PHY operation is done */ >>> - s->eir |=3D FEC_INT_MII; >>> + s->regs[ENET_EIR] |=3D FEC_INT_MII; >>> break; >>> - case 0x044: /* MSCR */ >>> - s->mscr =3D value & 0xfe; >>> + case ENET_MSCR: >>> + s->regs[index] =3D value & 0xfe; >>> break; >>> - case 0x064: /* MIBC */ >>> + case ENET_MIBC: >>> /* TODO: Implement MIB. */ >>> - s->mibc =3D (value & 0x80000000) ? 0xc0000000 : 0; >>> + s->regs[index] =3D (value & 0x80000000) ? 0xc0000000 : 0; >>> break; >>> - case 0x084: /* RCR */ >>> - s->rcr =3D value & 0x07ff003f; >>> + case ENET_RCR: >>> + s->regs[index] =3D value & 0x07ff003f; >>> /* TODO: Implement LOOP mode. */ >>> break; >>> - case 0x0c4: /* TCR */ >>> + case ENET_TCR: >>> /* We transmit immediately, so raise GRA immediately. */ >>> - s->tcr =3D value; >>> + s->regs[index] =3D value; >>> if (value & 1) { >>> - s->eir |=3D FEC_INT_GRA; >>> + s->regs[ENET_EIR] |=3D FEC_INT_GRA; >>> } >>> break; >>> - case 0x0e4: /* PALR */ >>> + case ENET_PALR: >>> + s->regs[index] =3D value; >>> s->conf.macaddr.a[0] =3D value >> 24; >>> s->conf.macaddr.a[1] =3D value >> 16; >>> s->conf.macaddr.a[2] =3D value >> 8; >>> s->conf.macaddr.a[3] =3D value; >> >> I believe we should use stl_be_p() here? > > I didn't change this bit ... > Sorry, you are right. I misread the patch. >> >>> break; >>> - case 0x0e8: /* PAUR */ >>> + case ENET_PAUR: >>> + s->regs[index] =3D (value | 0x0000ffff) & 0xffff8808; >>> s->conf.macaddr.a[4] =3D value >> 24; >>> s->conf.macaddr.a[5] =3D value >> 16; >>> break; >>> - case 0x0ec: /* OPDR */ [...] >>> diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h >>> index cbf8650..709f8a0 100644 >>> --- a/include/hw/net/imx_fec.h >>> +++ b/include/hw/net/imx_fec.h >>> @@ -30,6 +30,33 @@ >>> #include "hw/sysbus.h" >>> #include "net/net.h" >>> +#define ENET_EIR 1 >>> +#define ENET_EIMR 2 >>> +#define ENET_RDAR 4 >>> +#define ENET_TDAR 5 >>> +#define ENET_ECR 9 >>> +#define ENET_MMFR 16 >>> +#define ENET_MSCR 17 >>> +#define ENET_MIBC 25 >>> +#define ENET_RCR 33 >>> +#define ENET_TCR 49 >>> +#define ENET_PALR 57 >>> +#define ENET_PAUR 58 >>> +#define ENET_OPD 59 >>> +#define ENET_IAUR 70 >>> +#define ENET_IALR 71 >>> +#define ENET_GAUR 72 >>> +#define ENET_GALR 73 >>> +#define ENET_TFWR 81 >>> +#define ENET_FRBR 83 >>> +#define ENET_FRSR 84 >>> +#define ENET_RDSR 96 >>> +#define ENET_TDSR 97 >>> +#define ENET_MRBR 98 >>> +#define ENET_MIIGSK_CFGR 192 >>> +#define ENET_MIIGSK_ENR 194 >>> +#define ENET_MAX 400 >> >> Is this an arbitrary value or there's a plan to add more register=20 >> whose index is greater than 192? > > More registers are coming with the ENET device. > Right, I see. Thanks >> >>> + >>> #define FEC_MAX_FRAME_SIZE 2032 >>> #define FEC_INT_HB (1 << 31) >>> @@ -46,8 +73,14 @@ >>> #define FEC_INT_RL (1 << 20) >>> #define FEC_INT_UN (1 << 19) >>> -#define FEC_EN 2 >>> -#define FEC_RESET 1 >>> +/* RDAR */ >>> +#define ENET_RDAR_RDAR (1 << 24) >>> + >>> +/* TDAR */ >>> +#define ENET_TDAR_TDAR (1 << 24) >>> + >>> +#define FEC_EN (1 << 1) >>> +#define FEC_RESET (1 << 0) >>> /* Buffer Descriptor. */ >>> typedef struct { >>> @@ -83,25 +116,9 @@ typedef struct IMXFECState { >>> qemu_irq irq; >>> MemoryRegion iomem; >>> - uint32_t irq_state; >>> - uint32_t eir; >>> - uint32_t eimr; >>> - uint32_t rx_enabled; >>> + uint32_t regs[ENET_MAX]; >>> uint32_t rx_descriptor; >>> uint32_t tx_descriptor; >>> - uint32_t ecr; >>> - uint32_t mmfr; >>> - uint32_t mscr; >>> - uint32_t mibc; >>> - uint32_t rcr; >>> - uint32_t tcr; >>> - uint32_t tfwr; >>> - uint32_t frsr; >>> - uint32_t erdsr; >>> - uint32_t etdsr; >>> - uint32_t emrbr; >>> - uint32_t miigsk_cfgr; >>> - uint32_t miigsk_enr; >>> uint32_t phy_status; >>> uint32_t phy_control; >> >> > >