Re: [PATCH v8 08/11] hw/net: General GMAC Implementation

[Peter Maydell]

On Thu, 14 Dec 2023 at 21:15, Nabih Estefan <nabihestefan@google.com> wrote:
>
> From: Nabih Estefan Diaz <nabihestefan@google.com>
>
> - General GMAC Register handling
> - GMAC IRQ Handling
> - Added traces in some methods for debugging
> - Lots of declarations for accessing information on GMAC Descriptors 
> (npcm_gmac.h file)
>
> NOTE: With code on this state, the GMAC can boot-up properly and will show up 
> in the ifconfig command on the BMC

This commit message does not match the contents of the patch.

> Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
> ---
>  hw/net/npcm_gmac.c         |  26 -----
>  include/hw/net/npcm_gmac.h | 198 ++++++++++++++++++++++++++++++++++---
>  2 files changed, 184 insertions(+), 40 deletions(-)
>
> diff --git a/hw/net/npcm_gmac.c b/hw/net/npcm_gmac.c
> index be3f076200..09f048383b 100644
> --- a/hw/net/npcm_gmac.c
> +++ b/hw/net/npcm_gmac.c
> @@ -305,22 +305,6 @@ static void npcm_gmac_write(void *opaque, hwaddr offset,
>          break;
>
>      case A_NPCM_GMAC_MAC_CONFIG:
> -        prev = gmac->regs[offset / sizeof(uint32_t)];
> -        gmac->regs[offset / sizeof(uint32_t)] = v;
> -
> -        /* If transmit is being enabled for first time, update desc addr */
> -        if (~(prev & NPCM_GMAC_MAC_CONFIG_TX_EN) &
> -             (v & NPCM_GMAC_MAC_CONFIG_TX_EN)) {
> -            gmac->regs[R_NPCM_DMA_HOST_TX_DESC] =
> -                gmac->regs[R_NPCM_DMA_TX_BASE_ADDR];
> -        }
> -
> -        /* If receive is being enabled for first time, update desc addr */
> -        if (~(prev & NPCM_GMAC_MAC_CONFIG_RX_EN) &
> -             (v & NPCM_GMAC_MAC_CONFIG_RX_EN)) {
> -            gmac->regs[R_NPCM_DMA_HOST_RX_DESC] =
> -                gmac->regs[R_NPCM_DMA_RX_BASE_ADDR];
> -        }
>          break;
>
>      case A_NPCM_GMAC_MII_ADDR:
> @@ -371,16 +355,6 @@ static void npcm_gmac_write(void *opaque, hwaddr offset,
>                            "%s: Write of read-only bits of reg: offset: 0x%04"
>                             HWADDR_PRIx ", value: 0x%04" PRIx64 "\n",
>                             DEVICE(gmac)->canonical_path, offset, v);
> -        } else {
> -            /* for W1c bits, implement W1C */
> -            gmac->regs[offset / sizeof(uint32_t)] &=
> -                ~NPCM_DMA_STATUS_W1C_MASK(v);
> -            if (v & NPCM_DMA_STATUS_NIS_BITS) {
> -                gmac->regs[offset / sizeof(uint32_t)] &= 
> ~NPCM_DMA_STATUS_NIS;
> -            }
> -            if (v & NPCM_DMA_STATUS_AIS_BITS) {
> -                gmac->regs[offset / sizeof(uint32_t)] &= 
> ~NPCM_DMA_STATUS_AIS;
> -            }
>          }
>          break;

Why has this code been deleted in this patch ?

>
> diff --git a/include/hw/net/npcm_gmac.h b/include/hw/net/npcm_gmac.h
> index e5729e83ea..c97eb6fe6e 100644
> --- a/include/hw/net/npcm_gmac.h
> +++ b/include/hw/net/npcm_gmac.h
> @@ -34,13 +34,15 @@ struct NPCMGMACRxDesc {
>  };
>
>  /* NPCMGMACRxDesc.flags values */
> -/* RDES2 and RDES3 are buffer address pointers */
> -/* Owner: 0 = software, 1 = gmac */
> -#define RX_DESC_RDES0_OWNER_MASK BIT(31)
> +/* RDES2 and RDES3 are buffer addresses */
> +/* Owner: 0 = software, 1 = dma */
> +#define RX_DESC_RDES0_OWN BIT(31)
>  /* Destination Address Filter Fail */
> -#define RX_DESC_RDES0_DEST_ADDR_FILT_FAIL_MASK BIT(30)
> -/* Frame length*/
> -#define RX_DESC_RDES0_FRAME_LEN_MASK(word) extract32(word, 16, 29)
> +#define RX_DESC_RDES0_DEST_ADDR_FILT_FAIL BIT(30)
> +/* Frame length */
> +#define RX_DESC_RDES0_FRAME_LEN_MASK(word) extract32(word, 16, 14)
> +/* Frame length Shift*/
> +#define RX_DESC_RDES0_FRAME_LEN_SHIFT 16
>  /* Error Summary */
>  #define RX_DESC_RDES0_ERR_SUMM_MASK BIT(15)
>  /* Descriptor Error */
> @@ -83,9 +85,9 @@ struct NPCMGMACRxDesc {
>  /* Receive Buffer 2 Size */
>  #define RX_DESC_RDES1_BFFR2_SZ_SHIFT 11
>  #define RX_DESC_RDES1_BFFR2_SZ_MASK(word) extract32(word, \
> -    RX_DESC_RDES1_BFFR2_SZ_SHIFT, 10 + RX_DESC_RDES1_BFFR2_SZ_SHIFT)
> +    RX_DESC_RDES1_BFFR2_SZ_SHIFT, 11)
>  /* Receive Buffer 1 Size */
> -#define RX_DESC_RDES1_BFFR1_SZ_MASK(word) extract32(word, 0, 10)
> +#define RX_DESC_RDES1_BFFR1_SZ_MASK(word) extract32(word, 0, 11)

More churn here. Please go through your whole patchset reading
each patch and making sure that you don't have anything where
an earlier patch adds in some code and then a later patch
changes the way it's written unnecessarily.

thanks
-- PMM