Re: [PATCH 14/18] hw/net: cadence_gem: Add a new 'phy-addr' property

[Philippe Mathieu-Daudé]

On 8/16/20 10:29 AM, Bin Meng wrote:
> On Sat, Aug 15, 2020 at 5:06 PM Philippe Mathieu-Daudé <f4bug@amsat.org> 
> wrote:
>>
>> On 8/14/20 6:40 PM, Bin Meng wrote:
>>> From: Bin Meng <bin.meng@windriver.com>
>>>
>>> At present the PHY address of the PHY connected to GEM is hard-coded
>>> to either 23 (BOARD_PHY_ADDRESS) or 0. This might not be the case for
>>> all boards. Add a new 'phy-addr' property so that board can specify
>>> the PHY address for each GEM instance.
>>>
>>> Signed-off-by: Bin Meng <bin.meng@windriver.com>
>>> ---
>>>
>>>  hw/net/cadence_gem.c         | 7 +++++--
>>>  include/hw/net/cadence_gem.h | 2 ++
>>>  2 files changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>>> index a93b5c0..9fa03de 100644
>>> --- a/hw/net/cadence_gem.c
>>> +++ b/hw/net/cadence_gem.c
>>> @@ -1446,7 +1446,8 @@ static uint64_t gem_read(void *opaque, hwaddr offset, 
>>> unsigned size)
>>>              uint32_t phy_addr, reg_num;
>>>
>>>              phy_addr = (retval & GEM_PHYMNTNC_ADDR) >> 
>>> GEM_PHYMNTNC_ADDR_SHFT;
>>> -            if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0) {
>>> +            if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0 ||
>>> +                phy_addr == s->phy_addr) {
>>>                  reg_num = (retval & GEM_PHYMNTNC_REG) >> 
>>> GEM_PHYMNTNC_REG_SHIFT;
>>>                  retval &= 0xFFFF0000;
>>>                  retval |= gem_phy_read(s, reg_num);
>>> @@ -1569,7 +1570,8 @@ static void gem_write(void *opaque, hwaddr offset, 
>>> uint64_t val,
>>>              uint32_t phy_addr, reg_num;
>>>
>>>              phy_addr = (val & GEM_PHYMNTNC_ADDR) >> GEM_PHYMNTNC_ADDR_SHFT;
>>> -            if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0) {
>>> +            if (phy_addr == BOARD_PHY_ADDRESS || phy_addr == 0 ||
>>> +                phy_addr == s->phy_addr) {
>>>                  reg_num = (val & GEM_PHYMNTNC_REG) >> 
>>> GEM_PHYMNTNC_REG_SHIFT;
>>>                  gem_phy_write(s, reg_num, val);
>>>              }
>>> @@ -1682,6 +1684,7 @@ static Property gem_properties[] = {
>>>      DEFINE_NIC_PROPERTIES(CadenceGEMState, conf),
>>>      DEFINE_PROP_UINT32("revision", CadenceGEMState, revision,
>>>                         GEM_MODID_VALUE),
>>> +    DEFINE_PROP_UINT8("phy-addr", CadenceGEMState, phy_addr, 0),
>>
>> This patch would be complete by moving the BOARD_PHY_ADDRESS definition
>> to each board using this NIC, and setting the "phy-addr" property to
>> this value.
> 
> I actually have no idea which board in QEMU is using this special PHY
> address instead of default 0.

It seems safe to assume all do use address 23.

Anyway you can simply get ride of BOARD_PHY_ADDRESS in the read/write
using:

    DEFINE_PROP_UINT8("phy-addr", CadenceGEMState,
                      phy_addr, BOARD_PHY_ADDRESS),

> 
> It looks BOARD_PHY_ADDRESS has been there since the initial version of
> the cadence_gem model.
> 
> commit e9f186e514a70557d695cadd2c2287ef97737023
> Author: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> Date:   Mon Mar 5 14:39:12 2012 +1000
> 
>     cadence_gem: initial version of device model
> 
>     Device model for cadence gem ethernet controller.
> 
>     Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>     Signed-off-by: John Linn <john.linn@xilinx.com>
>     Acked-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
>     Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> 
> Later PHY address 0 was added via the following commit:
> 
> commit 553893736885e4f2dda424bff3e2200e1b6482a5
> Author: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Date:   Thu Apr 3 23:55:19 2014 -0700
> 
>     net: cadence_gem: Make phy respond to broadcast
> 
>     Phys must respond to address 0 by specification. Implement.
> 
>     Signed-off-by: Nathan Rossi <nathan.rossi@xilinx.com>
>     Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>     Message-id:
> 6f4d53b04ddbfb19895bfb61a595e69f1c08859a.1396594056.git.peter.crosthwaite@xilinx.com
>     Reviewed-by: Beniamino Galvani <b.galvani@gmail.com>
>     Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> 
> I doubt the commit message said that PHYs must respond to address 0. I
> am not aware of such specs. The issue was probably that whatever board
> 2nd commit was tested against did not have a PHY at address
> BOARD_PHY_ADDRESS.
> 
> + a couple of Xilinx folks to comment.
> 
>>
>>>      DEFINE_PROP_UINT8("num-priority-queues", CadenceGEMState,
>>>                        num_priority_queues, 1),
>>>      DEFINE_PROP_UINT8("num-type1-screeners", CadenceGEMState,
>>> diff --git a/include/hw/net/cadence_gem.h b/include/hw/net/cadence_gem.h
>>> index 54e646f..01c6189 100644
>>> --- a/include/hw/net/cadence_gem.h
>>> +++ b/include/hw/net/cadence_gem.h
>>> @@ -73,6 +73,8 @@ typedef struct CadenceGEMState {
>>>      /* Mask of register bits which are write 1 to clear */
>>>      uint32_t regs_w1c[CADENCE_GEM_MAXREG];
>>>
>>> +    /* PHY address */
>>> +    uint8_t phy_addr;
>>>      /* PHY registers backing store */
>>>      uint16_t phy_regs[32];
> 
> Regards,
> Bin
>