Message-ID: <d60f87d2-bfaf-40dc-97ed-481ad40aec60@ti.com>
Date: Mon, 22 Jan 2024 16:05:44 +0530
From: MD Danish Anwar <danishanwar@...com>
To: Andrew Lunn <andrew@...n.ch>
CC: Rob Herring <robh@...nel.org>, Dan Carpenter <dan.carpenter@...aro.org>,
        Jan Kiszka <jan.kiszka@...mens.com>,
        Vladimir Oltean
	<vladimir.oltean@....com>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        Arnd Bergmann <arnd@...db.de>,
        Grygorii Strashko <grygorii.strashko@...com>,
        Vignesh Raghavendra <vigneshr@...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>,
        <linux-arm-kernel@...ts.infradead.org>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <srk@...com>, <r-gunasekaran@...com>
Subject: Re: [RFC PATCH v2 3/3] net: ti: icssg-prueth: Add support for ICSSG
 switch firmware

Hi Andrew,

On 19/01/24 7:59 pm, Andrew Lunn wrote:
> On Thu, Jan 18, 2024 at 12:40:05PM +0530, 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
> 
> What are the limits when using Dual EMAC driver? I'm just wondering if
> we need to check that 257 VLANs have been offloaded, we cannot swap to
> switch mode, keep with Dual EMAC?
> 

Both Switch and dual EMAC has the same limit. Maximum 256 VIDs can ebe
offloaded in both dual EMAC and switch mode. When VID is greater than
256, we don't add the vid and return 0. You can see
prueth_switchdev_vlans_add() for details on how vlans are added.

>> Switch mode requires loading of new firmware into ICSSG cores. This
>> means interfaces have to taken down and then reconfigured to switch
>> mode.
> 
> This is now out of date?
> 
>>
>> 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
>>
>> Signed-off-by: MD Danish Anwar <danishanwar@...com>
>> ---
>>  drivers/net/ethernet/ti/Kconfig               |   1 +
>>  drivers/net/ethernet/ti/Makefile              |   3 +-
>>  drivers/net/ethernet/ti/icssg/icssg_config.c  | 136 +++++++++++-
>>  drivers/net/ethernet/ti/icssg/icssg_config.h  |   7 +
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.c  | 198 +++++++++++++++++-
>>  .../net/ethernet/ti/icssg/icssg_switchdev.c   |   2 +-
>>  6 files changed, 333 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/Kconfig b/drivers/net/ethernet/ti/Kconfig
>> index be01450c20dc..c72f26828b04 100644
>> --- a/drivers/net/ethernet/ti/Kconfig
>> +++ b/drivers/net/ethernet/ti/Kconfig
>> @@ -188,6 +188,7 @@ config TI_ICSSG_PRUETH
>>  	select TI_ICSS_IEP
>>  	select TI_K3_CPPI_DESC_POOL
>>  	depends on PRU_REMOTEPROC
>> +	depends on NET_SWITCHDEV
>>  	depends on ARCH_K3 && OF && TI_K3_UDMA_GLUE_LAYER
>>  	help
>>  	  Support dual Gigabit Ethernet ports over the ICSSG PRU Subsystem.
>> diff --git a/drivers/net/ethernet/ti/Makefile b/drivers/net/ethernet/ti/Makefile
>> index d8590304f3df..d295bded7a32 100644
>> --- a/drivers/net/ethernet/ti/Makefile
>> +++ b/drivers/net/ethernet/ti/Makefile
>> @@ -38,5 +38,6 @@ icssg-prueth-y := icssg/icssg_prueth.o \
>>  		  icssg/icssg_config.o \
>>  		  icssg/icssg_mii_cfg.o \
>>  		  icssg/icssg_stats.o \
>> -		  icssg/icssg_ethtool.o
>> +		  icssg/icssg_ethtool.o \
>> +		  icssg/icssg_switchdev.o
>>  obj-$(CONFIG_TI_ICSS_IEP) += icssg/icss_iep.o
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
>> index afc10014ec03..eda08a87c902 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
>> @@ -105,28 +105,49 @@ static const struct map hwq_map[2][ICSSG_NUM_OTHER_QUEUES] = {
>>  	},
>>  };
>>  
>> +static void icssg_config_mii_init_switch(struct prueth_emac *emac)
> 
> I'm surprised you need to configure the MII interface different in
> switch mode. Please could you explain this a bit more.>

Sure, I'll explain.

TX_MUX_SEL0 (BIT(8)) of TXCFG register indicated weather the port is
connected to txpru0 or txpru1

0h = TX data from PRU0 is selected
1h = TX data from PRU1 is selected

Refer to section 6.5.14.11.3 of TRM [1].

In dual EMAC mode, for port0 the connected PRU cores are pru0, rtu0 and
txpru0 similarly for port1 the connected PRU cores are pru1, rtu1 and
txpru1. Port0 and port1 can not communicate among each other as they
don't share any PRU cores. So BIT(8) for port0 is 0h (meaning TX data
from PRU0 is selected) and BIT(8) for port1 is 1h (meaning TX data from
PRU1 is selected)

In switch mode, for port0 the connected PRU cores are pru0, rtu0 and
*txpru1* similarly for port1 the connected PRU cores are pru1, rtu1 and
*txpru0*.

In switch mode, port0 is connected to txpru1 and port1 is connected to
txpru0. This enables the firmware to do the forwarding between the
ports. Now to enable this configuration BIT(8) needs to be set
differently in switch mode. BIT(8) for port0 is 1h (meaning TX data from
PRU1 is selected) and BIT(8) for port1 is 0h (meaning TX data from PRU0
is selected). This enables the forwarding between ports.

So MII interface needs to be configured differently for MAC and switch
mode. The only difference being the BIT(8)


>> +{
>> +	struct prueth *prueth = emac->prueth;
>> +	int mii = prueth_emac_slice(emac);
>> +	u32 txcfg_reg, pcnt_reg, txcfg;
>> +	struct regmap *mii_rt;
>> +
>> +	mii_rt = prueth->mii_rt;
>> +
>> +	txcfg_reg = (mii == ICSS_MII0) ? PRUSS_MII_RT_TXCFG0 :
>> +				       PRUSS_MII_RT_TXCFG1;
>> +	pcnt_reg = (mii == ICSS_MII0) ? PRUSS_MII_RT_RX_PCNT0 :
>> +				       PRUSS_MII_RT_RX_PCNT1;
>> +
>> +	txcfg = PRUSS_MII_RT_TXCFG_TX_ENABLE |
>> +		PRUSS_MII_RT_TXCFG_TX_AUTO_PREAMBLE |
>> +		PRUSS_MII_RT_TXCFG_TX_IPG_WIRE_CLK_EN;
>> +
>> +	if (emac->phy_if == PHY_INTERFACE_MODE_MII && mii == ICSS_MII1)
>> +		txcfg |= PRUSS_MII_RT_TXCFG_TX_MUX_SEL;
>> +	else if (emac->phy_if != PHY_INTERFACE_MODE_MII && mii == ICSS_MII0)
>> +		txcfg |= PRUSS_MII_RT_TXCFG_TX_MUX_SEL;
>> +
>> +	regmap_write(mii_rt, txcfg_reg, txcfg);
>> +	regmap_write(mii_rt, pcnt_reg, 0x1);
>> +}
>> +
>>  static void icssg_config_mii_init(struct prueth_emac *emac)
>>  {
>> -	u32 rxcfg, txcfg, rxcfg_reg, txcfg_reg, pcnt_reg;
>>  	struct prueth *prueth = emac->prueth;
>>  	int slice = prueth_emac_slice(emac);
>> +	u32 txcfg, txcfg_reg, pcnt_reg;
>>  	struct regmap *mii_rt;
>>  
>>  	mii_rt = prueth->mii_rt;
>>  
>> -	rxcfg_reg = (slice == ICSS_MII0) ? PRUSS_MII_RT_RXCFG0 :
>> -				       PRUSS_MII_RT_RXCFG1;
>>  	txcfg_reg = (slice == ICSS_MII0) ? PRUSS_MII_RT_TXCFG0 :
>>  				       PRUSS_MII_RT_TXCFG1;
>>  	pcnt_reg = (slice == ICSS_MII0) ? PRUSS_MII_RT_RX_PCNT0 :
>>  				       PRUSS_MII_RT_RX_PCNT1;
>>  
>> -	rxcfg = MII_RXCFG_DEFAULT;
>>  	txcfg = MII_TXCFG_DEFAULT;
>>  
>> -	if (slice == ICSS_MII1)
>> -		rxcfg |= PRUSS_MII_RT_RXCFG_RX_MUX_SEL;
>> -
>>  	/* In MII mode TX lines swapped inside ICSSG, so TX_MUX_SEL cfg need
>>  	 * to be swapped also comparing to RGMII mode.
>>  	 */
>> @@ -135,7 +156,6 @@ static void icssg_config_mii_init(struct prueth_emac *emac)
>>  	else if (emac->phy_if != PHY_INTERFACE_MODE_MII && slice == ICSS_MII1)
>>  		txcfg |= PRUSS_MII_RT_TXCFG_TX_MUX_SEL;
>>  
>> -	regmap_write(mii_rt, rxcfg_reg, rxcfg);
>>  	regmap_write(mii_rt, txcfg_reg, txcfg);
>>  	regmap_write(mii_rt, pcnt_reg, 0x1);
>>  }
>> @@ -249,6 +269,66 @@ static int emac_r30_is_done(struct prueth_emac *emac)
>>  	return 1;
>>  }
>>  
>> +static int prueth_switch_buffer_setup(struct prueth_emac *emac)
>> +{
>> +	struct icssg_buffer_pool_cfg __iomem *bpool_cfg;
>> +	struct icssg_rxq_ctx __iomem *rxq_ctx;
>> +	struct prueth *prueth = emac->prueth;
>> +	int slice = prueth_emac_slice(emac);
>> +	u32 addr;
>> +	int i;
>> +
>> +	addr = lower_32_bits(prueth->msmcram.pa);
>> +	if (slice)
>> +		addr += PRUETH_NUM_BUF_POOLS * PRUETH_EMAC_BUF_POOL_SIZE;
>> +
>> +	if (addr % SZ_64K) {
>> +		dev_warn(prueth->dev, "buffer pool needs to be 64KB aligned\n");
>> +		return -EINVAL;
>> +	}
> 
> What happens if its not? Do we cleanly stay in Dual EMAC mode without
> any loss of configuration? Or do bad things happen? Maybe this should
> be checked at probe time, so you can deny the swap to switch mode
> quickly and easily?
> 

This is independent of MAC or switch. The MSMC address always needs to
be 64KB aligned. This is a bug in Firmware. If it's not 64KB aligned bad
things might happen, that's why we just stop and return -EINVAL. The
interface will simply not work. The same check is also done in
prueth_emac_buffer_setup().

During probe we make sure that the MSMC is aligned with 64KB. You can
have a look at prueth_probe() [2]. After probe during open() we check
again to see if MSMC is 64KB aligned and then only do the needed
configuration. I can move this check from individual switch / mac APIs
to the beginning of ndo_open() if you want that.

> 	Andrew


[1]
https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf?ts=1705918869984&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FAM6548

[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/ti/icssg/icssg_prueth.c?h=v6.8-rc1#n2079

-- 
Thanks and Regards,
Danish

Powered by blists - more mailing lists