improve the PHY implementation with more generic code.
This patch remove a lot of harcoded values to replace them with
generic symbols from header files.
Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
---
v2: Not present
v3: Not present
v4: Not present
v5: improve PHY implementation.
hw/net/imx_fec.c | 76 +++++++++++++++++++++++++++-----------------
include/hw/net/mii.h | 4 +++
2 files changed, 50 insertions(+), 30 deletions(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 29e613699ee..bf9583a93f4 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -24,6 +24,7 @@
#include "qemu/osdep.h"
#include "hw/irq.h"
#include "hw/net/imx_fec.h"
+#include "hw/net/mii.h"
#include "hw/qdev-properties.h"
#include "migration/vmstate.h"
#include "sysemu/dma.h"
@@ -231,6 +232,9 @@ static const VMStateDescription vmstate_imx_eth = {
#define PHY_INT_PARFAULT (1 << 2)
#define PHY_INT_AUTONEG_PAGE (1 << 1)
+#define MII_SMC911X_ISF 29
+#define MII_SMC911X_IM 30
+
static void imx_eth_update(IMXFECState *s);
/*
@@ -249,11 +253,11 @@ static void imx_phy_update_link(IMXFECState *s)
/* Autonegotiation status mirrors link status. */
if (qemu_get_queue(s->nic)->link_down) {
trace_imx_phy_update_link("down");
- s->phy_status &= ~0x0024;
+ s->phy_status &= ~(MII_BMSR_LINK_ST | MII_BMSR_AN_COMP);
s->phy_int |= PHY_INT_DOWN;
} else {
trace_imx_phy_update_link("up");
- s->phy_status |= 0x0024;
+ s->phy_status |= MII_BMSR_LINK_ST | MII_BMSR_AN_COMP;
s->phy_int |= PHY_INT_ENERGYON;
s->phy_int |= PHY_INT_AUTONEG_COMPLETE;
}
@@ -269,9 +273,11 @@ static void imx_phy_reset(IMXFECState *s)
{
trace_imx_phy_reset();
- s->phy_status = 0x7809;
- s->phy_control = 0x3000;
- s->phy_advertise = 0x01e1;
+ s->phy_status = MII_BMSR_100TX_FD | MII_BMSR_100TX_HD | MII_BMSR_10T_FD |
+ MII_BMSR_10T_HD | MII_BMSR_AUTONEG | MII_BMSR_EXTCAP;
+ s->phy_control = MII_BMCR_AUTOEN | MII_BMCR_SPEED100;
+ s->phy_advertise = MII_ANAR_CSMACD | MII_ANAR_TX | MII_ANAR_10FD |
+ MII_ANAR_10 | MII_ANAR_TXFD;
s->phy_int_mask = 0;
s->phy_int = 0;
imx_phy_update_link(s);
@@ -285,37 +291,42 @@ static uint32_t imx_phy_read(IMXFECState *s, int reg)
reg %= 32;
switch (reg) {
- case 0: /* Basic Control */
+ case MII_BMCR: /* Basic Control */
val = s->phy_control;
break;
- case 1: /* Basic Status */
+ case MII_BMSR: /* Basic Status */
val = s->phy_status;
break;
- case 2: /* ID1 */
- val = 0x0007;
+ case MII_PHYID1: /* ID1 */
+ val = LAN911x_PHYID1;
break;
- case 3: /* ID2 */
- val = 0xc0d1;
+ case MII_PHYID2: /* ID2 */
+ val = LAN911x_PHYID2;
break;
- case 4: /* Auto-neg advertisement */
+ case MII_ANAR: /* Auto-neg advertisement */
val = s->phy_advertise;
break;
- case 5: /* Auto-neg Link Partner Ability */
- val = 0x0f71;
+ case MII_ANLPAR: /* Auto-neg Link Partner Ability */
+ val = MII_ANLPAR_CSMACD | MII_ANLPAR_10 | MII_ANLPAR_10FD |
+ MII_ANLPAR_TX | MII_ANLPAR_TXFD | MII_ANLPAR_PAUSE |
+ MII_ANLPAR_PAUSEASY;
break;
- case 6: /* Auto-neg Expansion */
- val = 1;
+ case MII_ANER: /* Auto-neg Expansion */
+ val = MII_ANER_NWAY;
break;
- case 29: /* Interrupt source. */
+ case MII_SMC911X_ISF: /* Interrupt source. */
val = s->phy_int;
s->phy_int = 0;
imx_phy_update_irq(s);
break;
- case 30: /* Interrupt mask */
+ case MII_SMC911X_IM: /* Interrupt mask */
val = s->phy_int_mask;
break;
- case 17:
- case 18:
+ case MII_NSR:
+ val = 1 << 6;
+ break;
+ case MII_LBREMR:
+ case MII_REC:
case 27:
case 31:
qemu_log_mask(LOG_UNIMP, "[%s.phy]%s: reg %d not implemented\n",
@@ -343,26 +354,31 @@ static void imx_phy_write(IMXFECState *s, int reg, uint32_t val)
trace_imx_phy_write(val, phy, reg);
switch (reg) {
- case 0: /* Basic Control */
- if (val & 0x8000) {
+ case MII_BMCR: /* Basic Control */
+ if (val & MII_BMCR_RESET) {
imx_phy_reset(s);
} else {
- s->phy_control = val & 0x7980;
+ s->phy_control = val & (MII_BMCR_LOOPBACK | MII_BMCR_SPEED100 |
+ MII_BMCR_AUTOEN | MII_BMCR_PDOWN |
+ MII_BMCR_FD | MII_BMCR_CTST);
/* Complete autonegotiation immediately. */
- if (val & 0x1000) {
- s->phy_status |= 0x0020;
+ if (val & MII_BMCR_AUTOEN) {
+ s->phy_status |= MII_BMSR_AN_COMP;
}
}
break;
- case 4: /* Auto-neg advertisement */
- s->phy_advertise = (val & 0x2d7f) | 0x80;
+ case MII_ANAR: /* Auto-neg advertisement */
+ s->phy_advertise = (val & (MII_ANAR_PAUSE_ASYM | MII_ANAR_PAUSE |
+ MII_ANAR_TXFD | MII_ANAR_TX |
+ MII_ANAR_10FD | MII_ANAR_10 | 0x1f)) |
+ MII_ANAR_TX;
break;
- case 30: /* Interrupt mask */
+ case MII_SMC911X_IM: /* Interrupt mask */
s->phy_int_mask = val & 0xff;
imx_phy_update_irq(s);
break;
- case 17:
- case 18:
+ case MII_LBREMR:
+ case MII_REC:
case 27:
case 31:
qemu_log_mask(LOG_UNIMP, "[%s.phy)%s: reg %d not implemented\n",
diff --git a/include/hw/net/mii.h b/include/hw/net/mii.h
index 4ae4dcce7e3..d2001bd859b 100644
--- a/include/hw/net/mii.h
+++ b/include/hw/net/mii.h
@@ -112,4 +112,8 @@
#define DP83848_PHYID1 0x2000
#define DP83848_PHYID2 0x5c90
+/* SMSC LAN911x Internal PHY */
+#define LAN911x_PHYID1 0x0007
+#define LAN911x_PHYID2 0xc0d1
+
#endif /* MII_H */
--
2.25.1
On Thu, 4 Jun 2020 at 13:39, Jean-Christophe Dubois <jcd@tribudubois.net> wrote: > > improve the PHY implementation with more generic code. > > This patch remove a lot of harcoded values to replace them with > generic symbols from header files. > > Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net> > --- > v2: Not present > v3: Not present > v4: Not present > v5: improve PHY implementation. > > hw/net/imx_fec.c | 76 +++++++++++++++++++++++++++----------------- > include/hw/net/mii.h | 4 +++ > 2 files changed, 50 insertions(+), 30 deletions(-) > - case 5: /* Auto-neg Link Partner Ability */ > - val = 0x0f71; > + case MII_ANLPAR: /* Auto-neg Link Partner Ability */ > + val = MII_ANLPAR_CSMACD | MII_ANLPAR_10 | MII_ANLPAR_10FD | > + MII_ANLPAR_TX | MII_ANLPAR_TXFD | MII_ANLPAR_PAUSE | > + MII_ANLPAR_PAUSEASY; The old value is 0x0f71, but the new one with the constants is 0x0de1. > - case 30: /* Interrupt mask */ > + case MII_SMC911X_IM: /* Interrupt mask */ > val = s->phy_int_mask; > break; > - case 17: > - case 18: > + case MII_NSR: > + val = 1 << 6; > + break; The old code didn't have a case for MII_NSR (16). > + case MII_LBREMR: > + case MII_REC: > case 27: > case 31: > - case 4: /* Auto-neg advertisement */ > - s->phy_advertise = (val & 0x2d7f) | 0x80; > + case MII_ANAR: /* Auto-neg advertisement */ > + s->phy_advertise = (val & (MII_ANAR_PAUSE_ASYM | MII_ANAR_PAUSE | > + MII_ANAR_TXFD | MII_ANAR_TX | > + MII_ANAR_10FD | MII_ANAR_10 | 0x1f)) | > + MII_ANAR_TX; The old code does & 0x2d7f; the new code is & 0xdff. > break; If some of these are bug fixes, please can you put them in a separate patch, so that the "use symbolic constants" change can be reviewed as making no functional changes? thanks -- PMM
Le 15/06/2020 à 15:03, Peter Maydell a écrit : > On Thu, 4 Jun 2020 at 13:39, Jean-Christophe Dubois <jcd@tribudubois.net> wrote: >> improve the PHY implementation with more generic code. >> >> This patch remove a lot of harcoded values to replace them with >> generic symbols from header files. >> >> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net> >> --- >> v2: Not present >> v3: Not present >> v4: Not present >> v5: improve PHY implementation. >> >> hw/net/imx_fec.c | 76 +++++++++++++++++++++++++++----------------- >> include/hw/net/mii.h | 4 +++ >> 2 files changed, 50 insertions(+), 30 deletions(-) > >> - case 5: /* Auto-neg Link Partner Ability */ >> - val = 0x0f71; >> + case MII_ANLPAR: /* Auto-neg Link Partner Ability */ >> + val = / | MII_ANLPAR_10 | MII_ANLPAR_10FD | >> + MII_ANLPAR_TX | MII_ANLPAR_TXFD | MII_ANLPAR_PAUSE | >> + MII_ANLPAR_PAUSEASY; > The old value is 0x0f71, but the new one with the constants > is 0x0de1. First of I should say that this PHY, first borrowed by the mfc_fec.c (coldfire ethernet device) from lan9118 (and now by imx_fec.c) is not one used on any real i.MX (i.MX6, i.MX7, i.MX31, i.MX25, ...) based board that I know of (this particular PHY is embedded n the lan9118 ethernet device) It is there because we were in need of a PHY and this PHY needs to be simple and more or less standard. I might have missed something but I am not really aware of way in Qemu to swap PHYs for a given ethernet emulator depending on the emulated board. So here this PHY was just a blind cut and paste of the lan9118.c PHY part to get a reasonable working PHY for the FEC/ENET device. So here the previous value of this register is not really meaningful. It is a mix of standard MII defined bits and LAN911X specific bits (for which I don't necessarily have definition ). Here I decided to restrict the implementation of this rather "virtual" PHY to only standard defined bits actually I think, I should have removed a lot more lan911x specific bits/registers to get to a really simple/trivial standard PHY. >> - case 30: /* Interrupt mask */ >> + case MII_SMC911X_IM: /* Interrupt mask */ >> val = s->phy_int_mask; >> break; >> - case 17: >> - case 18: >> + case MII_NSR: >> + val = 1 << 6; >> + break; > The old code didn't have a case for MII_NSR (16). I am not sure anymore why I added MII_NSR register. It is not present on lan9118 ethernet device but it is a standard defined register. >> + case MII_LBREMR: >> + case MII_REC: >> case 27: >> case 31: > >> - case 4: /* Auto-neg advertisement */ >> - s->phy_advertise = (val & 0x2d7f) | 0x80; >> + case MII_ANAR: /* Auto-neg advertisement */ >> + s->phy_advertise = (val & (MII_ANAR_PAUSE_ASYM | MII_ANAR_PAUSE | >> + MII_ANAR_TXFD | MII_ANAR_TX | >> + MII_ANAR_10FD | MII_ANAR_10 | 0x1f)) | >> + MII_ANAR_TX; > The old code does & 0x2d7f; the new code is & 0xdff. Same reason as the ANLPAR register. >> break; > If some of these are bug fixes, please can you put them in a separate > patch, so that the "use symbolic constants" change can be reviewed > as making no functional changes? > > thanks > -- PMM >
© 2016 - 2026 Red Hat, Inc.