Le 15/06/2020 à 15:03, Peter Maydell a écrit :
On Thu, 4 Jun 2020 at 13:39, Jean-Christophe Dubois <j...@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 <j...@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
