|
From: | Jean-Christophe DUBOIS |
Subject: | Re: [PATCH v5 3/3] hw/net/imx_fec: improve PHY implementation. |
Date: | Thu, 18 Jun 2020 22:53:30 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 |
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
[Prev in Thread] | Current Thread | [Next in Thread] |