On Mon, Feb 8, 2021 at 9:17 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> 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.
>

Ah. Fixed in v3.

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.