On Mon, Feb 8, 2021 at 9:17 AM Peter Maydell <peter.mayd...@linaro.org>
wrote:
> On Tue, 2 Feb 2021 at 23:29, Doug Evans <d...@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 <wuhao...@google.com>
> > Reviewed-by: Avi Fishman <avi.fish...@nuvoton.com>
> > Signed-off-by: Doug Evans <d...@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.
