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

[Vasilij Strassheim]

On Mon, 2026-09-07 at 21:28 +0200, Andrew Lunn wrote:
> > +#define SOCE_MAX_NUM_PORTS 31
> > +#define SOCE_MAX_MDIO_ADDR 32
> 
> Given what the binding says, that looks odd.
> 

The checks using these constants are unnecessary. I will remove them and
clarify the 31-port hardware limit.

> > +#define SOCE_MAX_MDIO_OUTPUTS SOCE_MAX_NUM_PORTS
> > +
> > +struct dsa_switch;
> > +
> > +struct soce_mdio_ops {
> > +	int (*phy_read)(struct dsa_switch *ds, int mdio_output, int phy_addr,
> > +			int regnum);
> > +	int (*phy_write)(struct dsa_switch *ds, int mdio_output, int phy_addr,
> > +			 int regnum, u16 val);
> > +	int (*phy_read_c45)(struct dsa_switch *ds, int mdio_output, int phy_addr,
> > +			    int devad, int regnum);
> > +	int (*phy_write_c45)(struct dsa_switch *ds, int mdio_output, int phy_addr,
> > +			     int devad, int regnum, u16 val);
> > +};
> > +
> > +struct soce_dsa_local {
> > +	void __iomem *base_addr;
> > +	void __iomem *mdio_master_addr;
> > +	/* Serializes all logical buses sharing the MDIO master. */
> > +	struct mutex mdio_lock;
> 
> What is the MDIO master?

It refers to the switch-integrated MDIO controller: one shared set of
MMIO transaction registers serving multiple selectable MDIO buses. The
switch documentation calls it an MDIO bridge. I will rename "master" to
"controller".

> 
> > +static const struct soce_mdio_ops soce_mdio_ops_c22_c45 = {
> > +	.phy_read	= soce_mdio_23_02_read,
> > +	.phy_write	= soce_mdio_23_02_write,
> > +	.phy_read_c45	= soce_mdio_23_02_read_c45,
> > +	.phy_write_c45	= soce_mdio_23_02_write_c45,
> > +};
> 
> It seems like this is the only struct soce_mdio_ops. Does the IP
> support different MDIO buses? Is this level of abstraction actually
> needed?
> 

Older IP versions used a different register layout. Since this driver
currently supports only one variant, I will remove the abstraction. It
can be added back when another variant is supported.

> > +static inline bool soce_mdio_output_valid(int mdio_output)
> > +{
> > +	return mdio_output >= 0 && mdio_output < SOCE_MAX_MDIO_OUTPUTS;
> > +}
> 
> No inline functions in .c files. Let the compile decide.

Yes, I will fix this along with some other issues reported by the
patchwork checks.

> 
> > +static inline bool soce_mdio_addr_valid(int phy_addr)
> > +{
> > +	return phy_addr >= 0 && phy_addr < SOCE_MAX_MDIO_ADDR;
> > +}
> > +
> > +static inline bool soce_mdio_c22_reg_valid(int regnum)
> > +{
> > +	return regnum >= 0 && regnum <= SOCE_MDIO_C22_REG_MAX;
> > +}
> > +
> > +static inline bool soce_mdio_c45_params_valid(int devad, int regnum)
> > +{
> > +	return devad >= 0 && devad <= SOCE_MDIO_C45_DEVAD_MAX &&
> > +	       regnum >= 0 && regnum <= SOCE_MDIO_C45_REG_MAX;
> > +}
> 
> Do you see any other MDIO driver doing this sort of checking?

No. I will remove all redundant checks. 

> 
> > +static int soce_mdio_read(struct mii_bus *bus, int addr, int reg)
> > +{
> > +	struct soce_mdio_bus *state = bus->priv;
> > +	struct soce_dsa_local *local;
> > +	struct soce_priv *priv;
> > +	int ret;
> > +
> > +	priv = state->ds->priv;
> > +	local = &priv->local;
> > +
> > +	if (!local->mdio_ops || !local->mdio_ops->phy_read)
> > +		return -EOPNOTSUPP;
> > +
> > +	if (!soce_mdio_addr_valid(addr))
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&local->mdio_lock);
> > +	ret = local->mdio_ops->phy_read(state->ds, state->mdio_output, addr,
> > +					reg);
> > +	mutex_unlock(&local->mdio_lock);
> 
> What is the lock protecting?

It serializes accesses to the shared MDIO controller transaction
registers across all MDIO buses.

> 
> > +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;
> 
> Can you think of a better name than mdio_output. It seems to be the
> bus number?
> 

Yes, it is the bus selector. I will rename it to mdio_bus_id.

> > +
> > +	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);
> > +	bus->parent = dev;
> > +
> > +	return devm_of_mdiobus_register(dev, bus, mdio_np);
> > +}
> > +int soce_mdio_23_02_read_c45(struct dsa_switch *ds, int mdio_output,
> > +			     int phy_addr, int devad, int regnum)
> > +{
> > +	void __iomem *ctrl, *params, *read_reg, *write_reg;
> > +	struct soce_priv *priv = ds->priv;
> > +	struct soce_dsa_local *local;
> > +	u32 regvalue;
> > +	int ret;
> > +
> > +	if (!soce_mdio_output_valid(mdio_output) ||
> > +	    !soce_mdio_addr_valid(phy_addr) ||
> > +	    !soce_mdio_c45_params_valid(devad, regnum))
> > +		return -EINVAL;
> 
> Hasn't this already been checked once?

Yes, I will remove this and other redundant checks. 

> 
> 	Andrew

Thanks,
Vasilij