[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
- [PATCH v8 00/11] Implementation of NPI Mailbox and GMAC Networking Module, Nabih Estefan, 2023/12/14
- [PATCH v8 02/11] hw/arm: Add PCI mailbox module to Nuvoton SoC, Nabih Estefan, 2023/12/14
- [PATCH v8 01/11] hw/misc: Add Nuvoton's PCI Mailbox Module, Nabih Estefan, 2023/12/14
- [PATCH v8 03/11] hw/misc: Add qtest for NPCM7xx PCI Mailbox, Nabih Estefan, 2023/12/14
- [PATCH v8 11/11] tests/qtest: Adding PCS Module test to GMAC Qtest, Nabih Estefan, 2023/12/14
- [PATCH v8 05/11] hw/arm: Add GMAC devices to NPCM7XX SoC, Nabih Estefan, 2023/12/14
- [PATCH v8 07/11] include/hw/net: Implemented Classes and Masks for GMAC Descriptors, Nabih Estefan, 2023/12/14
- Re: [PATCH v8 07/11] include/hw/net: Implemented Classes and Masks for GMAC Descriptors,
Peter Maydell <=
- [PATCH v8 06/11] tests/qtest: Creating qtest for GMAC Module, Nabih Estefan, 2023/12/14
- [PATCH v8 04/11] hw/net: Add NPCMXXX GMAC device, Nabih Estefan, 2023/12/14
- [PATCH v8 08/11] hw/net: General GMAC Implementation, Nabih Estefan, 2023/12/14
- [PATCH v8 09/11] hw/net: GMAC Rx Implementation, Nabih Estefan, 2023/12/14
- [PATCH v8 10/11] hw/net: GMAC Tx Implementation, Nabih Estefan, 2023/12/14