diff mbox series

[v5,2/3] hw/net/imx_fec: Allow phy not to be the first device on the mii bus.

Message ID a6223b7b5c1564afc5fb3c2a9ad514bdb41be5a5.1591272275.git.jcd@tribudubois.net
State New
Headers show
Series hw/net/imx_fec: improve the imx fec emulator | expand

Commit Message

Jean-Christophe DUBOIS June 4, 2020, 12:39 p.m. UTC
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(-)

Comments

Peter Maydell June 15, 2020, 12:23 p.m. UTC | #1
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
Jean-Christophe DUBOIS June 15, 2020, 7:45 p.m. UTC | #2
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 mbox series

Patch

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"