On Thu, 15 Oct 2026 at 19:11, Marcelo Manzo <[email protected]> wrote:
>
> From: Sergey Kambalin <[email protected]>
>
> Implement the MDIO command register handling used to read/write the
> BCM54213PE PHY's standard MII registers (BMCR, BMSR, LPA, ...) plus
> its vendor "shadow" register blocks reached through the AUX_CTL/
> EXP_SHD indirection scheme, and a simple link-state model (PHY
> starts down, comes up once BMCR is programmed and autonegotiation
> is (re)started).
>
> Part of Sergey Kambalin's original Raspberry Pi 4B PCIe/GENET
> networking series (patchwork series 829638, posted to qemu-devel in
> 2024, never merged). Carried forward and completed by Marcelo Manzo.
>
> While carrying this forward, fix the autonegotiation-restart handling
> in bcm2838_genet_mdio_cmd(): the guest PHY driver sets BMCR.ANRESTART
> and polls BMCR until it reads back as cleared, but the result of
> FIELD_DP16() (which returns a new value rather than modifying its
> argument) was discarded, so the bit never cleared and the guest
> spun forever at boot. Also refresh the link state once the restart
> has been handled.
>
> Signed-off-by: Sergey Kambalin <[email protected]>
> Signed-off-by: Marcelo Manzo <[email protected]>
> ---
> hw/net/bcm2838_genet.c | 139 ++++++++++++++++++++++++++++++++-
> include/hw/net/bcm2838_genet.h | 3 +-
> 2 files changed, 139 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/bcm2838_genet.c b/hw/net/bcm2838_genet.c
> index f3c5ca26c5..fe2a3cf890 100644
> --- a/hw/net/bcm2838_genet.c
> +++ b/hw/net/bcm2838_genet.c
> @@ -20,6 +20,8 @@
> #include "hw/net/bcm2838_genet.h"
> #include "trace.h"
>
> +static void bcm2838_genet_phy_update_link(BCM2838GenetState *s);
> +
> /* GENET layouts */
> REG32(GENET_SYS_REV_CTRL, 0)
> FIELD(GENET_SYS_REV_CTRL, GPHY_REV, 0, 16)
> @@ -216,6 +218,7 @@ FIELD(GENET_PHY_STAT_1000, LOCALRXOK, 13, 1)
> FIELD(GENET_PHY_STAT_1000, MSRES, 14, 1)
> FIELD(GENET_PHY_STAT_1000, MSFAIL, 15, 1)
>
> +/* There are two data representations for PHY_AUX_CTRL register */
> REG16(GENET_PHY_AUX_CTRL_0, 0)
> FIELD(GENET_PHY_AUX_CTRL_0, REG_ID_MASK, 0, 3)
> FIELD(GENET_PHY_AUX_CTRL_0, RSVD_3, 3, 1)
> @@ -272,6 +275,134 @@ static void
> bcm2838_genet_set_irq_prio(BCM2838GenetState *s)
> qemu_set_irq(s->irq_prio, level);
> }
>
> +static void bcm2838_genet_phy_aux_ctl_write(BCM2838GenetState *s,
> + uint16_t value)
> +{
> + uint16_t reg_id = FIELD_EX16(value, GENET_PHY_AUX_CTRL_0, REG_ID);
> + uint16_t reg_id_mask = FIELD_EX16(value, GENET_PHY_AUX_CTRL_0,
> REG_ID_MASK);
> + uint16_t misc_wren = FIELD_EX16(value, GENET_PHY_AUX_CTRL_0, MISC_WREN);
> + uint16_t reg_data = FIELD_EX16(value, GENET_PHY_AUX_CTRL_0, REG_DATA);
> + uint16_t reg_data12 = FIELD_EX16(value, GENET_PHY_AUX_CTRL_1, REG_DATA);
> +
> + uint16_t *phy_aux_ctl_shd_reg_id =
> + (uint16_t *)&s->phy_aux_ctl_shd_regs + reg_id;
> + uint16_t *phy_aux_ctl_shd_reg_id_mask =
> + (uint16_t *)&s->phy_aux_ctl_shd_regs + reg_id_mask;
Here again we are taking a pointer to a struct and assuming we
can also treat it as effectively an array of uint16_t. In this
case BCM2838GenetPhyAuxShdRegs is all uint16_t, but it would be
clearer IMHO to define it as an array of uint16_t rather than
as a struct, as we do with BCM2838GenetPhyExpShdRegs.
> +
> + if (reg_id_mask == BCM2838_GENET_PHY_AUX_CTL_MISC) {
> + if (reg_id == BCM2838_GENET_PHY_AUX_CTL_MISC) {
> + if (misc_wren == 0) {
> + /* write for subsequent read (8-bit from AUX_CTL_MISC) */
> + FIELD_DP16(value, GENET_PHY_AUX_CTRL_0, REG_DATA,
> + *phy_aux_ctl_shd_reg_id);
> + } else {
> + /* write 8 bits to AUX_CTL_MISC */
> + *phy_aux_ctl_shd_reg_id_mask = reg_data;
8 bits? This pointer is a uint16_t *.
> + }
> + } else {
> + /* write for subsequent read (12-bit) */
> + FIELD_DP16(value, GENET_PHY_AUX_CTRL_1, REG_DATA,
> + *phy_aux_ctl_shd_reg_id);
> + }
> + } else {
> + /* write 12 bits */
> + *phy_aux_ctl_shd_reg_id_mask = reg_data12;
> + }
> +
> + s->phy_regs.aux_ctl = value;
> +}
> +
> +static void bcm2838_genet_phy_shadow_write(BCM2838GenetState *s,
> + uint16_t value)
> +{
> + uint16_t reg_id = FIELD_EX16(value, GENET_PHY_SHADOW, REG_ID);
> + uint16_t wr = FIELD_EX16(value, GENET_PHY_SHADOW, WR);
> + uint16_t reg_data = FIELD_EX16(value, GENET_PHY_SHADOW, REG_DATA);
> +
> + uint16_t *phy_shd_reg = (uint16_t *)&s->phy_shd_regs + reg_id;
> +
> + if (wr == 0) {
> + FIELD_DP16(value, GENET_PHY_SHADOW, REG_DATA, *phy_shd_reg);
> + } else {
> + *phy_shd_reg = reg_data;
> + }
> +
> + s->phy_regs.shd = value;
> +}
> +
> +static void bcm2838_genet_phy_exp_shadow_write(BCM2838GenetState *s,
> + uint16_t value)
> +{
> + /*
> + * TODO Stub implementation without side effect,
> + * just storing registers values
> + */
> + uint16_t reg_id = FIELD_EX16(s->phy_regs.exp_ctrl,
> + GENET_PHY_EXP_SEL, REG_ID);
> + uint16_t block_id = FIELD_EX16(s->phy_regs.exp_ctrl,
> + GENET_PHY_EXP_SEL, BLOCK_ID);
> +
> + s->phy_exp_shd_regs.regs[block_id][reg_id] = value;
I think it would be nice to have some (preferably ARRAY_SIZE-based,
but maybe that's tricky for a 2D array) asserts here that the indexes
aren't out of range. In this case they can't be, because the fields
are both 8 bits and the arrays are 256 elements, but an assert()
guards against future danger and saves the reader having to go look up
the details.
thanks
-- PMM
