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),
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];
--
2.7.4
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
>
© 2016 - 2026 Red Hat, Inc.