| Message ID | 1597423256-14847-15-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/riscv: Add Microchip PolarFire SoC Icicle Kit board support | expand |
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. > 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]; > >
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 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
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 >
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. 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
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? Regards, Bin
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 >
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), 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];