Date:   Mon, 13 Jul 2020 08:30:25 +0200
From:   Kurt Kanzenbach <kurt@...utronix.de>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Richard Cochran <richardcochran@...il.com>,
        Kamil Alkhouri <kamil.alkhouri@...offenburg.de>,
        ilias.apalodimas@...aro.org, Vladimir Oltean <olteanv@...il.com>
Subject: Re: [PATCH v1 2/8] net: dsa: Add DSA driver for Hirschmann Hellcreek switches

On Sat Jul 11 2020, Florian Fainelli wrote:
> On 7/10/2020 4:36 AM, Kurt Kanzenbach wrote:
>> Add a basic DSA driver for Hirschmann Hellcreek switches. Those switches are
>> implementing features needed for Time Sensitive Networking (TSN) such as support
>> for the Time Precision Protocol and various shapers like the Time Aware Shaper.
>> 
>> This driver includes basic support for networking:
>> 
>>   * VLAN handling
>>   * FDB handling
>>   * Port statistics
>>   * STP
>>   * Phylink
>> 
>> Signed-off-by: Kurt Kanzenbach <kurt@...utronix.de>
>> ---
>
> [snip]
>
>> +++ b/drivers/net/dsa/hirschmann/Kconfig
>> @@ -0,0 +1,7 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +config NET_DSA_HIRSCHMANN_HELLCREEK
>> +	tristate "Hirschmann Hellcreek TSN Switch support"
>> +	depends on NET_DSA
>
> You most likely need a depends on HAS_IOMEM since this is a memory
> mapped interface.

OK.

>
> [snip]
>
>> +static void hellcreek_select_port(struct hellcreek *hellcreek, int port)
>> +{
>> +	u16 val = 0;
>> +
>> +	val |= port << HR_PSEL_PTWSEL_SHIFT;
>
> Why not just assign val to port << HR_PSEL_PTWSEL_SHIFT directly?

OK.

>
>> +
>> +	hellcreek_write(hellcreek, val, HR_PSEL);
>> +}
>> +
>> +static void hellcreek_select_prio(struct hellcreek *hellcreek, int prio)
>> +{
>> +	u16 val = 0;
>> +
>> +	val |= prio << HR_PSEL_PRTCWSEL_SHIFT;
>> +
>> +	hellcreek_write(hellcreek, val, HR_PSEL);
>
> Likewise
>
>> +}
>> +
>> +static void hellcreek_select_counter(struct hellcreek *hellcreek, int counter)
>> +{
>> +	u16 val = 0;
>> +
>> +	val |= counter << HR_CSEL_SHIFT;
>> +
>> +	hellcreek_write(hellcreek, val, HR_CSEL);
>> +
>> +	/* Data sheet states to wait at least 20 internal clock cycles */
>> +	ndelay(200);
>
> Likewise.
>
> [snip]
>
>> +static void hellcreek_feature_detect(struct hellcreek *hellcreek)
>> +{
>> +	u16 features;
>> +
>> +	features = hellcreek_read(hellcreek, HR_FEABITS0);
>> +
>> +	/* Currently we only detect the size of the FDB table */
>> +	hellcreek->fdb_entries = ((features & HR_FEABITS0_FDBBINS_MASK) >>
>> +			       HR_FEABITS0_FDBBINS_SHIFT) * 32;
>> +
>> +	dev_info(hellcreek->dev, "Feature detect: FDB entries=%zu\n",
>> +		 hellcreek->fdb_entries);
>
> You may consider reporting this through devlink.
>
>> +}
>> +
>> +static enum dsa_tag_protocol hellcreek_get_tag_protocol(struct dsa_switch *ds,
>> +							int port,
>> +							enum dsa_tag_protocol mp)
>> +{
>> +	return DSA_TAG_PROTO_HELLCREEK;
>> +}
>> +
>> +static int hellcreek_port_enable(struct dsa_switch *ds, int port,
>> +				 struct phy_device *phy)
>> +{
>> +	struct hellcreek *hellcreek = ds->priv;
>> +	struct hellcreek_port *hellcreek_port;
>> +	unsigned long flags;
>> +	u16 val;
>> +
>> +	hellcreek_port = &hellcreek->ports[port];
>> +
>> +	dev_dbg(hellcreek->dev, "Enable port %d\n", port);
>> +
>> +	spin_lock_irqsave(&hellcreek->reg_lock, flags);
>
> Your usage of the spin lock is not entirely clear to me, but it protects
> more than just the register access, usually it has been sprinkled at the
> very beginning of the operations to be performed.
>
> DSA operations should always be RTNL protected and they can sleep. You
> do not appear to have an interrupt handler registered at all, so maybe
> you can replace this by a mutex, or drop the spin lock entirely?

Yes. However, the TAPRIO offloading patch adds hrtimers. Therefore, the
spin lock in the irq variant is needed.

>
> [snip]
>
>> +static int hellcreek_port_bridge_join(struct dsa_switch *ds, int port,
>> +				      struct net_device *br)
>> +{
>> +	struct hellcreek *hellcreek = ds->priv;
>> +	u16 rx_vid = port;
>> +	int i;
>> +
>> +	dev_dbg(hellcreek->dev, "Port %d joins a bridge\n", port);
>> +
>> +	/* Configure port's vid to all other ports as egress untagged */
>> +	for (i = 2; i < NUM_PORTS; ++i) {
>> +		const struct switchdev_obj_port_vlan vlan = {
>> +			.vid_begin = rx_vid,
>> +			.vid_end = rx_vid,
>> +			.flags = BRIDGE_VLAN_INFO_UNTAGGED,
>> +		};
>> +
>> +		if (i == port)
>> +			continue;
>> +
>> +		hellcreek_vlan_add(ds, i, &vlan);
>
> Can you explain that part a little bit more what this VLAN programming
> does and why you need it?

Sure. To reflect the DSA port separation, I created two VLANs containing
each the CPU port and one front port. The bridge setup is now just
extending this to include all ports.

>
> The bridge will send a VLAN programming request with VLAN ID 1 as PVID,
> thus bringing all ports added to the bridge into the same broadcast
> domain.

I see. Maybe that works as well. I'll have to test it. Should the bridge
callbacks be removed then?

>
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void hellcreek_port_bridge_leave(struct dsa_switch *ds, int port,
>> +					struct net_device *br)
>> +{
>> +	struct hellcreek *hellcreek = ds->priv;
>> +	u16 rx_vid = port;
>> +	int i, err;
>> +
>> +	dev_dbg(hellcreek->dev, "Port %d leaves a bridge\n", port);
>> +
>> +	/* Remove port's vid from all other ports */
>> +	for (i = 2; i < NUM_PORTS; ++i) {
>> +		const struct switchdev_obj_port_vlan vlan = {
>> +			.vid_begin = rx_vid,
>> +			.vid_end = rx_vid,
>> +		};
>> +
>> +		if (i == port)
>> +			continue;
>> +
>> +		err = hellcreek_vlan_del(ds, i, &vlan);
>> +		if (err) {
>> +			dev_err(hellcreek->dev, "Failed add vid %d to port %d\n",
>> +				rx_vid, i);
>> +			return;
>> +		}
>> +	}
>> +}
>> +
>> +static int __hellcreek_fdb_add(struct hellcreek *hellcreek,
>> +			       const struct hellcreek_fdb_entry *entry)
>> +{
>> +	int ret;
>> +	u16 meta = 0;
>> +
>> +	dev_dbg(hellcreek->dev, "Add static FDB entry: MAC=%pM, MASK=0x%02x, "
>> +		"OBT=%d, REPRIO_EN=%d, PRIO=%d\n", entry->mac, entry->portmask,
>> +		entry->is_obt, entry->reprio_en, entry->reprio_tc);
>> +
>> +	/* Add mac address */
>> +	hellcreek_write(hellcreek, entry->mac[1] | (entry->mac[0] << 8), HR_FDBWDH);
>> +	hellcreek_write(hellcreek, entry->mac[3] | (entry->mac[2] << 8), HR_FDBWDM);
>> +	hellcreek_write(hellcreek, entry->mac[5] | (entry->mac[4] << 8), HR_FDBWDL);
>> +
>> +	/* Meta data */
>> +	meta |= entry->portmask << HR_FDBWRM0_PORTMASK_SHIFT;
>> +	if (entry->is_obt)
>> +		meta |= HR_FDBWRM0_OBT;
>> +	if (entry->reprio_en) {
>> +		meta |= HR_FDBWRM0_REPRIO_EN;
>> +		meta |= entry->reprio_tc << HR_FDBWRM0_REPRIO_TC_SHIFT;
>> +	}
>> +	hellcreek_write(hellcreek, meta, HR_FDBWRM0);
>> +
>> +	/* Commit */
>> +	hellcreek_write(hellcreek, 0x00, HR_FDBWRCMD);
>> +
>> +	/* Wait until done */
>> +	ret = hellcreek_wait_fdb_ready(hellcreek);
>> +
>> +	return ret;
>
> Can you just do a tail call return here?

Sure.

>
>> +}
>> +
>> +static int __hellcreek_fdb_del(struct hellcreek *hellcreek,
>> +			       const struct hellcreek_fdb_entry *entry)
>> +{
>> +	int ret;
>> +
>> +	dev_dbg(hellcreek->dev, "Delete FDB entry: MAC=%pM!\n", entry->mac);
>> +
>> +	/* Delete by matching idx */
>> +	hellcreek_write(hellcreek, entry->idx | HR_FDBWRCMD_FDBDEL, HR_FDBWRCMD);
>> +
>> +	/* Wait until done */
>> +	ret = hellcreek_wait_fdb_ready(hellcreek);
>> +
>> +	return ret;
>
> Likewise
>
>> +}
>> +
>> +/* Retrieve the index of a FDB entry by mac address. Currently we search through
>> + * the complete table in hardware. If that's too slow, we might have to cache
>> + * the complete FDB table in software.
>> + */
>> +static int hellcreek_fdb_get(struct hellcreek *hellcreek,
>> +			     const unsigned char *dest,
>> +			     struct hellcreek_fdb_entry *entry)
>> +{
>> +	size_t i;
>> +
>> +	/* Set read pointer to zero: The read of HR_FDBMAX (read-only register)
>> +	 * should reset the internal pointer. But, that doesn't work. The vendor
>> +	 * suggested a subsequent write as workaround. Same for HR_FDBRDH below.
>> +	 */
>> +	hellcreek_read(hellcreek, HR_FDBMAX);
>> +	hellcreek_write(hellcreek, 0x00, HR_FDBMAX);
>> +
>> +	/* We have to read the complete table, because the switch/driver might
>> +	 * enter new entries anywhere.
>> +	 */
>> +	for (i = 0; i < hellcreek->fdb_entries; ++i) {
>> +		unsigned char addr[ETH_ALEN];
>> +		u16 meta, mac;
>> +
>> +		meta	= hellcreek_read(hellcreek, HR_FDBMDRD);
>> +		mac	= hellcreek_read(hellcreek, HR_FDBRDL);
>> +		addr[5] = mac & 0xff;
>> +		addr[4] = (mac & 0xff00) >> 8;
>> +		mac	= hellcreek_read(hellcreek, HR_FDBRDM);
>> +		addr[3] = mac & 0xff;
>> +		addr[2] = (mac & 0xff00) >> 8;
>> +		mac	= hellcreek_read(hellcreek, HR_FDBRDH);
>> +		addr[1] = mac & 0xff;
>> +		addr[0] = (mac & 0xff00) >> 8;
>> +
>> +		/* Force next entry */
>> +		hellcreek_write(hellcreek, 0x00, HR_FDBRDH);
>> +
>> +		if (memcmp(addr, dest, 6))
>
> ETH_ALEN instead of 6 would make it obvious what this is about

Of course. I've simply missed that.

> , don't
> you also need to compare against a VLAN ID somehow?

The hardware doesn't provide VLAN information for fdb entries.

>
> [snip]
>
>> +
>> +/* Default setup for DSA:
>> + *  VLAN 2: CPU and Port 1 egress untagged.
>> + *  VLAN 3: CPU and Port 2 egress untagged.
>
> Can you use any of the DSA_TAG_8021Q services to help you with that?

Maybe dsa_port_setup_8021q_tagging() could be used. It does distinguish
between RX and TX, but I assume it'd also work. Needs to be tested.

>
> [snip]
>
>> +
>> +static const struct dsa_switch_ops hellcreek_ds_ops = {
>> +	.get_tag_protocol    = hellcreek_get_tag_protocol,
>> +	.setup		     = hellcreek_setup,
>> +	.get_strings	     = hellcreek_get_strings,
>> +	.get_ethtool_stats   = hellcreek_get_ethtool_stats,
>> +	.get_sset_count	     = hellcreek_get_sset_count,
>> +	.port_enable	     = hellcreek_port_enable,
>> +	.port_disable	     = hellcreek_port_disable,
>> +	.port_vlan_filtering = hellcreek_vlan_filtering,
>> +	.port_vlan_prepare   = hellcreek_vlan_prepare,
>> +	.port_vlan_add	     = hellcreek_vlan_add,
>> +	.port_vlan_del	     = hellcreek_vlan_del,
>> +	.port_fdb_dump	     = hellcreek_fdb_dump,
>> +	.port_fdb_add	     = hellcreek_fdb_add,
>> +	.port_fdb_del	     = hellcreek_fdb_del,
>> +	.port_bridge_join    = hellcreek_port_bridge_join,
>> +	.port_bridge_leave   = hellcreek_port_bridge_leave,
>> +	.port_stp_state_set  = hellcreek_port_stp_state_set,> +	.phylink_validate    = hellcreek_phylink_validate,
>
> No mac_config, mac_link_up or mac_link_down operations?

No, they're not needed. Currently the hardware only does 100Mbit/s full
duplex. So, I've just created the phylink validate function to advertise
that mask.

Thanks,
Kurt

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)

Powered by blists - more mailing lists