Message-ID: <57d78d3b-dadf-488f-87dc-08a07759fb3e@ti.com>
Date: Wed, 17 Apr 2024 10:45:02 +0530
From: MD Danish Anwar <danishanwar@...com>
To: Andrew Davis <afd@...com>, "Anwar, Md Danish" <a0501179@...com>,
        Diogo Ivo
	<diogo.ivo@...mens.com>, Rob Herring <robh@...nel.org>,
        Dan Carpenter
	<dan.carpenter@...aro.org>,
        Jan Kiszka <jan.kiszka@...mens.com>, Simon Horman
	<horms@...nel.org>,
        Andrew Lunn <andrew@...n.ch>,
        Wolfram Sang
	<wsa+renesas@...g-engineering.com>,
        Arnd Bergmann <arnd@...db.de>, Vignesh
 Raghavendra <vigneshr@...com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Roger Quadros <rogerq@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
        Jakub
 Kicinski <kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>,
        "David S.
 Miller" <davem@...emloft.net>
CC: <linux-arm-kernel@...ts.infradead.org>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <srk@...com>, <r-gunasekaran@...com>
Subject: Re: [PATCH net-next v3 3/3] net: ti: icssg-prueth: Add support for
 ICSSG switch firmware



On 16/04/24 10:49 pm, Andrew Davis wrote:
> 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.
> 

This was discussed during v1 of this series [1]. Andrew Lunn also
commented asking for a single firmware that can do both. But the problem
is having a single firmware to do both Switch and EMAC operation will
require additional check in firmware to check whether we are in switch
or mac mode based on that firmware will forward the packet to the other
port or host. I had talked to firmware team regarding this, this
additional check will result in consuming alot of extra cycles and the
performance will be degraded substantially. That is why we decided to
stick with two different firmwares as combining them in one has a very
big penalty.

Such firmware doesn't exist as of now. Furthermore, we are also working
on hsr mode for ICSSG driver and this will also introduce new hsr
firmware. In future their could be more different modes. Now combining
all this firmwares into one will consume all the budget cycles and that
is why firmware team has decided to have split firmware.

While switching modes, stopping and restarting the PRU cores after
changing firmware works fine and it's completely abstracted from user.
For a user it's same as a software bridge. There will be some packet
loss but as Andrew Lunn pointed out, even in sofware bridging there are
some packets losses.

> 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[] = {
>>

[1] https://lore.kernel.org/all/92864bda-3028-f8be-0e27-487024d1a874@ti.com/

-- 
Thanks and Regards,
Danish

Powered by blists - more mailing lists