Re: [PATCH v2 1/3] hw/net: Add npcm7xx emc model

[Peter Maydell]

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.

> +
> +static void emc_soft_reset(NPCM7xxEMCState *emc)
> +{
> +    /*
> +     * The docs say at least MCMDR.{LBK,OPMOD} bits are not changed during a
> +     * soft reset, but does not go into further detail. For now, KISS.
> +     */
> +    uint32_t mcmdr = emc->regs[REG_MCMDR];
> +    emc_reset(emc);
> +    emc->regs[REG_MCMDR] =
> +        mcmdr & (REG_MCMDR_LBK | REG_MCMDR_OPMOD);
> + }


> +static void emc_try_send_next_packet(NPCM7xxEMCState *emc)
> +{
> +    uint32_t desc_addr = TX_DESC_NTXDSA(emc->regs[REG_CTXDSA]);
> +    NPCM7xxEMCTxDesc tx_desc;
> +    if (emc_read_tx_desc(desc_addr, &tx_desc)) {
> +        /* Error reading descriptor, already reported. */
> +        emc_halt_tx(emc, REG_MISTA_TXBERR);
> +        emc_update_tx_irq(emc);
> +        return;
> +    }
> +
> +    /* Nothing we can do if we don't own the descriptor. */
> +    if (!(tx_desc.flags & TX_DESC_FLAG_OWNER_MASK)) {
> +        trace_npcm7xx_emc_cpu_owned_desc(desc_addr);
> +        emc_halt_tx(emc, REG_MISTA_TDU);
> +        emc_update_tx_irq(emc);
> +        return;
> +     }
> +
> +    /* Give the descriptor back regardless of what happens. */
> +    tx_desc.flags &= ~TX_DESC_FLAG_OWNER_MASK;
> +    tx_desc.status_and_length &= 0xffff;
> +
> +    /* Working buffer for sending out packets. Most packets fit in this. */
> +#define TX_BUFFER_SIZE 2048
> +    uint8_t tx_send_buffer[TX_BUFFER_SIZE];

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?

> +
> +    /*
> +     * Despite the h/w documentation saying the tx buffer is word aligned,
> +     * the linux driver does not word align the buffer. There is value in not
> +     * aligning the buffer: See the description of NET_IP_ALIGN in linux
> +     * kernel sources.
> +     */
> +    uint32_t next_buf_addr = tx_desc.txbsa;
> +    emc->regs[REG_CTXBSA] = next_buf_addr;
> +    uint32_t length = TX_DESC_PKT_LEN(tx_desc.status_and_length);
> +    uint8_t *buf = &tx_send_buffer[0];
> +    uint8_t *malloced_buf = NULL;

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.

> +
> +    if (length > sizeof(tx_send_buffer)) {
> +        malloced_buf = g_malloc(length);
> +        buf = malloced_buf;
> +    }

Otherwise the patch looks OK I think.

thanks
-- PMM