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

[Philippe Mathieu-Daudé]

On 8/16/20 3:42 PM, Bin Meng wrote:
> On Sun, Aug 16, 2020 at 8:08 PM Nathan Rossi <nathan@nathanrossi.com> wrote:
>>
>> On Sun, 16 Aug 2020 at 18:29, Bin Meng <bmeng.cn@gmail.com> 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.
>>
>> Given Xilinx's QEMU fork has not used this value for quite some time,
>> I suspect it was only used to match an early chip emulation
>> platform/board.
>>
>> https://github.com/Xilinx/qemu/blame/master/hw/net/cadence_gem.c#L248
>>
>>>
>>> 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.
>>
>> It is common for phy devices to support it as a broadcast address. It
>> is also commonly written in Xilinx documentation that it is treated as
>> a broadcast address. e.g. the axi ethernet core
>> (https://www.xilinx.com/support/documentation/ip_documentation/axi_ethernet/v7_0/pg138-axi-ethernet.pdf
>> page 45). But 802.3 does not require it, instead address 0 is only
>> reserved.
>>
>> With this commit the "must" refers to the device specification, in
>> that QEMU is emulating a real phy (though more specifically the phy(s)
>> that were being emulated for Zynq boards) that does respond to address
>> 0 as a broadcast. This change was a trade off in order to make QEMU
>> behave as it would on hardware, such that software using address 0 as
>> broadcast would work correctly.
>>
> 
> Thanks Nathan. So is it safe to just remove BOARD_PHY_ADDRESS and set
> "phy-addr" property default value to 0?

I'd do as following:

First patch, introduce "phy-addr" property (default to
BOARD_PHY_ADDRESS) and remove BOARD_PHY_ADDRESS in code:

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

Second patch set "phy-addr" to BOARD_PHY_ADDRESS in all
current boards using this PHY and set the default to 0.

Thanks,

Phil.

> 
> Regards,
> Bin
>