diff mbox series

[net-next,2/6] net: dsa: mt7530: Extend device data ready for adding a new hardware

Message ID 2d546d6bb15ff8b4b75af2220e20db4e634f4145.1575914275.git.landen.chao@mediatek.com (mailing list archive)
State New, archived
Headers show
Series net-next: dsa: mt7530: add support for MT7531 | expand

Commit Message

Landen Chao (趙皎宏) Dec. 10, 2019, 8:14 a.m. UTC
Add a structure holding required operations for each device such as device
initialization, PHY port read or write, a checker whether PHY interface is
supported on a certain port, MAC port setup for either bus pad or a
specific PHY interface.

The patch is done for ready adding a new hardware MT7531.

Signed-off-by: Landen Chao <landen.chao@mediatek.com>
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/net/dsa/mt7530.c | 231 +++++++++++++++++++++++++++++----------
 drivers/net/dsa/mt7530.h |  29 ++++-
 2 files changed, 203 insertions(+), 57 deletions(-)

Comments

Florian Fainelli Dec. 12, 2019, 3:45 a.m. UTC | #1
On 12/10/2019 12:14 AM, Landen Chao wrote:
> Add a structure holding required operations for each device such as device
> initialization, PHY port read or write, a checker whether PHY interface is
> supported on a certain port, MAC port setup for either bus pad or a
> specific PHY interface.
> 
> The patch is done for ready adding a new hardware MT7531.
> 
> Signed-off-by: Landen Chao <landen.chao@mediatek.com>
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
[snip]

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index ed1ec10ec62b..9a648d1f5d09 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -425,7 +425,7 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
>  }
>  
>  static int
> -mt7530_pad_clk_setup(struct dsa_switch *ds, int mode)
> +mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t mode)

Here you pass a phy_interface_t argument but not in mt7632_pad_clk_setup().

> +static int
> +mt7530_pad_setup(struct dsa_switch *ds, const struct phylink_link_state *state)
> +{
> +	struct mt7530_priv *priv = ds->priv;
> +
> +	/* Setup TX circuit incluing relevant PAD and driving */
> +	mt7530_pad_clk_setup(ds, state->interface);
> +
> +	if (priv->id == ID_MT7530) {
> +		/* Setup RX circuit, relevant PAD and driving on the
> +		 * host which must be placed after the setup on the
> +		 * device side is all finished.
> +		 */
> +		mt7623_pad_clk_setup(ds);

Would not it make sense to pass it down here as well for consistency?

[snip]

> @@ -1660,8 +1768,19 @@ mt7530_probe(struct mdio_device *mdiodev)
>  	/* Get the hardware identifier from the devicetree node.
>  	 * We will need it for some of the clock and regulator setup.
>  	 */
> -	priv->id = (unsigned int)(unsigned long)
> -		of_device_get_match_data(&mdiodev->dev);
> +	priv->info = of_device_get_match_data(&mdiodev->dev);
> +	if (!priv->info)
> +		return -EINVAL;
> +
> +	/* Sanity check if these required device operstaions are filled
> +	 * properly.

Typo: operations.

Other than that, this looks okay to me.
Landen Chao (趙皎宏) Dec. 12, 2019, 3:03 p.m. UTC | #2
On Thu, 2019-12-12 at 11:45 +0800, Florian Fainelli wrote:
> 
> On 12/10/2019 12:14 AM, Landen Chao wrote:
> > Add a structure holding required operations for each device such as device
> > initialization, PHY port read or write, a checker whether PHY interface is
> > supported on a certain port, MAC port setup for either bus pad or a
> > specific PHY interface.
> > 
> > The patch is done for ready adding a new hardware MT7531.
> > 
> > Signed-off-by: Landen Chao <landen.chao@mediatek.com>
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > ---
> [snip]
> 
> > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > index ed1ec10ec62b..9a648d1f5d09 100644
> > --- a/drivers/net/dsa/mt7530.c
> > +++ b/drivers/net/dsa/mt7530.c
> > @@ -425,7 +425,7 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
> >  }
> >  
> >  static int
> > -mt7530_pad_clk_setup(struct dsa_switch *ds, int mode)
> > +mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t mode)
> 
> Here you pass a phy_interface_t argument but not in mt7632_pad_clk_setup().
> 
> > +static int
> > +mt7530_pad_setup(struct dsa_switch *ds, const struct phylink_link_state *state)
> > +{
> > +	struct mt7530_priv *priv = ds->priv;
> > +
> > +	/* Setup TX circuit incluing relevant PAD and driving */
> > +	mt7530_pad_clk_setup(ds, state->interface);
> > +
> > +	if (priv->id == ID_MT7530) {
> > +		/* Setup RX circuit, relevant PAD and driving on the
> > +		 * host which must be placed after the setup on the
> > +		 * device side is all finished.
> > +		 */
> > +		mt7623_pad_clk_setup(ds);
> 
> Would not it make sense to pass it down here as well for consistency?
mt7623_pad_clk() contains the setting for PHY_INTERFACE_MODE_TRGMII. It
is really better to pass phy_interface_t for error handling.
> 
> [snip]
> 
> > @@ -1660,8 +1768,19 @@ mt7530_probe(struct mdio_device *mdiodev)
> >  	/* Get the hardware identifier from the devicetree node.
> >  	 * We will need it for some of the clock and regulator setup.
> >  	 */
> > -	priv->id = (unsigned int)(unsigned long)
> > -		of_device_get_match_data(&mdiodev->dev);
> > +	priv->info = of_device_get_match_data(&mdiodev->dev);
> > +	if (!priv->info)
> > +		return -EINVAL;
> > +
> > +	/* Sanity check if these required device operstaions are filled
> > +	 * properly.
> 
> Typo: operations.
Oops, sorry. I'll correct it.

Landen
> 
> Other than that, this looks okay to me.
Landen Chao (趙皎宏) Dec. 12, 2019, 3:05 p.m. UTC | #3
On Thu, 2019-12-12 at 11:45 +0800, Florian Fainelli wrote:
> 
> On 12/10/2019 12:14 AM, Landen Chao wrote:
> > Add a structure holding required operations for each device such as device
> > initialization, PHY port read or write, a checker whether PHY interface is
> > supported on a certain port, MAC port setup for either bus pad or a
> > specific PHY interface.
> > 
> > The patch is done for ready adding a new hardware MT7531.
> > 
> > Signed-off-by: Landen Chao <landen.chao@mediatek.com>
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > ---
> [snip]
> 
> > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > index ed1ec10ec62b..9a648d1f5d09 100644
> > --- a/drivers/net/dsa/mt7530.c
> > +++ b/drivers/net/dsa/mt7530.c
> > @@ -425,7 +425,7 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
> >  }
> >  
> >  static int
> > -mt7530_pad_clk_setup(struct dsa_switch *ds, int mode)
> > +mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t mode)
> 
> Here you pass a phy_interface_t argument but not in mt7632_pad_clk_setup().
> 
> > +static int
> > +mt7530_pad_setup(struct dsa_switch *ds, const struct phylink_link_state *state)
> > +{
> > +	struct mt7530_priv *priv = ds->priv;
> > +
> > +	/* Setup TX circuit incluing relevant PAD and driving */
> > +	mt7530_pad_clk_setup(ds, state->interface);
> > +
> > +	if (priv->id == ID_MT7530) {
> > +		/* Setup RX circuit, relevant PAD and driving on the
> > +		 * host which must be placed after the setup on the
> > +		 * device side is all finished.
> > +		 */
> > +		mt7623_pad_clk_setup(ds);
> 
> Would not it make sense to pass it down here as well for consistency?
mt7623_pad_clk() contains the setting for PHY_INTERFACE_MODE_TRGMII. It
is really better to pass phy_interface_t for error handling.
> 
> [snip]
> 
> > @@ -1660,8 +1768,19 @@ mt7530_probe(struct mdio_device *mdiodev)
> >  	/* Get the hardware identifier from the devicetree node.
> >  	 * We will need it for some of the clock and regulator setup.
> >  	 */
> > -	priv->id = (unsigned int)(unsigned long)
> > -		of_device_get_match_data(&mdiodev->dev);
> > +	priv->info = of_device_get_match_data(&mdiodev->dev);
> > +	if (!priv->info)
> > +		return -EINVAL;
> > +
> > +	/* Sanity check if these required device operstaions are filled
> > +	 * properly.
> 
> Typo: operations.
Oops, sorry. I'll correct it.

Landen
> 
> Other than that, this looks okay to me.
diff mbox series

Patch

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index ed1ec10ec62b..9a648d1f5d09 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -425,7 +425,7 @@  mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
 }
 
 static int
-mt7530_pad_clk_setup(struct dsa_switch *ds, int mode)
+mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t mode)
 {
 	struct mt7530_priv *priv = ds->priv;
 	u32 ncpo1, ssc_delta, trgint, i, xtal;
@@ -1380,13 +1380,111 @@  mt7530_setup(struct dsa_switch *ds)
 	return 0;
 }
 
-static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
+static bool mt7530_phy_supported(struct dsa_switch *ds, int port,
+				 const struct phylink_link_state *state)
+{
+	struct mt7530_priv *priv = ds->priv;
+
+	switch (port) {
+	case 0: /* Internal phy */
+	case 1:
+	case 2:
+	case 3:
+	case 4:
+		if (state->interface != PHY_INTERFACE_MODE_GMII)
+			goto unsupported;
+		break;
+	case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
+		if (!phy_interface_mode_is_rgmii(state->interface) &&
+		    state->interface != PHY_INTERFACE_MODE_MII &&
+		    state->interface != PHY_INTERFACE_MODE_GMII)
+			goto unsupported;
+		break;
+	case 6: /* 1st cpu port */
+		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
+		    state->interface != PHY_INTERFACE_MODE_TRGMII)
+			goto unsupported;
+		break;
+	default:
+		dev_err(priv->dev, "%s: unsupported port: %i\n", __func__,
+			port);
+		goto unsupported;
+	}
+
+	return true;
+
+unsupported:
+	return false;
+}
+
+static bool mt753x_phy_supported(struct dsa_switch *ds, int port,
+				 const struct phylink_link_state *state)
+{
+	struct mt7530_priv *priv = ds->priv;
+
+	return priv->info->phy_supported(ds, port, state);
+}
+
+static int
+mt7530_pad_setup(struct dsa_switch *ds, const struct phylink_link_state *state)
+{
+	struct mt7530_priv *priv = ds->priv;
+
+	/* Setup TX circuit incluing relevant PAD and driving */
+	mt7530_pad_clk_setup(ds, state->interface);
+
+	if (priv->id == ID_MT7530) {
+		/* Setup RX circuit, relevant PAD and driving on the
+		 * host which must be placed after the setup on the
+		 * device side is all finished.
+		 */
+		mt7623_pad_clk_setup(ds);
+	}
+
+	return 0;
+}
+
+static int
+mt753x_pad_setup(struct dsa_switch *ds, const struct phylink_link_state *state)
+{
+	struct mt7530_priv *priv = ds->priv;
+
+	return priv->info->pad_setup(ds, state);
+}
+
+static int
+mt7530_mac_setup(struct dsa_switch *ds, int port, unsigned int mode,
+		 const struct phylink_link_state *state)
+{
+	struct mt7530_priv *priv = ds->priv;
+
+	/* Only need to setup port5. */
+	if (port != 5)
+		return 0;
+
+	mt7530_setup_port5(priv->ds, state->interface);
+
+	return 0;
+}
+
+static int mt753x_mac_setup(struct dsa_switch *ds, int port, unsigned int mode,
+			    const struct phylink_link_state *state)
+{
+	struct mt7530_priv *priv = ds->priv;
+
+	return priv->info->mac_setup(ds, port, mode, state);
+}
+
+static void mt753x_phylink_mac_config(struct dsa_switch *ds, int port,
 				      unsigned int mode,
 				      const struct phylink_link_state *state)
 {
 	struct mt7530_priv *priv = ds->priv;
 	u32 mcr_cur, mcr_new;
 
+	if (!mt753x_phy_supported(ds, port, state))
+		return;
+
 	switch (port) {
 	case 0: /* Internal phy */
 	case 1:
@@ -1399,35 +1497,24 @@  static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
 	case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
 		if (priv->p5_interface == state->interface)
 			break;
-		if (!phy_interface_mode_is_rgmii(state->interface) &&
-		    state->interface != PHY_INTERFACE_MODE_MII &&
-		    state->interface != PHY_INTERFACE_MODE_GMII)
-			return;
 
-		mt7530_setup_port5(ds, state->interface);
+		if (mt753x_mac_setup(ds, port, mode, state) < 0)
+			goto unsupported;
+
 		break;
 	case 6: /* 1st cpu port */
 		if (priv->p6_interface == state->interface)
 			break;
 
-		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
-		    state->interface != PHY_INTERFACE_MODE_TRGMII)
-			return;
-
-		/* Setup TX circuit incluing relevant PAD and driving */
-		mt7530_pad_clk_setup(ds, state->interface);
+		mt753x_pad_setup(ds, state);
 
-		if (priv->id == ID_MT7530) {
-			/* Setup RX circuit, relevant PAD and driving on the
-			 * host which must be placed after the setup on the
-			 * device side is all finished.
-			 */
-			mt7623_pad_clk_setup(ds);
-		}
+		if (mt753x_mac_setup(ds, port, mode, state) < 0)
+			goto unsupported;
 
 		priv->p6_interface = state->interface;
 		break;
 	default:
+unsupported:
 		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
 		return;
 	}
@@ -1488,38 +1575,14 @@  static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port,
 	mt7530_port_set_status(priv, port, 1);
 }
 
-static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
+static void mt753x_phylink_validate(struct dsa_switch *ds, int port,
 				    unsigned long *supported,
 				    struct phylink_link_state *state)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
 
-	switch (port) {
-	case 0: /* Internal phy */
-	case 1:
-	case 2:
-	case 3:
-	case 4:
-		if (state->interface != PHY_INTERFACE_MODE_NA &&
-		    state->interface != PHY_INTERFACE_MODE_GMII)
-			goto unsupported;
-		break;
-	case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
-		if (state->interface != PHY_INTERFACE_MODE_NA &&
-		    !phy_interface_mode_is_rgmii(state->interface) &&
-		    state->interface != PHY_INTERFACE_MODE_MII &&
-		    state->interface != PHY_INTERFACE_MODE_GMII)
-			goto unsupported;
-		break;
-	case 6: /* 1st cpu port */
-		if (state->interface != PHY_INTERFACE_MODE_NA &&
-		    state->interface != PHY_INTERFACE_MODE_RGMII &&
-		    state->interface != PHY_INTERFACE_MODE_TRGMII)
-			goto unsupported;
-		break;
-	default:
-		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
-unsupported:
+	if (state->interface != PHY_INTERFACE_MODE_NA &&
+	    !mt753x_phy_supported(ds, port, state)) {
 		linkmode_zero(supported);
 		return;
 	}
@@ -1590,12 +1653,36 @@  mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port,
 	return 1;
 }
 
+static int
+mt753x_setup(struct dsa_switch *ds)
+{
+	struct mt7530_priv *priv = ds->priv;
+
+	return priv->info->setup(ds);
+}
+
+static int
+mt753x_phy_read(struct dsa_switch *ds, int port, int regnum)
+{
+	struct mt7530_priv *priv = ds->priv;
+
+	return priv->info->phy_read(ds, port, regnum);
+}
+
+static int
+mt753x_phy_write(struct dsa_switch *ds, int port, int regnum, u16 val)
+{
+	struct mt7530_priv *priv = ds->priv;
+
+	return priv->info->phy_write(ds, port, regnum, val);
+}
+
 static const struct dsa_switch_ops mt7530_switch_ops = {
 	.get_tag_protocol	= mtk_get_tag_protocol,
-	.setup			= mt7530_setup,
+	.setup			= mt753x_setup,
 	.get_strings		= mt7530_get_strings,
-	.phy_read		= mt7530_phy_read,
-	.phy_write		= mt7530_phy_write,
+	.phy_read		= mt753x_phy_read,
+	.phy_write		= mt753x_phy_write,
 	.get_ethtool_stats	= mt7530_get_ethtool_stats,
 	.get_sset_count		= mt7530_get_sset_count,
 	.port_enable		= mt7530_port_enable,
@@ -1610,16 +1697,37 @@  static const struct dsa_switch_ops mt7530_switch_ops = {
 	.port_vlan_prepare	= mt7530_port_vlan_prepare,
 	.port_vlan_add		= mt7530_port_vlan_add,
 	.port_vlan_del		= mt7530_port_vlan_del,
-	.phylink_validate	= mt7530_phylink_validate,
+	.phylink_validate	= mt753x_phylink_validate,
 	.phylink_mac_link_state = mt7530_phylink_mac_link_state,
-	.phylink_mac_config	= mt7530_phylink_mac_config,
+	.phylink_mac_config	= mt753x_phylink_mac_config,
 	.phylink_mac_link_down	= mt7530_phylink_mac_link_down,
 	.phylink_mac_link_up	= mt7530_phylink_mac_link_up,
 };
 
+static const struct mt753x_info mt753x_table[] = {
+	[ID_MT7621] = {
+		.id = ID_MT7621,
+		.setup = mt7530_setup,
+		.phy_read = mt7530_phy_read,
+		.phy_write = mt7530_phy_write,
+		.phy_supported = mt7530_phy_supported,
+		.pad_setup = mt7530_pad_setup,
+		.mac_setup = mt7530_mac_setup,
+	},
+	[ID_MT7530] = {
+		.id = ID_MT7530,
+		.setup = mt7530_setup,
+		.phy_read = mt7530_phy_read,
+		.phy_write = mt7530_phy_write,
+		.phy_supported = mt7530_phy_supported,
+		.pad_setup = mt7530_pad_setup,
+		.mac_setup = mt7530_mac_setup,
+	},
+};
+
 static const struct of_device_id mt7530_of_match[] = {
-	{ .compatible = "mediatek,mt7621", .data = (void *)ID_MT7621, },
-	{ .compatible = "mediatek,mt7530", .data = (void *)ID_MT7530, },
+	{ .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], },
+	{ .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, mt7530_of_match);
@@ -1660,8 +1768,19 @@  mt7530_probe(struct mdio_device *mdiodev)
 	/* Get the hardware identifier from the devicetree node.
 	 * We will need it for some of the clock and regulator setup.
 	 */
-	priv->id = (unsigned int)(unsigned long)
-		of_device_get_match_data(&mdiodev->dev);
+	priv->info = of_device_get_match_data(&mdiodev->dev);
+	if (!priv->info)
+		return -EINVAL;
+
+	/* Sanity check if these required device operstaions are filled
+	 * properly.
+	 */
+	if (!priv->info->setup || !priv->info->phy_read ||
+	    !priv->info->phy_write || !priv->info->phy_supported ||
+	    !priv->info->pad_setup || !priv->info->mac_setup)
+		return -EINVAL;
+
+	priv->id = priv->info->id;
 
 	if (priv->id == ID_MT7530) {
 		priv->core_pwr = devm_regulator_get(&mdiodev->dev, "core");
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index ccb9da8cad0d..aac86e4fc148 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -11,7 +11,7 @@ 
 #define MT7530_NUM_FDB_RECORDS		2048
 #define MT7530_ALL_MEMBERS		0xff
 
-enum {
+enum mt753x_id {
 	ID_MT7530 = 0,
 	ID_MT7621 = 1,
 };
@@ -428,6 +428,32 @@  static const char *p5_intf_modes(unsigned int p5_interface)
 	}
 }
 
+/* struct mt753x_info -	This is the main data structure for holding the specific
+ *			part for each supported device
+ * @setup:		Holding the handler to a device initialization
+ * @phy_read:		Holding the way reading PHY port
+ * @phy_write:		Holding the way writing PHY port
+ * @phy_supported:	Check if the PHY type is being supported on a certain
+ *			port
+ * @pad_setup:		Holding the way setting up the bus pad for a certain MAC
+ *			port
+ * @mac_setup:		Holding the way setting up the PHY attribute for a
+ *			certain MAC port
+ */
+struct mt753x_info {
+	enum mt753x_id id;
+
+	int (*setup)(struct dsa_switch *ds);
+	int (*phy_read)(struct dsa_switch *ds, int port, int regnum);
+	int (*phy_write)(struct dsa_switch *ds, int port, int regnum, u16 val);
+	bool (*phy_supported)(struct dsa_switch *ds, int port,
+			      const struct phylink_link_state *state);
+	int (*pad_setup)(struct dsa_switch *ds,
+			 const struct phylink_link_state *state);
+	int (*mac_setup)(struct dsa_switch *ds, int port, unsigned int mode,
+			 const struct phylink_link_state *state);
+};
+
 /* struct mt7530_priv -	This is the main data structure for holding the state
  *			of the driver
  * @dev:		The device pointer
@@ -455,6 +481,7 @@  struct mt7530_priv {
 	struct regulator	*core_pwr;
 	struct regulator	*io_pwr;
 	struct gpio_desc	*reset;
+	const struct mt753x_info *info;
 	unsigned int		id;
 	bool			mcm;
 	phy_interface_t		p6_interface;