On Tue, 2 Feb 2021 at 23:29, Doug Evans <dje@google.com> wrote:
>
> This is a 10/100 ethernet device that has several features.
> Only the ones needed by the Linux driver have been implemented.
> See npcm7xx_emc.c for a list of unimplemented features.
>
> Reviewed-by: Hao Wu <wuhaotsh@google.com>
> Reviewed-by: Avi Fishman <avi.fishman@nuvoton.com>
> Signed-off-by: Doug Evans <dje@google.com>
> ---
> hw/net/meson.build | 1 +
> hw/net/npcm7xx_emc.c | 852 +++++++++++++++++++++++++++++++++++
> hw/net/trace-events | 17 +
> include/hw/net/npcm7xx_emc.h | 286 ++++++++++++
> 4 files changed, 1156 insertions(+)
> create mode 100644 hw/net/npcm7xx_emc.c
> create mode 100644 include/hw/net/npcm7xx_emc.h
> +static void emc_reset(NPCM7xxEMCState *emc)
> +{
> + trace_npcm7xx_emc_reset(emc->emc_num);
> +
> + memset(&emc->regs[0], 0, sizeof(emc->regs));
> +
> + /* These regs have non-zero reset values. */
> + emc->regs[REG_TXDLSA] = 0xfffffffc;
> + emc->regs[REG_RXDLSA] = 0xfffffffc;
> + emc->regs[REG_MIIDA] = 0x00900000;
> + emc->regs[REG_FFTCR] = 0x0101;
> + emc->regs[REG_DMARFC] = 0x0800;
> + emc->regs[REG_MPCNT] = 0x7fff;
> +
> + emc->tx_active = false;
> + emc->rx_active = false;
> +
> + qemu_set_irq(emc->tx_irq, 0);
> + qemu_set_irq(emc->rx_irq, 0);
> +}
> +
> +static void npcm7xx_emc_reset(DeviceState *dev)
> +{
> + NPCM7xxEMCState *emc = NPCM7XX_EMC(dev);
> + emc_reset(emc);
> +}
You can't call qemu_set_irq() from a DeviceState::reset method.
Usually it's OK just not to try to set the outbound IRQs and
to assume that the device you're connected to has reset to the
state where its inbound IRQ line is not asserted. If you really
need to set the irq line then you need to switch to 3-phase
reset (some of the other npcm7xx devices do this). But I
suspect that just moving the qemu_set_irq() calls to
emc_soft_reset() would be enough.
Don't put local variable declarations in the middle of functions,
please. Coding style says they should be at the start of a
block (so, here, the start of the function). It looks like you've
got middle-of-function declarations in several places in other
functions too, so could you fix them all up please?
Fixed in v3.
Maybe now's a good time though to revisit this rule.
QEMU uses C99, and mixed decls/statements is an easy improvement to the coding standards.
I'm guessing this is an uncontroversial request. Is there just inertia behind not making the change thus far?
Optional, but you might consider using g_autofree for
malloced_buf, which would let the compiler deal with
g_free()ing it for you on all the function exit paths.
Done in v3.
Thanks.