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

[Vladimir Oltean <olteanv@gmail.com>]

On Wed, May 04, 2022 at 11:29:54AM +0200, Clément Léger wrote:
> Add Renesas RZ/N1 advanced 5 port switch driver. This switch handles 5
> ports including 1 CPU management port. A MDIO bus is also exposed by
> this switch and allows to communicate with PHYs connected to the ports.
> Each switch port (except for the CPU management ports) is connected to
> the MII converter.
> 
> This driver includes basic bridging support, more support will be added
> later (vlan, etc).
> 
> Suggested-by: Jean-Pierre Geslin <jean-pierre.geslin@non.se.com>
> Suggested-by: Phil Edworthy <phil.edworthy@renesas.com>
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
> +static void a5psw_port_disable(struct dsa_switch *ds, int port)
> +{
> +	struct a5psw *a5psw = ds->priv;
> +
> +	a5psw_port_authorize_set(a5psw, port, false);
> +	a5psw_port_enable_set(a5psw, port, false);
> +	a5psw_port_fdb_flush(a5psw, port);

The bridge core takes care of this by setting the port state to
DISABLED, which makes DSA call dsa_port_fast_age(), no?

Standalone ports shouldn't need fast ageing because they shouldn't have
address learning enabled in the first place.

> +}

> +static int a5psw_port_bridge_join(struct dsa_switch *ds, int port,
> +				  struct dsa_bridge bridge,
> +				  bool *tx_fwd_offload,
> +				  struct netlink_ext_ack *extack)
> +{
> +	struct a5psw *a5psw = ds->priv;
> +
> +	/* We only support 1 bridge device */
> +	if (a5psw->br_dev && bridge.dev != a5psw->br_dev)
> +		return -EINVAL;

return -EOPNOTSUPP, to allow software bridging.
You might also want to set an extack message here and avoid overwriting
it in dsa_slave_changeupper() with "Offloading not supported", but say
something more specific like "Forwarding offload supported for a single
bridge".

> +
> +	a5psw->br_dev = bridge.dev;
> +	a5psw_flooding_set_resolution(a5psw, port, true);
> +	a5psw_port_mgmtfwd_set(a5psw, port, false);
> +
> +	return 0;
> +}
> +
> +static void a5psw_port_bridge_leave(struct dsa_switch *ds, int port,
> +				    struct dsa_bridge bridge)
> +{
> +	struct a5psw *a5psw = ds->priv;
> +
> +	a5psw_flooding_set_resolution(a5psw, port, false);
> +	a5psw_port_mgmtfwd_set(a5psw, port, true);
> +
> +	/* No more port bridged */

s/port/ports/

> +	if (a5psw->bridged_ports == BIT(A5PSW_CPU_PORT))
> +		a5psw->br_dev = NULL;
> +}

> +static int a5psw_pcs_get(struct a5psw *a5psw)
> +{
> +	struct device_node *ports, *port, *pcs_node;
> +	struct phylink_pcs *pcs;
> +	int ret;
> +	u32 reg;
> +
> +	ports = of_get_child_by_name(a5psw->dev->of_node, "ports");

Can you please do:

	ports = of_get_child_by_name(a5psw->dev->of_node, "ethernet-ports");
	if (!ports)
		ports = of_get_child_by_name(a5psw->dev->of_node, "ports");

> +	if (!ports)
> +		return -EINVAL;
> +
> +	for_each_available_child_of_node(ports, port) {
> +		pcs_node = of_parse_phandle(port, "pcs-handle", 0);
> +		if (!pcs_node)
> +			continue;
> +
> +		if (of_property_read_u32(port, "reg", &reg)) {
> +			ret = -EINVAL;
> +			goto free_pcs;
> +		}
> +
> +		if (reg >= ARRAY_SIZE(a5psw->pcs)) {
> +			ret = -ENODEV;
> +			goto free_pcs;
> +		}
> +
> +		pcs = miic_create(pcs_node);
> +		if (IS_ERR(pcs)) {
> +			dev_err(a5psw->dev, "Failed to create PCS for port %d\n",
> +				reg);
> +			ret = PTR_ERR(pcs);
> +			goto free_pcs;
> +		}
> +
> +		a5psw->pcs[reg] = pcs;
> +	}
> +	of_node_put(ports);
> +
> +	return 0;
> +
> +free_pcs:
> +	a5psw_pcs_free(a5psw);
> +
> +	return ret;
> +}

> +/* Ensure enough space for 2 VLAN tags */
> +#define A5PSW_EXTRA_MTU_LEN		(A5PSW_TAG_LEN + 8)
> +#define A5PSW_MAX_MTU			(A5PSW_JUMBO_LEN - A5PSW_EXTRA_MTU_LEN)
> +#define A5PSW_MGMT_TAG_VALUE		0xE001
> +
> +#define A5PSW_PATTERN_MGMTFWD		0
> +
> +#define A5PSW_LK_BUSY_USEC_POLL		10
> +#define A5PSW_CTRL_TIMEOUT		1000
> +#define A5PSW_TABLE_ENTRIES		8192
> +
> +/**
> + * struct a5psw - switch struct
> + * @base: Base address of the switch
> + * @hclk: hclk_switch clock
> + * @clk: clk_switch clock
> + * @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
> + * @bridged_ports: List of ports that are bridged and should be flooded

s/List/Mask/

> + * @br_dev: Bridge net device
> + */
> +struct a5psw {
> +	void __iomem *base;
> +	struct clk *hclk;
> +	struct clk *clk;
> +	struct device *dev;
> +	struct mii_bus	*mii_bus;
> +	struct phylink_pcs *pcs[A5PSW_PORTS_NUM - 1];
> +	struct dsa_switch ds;
> +	spinlock_t lk_lock;
> +	spinlock_t reg_lock;
> +	u32 bridged_ports;
> +	struct net_device *br_dev;
> +};
> -- 
> 2.34.1
>