LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] net: phy: TLK10X initial driver submission
@ 2018-04-19  8:28 Måns Andersson
  2018-04-19 12:09 ` Andrew Lunn
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Måns Andersson @ 2018-04-19  8:28 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Andrew Lunn, Florian Fainelli, netdev,
	devicetree, linux-kernel
  Cc: Mans Andersson

From: Mans Andersson <mans.andersson@nibe.se>

Add suport for the TI TLK105 and TLK106 10/100Mbit ethernet phys.

In addition the TLK10X needs to be removed from DP83848 driver as the
power back off support is added here for this device.

Datasheet:
http://www.ti.com/lit/gpn/tlk106
---
 .../devicetree/bindings/net/ti,tlk10x.txt          |  27 +++
 drivers/net/phy/Kconfig                            |   5 +
 drivers/net/phy/Makefile                           |   1 +
 drivers/net/phy/dp83848.c                          |   3 -
 drivers/net/phy/tlk10x.c                           | 209 +++++++++++++++++++++
 5 files changed, 242 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/ti,tlk10x.txt
 create mode 100644 drivers/net/phy/tlk10x.c

diff --git a/Documentation/devicetree/bindings/net/ti,tlk10x.txt b/Documentation/devicetree/bindings/net/ti,tlk10x.txt
new file mode 100644
index 0000000..371d0d7
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ti,tlk10x.txt
@@ -0,0 +1,27 @@
+* Texas Instruments - TLK105 / TLK106 ethernet PHYs
+
+Required properties:
+	- reg - The ID number for the phy, usually a small integer
+
+Optional properties:
+	- ti,power-back-off - Power Back Off Level
+		Please refer to data sheet chapter 8.6 and TI Application
+		Note SLLA3228
+		0 - Normal Operation
+		1 - Level 1 (up to 140m cable between TLK link partners)
+		2 - Level 2 (up to 100m cable between TLK link partners)
+		3 - Level 3 (up to 80m cable between TLK link partners)
+
+Default child nodes are standard Ethernet PHY device
+nodes as described in Documentation/devicetree/bindings/net/phy.txt
+
+Example:
+
+	ethernet-phy@0 {
+		reg = <0>;
+		ti,power-back-off = <2>;
+	};
+
+Datasheets and documentation can be found at:
+http://www.ti.com/lit/gpn/tlk106
+http://www.ti.com/lit/an/slla328/slla328.pdf
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index bdfbabb..c980240 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -295,6 +295,11 @@ config DP83867_PHY
 	---help---
 	  Currently supports the DP83867 PHY.
 
+config TLK10X_PHY
+	tristate "Texas Instruments TLK10x PHY"
+	---help---
+	  Supports the TLK105 and TLK106 PHYs.
+
 config FIXED_PHY
 	tristate "MDIO Bus/PHY emulation with fixed speed/link PHYs"
 	depends on PHYLIB
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 01acbcb..37e4e02 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -79,5 +79,6 @@ obj-$(CONFIG_ROCKCHIP_PHY)	+= rockchip.o
 obj-$(CONFIG_SMSC_PHY)		+= smsc.o
 obj-$(CONFIG_STE10XP)		+= ste10Xp.o
 obj-$(CONFIG_TERANETICS_PHY)	+= teranetics.o
+obj-$(CONFIG_TLK10X_PHY)	+= tlk10x.o
 obj-$(CONFIG_VITESSE_PHY)	+= vitesse.o
 obj-$(CONFIG_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
diff --git a/drivers/net/phy/dp83848.c b/drivers/net/phy/dp83848.c
index cd09c3a..435f401 100644
--- a/drivers/net/phy/dp83848.c
+++ b/drivers/net/phy/dp83848.c
@@ -19,7 +19,6 @@
 #define TI_DP83848C_PHY_ID		0x20005ca0
 #define TI_DP83620_PHY_ID		0x20005ce0
 #define NS_DP83848C_PHY_ID		0x20005c90
-#define TLK10X_PHY_ID			0x2000a210
 
 /* Registers */
 #define DP83848_MICR			0x11 /* MII Interrupt Control Register */
@@ -78,7 +77,6 @@ static struct mdio_device_id __maybe_unused dp83848_tbl[] = {
 	{ TI_DP83848C_PHY_ID, 0xfffffff0 },
 	{ NS_DP83848C_PHY_ID, 0xfffffff0 },
 	{ TI_DP83620_PHY_ID, 0xfffffff0 },
-	{ TLK10X_PHY_ID, 0xfffffff0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(mdio, dp83848_tbl);
@@ -105,7 +103,6 @@ static struct phy_driver dp83848_driver[] = {
 	DP83848_PHY_DRIVER(TI_DP83848C_PHY_ID, "TI DP83848C 10/100 Mbps PHY"),
 	DP83848_PHY_DRIVER(NS_DP83848C_PHY_ID, "NS DP83848C 10/100 Mbps PHY"),
 	DP83848_PHY_DRIVER(TI_DP83620_PHY_ID, "TI DP83620 10/100 Mbps PHY"),
-	DP83848_PHY_DRIVER(TLK10X_PHY_ID, "TI TLK10X 10/100 Mbps PHY"),
 };
 module_phy_driver(dp83848_driver);
 
diff --git a/drivers/net/phy/tlk10x.c b/drivers/net/phy/tlk10x.c
new file mode 100644
index 0000000..1efc81e
--- /dev/null
+++ b/drivers/net/phy/tlk10x.c
@@ -0,0 +1,209 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * Driver for the Texas Instruments TLK105 / TLK106
+ *
+ * Copyright (C) 2018 NIBE Industrier AB - http://www.nibe.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/phy.h>
+#include <linux/of.h>
+
+#define TLK10X_PHY_ID			0x2000a210
+
+/* Registers */
+#define TLK10X_MICR			0x11 /* MII Interrupt Control Reg */
+#define TLK10X_MISR			0x12 /* MII Interrupt Status Reg */
+#define TLK10X_REGCR			0x0d /* Register Control Register */
+#define TLK10X_ADDAR			0x0e /* Data Register */
+#define TLK10X_PWRBOCR			0xae /* Power Backoff Register */
+
+/* MICR Register Fields */
+#define TLK10X_MICR_INT_OE		BIT(0) /* Interrupt Output Enable */
+#define TLK10X_MICR_INTEN		BIT(1) /* Interrupt Enable */
+
+/* MISR Register Fields */
+#define TLK10X_MISR_RHF_INT_EN		BIT(0) /* Receive Error Counter */
+#define TLK10X_MISR_FHF_INT_EN		BIT(1) /* False Carrier Counter */
+#define TLK10X_MISR_ANC_INT_EN		BIT(2) /* Auto-negotiation complete */
+#define TLK10X_MISR_DUP_INT_EN		BIT(3) /* Duplex Status */
+#define TLK10X_MISR_SPD_INT_EN		BIT(4) /* Speed status */
+#define TLK10X_MISR_LINK_INT_EN		BIT(5) /* Link status */
+#define TLK10X_MISR_ED_INT_EN		BIT(6) /* Energy detect */
+#define TLK10X_MISR_LQM_INT_EN		BIT(7) /* Link Quality Monitor */
+
+/* PWRBOCR Register Fields */
+#define TLK10X_PWRBOCR_MASK		0xe0 /* Power Backoff Mask */
+
+#define TLK10X_INT_EN_MASK		\
+	(TLK10X_MISR_ANC_INT_EN |	\
+	 TLK10X_MISR_DUP_INT_EN |	\
+	 TLK10X_MISR_SPD_INT_EN |	\
+	 TLK10X_MISR_LINK_INT_EN)
+
+struct tlk10x_private {
+	int pwrbo_level;
+};
+
+static int tlk10x_read(struct phy_device *phydev, int reg)
+{
+	if (reg & ~0x1f) {
+		/* Extended register */
+		phy_write(phydev, TLK10X_REGCR, 0x001F);
+		phy_write(phydev, TLK10X_ADDAR, reg);
+		phy_write(phydev, TLK10X_REGCR, 0x401F);
+		reg = TLK10X_ADDAR;
+	}
+
+	return phy_read(phydev, reg);
+}
+
+static int tlk10x_write(struct phy_device *phydev, int reg, int val)
+{
+	if (reg & ~0x1f) {
+		/* Extended register */
+		phy_write(phydev, TLK10X_REGCR, 0x001F);
+		phy_write(phydev, TLK10X_ADDAR, reg);
+		phy_write(phydev, TLK10X_REGCR, 0x401F);
+		reg = TLK10X_ADDAR;
+	}
+
+	return phy_write(phydev, reg, val);
+}
+
+#ifdef CONFIG_OF_MDIO
+static int tlk10x_of_init(struct phy_device *phydev)
+{
+	struct tlk10x_private *tlk10x = phydev->priv;
+	struct device *dev = &phydev->mdio.dev;
+	struct device_node *of_node = dev->of_node;
+	int ret;
+
+	if (!of_node)
+		return 0;
+
+	ret = of_property_read_u32(of_node, "ti,power-back-off",
+				   &tlk10x->pwrbo_level);
+	if (ret) {
+		dev_err(dev, "missing ti,power-back-off property");
+		tlk10x->pwrbo_level = 0;
+	}
+
+	return 0;
+}
+#else
+static int tlk10x_of_init(struct phy_device *phydev)
+{
+	return 0;
+}
+#endif /* CONFIG_OF_MDIO */
+
+static int tlk10x_config_init(struct phy_device *phydev)
+{
+	int ret, reg;
+	struct tlk10x_private *tlk10x;
+
+	ret = genphy_config_init(phydev);
+	if (ret < 0)
+		return ret;
+
+	if (!phydev->priv) {
+		tlk10x = devm_kzalloc(&phydev->mdio.dev, sizeof(*tlk10x),
+				      GFP_KERNEL);
+		if (!tlk10x)
+			return -ENOMEM;
+
+		phydev->priv = tlk10x;
+		ret = tlk10x_of_init(phydev);
+		if (ret)
+			return ret;
+	} else {
+		tlk10x = (struct tlk10x_private *)phydev->priv;
+	}
+
+	// Power back off
+	if (tlk10x->pwrbo_level < 0 || tlk10x->pwrbo_level > 3)
+		tlk10x->pwrbo_level = 0;
+	reg = tlk10x_read(phydev, TLK10X_PWRBOCR);
+	reg = ((reg & ~TLK10X_PWRBOCR_MASK)
+		| (tlk10x->pwrbo_level << 6));
+	ret = tlk10x_write(phydev, TLK10X_PWRBOCR, reg);
+	if (ret < 0) {
+		dev_err(&phydev->mdio.dev,
+			"unable to set power back-off (err=%d)\n", ret);
+		return ret;
+	}
+	dev_info(&phydev->mdio.dev, "power back-off set to level %d\n",
+		 tlk10x->pwrbo_level);
+
+	return 0;
+}
+
+static int tlk10x_ack_interrupt(struct phy_device *phydev)
+{
+	int err = tlk10x_read(phydev, TLK10X_MISR);
+
+	return err < 0 ? err : 0;
+}
+
+static int tlk10x_config_intr(struct phy_device *phydev)
+{
+	int control, ret;
+
+	control = tlk10x_read(phydev, TLK10X_MICR);
+	if (control < 0)
+		return control;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		control |= TLK10X_MICR_INT_OE;
+		control |= TLK10X_MICR_INTEN;
+
+		ret = tlk10x_write(phydev, TLK10X_MISR, TLK10X_INT_EN_MASK);
+		if (ret < 0)
+			return ret;
+	} else {
+		control &= ~TLK10X_MICR_INTEN;
+	}
+
+	return tlk10x_write(phydev, TLK10X_MICR, control);
+}
+
+static struct phy_driver tlk10x_driver[] = {
+	{
+		.phy_id		= TLK10X_PHY_ID,
+		.phy_id_mask	= 0xfffffff0,
+		.name		= "TI TLK10X 10/100 Mbps PHY",
+		.features	= PHY_BASIC_FEATURES,
+		.flags		= PHY_HAS_INTERRUPT,
+
+		.config_init	= tlk10x_config_init,
+		.soft_reset	= genphy_soft_reset,
+
+		/* IRQ related */
+		.ack_interrupt	= tlk10x_ack_interrupt,
+		.config_intr	= tlk10x_config_intr,
+
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+	},
+};
+module_phy_driver(tlk10x_driver);
+
+static struct mdio_device_id __maybe_unused tlk10x_tbl[] = {
+	{ TLK10X_PHY_ID, 0xfffffff0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(mdio, tlk10x_tbl);
+
+MODULE_DESCRIPTION("Texas Instruments TLK105 / TLK106 PHY driver");
+MODULE_AUTHOR("Mans Andersson <mans.andersson@nibe.se>");
+MODULE_LICENSE("GPL");
-- 
2.9.3

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: phy: TLK10X initial driver submission
  2018-04-19  8:28 [PATCH] net: phy: TLK10X initial driver submission Måns Andersson
@ 2018-04-19 12:09 ` Andrew Lunn
  2018-04-20  6:58   ` Måns Andersson
  2018-04-19 12:24 ` Miguel Ojeda
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Andrew Lunn @ 2018-04-19 12:09 UTC (permalink / raw)
  To: Måns Andersson
  Cc: Rob Herring, Mark Rutland, Florian Fainelli, netdev, devicetree,
	linux-kernel

On Thu, Apr 19, 2018 at 10:28:16AM +0200, Måns Andersson wrote:
> From: Mans Andersson <mans.andersson@nibe.se>
> 
> Add suport for the TI TLK105 and TLK106 10/100Mbit ethernet phys.
> 
> In addition the TLK10X needs to be removed from DP83848 driver as the
> power back off support is added here for this device.
> 
> Datasheet:
> http://www.ti.com/lit/gpn/tlk106
> ---
>  .../devicetree/bindings/net/ti,tlk10x.txt          |  27 +++
>  drivers/net/phy/Kconfig                            |   5 +
>  drivers/net/phy/Makefile                           |   1 +
>  drivers/net/phy/dp83848.c                          |   3 -
>  drivers/net/phy/tlk10x.c                           | 209 +++++++++++++++++++++
>  5 files changed, 242 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/ti,tlk10x.txt
>  create mode 100644 drivers/net/phy/tlk10x.c
> 
> diff --git a/Documentation/devicetree/bindings/net/ti,tlk10x.txt b/Documentation/devicetree/bindings/net/ti,tlk10x.txt
> new file mode 100644
> index 0000000..371d0d7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/ti,tlk10x.txt
> @@ -0,0 +1,27 @@
> +* Texas Instruments - TLK105 / TLK106 ethernet PHYs
> +
> +Required properties:
> +	- reg - The ID number for the phy, usually a small integer
> +
> +Optional properties:
> +	- ti,power-back-off - Power Back Off Level
> +		Please refer to data sheet chapter 8.6 and TI Application
> +		Note SLLA3228
> +		0 - Normal Operation
> +		1 - Level 1 (up to 140m cable between TLK link partners)
> +		2 - Level 2 (up to 100m cable between TLK link partners)
> +		3 - Level 3 (up to 80m cable between TLK link partners)

Hi Måns

Device tree is all about board properties. In most cases, power back
off is not a board properties, since it depends on the cable length
and the peer board. If however, your board has two PHYs back to back,
say to connect to an Ethernet switch, that would be a valid board
property.

How are you using this?

I know of others who would like such a configuration. Marvell PHYs can
do something similar. I've always suggested adding a PHY tunable. Pass
the cable length in meters and let the PHY driver pick the nearest it
can do, rounding up. The Marvell PHYs also support measuring the cable
length as part of the cable diagnostics. So it would be good to
reserve a configuration value to mean 'auto' - measure the cable and
then pick the best power back off. Quickly scanning the data sheet, i
see that this PHY also has the ability to measure the cable length.

> +static int tlk10x_read(struct phy_device *phydev, int reg)
> +{
> +	if (reg & ~0x1f) {
> +		/* Extended register */
> +		phy_write(phydev, TLK10X_REGCR, 0x001F);
> +		phy_write(phydev, TLK10X_ADDAR, reg);
> +		phy_write(phydev, TLK10X_REGCR, 0x401F);
> +		reg = TLK10X_ADDAR;
> +	}
> +
> +	return phy_read(phydev, reg);
> +}
> +
> +static int tlk10x_write(struct phy_device *phydev, int reg, int val)
> +{
> +	if (reg & ~0x1f) {
> +		/* Extended register */
> +		phy_write(phydev, TLK10X_REGCR, 0x001F);
> +		phy_write(phydev, TLK10X_ADDAR, reg);
> +		phy_write(phydev, TLK10X_REGCR, 0x401F);
> +		reg = TLK10X_ADDAR;
> +	}
> +
> +	return phy_write(phydev, reg, val);
> +}

This looks to be phy_read_mmd() and phy_write_mmd(). If so, please use
them, they get the locking correct.


> +#ifdef CONFIG_OF_MDIO
> +static int tlk10x_of_init(struct phy_device *phydev)
> +{
> +	struct tlk10x_private *tlk10x = phydev->priv;
> +	struct device *dev = &phydev->mdio.dev;
> +	struct device_node *of_node = dev->of_node;
> +	int ret;
> +
> +	if (!of_node)
> +		return 0;
> +
> +	ret = of_property_read_u32(of_node, "ti,power-back-off",
> +				   &tlk10x->pwrbo_level);
> +	if (ret) {
> +		dev_err(dev, "missing ti,power-back-off property");
> +		tlk10x->pwrbo_level = 0;
> +	}

If we decide to accept this, you should do range checking, and return
-EINVAL if the value is out of range.

> +static int tlk10x_config_init(struct phy_device *phydev)
> +{
> +	int ret, reg;
> +	struct tlk10x_private *tlk10x;
> +
> +	ret = genphy_config_init(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!phydev->priv) {
> +		tlk10x = devm_kzalloc(&phydev->mdio.dev, sizeof(*tlk10x),
> +				      GFP_KERNEL);
> +		if (!tlk10x)
> +			return -ENOMEM;
> +
> +		phydev->priv = tlk10x;
> +		ret = tlk10x_of_init(phydev);
> +		if (ret)
> +			return ret;
> +	} else {
> +		tlk10x = (struct tlk10x_private *)phydev->priv;
> +	}

This allocation should be done in .probe

> +
> +	// Power back off
> +	if (tlk10x->pwrbo_level < 0 || tlk10x->pwrbo_level > 3)
> +		tlk10x->pwrbo_level = 0;
> +	reg = tlk10x_read(phydev, TLK10X_PWRBOCR);
> +	reg = ((reg & ~TLK10X_PWRBOCR_MASK)
> +		| (tlk10x->pwrbo_level << 6));
> +	ret = tlk10x_write(phydev, TLK10X_PWRBOCR, reg);
> +	if (ret < 0) {
> +		dev_err(&phydev->mdio.dev,
> +			"unable to set power back-off (err=%d)\n", ret);
> +		return ret;
> +	}
> +	dev_info(&phydev->mdio.dev, "power back-off set to level %d\n",
> +		 tlk10x->pwrbo_level);
> +
> +	return 0;
> +}

  Andrew

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: phy: TLK10X initial driver submission
  2018-04-19  8:28 [PATCH] net: phy: TLK10X initial driver submission Måns Andersson
  2018-04-19 12:09 ` Andrew Lunn
@ 2018-04-19 12:24 ` Miguel Ojeda
  2018-04-20  7:07   ` Måns Andersson
  2018-04-24 16:34 ` Rob Herring
  2018-04-24 16:52 ` Florian Fainelli
  3 siblings, 1 reply; 8+ messages in thread
From: Miguel Ojeda @ 2018-04-19 12:24 UTC (permalink / raw)
  To: Måns Andersson
  Cc: Rob Herring, Mark Rutland, Andrew Lunn, Florian Fainelli,
	Network Development, devicetree, linux-kernel

On Thu, Apr 19, 2018 at 10:28 AM, Måns Andersson <mans.andersson@nibe.se> wrote:
> From: Mans Andersson <mans.andersson@nibe.se>
>
> Add suport for the TI TLK105 and TLK106 10/100Mbit ethernet phys.
>

Hi Mans,

Some quick notes.

> In addition the TLK10X needs to be removed from DP83848 driver as the
> power back off support is added here for this device.
>
> Datasheet:
> http://www.ti.com/lit/gpn/tlk106

Missing signature.

> ---
>  .../devicetree/bindings/net/ti,tlk10x.txt          |  27 +++
>  drivers/net/phy/Kconfig                            |   5 +
>  drivers/net/phy/Makefile                           |   1 +
>  drivers/net/phy/dp83848.c                          |   3 -
>  drivers/net/phy/tlk10x.c                           | 209 +++++++++++++++++++++
>  5 files changed, 242 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/ti,tlk10x.txt
>  create mode 100644 drivers/net/phy/tlk10x.c
>
> diff --git a/Documentation/devicetree/bindings/net/ti,tlk10x.txt b/Documentation/devicetree/bindings/net/ti,tlk10x.txt
> new file mode 100644
> index 0000000..371d0d7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/ti,tlk10x.txt
> @@ -0,0 +1,27 @@
> +* Texas Instruments - TLK105 / TLK106 ethernet PHYs
> +
> +Required properties:
> +       - reg - The ID number for the phy, usually a small integer
> +
> +Optional properties:
> +       - ti,power-back-off - Power Back Off Level
> +               Please refer to data sheet chapter 8.6 and TI Application
> +               Note SLLA3228
> +               0 - Normal Operation
> +               1 - Level 1 (up to 140m cable between TLK link partners)
> +               2 - Level 2 (up to 100m cable between TLK link partners)
> +               3 - Level 3 (up to 80m cable between TLK link partners)
> +
> +Default child nodes are standard Ethernet PHY device
> +nodes as described in Documentation/devicetree/bindings/net/phy.txt
> +
> +Example:
> +
> +       ethernet-phy@0 {
> +               reg = <0>;
> +               ti,power-back-off = <2>;
> +       };
> +
> +Datasheets and documentation can be found at:
> +http://www.ti.com/lit/gpn/tlk106
> +http://www.ti.com/lit/an/slla328/slla328.pdf
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index bdfbabb..c980240 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -295,6 +295,11 @@ config DP83867_PHY
>         ---help---
>           Currently supports the DP83867 PHY.
>
> +config TLK10X_PHY
> +       tristate "Texas Instruments TLK10x PHY"
> +       ---help---
> +         Supports the TLK105 and TLK106 PHYs.
> +
>  config FIXED_PHY
>         tristate "MDIO Bus/PHY emulation with fixed speed/link PHYs"
>         depends on PHYLIB
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index 01acbcb..37e4e02 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -79,5 +79,6 @@ obj-$(CONFIG_ROCKCHIP_PHY)    += rockchip.o
>  obj-$(CONFIG_SMSC_PHY)         += smsc.o
>  obj-$(CONFIG_STE10XP)          += ste10Xp.o
>  obj-$(CONFIG_TERANETICS_PHY)   += teranetics.o
> +obj-$(CONFIG_TLK10X_PHY)       += tlk10x.o
>  obj-$(CONFIG_VITESSE_PHY)      += vitesse.o
>  obj-$(CONFIG_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
> diff --git a/drivers/net/phy/dp83848.c b/drivers/net/phy/dp83848.c
> index cd09c3a..435f401 100644
> --- a/drivers/net/phy/dp83848.c
> +++ b/drivers/net/phy/dp83848.c
> @@ -19,7 +19,6 @@
>  #define TI_DP83848C_PHY_ID             0x20005ca0
>  #define TI_DP83620_PHY_ID              0x20005ce0
>  #define NS_DP83848C_PHY_ID             0x20005c90
> -#define TLK10X_PHY_ID                  0x2000a210
>
>  /* Registers */
>  #define DP83848_MICR                   0x11 /* MII Interrupt Control Register */
> @@ -78,7 +77,6 @@ static struct mdio_device_id __maybe_unused dp83848_tbl[] = {
>         { TI_DP83848C_PHY_ID, 0xfffffff0 },
>         { NS_DP83848C_PHY_ID, 0xfffffff0 },
>         { TI_DP83620_PHY_ID, 0xfffffff0 },
> -       { TLK10X_PHY_ID, 0xfffffff0 },
>         { }
>  };
>  MODULE_DEVICE_TABLE(mdio, dp83848_tbl);
> @@ -105,7 +103,6 @@ static struct phy_driver dp83848_driver[] = {
>         DP83848_PHY_DRIVER(TI_DP83848C_PHY_ID, "TI DP83848C 10/100 Mbps PHY"),
>         DP83848_PHY_DRIVER(NS_DP83848C_PHY_ID, "NS DP83848C 10/100 Mbps PHY"),
>         DP83848_PHY_DRIVER(TI_DP83620_PHY_ID, "TI DP83620 10/100 Mbps PHY"),
> -       DP83848_PHY_DRIVER(TLK10X_PHY_ID, "TI TLK10X 10/100 Mbps PHY"),
>  };
>  module_phy_driver(dp83848_driver);
>
> diff --git a/drivers/net/phy/tlk10x.c b/drivers/net/phy/tlk10x.c
> new file mode 100644
> index 0000000..1efc81e
> --- /dev/null
> +++ b/drivers/net/phy/tlk10x.c
> @@ -0,0 +1,209 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * Driver for the Texas Instruments TLK105 / TLK106
> + *
> + * Copyright (C) 2018 NIBE Industrier AB - http://www.nibe.com
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

Since you are using the SPDX id, please remove the license text (which
is actually wrong: it seems you have cut the v2+ version and then
removed the last sentence of the first paragraph? :-).

> + */
> +
> +#include <linux/module.h>
> +#include <linux/phy.h>
> +#include <linux/of.h>
> +
> +#define TLK10X_PHY_ID                  0x2000a210
> +
> +/* Registers */
> +#define TLK10X_MICR                    0x11 /* MII Interrupt Control Reg */
> +#define TLK10X_MISR                    0x12 /* MII Interrupt Status Reg */
> +#define TLK10X_REGCR                   0x0d /* Register Control Register */
> +#define TLK10X_ADDAR                   0x0e /* Data Register */
> +#define TLK10X_PWRBOCR                 0xae /* Power Backoff Register */
> +
> +/* MICR Register Fields */
> +#define TLK10X_MICR_INT_OE             BIT(0) /* Interrupt Output Enable */
> +#define TLK10X_MICR_INTEN              BIT(1) /* Interrupt Enable */
> +
> +/* MISR Register Fields */
> +#define TLK10X_MISR_RHF_INT_EN         BIT(0) /* Receive Error Counter */
> +#define TLK10X_MISR_FHF_INT_EN         BIT(1) /* False Carrier Counter */
> +#define TLK10X_MISR_ANC_INT_EN         BIT(2) /* Auto-negotiation complete */
> +#define TLK10X_MISR_DUP_INT_EN         BIT(3) /* Duplex Status */
> +#define TLK10X_MISR_SPD_INT_EN         BIT(4) /* Speed status */
> +#define TLK10X_MISR_LINK_INT_EN                BIT(5) /* Link status */
> +#define TLK10X_MISR_ED_INT_EN          BIT(6) /* Energy detect */
> +#define TLK10X_MISR_LQM_INT_EN         BIT(7) /* Link Quality Monitor */
> +
> +/* PWRBOCR Register Fields */
> +#define TLK10X_PWRBOCR_MASK            0xe0 /* Power Backoff Mask */
> +
> +#define TLK10X_INT_EN_MASK             \
> +       (TLK10X_MISR_ANC_INT_EN |       \
> +        TLK10X_MISR_DUP_INT_EN |       \
> +        TLK10X_MISR_SPD_INT_EN |       \
> +        TLK10X_MISR_LINK_INT_EN)
> +
> +struct tlk10x_private {
> +       int pwrbo_level;
> +};
> +
> +static int tlk10x_read(struct phy_device *phydev, int reg)
> +{
> +       if (reg & ~0x1f) {

0x1f or ~0x1f should probably have a #define name.

> +               /* Extended register */
> +               phy_write(phydev, TLK10X_REGCR, 0x001F);
> +               phy_write(phydev, TLK10X_ADDAR, reg);
> +               phy_write(phydev, TLK10X_REGCR, 0x401F);
> +               reg = TLK10X_ADDAR;
> +       }
> +
> +       return phy_read(phydev, reg);
> +}
> +
> +static int tlk10x_write(struct phy_device *phydev, int reg, int val)
> +{
> +       if (reg & ~0x1f) {

Ditto.

> +               /* Extended register */
> +               phy_write(phydev, TLK10X_REGCR, 0x001F);
> +               phy_write(phydev, TLK10X_ADDAR, reg);
> +               phy_write(phydev, TLK10X_REGCR, 0x401F);
> +               reg = TLK10X_ADDAR;
> +       }
> +
> +       return phy_write(phydev, reg, val);
> +}
> +
> +#ifdef CONFIG_OF_MDIO

Maybe you want the #ifdef inside.

> +static int tlk10x_of_init(struct phy_device *phydev)
> +{
> +       struct tlk10x_private *tlk10x = phydev->priv;
> +       struct device *dev = &phydev->mdio.dev;
> +       struct device_node *of_node = dev->of_node;
> +       int ret;
> +
> +       if (!of_node)
> +               return 0;
> +
> +       ret = of_property_read_u32(of_node, "ti,power-back-off",
> +                                  &tlk10x->pwrbo_level);
> +       if (ret) {
> +               dev_err(dev, "missing ti,power-back-off property");
> +               tlk10x->pwrbo_level = 0;
> +       }
> +
> +       return 0;
> +}
> +#else
> +static int tlk10x_of_init(struct phy_device *phydev)
> +{
> +       return 0;
> +}
> +#endif /* CONFIG_OF_MDIO */
> +
> +static int tlk10x_config_init(struct phy_device *phydev)
> +{
> +       int ret, reg;
> +       struct tlk10x_private *tlk10x;
> +
> +       ret = genphy_config_init(phydev);
> +       if (ret < 0)
> +               return ret;
> +
> +       if (!phydev->priv) {
> +               tlk10x = devm_kzalloc(&phydev->mdio.dev, sizeof(*tlk10x),
> +                                     GFP_KERNEL);
> +               if (!tlk10x)
> +                       return -ENOMEM;
> +
> +               phydev->priv = tlk10x;
> +               ret = tlk10x_of_init(phydev);
> +               if (ret)
> +                       return ret;
> +       } else {
> +               tlk10x = (struct tlk10x_private *)phydev->priv;
> +       }
> +
> +       // Power back off
> +       if (tlk10x->pwrbo_level < 0 || tlk10x->pwrbo_level > 3)
> +               tlk10x->pwrbo_level = 0;
> +       reg = tlk10x_read(phydev, TLK10X_PWRBOCR);
> +       reg = ((reg & ~TLK10X_PWRBOCR_MASK)
> +               | (tlk10x->pwrbo_level << 6));

Maybe the 6 should have a name, or maybe a bigger macro for this would clarify.

> +       ret = tlk10x_write(phydev, TLK10X_PWRBOCR, reg);
> +       if (ret < 0) {
> +               dev_err(&phydev->mdio.dev,
> +                       "unable to set power back-off (err=%d)\n", ret);
> +               return ret;
> +       }
> +       dev_info(&phydev->mdio.dev, "power back-off set to level %d\n",
> +                tlk10x->pwrbo_level);
> +
> +       return 0;
> +}
> +
> +static int tlk10x_ack_interrupt(struct phy_device *phydev)
> +{
> +       int err = tlk10x_read(phydev, TLK10X_MISR);

Following the style of the rest of the file, shouldn't this be:

    if (err < 0)
        return err;

    return 0;

?

> +
> +       return err < 0 ? err : 0;
> +}
> +
> +static int tlk10x_config_intr(struct phy_device *phydev)
> +{
> +       int control, ret;
> +
> +       control = tlk10x_read(phydev, TLK10X_MICR);
> +       if (control < 0)
> +               return control;
> +
> +       if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> +               control |= TLK10X_MICR_INT_OE;
> +               control |= TLK10X_MICR_INTEN;
> +
> +               ret = tlk10x_write(phydev, TLK10X_MISR, TLK10X_INT_EN_MASK);
> +               if (ret < 0)
> +                       return ret;
> +       } else {
> +               control &= ~TLK10X_MICR_INTEN;
> +       }
> +
> +       return tlk10x_write(phydev, TLK10X_MICR, control);
> +}
> +
> +static struct phy_driver tlk10x_driver[] = {
> +       {
> +               .phy_id         = TLK10X_PHY_ID,
> +               .phy_id_mask    = 0xfffffff0,
> +               .name           = "TI TLK10X 10/100 Mbps PHY",
> +               .features       = PHY_BASIC_FEATURES,
> +               .flags          = PHY_HAS_INTERRUPT,
> +
> +               .config_init    = tlk10x_config_init,
> +               .soft_reset     = genphy_soft_reset,
> +
> +               /* IRQ related */
> +               .ack_interrupt  = tlk10x_ack_interrupt,
> +               .config_intr    = tlk10x_config_intr,
> +
> +               .suspend        = genphy_suspend,
> +               .resume         = genphy_resume,
> +       },
> +};
> +module_phy_driver(tlk10x_driver);
> +
> +static struct mdio_device_id __maybe_unused tlk10x_tbl[] = {
> +       { TLK10X_PHY_ID, 0xfffffff0 },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(mdio, tlk10x_tbl);
> +
> +MODULE_DESCRIPTION("Texas Instruments TLK105 / TLK106 PHY driver");
> +MODULE_AUTHOR("Mans Andersson <mans.andersson@nibe.se>");
> +MODULE_LICENSE("GPL");

Should be "GPL v2".

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: phy: TLK10X initial driver submission
  2018-04-19 12:09 ` Andrew Lunn
@ 2018-04-20  6:58   ` Måns Andersson
  0 siblings, 0 replies; 8+ messages in thread
From: Måns Andersson @ 2018-04-20  6:58 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Rob Herring, Mark Rutland, Florian Fainelli, netdev, devicetree,
	linux-kernel

On Thu, Apr 19, 2018 at 02:09:02PM +0200, Andrew Lunn wrote:
> On Thu, Apr 19, 2018 at 10:28:16AM +0200, Måns Andersson wrote:
> > From: Mans Andersson <mans.andersson@nibe.se>
> > 
> > Add suport for the TI TLK105 and TLK106 10/100Mbit ethernet phys.
> > 
> > In addition the TLK10X needs to be removed from DP83848 driver as the
> > power back off support is added here for this device.
> > 
> > Datasheet:
> > http://www.ti.com/lit/gpn/tlk106
> > ---
> >  .../devicetree/bindings/net/ti,tlk10x.txt          |  27 +++
> >  drivers/net/phy/Kconfig                            |   5 +
> >  drivers/net/phy/Makefile                           |   1 +
> >  drivers/net/phy/dp83848.c                          |   3 -
> >  drivers/net/phy/tlk10x.c                           | 209 +++++++++++++++++++++
> >  5 files changed, 242 insertions(+), 3 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/net/ti,tlk10x.txt
> >  create mode 100644 drivers/net/phy/tlk10x.c
> > 
> > diff --git a/Documentation/devicetree/bindings/net/ti,tlk10x.txt b/Documentation/devicetree/bindings/net/ti,tlk10x.txt
> > new file mode 100644
> > index 0000000..371d0d7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/ti,tlk10x.txt
> > @@ -0,0 +1,27 @@
> > +* Texas Instruments - TLK105 / TLK106 ethernet PHYs
> > +
> > +Required properties:
> > +	- reg - The ID number for the phy, usually a small integer
> > +
> > +Optional properties:
> > +	- ti,power-back-off - Power Back Off Level
> > +		Please refer to data sheet chapter 8.6 and TI Application
> > +		Note SLLA3228
> > +		0 - Normal Operation
> > +		1 - Level 1 (up to 140m cable between TLK link partners)
> > +		2 - Level 2 (up to 100m cable between TLK link partners)
> > +		3 - Level 3 (up to 80m cable between TLK link partners)
> 
> Hi Måns
> 
> Device tree is all about board properties. In most cases, power back
> off is not a board properties, since it depends on the cable length
> and the peer board. If however, your board has two PHYs back to back,
> say to connect to an Ethernet switch, that would be a valid board
> property.
> 
> How are you using this?
> 
> I know of others who would like such a configuration. Marvell PHYs can
> do something similar. I've always suggested adding a PHY tunable. Pass
> the cable length in meters and let the PHY driver pick the nearest it
> can do, rounding up. The Marvell PHYs also support measuring the cable
> length as part of the cable diagnostics. So it would be good to
> reserve a configuration value to mean 'auto' - measure the cable and
> then pick the best power back off. Quickly scanning the data sheet, i
> see that this PHY also has the ability to measure the cable length.
>

Hi Andrew,

Thanks for your comments, highly appreciated!

I'm using this to lock down the PHY to the IEEE 802.3 100m standard cable
length, as opposed to the extended 150m which the PHY supports in its
default operation (see pg. 2 of the data sheet). The reason why I need this
is that the board has too high EMC emissions when running with the default
operation. For me the setting is therefore used as a board property, i.e.
it's not something that will be changed during operation.
 
> > +static int tlk10x_read(struct phy_device *phydev, int reg)
> > +{
> > +	if (reg & ~0x1f) {
> > +		/* Extended register */
> > +		phy_write(phydev, TLK10X_REGCR, 0x001F);
> > +		phy_write(phydev, TLK10X_ADDAR, reg);
> > +		phy_write(phydev, TLK10X_REGCR, 0x401F);
> > +		reg = TLK10X_ADDAR;
> > +	}
> > +
> > +	return phy_read(phydev, reg);
> > +}
> > +
> > +static int tlk10x_write(struct phy_device *phydev, int reg, int val)
> > +{
> > +	if (reg & ~0x1f) {
> > +		/* Extended register */
> > +		phy_write(phydev, TLK10X_REGCR, 0x001F);
> > +		phy_write(phydev, TLK10X_ADDAR, reg);
> > +		phy_write(phydev, TLK10X_REGCR, 0x401F);
> > +		reg = TLK10X_ADDAR;
> > +	}
> > +
> > +	return phy_write(phydev, reg, val);
> > +}
> 
> This looks to be phy_read_mmd() and phy_write_mmd(). If so, please use
> them, they get the locking correct.
>

Yes, that's correct, will fix that.

> 
> > +#ifdef CONFIG_OF_MDIO
> > +static int tlk10x_of_init(struct phy_device *phydev)
> > +{
> > +	struct tlk10x_private *tlk10x = phydev->priv;
> > +	struct device *dev = &phydev->mdio.dev;
> > +	struct device_node *of_node = dev->of_node;
> > +	int ret;
> > +
> > +	if (!of_node)
> > +		return 0;
> > +
> > +	ret = of_property_read_u32(of_node, "ti,power-back-off",
> > +				   &tlk10x->pwrbo_level);
> > +	if (ret) {
> > +		dev_err(dev, "missing ti,power-back-off property");
> > +		tlk10x->pwrbo_level = 0;
> > +	}
> 
> If we decide to accept this, you should do range checking, and return
> -EINVAL if the value is out of range.

Ok, will fix that.

> 
> > +static int tlk10x_config_init(struct phy_device *phydev)
> > +{
> > +	int ret, reg;
> > +	struct tlk10x_private *tlk10x;
> > +
> > +	ret = genphy_config_init(phydev);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (!phydev->priv) {
> > +		tlk10x = devm_kzalloc(&phydev->mdio.dev, sizeof(*tlk10x),
> > +				      GFP_KERNEL);
> > +		if (!tlk10x)
> > +			return -ENOMEM;
> > +
> > +		phydev->priv = tlk10x;
> > +		ret = tlk10x_of_init(phydev);
> > +		if (ret)
> > +			return ret;
> > +	} else {
> > +		tlk10x = (struct tlk10x_private *)phydev->priv;
> > +	}
> 
> This allocation should be done in .probe

Ok, will fix that.

> 
> > +
> > +	// Power back off
> > +	if (tlk10x->pwrbo_level < 0 || tlk10x->pwrbo_level > 3)
> > +		tlk10x->pwrbo_level = 0;
> > +	reg = tlk10x_read(phydev, TLK10X_PWRBOCR);
> > +	reg = ((reg & ~TLK10X_PWRBOCR_MASK)
> > +		| (tlk10x->pwrbo_level << 6));
> > +	ret = tlk10x_write(phydev, TLK10X_PWRBOCR, reg);
> > +	if (ret < 0) {
> > +		dev_err(&phydev->mdio.dev,
> > +			"unable to set power back-off (err=%d)\n", ret);
> > +		return ret;
> > +	}
> > +	dev_info(&phydev->mdio.dev, "power back-off set to level %d\n",
> > +		 tlk10x->pwrbo_level);
> > +
> > +	return 0;
> > +}
> 
>   Andrew

// Måns

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: phy: TLK10X initial driver submission
  2018-04-19 12:24 ` Miguel Ojeda
@ 2018-04-20  7:07   ` Måns Andersson
  0 siblings, 0 replies; 8+ messages in thread
From: Måns Andersson @ 2018-04-20  7:07 UTC (permalink / raw)
  To: Miguel Ojeda
  Cc: Rob Herring, Mark Rutland, Andrew Lunn, Florian Fainelli,
	Network Development, devicetree, linux-kernel

On Thu, Apr 19, 2018 at 02:24:27PM +0200, Miguel Ojeda wrote:
> On Thu, Apr 19, 2018 at 10:28 AM, Måns Andersson <mans.andersson@nibe.se> wrote:
> > From: Mans Andersson <mans.andersson@nibe.se>
> >
> > Add suport for the TI TLK105 and TLK106 10/100Mbit ethernet phys.
> >
> 
> Hi Mans,
> 
> Some quick notes.
> 
> > In addition the TLK10X needs to be removed from DP83848 driver as the
> > power back off support is added here for this device.
> >
> > Datasheet:
> > http://www.ti.com/lit/gpn/tlk106
> 
> Missing signature.

Hi Miguel,

My bad, will fix.

> 
> > ---
> >  .../devicetree/bindings/net/ti,tlk10x.txt          |  27 +++
> >  drivers/net/phy/Kconfig                            |   5 +
> >  drivers/net/phy/Makefile                           |   1 +
> >  drivers/net/phy/dp83848.c                          |   3 -
> >  drivers/net/phy/tlk10x.c                           | 209 +++++++++++++++++++++
> >  5 files changed, 242 insertions(+), 3 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/net/ti,tlk10x.txt
> >  create mode 100644 drivers/net/phy/tlk10x.c
> >
> > diff --git a/Documentation/devicetree/bindings/net/ti,tlk10x.txt b/Documentation/devicetree/bindings/net/ti,tlk10x.txt
> > new file mode 100644
> > index 0000000..371d0d7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/ti,tlk10x.txt
> > @@ -0,0 +1,27 @@
> > +* Texas Instruments - TLK105 / TLK106 ethernet PHYs
> > +
> > +Required properties:
> > +       - reg - The ID number for the phy, usually a small integer
> > +
> > +Optional properties:
> > +       - ti,power-back-off - Power Back Off Level
> > +               Please refer to data sheet chapter 8.6 and TI Application
> > +               Note SLLA3228
> > +               0 - Normal Operation
> > +               1 - Level 1 (up to 140m cable between TLK link partners)
> > +               2 - Level 2 (up to 100m cable between TLK link partners)
> > +               3 - Level 3 (up to 80m cable between TLK link partners)
> > +
> > +Default child nodes are standard Ethernet PHY device
> > +nodes as described in Documentation/devicetree/bindings/net/phy.txt
> > +
> > +Example:
> > +
> > +       ethernet-phy@0 {
> > +               reg = <0>;
> > +               ti,power-back-off = <2>;
> > +       };
> > +
> > +Datasheets and documentation can be found at:
> > +http://www.ti.com/lit/gpn/tlk106
> > +http://www.ti.com/lit/an/slla328/slla328.pdf
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index bdfbabb..c980240 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -295,6 +295,11 @@ config DP83867_PHY
> >         ---help---
> >           Currently supports the DP83867 PHY.
> >
> > +config TLK10X_PHY
> > +       tristate "Texas Instruments TLK10x PHY"
> > +       ---help---
> > +         Supports the TLK105 and TLK106 PHYs.
> > +
> >  config FIXED_PHY
> >         tristate "MDIO Bus/PHY emulation with fixed speed/link PHYs"
> >         depends on PHYLIB
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> > index 01acbcb..37e4e02 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -79,5 +79,6 @@ obj-$(CONFIG_ROCKCHIP_PHY)    += rockchip.o
> >  obj-$(CONFIG_SMSC_PHY)         += smsc.o
> >  obj-$(CONFIG_STE10XP)          += ste10Xp.o
> >  obj-$(CONFIG_TERANETICS_PHY)   += teranetics.o
> > +obj-$(CONFIG_TLK10X_PHY)       += tlk10x.o
> >  obj-$(CONFIG_VITESSE_PHY)      += vitesse.o
> >  obj-$(CONFIG_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
> > diff --git a/drivers/net/phy/dp83848.c b/drivers/net/phy/dp83848.c
> > index cd09c3a..435f401 100644
> > --- a/drivers/net/phy/dp83848.c
> > +++ b/drivers/net/phy/dp83848.c
> > @@ -19,7 +19,6 @@
> >  #define TI_DP83848C_PHY_ID             0x20005ca0
> >  #define TI_DP83620_PHY_ID              0x20005ce0
> >  #define NS_DP83848C_PHY_ID             0x20005c90
> > -#define TLK10X_PHY_ID                  0x2000a210
> >
> >  /* Registers */
> >  #define DP83848_MICR                   0x11 /* MII Interrupt Control Register */
> > @@ -78,7 +77,6 @@ static struct mdio_device_id __maybe_unused dp83848_tbl[] = {
> >         { TI_DP83848C_PHY_ID, 0xfffffff0 },
> >         { NS_DP83848C_PHY_ID, 0xfffffff0 },
> >         { TI_DP83620_PHY_ID, 0xfffffff0 },
> > -       { TLK10X_PHY_ID, 0xfffffff0 },
> >         { }
> >  };
> >  MODULE_DEVICE_TABLE(mdio, dp83848_tbl);
> > @@ -105,7 +103,6 @@ static struct phy_driver dp83848_driver[] = {
> >         DP83848_PHY_DRIVER(TI_DP83848C_PHY_ID, "TI DP83848C 10/100 Mbps PHY"),
> >         DP83848_PHY_DRIVER(NS_DP83848C_PHY_ID, "NS DP83848C 10/100 Mbps PHY"),
> >         DP83848_PHY_DRIVER(TI_DP83620_PHY_ID, "TI DP83620 10/100 Mbps PHY"),
> > -       DP83848_PHY_DRIVER(TLK10X_PHY_ID, "TI TLK10X 10/100 Mbps PHY"),
> >  };
> >  module_phy_driver(dp83848_driver);
> >
> > diff --git a/drivers/net/phy/tlk10x.c b/drivers/net/phy/tlk10x.c
> > new file mode 100644
> > index 0000000..1efc81e
> > --- /dev/null
> > +++ b/drivers/net/phy/tlk10x.c
> > @@ -0,0 +1,209 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/**
> > + * Driver for the Texas Instruments TLK105 / TLK106
> > + *
> > + * Copyright (C) 2018 NIBE Industrier AB - http://www.nibe.com
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> 
> Since you are using the SPDX id, please remove the license text (which
> is actually wrong: it seems you have cut the v2+ version and then
> removed the last sentence of the first paragraph? :-).
>

Ok.
 
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/phy.h>
> > +#include <linux/of.h>
> > +
> > +#define TLK10X_PHY_ID                  0x2000a210
> > +
> > +/* Registers */
> > +#define TLK10X_MICR                    0x11 /* MII Interrupt Control Reg */
> > +#define TLK10X_MISR                    0x12 /* MII Interrupt Status Reg */
> > +#define TLK10X_REGCR                   0x0d /* Register Control Register */
> > +#define TLK10X_ADDAR                   0x0e /* Data Register */
> > +#define TLK10X_PWRBOCR                 0xae /* Power Backoff Register */
> > +
> > +/* MICR Register Fields */
> > +#define TLK10X_MICR_INT_OE             BIT(0) /* Interrupt Output Enable */
> > +#define TLK10X_MICR_INTEN              BIT(1) /* Interrupt Enable */
> > +
> > +/* MISR Register Fields */
> > +#define TLK10X_MISR_RHF_INT_EN         BIT(0) /* Receive Error Counter */
> > +#define TLK10X_MISR_FHF_INT_EN         BIT(1) /* False Carrier Counter */
> > +#define TLK10X_MISR_ANC_INT_EN         BIT(2) /* Auto-negotiation complete */
> > +#define TLK10X_MISR_DUP_INT_EN         BIT(3) /* Duplex Status */
> > +#define TLK10X_MISR_SPD_INT_EN         BIT(4) /* Speed status */
> > +#define TLK10X_MISR_LINK_INT_EN                BIT(5) /* Link status */
> > +#define TLK10X_MISR_ED_INT_EN          BIT(6) /* Energy detect */
> > +#define TLK10X_MISR_LQM_INT_EN         BIT(7) /* Link Quality Monitor */
> > +
> > +/* PWRBOCR Register Fields */
> > +#define TLK10X_PWRBOCR_MASK            0xe0 /* Power Backoff Mask */
> > +
> > +#define TLK10X_INT_EN_MASK             \
> > +       (TLK10X_MISR_ANC_INT_EN |       \
> > +        TLK10X_MISR_DUP_INT_EN |       \
> > +        TLK10X_MISR_SPD_INT_EN |       \
> > +        TLK10X_MISR_LINK_INT_EN)
> > +
> > +struct tlk10x_private {
> > +       int pwrbo_level;
> > +};
> > +
> > +static int tlk10x_read(struct phy_device *phydev, int reg)
> > +{
> > +       if (reg & ~0x1f) {
> 
> 0x1f or ~0x1f should probably have a #define name.
> 

That's true, will fix that.

> > +               /* Extended register */
> > +               phy_write(phydev, TLK10X_REGCR, 0x001F);
> > +               phy_write(phydev, TLK10X_ADDAR, reg);
> > +               phy_write(phydev, TLK10X_REGCR, 0x401F);
> > +               reg = TLK10X_ADDAR;
> > +       }
> > +
> > +       return phy_read(phydev, reg);
> > +}
> > +
> > +static int tlk10x_write(struct phy_device *phydev, int reg, int val)
> > +{
> > +       if (reg & ~0x1f) {
> 
> Ditto.
> 

Ok.

> > +               /* Extended register */
> > +               phy_write(phydev, TLK10X_REGCR, 0x001F);
> > +               phy_write(phydev, TLK10X_ADDAR, reg);
> > +               phy_write(phydev, TLK10X_REGCR, 0x401F);
> > +               reg = TLK10X_ADDAR;
> > +       }
> > +
> > +       return phy_write(phydev, reg, val);
> > +}
> > +
> > +#ifdef CONFIG_OF_MDIO
> 
> Maybe you want the #ifdef inside.
> 

What's the preferred way by the kernel maintainers? I've seen both solutions
used but figured this was the most common.

> > +static int tlk10x_of_init(struct phy_device *phydev)
> > +{
> > +       struct tlk10x_private *tlk10x = phydev->priv;
> > +       struct device *dev = &phydev->mdio.dev;
> > +       struct device_node *of_node = dev->of_node;
> > +       int ret;
> > +
> > +       if (!of_node)
> > +               return 0;
> > +
> > +       ret = of_property_read_u32(of_node, "ti,power-back-off",
> > +                                  &tlk10x->pwrbo_level);
> > +       if (ret) {
> > +               dev_err(dev, "missing ti,power-back-off property");
> > +               tlk10x->pwrbo_level = 0;
> > +       }
> > +
> > +       return 0;
> > +}
> > +#else
> > +static int tlk10x_of_init(struct phy_device *phydev)
> > +{
> > +       return 0;
> > +}
> > +#endif /* CONFIG_OF_MDIO */
> > +
> > +static int tlk10x_config_init(struct phy_device *phydev)
> > +{
> > +       int ret, reg;
> > +       struct tlk10x_private *tlk10x;
> > +
> > +       ret = genphy_config_init(phydev);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       if (!phydev->priv) {
> > +               tlk10x = devm_kzalloc(&phydev->mdio.dev, sizeof(*tlk10x),
> > +                                     GFP_KERNEL);
> > +               if (!tlk10x)
> > +                       return -ENOMEM;
> > +
> > +               phydev->priv = tlk10x;
> > +               ret = tlk10x_of_init(phydev);
> > +               if (ret)
> > +                       return ret;
> > +       } else {
> > +               tlk10x = (struct tlk10x_private *)phydev->priv;
> > +       }
> > +
> > +       // Power back off
> > +       if (tlk10x->pwrbo_level < 0 || tlk10x->pwrbo_level > 3)
> > +               tlk10x->pwrbo_level = 0;
> > +       reg = tlk10x_read(phydev, TLK10X_PWRBOCR);
> > +       reg = ((reg & ~TLK10X_PWRBOCR_MASK)
> > +               | (tlk10x->pwrbo_level << 6));
> 
> Maybe the 6 should have a name, or maybe a bigger macro for this would clarify.
> 

Will look into this.

> > +       ret = tlk10x_write(phydev, TLK10X_PWRBOCR, reg);
> > +       if (ret < 0) {
> > +               dev_err(&phydev->mdio.dev,
> > +                       "unable to set power back-off (err=%d)\n", ret);
> > +               return ret;
> > +       }
> > +       dev_info(&phydev->mdio.dev, "power back-off set to level %d\n",
> > +                tlk10x->pwrbo_level);
> > +
> > +       return 0;
> > +}
> > +
> > +static int tlk10x_ack_interrupt(struct phy_device *phydev)
> > +{
> > +       int err = tlk10x_read(phydev, TLK10X_MISR);
> 
> Following the style of the rest of the file, shouldn't this be:
> 
>     if (err < 0)
>         return err;
> 
>     return 0;
> 
> ?
> 

True, will fix that.

> > +
> > +       return err < 0 ? err : 0;
> > +}
> > +
> > +static int tlk10x_config_intr(struct phy_device *phydev)
> > +{
> > +       int control, ret;
> > +
> > +       control = tlk10x_read(phydev, TLK10X_MICR);
> > +       if (control < 0)
> > +               return control;
> > +
> > +       if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> > +               control |= TLK10X_MICR_INT_OE;
> > +               control |= TLK10X_MICR_INTEN;
> > +
> > +               ret = tlk10x_write(phydev, TLK10X_MISR, TLK10X_INT_EN_MASK);
> > +               if (ret < 0)
> > +                       return ret;
> > +       } else {
> > +               control &= ~TLK10X_MICR_INTEN;
> > +       }
> > +
> > +       return tlk10x_write(phydev, TLK10X_MICR, control);
> > +}
> > +
> > +static struct phy_driver tlk10x_driver[] = {
> > +       {
> > +               .phy_id         = TLK10X_PHY_ID,
> > +               .phy_id_mask    = 0xfffffff0,
> > +               .name           = "TI TLK10X 10/100 Mbps PHY",
> > +               .features       = PHY_BASIC_FEATURES,
> > +               .flags          = PHY_HAS_INTERRUPT,
> > +
> > +               .config_init    = tlk10x_config_init,
> > +               .soft_reset     = genphy_soft_reset,
> > +
> > +               /* IRQ related */
> > +               .ack_interrupt  = tlk10x_ack_interrupt,
> > +               .config_intr    = tlk10x_config_intr,
> > +
> > +               .suspend        = genphy_suspend,
> > +               .resume         = genphy_resume,
> > +       },
> > +};
> > +module_phy_driver(tlk10x_driver);
> > +
> > +static struct mdio_device_id __maybe_unused tlk10x_tbl[] = {
> > +       { TLK10X_PHY_ID, 0xfffffff0 },
> > +       { }
> > +};
> > +MODULE_DEVICE_TABLE(mdio, tlk10x_tbl);
> > +
> > +MODULE_DESCRIPTION("Texas Instruments TLK105 / TLK106 PHY driver");
> > +MODULE_AUTHOR("Mans Andersson <mans.andersson@nibe.se>");
> > +MODULE_LICENSE("GPL");
> 
> Should be "GPL v2".
> 
> Cheers,
> Miguel

Good catch, will update that as well. Thanks for all the input! I will
get back in a couple of days with an updated patch.
// Måns

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: phy: TLK10X initial driver submission
  2018-04-19  8:28 [PATCH] net: phy: TLK10X initial driver submission Måns Andersson
  2018-04-19 12:09 ` Andrew Lunn
  2018-04-19 12:24 ` Miguel Ojeda
@ 2018-04-24 16:34 ` Rob Herring
  2018-04-24 17:52   ` Andrew Lunn
  2018-04-24 16:52 ` Florian Fainelli
  3 siblings, 1 reply; 8+ messages in thread
From: Rob Herring @ 2018-04-24 16:34 UTC (permalink / raw)
  To: Måns Andersson
  Cc: Mark Rutland, Andrew Lunn, Florian Fainelli, netdev, devicetree,
	linux-kernel

On Thu, Apr 19, 2018 at 10:28:16AM +0200, Måns Andersson wrote:
> From: Mans Andersson <mans.andersson@nibe.se>
> 
> Add suport for the TI TLK105 and TLK106 10/100Mbit ethernet phys.
> 
> In addition the TLK10X needs to be removed from DP83848 driver as the
> power back off support is added here for this device.
> 
> Datasheet:
> http://www.ti.com/lit/gpn/tlk106
> ---
>  .../devicetree/bindings/net/ti,tlk10x.txt          |  27 +++

Please split bindings to a separate patch.

>  drivers/net/phy/Kconfig                            |   5 +
>  drivers/net/phy/Makefile                           |   1 +
>  drivers/net/phy/dp83848.c                          |   3 -
>  drivers/net/phy/tlk10x.c                           | 209 +++++++++++++++++++++
>  5 files changed, 242 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/ti,tlk10x.txt
>  create mode 100644 drivers/net/phy/tlk10x.c
> 
> diff --git a/Documentation/devicetree/bindings/net/ti,tlk10x.txt b/Documentation/devicetree/bindings/net/ti,tlk10x.txt
> new file mode 100644
> index 0000000..371d0d7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/ti,tlk10x.txt
> @@ -0,0 +1,27 @@
> +* Texas Instruments - TLK105 / TLK106 ethernet PHYs
> +
> +Required properties:
> +	- reg - The ID number for the phy, usually a small integer

Isn't this the MDIO bus address?

This should have a compatible string too.

> +
> +Optional properties:
> +	- ti,power-back-off - Power Back Off Level
> +		Please refer to data sheet chapter 8.6 and TI Application
> +		Note SLLA3228
> +		0 - Normal Operation
> +		1 - Level 1 (up to 140m cable between TLK link partners)
> +		2 - Level 2 (up to 100m cable between TLK link partners)
> +		3 - Level 3 (up to 80m cable between TLK link partners)
> +
> +Default child nodes are standard Ethernet PHY device
> +nodes as described in Documentation/devicetree/bindings/net/phy.txt
> +
> +Example:
> +
> +	ethernet-phy@0 {
> +		reg = <0>;
> +		ti,power-back-off = <2>;
> +	};
> +
> +Datasheets and documentation can be found at:
> +http://www.ti.com/lit/gpn/tlk106
> +http://www.ti.com/lit/an/slla328/slla328.pdf

Move this to before the properties.

Rob

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: phy: TLK10X initial driver submission
  2018-04-19  8:28 [PATCH] net: phy: TLK10X initial driver submission Måns Andersson
                   ` (2 preceding siblings ...)
  2018-04-24 16:34 ` Rob Herring
@ 2018-04-24 16:52 ` Florian Fainelli
  3 siblings, 0 replies; 8+ messages in thread
From: Florian Fainelli @ 2018-04-24 16:52 UTC (permalink / raw)
  To: Måns Andersson, Rob Herring, Mark Rutland, Andrew Lunn,
	netdev, devicetree, linux-kernel



On 04/19/2018 01:28 AM, Måns Andersson wrote:
> From: Mans Andersson <mans.andersson@nibe.se>
> 
> Add suport for the TI TLK105 and TLK106 10/100Mbit ethernet phys.
> 
> In addition the TLK10X needs to be removed from DP83848 driver as the
> power back off support is added here for this device.

I would not think this is a compelling enough reason, you could very
well just adjust the dp83848.c driver just to account for these
properties that you are introducing. More comments below.

[snip]

> +#define TLK10X_INT_EN_MASK		\
> +	(TLK10X_MISR_ANC_INT_EN |	\
> +	 TLK10X_MISR_DUP_INT_EN |	\
> +	 TLK10X_MISR_SPD_INT_EN |	\
> +	 TLK10X_MISR_LINK_INT_EN)
> +
> +struct tlk10x_private {
> +	int pwrbo_level;

unsigned int

> +};
> +
> +static int tlk10x_read(struct phy_device *phydev, int reg)
> +{
> +	if (reg & ~0x1f) {
> +		/* Extended register */
> +		phy_write(phydev, TLK10X_REGCR, 0x001F);
> +		phy_write(phydev, TLK10X_ADDAR, reg);
> +		phy_write(phydev, TLK10X_REGCR, 0x401F);
> +		reg = TLK10X_ADDAR;
> +	}

Humm, this looks a bit fragile, you would likely want to create separate
helper functions for these extended registers and make sure you handle
write failures as well. Also consider making use of the page helpers
from include/linux/phy.h.

> +
> +	return phy_read(phydev, reg);
> +}
> +
> +static int tlk10x_write(struct phy_device *phydev, int reg, int val)
> +{
> +	if (reg & ~0x1f) {
> +		/* Extended register */
> +		phy_write(phydev, TLK10X_REGCR, 0x001F);
> +		phy_write(phydev, TLK10X_ADDAR, reg);
> +		phy_write(phydev, TLK10X_REGCR, 0x401F);
> +		reg = TLK10X_ADDAR;
> +	}

Same here.

> +
> +	return phy_write(phydev, reg, val);
> +}
> +
> +#ifdef CONFIG_OF_MDIO
> +static int tlk10x_of_init(struct phy_device *phydev)
> +{
> +	struct tlk10x_private *tlk10x = phydev->priv;
> +	struct device *dev = &phydev->mdio.dev;
> +	struct device_node *of_node = dev->of_node;
> +	int ret;
> +
> +	if (!of_node)
> +		return 0;
> +
> +	ret = of_property_read_u32(of_node, "ti,power-back-off",
> +				   &tlk10x->pwrbo_level);
> +	if (ret) {
> +		dev_err(dev, "missing ti,power-back-off property");
> +		tlk10x->pwrbo_level = 0;

This should not be necessary, that should be the default with a zero
initialized private data structure.

> +	}
> +
> +	return 0;
> +}
> +#else
> +static int tlk10x_of_init(struct phy_device *phydev)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_OF_MDIO */
> +
> +static int tlk10x_config_init(struct phy_device *phydev)
> +{
> +	int ret, reg;
> +	struct tlk10x_private *tlk10x;
> +
> +	ret = genphy_config_init(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!phydev->priv) {
> +		tlk10x = devm_kzalloc(&phydev->mdio.dev, sizeof(*tlk10x),
> +				      GFP_KERNEL);
> +		if (!tlk10x)
> +			return -ENOMEM;
> +
> +		phydev->priv = tlk10x;
> +		ret = tlk10x_of_init(phydev);
> +		if (ret)
> +			return ret;
> +	} else {
> +		tlk10x = (struct tlk10x_private *)phydev->priv;
> +	}

You need to implement a probe() function that is responsible for
allocation private memory instead of doing this check.

> +
> +	// Power back off
> +	if (tlk10x->pwrbo_level < 0 || tlk10x->pwrbo_level > 3)
> +		tlk10x->pwrbo_level = 0;

How can you have pwrb_level < 0 when you use of_read_property_u32()?

> +	reg = tlk10x_read(phydev, TLK10X_PWRBOCR);
> +	reg = ((reg & ~TLK10X_PWRBOCR_MASK)
> +		| (tlk10x->pwrbo_level << 6));

One too many levels of parenthesis, the outer ones should not be necessary.

> +	ret = tlk10x_write(phydev, TLK10X_PWRBOCR, reg);
> +	if (ret < 0) {
> +		dev_err(&phydev->mdio.dev,
> +			"unable to set power back-off (err=%d)\n", ret);
> +		return ret;
> +	}
> +	dev_info(&phydev->mdio.dev, "power back-off set to level %d\n",
> +		 tlk10x->pwrbo_level);

config_init() is called often, consider making this a debugging statement.

-- 
Florian

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: phy: TLK10X initial driver submission
  2018-04-24 16:34 ` Rob Herring
@ 2018-04-24 17:52   ` Andrew Lunn
  0 siblings, 0 replies; 8+ messages in thread
From: Andrew Lunn @ 2018-04-24 17:52 UTC (permalink / raw)
  To: Rob Herring
  Cc: Måns Andersson, Mark Rutland, Florian Fainelli, netdev,
	devicetree, linux-kernel

> > +Required properties:
> > +	- reg - The ID number for the phy, usually a small integer
> 
> Isn't this the MDIO bus address?

Hi Rob

Yes. This text has been take direct from the generic PHY binding.
 
> This should have a compatible string too.

Please see Documentation/devicetree/bindings/net/phy.txt

compatible strings are optional. We know what device it is from its ID
registers, which are in a well known location.

	Andrew

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-04-24 17:52 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-19  8:28 [PATCH] net: phy: TLK10X initial driver submission Måns Andersson
2018-04-19 12:09 ` Andrew Lunn
2018-04-20  6:58   ` Måns Andersson
2018-04-19 12:24 ` Miguel Ojeda
2018-04-20  7:07   ` Måns Andersson
2018-04-24 16:34 ` Rob Herring
2018-04-24 17:52   ` Andrew Lunn
2018-04-24 16:52 ` Florian Fainelli

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).