[RFC net-next 0/4] MT7530 interrupt support

[RFC net-next 0/4] MT7530 interrupt support

[DENG Qingfang]

Add support for MT7530 interrupt controller.

DENG Qingfang (4):
  net: phy: add MediaTek PHY driver
  net: dsa: mt7530: add interrupt support
  dt-bindings: net: dsa: add MT7530 interrupt controller binding
  staging: mt7621-dts: enable MT7530 interrupt controller

 .../devicetree/bindings/net/dsa/mt7530.txt    |   5 +
 drivers/net/dsa/mt7530.c                      | 203 ++++++++++++++++--
 drivers/net/dsa/mt7530.h                      |  18 +-
 drivers/net/phy/Kconfig                       |   5 +
 drivers/net/phy/Makefile                      |   1 +
 drivers/net/phy/mediatek.c                    | 109 ++++++++++
 drivers/staging/mt7621-dts/mt7621.dtsi        |   3 +
 7 files changed, 323 insertions(+), 21 deletions(-)
 create mode 100644 drivers/net/phy/mediatek.c

-- 
2.25.1

[RFC net-next 1/4] net: phy: add MediaTek PHY driver

[DENG Qingfang]

Add support for MediaTek PHYs found in MT7530 and MT7531 switches.
The initialization procedure is from the vendor driver, but due to lack
of documentation, the function of some register values remains unknown.

Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---
 drivers/net/phy/Kconfig    |   5 ++
 drivers/net/phy/Makefile   |   1 +
 drivers/net/phy/mediatek.c | 109 +++++++++++++++++++++++++++++++++++++
 3 files changed, 115 insertions(+)
 create mode 100644 drivers/net/phy/mediatek.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index a615b3660b05..edd858cec9ec 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -207,6 +207,11 @@ config MARVELL_88X2222_PHY
 	  Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet
 	  Transceiver.
 
+config MEDIATEK_PHY
+	tristate "MediaTek PHYs"
+	help
+	  Supports the MediaTek switch integrated PHYs.
+
 config MICREL_PHY
 	tristate "Micrel PHYs"
 	help
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index de683e3abe63..9ed7dbab7770 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -64,6 +64,7 @@ obj-$(CONFIG_LXT_PHY)		+= lxt.o
 obj-$(CONFIG_MARVELL_10G_PHY)	+= marvell10g.o
 obj-$(CONFIG_MARVELL_PHY)	+= marvell.o
 obj-$(CONFIG_MARVELL_88X2222_PHY)	+= marvell-88x2222.o
+obj-$(CONFIG_MEDIATEK_PHY)	+= mediatek.o
 obj-$(CONFIG_MESON_GXL_PHY)	+= meson-gxl.o
 obj-$(CONFIG_MICREL_KS8995MA)	+= spi_ks8995.o
 obj-$(CONFIG_MICREL_PHY)	+= micrel.o
diff --git a/drivers/net/phy/mediatek.c b/drivers/net/phy/mediatek.c
new file mode 100644
index 000000000000..18fefd79a9bd
--- /dev/null
+++ b/drivers/net/phy/mediatek.c
@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: GPL-2.0+
+#include <linux/module.h>
+#include <linux/phy.h>
+#include <linux/version.h>
+
+#define MTK_EXT_PAGE_ACCESS		0x1f
+#define MTK_PHY_PAGE_STANDARD		0x0000
+#define MTK_PHY_PAGE_EXTENDED		0x0001
+#define MTK_PHY_PAGE_EXTENDED_2		0x0002
+#define MTK_PHY_PAGE_EXTENDED_3		0x0003
+#define MTK_PHY_PAGE_EXTENDED_2A30	0x2a30
+#define MTK_PHY_PAGE_EXTENDED_52B5	0x52b5
+
+static int mtk_phy_read_page(struct phy_device *phydev)
+{
+	return __phy_read(phydev, MTK_EXT_PAGE_ACCESS);
+}
+
+static int mtk_phy_write_page(struct phy_device *phydev, int page)
+{
+	return __phy_write(phydev, MTK_EXT_PAGE_ACCESS, page);
+}
+
+static void mtk_phy_config_init(struct phy_device *phydev)
+{
+	/* Disable EEE */
+	phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, 0);
+
+	/* Enable HW auto downshift */
+	phy_modify_paged(phydev, MTK_PHY_PAGE_EXTENDED, 0x14, 0, BIT(4));
+
+	/* Increase SlvDPSready time */
+	phy_select_page(phydev, MTK_PHY_PAGE_EXTENDED_52B5);
+	__phy_write(phydev, 0x10, 0xafae);
+	__phy_write(phydev, 0x12, 0x2f);
+	__phy_write(phydev, 0x10, 0x8fae);
+	phy_restore_page(phydev, MTK_PHY_PAGE_STANDARD, 0);
+
+	/* Adjust 100_mse_threshold */
+	phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x123, 0xffff);
+
+	/* Disable mcc */
+	phy_write_mmd(phydev, MDIO_MMD_VEND1, 0xa6, 0x300);
+}
+
+static int mt7530_phy_config_init(struct phy_device *phydev)
+{
+	mtk_phy_config_init(phydev);
+
+	/* Increase post_update_timer */
+	phy_write_paged(phydev, MTK_PHY_PAGE_EXTENDED_3, 0x11, 0x4b);
+
+	return 0;
+}
+
+static int mt7531_phy_config_init(struct phy_device *phydev)
+{
+	mtk_phy_config_init(phydev);
+
+	/* PHY link down power saving enable */
+	phy_set_bits(phydev, 0x17, BIT(4));
+	phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, 0xc6, 0x300);
+
+	/* Set TX Pair delay selection */
+	phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x13, 0x404);
+	phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x14, 0x404);
+
+	return 0;
+}
+
+static struct phy_driver mtk_phy_driver[] = {
+	{
+		PHY_ID_MATCH_EXACT(0x03a29412),
+		.name		= "MediaTek MT7530 PHY",
+		.config_init	= mt7530_phy_config_init,
+		/* Interrupts are handled by the switch, not the PHY
+		 * itself.
+		 */
+		.config_intr	= genphy_no_config_intr,
+		.handle_interrupt = genphy_handle_interrupt_no_ack,
+		.read_page	= mtk_phy_read_page,
+		.write_page	= mtk_phy_write_page,
+	},
+	{
+		PHY_ID_MATCH_EXACT(0x03a29441),
+		.name		= "MediaTek MT7531 PHY",
+		.config_init	= mt7531_phy_config_init,
+		/* Interrupts are handled by the switch, not the PHY
+		 * itself.
+		 */
+		.config_intr	= genphy_no_config_intr,
+		.handle_interrupt = genphy_handle_interrupt_no_ack,
+		.read_page	= mtk_phy_read_page,
+		.write_page	= mtk_phy_write_page,
+	},
+};
+
+module_phy_driver(mtk_phy_driver);
+
+static struct mdio_device_id __maybe_unused mtk_phy_tbl[] = {
+	{ PHY_ID_MATCH_VENDOR(0x03a29400) },
+	{ }
+};
+
+MODULE_DESCRIPTION("MediaTek switch integrated PHY driver");
+MODULE_AUTHOR("DENG, Qingfang <dqfext@gmail.com>");
+MODULE_LICENSE("GPL");
+
+MODULE_DEVICE_TABLE(mdio, mtk_phy_tbl);
-- 
2.25.1

[RFC net-next 2/4] net: dsa: mt7530: add interrupt support

[DENG Qingfang]

Add support for MT7530 interrupt controller to handle internal PHYs.
In order to assign an IRQ number to each PHY, the registration of MDIO bus
is also done in this driver.

Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---
 drivers/net/dsa/mt7530.c | 203 +++++++++++++++++++++++++++++++++++----
 drivers/net/dsa/mt7530.h |  18 +++-
 2 files changed, 200 insertions(+), 21 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 2bd1bab71497..4c334e90090b 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -10,6 +10,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
+#include <linux/of_irq.h>
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
 #include <linux/of_platform.h>
@@ -596,18 +597,14 @@ mt7530_mib_reset(struct dsa_switch *ds)
 	mt7530_write(priv, MT7530_MIB_CCR, CCR_MIB_ACTIVATE);
 }
 
-static int mt7530_phy_read(struct dsa_switch *ds, int port, int regnum)
+static int mt7530_phy_read(struct mt7530_priv *priv, int port, int regnum)
 {
-	struct mt7530_priv *priv = ds->priv;
-
 	return mdiobus_read_nested(priv->bus, port, regnum);
 }
 
-static int mt7530_phy_write(struct dsa_switch *ds, int port, int regnum,
+static int mt7530_phy_write(struct mt7530_priv *priv, int port, int regnum,
 			    u16 val)
 {
-	struct mt7530_priv *priv = ds->priv;
-
 	return mdiobus_write_nested(priv->bus, port, regnum, val);
 }
 
@@ -785,9 +782,8 @@ mt7531_ind_c22_phy_write(struct mt7530_priv *priv, int port, int regnum,
 }
 
 static int
-mt7531_ind_phy_read(struct dsa_switch *ds, int port, int regnum)
+mt7531_ind_phy_read(struct mt7530_priv *priv, int port, int regnum)
 {
-	struct mt7530_priv *priv = ds->priv;
 	int devad;
 	int ret;
 
@@ -803,10 +799,9 @@ mt7531_ind_phy_read(struct dsa_switch *ds, int port, int regnum)
 }
 
 static int
-mt7531_ind_phy_write(struct dsa_switch *ds, int port, int regnum,
+mt7531_ind_phy_write(struct mt7530_priv *priv, int port, int regnum,
 		     u16 data)
 {
-	struct mt7530_priv *priv = ds->priv;
 	int devad;
 	int ret;
 
@@ -1828,6 +1823,159 @@ mt7530_setup_gpio(struct mt7530_priv *priv)
 }
 #endif /* CONFIG_GPIOLIB */
 
+static irqreturn_t
+mt7530_irq(int irq, void *data)
+{
+	struct mt7530_priv *priv = data;
+	bool handled = false;
+	u32 val;
+	int p;
+
+	mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);
+	val = mt7530_mii_read(priv, MT7530_SYS_INT_STS);
+	mt7530_mii_write(priv, MT7530_SYS_INT_STS, val);
+	mutex_unlock(&priv->bus->mdio_lock);
+
+	for (p = 0; p < MT7530_NUM_PHYS; p++) {
+		if (BIT(p) & val) {
+			handle_nested_irq(irq_find_mapping(priv->irq_domain,
+					  p));
+			handled = true;
+		}
+	}
+
+	return IRQ_RETVAL(handled);
+}
+
+static void
+mt7530_irq_mask(struct irq_data *d)
+{
+	struct mt7530_priv *priv = irq_data_get_irq_chip_data(d);
+
+	priv->irq_enable &= ~BIT(d->hwirq);
+}
+
+static void
+mt7530_irq_unmask(struct irq_data *d)
+{
+	struct mt7530_priv *priv = irq_data_get_irq_chip_data(d);
+
+	priv->irq_enable |= BIT(d->hwirq);
+}
+
+static void
+mt7530_irq_bus_lock(struct irq_data *d)
+{
+	struct mt7530_priv *priv = irq_data_get_irq_chip_data(d);
+
+	mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);
+}
+
+static void
+mt7530_irq_bus_sync_unlock(struct irq_data *d)
+{
+	struct mt7530_priv *priv = irq_data_get_irq_chip_data(d);
+
+	mt7530_mii_write(priv, MT7530_SYS_INT_EN, priv->irq_enable);
+	mutex_unlock(&priv->bus->mdio_lock);
+}
+
+static struct irq_chip mt7530_irq_chip = {
+	.name = KBUILD_MODNAME,
+	.irq_mask = mt7530_irq_mask,
+	.irq_unmask = mt7530_irq_unmask,
+	.irq_bus_lock = mt7530_irq_bus_lock,
+	.irq_bus_sync_unlock = mt7530_irq_bus_sync_unlock,
+};
+
+static int
+mt7530_irq_map(struct irq_domain *domain, unsigned int irq,
+	       irq_hw_number_t hwirq)
+{
+	irq_set_chip_data(irq, domain->host_data);
+	irq_set_chip_and_handler(irq, &mt7530_irq_chip, handle_simple_irq);
+	irq_set_noprobe(irq);
+
+	return 0;
+}
+
+static const struct irq_domain_ops mt7530_irq_domain_ops = {
+	.map = mt7530_irq_map,
+	.xlate = irq_domain_xlate_onecell,
+};
+
+static int
+mt7530_setup_mdiobus_irq(struct mt7530_priv *priv)
+{
+	struct device *dev = priv->dev;
+	struct device_node *np = dev->of_node;
+	struct dsa_switch *ds = priv->ds;
+	int parent_irq, ret, p;
+	struct mii_bus *bus;
+	static int idx;
+
+	bus = devm_mdiobus_alloc(dev);
+	if (!bus)
+		return -ENOMEM;
+
+	ds->slave_mii_bus = bus;
+	bus->priv = priv;
+	bus->name = KBUILD_MODNAME "-mii";
+	snprintf(bus->id, MII_BUS_ID_SIZE, KBUILD_MODNAME "-%d", idx++);
+	bus->read = mt753x_phy_read;
+	bus->write = mt753x_phy_write;
+	bus->parent = dev;
+	bus->phy_mask = ~ds->phys_mii_mask;
+
+	if (!of_property_read_bool(np, "interrupt-controller")) {
+		dev_info(dev, "no interrupt support\n");
+		goto register_mdiobus;
+	}
+
+	parent_irq = of_irq_get(np, 0);
+	if (parent_irq <= 0) {
+		dev_err(dev, "failed to get parent IRQ: %d\n", parent_irq);
+		return parent_irq ? : -EINVAL;
+	}
+
+	priv->irq_domain = irq_domain_add_linear(np, MT7530_NUM_PHYS,
+						&mt7530_irq_domain_ops, priv);
+	if (!priv->irq_domain) {
+		dev_err(dev, "failed to create IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	/* This register must be set for MT7530 to properly fire interrupts */
+	if (priv->id != ID_MT7531)
+		mt7530_set(priv, MT7530_TOP_SIG_CTRL, TOP_SIG_CTRL_NORMAL);
+
+	ret = devm_request_threaded_irq(dev, parent_irq, NULL, mt7530_irq,
+					IRQF_ONESHOT, KBUILD_MODNAME, priv);
+	if (ret) {
+		dev_err(dev, "failed to request IRQ: %d\n", ret);
+		return ret;
+	}
+
+	for (p = 0; p < MT7530_NUM_PHYS; p++) {
+		if (BIT(p) & ds->phys_mii_mask) {
+			unsigned int irq;
+
+			irq = irq_create_mapping(priv->irq_domain, p);
+			irq_set_parent(irq, parent_irq);
+			bus->irq[p] = irq;
+		}
+	}
+
+register_mdiobus:
+	ret = mdiobus_register(bus);
+	if (ret) {
+		dev_err(dev, "failed to register MDIO bus: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 static int
 mt7530_setup(struct dsa_switch *ds)
 {
@@ -2780,32 +2928,34 @@ static int
 mt753x_setup(struct dsa_switch *ds)
 {
 	struct mt7530_priv *priv = ds->priv;
+	int ret = priv->info->sw_setup(ds);
+
+	if (!ret)
+		ret = mt7530_setup_mdiobus_irq(priv);
 
-	return priv->info->sw_setup(ds);
+	return ret;
 }
 
 static int
-mt753x_phy_read(struct dsa_switch *ds, int port, int regnum)
+mt753x_phy_read(struct mii_bus *bus, int port, int regnum)
 {
-	struct mt7530_priv *priv = ds->priv;
+	struct mt7530_priv *priv = bus->priv;
 
-	return priv->info->phy_read(ds, port, regnum);
+	return priv->info->phy_read(priv, port, regnum);
 }
 
 static int
-mt753x_phy_write(struct dsa_switch *ds, int port, int regnum, u16 val)
+mt753x_phy_write(struct mii_bus *bus, int port, int regnum, u16 val)
 {
-	struct mt7530_priv *priv = ds->priv;
+	struct mt7530_priv *priv = bus->priv;
 
-	return priv->info->phy_write(ds, port, regnum, val);
+	return priv->info->phy_write(priv, port, regnum, val);
 }
 
 static const struct dsa_switch_ops mt7530_switch_ops = {
 	.get_tag_protocol	= mtk_get_tag_protocol,
 	.setup			= mt753x_setup,
 	.get_strings		= mt7530_get_strings,
-	.phy_read		= mt753x_phy_read,
-	.phy_write		= mt753x_phy_write,
 	.get_ethtool_stats	= mt7530_get_ethtool_stats,
 	.get_sset_count		= mt7530_get_sset_count,
 	.set_ageing_time	= mt7530_set_ageing_time,
@@ -2986,6 +3136,21 @@ mt7530_remove(struct mdio_device *mdiodev)
 		dev_err(priv->dev, "Failed to disable io pwr: %d\n",
 			ret);
 
+	if (priv->irq_domain) {
+		int p;
+
+		for (p = 0; p < MT7530_NUM_PHYS; p++) {
+			if (BIT(p) & priv->ds->phys_mii_mask) {
+				unsigned int irq;
+
+				irq = irq_find_mapping(priv->irq_domain, p);
+				irq_dispose_mapping(irq);
+			}
+		}
+
+		irq_domain_remove(priv->irq_domain);
+	}
+
 	dsa_unregister_switch(priv->ds);
 	mutex_destroy(&priv->reg_mutex);
 }
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index ec36ea5dfd57..e4429e152cde 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -7,6 +7,7 @@
 #define __MT7530_H
 
 #define MT7530_NUM_PORTS		7
+#define MT7530_NUM_PHYS			5
 #define MT7530_CPU_PORT			6
 #define MT7530_NUM_FDB_RECORDS		2048
 #define MT7530_ALL_MEMBERS		0xff
@@ -381,6 +382,12 @@ enum mt7531_sgmii_force_duplex {
 #define  SYS_CTRL_SW_RST		BIT(1)
 #define  SYS_CTRL_REG_RST		BIT(0)
 
+/* Register for system interrupt */
+#define MT7530_SYS_INT_EN		0x7008
+
+/* Register for system interrupt status */
+#define MT7530_SYS_INT_STS		0x700c
+
 /* Register for PHY Indirect Access Control */
 #define MT7531_PHY_IAC			0x701C
 #define  MT7531_PHY_ACS_ST		BIT(31)
@@ -702,6 +709,11 @@ static const char *p5_intf_modes(unsigned int p5_interface)
 	}
 }
 
+/* Forward declaration */
+struct mt7530_priv;
+static int mt753x_phy_read(struct mii_bus *, int, int);
+static int mt753x_phy_write(struct mii_bus *, int, int, u16);
+
 /* struct mt753x_info -	This is the main data structure for holding the specific
  *			part for each supported device
  * @sw_setup:		Holding the handler to a device initialization
@@ -726,8 +738,8 @@ struct mt753x_info {
 	enum mt753x_id id;
 
 	int (*sw_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);
+	int (*phy_read)(struct mt7530_priv *priv, int port, int regnum);
+	int (*phy_write)(struct mt7530_priv *priv, int port, int regnum, u16 val);
 	int (*pad_setup)(struct dsa_switch *ds, phy_interface_t interface);
 	int (*cpu_port_config)(struct dsa_switch *ds, int port);
 	bool (*phy_mode_supported)(struct dsa_switch *ds, int port,
@@ -782,6 +794,8 @@ struct mt7530_priv {
 	struct mt7530_port	ports[MT7530_NUM_PORTS];
 	/* protect among processes for registers access*/
 	struct mutex reg_mutex;
+	struct irq_domain *irq_domain;
+	u32 irq_enable;
 };
 
 struct mt7530_hw_vlan_entry {
-- 
2.25.1

[RFC net-next 3/4] dt-bindings: net: dsa: add MT7530 interrupt controller binding

[DENG Qingfang]

Add device tree binding to support MT7530 interrupt controller.

Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---
 Documentation/devicetree/bindings/net/dsa/mt7530.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/mt7530.txt b/Documentation/devicetree/bindings/net/dsa/mt7530.txt
index de04626a8e9d..26b34888eb62 100644
--- a/Documentation/devicetree/bindings/net/dsa/mt7530.txt
+++ b/Documentation/devicetree/bindings/net/dsa/mt7530.txt
@@ -81,6 +81,11 @@ Optional properties:
 - gpio-controller: Boolean; if defined, MT7530's LED controller will run on
 	GPIO mode.
 - #gpio-cells: Must be 2 if gpio-controller is defined.
+- interrupt-controller: Boolean; Enables the internal interrupt controller.
+
+If interrupt-controller is defined, the following property is required.
+
+- interrupts: Parent interrupt for the interrupt controller.
 
 See Documentation/devicetree/bindings/net/dsa/dsa.txt for a list of additional
 required, optional properties and how the integrated switch subnodes must
-- 
2.25.1

[RFC net-next 4/4] staging: mt7621-dts: enable MT7530 interrupt controller

[DENG Qingfang]

Enable MT7530 interrupt controller in the MT7621 SoC.

Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---
 drivers/staging/mt7621-dts/mt7621.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index 16fc94f65486..ebf8b0633e88 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -447,6 +447,9 @@ switch0: switch0@0 {
 				mediatek,mcm;
 				resets = <&rstctrl 2>;
 				reset-names = "mcm";
+				interrupt-controller;
+				interrupt-parent = <&gic>;
+				interrupts = <GIC_SHARED 23 IRQ_TYPE_LEVEL_HIGH>;
 
 				ports {
 					#address-cells = <1>;
-- 
2.25.1

Re: [RFC net-next 1/4] net: phy: add MediaTek PHY driver

[Andrew Lunn]

On Tue, Apr 06, 2021 at 10:18:16PM +0800, DENG Qingfang wrote:
> Add support for MediaTek PHYs found in MT7530 and MT7531 switches.

Do you know if this PHY is available standalone?

> +static int mt7531_phy_config_init(struct phy_device *phydev)
> +{
> +	mtk_phy_config_init(phydev);
> +
> +	/* PHY link down power saving enable */
> +	phy_set_bits(phydev, 0x17, BIT(4));
> +	phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, 0xc6, 0x300);
> +
> +	/* Set TX Pair delay selection */
> +	phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x13, 0x404);
> +	phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x14, 0x404);

This gets me worried about RGMII delays. We have had bad backwards
compatibility problems with PHY drivers which get RGMII delays wrong.

Since this is an internal PHY, i suggest you add a test to the
beginning of mt7531_phy_config_init():

	if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
		return -EINVAL;

We can then solve RGMII problems when somebody actually needs RGMII.

   Andrew

Re: [RFC net-next 2/4] net: dsa: mt7530: add interrupt support

[Andrew Lunn]

On Tue, Apr 06, 2021 at 10:18:17PM +0800, DENG Qingfang wrote:
> Add support for MT7530 interrupt controller to handle internal PHYs.

Are the interrupts purely PHY interrupts? Or are there some switch
operation interrupts, which are currently not used?

I'm just wondering if it is correct to so closely tie interrupts and
MDIO together.

     Andrew

Re: [RFC net-next 1/4] net: phy: add MediaTek PHY driver

[DENG Qingfang]

On Tue, Apr 6, 2021 at 11:21 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Apr 06, 2021 at 10:18:16PM +0800, DENG Qingfang wrote:
> > Add support for MediaTek PHYs found in MT7530 and MT7531 switches.
>
> Do you know if this PHY is available standalone?

Not that I know of.

>
> > +static int mt7531_phy_config_init(struct phy_device *phydev)
> > +{
> > +     mtk_phy_config_init(phydev);
> > +
> > +     /* PHY link down power saving enable */
> > +     phy_set_bits(phydev, 0x17, BIT(4));
> > +     phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, 0xc6, 0x300);
> > +
> > +     /* Set TX Pair delay selection */
> > +     phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x13, 0x404);
> > +     phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x14, 0x404);
>
> This gets me worried about RGMII delays. We have had bad backwards
> compatibility problems with PHY drivers which get RGMII delays wrong.
>
> Since this is an internal PHY, i suggest you add a test to the
> beginning of mt7531_phy_config_init():
>
>         if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
>                 return -EINVAL;

Okay. Will add it to v2.

>
> We can then solve RGMII problems when somebody actually needs RGMII.
>
>    Andrew

Re: [RFC net-next 2/4] net: dsa: mt7530: add interrupt support

[DENG Qingfang]

On Tue, Apr 6, 2021 at 11:30 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Apr 06, 2021 at 10:18:17PM +0800, DENG Qingfang wrote:
> > Add support for MT7530 interrupt controller to handle internal PHYs.
>
> Are the interrupts purely PHY interrupts? Or are there some switch
> operation interrupts, which are currently not used?

There are other switch operations interrupts as well, such as VLAN
member violation, switch ACL hit.

>
> I'm just wondering if it is correct to so closely tie interrupts and
> MDIO together.
>
>      Andrew

Re: [RFC net-next 1/4] net: phy: add MediaTek PHY driver

[Chun-Kuang Hu]

Hi, Qingfang:

DENG Qingfang <dqfext@gmail.com> 於 2021年4月6日 週二 下午10:19寫道：
>
> Add support for MediaTek PHYs found in MT7530 and MT7531 switches.
> The initialization procedure is from the vendor driver, but due to lack
> of documentation, the function of some register values remains unknown.
>
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> ---
>  drivers/net/phy/Kconfig    |   5 ++
>  drivers/net/phy/Makefile   |   1 +
>  drivers/net/phy/mediatek.c | 109 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 115 insertions(+)
>  create mode 100644 drivers/net/phy/mediatek.c
>
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index a615b3660b05..edd858cec9ec 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -207,6 +207,11 @@ config MARVELL_88X2222_PHY
>           Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet
>           Transceiver.
>
> +config MEDIATEK_PHY

There are many Mediatek phy drivers in [1], so use a specific name.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/mediatek?h=v5.12-rc6

Regards,
Chun-Kuang.

> +       tristate "MediaTek PHYs"
> +       help
> +         Supports the MediaTek switch integrated PHYs.
> +
>  config MICREL_PHY
>         tristate "Micrel PHYs"
>         help

Re: [RFC net-next 2/4] net: dsa: mt7530: add interrupt support

[Andrew Lunn]

On Tue, Apr 06, 2021 at 11:39:12PM +0800, DENG Qingfang wrote:
> On Tue, Apr 6, 2021 at 11:30 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Tue, Apr 06, 2021 at 10:18:17PM +0800, DENG Qingfang wrote:
> > > Add support for MT7530 interrupt controller to handle internal PHYs.
> >
> > Are the interrupts purely PHY interrupts? Or are there some switch
> > operation interrupts, which are currently not used?
> 
> There are other switch operations interrupts as well, such as VLAN
> member violation, switch ACL hit.

O.K. So that makes it similar to the mv88e6xxx. With that driver, i
kept interrupt setup and mdio setup separate. I add the interrupt
controller first, and then do mdio setup, calling a helper to map the
PHY interrupts and assign them to bus->irq[].

That gives you a cleaner structure when you start using the other
interrupts.

	Andrew

Re: [RFC net-next 1/4] net: phy: add MediaTek PHY driver

[DENG Qingfang]

On Tue, Apr 6, 2021 at 11:47 PM Chun-Kuang Hu <chunkuang.hu@kernel.org> wrote:
>
> Hi, Qingfang:
>
> DENG Qingfang <dqfext@gmail.com> 於 2021年4月6日 週二 下午10:19寫道：
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -207,6 +207,11 @@ config MARVELL_88X2222_PHY
> >           Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet
> >           Transceiver.
> >
> > +config MEDIATEK_PHY
>
> There are many Mediatek phy drivers in [1], so use a specific name.

So "MEDIATEK_MT7530_PHY" should be okay?

>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/mediatek?h=v5.12-rc6
>
> Regards,
> Chun-Kuang.

Re: [RFC net-next 2/4] net: dsa: mt7530: add interrupt support

[DENG Qingfang]

On Tue, Apr 6, 2021 at 11:49 PM Andrew Lunn <andrew@lunn.ch> wrote:
> O.K. So that makes it similar to the mv88e6xxx. With that driver, i
> kept interrupt setup and mdio setup separate. I add the interrupt
> controller first, and then do mdio setup, calling a helper to map the
> PHY interrupts and assign them to bus->irq[].
>
> That gives you a cleaner structure when you start using the other
> interrupts.

Okay. Will split the function in v2. Thanks.

>
>         Andrew

Re: [RFC net-next 1/4] net: phy: add MediaTek PHY driver

[Andrew Lunn]

On Tue, Apr 06, 2021 at 11:47:08PM +0800, Chun-Kuang Hu wrote:
> Hi, Qingfang:
> 
> DENG Qingfang <dqfext@gmail.com> 於 2021年4月6日 週二 下午10:19寫道：
> >
> > Add support for MediaTek PHYs found in MT7530 and MT7531 switches.
> > The initialization procedure is from the vendor driver, but due to lack
> > of documentation, the function of some register values remains unknown.
> >
> > Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> > ---
> >  drivers/net/phy/Kconfig    |   5 ++
> >  drivers/net/phy/Makefile   |   1 +
> >  drivers/net/phy/mediatek.c | 109 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 115 insertions(+)
> >  create mode 100644 drivers/net/phy/mediatek.c
> >
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index a615b3660b05..edd858cec9ec 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -207,6 +207,11 @@ config MARVELL_88X2222_PHY
> >           Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet
> >           Transceiver.
> >
> > +config MEDIATEK_PHY
> 
> There are many Mediatek phy drivers in [1], so use a specific name.

Those are generic PHY drivers, where as this patch is add a PHY
driver. The naming used in this patch is consistent with other PHY
drivers. So i'm happy with this patch in this respect.

PHY drivers have been around a lot longer than generic PHY drivers. So
i would actually say the generic PHY driver naming should make it
clear they are generic PHYs, not PHYs.

But lets not bike shed about this too much.

      Andrew

Re: [RFC net-next 1/4] net: phy: add MediaTek PHY driver

[Andrew Lunn]

On Tue, Apr 06, 2021 at 11:57:14PM +0800, DENG Qingfang wrote:
> On Tue, Apr 6, 2021 at 11:47 PM Chun-Kuang Hu <chunkuang.hu@kernel.org> wrote:
> >
> > Hi, Qingfang:
> >
> > DENG Qingfang <dqfext@gmail.com> 於 2021年4月6日 週二 下午10:19寫道：
> > > --- a/drivers/net/phy/Kconfig
> > > +++ b/drivers/net/phy/Kconfig
> > > @@ -207,6 +207,11 @@ config MARVELL_88X2222_PHY
> > >           Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet
> > >           Transceiver.
> > >
> > > +config MEDIATEK_PHY
> >
> > There are many Mediatek phy drivers in [1], so use a specific name.
> 
> So "MEDIATEK_MT7530_PHY" should be okay?

No. MEDIATEK_PHY is consistent with other PHY drivers. Please leave it
as it is. And with time, we expect other devices will be supported by
this driver, so having MT7530 in the name would be confusing.

   Andrew

Re: [RFC net-next 1/4] net: phy: add MediaTek PHY driver

[Chun-Kuang Hu]

DENG Qingfang <dqfext@gmail.com> 於 2021年4月6日 週二 下午11:57寫道：
>
> On Tue, Apr 6, 2021 at 11:47 PM Chun-Kuang Hu <chunkuang.hu@kernel.org> wrote:
> >
> > Hi, Qingfang:
> >
> > DENG Qingfang <dqfext@gmail.com> 於 2021年4月6日 週二 下午10:19寫道：
> > > --- a/drivers/net/phy/Kconfig
> > > +++ b/drivers/net/phy/Kconfig
> > > @@ -207,6 +207,11 @@ config MARVELL_88X2222_PHY
> > >           Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet
> > >           Transceiver.
> > >
> > > +config MEDIATEK_PHY
> >
> > There are many Mediatek phy drivers in [1], so use a specific name.
>
> So "MEDIATEK_MT7530_PHY" should be okay?

This is ok, but this name looks only for one SoC.
MEDIATEK_ETHERNET_PHY could support more SoC, how do you think?

Regards,
Chun-Kuang.

>
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/mediatek?h=v5.12-rc6
> >
> > Regards,
> > Chun-Kuang.

Re: [RFC net-next 1/4] net: phy: add MediaTek PHY driver

[Chun-Kuang Hu]

Andrew Lunn <andrew@lunn.ch> 於 2021年4月7日 週三 上午12:02寫道：
>
> On Tue, Apr 06, 2021 at 11:47:08PM +0800, Chun-Kuang Hu wrote:
> > Hi, Qingfang:
> >
> > DENG Qingfang <dqfext@gmail.com> 於 2021年4月6日 週二 下午10:19寫道：
> > >
> > > Add support for MediaTek PHYs found in MT7530 and MT7531 switches.
> > > The initialization procedure is from the vendor driver, but due to lack
> > > of documentation, the function of some register values remains unknown.
> > >
> > > Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> > > ---
> > >  drivers/net/phy/Kconfig    |   5 ++
> > >  drivers/net/phy/Makefile   |   1 +
> > >  drivers/net/phy/mediatek.c | 109 +++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 115 insertions(+)
> > >  create mode 100644 drivers/net/phy/mediatek.c
> > >
> > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > > index a615b3660b05..edd858cec9ec 100644
> > > --- a/drivers/net/phy/Kconfig
> > > +++ b/drivers/net/phy/Kconfig
> > > @@ -207,6 +207,11 @@ config MARVELL_88X2222_PHY
> > >           Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet
> > >           Transceiver.
> > >
> > > +config MEDIATEK_PHY
> >
> > There are many Mediatek phy drivers in [1], so use a specific name.
>
> Those are generic PHY drivers, where as this patch is add a PHY
> driver. The naming used in this patch is consistent with other PHY
> drivers. So i'm happy with this patch in this respect.
>
> PHY drivers have been around a lot longer than generic PHY drivers. So
> i would actually say the generic PHY driver naming should make it
> clear they are generic PHYs, not PHYs.
>

OK, so just ignore my comment.

> But lets not bike shed about this too much.
>
>       Andrew