linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Vladimir Oltean <olteanv@gmail.com>
To: Prasanna Vengateshan <prasanna.vengateshan@microchip.com>
Cc: andrew@lunn.ch, netdev@vger.kernel.org, robh+dt@kernel.org,
	UNGLinuxDriver@microchip.com, Woojung.Huh@microchip.com,
	hkallweit1@gmail.com, linux@armlinux.org.uk, davem@davemloft.net,
	kuba@kernel.org, linux-kernel@vger.kernel.org,
	vivien.didelot@gmail.com, f.fainelli@gmail.com,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v3 net-next 05/10] net: dsa: microchip: add DSA support for microchip lan937x
Date: Sat, 31 Jul 2021 18:04:16 +0300	[thread overview]
Message-ID: <20210731150416.upe5nwkwvwajhwgg@skbuf> (raw)
In-Reply-To: <20210723173108.459770-6-prasanna.vengateshan@microchip.com>

On Fri, Jul 23, 2021 at 11:01:03PM +0530, Prasanna Vengateshan wrote:
> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index 2e6bfd333f50..690d339edd7b 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -60,6 +60,8 @@ struct ksz_device {
>  
>  	struct gpio_desc *reset_gpio;	/* Optional reset GPIO */
>  
> +	struct device_node *mdio_np;
> +

I don't think you need to hold a reference to the MDIO bus DT node across
the lifetime of the driver, at least other drivers don't, they drop the
reference to it as soon as they finish with of_mdiobus_register and I
don't think they have seen any adverse effects.

> +int lan937x_cfg32(struct ksz_device *dev, u32 addr, u32 bits, bool set)
> +{
> +	return regmap_update_bits(dev->regmap[2], addr, bits, set ? bits : 0);
> +}

This function is unused.

> +int lan937x_port_cfg32(struct ksz_device *dev, int port, int offset, u32 bits,
> +		       bool set)
> +{
> +	return regmap_update_bits(dev->regmap[2], PORT_CTRL_ADDR(port, offset),
> +				  bits, set ? bits : 0);
> +}

Likewise.

> +static void lan937x_r_mib_cnt(struct ksz_device *dev, int port, u16 addr,
> +			      u64 *cnt)
> +{
> +	unsigned int val;
> +	u32 data;
> +	int ret;
> +
> +	/* Enable MIB Counter read*/

You can try to be more careful with the style of the comments, ensure
that there is a space between the last character and the */ marker, here
and everywhere.

> +	data = MIB_COUNTER_READ;
> +	data |= (addr << MIB_COUNTER_INDEX_S);
> +	lan937x_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT, data);
> +
> +	ret = regmap_read_poll_timeout(dev->regmap[2],
> +				       PORT_CTRL_ADDR(port, REG_PORT_MIB_CTRL_STAT),
> +				       val, !(val & MIB_COUNTER_READ),
> +				       10, 1000);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to get MIB\n");
> +		return;
> +	}
> +
> +	/* count resets upon read */
> +	lan937x_pread32(dev, port, REG_PORT_MIB_DATA, &data);
> +	*cnt += data;
> +}

> +
> +bool lan937x_is_internal_100BTX_phy_port(struct ksz_device *dev, int port)

In another similar driver (sja1110), instead of adding camel case names,
I went for "base_tx" and "base_t1". Maybe that looks slightly better and
more uniform between your "100BTX" vs "t1".

> +void lan937x_mac_config(struct ksz_device *dev, int port,
> +			phy_interface_t interface)
> +{
> +	u8 data8;
> +
> +	lan937x_pread8(dev, port, REG_PORT_XMII_CTRL_1, &data8);
> +
> +	/* clear MII selection & set it based on interface later */
> +	data8 &= ~PORT_MII_SEL_M;
> +
> +	/* configure MAC based on interface */
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_MII:
> +		lan937x_config_gbit(dev, false, &data8);
> +		data8 |= PORT_MII_SEL;
> +		break;
> +	case PHY_INTERFACE_MODE_RMII:
> +		lan937x_config_gbit(dev, false, &data8);
> +		data8 |= PORT_RMII_SEL;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII:
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		lan937x_config_gbit(dev, true, &data8);
> +		data8 |= PORT_RGMII_SEL;
> +
> +		/* Add RGMII internal delay for cpu port*/
> +		if (dsa_is_cpu_port(dev->ds, port)) {

Why only for the CPU port? I would like Andrew/Florian to have a look
here, I guess the assumption is that if the port has a phy-handle, the
RGMII delays should be dealt with by the PHY, but the logic seems to be
"is a CPU port <=> has a phy-handle / isn't a CPU port <=> doesn't have
a phy-handle"? What if it's a fixed-link port connected to a downstream
switch, for which this one is a DSA master?

> +			if (interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +			    interface == PHY_INTERFACE_MODE_RGMII_RXID)
> +				data8 |= PORT_RGMII_ID_IG_ENABLE;
> +
> +			if (interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +			    interface == PHY_INTERFACE_MODE_RGMII_TXID)
> +				data8 |= PORT_RGMII_ID_EG_ENABLE;
> +		}
> +		break;
> +	default:
> +		dev_err(dev->dev, "Unsupported interface '%s' for port %d\n",
> +			phy_modes(interface), port);
> +		return;
> +	}
> +
> +	/* Write the updated value */
> +	lan937x_pwrite8(dev, port, REG_PORT_XMII_CTRL_1, data8);
> +}

> +static int lan937x_mdio_register(struct dsa_switch *ds)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	int ret;
> +
> +	dev->mdio_np = of_get_child_by_name(ds->dev->of_node, "mdio");

So you support both the cases where an internal PHY is described using
OF bindings, and where the internal PHY is implicitly accessed using the
slave_mii_bus of the switch, at a PHY address equal to the port number,
and with no phy-handle or fixed-link device tree property for that port?

Do you need both alternatives? The first is already more flexible than
the second.

> +static void lan937x_port_stp_state_set(struct dsa_switch *ds, int port,
> +				       u8 state)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_port *p = &dev->ports[port];
> +	int forward = dev->member;
> +	int member = -1;
> +	u8 data;
> +
> +	lan937x_pread8(dev, port, P_STP_CTRL, &data);
> +	data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE | PORT_LEARN_DISABLE);
> +
> +	switch (state) {
> +	case BR_STATE_DISABLED:
> +		data |= PORT_LEARN_DISABLE;
> +		break;
> +	case BR_STATE_LISTENING:
> +		data |= (PORT_RX_ENABLE | PORT_LEARN_DISABLE);
> +		if (p->stp_state == BR_STATE_DISABLED)
> +			member = dev->host_mask | p->vid_member;
> +		break;
> +	case BR_STATE_LEARNING:
> +		data |= PORT_RX_ENABLE;
> +		break;
> +	case BR_STATE_FORWARDING:
> +		data |= (PORT_TX_ENABLE | PORT_RX_ENABLE);
> +
> +		member = dev->host_mask | p->vid_member;
> +		mutex_lock(&dev->dev_mutex);

I am not convinced that the dev_mutex brings any value in any of the ksz
drivers that use it.

> +
> +		/* Port is a member of a bridge. */
> +		if (dev->br_member & (1 << port)) {
> +			dev->member |= (1 << port);
> +			member = dev->member;
> +		}
> +		mutex_unlock(&dev->dev_mutex);
> +		break;
> +	case BR_STATE_BLOCKING:
> +		data |= PORT_LEARN_DISABLE;
> +		if (p->stp_state == BR_STATE_DISABLED)
> +			member = dev->host_mask | p->vid_member;
> +		break;
> +	default:
> +		dev_err(ds->dev, "invalid STP state: %d\n", state);
> +		return;
> +	}
> +
> +	lan937x_pwrite8(dev, port, P_STP_CTRL, data);
> +
> +	p->stp_state = state;
> +	mutex_lock(&dev->dev_mutex);
> +
> +	/* Port membership may share register with STP state. */
> +	if (member >= 0 && member != p->member)
> +		lan937x_cfg_port_member(dev, port, (u8)member);
> +
> +	/* Check if forwarding needs to be updated. */
> +	if (state != BR_STATE_FORWARDING) {
> +		if (dev->br_member & (1 << port))
> +			dev->member &= ~(1 << port);
> +	}
> +
> +	/* When topology has changed the function ksz_update_port_member
> +	 * should be called to modify port forwarding behavior.
> +	 */
> +	if (forward != dev->member)
> +		ksz_update_port_member(dev, port);
> +
> +	mutex_unlock(&dev->dev_mutex);
> +}

  reply	other threads:[~2021-07-31 15:06 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-23 17:30 [PATCH v3 net-next 00/10] net: dsa: microchip: DSA driver support for LAN937x switch Prasanna Vengateshan
2021-07-23 17:30 ` [PATCH v3 net-next 01/10] dt-bindings: net: dsa: dt bindings for microchip lan937x Prasanna Vengateshan
2021-07-26 22:49   ` Rob Herring
2021-07-23 17:31 ` [PATCH v3 net-next 02/10] net: dsa: move mib->cnt_ptr reset code to ksz_common.c Prasanna Vengateshan
2021-07-23 18:53   ` Vladimir Oltean
2021-07-23 17:31 ` [PATCH v3 net-next 03/10] net: phy: Add support for LAN937x T1 phy driver Prasanna Vengateshan
2021-08-11 17:52   ` Prasanna Vengateshan
2021-07-23 17:31 ` [PATCH v3 net-next 04/10] net: dsa: tag_ksz: add tag handling for Microchip LAN937x Prasanna Vengateshan
2021-07-23 19:23   ` Vladimir Oltean
2021-07-23 17:31 ` [PATCH v3 net-next 05/10] net: dsa: microchip: add DSA support for microchip lan937x Prasanna Vengateshan
2021-07-31 15:04   ` Vladimir Oltean [this message]
2021-07-31 22:05     ` Andrew Lunn
2021-08-02 21:33       ` Vladimir Oltean
2021-08-03 14:43         ` Andrew Lunn
2021-08-03 15:05           ` Vladimir Oltean
2021-08-02 10:45     ` Prasanna Vengateshan
2021-08-02 12:15       ` Vladimir Oltean
2021-08-02 13:13         ` Andrew Lunn
2021-08-02 13:59           ` Vladimir Oltean
2021-08-02 20:47             ` Andrew Lunn
2021-08-03 16:54             ` Prasanna Vengateshan
2021-08-03 23:54               ` Vladimir Oltean
2021-08-04  9:59                 ` Russell King (Oracle)
2021-08-04 10:46                   ` Vladimir Oltean
2021-08-04 14:28                     ` Prasanna Vengateshan
2021-08-04 14:51                       ` Vladimir Oltean
2021-08-07 15:40                     ` Andrew Lunn
2021-08-07 17:00                       ` Vladimir Oltean
2021-08-11 17:44                       ` Prasanna Vengateshan
2021-08-11 18:23                         ` Andrew Lunn
2021-08-11 20:14                           ` Russell King (Oracle)
2021-08-11 20:20                             ` Vladimir Oltean
2021-08-11 20:22                               ` Andrew Lunn
2021-07-23 17:31 ` [PATCH v3 net-next 06/10] net: dsa: microchip: add support for phylink management Prasanna Vengateshan
2021-07-31 15:27   ` Vladimir Oltean
2021-08-03 17:04     ` Prasanna Vengateshan
2021-07-23 17:31 ` [PATCH v3 net-next 07/10] net: dsa: microchip: add support for ethtool port counters Prasanna Vengateshan
2021-07-23 17:31 ` [PATCH v3 net-next 08/10] net: dsa: microchip: add support for port mirror operations Prasanna Vengateshan
2021-07-31 15:24   ` Vladimir Oltean
2021-07-23 17:31 ` [PATCH v3 net-next 09/10] net: dsa: microchip: add support for fdb and mdb management Prasanna Vengateshan
2021-07-31 15:19   ` Vladimir Oltean
2021-07-23 17:31 ` [PATCH v3 net-next 10/10] net: dsa: microchip: add support for vlan operations Prasanna Vengateshan
2021-07-31 15:08   ` Vladimir Oltean
2021-08-02 10:48     ` Prasanna Vengateshan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210731150416.upe5nwkwvwajhwgg@skbuf \
    --to=olteanv@gmail.com \
    --cc=UNGLinuxDriver@microchip.com \
    --cc=Woojung.Huh@microchip.com \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=f.fainelli@gmail.com \
    --cc=hkallweit1@gmail.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=netdev@vger.kernel.org \
    --cc=prasanna.vengateshan@microchip.com \
    --cc=robh+dt@kernel.org \
    --cc=vivien.didelot@gmail.com \
    --subject='Re: [PATCH v3 net-next 05/10] net: dsa: microchip: add DSA support for microchip lan937x' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).