Re: [Qemu-devel] [PATCH v5 4/7] hw/mdio: Mask out read-only bits.

[Alistair Francis]

On Fri, Sep 22, 2017 at 10:13 AM, Philippe Mathieu-Daudé
<address@hidden> wrote:
> From: Grant Likely <address@hidden>
>
> The RST and ANEG_RST bits are commands, not settings. An operating
> system will get confused (or at least u-boot does) if those bits remain
> set after writing to them. Therefore, mask them out on write.
>
> Similarly, no bits in the ID1, ID2, and remote capability registers are
> writeable; so mask them out also.
>
> Signed-off-by: Grant Likely <address@hidden>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> [PMD: just rebased]
> ---
>  include/hw/net/mdio.h |  1 +
>  hw/net/mdio.c         | 16 ++++++++++++----
>  2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/include/hw/net/mdio.h b/include/hw/net/mdio.h
> index b3b4f497c0..ed1879a728 100644
> --- a/include/hw/net/mdio.h
> +++ b/include/hw/net/mdio.h
> @@ -53,6 +53,7 @@
>
>  struct qemu_phy {
>      uint32_t regs[NUM_PHY_REGS];
> +    const uint16_t *regs_readonly_mask; /* 0=writable, 1=read-only */
>
>      int link;
>
> diff --git a/hw/net/mdio.c b/hw/net/mdio.c
> index 33bfbb4623..89a6a3a590 100644
> --- a/hw/net/mdio.c
> +++ b/hw/net/mdio.c
> @@ -109,17 +109,24 @@ static unsigned int mdio_phy_read(struct qemu_phy *phy, 
> unsigned int req)
>
>  static void mdio_phy_write(struct qemu_phy *phy, unsigned int req, unsigned 
> int data)
>  {
> -    int regnum;
> +    int regnum = req & 0x1f;
> +    uint16_t mask = phy->regs_readonly_mask[regnum];
>
> -    regnum = req & 0x1f;
> -    D(printf("%s reg[%d] = %x\n", __func__, regnum, data));
> +    D(printf("%s reg[%d] = %x; mask=%x\n", __func__, regnum, data, mask));
>      switch (regnum) {
>      default:
> -        phy->regs[regnum] = data;
> +        phy->regs[regnum] = (phy->regs[regnum] & mask) | (data & ~mask);
>          break;
>      }
>  }
>
> +static const uint16_t default_readonly_mask[32] = {
> +    [PHY_CTRL] = PHY_CTRL_RST | PHY_CTRL_ANEG_RST,
> +    [PHY_ID1] = 0xffff,
> +    [PHY_ID2] = 0xffff,
> +    [PHY_LP_ABILITY] = 0xffff,
> +};

This is what the register API is really good at :)

Overall this looks fine, can we use a macro for the 32 though and then
protect accesses with an assert() or if()?

Alistair

> +
>  void mdio_phy_init(struct qemu_phy *phy, uint16_t id1, uint16_t id2)
>  {
>      phy->regs[PHY_CTRL] = 0x3100;
> @@ -128,6 +135,7 @@ void mdio_phy_init(struct qemu_phy *phy, uint16_t id1, 
> uint16_t id2)
>      phy->regs[PHY_ID2] = id2;
>      /* Autonegotiation advertisement reg. */
>      phy->regs[PHY_AUTONEG_ADV] = 0x01e1;
> +    phy->regs_readonly_mask = default_readonly_mask;
>      phy->link = 1;
>
>      phy->read = mdio_phy_read;
> --
> 2.14.1
>
>