| Message ID | 1682535272-32249-1-git-send-email-justinpopo6@gmail.com |
|---|---|
| Headers | show |
| Series | Brcm ASP 2.0 Ethernet controller | expand |
On Thu, Apr 27, 2023 at 10:16 AM Rob Herring <robh@kernel.org> wrote: > > On Wed, Apr 26, 2023 at 11:54:28AM -0700, Justin Chen wrote: > > From: Florian Fainelli <f.fainelli@gmail.com> > > > > Add a binding document for the Broadcom ASP 2.0 Ethernet > > controller. > > > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > Signed-off-by: Justin Chen <justinpopo6@gmail.com> > > --- > > .../devicetree/bindings/net/brcm,asp-v2.0.yaml | 145 +++++++++++++++++++++ > > 1 file changed, 145 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml > > > > diff --git a/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml b/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml > > new file mode 100644 > > index 000000000000..818d91692e6e > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml > > @@ -0,0 +1,145 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/net/brcm,asp-v2.0.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Broadcom ASP 2.0 Ethernet controller > > + > > +maintainers: > > + - Justin Chen <justinpopo6@gmail.com> > > + - Florian Fainelli <f.fainelli@gmail.com> > > + > > +description: Broadcom Ethernet controller first introduced with 72165 > > + > > +properties: > > + '#address-cells': > > + const: 1 > > + '#size-cells': > > + const: 1 > > + > > + compatible: > > + enum: > > + - brcm,asp-v2.0 > > + - brcm,bcm72165-asp-v2.0 > > + - brcm,asp-v2.1 > > + - brcm,bcm74165-asp-v2.1 > > You have 1 SoC per version, so what's the point of versions? If you have > more coming, then fine, but I'd expect it to be something like this: > > compatible = "brcm,bcm74165-asp-v2.1", "brcm,asp-v2.1"; > > Also, the version in the SoC specific compatible is redundant. Just > "brcm,bcm74165-asp" is enough. > > v2.1 is not compatible with v2.0? What that means is would a client/OS > that only understands what v2.0 is work with v2.1 h/w? If so, you should > have fallback compatible. > v2.1 is not compatible with v2.0 unfortunately. So no, a client/OS that only understands v2.0 will not work with v2.1 h/w. > > + > > + reg: > > + maxItems: 1 > > + > > + ranges: true > > + > > + interrupts: > > + minItems: 1 > > + items: > > + - description: RX/TX interrupt > > + - description: Port 0 Wake-on-LAN > > + - description: Port 1 Wake-on-LAN > > + > > + clocks: > > + maxItems: 1 > > + > > + ethernet-ports: > > The ethernet-switch.yaml schema doesn't work for you? > Technically it is not a switch. But it might work... If we use port to reference the unimac and reg to reference the ethernet channel. I rather not though, just cause it is not a switch, so calling it an ethernet-switch is confusing. > > + type: object > > + properties: > > + '#address-cells': > > + const: 1 > > + '#size-cells': > > + const: 0 > > + > > + patternProperties: > > + "^port@[0-9]+$": > > + type: object > > + > > + $ref: ethernet-controller.yaml# > > + > > + properties: > > + reg: > > + maxItems: 1 > > + description: Port number > > + > > + channel: > > + maxItems: 1 > > + description: ASP channel number > > Not a standard property, so it needs a type and vendor prefix. However, > what's the difference between channel and port? Can the port numbers > correspond to the channels? > Port refers to the unimac. In our case we currently have a maximum of 2. Channel refers to the ethernet hardware channel proper, in which we have many. So yes, you can have a port correlate to any channel. > > + > > + required: > > + - reg > > + - channel > > + > > + additionalProperties: false > > + > > +patternProperties: > > + "^mdio@[0-9a-f]+$": > > + type: object > > + $ref: "brcm,unimac-mdio.yaml" > > Drop quotes. > > > + > > + description: > > + ASP internal UniMAC MDIO bus > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - clocks > > + - ranges > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/irq.h> > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + > > + ethernet@9c00000 { > > + compatible = "brcm,asp-v2.0"; > > + reg = <0x9c00000 0x1fff14>; > > + interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>; > > + ranges; > > + clocks = <&scmi 14>; > > + #address-cells = <1>; > > + #size-cells = <1>; > > + > > + mdio@c614 { > > + compatible = "brcm,asp-v2.0-mdio"; > > + reg = <0xc614 0x8>; > > You have 1:1 ranges, is that really what you want? That means 0xc614 is > an absolute address. Ack, will fix. Thanks for the review, Justin > > > + reg-names = "mdio"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + phy0: ethernet-phy@1 { > > + reg = <1>; > > + }; > > + }; > > + > > + mdio@ce14 { > > + compatible = "brcm,asp-v2.0-mdio"; > > + reg = <0xce14 0x8>; > > + reg-names = "mdio"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + phy1: ethernet-phy@1 { > > + reg = <1>; > > + }; > > + }; > > + > > + ethernet-ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + port@0 { > > + reg = <0>; > > + channel = <8>; > > + phy-mode = "rgmii"; > > + phy-handle = <&phy0>; > > + }; > > + > > + port@1 { > > + reg = <1>; > > + channel = <9>; > > + phy-mode = "rgmii"; > > + phy-handle = <&phy1>; > > + }; > > + }; > > + }; > > -- > > 2.7.4 > >
On Wed, Apr 26, 2023 at 11:54:29AM -0700, Justin Chen wrote: > Add support for the Broadcom ASP 2.0 Ethernet controller which is first > introduced with 72165. This controller features two distinct Ethernet > ports that can be independently operated. > > This patch supports: > > - Wake-on-LAN using magic packets > - basic ethtool operations (link, counters, message level) > - MAC destination address filtering (promiscuous, ALL_MULTI, etc.) > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > Signed-off-by: Justin Chen <justinpopo6@gmail.com> ... > diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c ... > +static int bcmasp_netfilt_get_reg_offset(struct bcmasp_priv *priv, > + struct bcmasp_net_filter *nfilt, > + enum asp_netfilt_reg_type reg_type, > + u32 offset) > +{ > + u32 block_index, filter_sel; > + > + if (offset < 32) { > + block_index = ASP_RX_FILTER_NET_L2; > + filter_sel = nfilt->hw_index; > + } else if (offset < 64) { > + block_index = ASP_RX_FILTER_NET_L2; > + filter_sel = nfilt->hw_index + 1; > + } else if (offset < 96) { > + block_index = ASP_RX_FILTER_NET_L3_0; > + filter_sel = nfilt->hw_index; > + } else if (offset < 128) { > + block_index = ASP_RX_FILTER_NET_L3_0; > + filter_sel = nfilt->hw_index + 1; > + } else if (offset < 160) { > + block_index = ASP_RX_FILTER_NET_L3_1; > + filter_sel = nfilt->hw_index; > + } else if (offset < 192) { > + block_index = ASP_RX_FILTER_NET_L3_1; > + filter_sel = nfilt->hw_index + 1; > + } else if (offset < 224) { > + block_index = ASP_RX_FILTER_NET_L4; > + filter_sel = nfilt->hw_index; > + } else if (offset < 256) { > + block_index = ASP_RX_FILTER_NET_L4; > + filter_sel = nfilt->hw_index + 1; > + } block_index and filter_sel are uninitialised if offset doesn't match any of the conditions above. Can that happen? > + > + switch (reg_type) { > + case ASP_NETFILT_MATCH: > + return ASP_RX_FILTER_NET_PAT(filter_sel, block_index, > + (offset % 32)); > + case ASP_NETFILT_MASK: > + return ASP_RX_FILTER_NET_MASK(filter_sel, block_index, > + (offset % 32)); > + default: > + return -EINVAL; > + } > +} ... > +static void bcmasp_netfilt_tcpip4_wr(struct bcmasp_priv *priv, > + struct bcmasp_net_filter *nfilt, > + struct ethtool_tcpip4_spec *match, > + struct ethtool_tcpip4_spec *mask, > + u32 offset) > +{ > + __be16 val_16, mask_16; > + > + val_16 = htons(ETH_P_IP); > + mask_16 = 0xFFFF; mask_17 is __be16, but 0xFFFF is host byte order. Please make sure there are no new warnings when building with W=1 C=1. ... > +/* If no network filter found, return open filter. > + * If no more open filters return NULL > + */ > +struct bcmasp_net_filter *bcmasp_netfilt_get_init(struct bcmasp_intf *intf, > + int loc, bool wake_filter, > + bool init) > +{ > + struct bcmasp_priv *priv = intf->parent; > + struct bcmasp_net_filter *nfilter = NULL; > + int i, open_index = -1; Please use reverse xmas tree - longest line to shortest - for local variable declarations in networking code. You can check for this using https://github.com/ecree-solarflare/xmastree ... > +static int bcmasp_combine_set_filter(struct bcmasp_intf *intf, > + unsigned char *addr, unsigned char *mask, > + int i) > +{ > + u64 addr1, addr2, mask1, mask2, mask3; > + struct bcmasp_priv *priv = intf->parent; > + > + /* Switch to u64 to help with the calculations */ > + addr1 = ether_addr_to_u64(priv->mda_filters[i].addr); > + mask1 = ether_addr_to_u64(priv->mda_filters[i].mask); > + addr2 = ether_addr_to_u64(addr); > + mask2 = ether_addr_to_u64(mask); > + > + /* Check if one filter resides within the other */ > + mask3 = mask1 & mask2; > + if (mask3 == mask1 && ((addr1 & mask1) == (addr2 & mask1))) { > + /* Filter 2 resides within fitler 1, so everthing is good */ nit: s/fitler/filter/ Please consider running ./scripts/checkpatch.pl --codespell ... > +static void bcmasp_update_mib_counters(struct bcmasp_intf *priv) > +{ > + int i, j = 0; > + > + for (i = 0; i < BCMASP_STATS_LEN; i++) { > + const struct bcmasp_stats *s; > + u16 offset = 0; > + u32 val = 0; > + char *p; > + > + s = &bcmasp_gstrings_stats[i]; > + switch (s->type) { > + case BCMASP_STAT_NETDEV: > + case BCMASP_STAT_SOFT: > + continue; > + case BCMASP_STAT_RUNT: > + offset += BCMASP_STAT_OFFSET; > + fallthrough; > + case BCMASP_STAT_MIB_TX: > + offset += BCMASP_STAT_OFFSET; > + fallthrough; > + case BCMASP_STAT_MIB_RX: > + val = umac_rl(priv, UMC_MIB_START + j + offset); > + offset = 0; /* Reset Offset */ > + break; > + case BCMASP_STAT_RX_EDPKT: > + val = rx_edpkt_core_rl(priv->parent, s->reg_offset); > + break; > + case BCMASP_STAT_RX_CTRL: > + offset = bcmasp_stat_fixup_offset(priv, s); > + if (offset != ASP_RX_CTRL_FB_FILT_OUT_FRAME_COUNT) > + offset += sizeof(u32) * priv->port; > + val = rx_ctrl_core_rl(priv->parent, offset); > + break; > + } > + > + j += s->stat_sizeof; > + p = (char *)priv + s->stat_offset; > + *(u32 *)p = val; Is p always 32bit aligned? > + } > +} > + > +static void bcmasp_get_ethtool_stats(struct net_device *dev, > + struct ethtool_stats *stats, > + u64 *data) > +{ > + struct bcmasp_intf *priv = netdev_priv(dev); > + int i, j = 0; > + > + if (netif_running(dev)) > + bcmasp_update_mib_counters(priv); > + > + dev->netdev_ops->ndo_get_stats(dev); > + > + for (i = 0; i < BCMASP_STATS_LEN; i++) { > + const struct bcmasp_stats *s; > + char *p; > + > + s = &bcmasp_gstrings_stats[i]; > + if (!bcmasp_stat_available(priv, s->type)) > + continue; > + if (s->type == BCMASP_STAT_NETDEV) > + p = (char *)&dev->stats; > + else > + p = (char *)priv; > + p += s->stat_offset; > + if (sizeof(unsigned long) != sizeof(u32) && > + s->stat_sizeof == sizeof(unsigned long)) > + data[j] = *(unsigned long *)p; > + else > + data[j] = *(u32 *)p; Maybe memcpy would make this a little easier to read. > + j++; > + } > +} ... > diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c ... > +static int bcmasp_init_rx(struct bcmasp_intf *intf) > +{ > + struct device *kdev = &intf->parent->pdev->dev; > + struct net_device *ndev = intf->ndev; > + void *p; > + dma_addr_t dma; > + struct page *buffer_pg; > + u32 reg; > + int ret; > + > + intf->rx_buf_order = get_order(RING_BUFFER_SIZE); > + buffer_pg = alloc_pages(GFP_KERNEL, intf->rx_buf_order); > + > + dma = dma_map_page(kdev, buffer_pg, 0, RING_BUFFER_SIZE, > + DMA_FROM_DEVICE); > + if (dma_mapping_error(kdev, dma)) { > + netdev_err(ndev, "Cannot allocate RX buffer\n"); I think the core will log an error on allocation failure, so the message above is not needed. > + __free_pages(buffer_pg, intf->rx_buf_order); > + return -ENOMEM; > + } ...
Le 26/04/2023 à 20:54, Justin Chen a écrit : > Add support for the Broadcom ASP 2.0 Ethernet controller which is first > introduced with 72165. This controller features two distinct Ethernet > ports that can be independently operated. > > This patch supports: > > - Wake-on-LAN using magic packets > - basic ethtool operations (link, counters, message level) > - MAC destination address filtering (promiscuous, ALL_MULTI, etc.) > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > Signed-off-by: Justin Chen <justinpopo6@gmail.com> > --- [...] > +void bcmasp_disable_all_filters(struct bcmasp_intf *intf) > +{ > + struct bcmasp_priv *priv = intf->parent; > + unsigned int i; Hi, Nit: Some loop index are unsigned int, but most are int. This could be done consistantly. > + > + /* Disable all filters held by this port */ > + for (i = ASP_RX_FILT_MDA_RES_COUNT(intf); i < NUM_MDA_FILTERS; i++) { > + if (priv->mda_filters[i].en && > + priv->mda_filters[i].port == intf->port) > + bcmasp_en_mda_filter(intf, 0, i); > + } > +} [...] > +static int bcmasp_probe(struct platform_device *pdev) > +{ > + struct device_node *ports_node, *intf_node; > + const struct bcmasp_plat_data *pdata; > + struct device *dev = &pdev->dev; > + int ret, i, count = 0, port; > + struct bcmasp_priv *priv; > + struct bcmasp_intf *intf; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->irq = platform_get_irq(pdev, 0); > + if (priv->irq <= 0) { > + dev_err(dev, "invalid interrupt\n"); > + return -EINVAL; > + } > + > + priv->clk = devm_clk_get_optional_enabled(dev, "sw_asp"); > + if (IS_ERR(priv->clk)) { > + dev_err(dev, "failed to request clock\n"); > + return PTR_ERR(priv->clk); > + } > + > + /* Base from parent node */ > + priv->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(priv->base)) { > + dev_err(dev, "failed to iomap\n"); > + return PTR_ERR(priv->base); > + } > + > + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40)); > + if (ret) > + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); I don't think that this fallback is needed. See [1]. More over, using dev_err_probe() would slighly simplify the probe function. (saves a few LoC, logs the error code in a human reading format) [1]: https://lore.kernel.org/lkml/86bf852e-4220-52d4-259d-3455bc24def1@wanadoo.fr/T/#m022abc0051ede3ba1feeb06cefd59e2a8a5c7864 > + if (ret) { > + dev_err(&pdev->dev, "unable to set DMA mask: %d\n", ret); > + return ret; > + } > + [...] > +static int __maybe_unused bcmasp_suspend(struct device *d) > +{ > + struct bcmasp_priv *priv = dev_get_drvdata(d); > + struct bcmasp_intf *intf; > + unsigned int i; Same > + int ret = 0; no need to initialize, but it is mostmy a matter of taste. > + > + for (i = 0; i < priv->intf_count; i++) { > + intf = priv->intfs[i]; > + if (!intf) > + continue; > + > + ret = bcmasp_interface_suspend(intf); > + if (ret) > + break; > + } > + > + ret = clk_prepare_enable(priv->clk); > + if (ret) > + return ret; > + > + /* Whether Wake-on-LAN is enabled or not, we can always disable > + * the shared TX clock > + */ > + bcmasp_core_clock_set(priv, 0, ASP_CTRL_CLOCK_CTRL_ASP_TX_DISABLE); > + > + bcmasp_core_clock_select(priv, true); > + > + clk_disable_unprepare(priv->clk); > + > + return ret; > +} > + > +static int __maybe_unused bcmasp_resume(struct device *d) > +{ > + struct bcmasp_priv *priv = dev_get_drvdata(d); > + struct bcmasp_intf *intf; > + unsigned int i; same > + int ret = 0; no need to initialize, but it is mostmy a matter of taste. Just my 2c, CJ
On Tue, May 2, 2023 at 12:44 PM Simon Horman <simon.horman@corigine.com> wrote: > > On Wed, Apr 26, 2023 at 11:54:29AM -0700, Justin Chen wrote: > > Add support for the Broadcom ASP 2.0 Ethernet controller which is first > > introduced with 72165. This controller features two distinct Ethernet > > ports that can be independently operated. > > > > This patch supports: > > > > - Wake-on-LAN using magic packets > > - basic ethtool operations (link, counters, message level) > > - MAC destination address filtering (promiscuous, ALL_MULTI, etc.) > > > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > Signed-off-by: Justin Chen <justinpopo6@gmail.com> > > ... > > > diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c > > ... > > > +static int bcmasp_netfilt_get_reg_offset(struct bcmasp_priv *priv, > > + struct bcmasp_net_filter *nfilt, > > + enum asp_netfilt_reg_type reg_type, > > + u32 offset) > > +{ > > + u32 block_index, filter_sel; > > + > > + if (offset < 32) { > > + block_index = ASP_RX_FILTER_NET_L2; > > + filter_sel = nfilt->hw_index; > > + } else if (offset < 64) { > > + block_index = ASP_RX_FILTER_NET_L2; > > + filter_sel = nfilt->hw_index + 1; > > + } else if (offset < 96) { > > + block_index = ASP_RX_FILTER_NET_L3_0; > > + filter_sel = nfilt->hw_index; > > + } else if (offset < 128) { > > + block_index = ASP_RX_FILTER_NET_L3_0; > > + filter_sel = nfilt->hw_index + 1; > > + } else if (offset < 160) { > > + block_index = ASP_RX_FILTER_NET_L3_1; > > + filter_sel = nfilt->hw_index; > > + } else if (offset < 192) { > > + block_index = ASP_RX_FILTER_NET_L3_1; > > + filter_sel = nfilt->hw_index + 1; > > + } else if (offset < 224) { > > + block_index = ASP_RX_FILTER_NET_L4; > > + filter_sel = nfilt->hw_index; > > + } else if (offset < 256) { > > + block_index = ASP_RX_FILTER_NET_L4; > > + filter_sel = nfilt->hw_index + 1; > > + } > > block_index and filter_sel are uninitialised if offset doesn't match any > of the conditions above. Can that happen? > Nope. This is a helper function for netfilter read and write reg, we check offset sizes in those functions. > > + > > + switch (reg_type) { > > + case ASP_NETFILT_MATCH: > > + return ASP_RX_FILTER_NET_PAT(filter_sel, block_index, > > + (offset % 32)); > > + case ASP_NETFILT_MASK: > > + return ASP_RX_FILTER_NET_MASK(filter_sel, block_index, > > + (offset % 32)); > > + default: > > + return -EINVAL; > > + } > > +} > > ... > > > +static void bcmasp_netfilt_tcpip4_wr(struct bcmasp_priv *priv, > > + struct bcmasp_net_filter *nfilt, > > + struct ethtool_tcpip4_spec *match, > > + struct ethtool_tcpip4_spec *mask, > > + u32 offset) > > +{ > > + __be16 val_16, mask_16; > > + > > + val_16 = htons(ETH_P_IP); > > + mask_16 = 0xFFFF; > > mask_17 is __be16, but 0xFFFF is host byte order. > > Please make sure there are no new warnings when building with W=1 C=1. > > ... > > > +/* If no network filter found, return open filter. > > + * If no more open filters return NULL > > + */ > > +struct bcmasp_net_filter *bcmasp_netfilt_get_init(struct bcmasp_intf *intf, > > + int loc, bool wake_filter, > > + bool init) > > +{ > > + struct bcmasp_priv *priv = intf->parent; > > + struct bcmasp_net_filter *nfilter = NULL; > > + int i, open_index = -1; > > Please use reverse xmas tree - longest line to shortest - for local > variable declarations in networking code. > > You can check for this using https://github.com/ecree-solarflare/xmastree > > ... > > > +static int bcmasp_combine_set_filter(struct bcmasp_intf *intf, > > + unsigned char *addr, unsigned char *mask, > > + int i) > > +{ > > + u64 addr1, addr2, mask1, mask2, mask3; > > + struct bcmasp_priv *priv = intf->parent; > > + > > + /* Switch to u64 to help with the calculations */ > > + addr1 = ether_addr_to_u64(priv->mda_filters[i].addr); > > + mask1 = ether_addr_to_u64(priv->mda_filters[i].mask); > > + addr2 = ether_addr_to_u64(addr); > > + mask2 = ether_addr_to_u64(mask); > > + > > + /* Check if one filter resides within the other */ > > + mask3 = mask1 & mask2; > > + if (mask3 == mask1 && ((addr1 & mask1) == (addr2 & mask1))) { > > + /* Filter 2 resides within fitler 1, so everthing is good */ > > nit: s/fitler/filter/ > > Please consider running ./scripts/checkpatch.pl --codespell > > ... > > > +static void bcmasp_update_mib_counters(struct bcmasp_intf *priv) > > +{ > > + int i, j = 0; > > + > > + for (i = 0; i < BCMASP_STATS_LEN; i++) { > > + const struct bcmasp_stats *s; > > + u16 offset = 0; > > + u32 val = 0; > > + char *p; > > + > > + s = &bcmasp_gstrings_stats[i]; > > + switch (s->type) { > > + case BCMASP_STAT_NETDEV: > > + case BCMASP_STAT_SOFT: > > + continue; > > + case BCMASP_STAT_RUNT: > > + offset += BCMASP_STAT_OFFSET; > > + fallthrough; > > + case BCMASP_STAT_MIB_TX: > > + offset += BCMASP_STAT_OFFSET; > > + fallthrough; > > + case BCMASP_STAT_MIB_RX: > > + val = umac_rl(priv, UMC_MIB_START + j + offset); > > + offset = 0; /* Reset Offset */ > > + break; > > + case BCMASP_STAT_RX_EDPKT: > > + val = rx_edpkt_core_rl(priv->parent, s->reg_offset); > > + break; > > + case BCMASP_STAT_RX_CTRL: > > + offset = bcmasp_stat_fixup_offset(priv, s); > > + if (offset != ASP_RX_CTRL_FB_FILT_OUT_FRAME_COUNT) > > + offset += sizeof(u32) * priv->port; > > + val = rx_ctrl_core_rl(priv->parent, offset); > > + break; > > + } > > + > > + j += s->stat_sizeof; > > + p = (char *)priv + s->stat_offset; > > + *(u32 *)p = val; > > Is p always 32bit aligned? > Nope. I can make sure it is 32 bit aligned. Acked, the other comments. Will submit v3 when net-next window is open. Thank you for the review. Justin > > + } > > +} > > + > > +static void bcmasp_get_ethtool_stats(struct net_device *dev, > > + struct ethtool_stats *stats, > > + u64 *data) > > +{ > > + struct bcmasp_intf *priv = netdev_priv(dev); > > + int i, j = 0; > > + > > + if (netif_running(dev)) > > + bcmasp_update_mib_counters(priv); > > + > > + dev->netdev_ops->ndo_get_stats(dev); > > + > > + for (i = 0; i < BCMASP_STATS_LEN; i++) { > > + const struct bcmasp_stats *s; > > + char *p; > > + > > + s = &bcmasp_gstrings_stats[i]; > > + if (!bcmasp_stat_available(priv, s->type)) > > + continue; > > + if (s->type == BCMASP_STAT_NETDEV) > > + p = (char *)&dev->stats; > > + else > > + p = (char *)priv; > > + p += s->stat_offset; > > + if (sizeof(unsigned long) != sizeof(u32) && > > + s->stat_sizeof == sizeof(unsigned long)) > > + data[j] = *(unsigned long *)p; > > + else > > + data[j] = *(u32 *)p; > > Maybe memcpy would make this a little easier to read. > > > + j++; > > + } > > +} > > ... > > > diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c > > ... > > > +static int bcmasp_init_rx(struct bcmasp_intf *intf) > > +{ > > + struct device *kdev = &intf->parent->pdev->dev; > > + struct net_device *ndev = intf->ndev; > > + void *p; > > + dma_addr_t dma; > > + struct page *buffer_pg; > > + u32 reg; > > + int ret; > > + > > + intf->rx_buf_order = get_order(RING_BUFFER_SIZE); > > + buffer_pg = alloc_pages(GFP_KERNEL, intf->rx_buf_order); > > + > > + dma = dma_map_page(kdev, buffer_pg, 0, RING_BUFFER_SIZE, > > + DMA_FROM_DEVICE); > > + if (dma_mapping_error(kdev, dma)) { > > + netdev_err(ndev, "Cannot allocate RX buffer\n"); > > I think the core will log an error on allocation failure, > so the message above is not needed. > > > + __free_pages(buffer_pg, intf->rx_buf_order); > > + return -ENOMEM; > > + } > > ...
On Tue, May 02, 2023 at 02:26:53PM -0700, Justin Chen wrote: > On Tue, May 2, 2023 at 12:44 PM Simon Horman <simon.horman@corigine.com> wrote: > > > > On Wed, Apr 26, 2023 at 11:54:29AM -0700, Justin Chen wrote: ... > > > +static void bcmasp_update_mib_counters(struct bcmasp_intf *priv) > > > +{ > > > + int i, j = 0; > > > + > > > + for (i = 0; i < BCMASP_STATS_LEN; i++) { > > > + const struct bcmasp_stats *s; > > > + u16 offset = 0; > > > + u32 val = 0; > > > + char *p; > > > + > > > + s = &bcmasp_gstrings_stats[i]; > > > + switch (s->type) { > > > + case BCMASP_STAT_NETDEV: > > > + case BCMASP_STAT_SOFT: > > > + continue; > > > + case BCMASP_STAT_RUNT: > > > + offset += BCMASP_STAT_OFFSET; > > > + fallthrough; > > > + case BCMASP_STAT_MIB_TX: > > > + offset += BCMASP_STAT_OFFSET; > > > + fallthrough; > > > + case BCMASP_STAT_MIB_RX: > > > + val = umac_rl(priv, UMC_MIB_START + j + offset); > > > + offset = 0; /* Reset Offset */ > > > + break; > > > + case BCMASP_STAT_RX_EDPKT: > > > + val = rx_edpkt_core_rl(priv->parent, s->reg_offset); > > > + break; > > > + case BCMASP_STAT_RX_CTRL: > > > + offset = bcmasp_stat_fixup_offset(priv, s); > > > + if (offset != ASP_RX_CTRL_FB_FILT_OUT_FRAME_COUNT) > > > + offset += sizeof(u32) * priv->port; > > > + val = rx_ctrl_core_rl(priv->parent, offset); > > > + break; > > > + } > > > + > > > + j += s->stat_sizeof; > > > + p = (char *)priv + s->stat_offset; > > > + *(u32 *)p = val; > > > > Is p always 32bit aligned? > > > > Nope. I can make sure it is 32 bit aligned. I'm not sure if it helps, but you could also consider put_unaligned(). > Acked, the other comments. Will submit v3 when net-next window is > open. Thank you for the review. Likewise, thanks.