On Sun, 16 Aug 2020 at 18:29, Bin Meng <bmeng...@gmail.com> wrote:
>
> On Sat, Aug 15, 2020 at 5:06 PM Philippe Mathieu-Daudé <f4...@amsat.org>
> wrote:
> >
> > On 8/14/20 6:40 PM, Bin Meng wrote:
> > > From: Bin Meng <bin.m...@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.m...@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.crosthwa...@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.crosthwa...@petalogix.com>
> Signed-off-by: John Linn <john.l...@xilinx.com>
> Acked-by: Edgar E. Iglesias <edgar.igles...@gmail.com>
> Signed-off-by: Edgar E. Iglesias <edgar.igles...@gmail.com>
>
> Later PHY address 0 was added via the following commit:
>
> commit 553893736885e4f2dda424bff3e2200e1b6482a5
> Author: Peter Crosthwaite <peter.crosthwa...@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.ro...@xilinx.com>
> Signed-off-by: Peter Crosthwaite <peter.crosthwa...@xilinx.com>
> Message-id:
> 6f4d53b04ddbfb19895bfb61a595e69f1c08859a.1396594056.git.peter.crosthwa...@xilinx.com
> Reviewed-by: Beniamino Galvani <b.galv...@gmail.com>
> Signed-off-by: Peter Maydell <peter.mayd...@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.
Regards,
Nathan
>
> + 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
