Re: [PATCH net-next v6 07/11] net: ti: prueth: Adds support for network filters for traffic control supported by PRU-ICSS

Parvathi Pudi
2025-05-05T13:35:02Z

Hi,

>> + /* for LRE, it is a shared table. So lock the access */
>> + spin_lock_irqsave(&emac->addr_lock, flags);
>> +
>> + /* VLAN filter table is 512 bytes (4096 bit) bitmap.
>> + * Each bit controls enabling or disabling corresponding
>> + * VID. Therefore byte index that controls a given VID is
>> + * can calculated as vid / 8 and the bit within that byte
>> + * that controls VID is given by vid % 8. Allow untagged
>> + * frames to host by default.
>> + */
>> + byte_index = vid / BITS_PER_BYTE;
>> + bit_index = vid % BITS_PER_BYTE;
>> + val = readb(ram + vlan_filter_tbl + byte_index);
>> + if (add)
>> + val |= BIT(bit_index);
>> + else
>> + val &= ~BIT(bit_index);
>> + writeb(val, ram + vlan_filter_tbl + byte_index);
>> +
>> + spin_unlock_irqrestore(&emac->addr_lock, flags);
>> +
>> + netdev_dbg(emac->ndev, "%s VID bit at index %d and bit %d\n",
>> + add ? "Setting" : "Clearing", byte_index, bit_index);
>
> VID bit at byte index
>
>> +
>> + return 0;
>> +}
>> +
>> +static int icssm_emac_ndo_vlan_rx_add_vid(struct net_device *dev,
>> + __be16 proto, u16 vid)
>> +{
>> + struct prueth_emac *emac = netdev_priv(dev);
>> +
>> + return icssm_emac_add_del_vid(emac, true, proto, vid);
>> +}
>> +
>> +static int icssm_emac_ndo_vlan_rx_kill_vid(struct net_device *dev,
>> + __be16 proto, u16 vid)
>> +{
>> + struct prueth_emac *emac = netdev_priv(dev);
>> +
>> + return icssm_emac_add_del_vid(emac, false, proto, vid);
>> +}
>> +
>> +static int icssm_emac_get_port_parent_id(struct net_device *dev,
>> + struct netdev_phys_item_id *ppid)
>> +{
>> + struct prueth_emac *emac = netdev_priv(dev);
>> + struct prueth *prueth = emac->prueth;
>> +
>> + ppid->id_len = sizeof(prueth->base_mac);
>> + memcpy(&ppid->id, &prueth->base_mac, ppid->id_len);
>> +
>> + return 0;
>> +}
>> +
>> +static int icssm_emac_ndo_get_phys_port_name(struct net_device *ndev,
>> + char *name, size_t len)
>> +{
>> + struct prueth_emac *emac = netdev_priv(ndev);
>> + int err;
>> +
>> + err = snprintf(name, len, "p%d", emac->port_id);
>> +
>> + if (err >= len)
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> static const struct net_device_ops emac_netdev_ops = {
>> .ndo_open = icssm_emac_ndo_open,
>> .ndo_stop = icssm_emac_ndo_stop,
>> .ndo_start_xmit = icssm_emac_ndo_start_xmit,
>> + .ndo_set_mac_address = eth_mac_addr,
>> + .ndo_validate_addr = eth_validate_addr,
>> .ndo_tx_timeout = icssm_emac_ndo_tx_timeout,
>> .ndo_get_stats64 = icssm_emac_ndo_get_stats64,
>> + .ndo_set_rx_mode = icssm_emac_ndo_set_rx_mode,
>> .ndo_eth_ioctl = icssm_emac_ndo_ioctl,
>> + .ndo_vlan_rx_add_vid = icssm_emac_ndo_vlan_rx_add_vid,
>> + .ndo_vlan_rx_kill_vid = icssm_emac_ndo_vlan_rx_kill_vid,
>> + .ndo_setup_tc = icssm_emac_ndo_setup_tc,
>> + .ndo_get_port_parent_id = icssm_emac_get_port_parent_id,
>> + .ndo_get_phys_port_name = icssm_emac_ndo_get_phys_port_name,
>> };
>>
>> /* get emac_port corresponding to eth_node name */
>> @@ -1567,6 +1865,7 @@ static int icssm_prueth_netdev_init(struct prueth *prueth,
>> emac->prueth = prueth;
>> emac->ndev = ndev;
>> emac->port_id = port;
>> + memset(&emac->mc_filter_mask[0], 0xff, ETH_ALEN); /* default mask */
>>
>> /* by default eth_type is EMAC */
>> switch (port) {
>> @@ -1608,7 +1907,9 @@ static int icssm_prueth_netdev_init(struct prueth *prueth,
>> dev_err(prueth->dev, "could not get ptp tx irq. Skipping PTP support\n");
>> }
>>
>> + spin_lock_init(&emac->lock);
>> spin_lock_init(&emac->ptp_skb_lock);
>> + spin_lock_init(&emac->addr_lock);
>>
>> /* get mac address from DT and set private and netdev addr */
>> ret = of_get_ethdev_address(eth_node, ndev);
>> @@ -1637,6 +1938,10 @@ static int icssm_prueth_netdev_init(struct prueth
>> *prueth,
>> phy_remove_link_mode(emac->phydev, ETHTOOL_LINK_MODE_Pause_BIT);
>> phy_remove_link_mode(emac->phydev, ETHTOOL_LINK_MODE_Asym_Pause_BIT);
>>
>> + ndev->features |= NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_TC;
>> +
>> + ndev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER;
>> +
>> ndev->netdev_ops = &emac_netdev_ops;
>> ndev->ethtool_ops = &emac_ethtool_ops;
>>
>> @@ -1689,6 +1994,7 @@ static int icssm_prueth_probe(struct platform_device
>> *pdev)
>> platform_set_drvdata(pdev, prueth);
>> prueth->dev = dev;
>> prueth->fw_data = device_get_match_data(dev);
>> + prueth->fw_offsets = &fw_offsets_v2_1;
>>
>> eth_ports_node = of_get_child_by_name(np, "ethernet-ports");
>> if (!eth_ports_node)
>> @@ -1875,6 +2181,8 @@ static int icssm_prueth_probe(struct platform_device
>> *pdev)
>> prueth->emac[PRUETH_MAC1]->ndev;
>> }
>>
>> + eth_random_addr(prueth->base_mac);
>> +
>> dev_info(dev, "TI PRU ethernet driver initialized: %s EMAC mode\n",
>> (!eth0_node || !eth1_node) ? "single" : "dual");
>>
>> diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth.h
>> b/drivers/net/ethernet/ti/icssm/icssm_prueth.h
>> index 1709b3b6c2be..8a5f1647466a 100644
>> --- a/drivers/net/ethernet/ti/icssm/icssm_prueth.h
>> +++ b/drivers/net/ethernet/ti/icssm/icssm_prueth.h
>> @@ -28,6 +28,9 @@
>> #define EMAC_MAX_FRM_SUPPORT (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN + \
>> ICSSM_LRE_TAG_SIZE)
>>
>> +/* default timer for NSP and HSR/PRP */
>> +#define PRUETH_NSP_TIMER_MS (100) /* Refresh NSP counters every 100ms */
>> +
>> #define PRUETH_REG_DUMP_VER 1
>>
>> /* Encoding: 32-16: Reserved, 16-8: Reg dump version, 8-0: Ethertype */
>
> remove extra ' ' after Ethertype
>
>> @@ -293,6 +296,29 @@ enum prueth_mem {
>> PRUETH_MEM_MAX,
>> };
>>
>> +/* Firmware offsets/size information */
>> +struct prueth_fw_offsets {
>> + u32 index_array_offset;
>> + u32 bin_array_offset;
>> + u32 nt_array_offset;
>> + u32 index_array_loc;
>> + u32 bin_array_loc;
>> + u32 nt_array_loc;
>> + u32 index_array_max_entries;
>> + u32 bin_array_max_entries;
>> + u32 nt_array_max_entries;
>> + u32 vlan_ctrl_byte;
>> + u32 vlan_filter_tbl;
>> + u32 mc_ctrl_byte;
>> + u32 mc_filter_mask;
>> + u32 mc_filter_tbl;
>> + /* IEP wrap is used in the rx packet ordering logic and
>> + * is different for ICSSM v1.0 vs 2.1
>> + */
>> + u32 iep_wrap;
>> + u16 hash_mask;
>> +};
>> +
> [clip]
>> @@ -0,0 +1,120 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +/* Copyright (C) 2015-2021 Texas Instruments Incorporated -
>> https://urldefense.com/v3/__https://www.ti.com__;!!ACWV5N9M2RV99hQ!Pnt8LQPwsRI73TtUPzBpwVw_Cn90DbuNXinXJ5m2isPHfFxjNTp4JBlr6UedPapFerELKSzV4SFNoiUfE1xa8g$
>> + *
>> + * This file contains VLAN/Multicast filtering feature memory map
>> + *
>> + */
>> +
>> +#ifndef ICSS_VLAN_MULTICAST_FILTER_MM_H
>> +#define ICSS_VLAN_MULTICAST_FILTER_MM_H
>> +
>> +/* VLAN/Multicast filter defines & offsets,
>> + * present on both PRU0 and PRU1 DRAM
>
> remove extra ' '
>
>> + */
>> +
>> +/* Feature enable/disable values for multicast filtering */
>> +#define ICSS_EMAC_FW_MULTICAST_FILTER_CTRL_DISABLED 0x00
>> +#define ICSS_EMAC_FW_MULTICAST_FILTER_CTRL_ENABLED 0x01
>> +
>> +/* Feature enable/disable values for VLAN filtering */
>
> remove extra ' ' after values
>
>> +#define ICSS_EMAC_FW_VLAN_FILTER_CTRL_DISABLED 0x00
>> +#define ICSS_EMAC_FW_VLAN_FILTER_CTRL_ENABLED 0x01
>> +
>> +/* Add/remove multicast mac id for filtering bin */
>> +#define ICSS_EMAC_FW_MULTICAST_FILTER_HOST_RCV_ALLOWED 0x01
>> +#define ICSS_EMAC_FW_MULTICAST_FILTER_HOST_RCV_NOT_ALLOWED 0x00
>> +
>> +/* Default HASH value for the multicast filtering Mask */
>> +#define ICSS_EMAC_FW_MULTICAST_FILTER_INIT_VAL 0xFF
>> +
>> +/* Size requirements for Multicast filtering feature */
>> +#define ICSS_EMAC_FW_MULTICAST_TABLE_SIZE_BYTES 256
>> +#define ICSS_EMAC_FW_MULTICAST_FILTER_MASK_SIZE_BYTES 6
>> +#define ICSS_EMAC_FW_MULTICAST_FILTER_CTRL_SIZE_BYTES 1
>> +#define ICSS_EMAC_FW_MULTICAST_FILTER_MASK_OVERRIDE_STATUS_SIZE_BYTES 1
>> +#define ICSS_EMAC_FW_MULTICAST_FILTER_DROP_CNT_SIZE_BYTES 4
>> +
>> +/* Size requirements for VLAN filtering feature : 4096 bits = 512 bytes */
>> +#define ICSS_EMAC_FW_VLAN_FILTER_TABLE_SIZE_BYTES 512
>> +#define ICSS_EMAC_FW_VLAN_FILTER_CTRL_SIZE_BYTES 1
>> +#define ICSS_EMAC_FW_VLAN_FILTER_DROP_CNT_SIZE_BYTES 4
>> +
>> +/* Mask override set status */
>> +#define ICSS_EMAC_FW_MULTICAST_FILTER_MASK_OVERRIDE_SET 1
>> +/* Mask override not set status */
>> +#define ICSS_EMAC_FW_MULTICAST_FILTER_MASK_OVERRIDE_NOT_SET 0
>> +/* 6 bytes HASH Mask for the MAC */
>> +#define ICSS_EMAC_FW_MULTICAST_FILTER_MASK_OFFSET 0xF4
>> +/* 0 -> multicast filtering disabled | 1 -> multicast filtering enabled */
>> +#define ICSS_EMAC_FW_MULTICAST_FILTER_CTRL_OFFSET \
>> + (ICSS_EMAC_FW_MULTICAST_FILTER_MASK_OFFSET + \
>> + ICSS_EMAC_FW_MULTICAST_FILTER_MASK_SIZE_BYTES)
>> +/* Status indicating if the HASH override is done or not: 0: no, 1: yes */
>> +#define ICSS_EMAC_FW_MULTICAST_FILTER_OVERRIDE_STATUS \
>> + (ICSS_EMAC_FW_MULTICAST_FILTER_CTRL_OFFSET + \
>> + ICSS_EMAC_FW_MULTICAST_FILTER_CTRL_SIZE_BYTES)
>> +/* Multicast drop statistics */
>> +#define ICSS_EMAC_FW_MULTICAST_FILTER_DROP_CNT_OFFSET \
>> + (ICSS_EMAC_FW_MULTICAST_FILTER_OVERRIDE_STATUS +\
>> + ICSS_EMAC_FW_MULTICAST_FILTER_MASK_OVERRIDE_STATUS_SIZE_BYTES)
>> +/* Multicast table */
>> +#define ICSS_EMAC_FW_MULTICAST_FILTER_TABLE \
>> + (ICSS_EMAC_FW_MULTICAST_FILTER_DROP_CNT_OFFSET +\
>> + ICSS_EMAC_FW_MULTICAST_FILTER_DROP_CNT_SIZE_BYTES)
>> +
>> +/* Multicast filter defines & offsets for LRE
>> + */
>> +#define ICSS_LRE_FW_MULTICAST_TABLE_SEARCH_OP_CONTROL_BIT 0xE0
>> +/* one byte field :
>> + * 0 -> multicast filtering disabled
>> + * 1 -> multicast filtering enabled
>> + */
>> +#define ICSS_LRE_FW_MULTICAST_FILTER_MASK 0xE4
>> +#define ICSS_LRE_FW_MULTICAST_FILTER_TABLE 0x100
>> +
>> +/* VLAN table Offsets */
>> +#define ICSS_EMAC_FW_VLAN_FLTR_TBL_BASE_ADDR 0x200
>> +#define ICSS_EMAC_FW_VLAN_FILTER_CTRL_BITMAP_OFFSET 0xEF
>> +#define ICSS_EMAC_FW_VLAN_FILTER_DROP_CNT_OFFSET \
>> + (ICSS_EMAC_FW_VLAN_FILTER_CTRL_BITMAP_OFFSET + \
>> + ICSS_EMAC_FW_VLAN_FILTER_CTRL_SIZE_BYTES)
>> +
>> +/* VLAN filter Control Bit maps */
>> +/* one bit field, bit 0: | 0 : VLAN filter disabled (default),
>> + * 1: VLAN filter enabled
>> + */
>> +#define ICSS_EMAC_FW_VLAN_FILTER_CTRL_ENABLE_BIT 0
>> +/* one bit field, bit 1: | 0 : untagged host rcv allowed (default),
>> + * 1: untagged host rcv not allowed
>> + */
>> +#define ICSS_EMAC_FW_VLAN_FILTER_UNTAG_HOST_RCV_ALLOW_CTRL_BIT 1
>> +/* one bit field, bit 1: | 0 : priotag host rcv allowed (default),
>> + * 1: priotag host rcv not allowed
>> + */
>> +#define ICSS_EMAC_FW_VLAN_FILTER_PRIOTAG_HOST_RCV_ALLOW_CTRL_BIT 2
>> +/* one bit field, bit 1: | 0 : skip sv vlan flow
>> + * :1 : take sv vlan flow (not applicable for dual emac )
>> + */
>> +#define ICSS_EMAC_FW_VLAN_FILTER_SV_VLAN_FLOW_HOST_RCV_ALLOW_CTRL_BIT 3
>> +
>> +/* VLAN IDs */
>> +#define ICSS_EMAC_FW_VLAN_FILTER_PRIOTAG_VID 0
>> +#define ICSS_EMAC_FW_VLAN_FILTER_VID_MIN 0x0000
>> +#define ICSS_EMAC_FW_VLAN_FILTER_VID_MAX 0x0FFF
>> +
>> +/* VLAN Filtering Commands */
>> +#define ICSS_EMAC_FW_VLAN_FILTER_ADD_VLAN_VID_CMD 0x00
>> +#define ICSS_EMAC_FW_VLAN_FILTER_REMOVE_VLAN_VID_CMD 0x01
>> +
>> +/* Switch defines for VLAN/MC filtering */
>> +/* SRAM
>> + * VLAN filter defines & offsets
>> + */
>> +#define ICSS_LRE_FW_VLAN_FLTR_CTRL_BYTE 0x1FE
>
> lowercase hex please, all place.
>

Thank you for the feedback.

We have addressed all the comments provided, except for the one regarding
hexadecimal format, where we need some clarity.

Is there any specific style guide or convention that mandates lowercase
hexadecimal formatting? We've observed that other drivers in the code base
consistently use uppercase hex values, and we've followed the same convention
throughout in our implementation.

Thanks and Regards,
Parvathi.