| Message ID | a6223b7b5c1564afc5fb3c2a9ad514bdb41be5a5.1591272275.git.jcd@tribudubois.net |
|---|---|
| State | New |
| Headers | show |
| Series | hw/net/imx_fec: improve the imx fec emulator | expand |
On Thu, 4 Jun 2020 at 13:39, Jean-Christophe Dubois <jcd@tribudubois.net> wrote: > > Up to now we were allowing only one PHY device and it had to be the > first device on the bus. > > The i.MX6UL has 2 Ethernet devices and can therefore have several > PHY devices on the bus (and not necessarilly as device 0). > > This patch allows for PHY devices on 2nd, 3rd or any position. > > Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net> > diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c > index eefedc252de..29e613699ee 100644 > --- a/hw/net/imx_fec.c > +++ b/hw/net/imx_fec.c > @@ -280,11 +280,9 @@ static void imx_phy_reset(IMXFECState *s) > static uint32_t imx_phy_read(IMXFECState *s, int reg) > { > uint32_t val; > + uint32_t phy = reg / 32; > > - if (reg > 31) { > - /* we only advertise one phy */ > - return 0; > - } > + reg %= 32; This change means we now support multiple PHYs... > > switch (reg) { > case 0: /* Basic Control */ > @@ -331,19 +329,18 @@ static uint32_t imx_phy_read(IMXFECState *s, int reg) > break; > } > > - trace_imx_phy_read(val, reg); > + trace_imx_phy_read(val, phy, reg); ...but the only change we actually make is to trace the phy number. Surely if there is more than one phy then each phy needs to have its own state (phy_control/phy_status/phy_advertise/etc), rather than all these PHYs all sharing the same state under the hood? It also sounds from your commit message as if there isn't a large number of PHYs, but only perhaps two. In that case don't we need to fail attempts to access non-existent PHYs and only work with the ones which actually exist on any given board? thanks -- PMM
Le 15/06/2020 à 14:23, Peter Maydell a écrit : > On Thu, 4 Jun 2020 at 13:39, Jean-Christophe Dubois <jcd@tribudubois.net> wrote: >> Up to now we were allowing only one PHY device and it had to be the >> first device on the bus. >> >> The i.MX6UL has 2 Ethernet devices and can therefore have several >> PHY devices on the bus (and not necessarilly as device 0). >> >> This patch allows for PHY devices on 2nd, 3rd or any position. >> >> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net> >> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c >> index eefedc252de..29e613699ee 100644 >> --- a/hw/net/imx_fec.c >> +++ b/hw/net/imx_fec.c >> @@ -280,11 +280,9 @@ static void imx_phy_reset(IMXFECState *s) >> static uint32_t imx_phy_read(IMXFECState *s, int reg) >> { >> uint32_t val; >> + uint32_t phy = reg / 32; >> >> - if (reg > 31) { >> - /* we only advertise one phy */ >> - return 0; >> - } >> + reg %= 32; > This change means we now support multiple PHYs... Yes, on the i.MX6UL there are 2 ENET devices but only one MDIO bus to acess the PHYs. On the i.MX6UL evaluation board, PHY seems to be at offset/adress 1 and 2. See linux/arch/arm/boot/dts/imx6ul-14x14-evk.dtsi for details. > >> switch (reg) { >> case 0: /* Basic Control */ >> @@ -331,19 +329,18 @@ static uint32_t imx_phy_read(IMXFECState *s, int reg) >> break; >> } >> >> - trace_imx_phy_read(val, reg); >> + trace_imx_phy_read(val, phy, reg); > ...but the only change we actually make is to trace the phy number. > Surely if there is more than one phy then each phy needs to have > its own state (phy_control/phy_status/phy_advertise/etc), rather > than all these PHYs all sharing the same state under the hood? Well there might be several PHYs on the MDIO bus but each PHY is used only by one ENET device. There is no plan for one ENET to use either PHY. It can only use one. In Qemu (or at least in the FEC ethernet device emulator) the phy state is embedded in the ethernet device state. > > It also sounds from your commit message as if there isn't a > large number of PHYs, but only perhaps two. In that case > don't we need to fail attempts to access non-existent PHYs > and only work with the ones which actually exist on any > given board? As stated on the particular i.MX6UL evaluation board there are 2 phys connected to the MDIO bus at adresse 1 and 2. On another board the PHYs could be at different addresses or we might use only one MAC and therefore only one PHY. So in order to be able to fail on bad PHY access we need to discriminate for each board which PHYs are actually present and at what address on the MDIO bus. We would also need to know which PHY is connected to which MAC. I might have overlook something but so far there is no clear separate PHY and MAC implementation. Usually the PHY is more or less part of the MAC implementation and there are no easy way to instantiate them separately then connect them with the required bus (MDIO and MAC). I guess it might be feasible even if it sounds like overkill most of the time (you usually get one MAC connected to one PHY). Is there such a qemu framework that would allow to connect multiple PHY to a single MDIO bus and then each PHY to a specific MAC device? Could you point me in the right direction ? > > thanks > -- PMM
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c index eefedc252de..29e613699ee 100644 --- a/hw/net/imx_fec.c +++ b/hw/net/imx_fec.c @@ -280,11 +280,9 @@ static void imx_phy_reset(IMXFECState *s) static uint32_t imx_phy_read(IMXFECState *s, int reg) { uint32_t val; + uint32_t phy = reg / 32; - if (reg > 31) { - /* we only advertise one phy */ - return 0; - } + reg %= 32; switch (reg) { case 0: /* Basic Control */ @@ -331,19 +329,18 @@ static uint32_t imx_phy_read(IMXFECState *s, int reg) break; } - trace_imx_phy_read(val, reg); + trace_imx_phy_read(val, phy, reg); return val; } static void imx_phy_write(IMXFECState *s, int reg, uint32_t val) { - trace_imx_phy_write(val, reg); + uint32_t phy = reg / 32; - if (reg > 31) { - /* we only advertise one phy */ - return; - } + reg %= 32; + + trace_imx_phy_write(val, phy, reg); switch (reg) { case 0: /* Basic Control */ @@ -926,7 +923,7 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value, extract32(value, 18, 10))); } else { - /* This a write operation */ + /* This is a write operation */ imx_phy_write(s, extract32(value, 18, 10), extract32(value, 0, 16)); } /* raise the interrupt as the PHY operation is done */ diff --git a/hw/net/trace-events b/hw/net/trace-events index 26700dad997..27dfa0ef775 100644 --- a/hw/net/trace-events +++ b/hw/net/trace-events @@ -410,8 +410,8 @@ i82596_set_multicast(uint16_t count) "Added %d multicast entries" i82596_channel_attention(void *s) "%p: Received CHANNEL ATTENTION" # imx_fec.c -imx_phy_read(uint32_t val, int reg) "0x%04"PRIx32" <= reg[%d]" -imx_phy_write(uint32_t val, int reg) "0x%04"PRIx32" => reg[%d]" +imx_phy_read(uint32_t val, int phy, int reg) "0x%04"PRIx32" <= phy[%d].reg[%d]" +imx_phy_write(uint32_t val, int phy, int reg) "0x%04"PRIx32" => phy[%d].reg[%d]" imx_phy_update_link(const char *s) "%s" imx_phy_reset(void) "" imx_fec_read_bd(uint64_t addr, int flags, int len, int data) "tx_bd 0x%"PRIx64" flags 0x%04x len %d data 0x%08x"
Up to now we were allowing only one PHY device and it had to be the first device on the bus. The i.MX6UL has 2 Ethernet devices and can therefore have several PHY devices on the bus (and not necessarilly as device 0). This patch allows for PHY devices on 2nd, 3rd or any position. Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net> --- v2: Not present v3: Not present v4: Not present v5: Allow phy not to be the first device on the mii bus. hw/net/imx_fec.c | 19 ++++++++----------- hw/net/trace-events | 4 ++-- 2 files changed, 10 insertions(+), 13 deletions(-)