Re: [PATCH net-next v2 4/4] net: dsa: soce: Add basic support for SoC-e switch IP cores

[Andrew Lunn <andrew@lunn.ch>]

> +#define SOCE_CORE_VERSION_SHIFT			24
> +#define SOCE_CORE_SUBVERSION_SHIFT		16
> +#define SOCE_LICENSED_NUM_PORTS_MASK		GENMASK(31, 27)
> +#define SOCE_IMPLEMENTED_NUM_PORTS_MASK		GENMASK(31, 27)
> +#define SOCE_IMPLEMENTED_DSA			BIT(23)
> +#define SOCE_DSA_REGS_BASE			0x1200
> +#define SOCE_TAG_ALL_FRAMES_CTRL_OFFSET		(SOCE_DSA_REGS_BASE + 0x001c)
> +#define SOCE_TAG_ALL_FRAMES_ENABLE		BIT(0)
> +#define SOCE_CUSTOM_RULES_TAGGING_OFFSET	(SOCE_DSA_REGS_BASE + 0x0020)
> +#define SOCE_CUSTOM_RULES_TAGGING_ENABLE	BIT(0)
> +#define SOCE_MIN_CORE_VERSION			0x24
> +#define SOCE_MIN_CORE_SUBVERSION		0x01

Could you fully describe the feature registers, so we have an idea
what is actually there.

> +struct soce_probe_desc {
> +	u32 core_version_offset;
> +	u32 licensed_features_offset;
> +	u32 implemented_features0_offset;
> +	u32 mdio_master_offset;
> +};
> +
> +static const struct soce_probe_desc soce_probe_desc_swip_00_04_0c_10 = {
> +	.core_version_offset		= 0x0000,
> +	.licensed_features_offset	= 0x0004,
> +	.implemented_features0_offset	= 0x000c,
> +	.mdio_master_offset		= 0x0200,
> +};

How fixed/variable are these? I'm just thinking there may be too much
abstraction here. To some extent, we leave abstractions out until they
are needed. If you know there are other devices out there which have
these at other addresses, then O.K. But if not, i would keep it KISS.

> +static int soce_sw_detect_features(struct soce_dsa_local *local,
> +				   const struct soce_probe_desc *probe_desc,
> +				   u32 *numports)
> +{
> +	u32 implemented_numports;
> +	u32 licensed_numports;
> +	u32 regval;
> +
> +	regval = readl(local->base_addr + probe_desc->licensed_features_offset);
> +	licensed_numports = FIELD_GET(SOCE_LICENSED_NUM_PORTS_MASK, regval);
> +	if (!licensed_numports || licensed_numports > SOCE_MAX_NUM_PORTS)
> +		return -EINVAL;
> +
> +	regval = readl(local->base_addr +
> +		       probe_desc->implemented_features0_offset);
> +	if (!(regval & SOCE_IMPLEMENTED_DSA))
> +		return -ENODEV;

I find it useful to have macros like SOCE_IMPLEMENTED_DSA indicate
they apply to features0. It makes it easier to spot dumb typos when
you apply it to features1, not features0.

I also wounder at the name. Does the data sheet really call it DSA?

> +
> +	implemented_numports = FIELD_GET(SOCE_IMPLEMENTED_NUM_PORTS_MASK,
> +					 regval);
> +	if (!implemented_numports ||
> +	    implemented_numports > licensed_numports)
> +		return -EINVAL;
> +
> +	*numports = implemented_numports;

How is this going to scale when you need to look at more bits in these
registers? It seems like at some point you are going to need to pass a
structure to be filled in.

> +static int soce_register_mdio_bus(struct soce_priv *priv, struct device *dev,
> +				  struct device_node *mdio_np,
> +				  u32 mdio_output)
> +{
> +	struct soce_mdio_bus *state;
> +	struct mii_bus *bus;
> +
> +	bus = devm_mdiobus_alloc(dev);
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	state->ds = priv->ds;
> +	state->mdio_output = mdio_output;
> +
> +	bus->priv = state;
> +	bus->name = "soce mdio";
> +	bus->read = soce_mdio_read;
> +	bus->write = soce_mdio_write;
> +	bus->read_c45 = soce_mdio_read_c45;
> +	bus->write_c45 = soce_mdio_write_c45;
> +	/* ds->dst can be NULL during probe, before dsa_register_switch() */
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mdio-%u", dev_name(dev),
> +		 mdio_output);

Why the comment. How is it relevant?

> +static int soce_mdio_23_02_read_c22(struct dsa_switch *ds, int mdio_output,
> +				    int phy_addr, int regnum)
> +{
> +	void __iomem *ctrl, *params, *read_reg;
> +	struct soce_priv *priv = ds->priv;
> +	struct soce_dsa_local *local;
> +	u32 regvalue;
> +	int ret;
> +
> +	local = &priv->local;
> +	ctrl = local->mdio_master_addr + SOCE_MDIO_CTRL_OFFSET;
> +	params = local->mdio_master_addr + SOCE_MDIO_23_02_PARAMS_OFFSET;
> +	read_reg = local->mdio_master_addr + SOCE_MDIO_23_02_READ_OFFSET;
> +
> +	regvalue = (regnum << SOCE_MDIO_23_02_CTRL_REGADDRDEVTYPE_OFFSET) +
> +		   (phy_addr << SOCE_MDIO_23_02_CTRL_PHYADDR_OFFSET);
> +	writel(regvalue, params);
> +
> +	regvalue = ((mdio_output << SOCE_MDIO_23_02_CTRL_BUS_OFFSET) +
> +		    (0x3 << SOCE_MDIO_23_02_CTRL_TRANSTYPE_OFFSET) +

0x3 means read?

> +		    (0x0 << SOCE_MDIO_23_02_CTRL_CLAUSE_OFFSET) +

0x0 means C22

> +		    (0x1 << SOCE_MDIO_23_02_CTRL_OPSTATUS_OFFSET));

What does 0x1 mean? Please add some #defines for theses to act as
documentation.

	Andrew