On Fri, Sep 22, 2017 at 10:13 AM, Philippe Mathieu-Daudé
<f4...@amsat.org> wrote:
> From: Grant Likely <grant.lik...@arm.com>
>
> 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 <grant.lik...@arm.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org>
> [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
>
>
