Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Kurt Kanzenbach <kurt@linutronix.de>
To: Florian Fainelli <f.fainelli@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Vivien Didelot <vivien.didelot@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	netdev@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	devicetree@vger.kernel.org,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	Richard Cochran <richardcochran@gmail.com>,
	Kamil Alkhouri <kamil.alkhouri@hs-offenburg.de>,
	ilias.apalodimas@linaro.org, Vladimir Oltean <olteanv@gmail.com>
Subject: Re: [PATCH v1 2/8] net: dsa: Add DSA driver for Hirschmann Hellcreek switches
Date: Mon, 13 Jul 2020 08:30:25 +0200	[thread overview]
Message-ID: <87v9islyf2.fsf@kurt> (raw)
In-Reply-To: <def49ff6-72fe-7ca0-9e00-863c314c1c3d@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 10656 bytes --]

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@linutronix.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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

  reply	other threads:[~2020-07-13  6:30 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-10 11:36 [PATCH v1 0/8] Hirschmann Hellcreek DSA driver Kurt Kanzenbach
2020-07-10 11:36 ` [PATCH v1 1/8] net: dsa: Add tag handling for Hirschmann Hellcreek switches Kurt Kanzenbach
2020-07-12  2:57   ` Florian Fainelli
2020-07-10 11:36 ` [PATCH v1 2/8] net: dsa: Add DSA driver " Kurt Kanzenbach
2020-07-11 20:33   ` Florian Fainelli
2020-07-13  6:30     ` Kurt Kanzenbach [this message]
2020-07-16  8:29       ` Vladimir Oltean
2020-07-16  9:23         ` Kurt Kanzenbach
2020-07-16  9:39           ` Vladimir Oltean
2020-07-16 10:06             ` Kurt Kanzenbach
2020-07-10 11:36 ` [PATCH v1 3/8] net: dsa: hellcreek: Add PTP clock support Kurt Kanzenbach
2020-07-11 20:30   ` Richard Cochran
2020-07-11 20:38   ` Florian Fainelli
2020-07-13  6:34     ` Kurt Kanzenbach
2020-07-10 11:36 ` [PATCH v1 4/8] net: dsa: hellcreek: Add support for hardware timestamping Kurt Kanzenbach
2020-07-11 20:38   ` Richard Cochran
2020-07-13  6:35     ` Kurt Kanzenbach
2020-07-13  9:57   ` Vladimir Oltean
2020-07-13 10:57     ` Kurt Kanzenbach
2020-07-13 14:01       ` Richard Cochran
2020-07-13 14:12         ` Vladimir Oltean
2020-07-13 15:38           ` Richard Cochran
2020-07-10 11:36 ` [PATCH v1 5/8] net: dsa: hellcreek: Add TAPRIO offloading support Kurt Kanzenbach
2020-07-10 11:36 ` [PATCH v1 6/8] net: dsa: hellcreek: Add PTP status LEDs Kurt Kanzenbach
2020-07-10 11:36 ` [PATCH v1 7/8] dt-bindings: Add vendor prefix for Hirschmann Kurt Kanzenbach
2020-07-11 20:11   ` Florian Fainelli
2020-07-20 22:49   ` Rob Herring
2020-07-10 11:36 ` [PATCH v1 8/8] dt-bindings: net: dsa: Add documentation for Hellcreek switches Kurt Kanzenbach
2020-07-11 20:18   ` Florian Fainelli
2020-07-13  6:45     ` Kurt Kanzenbach
2020-07-13 14:44       ` Rob Herring

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87v9islyf2.fsf@kurt \
    --to=kurt@linutronix.de \
    --cc=andrew@lunn.ch \
    --cc=bigeasy@linutronix.de \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=f.fainelli@gmail.com \
    --cc=ilias.apalodimas@linaro.org \
    --cc=kamil.alkhouri@hs-offenburg.de \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=olteanv@gmail.com \
    --cc=richardcochran@gmail.com \
    --cc=robh+dt@kernel.org \
    --cc=vivien.didelot@gmail.com \
    --subject='Re: [PATCH v1 2/8] net: dsa: Add DSA driver for Hirschmann Hellcreek switches' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox