Re: [PATCH net-next v2 4/7] net: ti: icssg-prueth: Add support for HSR frame forward offload

[MD Danish Anwar <danishanwar@ti.com>]

On 13/08/24 8:47 pm, Andrew Lunn wrote:
> On Tue, Aug 13, 2024 at 01:12:30PM +0530, MD Danish Anwar wrote:
>> Add support for offloading HSR port-to-port frame forward to hardware.
>> When the slave interfaces are added to the HSR interface, the PRU cores
>> will be stopped and ICSSG HSR firmwares will be loaded to them.
>>
>> Similarly, when HSR interface is deleted, the PRU cores will be stopped
>> and dual EMAC firmware will be loaded to them.
> 
> Maybe a dumb question, because i don't know HSR....
> 
> Can you have one interface in a HSR network, another interface in a
> non-HSR network, and bridge packets between the two worlds? Do you
> want the HSR firmware, the Switchdev firmware, or Dual EMAC and do the
> bridge in software?
> 

As far as I know, when adding an hsr interface we need to specify both
the slave interfaces

` ip link add name hsr0 type hsr slave1 eth1 slave2 eth2 supervision 45
version 1`

HSR is only enabled when both the ports are added to hsr interface. If
hsr-fwd-offload is set, the firmware will be changed to HSR otherwise
Dual EMAC firmware will keep running and hsr forwarding will happen in
software.

>>  void icssg_class_set_mac_addr(struct regmap *miig_rt, int slice, u8 *mac)
>>  {
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
>> index dae52a83a378..2f485318c940 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
>> @@ -455,7 +455,7 @@ int icssg_config(struct prueth *prueth, struct prueth_emac *emac, int slice)
>>  	struct icssg_flow_cfg __iomem *flow_cfg;
>>  	int ret;
>>  
>> -	if (prueth->is_switch_mode)
>> +	if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
>>  		icssg_init_switch_mode(prueth);
> 
> Maybe icssg_init_switch_mode() needs renaming if it is used for more
> than switch mode? There are other functions which might need
> generalising.
> 

Yes, the icssg_init_ and many other APIs are common for switch and hsr.
They can be renamed to indicate that as well.

How does icssg_init_switch_or_hsr_mode() sound?

>> +#define NETIF_PRUETH_HSR_OFFLOAD	NETIF_F_HW_HSR_FWD
>> +
>>  /* CTRLMMR_ICSSG_RGMII_CTRL register bits */
>>  #define ICSSG_CTRL_RGMII_ID_MODE                BIT(24)
>>  
>> @@ -118,6 +121,19 @@ static irqreturn_t prueth_tx_ts_irq(int irq, void *dev_id)
>>  	return IRQ_HANDLED;
>>  }
>>  
>> +static struct icssg_firmwares icssg_hsr_firmwares[] = {
>> +	{
>> +		.pru = "ti-pruss/am65x-sr2-pru0-pruhsr-fw.elf",
>> +		.rtu = "ti-pruss/am65x-sr2-rtu0-pruhsr-fw.elf",
>> +		.txpru = "ti-pruss/am65x-sr2-txpru0-pruhsr-fw.elf",
>> +	},
>> +	{
>> +		.pru = "ti-pruss/am65x-sr2-pru1-pruhsr-fw.elf",
>> +		.rtu = "ti-pruss/am65x-sr2-rtu1-pruhsr-fw.elf",
>> +		.txpru = "ti-pruss/am65x-sr2-txpru1-pruhsr-fw.elf",
>> +	}
>> +};
>> +
>>  static struct icssg_firmwares icssg_switch_firmwares[] = {
>>  	{
>>  		.pru = "ti-pruss/am65x-sr2-pru0-prusw-fw.elf",
>> @@ -152,6 +168,8 @@ static int prueth_emac_start(struct prueth *prueth, struct prueth_emac *emac)
>>  
>>  	if (prueth->is_switch_mode)
>>  		firmwares = icssg_switch_firmwares;
>> +	else if (prueth->is_hsr_offload_mode)
>> +		firmwares = icssg_hsr_firmwares;
> 
> Documentation/networking/netdev-features.rst
> 
> * hsr-fwd-offload
> 
> This should be set for devices which forward HSR (High-availability Seamless
> Redundancy) frames from one port to another in hardware.
> 
> To me, this suggests if the flag is not set, you should keep in dual
> EMACS or switchdev mode and perform HSR in software.


Correct. This is the expected behavior. If the flag is not set we remain
in dual EMAC firmware and do HSR in software. Please see
prueth_hsr_port_link() for detail on this.

> 
>> +static int emac_ndo_set_features(struct net_device *ndev,
>> +				 netdev_features_t features)
>> +{
>> +	netdev_features_t hsr_feature_present = ndev->features & NETIF_PRUETH_HSR_OFFLOAD;
>> +	netdev_features_t hsr_feature_wanted = features & NETIF_PRUETH_HSR_OFFLOAD;
> 
> I would not add the _PRUETH_ alias. There is nothing _PRUETH_ specific
> here, its just plain HSR offload.
> 

I see your query in this is resolved by
https://lore.kernel.org/all/985e10e4-49df-46d8-b9c2-d385dab569a9@lunn.ch/

>> +static int prueth_hsr_port_link(struct net_device *ndev)
>> +{
>> +	struct prueth_emac *emac = netdev_priv(ndev);
>> +	struct prueth *prueth = emac->prueth;
>> +	struct prueth_emac *emac0;
>> +	struct prueth_emac *emac1;
>> +
>> +	emac0 = prueth->emac[PRUETH_MAC0];
>> +	emac1 = prueth->emac[PRUETH_MAC1];
>> +
>> +	if (prueth->is_switch_mode) {
>> +		dev_err(prueth->dev, "Switching from bridge to HSR mode not allowed\n");
>> +		return -EINVAL;
> 
> I think you want EOPNOTSUPP, so that it is performed in software, not
> offloaded to hardware. And this is not an error condition, it is just
> a limitation of your hardware/firmware.
> 

Sure.

>> +	prueth->hsr_members |= BIT(emac->port_id);
>> +	if (!prueth->is_switch_mode && !prueth->is_hsr_offload_mode) {
>> +		if (prueth->hsr_members & BIT(PRUETH_PORT_MII0) &&
>> +		    prueth->hsr_members & BIT(PRUETH_PORT_MII1)) {
>> +			if (!(emac0->ndev->features & NETIF_PRUETH_HSR_OFFLOAD) &&
>> +			    !(emac1->ndev->features & NETIF_PRUETH_HSR_OFFLOAD)) {
>> +				dev_err(prueth->dev, "Enable HSR offload on both interfaces\n");
>> +				return -EINVAL;
> 
> Again, EOPNOTSUPP, so it falls back to software, and no dev_err().

sure I will change this to EOPNOTSUPP and remove the print.

> 
>> +			}
>> +			prueth->is_hsr_offload_mode = true;
>> +			prueth->default_vlan = 1;
>> +			emac0->port_vlan = prueth->default_vlan;
>> +			emac1->port_vlan = prueth->default_vlan;
>> +			icssg_change_mode(prueth);
>> +			dev_err(prueth->dev, "Enabling HSR offload mode\n");
> 
> This is not an error condition. dev_dbg().

Sure.

> 
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void prueth_hsr_port_unlink(struct net_device *ndev)
>> +{
>> +	struct prueth_emac *emac = netdev_priv(ndev);
>> +	struct prueth *prueth = emac->prueth;
>> +	struct prueth_emac *emac0;
>> +	struct prueth_emac *emac1;
>> +
>> +	emac0 = prueth->emac[PRUETH_MAC0];
>> +	emac1 = prueth->emac[PRUETH_MAC1];
>> +
>> +	prueth->hsr_members &= ~BIT(emac->port_id);
>> +	if (prueth->is_hsr_offload_mode) {
>> +		prueth->is_hsr_offload_mode = false;
>> +		emac0->port_vlan = 0;
>> +		emac1->port_vlan = 0;
>> +		prueth->hsr_dev = NULL;
>> +		prueth_emac_restart(prueth);
>> +		dev_info(prueth->dev, "Enabling Dual EMAC mode\n");
> 
> dev_dbg().

Sure.

> 
>> +	}
>> +}
>> +
>>  /* netdev notifier */
>>  static int prueth_netdevice_event(struct notifier_block *unused,
>>  				  unsigned long event, void *ptr)
>> @@ -1047,6 +1141,8 @@ static int prueth_netdevice_event(struct notifier_block *unused,
>>  	struct netlink_ext_ack *extack = netdev_notifier_info_to_extack(ptr);
>>  	struct net_device *ndev = netdev_notifier_info_to_dev(ptr);
>>  	struct netdev_notifier_changeupper_info *info;
>> +	struct prueth_emac *emac = netdev_priv(ndev);
>> +	struct prueth *prueth = emac->prueth;
>>  	int ret = NOTIFY_DONE;
>>  
>>  	if (ndev->netdev_ops != &emac_netdev_ops)
>> @@ -1056,6 +1152,26 @@ static int prueth_netdevice_event(struct notifier_block *unused,
>>  	case NETDEV_CHANGEUPPER:
>>  		info = ptr;
>>  
>> +		if ((ndev->features & NETIF_PRUETH_HSR_OFFLOAD) &&
>> +		    is_hsr_master(info->upper_dev)) {
>> +			if (info->linking) {
>> +				if (!prueth->hsr_dev) {
>> +					prueth->hsr_dev = info->upper_dev;
>> +
>> +					icssg_class_set_host_mac_addr(prueth->miig_rt,
>> +								      prueth->hsr_dev->dev_addr);
>> +				} else {
>> +					if (prueth->hsr_dev != info->upper_dev) {
>> +						dev_err(prueth->dev, "Both interfaces must be linked to same upper device\n");
> 
> dev_dbg()

Sure. I will do these changes and send out a new version. Please let me
know if any other change is needed.

> 
> 	Andrew

-- 
Thanks and Regards,
Danish