Re: [PATCH net-next 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

[Clément Léger]

Le Thu, 14 Apr 2022 17:47:09 +0300,
Vladimir Oltean <olteanv@xxxxxxxxx> a écrit :

> 
> > later (vlan, etc).
> > 
> > Suggested-by: Laurent Gonzales <laurent.gonzales@xxxxxxxxxx>
> > Suggested-by: Jean-Pierre Geslin <jean-pierre.geslin@xxxxxxxxxx>
> > Suggested-by: Phil Edworthy <phil.edworthy@xxxxxxxxxxx>  
> 
> Suggested? What did they suggest? "You should write a driver"?
> We have a Co-developed-by: tag, maybe it's more appropriate here?

This driver was written from scratch but some ideas (port isolation
using pattern matcher) was inspired from a previous driver. I thought it
would be nice to give them credit for that.

[...]

> >  obj-y				+= hirschmann/
> >  obj-y				+= microchip/
> > diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
> > new file mode 100644
> > index 000000000000..5bee999f7050
> > --- /dev/null
> > +++ b/drivers/net/dsa/rzn1_a5psw.c
> > @@ -0,0 +1,676 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2022 Schneider-Electric
> > + *
> > + * Clément Léger <clement.leger@xxxxxxxxxxx>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/etherdevice.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_mdio.h>
> > +#include <net/dsa.h>
> > +#include <uapi/linux/if_bridge.h>  
> 
> Why do you need to include this header?

It defines BR_STATE_* but I guess linux/if_bridge.h does include it.

> > +
> > +static void a5psw_port_pattern_set(struct a5psw *a5psw, int port, int pattern,
> > +				   bool enable)
> > +{
> > +	u32 rx_match = 0;
> > +
> > +	if (enable)
> > +		rx_match |= A5PSW_RXMATCH_CONFIG_PATTERN(pattern);
> > +
> > +	a5psw_reg_rmw(a5psw, A5PSW_RXMATCH_CONFIG(port),
> > +		      A5PSW_RXMATCH_CONFIG_PATTERN(pattern), rx_match);
> > +}
> > +
> > +static void a5psw_port_mgmtfwd_set(struct a5psw *a5psw, int port, bool enable)  
> 
> Some explanation on what "management forward" means/does?

I'll probably rename that cpu_port_forward to match the dsa naming.
It'll actually isolate the port from other ports by only forwarding the
packets to the CPU port.

> > +
> > +static int a5psw_setup(struct dsa_switch *ds)
> > +{
> > +	struct a5psw *a5psw = ds->priv;
> > +	int port, vlan, ret;
> > +	u32 reg;
> > +
> > +	/* Configure management port */
> > +	reg = A5PSW_CPU_PORT | A5PSW_MGMT_CFG_DISCARD;
> > +	a5psw_reg_writel(a5psw, A5PSW_MGMT_CFG, reg);  
> 
> Perhaps you should validate the DT blob that the CPU port is the one
> that you think it is?

You are right, the datasheet says that the management port should
actually always be the CPU port so I guess a check would be nice.

> > +
> > +	/* Reset learn count to 0 */
> > +	reg = A5PSW_LK_LEARNCOUNT_MODE_SET;
> > +	a5psw_reg_writel(a5psw, A5PSW_LK_LEARNCOUNT, reg);
> > +
> > +	/* Clear VLAN resource table */
> > +	reg = A5PSW_VLAN_RES_WR_PORTMASK | A5PSW_VLAN_RES_WR_TAGMASK;
> > +	for (vlan = 0; vlan < A5PSW_VLAN_COUNT; vlan++)
> > +		a5psw_reg_writel(a5psw, A5PSW_VLAN_RES(vlan), reg);
> > +
> > +	/* Reset all ports */
> > +	for (port = 0; port < ds->num_ports; port++) {  
> 
> Because dsa_is_cpu_port() internally calls dsa_to_port() which iterates
> through a list, we tend to avoid the pattern where we call a list
> iterating function from a loop over essentially the same data.
> Instead, we have:
> 
> 	dsa_switch_for_each_port(dp, ds) {
> 		if (dsa_port_is_unused(dp))
> 			do stuff with dp->index
> 		if (dsa_port_is_cpu(dp))
> 			...
> 		if (dsa_port_is_user(dp))
> 			...
> 	}

Nice catch indeed, I'll convert that.

> > +
> > +static int a5psw_probe_mdio(struct a5psw *a5psw)
> > +{
> > +	struct device *dev = a5psw->dev;
> > +	struct device_node *mdio_node;
> > +	struct mii_bus *bus;
> > +	int err;
> > +
> > +	if (of_property_read_u32(dev->of_node, "clock-frequency",
> > +				 &a5psw->mdio_freq))
> > +		a5psw->mdio_freq = A5PSW_MDIO_DEF_FREQ;  
> 
> Shouldn't the clock-frequency be a property of the "mdio" node?
> At least I see it in Documentation/devicetree/bindings/net/mdio.yaml.

Yes, totally.

> > +static const struct of_device_id a5psw_of_mtable[] = {
> > +	{ .compatible = "renesas,rzn1-a5psw", },
> > +	{ /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, a5psw_of_mtable);
> > +
> > +static struct platform_driver a5psw_driver = {
> > +	.driver = {
> > +		.name	 = "rzn1_a5psw",
> > +		.of_match_table = of_match_ptr(a5psw_of_mtable),
> > +	},
> > +	.probe = a5psw_probe,
> > +	.remove = a5psw_remove,  
> 
> Please implement .shutdown too, it's non-optional.

Hum, platform_shutdown does seems to check for the .shutdown callback:

static void platform_shutdown(struct device *_dev)
{
	struct platform_device *dev = to_platform_device(_dev);
	struct platform_driver *drv;

	if (!_dev->driver)
		return;

	drv = to_platform_driver(_dev->driver);
	if (drv->shutdown)
		drv->shutdown(dev);
}

Is there some documentation specifying that this is mandatory ?
If so, should I just set it to point to an empty shutdown function then
?

> 
> > +/**
> > + * struct a5psw - switch struct
> > + * @base: Base address of the switch
> > + * @hclk_rate: hclk_switch clock rate
> > + * @clk_rate: clk_switch clock rate
> > + * @dev: Device associated to the switch
> > + * @mii_bus: MDIO bus struct
> > + * @mdio_freq: MDIO bus frequency requested
> > + * @pcs: Array of PCS connected to the switch ports (not for the CPU)
> > + * @ds: DSA switch struct
> > + * @lk_lock: Lock for the lookup table
> > + * @reg_lock: Lock for register read-modify-write operation  
> 
> Interesting concept. Generally we see higher-level locking schemes
> (i.e. a rmw lock won't really ensure much in terms of consistency of
> settings if that's the only thing that serializes concurrent thread
> accesses to some register).

Agreed, this does not guarantee consistency of settings but guarantees
that rmw modifications are atomic between devices. I wasn't sure about
the locking guarantee that I could have. After looking at other
drivers, I guess I will switch to something more common such as using
a global mutex for register accesses.

> 
> Anyway, probably doesn't hurt to have it.
> 
> > + * @flooding_ports: List of ports that should be flooded
> > + */
> > +struct a5psw {
> > +	void __iomem *base;
> > +	struct clk* hclk;
> > +	struct clk* clk;
> > +	struct device *dev;
> > +	struct mii_bus	*mii_bus;
> > +	u32 mdio_freq;
> > +	struct phylink_pcs *pcs[A5PSW_PORTS_NUM - 1];
> > +	struct dsa_switch ds;
> > +	spinlock_t lk_lock;
> > +	spinlock_t reg_lock;
> > +	u32 flooding_ports;
> > +};
> > -- 
> > 2.34.1
> >   
> 
> We have some selftests in tools/testing/selftests/net/forwarding/, like
> for example bridge_vlan_unaware.sh. They create veth pairs by default,
> but if you edit the NETIF_CREATE configuration you should be able to
> pass your DSA interfaces.

Ok, great to know that there are some tests that can be used.

> The selftests don't cover nearly enough, but just to make sure that they
> pass for your switch, when you use 2 switch ports as h1 and h2 (hosts),
> and 2 ports as swp1 and swp2? There's surprisingly little that you do on
> .port_bridge_join, I need to study the code more.

Port isolation is handled by using a pattern matcher which is enabled
for each port at setup. If set, the port packet will only be forwarded
to the CPU port. When bridging is needed, the pattern matching is
disabled and thus, the packets are forwarded between all the ports that
are enabled in the bridge.

Thanks,

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com