[PATCH net-next] net: ethernet: ti: am65-cpsw: Add priv-flag for Switch VLAN Aware mode

[PATCH net-next] net: ethernet: ti: am65-cpsw: Add priv-flag for Switch VLAN Aware mode

[Siddharth Vadapalli]

The CPSW Ethernet Switch on TI's K3 SoCs can be configured to operate in
VLAN Aware or VLAN Unaware modes of operation. This is different from
the ALE being VLAN Aware and Unaware. The Ethernet Switch being VLAN Aware
results in the addition/removal/replacement of VLAN tag of packets during
egress as described in section "12.2.1.4.6.4.1 Transmit VLAN Processing" of
the AM65x Technical Reference Manual available at:
https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
In VLAN Unaware mode, packets remain unmodified on egress.

The driver currently configures the Ethernet Switch in VLAN Aware mode by
default and there is no support to toggle this capability of the Ethernet
Switch at runtime. Thus, add support to toggle the capability by exporting
it via the ethtool "priv-flags" interface.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---

Hello,

This patch is based on net-next main branch's commit:
55a7246025cd Merge branch 'mptcp-various-small-improvements'

Regards,
Siddharth.

 drivers/net/ethernet/ti/am65-cpsw-ethtool.c |  9 ++++++++-
 drivers/net/ethernet/ti/am65-cpsw-nuss.c    | 12 +++++++++---
 drivers/net/ethernet/ti/am65-cpsw-nuss.h    |  1 +
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-ethtool.c b/drivers/net/ethernet/ti/am65-cpsw-ethtool.c
index d6ce2c9f0a8d..42c317874b75 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-ethtool.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-ethtool.c
@@ -374,6 +374,8 @@ static const struct am65_cpsw_ethtool_stat am65_slave_stats[] = {
 static const char am65_cpsw_ethtool_priv_flags[][ETH_GSTRING_LEN] = {
 #define	AM65_CPSW_PRIV_P0_RX_PTYPE_RROBIN	BIT(0)
 	"p0-rx-ptype-rrobin",
+#define	AM65_CPSW_PRIV_VLAN_AWARE		BIT(1)
+	"cpsw-vlan-aware",
 };
 
 static int am65_cpsw_ethtool_op_begin(struct net_device *ndev)
@@ -720,15 +722,19 @@ static u32 am65_cpsw_get_ethtool_priv_flags(struct net_device *ndev)
 	if (common->pf_p0_rx_ptype_rrobin)
 		priv_flags |= AM65_CPSW_PRIV_P0_RX_PTYPE_RROBIN;
 
+	if (common->cpsw_vlan_aware)
+		priv_flags |= AM65_CPSW_PRIV_VLAN_AWARE;
+
 	return priv_flags;
 }
 
 static int am65_cpsw_set_ethtool_priv_flags(struct net_device *ndev, u32 flags)
 {
 	struct am65_cpsw_common *common = am65_ndev_to_common(ndev);
-	int rrobin;
+	int rrobin, cpsw_vlan_aware;
 
 	rrobin = !!(flags & AM65_CPSW_PRIV_P0_RX_PTYPE_RROBIN);
+	cpsw_vlan_aware = !!(flags & AM65_CPSW_PRIV_VLAN_AWARE);
 
 	if (common->usage_count)
 		return -EBUSY;
@@ -740,6 +746,7 @@ static int am65_cpsw_set_ethtool_priv_flags(struct net_device *ndev, u32 flags)
 	}
 
 	common->pf_p0_rx_ptype_rrobin = rrobin;
+	common->cpsw_vlan_aware = cpsw_vlan_aware;
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 9d2f4ac783e4..fec2a5968a12 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -451,9 +451,14 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
 		return 0;
 
 	/* Control register */
-	writel(AM65_CPSW_CTL_P0_ENABLE | AM65_CPSW_CTL_P0_TX_CRC_REMOVE |
-	       AM65_CPSW_CTL_VLAN_AWARE | AM65_CPSW_CTL_P0_RX_PAD,
-	       common->cpsw_base + AM65_CPSW_REG_CTL);
+	val = AM65_CPSW_CTL_P0_ENABLE | AM65_CPSW_CTL_P0_TX_CRC_REMOVE |
+	      AM65_CPSW_CTL_P0_RX_PAD;
+
+	if (common->cpsw_vlan_aware)
+		val |= AM65_CPSW_CTL_VLAN_AWARE;
+
+	writel(val, common->cpsw_base + AM65_CPSW_REG_CTL);
+
 	/* Max length register */
 	writel(AM65_CPSW_MAX_PACKET_SIZE,
 	       host_p->port_base + AM65_CPSW_PORT_REG_RX_MAXLEN);
@@ -2977,6 +2982,7 @@ static int am65_cpsw_nuss_probe(struct platform_device *pdev)
 	init_completion(&common->tdown_complete);
 	common->tx_ch_num = AM65_CPSW_DEFAULT_TX_CHNS;
 	common->pf_p0_rx_ptype_rrobin = false;
+	common->cpsw_vlan_aware = true;
 	common->default_vlan = 1;
 
 	common->ports = devm_kcalloc(dev, common->port_num,
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.h b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
index 7da0492dc091..91f625ea3859 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.h
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
@@ -147,6 +147,7 @@ struct am65_cpsw_common {
 	u32			cpsw_ver;
 	unsigned long		bus_freq;
 	bool			pf_p0_rx_ptype_rrobin;
+	bool			cpsw_vlan_aware;
 	struct am65_cpts	*cpts;
 	int			est_enabled;
 	bool			iet_enabled;
-- 
2.34.1

Re: [PATCH net-next] net: ethernet: ti: am65-cpsw: Add priv-flag for Switch VLAN Aware mode

[Jiri Pirko]

Tue, Feb 27, 2024 at 09:28:15AM CET, s-vadapalli@ti.com wrote:
>The CPSW Ethernet Switch on TI's K3 SoCs can be configured to operate in
>VLAN Aware or VLAN Unaware modes of operation. This is different from
>the ALE being VLAN Aware and Unaware. The Ethernet Switch being VLAN Aware
>results in the addition/removal/replacement of VLAN tag of packets during
>egress as described in section "12.2.1.4.6.4.1 Transmit VLAN Processing" of
>the AM65x Technical Reference Manual available at:
>https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
>In VLAN Unaware mode, packets remain unmodified on egress.
>
>The driver currently configures the Ethernet Switch in VLAN Aware mode by
>default and there is no support to toggle this capability of the Ethernet
>Switch at runtime. Thus, add support to toggle the capability by exporting
>it via the ethtool "priv-flags" interface.

I don't follow. You have all the means to offload all bridge/vlan
configurations properly and setup your hw according to that. See mlxsw
for a reference. I don't see the need for any custom driver knobs.

Re: [PATCH net-next] net: ethernet: ti: am65-cpsw: Add priv-flag for Switch VLAN Aware mode

[Siddharth Vadapalli]

On 27/02/24 18:09, Jiri Pirko wrote:
> Tue, Feb 27, 2024 at 09:28:15AM CET, s-vadapalli@ti.com wrote:
>> The CPSW Ethernet Switch on TI's K3 SoCs can be configured to operate in
>> VLAN Aware or VLAN Unaware modes of operation. This is different from
>> the ALE being VLAN Aware and Unaware. The Ethernet Switch being VLAN Aware
>> results in the addition/removal/replacement of VLAN tag of packets during
>> egress as described in section "12.2.1.4.6.4.1 Transmit VLAN Processing" of
>> the AM65x Technical Reference Manual available at:
>> https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
>> In VLAN Unaware mode, packets remain unmodified on egress.
>>
>> The driver currently configures the Ethernet Switch in VLAN Aware mode by
>> default and there is no support to toggle this capability of the Ethernet
>> Switch at runtime. Thus, add support to toggle the capability by exporting
>> it via the ethtool "priv-flags" interface.
> 
> I don't follow. You have all the means to offload all bridge/vlan
> configurations properly and setup your hw according to that. See mlxsw
> for a reference. I don't see the need for any custom driver knobs.
> 

Thank you for reviewing the patch. Please note that the "VLAN Aware mode" being
referred to here is different from ALE being VLAN aware. The hw offload of
bridge/vlan configurations is already supported in the context of the ALE. The
Ethernet Switch being VLAN Aware is a layer on top of that, which enables
further processing on top of the untagged/VLAN packets. This patch aims to
provide a method to enable the following use-cases:
1. ALE VLAN Aware + CPSW VLAN Aware
2. ALE VLAN Aware + CPSW VLAN Unaware

All hw offloads of bridge/vlan configurations are w.r.t. ALE VLAN Aware alone.
Currently, only use-case 1 is enabled by the driver by default and there is no
knob to toggle to use-case 2.

I am quoting sections of the Technical Reference Manual mentioned in my commit
message, in order to clarify the CPSW VLAN Unaware and CPSW VLAN Aware terminology.

CPSW VLAN Unaware:
Transmit packets are NOT modified during switch egress.

CPSW VLAN Aware:
1. Untagged Packet Operations
Untagged packets are all packets that are not a VLAN packet or a priority tagged
packet. According to the CPWS0_FORCE_UNTAGGED_EGRESS_REG[1-0] MASK bit in the
packet header the packet may exit the switch with a VLAN tag inserted or the
packet may leave the switch unchanged....
2. Priority Tagged Packet Operations (VLAN VID == 0 && EN_VID0_MODE ==0h)
Priority tagged packets are packets that contain a VLAN header with VID = 0.
According to the CPSW_ALE_FORCE_UNTAGGED_EGRESS_REG[1-0] MASK bit in the packet
header, priority tagged packets may exit the switch with their VLAN ID and
priority replaced or they may have their priority tag completely removed....
3. VLAN Tagged Packet Operations (VLAN VID != 0 || (EN_VID0_MODE ==1h && VLAN
VID ==0))
VLAN tagged packets are packets that contain a VLAN header specifying the VLAN
the packet belongs to
(VID), the packet priority (PRI), and the drop eligibility indicator (CFI).
According to the CPSW_ALE_FORCE_UNTAGGED_EGRESS_REG[1-0] MASK bit in the packet
header, VLAN tagged packets may exit the switch with their VLAN priority
replaced or they may have their VLAN header completely removed...

I hope that this clarifies that CPSW VLAN Unaware/Aware is a layer on top of the
hw offload-able bridge/vlan configuration.  Please let me know if there is
anything specific that could enable this without requiring the "priv-flag" based
implementation of this patch.

-- 
Regards,
Siddharth.

Re: [PATCH net-next] net: ethernet: ti: am65-cpsw: Add priv-flag for Switch VLAN Aware mode

[Jiri Pirko]

Wed, Feb 28, 2024 at 08:06:39AM CET, s-vadapalli@ti.com wrote:
>
>
>On 27/02/24 18:09, Jiri Pirko wrote:
>> Tue, Feb 27, 2024 at 09:28:15AM CET, s-vadapalli@ti.com wrote:
>>> The CPSW Ethernet Switch on TI's K3 SoCs can be configured to operate in
>>> VLAN Aware or VLAN Unaware modes of operation. This is different from
>>> the ALE being VLAN Aware and Unaware. The Ethernet Switch being VLAN Aware
>>> results in the addition/removal/replacement of VLAN tag of packets during
>>> egress as described in section "12.2.1.4.6.4.1 Transmit VLAN Processing" of
>>> the AM65x Technical Reference Manual available at:
>>> https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
>>> In VLAN Unaware mode, packets remain unmodified on egress.
>>>
>>> The driver currently configures the Ethernet Switch in VLAN Aware mode by
>>> default and there is no support to toggle this capability of the Ethernet
>>> Switch at runtime. Thus, add support to toggle the capability by exporting
>>> it via the ethtool "priv-flags" interface.
>> 
>> I don't follow. You have all the means to offload all bridge/vlan
>> configurations properly and setup your hw according to that. See mlxsw
>> for a reference. I don't see the need for any custom driver knobs.
>> 
>
>Thank you for reviewing the patch. Please note that the "VLAN Aware mode" being
>referred to here is different from ALE being VLAN aware. The hw offload of
>bridge/vlan configurations is already supported in the context of the ALE. The
>Ethernet Switch being VLAN Aware is a layer on top of that, which enables
>further processing on top of the untagged/VLAN packets. This patch aims to
>provide a method to enable the following use-cases:
>1. ALE VLAN Aware + CPSW VLAN Aware
>2. ALE VLAN Aware + CPSW VLAN Unaware
>
>All hw offloads of bridge/vlan configurations are w.r.t. ALE VLAN Aware alone.
>Currently, only use-case 1 is enabled by the driver by default and there is no
>knob to toggle to use-case 2.
>
>I am quoting sections of the Technical Reference Manual mentioned in my commit
>message, in order to clarify the CPSW VLAN Unaware and CPSW VLAN Aware terminology.
>
>CPSW VLAN Unaware:
>Transmit packets are NOT modified during switch egress.
>
>CPSW VLAN Aware:
>1. Untagged Packet Operations
>Untagged packets are all packets that are not a VLAN packet or a priority tagged
>packet. According to the CPWS0_FORCE_UNTAGGED_EGRESS_REG[1-0] MASK bit in the
>packet header the packet may exit the switch with a VLAN tag inserted or the
>packet may leave the switch unchanged....
>2. Priority Tagged Packet Operations (VLAN VID == 0 && EN_VID0_MODE ==0h)
>Priority tagged packets are packets that contain a VLAN header with VID = 0.
>According to the CPSW_ALE_FORCE_UNTAGGED_EGRESS_REG[1-0] MASK bit in the packet
>header, priority tagged packets may exit the switch with their VLAN ID and
>priority replaced or they may have their priority tag completely removed....
>3. VLAN Tagged Packet Operations (VLAN VID != 0 || (EN_VID0_MODE ==1h && VLAN
>VID ==0))
>VLAN tagged packets are packets that contain a VLAN header specifying the VLAN
>the packet belongs to
>(VID), the packet priority (PRI), and the drop eligibility indicator (CFI).
>According to the CPSW_ALE_FORCE_UNTAGGED_EGRESS_REG[1-0] MASK bit in the packet
>header, VLAN tagged packets may exit the switch with their VLAN priority
>replaced or they may have their VLAN header completely removed...
>
>I hope that this clarifies that CPSW VLAN Unaware/Aware is a layer on top of the
>hw offload-able bridge/vlan configuration.  Please let me know if there is
>anything specific that could enable this without requiring the "priv-flag" based
>implementation of this patch.

I have no clue what "ALE" is. But in general. User provided
configuration, using ip/bridge/etc tools/uapi. According to this
configuration, kernel is bahaving. When you do offload, you should just
make sure to mimic/mirror the kernel behaviour. With this in mind, why
can't you do it without adding additional knob? And if you really need
it because the know does some internal hw/fw tuning, priv flag of netdev
is most probably not the correct place to put it. If it is, make sure
you advocate for it properly in the patch description.

pw-bot: cr


>
>-- 
>Regards,
>Siddharth.

Re: [PATCH net-next] net: ethernet: ti: am65-cpsw: Add priv-flag for Switch VLAN Aware mode

[Siddharth Vadapalli]

On 28/02/24 13:53, Jiri Pirko wrote:
> Wed, Feb 28, 2024 at 08:06:39AM CET, s-vadapalli@ti.com wrote:
>>
>>
>> On 27/02/24 18:09, Jiri Pirko wrote:
>>> Tue, Feb 27, 2024 at 09:28:15AM CET, s-vadapalli@ti.com wrote:
>>>> The CPSW Ethernet Switch on TI's K3 SoCs can be configured to operate in
>>>> VLAN Aware or VLAN Unaware modes of operation. This is different from
>>>> the ALE being VLAN Aware and Unaware. The Ethernet Switch being VLAN Aware
>>>> results in the addition/removal/replacement of VLAN tag of packets during
>>>> egress as described in section "12.2.1.4.6.4.1 Transmit VLAN Processing" of
>>>> the AM65x Technical Reference Manual available at:
>>>> https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
>>>> In VLAN Unaware mode, packets remain unmodified on egress.
>>>>
>>>> The driver currently configures the Ethernet Switch in VLAN Aware mode by
>>>> default and there is no support to toggle this capability of the Ethernet
>>>> Switch at runtime. Thus, add support to toggle the capability by exporting
>>>> it via the ethtool "priv-flags" interface.
>>>
>>> I don't follow. You have all the means to offload all bridge/vlan
>>> configurations properly and setup your hw according to that. See mlxsw
>>> for a reference. I don't see the need for any custom driver knobs.
>>>
>>
>> Thank you for reviewing the patch. Please note that the "VLAN Aware mode" being
>> referred to here is different from ALE being VLAN aware. The hw offload of
>> bridge/vlan configurations is already supported in the context of the ALE. The
>> Ethernet Switch being VLAN Aware is a layer on top of that, which enables
>> further processing on top of the untagged/VLAN packets. This patch aims to
>> provide a method to enable the following use-cases:
>> 1. ALE VLAN Aware + CPSW VLAN Aware
>> 2. ALE VLAN Aware + CPSW VLAN Unaware
>>
>> All hw offloads of bridge/vlan configurations are w.r.t. ALE VLAN Aware alone.
>> Currently, only use-case 1 is enabled by the driver by default and there is no
>> knob to toggle to use-case 2.
>>
>> I am quoting sections of the Technical Reference Manual mentioned in my commit
>> message, in order to clarify the CPSW VLAN Unaware and CPSW VLAN Aware terminology.
>>
>> CPSW VLAN Unaware:
>> Transmit packets are NOT modified during switch egress.
>>
>> CPSW VLAN Aware:
>> 1. Untagged Packet Operations
>> Untagged packets are all packets that are not a VLAN packet or a priority tagged
>> packet. According to the CPWS0_FORCE_UNTAGGED_EGRESS_REG[1-0] MASK bit in the
>> packet header the packet may exit the switch with a VLAN tag inserted or the
>> packet may leave the switch unchanged....
>> 2. Priority Tagged Packet Operations (VLAN VID == 0 && EN_VID0_MODE ==0h)
>> Priority tagged packets are packets that contain a VLAN header with VID = 0.
>> According to the CPSW_ALE_FORCE_UNTAGGED_EGRESS_REG[1-0] MASK bit in the packet
>> header, priority tagged packets may exit the switch with their VLAN ID and
>> priority replaced or they may have their priority tag completely removed....
>> 3. VLAN Tagged Packet Operations (VLAN VID != 0 || (EN_VID0_MODE ==1h && VLAN
>> VID ==0))
>> VLAN tagged packets are packets that contain a VLAN header specifying the VLAN
>> the packet belongs to
>> (VID), the packet priority (PRI), and the drop eligibility indicator (CFI).
>> According to the CPSW_ALE_FORCE_UNTAGGED_EGRESS_REG[1-0] MASK bit in the packet
>> header, VLAN tagged packets may exit the switch with their VLAN priority
>> replaced or they may have their VLAN header completely removed...
>>
>> I hope that this clarifies that CPSW VLAN Unaware/Aware is a layer on top of the
>> hw offload-able bridge/vlan configuration.  Please let me know if there is
>> anything specific that could enable this without requiring the "priv-flag" based
>> implementation of this patch.
> 
> I have no clue what "ALE" is. But in general. User provided

ALE is Address Lookup Engine.

> configuration, using ip/bridge/etc tools/uapi. According to this
> configuration, kernel is bahaving. When you do offload, you should just
> make sure to mimic/mirror the kernel behaviour. With this in mind, why

What if there is no kernel behavior associated with it? How can it be mimicked
then? This patch isn't offloading any feature that is supported in software. It
might not be possible to offload features which act on the forwarding path of
packets entirely in Hardware within the Ethernet Switch.

Please consider the following:
Untagged packets sent from Software via the corresponding VLAN interfaces will
be tagged which is the expected behavior. However, if this is offloaded, it will
imply that even untagged packets that are simply forwarded in the Ethernet
Switch and never get to software will also have to be tagged by the Ethernet
Switch. This is not allowing the choice of leaving untagged packets as-is on the
Ethernet Switch's forwarding path. This patch attempts to allow configuring
something quite similar to this, where it is possible to *choose* whether or not
to tag packets being forwarded.

> can't you do it without adding additional knob? And if you really need
> it because the know does some internal hw/fw tuning, priv flag of netdev

The feature can be turned on or off depending on the use-case. Is it acceptable
to have build configs scattered in the driver code? I don't suppose that is
acceptable, due to which it will be preferable to have a runtime configuration
option, which is what this patch provides.

> is most probably not the correct place to put it. If it is, make sure

Please suggest an alternative if this isn't the right place. Otherwise, I can
only assume that there isn't one.

> you advocate for it properly in the patch description.
> 
> pw-bot: cr
>

-- 
Regards,
Siddharth.

Re: [PATCH net-next] net: ethernet: ti: am65-cpsw: Add priv-flag for Switch VLAN Aware mode

[Jiri Pirko]

Wed, Feb 28, 2024 at 11:04:55AM CET, s-vadapalli@ti.com wrote:
>
>
>On 28/02/24 13:53, Jiri Pirko wrote:
>> Wed, Feb 28, 2024 at 08:06:39AM CET, s-vadapalli@ti.com wrote:
>>>
>>>
>>> On 27/02/24 18:09, Jiri Pirko wrote:
>>>> Tue, Feb 27, 2024 at 09:28:15AM CET, s-vadapalli@ti.com wrote:
>>>>> The CPSW Ethernet Switch on TI's K3 SoCs can be configured to operate in
>>>>> VLAN Aware or VLAN Unaware modes of operation. This is different from
>>>>> the ALE being VLAN Aware and Unaware. The Ethernet Switch being VLAN Aware
>>>>> results in the addition/removal/replacement of VLAN tag of packets during
>>>>> egress as described in section "12.2.1.4.6.4.1 Transmit VLAN Processing" of
>>>>> the AM65x Technical Reference Manual available at:
>>>>> https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
>>>>> In VLAN Unaware mode, packets remain unmodified on egress.
>>>>>
>>>>> The driver currently configures the Ethernet Switch in VLAN Aware mode by
>>>>> default and there is no support to toggle this capability of the Ethernet
>>>>> Switch at runtime. Thus, add support to toggle the capability by exporting
>>>>> it via the ethtool "priv-flags" interface.
>>>>
>>>> I don't follow. You have all the means to offload all bridge/vlan
>>>> configurations properly and setup your hw according to that. See mlxsw
>>>> for a reference. I don't see the need for any custom driver knobs.
>>>>
>>>
>>> Thank you for reviewing the patch. Please note that the "VLAN Aware mode" being
>>> referred to here is different from ALE being VLAN aware. The hw offload of
>>> bridge/vlan configurations is already supported in the context of the ALE. The
>>> Ethernet Switch being VLAN Aware is a layer on top of that, which enables
>>> further processing on top of the untagged/VLAN packets. This patch aims to
>>> provide a method to enable the following use-cases:
>>> 1. ALE VLAN Aware + CPSW VLAN Aware
>>> 2. ALE VLAN Aware + CPSW VLAN Unaware
>>>
>>> All hw offloads of bridge/vlan configurations are w.r.t. ALE VLAN Aware alone.
>>> Currently, only use-case 1 is enabled by the driver by default and there is no
>>> knob to toggle to use-case 2.
>>>
>>> I am quoting sections of the Technical Reference Manual mentioned in my commit
>>> message, in order to clarify the CPSW VLAN Unaware and CPSW VLAN Aware terminology.
>>>
>>> CPSW VLAN Unaware:
>>> Transmit packets are NOT modified during switch egress.
>>>
>>> CPSW VLAN Aware:
>>> 1. Untagged Packet Operations
>>> Untagged packets are all packets that are not a VLAN packet or a priority tagged
>>> packet. According to the CPWS0_FORCE_UNTAGGED_EGRESS_REG[1-0] MASK bit in the
>>> packet header the packet may exit the switch with a VLAN tag inserted or the
>>> packet may leave the switch unchanged....
>>> 2. Priority Tagged Packet Operations (VLAN VID == 0 && EN_VID0_MODE ==0h)
>>> Priority tagged packets are packets that contain a VLAN header with VID = 0.
>>> According to the CPSW_ALE_FORCE_UNTAGGED_EGRESS_REG[1-0] MASK bit in the packet
>>> header, priority tagged packets may exit the switch with their VLAN ID and
>>> priority replaced or they may have their priority tag completely removed....
>>> 3. VLAN Tagged Packet Operations (VLAN VID != 0 || (EN_VID0_MODE ==1h && VLAN
>>> VID ==0))
>>> VLAN tagged packets are packets that contain a VLAN header specifying the VLAN
>>> the packet belongs to
>>> (VID), the packet priority (PRI), and the drop eligibility indicator (CFI).
>>> According to the CPSW_ALE_FORCE_UNTAGGED_EGRESS_REG[1-0] MASK bit in the packet
>>> header, VLAN tagged packets may exit the switch with their VLAN priority
>>> replaced or they may have their VLAN header completely removed...
>>>
>>> I hope that this clarifies that CPSW VLAN Unaware/Aware is a layer on top of the
>>> hw offload-able bridge/vlan configuration.  Please let me know if there is
>>> anything specific that could enable this without requiring the "priv-flag" based
>>> implementation of this patch.
>> 
>> I have no clue what "ALE" is. But in general. User provided
>
>ALE is Address Lookup Engine.
>
>> configuration, using ip/bridge/etc tools/uapi. According to this
>> configuration, kernel is bahaving. When you do offload, you should just
>> make sure to mimic/mirror the kernel behaviour. With this in mind, why
>
>What if there is no kernel behavior associated with it? How can it be mimicked
>then? This patch isn't offloading any feature that is supported in software. It
>might not be possible to offload features which act on the forwarding path of
>packets entirely in Hardware within the Ethernet Switch.
>
>Please consider the following:
>Untagged packets sent from Software via the corresponding VLAN interfaces will
>be tagged which is the expected behavior. However, if this is offloaded, it will
>imply that even untagged packets that are simply forwarded in the Ethernet
>Switch and never get to software will also have to be tagged by the Ethernet
>Switch. This is not allowing the choice of leaving untagged packets as-is on the
>Ethernet Switch's forwarding path. This patch attempts to allow configuring
>something quite similar to this, where it is possible to *choose* whether or not
>to tag packets being forwarded.

What would kernel datapath do? That is the question you need to ask and
configure the hw accordingly. If 2 interfaces are in the bridge, vlans
involved, etc, the forward behavior is well defined, isn't it. What am I
missing?


>
>> can't you do it without adding additional knob? And if you really need
>> it because the know does some internal hw/fw tuning, priv flag of netdev
>
>The feature can be turned on or off depending on the use-case. Is it acceptable
>to have build configs scattered in the driver code? I don't suppose that is
>acceptable, due to which it will be preferable to have a runtime configuration
>option, which is what this patch provides.
>
>> is most probably not the correct place to put it. If it is, make sure
>
>Please suggest an alternative if this isn't the right place. Otherwise, I can
>only assume that there isn't one.
>
>> you advocate for it properly in the patch description.
>> 
>> pw-bot: cr
>>
>
>-- 
>Regards,
>Siddharth.

Re: [PATCH net-next] net: ethernet: ti: am65-cpsw: Add priv-flag for Switch VLAN Aware mode

[Andrew Lunn]

> What if there is no kernel behavior associated with it? How can it be mimicked
> then?

Simple. Implement the feature in software in the kernel for
everybody. Then offload it to your hardware.

Your hardware is an accelerator. You use it to accelerate what linux
can already do. If Linux does not have the feature your accelerator
has, that accelerator feature goes unused.

> Please consider the following:
> Untagged packets sent from Software via the corresponding VLAN interfaces will
> be tagged which is the expected behavior. However, if this is offloaded, it will
> imply that even untagged packets that are simply forwarded in the Ethernet
> Switch and never get to software will also have to be tagged by the Ethernet
> Switch. This is not allowing the choice of leaving untagged packets as-is on the
> Ethernet Switch's forwarding path. This patch attempts to allow configuring
> something quite similar to this, where it is possible to *choose* whether or not
> to tag packets being forwarded.

So step back. Forget about your accelerator. Use just a Linux software
bridge. Describe what you want the Linux software bridge to do.

	Andrew

Re: [PATCH net-next] net: ethernet: ti: am65-cpsw: Add priv-flag for Switch VLAN Aware mode

[Siddharth Vadapalli]

On Wed, Feb 28, 2024 at 02:36:55PM +0100, Andrew Lunn wrote:
> > What if there is no kernel behavior associated with it? How can it be mimicked
> > then?
> 
> Simple. Implement the feature in software in the kernel for
> everybody. Then offload it to your hardware.
> 
> Your hardware is an accelerator. You use it to accelerate what linux
> can already do. If Linux does not have the feature your accelerator
> has, that accelerator feature goes unused.

Is it acceptable to have a macro in the Ethernet Driver to conditionally
disable/enable the feature (via setting the corresponding bit in the
register)?

The current implementation is:

	/* Control register */
	writel(AM65_CPSW_CTL_P0_ENABLE | AM65_CPSW_CTL_P0_TX_CRC_REMOVE |
	       AM65_CPSW_CTL_VLAN_AWARE | AM65_CPSW_CTL_P0_RX_PAD,
	       common->cpsw_base + AM65_CPSW_REG_CTL);

which sets the "AM65_CPSW_CTL_VLAN_AWARE" bit by default.

Could it be changed to:

#define TI_K3_CPSW_VLAN_AWARE 1

....

	/* Control register */
	val = AM65_CPSW_CTL_P0_ENABLE | AM65_CPSW_CTL_P0_TX_CRC_REMOVE |
	      AM65_CPSW_CTL_P0_RX_PAD;

#ifdef TI_K3_CPSW_VLAN_AWARE
	val |= AM65_CPSW_CTL_VLAN_AWARE;
#endif

	writel(val, common->cpsw_base + AM65_CPSW_REG_CTL);

Since no additional configuration is necessary to disable/enable the
functionality except clearing/setting a bit in a register, I am unsure of
the implementation for the offloading part being suggested. Please let me
know if the above implementation is an acceptable alternative.

Regards,
Siddharth.

Re: [PATCH net-next] net: ethernet: ti: am65-cpsw: Add priv-flag for Switch VLAN Aware mode

[Roger Quadros]

On 29/02/2024 11:27, Siddharth Vadapalli wrote:
> On Wed, Feb 28, 2024 at 02:36:55PM +0100, Andrew Lunn wrote:
>>> What if there is no kernel behavior associated with it? How can it be mimicked
>>> then?
>>
>> Simple. Implement the feature in software in the kernel for
>> everybody. Then offload it to your hardware.
>>
>> Your hardware is an accelerator. You use it to accelerate what linux
>> can already do. If Linux does not have the feature your accelerator
>> has, that accelerator feature goes unused.
> 
> Is it acceptable to have a macro in the Ethernet Driver to conditionally
> disable/enable the feature (via setting the corresponding bit in the
> register)?
> 
> The current implementation is:
> 
> 	/* Control register */
> 	writel(AM65_CPSW_CTL_P0_ENABLE | AM65_CPSW_CTL_P0_TX_CRC_REMOVE |
> 	       AM65_CPSW_CTL_VLAN_AWARE | AM65_CPSW_CTL_P0_RX_PAD,
> 	       common->cpsw_base + AM65_CPSW_REG_CTL);
> 
> which sets the "AM65_CPSW_CTL_VLAN_AWARE" bit by default.
> 
> Could it be changed to:
> 
> #define TI_K3_CPSW_VLAN_AWARE 1
> 
> ....
> 
> 	/* Control register */
> 	val = AM65_CPSW_CTL_P0_ENABLE | AM65_CPSW_CTL_P0_TX_CRC_REMOVE |
> 	      AM65_CPSW_CTL_P0_RX_PAD;
> 
> #ifdef TI_K3_CPSW_VLAN_AWARE
> 	val |= AM65_CPSW_CTL_VLAN_AWARE;
> #endif
> 
> 	writel(val, common->cpsw_base + AM65_CPSW_REG_CTL);
> 
> Since no additional configuration is necessary to disable/enable the
> functionality except clearing/setting a bit in a register, I am unsure of
> the implementation for the offloading part being suggested. Please let me
> know if the above implementation is an acceptable alternative.

This doesn't really solve the problem as it leaves the question open as to
who will set TI_K3_CPSW_VLAN_AWARE. And the configuration is then fixed at build.

Can you please explain in which scenario the default case does not work for you?
Why would end user want to disable VLAN_AWARE mode?

TRM states
"Transmit packets are NOT modified during switch egress when the VLAN_AWARE bit in the
CPSW_CONTROL_REG register is cleared to 0h. This means that the switch is not in VLAN-aware mode."

The same problem would also apply to cpsw.c and cpsw_new.c correct?

A bit later the driver does this

        /* switch to vlan unaware mode */
        cpsw_ale_control_set(common->ale, HOST_PORT_NUM, ALE_VLAN_AWARE, 1);
        cpsw_ale_control_set(common->ale, HOST_PORT_NUM,
                             ALE_PORT_STATE, ALE_PORT_STATE_FORWARD);

The comment says vlan unaware but code is setting ALE_VLAN_AWARE to 1.
Is the comment wrong?

-- 
cheers,
-roger

Re: [PATCH net-next] net: ethernet: ti: am65-cpsw: Add priv-flag for Switch VLAN Aware mode

[Siddharth Vadapalli]

On Thu, Feb 29, 2024 at 12:52:07PM +0200, Roger Quadros wrote:
> 
> 
> On 29/02/2024 11:27, Siddharth Vadapalli wrote:
> > On Wed, Feb 28, 2024 at 02:36:55PM +0100, Andrew Lunn wrote:
> >>> What if there is no kernel behavior associated with it? How can it be mimicked
> >>> then?
> >>
> >> Simple. Implement the feature in software in the kernel for
> >> everybody. Then offload it to your hardware.
> >>
> >> Your hardware is an accelerator. You use it to accelerate what linux
> >> can already do. If Linux does not have the feature your accelerator
> >> has, that accelerator feature goes unused.
> > 
> > Is it acceptable to have a macro in the Ethernet Driver to conditionally
> > disable/enable the feature (via setting the corresponding bit in the
> > register)?
> > 
> > The current implementation is:
> > 
> > 	/* Control register */
> > 	writel(AM65_CPSW_CTL_P0_ENABLE | AM65_CPSW_CTL_P0_TX_CRC_REMOVE |
> > 	       AM65_CPSW_CTL_VLAN_AWARE | AM65_CPSW_CTL_P0_RX_PAD,
> > 	       common->cpsw_base + AM65_CPSW_REG_CTL);
> > 
> > which sets the "AM65_CPSW_CTL_VLAN_AWARE" bit by default.
> > 
> > Could it be changed to:
> > 
> > #define TI_K3_CPSW_VLAN_AWARE 1
> > 
> > ....
> > 
> > 	/* Control register */
> > 	val = AM65_CPSW_CTL_P0_ENABLE | AM65_CPSW_CTL_P0_TX_CRC_REMOVE |
> > 	      AM65_CPSW_CTL_P0_RX_PAD;
> > 
> > #ifdef TI_K3_CPSW_VLAN_AWARE
> > 	val |= AM65_CPSW_CTL_VLAN_AWARE;
> > #endif
> > 
> > 	writel(val, common->cpsw_base + AM65_CPSW_REG_CTL);
> > 
> > Since no additional configuration is necessary to disable/enable the
> > functionality except clearing/setting a bit in a register, I am unsure of
> > the implementation for the offloading part being suggested. Please let me
> > know if the above implementation is an acceptable alternative.
> 
> This doesn't really solve the problem as it leaves the question open as to
> who will set TI_K3_CPSW_VLAN_AWARE. And the configuration is then fixed at build.

I have set it above in my proposed solution:
#define TI_K3_CPSW_VLAN_AWARE 1
to match the existing driver implementation where it is already set by
default. Yes, the configuration is fixed at build since the implementation
in this patch which allows toggling it at runtime doesn't appear to be
acceptable, despite being quite similar to how the "Round Robin" and
"Fixed" Priority modes can be toggled using the "p0-rx-ptype-rrobin"
ethtool priv-flag.

> 
> Can you please explain in which scenario the default case does not work for you?
> Why would end user want to disable VLAN_AWARE mode?
> 
> TRM states
> "Transmit packets are NOT modified during switch egress when the VLAN_AWARE bit in the
> CPSW_CONTROL_REG register is cleared to 0h. This means that the switch is not in VLAN-aware mode."
> 
> The same problem would also apply to cpsw.c and cpsw_new.c correct?
> 
> A bit later the driver does this
> 
>         /* switch to vlan unaware mode */
>         cpsw_ale_control_set(common->ale, HOST_PORT_NUM, ALE_VLAN_AWARE, 1);
>         cpsw_ale_control_set(common->ale, HOST_PORT_NUM,
>                              ALE_PORT_STATE, ALE_PORT_STATE_FORWARD);
> 
> The comment says vlan unaware but code is setting ALE_VLAN_AWARE to 1.
> Is the comment wrong?

I have mentioned the following in my commit message to avoid confusion:
"The CPSW Ethernet Switch on TI's K3 SoCs can be configured to operate in
VLAN Aware or VLAN Unaware modes of operation. This is different from
the ALE being VLAN Aware and Unaware."

with emphasis being on "This is different from the ALE being VLAN Aware and
Unaware."

ALE_VLAN_AWARE belongs to the "CPSW_ALE_CONTROL" register and is defined
as:
ALE_VLAN_AWARE determines how traffic is forwarded using VLAN rules.
0h = Simple switch rules, packets forwarded to all ports for unknown
destinations.
1h = VLAN Aware rules, packets forwarded based on VLAN members.

On the other hand, AM65_CPSW_CTL_VLAN_AWARE belongs to the "CPSW_CONTROL"
register and is defined as:
VLAN Aware Mode:
0h = CPSW_NU is in the VLAN unaware mode.
1h = CPSW_NU is in the VLAN aware mode.

They are completely different and offer entirely different
functionality. CPSW_VLAN_AWARE corresponding to the
AM65_CPSW_CTL_VLAN_AWARE bit enables further packet processing:
VLAN tag addition/removal/replacement
which is a layer on top of the Software generated packets Egressing out
of the Ethernet Switch ports, or Forwarded packets Egressing out of the
Ethernet Switch ports. If the aforementioned modification is handled in
Software for example and we don't want packets from Software or on the
Forwarding path to be modified, then turning off the CPSW_VLAN_AWARE
mode is necessary.

Regards,
Siddharth.

Re: [PATCH net-next] net: ethernet: ti: am65-cpsw: Add priv-flag for Switch VLAN Aware mode

[Andrew Lunn]

On Thu, Feb 29, 2024 at 04:37:06PM +0530, Siddharth Vadapalli wrote:
> On Thu, Feb 29, 2024 at 12:52:07PM +0200, Roger Quadros wrote:
> > 
> > 
> > On 29/02/2024 11:27, Siddharth Vadapalli wrote:
> > > On Wed, Feb 28, 2024 at 02:36:55PM +0100, Andrew Lunn wrote:
> > >>> What if there is no kernel behavior associated with it? How can it be mimicked
> > >>> then?
> > >>
> > >> Simple. Implement the feature in software in the kernel for
> > >> everybody. Then offload it to your hardware.
> > >>
> > >> Your hardware is an accelerator. You use it to accelerate what linux
> > >> can already do. If Linux does not have the feature your accelerator
> > >> has, that accelerator feature goes unused.
> > > 
> > > Is it acceptable to have a macro in the Ethernet Driver to conditionally
> > > disable/enable the feature (via setting the corresponding bit in the
> > > register)?

Please re-read my sentence above.

Your hardware is an accelerate for what Linux already does. That is
the key point here. Please understand that. Once you understand that
point, everything else becomes simple.

Does linux have this feature? If yes, accelerate it. If no, you have
something in your hardware which is currently not usable.

	Andrew

Re: [PATCH net-next] net: ethernet: ti: am65-cpsw: Add priv-flag for Switch VLAN Aware mode

[Sverdlin, Alexander]

Hello Siddharth,

On Thu, 2024-02-29 at 16:37 +0530, Siddharth Vadapalli wrote:
> > A bit later the driver does this
> > 
> >          /* switch to vlan unaware mode */
> >          cpsw_ale_control_set(common->ale, HOST_PORT_NUM, ALE_VLAN_AWARE, 1);
> >          cpsw_ale_control_set(common->ale, HOST_PORT_NUM,
> >                               ALE_PORT_STATE, ALE_PORT_STATE_FORWARD);
> > 
> > The comment says vlan unaware but code is setting ALE_VLAN_AWARE to 1.
> > Is the comment wrong?

I was asking myself the same question looking into the 3 drivers.

> ALE_VLAN_AWARE belongs to the "CPSW_ALE_CONTROL" register and is defined
> as:
> ALE_VLAN_AWARE determines how traffic is forwarded using VLAN rules.
> 0h = Simple switch rules, packets forwarded to all ports for unknown
> destinations.
> 1h = VLAN Aware rules, packets forwarded based on VLAN members.

So is our understanding correct? Shall I send a patch inverting the comment
in question?

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH net-next] net: ethernet: ti: am65-cpsw: Add priv-flag for Switch VLAN Aware mode

[Sverdlin, Alexander]

Hello Siddharth,

On Thu, 2024-02-29 at 16:37 +0530, Siddharth Vadapalli wrote:
> > > The current implementation is:
> > > 
> > >  	/* Control register */
> > >  	writel(AM65_CPSW_CTL_P0_ENABLE | AM65_CPSW_CTL_P0_TX_CRC_REMOVE |
> > >  	       AM65_CPSW_CTL_VLAN_AWARE | AM65_CPSW_CTL_P0_RX_PAD,
> > >  	       common->cpsw_base + AM65_CPSW_REG_CTL);
> > > 
> > > which sets the "AM65_CPSW_CTL_VLAN_AWARE" bit by default.

...

> functionality. CPSW_VLAN_AWARE corresponding to the
> AM65_CPSW_CTL_VLAN_AWARE bit enables further packet processing:
> VLAN tag addition/removal/replacement
> which is a layer on top of the Software generated packets Egressing out
> of the Ethernet Switch ports, or Forwarded packets Egressing out of the
> Ethernet Switch ports. If the aforementioned modification is handled in
> Software for example and we don't want packets from Software or on the
> Forwarding path to be modified, then turning off the CPSW_VLAN_AWARE
> mode is necessary.

I think the question many had in mind and which is I believe still
remains unclear, what exactly AM65_CPSW_CTL_VLAN_AWARE asserted by
default is currently used for?

Is it NETIF_F_HW_VLAN_CTAG_FILTER or in other words

static const struct net_device_ops am65_cpsw_nuss_netdev_ops = {
...
	.ndo_vlan_rx_add_vid	= am65_cpsw_nuss_ndo_slave_add_vid,
	.ndo_vlan_rx_kill_vid	= am65_cpsw_nuss_ndo_slave_kill_vid,
?

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com