Received-SPF: pass (google.com: domain of linux-kernel+bounces-86577-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <0106ce78-c83f-4552-a234-1bf7a33f1ed1@kernel.org>
Date: Thu, 29 Feb 2024 12:52:07 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next] net: ethernet: ti: am65-cpsw: Add priv-flag for
 Switch VLAN Aware mode
Content-Language: en-US
To: Siddharth Vadapalli <s-vadapalli@ti.com>, Andrew Lunn <andrew@lunn.ch>
Cc: Jiri Pirko <jiri@resnulli.us>, davem@davemloft.net, edumazet@google.com,
 kuba@kernel.org, pabeni@redhat.com, vladimir.oltean@nxp.com,
 hkallweit1@gmail.com, dan.carpenter@linaro.org, horms@kernel.org,
 yuehaibing@huawei.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org, srk@ti.com,
 Pekka Varis <p-varis@ti.com>
References: <20240227082815.2073826-1-s-vadapalli@ti.com>
 <Zd3YHQRMnv-ZvSWs@nanopsycho> <7d1496da-100a-4336-b744-33e843eba930@ti.com>
 <Zd7taFB2nEvtZh8E@nanopsycho> <49e531f7-9465-40ea-b604-22a3a7f13d62@ti.com>
 <10287788-614a-4eef-9c9c-a0ef4039b78f@lunn.ch>
 <0004e3d5-0f62-49dc-b51f-5a302006c303@ti.com>
From: Roger Quadros <rogerq@kernel.org>
In-Reply-To: <0004e3d5-0f62-49dc-b51f-5a302006c303@ti.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


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