qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v8 07/11] include/hw/net: Implemented Classes and Masks for G


From: Peter Maydell
Subject: Re: [PATCH v8 07/11] include/hw/net: Implemented Classes and Masks for GMAC Descriptors
Date: Mon, 18 Dec 2023 14:36:14 +0000

On Thu, 14 Dec 2023 at 21:15, Nabih Estefan <nabihestefan@google.com> wrote:
>
> From: Nabih Estefan Diaz <nabihestefan@google.com>
>
>  - Implemeted classes for GMAC Receive and Transmit Descriptors
>  - Implemented Masks for said descriptors
>
> Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> Reviewed-by: Tyrone Ting <kfting@nuvoton.com>


>  static const uint32_t npcm_gmac_cold_reset_values[NPCM_GMAC_NR_REGS] = {
> -    [R_NPCM_GMAC_VERSION]         = 0x00001037,
> +    /* Reduce version to 3.2 so that the kernel can enable interrupt. */
> +    [R_NPCM_GMAC_VERSION]         = 0x00001032,

This needs more description. What's going on here? Generally
workarounds for guest bugs are a bad idea, because once we put
them in we have pretty much no way of ever getting them out
of the codebase again. This goes doubly so if we don't record
exactly what the problem was that made us do the workaround...

Also, if you want to implement only v3.2, then do that from
the start, don't start with a patch that says "3.7" and then
change it in this patch.

>      [R_NPCM_GMAC_TIMER_CTRL]      = 0x03e80000,
>      [R_NPCM_GMAC_MAC0_ADDR_HI]    = 0x8000ffff,
>      [R_NPCM_GMAC_MAC0_ADDR_LO]    = 0xffffffff,
> @@ -125,12 +126,12 @@ static const uint16_t phy_reg_init[] = {
>      [MII_EXTSTAT]   = 0x3000, /* 1000BASTE_T full-duplex capable */
>  };
>
> -static void npcm_gmac_soft_reset(NPCMGMACState *s)
> +static void npcm_gmac_soft_reset(NPCMGMACState *gmac)
>  {
> -    memcpy(s->regs, npcm_gmac_cold_reset_values,
> +    memcpy(gmac->regs, npcm_gmac_cold_reset_values,
>             NPCM_GMAC_NR_REGS * sizeof(uint32_t));
>      /* Clear reset bits */
> -    s->regs[R_NPCM_DMA_BUS_MODE] &= ~NPCM_DMA_BUS_MODE_SWR;
> +    gmac->regs[R_NPCM_DMA_BUS_MODE] &= ~NPCM_DMA_BUS_MODE_SWR;
>  }

What is this churn from 's' to 'gmac' doing in this patch?
If you want 'gmac', that's fine, but this file is new in
this patch series, so just make it be the variable name you
want in the patch where you add this function, rather than
adding it with one name then changing it later in the series.

>  static void gmac_phy_set_link(NPCMGMACState *s, bool active)
> @@ -148,11 +149,53 @@ static bool gmac_can_receive(NetClientState *nc)
>      return true;
>  }
>
> -static ssize_t gmac_receive(NetClientState *nc, const uint8_t *buf, size_t 
> len1)

Similarly, why did the 'len1' argument here change to 'len',
and the "/* Placeholder */" comment appear only now? The effect
is it looks like this patch changed that function, but it didn't
in any interesting way.

That comment is not very helpful, by the way -- if, eg, the
idea is that the function gets filled in later in the patchseries,
then say so. If the function is a dummy one because of a missing
feature that won't be added in this patchset, then say that. Etc.

> +/*
> + * Function that updates the GMAC IRQ
> + * It find the logical OR of the enabled bits for NIS (if enabled)
> + * It find the logical OR of the enabled bits for AIS (if enabled)
> + */
> +static void gmac_update_irq(NPCMGMACState *gmac)
>  {
> -    return 0;
> +    /*
> +     * Check if the normal interrupts summery is enabled

It might be helpful to run a spellcheck on your comments.
This is "summary" (which you get right on the line below).

> +     * if so, add the bits for the summary that are enabled
> +     */
> +    if (gmac->regs[R_NPCM_DMA_INTR_ENA] & gmac->regs[R_NPCM_DMA_STATUS] &
> +        (NPCM_DMA_INTR_ENAB_NIE_BITS))
> +    {
> +        gmac->regs[R_NPCM_DMA_STATUS] |=  NPCM_DMA_STATUS_NIS;
> +    }

Our coding style puts the opening brace on the same line as the if(),
not on a line of its own.

> +    /*
> +     * Check if the abnormal interrupts summery is enabled
> +     * if so, add the bits for the summary that are enabled
> +     */
> +    if (gmac->regs[R_NPCM_DMA_INTR_ENA] & gmac->regs[R_NPCM_DMA_STATUS] &
> +        (NPCM_DMA_INTR_ENAB_AIE_BITS))
> +    {
> +        gmac->regs[R_NPCM_DMA_STATUS] |=  NPCM_DMA_STATUS_AIS;
> +    }
> +
> +    /* Get the logical OR of both normal and abnormal interrupts */
> +    int level = !!((gmac->regs[R_NPCM_DMA_STATUS] &
> +                    gmac->regs[R_NPCM_DMA_INTR_ENA] &
> +                    NPCM_DMA_STATUS_NIS) |
> +                   (gmac->regs[R_NPCM_DMA_STATUS] &
> +                   gmac->regs[R_NPCM_DMA_INTR_ENA] &
> +                   NPCM_DMA_STATUS_AIS));
> +
> +    /* Set the IRQ */
> +    trace_npcm_gmac_update_irq(DEVICE(gmac)->canonical_path,
> +                               gmac->regs[R_NPCM_DMA_STATUS],
> +                               gmac->regs[R_NPCM_DMA_INTR_ENA],
> +                               level);
> +    qemu_set_irq(gmac->irq, level);
>  }
>
> +static ssize_t gmac_receive(NetClientState *nc, const uint8_t *buf, size_t 
> len)
> +{
> +    /* Placeholder */
> +    return 0;
> +}
>  static void gmac_cleanup(NetClientState *nc)
>  {
>      /* Nothing to do yet. */
> @@ -166,7 +209,7 @@ static void gmac_set_link(NetClientState *nc)
>      gmac_phy_set_link(s, !nc->link_down);
>  }
>
> -static void npcm_gmac_mdio_access(NPCMGMACState *s, uint16_t v)
> +static void npcm_gmac_mdio_access(NPCMGMACState *gmac, uint16_t v)

More changes of variable names. Please clean all this up into
the right patches.

>  {
>      bool busy = v & NPCM_GMAC_MII_ADDR_BUSY;
>      uint8_t is_write;

> diff --git a/include/hw/net/npcm_gmac.h b/include/hw/net/npcm_gmac.h
> index a92a654278..e5729e83ea 100644
> --- a/include/hw/net/npcm_gmac.h
> +++ b/include/hw/net/npcm_gmac.h
> @@ -37,8 +37,6 @@ struct NPCMGMACRxDesc {
>  /* RDES2 and RDES3 are buffer address pointers */
>  /* Owner: 0 = software, 1 = gmac */
>  #define RX_DESC_RDES0_OWNER_MASK BIT(31)
> -/* Owner*/
> -#define RX_DESC_RDES0_OWNER_SHIFT 31

Why did this go away?

>  /* Destination Address Filter Fail */
>  #define RX_DESC_RDES0_DEST_ADDR_FILT_FAIL_MASK BIT(30)
>  /* Frame length*/
> diff --git a/tests/qtest/npcm_gmac-test.c b/tests/qtest/npcm_gmac-test.c
> index 77a83c4c58..130a1599a8 100644
> --- a/tests/qtest/npcm_gmac-test.c
> +++ b/tests/qtest/npcm_gmac-test.c
> @@ -154,7 +154,7 @@ static void test_init(gconstpointer test_data)
>      CHECK_REG32(NPCM_GMAC_MII_DATA, 0);
>      CHECK_REG32(NPCM_GMAC_FLOW_CTRL, 0);
>      CHECK_REG32(NPCM_GMAC_VLAN_FLAG, 0);
> -    CHECK_REG32(NPCM_GMAC_VERSION, 0x00001037);
> +    CHECK_REG32(NPCM_GMAC_VERSION, 0x00001032);
>      CHECK_REG32(NPCM_GMAC_WAKEUP_FILTER, 0);
>      CHECK_REG32(NPCM_GMAC_PMT, 0);
>      CHECK_REG32(NPCM_GMAC_LPI_CTRL, 0);
> --

thanks
-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]