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@xxxxxxxxxxxxx>
>> ---
>
> [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:
signature.asc
Description: PGP signature
