Received-SPF: pass (google.com: domain of linux-kernel+bounces-147297-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <f4a91360-bf31-4dd0-a00d-cd4b7464160e@ti.com>
Date: Tue, 16 Apr 2024 12:19:57 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next v3 3/3] net: ti: icssg-prueth: Add support for
 ICSSG switch firmware
To: "Anwar, Md Danish" <a0501179@ti.com>,
        MD Danish Anwar
	<danishanwar@ti.com>,
        Diogo Ivo <diogo.ivo@siemens.com>, Rob Herring
	<robh@kernel.org>,
        Dan Carpenter <dan.carpenter@linaro.org>,
        Jan Kiszka
	<jan.kiszka@siemens.com>, Simon Horman <horms@kernel.org>,
        Andrew Lunn
	<andrew@lunn.ch>,
        Wolfram Sang <wsa+renesas@sang-engineering.com>,
        Arnd
 Bergmann <arnd@arndb.de>, Vignesh Raghavendra <vigneshr@ti.com>,
        Vladimir
 Oltean <vladimir.oltean@nxp.com>,
        Roger Quadros <rogerq@kernel.org>, Paolo
 Abeni <pabeni@redhat.com>,
        Jakub Kicinski <kuba@kernel.org>, Eric Dumazet
	<edumazet@google.com>,
        "David S. Miller" <davem@davemloft.net>
CC: <linux-arm-kernel@lists.infradead.org>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <srk@ti.com>, <r-gunasekaran@ti.com>
References: <20240327114054.1907278-1-danishanwar@ti.com>
 <20240327114054.1907278-4-danishanwar@ti.com>
 <cb13da4a-13c9-409a-a813-0ac852062163@ti.com>
 <bc0e05c5-11a8-4519-b50d-04dabd6e5999@ti.com>
Content-Language: en-US
From: Andrew Davis <afd@ti.com>
In-Reply-To: <bc0e05c5-11a8-4519-b50d-04dabd6e5999@ti.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit

On 4/15/24 1:56 AM, Anwar, Md Danish wrote:
> Hi Andrew,
> 
> On 4/13/2024 12:22 AM, Andrew Davis wrote:
>> On 3/27/24 6:40 AM, MD Danish Anwar wrote:
>>> Add support for ICSSG switch firmware using existing Dual EMAC driver
>>> with switchdev.
>>>
>>> Limitations:
>>> VLAN offloading is limited to 0-256 IDs.
>>> MDB/FDB static entries are limited to 511 entries and different FDBs can
>>> hash to same bucket and thus may not completely offloaded
>>>
>>> Switch mode requires loading of new firmware into ICSSG cores. This
>>> means interfaces have to taken down and then reconfigured to switch
>>> mode.
>>>
>>> Example assuming ETH1 and ETH2 as ICSSG2 interfaces:
>>>
>>> Switch to ICSSG Switch mode:
>>>    ip link set dev eth1 down
>>>    ip link set dev eth2 down
>>>    ip link add name br0 type bridge
>>>    ip link set dev eth1 master br0
>>>    ip link set dev eth2 master br0
>>>    ip link set dev br0 up
>>>    ip link set dev eth1 up
>>>    ip link set dev eth2 up
>>>    bridge vlan add dev br0 vid 1 pvid untagged self
>>>
>>> Going back to Dual EMAC mode:
>>>
>>>    ip link set dev br0 down
>>>    ip link set dev eth1 nomaster
>>>    ip link set dev eth2 nomaster
>>>    ip link set dev eth1 down
>>>    ip link set dev eth2 down
>>>    ip link del name br0 type bridge
>>>    ip link set dev eth1 up
>>>    ip link set dev eth2 up
>>>
>>> By default, Dual EMAC firmware is loaded, and can be changed to switch
>>> mode by above steps
>>>
>>
>> This was asked before, maybe I missed the answer, but why do we
>> default to Dual-EMAC firmware? I remember when I was working on
>> the original ICSS-ETH driver, we started with the Dual-EMAC
>> firmware as the switch firmware was not ready yet (and EMAC mode
>> was easier). Now that we have both available, if we just use Switch
>> firmwar by default, what would we lose? Seems that would solve
>> the issues with re-loading firmware at runtime (configuration loss
>> and dropping packets, etc..).
>>
> 
> We can start the driver with either Dual-EMAC firmware or SWITCH
> firmware. But the problem lies in switching between these two firmwares.
> For switching to / from Dual-EMAC and switch firmwares we need to stop
> the cores and that is where we previously used to bring down the
> interfaces, switch firmware and bring it up again. But as discussed on
> this thread, I can now do the same without bringing interfaces up /
> down. We'll just need to stop the cores and change firmware this will
> also result in preserving the configuration. There will be packet loss
> but that will not be a big concern as Andrew L. pointed out.
> 

Yes I saw that and understand all this, but that is not my question.

> Currently we are starting in Dual-EMAC mode as by default the interfaces
> are not needed to forward packets. They are supposed to act as
> individual ports. Port to port forwarding is not needed. Only when user
> adds a bridge and starts forwarding we switch to Switch mode and load
> different firmware so that packet forwarding can happen in firmware.
> That is why currently we are starting Dual-EMAC mode and then switching
> to firmware.
> 

Same, I see what we do here, this doesn't give me the "why".

> If we use switch firmware by default, we will not be able to use
> individual ports separately and any data sent to one port will be
> forwarded to the second port.

So this seems to almost answer the question, but I still do not see
why we could not use the ports separately when using SWITCH firmware.

Why not have a filter/rule set by default to each port so that they
do not forward packets automatically but instead always forward
to the host port? That would result in the same functionality as
the Dual-EMAC firmware, but without all the mess of runtime firmware
switching based on usecase (simply update the forwarding rules when
in bridge mode).

Andrew

> 
> I will be posting v4 soon and I will describe all the details on how to
> use and switch between different modes in the cover letter.
> 
>> Andrew
>>
> 
> [ ... ]
> 
>>>      static const struct prueth_pdata am64x_icssg_pdata = {
>>>        .fdqring_mode = K3_RINGACC_RING_MODE_RING,
>>> +    .switch_mode = 1,
>>>    };
>>>      static const struct of_device_id prueth_dt_match[] = {
>