[PATCH v5 0/3] add clkout support to mscc phys

[PATCH v5 0/3] add clkout support to mscc phys

[Heiko Stuebner]

The main part of this series is adding handling of the clkout
controls some of the mscc phys have and while at it Andrew
asked for some of the duplicated probe functionality to be
factored out into a common function.

A working config on rockchip/rk3368-lion for example now looks like:

&gmac {
	status = "okay";

	mdio {
		compatible = "snps,dwmac-mdio";
		#address-cells = <1>;
		#size-cells = <0>;

		phy0: phy@0 {
			compatible = "ethernet-phy-id0007.0570";
			reg = <0>;
			assigned-clocks = <&phy0>, <&cru SCLK_MAC>;
			assigned-clock-rates = <125000000>, <125000000>;
			assigned-clock-parents = <0>, <&phy0>;
			clock-output-names = "ext_gmac";
			#clock-cells = <0>;
			vsc8531,edge-slowdown = <7>;
			vsc8531,led-0-mode = <1>;
			vsc8531,led-1-mode = <2>;
		};
	};
};


changes in v5:
- added Andrew's Rb for patch 1
- modified clkout handling to be a clock-provider
  to fit into the existing clock structures
changes in v4:
- fix missing variable initialization in one probe function
changes in v3:
- adapt to 5.8 merge-window results
- introduce a more generic enet-phy-property instead of
  using a vsc8531,* one - suggested by Andrew
changes in v2:
- new probe factoring patch as suggested by Andrew


Heiko Stuebner (3):
  net: phy: mscc: move shared probe code into a helper
  dt-bindings: net: mscc-vsc8531: add optional clock properties
  net: phy: mscc: handle the clkout control on some phy variants

 .../bindings/net/mscc-phy-vsc8531.txt         |   2 +
 drivers/net/phy/mscc/mscc.h                   |  13 +
 drivers/net/phy/mscc/mscc_main.c              | 306 ++++++++++++++----
 3 files changed, 250 insertions(+), 71 deletions(-)

-- 
2.26.2

[PATCH v5 1/3] net: phy: mscc: move shared probe code into a helper

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

The different probe functions share a lot of code, so move the
common parts into a helper to reduce duplication.

This moves the devm_phy_package_join below the general allocation
but as all components just allocate things, this should be ok.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/mscc/mscc_main.c | 124 +++++++++++++++----------------
 1 file changed, 61 insertions(+), 63 deletions(-)

diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 5ddc44f87eaf..5d2777522fb4 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -1935,12 +1935,11 @@ static int vsc85xx_read_status(struct phy_device *phydev)
 	return genphy_read_status(phydev);
 }
 
-static int vsc8514_probe(struct phy_device *phydev)
+static int vsc85xx_probe_helper(struct phy_device *phydev,
+				u32 *leds, int num_leds, u16 led_modes,
+				const struct vsc85xx_hw_stat *stats, int nstats)
 {
 	struct vsc8531_private *vsc8531;
-	u32 default_mode[4] = {VSC8531_LINK_1000_ACTIVITY,
-	   VSC8531_LINK_100_ACTIVITY, VSC8531_LINK_ACTIVITY,
-	   VSC8531_DUPLEX_COLLISION};
 
 	vsc8531 = devm_kzalloc(&phydev->mdio.dev, sizeof(*vsc8531), GFP_KERNEL);
 	if (!vsc8531)
@@ -1948,54 +1947,66 @@ static int vsc8514_probe(struct phy_device *phydev)
 
 	phydev->priv = vsc8531;
 
-	vsc8584_get_base_addr(phydev);
-	devm_phy_package_join(&phydev->mdio.dev, phydev,
-			      vsc8531->base_addr, 0);
-
-	vsc8531->nleds = 4;
-	vsc8531->supp_led_modes = VSC85XX_SUPP_LED_MODES;
-	vsc8531->hw_stats = vsc85xx_hw_stats;
-	vsc8531->nstats = ARRAY_SIZE(vsc85xx_hw_stats);
+	vsc8531->nleds = num_leds;
+	vsc8531->supp_led_modes = led_modes;
+	vsc8531->hw_stats = stats;
+	vsc8531->nstats = nstats;
 	vsc8531->stats = devm_kcalloc(&phydev->mdio.dev, vsc8531->nstats,
 				      sizeof(u64), GFP_KERNEL);
 	if (!vsc8531->stats)
 		return -ENOMEM;
 
-	return vsc85xx_dt_led_modes_get(phydev, default_mode);
+	return vsc85xx_dt_led_modes_get(phydev, leds);
 }
 
-static int vsc8574_probe(struct phy_device *phydev)
+static int vsc8514_probe(struct phy_device *phydev)
 {
 	struct vsc8531_private *vsc8531;
+	int rc;
 	u32 default_mode[4] = {VSC8531_LINK_1000_ACTIVITY,
 	   VSC8531_LINK_100_ACTIVITY, VSC8531_LINK_ACTIVITY,
 	   VSC8531_DUPLEX_COLLISION};
 
-	vsc8531 = devm_kzalloc(&phydev->mdio.dev, sizeof(*vsc8531), GFP_KERNEL);
-	if (!vsc8531)
-		return -ENOMEM;
-
-	phydev->priv = vsc8531;
+	rc = vsc85xx_probe_helper(phydev, default_mode,
+				  ARRAY_SIZE(default_mode),
+				  VSC85XX_SUPP_LED_MODES,
+				  vsc85xx_hw_stats,
+				  ARRAY_SIZE(vsc85xx_hw_stats));
+	if (rc < 0)
+		return rc;
 
+	vsc8531 = phydev->priv;
 	vsc8584_get_base_addr(phydev);
-	devm_phy_package_join(&phydev->mdio.dev, phydev,
-			      vsc8531->base_addr, 0);
+	return devm_phy_package_join(&phydev->mdio.dev, phydev,
+				     vsc8531->base_addr, 0);
+}
 
-	vsc8531->nleds = 4;
-	vsc8531->supp_led_modes = VSC8584_SUPP_LED_MODES;
-	vsc8531->hw_stats = vsc8584_hw_stats;
-	vsc8531->nstats = ARRAY_SIZE(vsc8584_hw_stats);
-	vsc8531->stats = devm_kcalloc(&phydev->mdio.dev, vsc8531->nstats,
-				      sizeof(u64), GFP_KERNEL);
-	if (!vsc8531->stats)
-		return -ENOMEM;
+static int vsc8574_probe(struct phy_device *phydev)
+{
+	struct vsc8531_private *vsc8531;
+	int rc;
+	u32 default_mode[4] = {VSC8531_LINK_1000_ACTIVITY,
+	   VSC8531_LINK_100_ACTIVITY, VSC8531_LINK_ACTIVITY,
+	   VSC8531_DUPLEX_COLLISION};
+
+	rc = vsc85xx_probe_helper(phydev, default_mode,
+				  ARRAY_SIZE(default_mode),
+				  VSC8584_SUPP_LED_MODES,
+				  vsc8584_hw_stats,
+				  ARRAY_SIZE(vsc8584_hw_stats));
+	if (rc < 0)
+		return rc;
 
-	return vsc85xx_dt_led_modes_get(phydev, default_mode);
+	vsc8531 = phydev->priv;
+	vsc8584_get_base_addr(phydev);
+	return devm_phy_package_join(&phydev->mdio.dev, phydev,
+				     vsc8531->base_addr, 0);
 }
 
 static int vsc8584_probe(struct phy_device *phydev)
 {
 	struct vsc8531_private *vsc8531;
+	int rc;
 	u32 default_mode[4] = {VSC8531_LINK_1000_ACTIVITY,
 	   VSC8531_LINK_100_ACTIVITY, VSC8531_LINK_ACTIVITY,
 	   VSC8531_DUPLEX_COLLISION};
@@ -2005,32 +2016,24 @@ static int vsc8584_probe(struct phy_device *phydev)
 		return -ENOTSUPP;
 	}
 
-	vsc8531 = devm_kzalloc(&phydev->mdio.dev, sizeof(*vsc8531), GFP_KERNEL);
-	if (!vsc8531)
-		return -ENOMEM;
-
-	phydev->priv = vsc8531;
+	rc = vsc85xx_probe_helper(phydev, default_mode,
+				  ARRAY_SIZE(default_mode),
+				  VSC8584_SUPP_LED_MODES,
+				  vsc8584_hw_stats,
+				  ARRAY_SIZE(vsc8584_hw_stats));
+	if (rc < 0)
+		return rc;
 
+	vsc8531 = phydev->priv;
 	vsc8584_get_base_addr(phydev);
-	devm_phy_package_join(&phydev->mdio.dev, phydev,
-			      vsc8531->base_addr, 0);
-
-	vsc8531->nleds = 4;
-	vsc8531->supp_led_modes = VSC8584_SUPP_LED_MODES;
-	vsc8531->hw_stats = vsc8584_hw_stats;
-	vsc8531->nstats = ARRAY_SIZE(vsc8584_hw_stats);
-	vsc8531->stats = devm_kcalloc(&phydev->mdio.dev, vsc8531->nstats,
-				      sizeof(u64), GFP_KERNEL);
-	if (!vsc8531->stats)
-		return -ENOMEM;
-
-	return vsc85xx_dt_led_modes_get(phydev, default_mode);
+	return devm_phy_package_join(&phydev->mdio.dev, phydev,
+				     vsc8531->base_addr, 0);
 }
 
 static int vsc85xx_probe(struct phy_device *phydev)
 {
 	struct vsc8531_private *vsc8531;
-	int rate_magic;
+	int rate_magic, rc;
 	u32 default_mode[2] = {VSC8531_LINK_1000_ACTIVITY,
 	   VSC8531_LINK_100_ACTIVITY};
 
@@ -2038,23 +2041,18 @@ static int vsc85xx_probe(struct phy_device *phydev)
 	if (rate_magic < 0)
 		return rate_magic;
 
-	vsc8531 = devm_kzalloc(&phydev->mdio.dev, sizeof(*vsc8531), GFP_KERNEL);
-	if (!vsc8531)
-		return -ENOMEM;
-
-	phydev->priv = vsc8531;
+	rc = vsc85xx_probe_helper(phydev, default_mode,
+				  ARRAY_SIZE(default_mode),
+				  VSC85XX_SUPP_LED_MODES,
+				  vsc85xx_hw_stats,
+				  ARRAY_SIZE(vsc85xx_hw_stats));
+	if (rc < 0)
+		return rc;
 
+	vsc8531 = phydev->priv;
 	vsc8531->rate_magic = rate_magic;
-	vsc8531->nleds = 2;
-	vsc8531->supp_led_modes = VSC85XX_SUPP_LED_MODES;
-	vsc8531->hw_stats = vsc85xx_hw_stats;
-	vsc8531->nstats = ARRAY_SIZE(vsc85xx_hw_stats);
-	vsc8531->stats = devm_kcalloc(&phydev->mdio.dev, vsc8531->nstats,
-				      sizeof(u64), GFP_KERNEL);
-	if (!vsc8531->stats)
-		return -ENOMEM;
 
-	return vsc85xx_dt_led_modes_get(phydev, default_mode);
+	return 0;
 }
 
 /* Microsemi VSC85xx PHYs */
-- 
2.26.2

[PATCH v5 2/3] dt-bindings: net: mscc-vsc8531: add optional clock properties

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

Some mscc ethernet phys have a configurable clock output, so describe the
generic properties to access them in devicetrees.

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
index 5ff37c68c941..67625ba27f53 100644
--- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
+++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
@@ -1,6 +1,8 @@
 * Microsemi - vsc8531 Giga bit ethernet phy
 
 Optional properties:
+- clock-output-names	: Name for the exposed clock output
+- #clock-cells		: should be 0
 - vsc8531,vddmac	: The vddmac in mV. Allowed values is listed
 			  in the first row of Table 1 (below).
 			  This property is only used in combination
-- 
2.26.2

Re: [PATCH v5 2/3] dt-bindings: net: mscc-vsc8531: add optional clock properties

[Florian Fainelli]

On 6/18/2020 5:11 AM, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> Some mscc ethernet phys have a configurable clock output, so describe the
> generic properties to access them in devicetrees.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> ---
>  Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> index 5ff37c68c941..67625ba27f53 100644
> --- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> +++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> @@ -1,6 +1,8 @@
>  * Microsemi - vsc8531 Giga bit ethernet phy
>  
>  Optional properties:
> +- clock-output-names	: Name for the exposed clock output
> +- #clock-cells		: should be 0
>  - vsc8531,vddmac	: The vddmac in mV. Allowed values is listed
>  			  in the first row of Table 1 (below).
>  			  This property is only used in combination
> 

With that approach, you also need to be careful as a driver writer to
ensure that you have at least probed the MDIO bus to ensure that the PHY
device has been created (and therefore it is available as a clock
provider) if that same Ethernet MAC is a consumer of that clock (which
it appears to be). Otherwise you may just never probe and be trapped in
a circular dependency.
-- 
Florian

Re: [PATCH v5 2/3] dt-bindings: net: mscc-vsc8531: add optional clock properties

[Heiko Stuebner]

Am Freitag, 19. Juni 2020, 07:01:58 CEST schrieb Florian Fainelli:
> 
> On 6/18/2020 5:11 AM, Heiko Stuebner wrote:
> > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > 
> > Some mscc ethernet phys have a configurable clock output, so describe the
> > generic properties to access them in devicetrees.
> > 
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > ---
> >  Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> > index 5ff37c68c941..67625ba27f53 100644
> > --- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> > +++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> > @@ -1,6 +1,8 @@
> >  * Microsemi - vsc8531 Giga bit ethernet phy
> >  
> >  Optional properties:
> > +- clock-output-names	: Name for the exposed clock output
> > +- #clock-cells		: should be 0
> >  - vsc8531,vddmac	: The vddmac in mV. Allowed values is listed
> >  			  in the first row of Table 1 (below).
> >  			  This property is only used in combination
> > 
> 
> With that approach, you also need to be careful as a driver writer to
> ensure that you have at least probed the MDIO bus to ensure that the PHY
> device has been created (and therefore it is available as a clock
> provider) if that same Ethernet MAC is a consumer of that clock (which
> it appears to be). Otherwise you may just never probe and be trapped in
> a circular dependency.

Yep - although without anything like this, the phy won't emit any clock
at all. Even when enabling the clock output in u-boot already, when the
kernel starts that config is lost,  so no existing board should break.


As you can see in the discussion about patch 3/3 the wanted solution
is not so clear cut as well. With Rob suggesting this clock-provider way
and Russell strongly encouraging taking a second look.

[My first iteration (till v4) was doing it like other phys by specifying
a property to just tell the phy what frequency to output]

I don't really have a preference for one or the other, so
maybe you can also give a vote over there ;-)

Heiko

[PATCH v5 3/3] net: phy: mscc: handle the clkout control on some phy variants

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

At least VSC8530/8531/8540/8541 contain a clock output that can emit
a predefined rate of 25, 50 or 125MHz.

This may then feed back into the network interface as source clock.
So expose a clock-provider from the phy using the common clock framework
to allow setting the rate.

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 drivers/net/phy/mscc/mscc.h      |  13 +++
 drivers/net/phy/mscc/mscc_main.c | 182 +++++++++++++++++++++++++++++--
 2 files changed, 187 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
index fbcee5fce7b2..94883dab5cc1 100644
--- a/drivers/net/phy/mscc/mscc.h
+++ b/drivers/net/phy/mscc/mscc.h
@@ -218,6 +218,13 @@ enum rgmii_clock_delay {
 #define INT_MEM_DATA_M			  0x00ff
 #define INT_MEM_DATA(x)			  (INT_MEM_DATA_M & (x))
 
+#define MSCC_CLKOUT_CNTL		  13
+#define CLKOUT_ENABLE			  BIT(15)
+#define CLKOUT_FREQ_MASK		  GENMASK(14, 13)
+#define CLKOUT_FREQ_25M			  (0x0 << 13)
+#define CLKOUT_FREQ_50M			  (0x1 << 13)
+#define CLKOUT_FREQ_125M		  (0x2 << 13)
+
 #define MSCC_PHY_PROC_CMD		  18
 #define PROC_CMD_NCOMPLETED		  0x8000
 #define PROC_CMD_FAILED			  0x4000
@@ -360,6 +367,12 @@ struct vsc8531_private {
 	 */
 	unsigned int base_addr;
 
+#ifdef CONFIG_COMMON_CLK
+	struct clk_hw clkout_hw;
+#endif
+	u32 clkout_rate;
+	int clkout_enabled;
+
 #if IS_ENABLED(CONFIG_MACSEC)
 	/* MACsec fields:
 	 * - One SecY per device (enforced at the s/w implementation level)
diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 5d2777522fb4..727a9dd58403 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -7,6 +7,7 @@
  * Copyright (c) 2016 Microsemi Corporation
  */
 
+#include <linux/clk-provider.h>
 #include <linux/firmware.h>
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
@@ -431,7 +432,6 @@ static int vsc85xx_dt_led_mode_get(struct phy_device *phydev,
 
 	return led_mode;
 }
-
 #else
 static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
 {
@@ -1508,6 +1508,43 @@ static int vsc85xx_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+static int vsc8531_config_init(struct phy_device *phydev)
+{
+	struct vsc8531_private *vsc8531 = phydev->priv;
+	u16 val;
+	int rc;
+
+	rc = vsc85xx_config_init(phydev);
+	if (rc)
+		return rc;
+
+#ifdef CONFIG_COMMON_CLK
+	switch (vsc8531->clkout_rate) {
+	case 25000000:
+		val = CLKOUT_FREQ_25M;
+		break;
+	case 50000000:
+		val = CLKOUT_FREQ_50M;
+		break;
+	case 125000000:
+		val = CLKOUT_FREQ_125M;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (vsc8531->clkout_enabled)
+		val |= CLKOUT_ENABLE;
+
+	rc = phy_write_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO,
+			     MSCC_CLKOUT_CNTL, val);
+	if (rc)
+		return rc;
+#endif
+
+	return 0;
+}
+
 static int vsc8584_did_interrupt(struct phy_device *phydev)
 {
 	int rc = 0;
@@ -1935,6 +1972,107 @@ static int vsc85xx_read_status(struct phy_device *phydev)
 	return genphy_read_status(phydev);
 }
 
+#ifdef CONFIG_COMMON_CLK
+#define clkout_hw_to_vsc8531(_hw) container_of(_hw, struct vsc8531_private, clkout_hw)
+
+static int clkout_rates[] = {
+	125000000,
+	50000000,
+	25000000,
+};
+
+static unsigned long vsc8531_clkout_recalc_rate(struct clk_hw *hw,
+						unsigned long parent_rate)
+{
+	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
+
+	return vsc8531->clkout_rate;
+}
+
+static long vsc8531_clkout_round_rate(struct clk_hw *hw, unsigned long rate,
+				      unsigned long *prate)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(clkout_rates); i++)
+		if (clkout_rates[i] <= rate)
+			return clkout_rates[i];
+	return 0;
+}
+
+static int vsc8531_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
+				   unsigned long parent_rate)
+{
+	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
+
+	vsc8531->clkout_rate = rate;
+	return 0;
+}
+
+static int vsc8531_clkout_prepare(struct clk_hw *hw)
+{
+	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
+
+	vsc8531->clkout_enabled = true;
+	return 0;
+}
+
+static void vsc8531_clkout_unprepare(struct clk_hw *hw)
+{
+	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
+
+	vsc8531->clkout_enabled = false;
+}
+
+static int vsc8531_clkout_is_prepared(struct clk_hw *hw)
+{
+	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
+
+	return vsc8531->clkout_enabled;
+}
+
+static const struct clk_ops vsc8531_clkout_ops = {
+	.prepare = vsc8531_clkout_prepare,
+	.unprepare = vsc8531_clkout_unprepare,
+	.is_prepared = vsc8531_clkout_is_prepared,
+	.recalc_rate = vsc8531_clkout_recalc_rate,
+	.round_rate = vsc8531_clkout_round_rate,
+	.set_rate = vsc8531_clkout_set_rate,
+};
+
+static int vsc8531_register_clkout(struct phy_device *phydev)
+{
+	struct vsc8531_private *vsc8531 = phydev->priv;
+	struct device *dev = &phydev->mdio.dev;
+	struct device_node *of_node = dev->of_node;
+	struct clk_init_data init;
+	int ret;
+
+	init.name = "vsc8531-clkout";
+	init.ops = &vsc8531_clkout_ops;
+	init.flags = 0;
+	init.parent_names = NULL;
+	init.num_parents = 0;
+	vsc8531->clkout_hw.init = &init;
+
+	/* optional override of the clockname */
+	of_property_read_string(of_node, "clock-output-names", &init.name);
+
+	/* register the clock */
+	ret = devm_clk_hw_register(dev, &vsc8531->clkout_hw);
+	if (!ret)
+		ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
+						  &vsc8531->clkout_hw);
+
+	return ret;
+}
+#else
+static int vsc8531_register_clkout(struct phy_device *phydev)
+{
+	return 0;
+}
+#endif
+
 static int vsc85xx_probe_helper(struct phy_device *phydev,
 				u32 *leds, int num_leds, u16 led_modes,
 				const struct vsc85xx_hw_stat *stats, int nstats)
@@ -1981,6 +2119,34 @@ static int vsc8514_probe(struct phy_device *phydev)
 				     vsc8531->base_addr, 0);
 }
 
+static int vsc8531_probe(struct phy_device *phydev)
+{
+	struct vsc8531_private *vsc8531;
+	int rate_magic, rc;
+	u32 default_mode[2] = {VSC8531_LINK_1000_ACTIVITY,
+	   VSC8531_LINK_100_ACTIVITY};
+
+	rate_magic = vsc85xx_edge_rate_magic_get(phydev);
+	if (rate_magic < 0)
+		return rate_magic;
+
+	rc = vsc85xx_probe_helper(phydev, default_mode,
+				  ARRAY_SIZE(default_mode),
+				  VSC85XX_SUPP_LED_MODES,
+				  vsc85xx_hw_stats,
+				  ARRAY_SIZE(vsc85xx_hw_stats));
+	if (rc < 0)
+		return rc;
+
+	vsc8531 = phydev->priv;
+	vsc8531->rate_magic = rate_magic;
+	rc = vsc8531_register_clkout(phydev);
+	if (rc < 0)
+		return rc;
+
+	return 0;
+}
+
 static int vsc8574_probe(struct phy_device *phydev)
 {
 	struct vsc8531_private *vsc8531;
@@ -2136,14 +2302,14 @@ static struct phy_driver vsc85xx_driver[] = {
 	.phy_id_mask	= 0xfffffff0,
 	/* PHY_BASIC_FEATURES */
 	.soft_reset	= &genphy_soft_reset,
-	.config_init	= &vsc85xx_config_init,
+	.config_init	= &vsc8531_config_init,
 	.config_aneg    = &vsc85xx_config_aneg,
 	.read_status	= &vsc85xx_read_status,
 	.ack_interrupt	= &vsc85xx_ack_interrupt,
 	.config_intr	= &vsc85xx_config_intr,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
-	.probe		= &vsc85xx_probe,
+	.probe		= &vsc8531_probe,
 	.set_wol	= &vsc85xx_wol_set,
 	.get_wol	= &vsc85xx_wol_get,
 	.get_tunable	= &vsc85xx_get_tunable,
@@ -2160,14 +2326,14 @@ static struct phy_driver vsc85xx_driver[] = {
 	.phy_id_mask    = 0xfffffff0,
 	/* PHY_GBIT_FEATURES */
 	.soft_reset	= &genphy_soft_reset,
-	.config_init    = &vsc85xx_config_init,
+	.config_init    = &vsc8531_config_init,
 	.config_aneg    = &vsc85xx_config_aneg,
 	.read_status	= &vsc85xx_read_status,
 	.ack_interrupt  = &vsc85xx_ack_interrupt,
 	.config_intr    = &vsc85xx_config_intr,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
-	.probe		= &vsc85xx_probe,
+	.probe		= &vsc8531_probe,
 	.set_wol	= &vsc85xx_wol_set,
 	.get_wol	= &vsc85xx_wol_get,
 	.get_tunable	= &vsc85xx_get_tunable,
@@ -2184,14 +2350,14 @@ static struct phy_driver vsc85xx_driver[] = {
 	.phy_id_mask	= 0xfffffff0,
 	/* PHY_BASIC_FEATURES */
 	.soft_reset	= &genphy_soft_reset,
-	.config_init	= &vsc85xx_config_init,
+	.config_init	= &vsc8531_config_init,
 	.config_aneg	= &vsc85xx_config_aneg,
 	.read_status	= &vsc85xx_read_status,
 	.ack_interrupt	= &vsc85xx_ack_interrupt,
 	.config_intr	= &vsc85xx_config_intr,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
-	.probe		= &vsc85xx_probe,
+	.probe		= &vsc8531_probe,
 	.set_wol	= &vsc85xx_wol_set,
 	.get_wol	= &vsc85xx_wol_get,
 	.get_tunable	= &vsc85xx_get_tunable,
@@ -2208,7 +2374,7 @@ static struct phy_driver vsc85xx_driver[] = {
 	.phy_id_mask    = 0xfffffff0,
 	/* PHY_GBIT_FEATURES */
 	.soft_reset	= &genphy_soft_reset,
-	.config_init    = &vsc85xx_config_init,
+	.config_init    = &vsc8531_config_init,
 	.config_aneg    = &vsc85xx_config_aneg,
 	.read_status	= &vsc85xx_read_status,
 	.ack_interrupt  = &vsc85xx_ack_interrupt,
-- 
2.26.2

Re: [PATCH v5 3/3] net: phy: mscc: handle the clkout control on some phy variants

[Andrew Lunn]

On Thu, Jun 18, 2020 at 02:11:39PM +0200, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> At least VSC8530/8531/8540/8541 contain a clock output that can emit
> a predefined rate of 25, 50 or 125MHz.
> 
> This may then feed back into the network interface as source clock.
> So expose a clock-provider from the phy using the common clock framework
> to allow setting the rate.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> ---
>  drivers/net/phy/mscc/mscc.h      |  13 +++
>  drivers/net/phy/mscc/mscc_main.c | 182 +++++++++++++++++++++++++++++--
>  2 files changed, 187 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
> index fbcee5fce7b2..94883dab5cc1 100644
> --- a/drivers/net/phy/mscc/mscc.h
> +++ b/drivers/net/phy/mscc/mscc.h
> @@ -218,6 +218,13 @@ enum rgmii_clock_delay {
>  #define INT_MEM_DATA_M			  0x00ff
>  #define INT_MEM_DATA(x)			  (INT_MEM_DATA_M & (x))
>  
> +#define MSCC_CLKOUT_CNTL		  13
> +#define CLKOUT_ENABLE			  BIT(15)
> +#define CLKOUT_FREQ_MASK		  GENMASK(14, 13)
> +#define CLKOUT_FREQ_25M			  (0x0 << 13)
> +#define CLKOUT_FREQ_50M			  (0x1 << 13)
> +#define CLKOUT_FREQ_125M		  (0x2 << 13)
> +
>  #define MSCC_PHY_PROC_CMD		  18
>  #define PROC_CMD_NCOMPLETED		  0x8000
>  #define PROC_CMD_FAILED			  0x4000
> @@ -360,6 +367,12 @@ struct vsc8531_private {
>  	 */
>  	unsigned int base_addr;
>  
> +#ifdef CONFIG_COMMON_CLK
> +	struct clk_hw clkout_hw;
> +#endif
> +	u32 clkout_rate;
> +	int clkout_enabled;
> +
>  #if IS_ENABLED(CONFIG_MACSEC)
>  	/* MACsec fields:
>  	 * - One SecY per device (enforced at the s/w implementation level)
> diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
> index 5d2777522fb4..727a9dd58403 100644
> --- a/drivers/net/phy/mscc/mscc_main.c
> +++ b/drivers/net/phy/mscc/mscc_main.c
> @@ -7,6 +7,7 @@
>   * Copyright (c) 2016 Microsemi Corporation
>   */
>  
> +#include <linux/clk-provider.h>
>  #include <linux/firmware.h>
>  #include <linux/jiffies.h>
>  #include <linux/kernel.h>
> @@ -431,7 +432,6 @@ static int vsc85xx_dt_led_mode_get(struct phy_device *phydev,
>  
>  	return led_mode;
>  }
> -
>  #else
>  static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
>  {
> @@ -1508,6 +1508,43 @@ static int vsc85xx_config_init(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int vsc8531_config_init(struct phy_device *phydev)
> +{
> +	struct vsc8531_private *vsc8531 = phydev->priv;
> +	u16 val;
> +	int rc;
> +
> +	rc = vsc85xx_config_init(phydev);
> +	if (rc)
> +		return rc;
> +
> +#ifdef CONFIG_COMMON_CLK
> +	switch (vsc8531->clkout_rate) {
> +	case 25000000:
> +		val = CLKOUT_FREQ_25M;
> +		break;
> +	case 50000000:
> +		val = CLKOUT_FREQ_50M;
> +		break;
> +	case 125000000:
> +		val = CLKOUT_FREQ_125M;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (vsc8531->clkout_enabled)
> +		val |= CLKOUT_ENABLE;
> +
> +	rc = phy_write_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO,
> +			     MSCC_CLKOUT_CNTL, val);
> +	if (rc)
> +		return rc;
> +#endif
> +
> +	return 0;
> +}
> +

> +static int vsc8531_clkout_prepare(struct clk_hw *hw)
> +{
> +	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
> +
> +	vsc8531->clkout_enabled = true;
> +	return 0;
> +}
> +
> +static void vsc8531_clkout_unprepare(struct clk_hw *hw)
> +{
> +	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
> +
> +	vsc8531->clkout_enabled = false;
> +}
> +

> +static const struct clk_ops vsc8531_clkout_ops = {
> +	.prepare = vsc8531_clkout_prepare,
> +	.unprepare = vsc8531_clkout_unprepare,
> +	.is_prepared = vsc8531_clkout_is_prepared,
> +	.recalc_rate = vsc8531_clkout_recalc_rate,
> +	.round_rate = vsc8531_clkout_round_rate,
> +	.set_rate = vsc8531_clkout_set_rate,

I'm not sure this is the expected behaviour. The clk itself should
only start ticking when the enable callback is called. But this code
will enable the clock when config_init() is called. I think you should
implement the enable and disable methods.

	  Andrew

Re: [PATCH v5 3/3] net: phy: mscc: handle the clkout control on some phy variants

[Russell King - ARM Linux admin]

On Thu, Jun 18, 2020 at 03:28:22PM +0200, Andrew Lunn wrote:
> On Thu, Jun 18, 2020 at 02:11:39PM +0200, Heiko Stuebner wrote:
> > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > 
> > At least VSC8530/8531/8540/8541 contain a clock output that can emit
> > a predefined rate of 25, 50 or 125MHz.
> > 
> > This may then feed back into the network interface as source clock.
> > So expose a clock-provider from the phy using the common clock framework
> > to allow setting the rate.
> > 
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > ---
> >  drivers/net/phy/mscc/mscc.h      |  13 +++
> >  drivers/net/phy/mscc/mscc_main.c | 182 +++++++++++++++++++++++++++++--
> >  2 files changed, 187 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
> > index fbcee5fce7b2..94883dab5cc1 100644
> > --- a/drivers/net/phy/mscc/mscc.h
> > +++ b/drivers/net/phy/mscc/mscc.h
> > @@ -218,6 +218,13 @@ enum rgmii_clock_delay {
> >  #define INT_MEM_DATA_M			  0x00ff
> >  #define INT_MEM_DATA(x)			  (INT_MEM_DATA_M & (x))
> >  
> > +#define MSCC_CLKOUT_CNTL		  13
> > +#define CLKOUT_ENABLE			  BIT(15)
> > +#define CLKOUT_FREQ_MASK		  GENMASK(14, 13)
> > +#define CLKOUT_FREQ_25M			  (0x0 << 13)
> > +#define CLKOUT_FREQ_50M			  (0x1 << 13)
> > +#define CLKOUT_FREQ_125M		  (0x2 << 13)
> > +
> >  #define MSCC_PHY_PROC_CMD		  18
> >  #define PROC_CMD_NCOMPLETED		  0x8000
> >  #define PROC_CMD_FAILED			  0x4000
> > @@ -360,6 +367,12 @@ struct vsc8531_private {
> >  	 */
> >  	unsigned int base_addr;
> >  
> > +#ifdef CONFIG_COMMON_CLK
> > +	struct clk_hw clkout_hw;
> > +#endif
> > +	u32 clkout_rate;
> > +	int clkout_enabled;
> > +
> >  #if IS_ENABLED(CONFIG_MACSEC)
> >  	/* MACsec fields:
> >  	 * - One SecY per device (enforced at the s/w implementation level)
> > diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
> > index 5d2777522fb4..727a9dd58403 100644
> > --- a/drivers/net/phy/mscc/mscc_main.c
> > +++ b/drivers/net/phy/mscc/mscc_main.c
> > @@ -7,6 +7,7 @@
> >   * Copyright (c) 2016 Microsemi Corporation
> >   */
> >  
> > +#include <linux/clk-provider.h>
> >  #include <linux/firmware.h>
> >  #include <linux/jiffies.h>
> >  #include <linux/kernel.h>
> > @@ -431,7 +432,6 @@ static int vsc85xx_dt_led_mode_get(struct phy_device *phydev,
> >  
> >  	return led_mode;
> >  }
> > -
> >  #else
> >  static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
> >  {
> > @@ -1508,6 +1508,43 @@ static int vsc85xx_config_init(struct phy_device *phydev)
> >  	return 0;
> >  }
> >  
> > +static int vsc8531_config_init(struct phy_device *phydev)
> > +{
> > +	struct vsc8531_private *vsc8531 = phydev->priv;
> > +	u16 val;
> > +	int rc;
> > +
> > +	rc = vsc85xx_config_init(phydev);
> > +	if (rc)
> > +		return rc;
> > +
> > +#ifdef CONFIG_COMMON_CLK
> > +	switch (vsc8531->clkout_rate) {
> > +	case 25000000:
> > +		val = CLKOUT_FREQ_25M;
> > +		break;
> > +	case 50000000:
> > +		val = CLKOUT_FREQ_50M;
> > +		break;
> > +	case 125000000:
> > +		val = CLKOUT_FREQ_125M;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (vsc8531->clkout_enabled)
> > +		val |= CLKOUT_ENABLE;
> > +
> > +	rc = phy_write_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO,
> > +			     MSCC_CLKOUT_CNTL, val);
> > +	if (rc)
> > +		return rc;
> > +#endif
> > +
> > +	return 0;
> > +}
> > +
> 
> > +static int vsc8531_clkout_prepare(struct clk_hw *hw)
> > +{
> > +	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
> > +
> > +	vsc8531->clkout_enabled = true;
> > +	return 0;
> > +}
> > +
> > +static void vsc8531_clkout_unprepare(struct clk_hw *hw)
> > +{
> > +	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
> > +
> > +	vsc8531->clkout_enabled = false;
> > +}
> > +
> 
> > +static const struct clk_ops vsc8531_clkout_ops = {
> > +	.prepare = vsc8531_clkout_prepare,
> > +	.unprepare = vsc8531_clkout_unprepare,
> > +	.is_prepared = vsc8531_clkout_is_prepared,
> > +	.recalc_rate = vsc8531_clkout_recalc_rate,
> > +	.round_rate = vsc8531_clkout_round_rate,
> > +	.set_rate = vsc8531_clkout_set_rate,
> 
> I'm not sure this is the expected behaviour. The clk itself should
> only start ticking when the enable callback is called. But this code
> will enable the clock when config_init() is called. I think you should
> implement the enable and disable methods.

That is actually incorrect.  The whole "prepare" vs "enable" difference
is that prepare can schedule, enable isn't permitted.  So, if you need
to sleep to enable the clock, then enabling the clock in the prepare
callback is the right thing to do.

However, the above driver just sets a flag, which only gets used when
the PHY's config_init method is called; that really doesn't seem to be
sane - the clock is available from the point that the PHY has been
probed, and it'll be expected that once the clock is published, it can
be made functional.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v5 3/3] net: phy: mscc: handle the clkout control on some phy variants

[Heiko Stübner]

Am Donnerstag, 18. Juni 2020, 15:41:02 CEST schrieb Russell King - ARM Linux admin:
> On Thu, Jun 18, 2020 at 03:28:22PM +0200, Andrew Lunn wrote:
> > On Thu, Jun 18, 2020 at 02:11:39PM +0200, Heiko Stuebner wrote:
> > > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > > 
> > > At least VSC8530/8531/8540/8541 contain a clock output that can emit
> > > a predefined rate of 25, 50 or 125MHz.
> > > 
> > > This may then feed back into the network interface as source clock.
> > > So expose a clock-provider from the phy using the common clock framework
> > > to allow setting the rate.
> > > 
> > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > > ---
> > >  drivers/net/phy/mscc/mscc.h      |  13 +++
> > >  drivers/net/phy/mscc/mscc_main.c | 182 +++++++++++++++++++++++++++++--
> > >  2 files changed, 187 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
> > > index fbcee5fce7b2..94883dab5cc1 100644
> > > --- a/drivers/net/phy/mscc/mscc.h
> > > +++ b/drivers/net/phy/mscc/mscc.h
> > > @@ -218,6 +218,13 @@ enum rgmii_clock_delay {
> > >  #define INT_MEM_DATA_M			  0x00ff
> > >  #define INT_MEM_DATA(x)			  (INT_MEM_DATA_M & (x))
> > >  
> > > +#define MSCC_CLKOUT_CNTL		  13
> > > +#define CLKOUT_ENABLE			  BIT(15)
> > > +#define CLKOUT_FREQ_MASK		  GENMASK(14, 13)
> > > +#define CLKOUT_FREQ_25M			  (0x0 << 13)
> > > +#define CLKOUT_FREQ_50M			  (0x1 << 13)
> > > +#define CLKOUT_FREQ_125M		  (0x2 << 13)
> > > +
> > >  #define MSCC_PHY_PROC_CMD		  18
> > >  #define PROC_CMD_NCOMPLETED		  0x8000
> > >  #define PROC_CMD_FAILED			  0x4000
> > > @@ -360,6 +367,12 @@ struct vsc8531_private {
> > >  	 */
> > >  	unsigned int base_addr;
> > >  
> > > +#ifdef CONFIG_COMMON_CLK
> > > +	struct clk_hw clkout_hw;
> > > +#endif
> > > +	u32 clkout_rate;
> > > +	int clkout_enabled;
> > > +
> > >  #if IS_ENABLED(CONFIG_MACSEC)
> > >  	/* MACsec fields:
> > >  	 * - One SecY per device (enforced at the s/w implementation level)
> > > diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
> > > index 5d2777522fb4..727a9dd58403 100644
> > > --- a/drivers/net/phy/mscc/mscc_main.c
> > > +++ b/drivers/net/phy/mscc/mscc_main.c
> > > @@ -7,6 +7,7 @@
> > >   * Copyright (c) 2016 Microsemi Corporation
> > >   */
> > >  
> > > +#include <linux/clk-provider.h>
> > >  #include <linux/firmware.h>
> > >  #include <linux/jiffies.h>
> > >  #include <linux/kernel.h>
> > > @@ -431,7 +432,6 @@ static int vsc85xx_dt_led_mode_get(struct phy_device *phydev,
> > >  
> > >  	return led_mode;
> > >  }
> > > -
> > >  #else
> > >  static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
> > >  {
> > > @@ -1508,6 +1508,43 @@ static int vsc85xx_config_init(struct phy_device *phydev)
> > >  	return 0;
> > >  }
> > >  
> > > +static int vsc8531_config_init(struct phy_device *phydev)
> > > +{
> > > +	struct vsc8531_private *vsc8531 = phydev->priv;
> > > +	u16 val;
> > > +	int rc;
> > > +
> > > +	rc = vsc85xx_config_init(phydev);
> > > +	if (rc)
> > > +		return rc;
> > > +
> > > +#ifdef CONFIG_COMMON_CLK
> > > +	switch (vsc8531->clkout_rate) {
> > > +	case 25000000:
> > > +		val = CLKOUT_FREQ_25M;
> > > +		break;
> > > +	case 50000000:
> > > +		val = CLKOUT_FREQ_50M;
> > > +		break;
> > > +	case 125000000:
> > > +		val = CLKOUT_FREQ_125M;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (vsc8531->clkout_enabled)
> > > +		val |= CLKOUT_ENABLE;
> > > +
> > > +	rc = phy_write_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO,
> > > +			     MSCC_CLKOUT_CNTL, val);
> > > +	if (rc)
> > > +		return rc;
> > > +#endif
> > > +
> > > +	return 0;
> > > +}
> > > +
> > 
> > > +static int vsc8531_clkout_prepare(struct clk_hw *hw)
> > > +{
> > > +	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
> > > +
> > > +	vsc8531->clkout_enabled = true;
> > > +	return 0;
> > > +}
> > > +
> > > +static void vsc8531_clkout_unprepare(struct clk_hw *hw)
> > > +{
> > > +	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
> > > +
> > > +	vsc8531->clkout_enabled = false;
> > > +}
> > > +
> > 
> > > +static const struct clk_ops vsc8531_clkout_ops = {
> > > +	.prepare = vsc8531_clkout_prepare,
> > > +	.unprepare = vsc8531_clkout_unprepare,
> > > +	.is_prepared = vsc8531_clkout_is_prepared,
> > > +	.recalc_rate = vsc8531_clkout_recalc_rate,
> > > +	.round_rate = vsc8531_clkout_round_rate,
> > > +	.set_rate = vsc8531_clkout_set_rate,
> > 
> > I'm not sure this is the expected behaviour. The clk itself should
> > only start ticking when the enable callback is called. But this code
> > will enable the clock when config_init() is called. I think you should
> > implement the enable and disable methods.
> 
> That is actually incorrect.  The whole "prepare" vs "enable" difference
> is that prepare can schedule, enable isn't permitted.  So, if you need
> to sleep to enable the clock, then enabling the clock in the prepare
> callback is the right thing to do.
> 
> However, the above driver just sets a flag, which only gets used when
> the PHY's config_init method is called; that really doesn't seem to be
> sane - the clock is available from the point that the PHY has been
> probed, and it'll be expected that once the clock is published, it can
> be made functional.

Though I'm not sure how this fits in the whole bringup of ethernet phys.
Like the phy is dependent on the underlying ethernet controller to
actually turn it on.

I guess we should check the phy-state and if it's not accessible, just
keep the values and if it's in a suitable state do the configuration.

Calling a vsc8531_config_clkout() from both the vsc8531_config_init()
as well as the clk_(un-)prepare  and clk_set_rate functions and being
protected by a check against phy_is_started() ?


Heiko

Re: [PATCH v5 3/3] net: phy: mscc: handle the clkout control on some phy variants

[Russell King - ARM Linux admin]

On Thu, Jun 18, 2020 at 05:41:54PM +0200, Heiko Stübner wrote:
> Am Donnerstag, 18. Juni 2020, 15:41:02 CEST schrieb Russell King - ARM Linux admin:
> > On Thu, Jun 18, 2020 at 03:28:22PM +0200, Andrew Lunn wrote:
> > > On Thu, Jun 18, 2020 at 02:11:39PM +0200, Heiko Stuebner wrote:
> > > > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > > > 
> > > > At least VSC8530/8531/8540/8541 contain a clock output that can emit
> > > > a predefined rate of 25, 50 or 125MHz.
> > > > 
> > > > This may then feed back into the network interface as source clock.
> > > > So expose a clock-provider from the phy using the common clock framework
> > > > to allow setting the rate.
> > > > 
> > > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > > > ---
> > > >  drivers/net/phy/mscc/mscc.h      |  13 +++
> > > >  drivers/net/phy/mscc/mscc_main.c | 182 +++++++++++++++++++++++++++++--
> > > >  2 files changed, 187 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
> > > > index fbcee5fce7b2..94883dab5cc1 100644
> > > > --- a/drivers/net/phy/mscc/mscc.h
> > > > +++ b/drivers/net/phy/mscc/mscc.h
> > > > @@ -218,6 +218,13 @@ enum rgmii_clock_delay {
> > > >  #define INT_MEM_DATA_M			  0x00ff
> > > >  #define INT_MEM_DATA(x)			  (INT_MEM_DATA_M & (x))
> > > >  
> > > > +#define MSCC_CLKOUT_CNTL		  13
> > > > +#define CLKOUT_ENABLE			  BIT(15)
> > > > +#define CLKOUT_FREQ_MASK		  GENMASK(14, 13)
> > > > +#define CLKOUT_FREQ_25M			  (0x0 << 13)
> > > > +#define CLKOUT_FREQ_50M			  (0x1 << 13)
> > > > +#define CLKOUT_FREQ_125M		  (0x2 << 13)
> > > > +
> > > >  #define MSCC_PHY_PROC_CMD		  18
> > > >  #define PROC_CMD_NCOMPLETED		  0x8000
> > > >  #define PROC_CMD_FAILED			  0x4000
> > > > @@ -360,6 +367,12 @@ struct vsc8531_private {
> > > >  	 */
> > > >  	unsigned int base_addr;
> > > >  
> > > > +#ifdef CONFIG_COMMON_CLK
> > > > +	struct clk_hw clkout_hw;
> > > > +#endif
> > > > +	u32 clkout_rate;
> > > > +	int clkout_enabled;
> > > > +
> > > >  #if IS_ENABLED(CONFIG_MACSEC)
> > > >  	/* MACsec fields:
> > > >  	 * - One SecY per device (enforced at the s/w implementation level)
> > > > diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
> > > > index 5d2777522fb4..727a9dd58403 100644
> > > > --- a/drivers/net/phy/mscc/mscc_main.c
> > > > +++ b/drivers/net/phy/mscc/mscc_main.c
> > > > @@ -7,6 +7,7 @@
> > > >   * Copyright (c) 2016 Microsemi Corporation
> > > >   */
> > > >  
> > > > +#include <linux/clk-provider.h>
> > > >  #include <linux/firmware.h>
> > > >  #include <linux/jiffies.h>
> > > >  #include <linux/kernel.h>
> > > > @@ -431,7 +432,6 @@ static int vsc85xx_dt_led_mode_get(struct phy_device *phydev,
> > > >  
> > > >  	return led_mode;
> > > >  }
> > > > -
> > > >  #else
> > > >  static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
> > > >  {
> > > > @@ -1508,6 +1508,43 @@ static int vsc85xx_config_init(struct phy_device *phydev)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static int vsc8531_config_init(struct phy_device *phydev)
> > > > +{
> > > > +	struct vsc8531_private *vsc8531 = phydev->priv;
> > > > +	u16 val;
> > > > +	int rc;
> > > > +
> > > > +	rc = vsc85xx_config_init(phydev);
> > > > +	if (rc)
> > > > +		return rc;
> > > > +
> > > > +#ifdef CONFIG_COMMON_CLK
> > > > +	switch (vsc8531->clkout_rate) {
> > > > +	case 25000000:
> > > > +		val = CLKOUT_FREQ_25M;
> > > > +		break;
> > > > +	case 50000000:
> > > > +		val = CLKOUT_FREQ_50M;
> > > > +		break;
> > > > +	case 125000000:
> > > > +		val = CLKOUT_FREQ_125M;
> > > > +		break;
> > > > +	default:
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	if (vsc8531->clkout_enabled)
> > > > +		val |= CLKOUT_ENABLE;
> > > > +
> > > > +	rc = phy_write_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO,
> > > > +			     MSCC_CLKOUT_CNTL, val);
> > > > +	if (rc)
> > > > +		return rc;
> > > > +#endif
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > 
> > > > +static int vsc8531_clkout_prepare(struct clk_hw *hw)
> > > > +{
> > > > +	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
> > > > +
> > > > +	vsc8531->clkout_enabled = true;
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void vsc8531_clkout_unprepare(struct clk_hw *hw)
> > > > +{
> > > > +	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
> > > > +
> > > > +	vsc8531->clkout_enabled = false;
> > > > +}
> > > > +
> > > 
> > > > +static const struct clk_ops vsc8531_clkout_ops = {
> > > > +	.prepare = vsc8531_clkout_prepare,
> > > > +	.unprepare = vsc8531_clkout_unprepare,
> > > > +	.is_prepared = vsc8531_clkout_is_prepared,
> > > > +	.recalc_rate = vsc8531_clkout_recalc_rate,
> > > > +	.round_rate = vsc8531_clkout_round_rate,
> > > > +	.set_rate = vsc8531_clkout_set_rate,
> > > 
> > > I'm not sure this is the expected behaviour. The clk itself should
> > > only start ticking when the enable callback is called. But this code
> > > will enable the clock when config_init() is called. I think you should
> > > implement the enable and disable methods.
> > 
> > That is actually incorrect.  The whole "prepare" vs "enable" difference
> > is that prepare can schedule, enable isn't permitted.  So, if you need
> > to sleep to enable the clock, then enabling the clock in the prepare
> > callback is the right thing to do.
> > 
> > However, the above driver just sets a flag, which only gets used when
> > the PHY's config_init method is called; that really doesn't seem to be
> > sane - the clock is available from the point that the PHY has been
> > probed, and it'll be expected that once the clock is published, it can
> > be made functional.
> 
> Though I'm not sure how this fits in the whole bringup of ethernet phys.
> Like the phy is dependent on the underlying ethernet controller to
> actually turn it on.
> 
> I guess we should check the phy-state and if it's not accessible, just
> keep the values and if it's in a suitable state do the configuration.
> 
> Calling a vsc8531_config_clkout() from both the vsc8531_config_init()
> as well as the clk_(un-)prepare  and clk_set_rate functions and being
> protected by a check against phy_is_started() ?

It sounds like it doesn't actually fit the clk API paradym then.  I
see that Rob suggested it, and from the DT point of view, it makes
complete sense, but then if the hardware can't actually be used in
the way the clk API expects it to be used, then there's a semantic
problem.

What is this clock used for?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v5 3/3] net: phy: mscc: handle the clkout control on some phy variants

[Heiko Stübner]

Am Donnerstag, 18. Juni 2020, 17:47:48 CEST schrieb Russell King - ARM Linux admin:
> On Thu, Jun 18, 2020 at 05:41:54PM +0200, Heiko Stübner wrote:
> > Am Donnerstag, 18. Juni 2020, 15:41:02 CEST schrieb Russell King - ARM Linux admin:
> > > On Thu, Jun 18, 2020 at 03:28:22PM +0200, Andrew Lunn wrote:
> > > > On Thu, Jun 18, 2020 at 02:11:39PM +0200, Heiko Stuebner wrote:
> > > > > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > > > > 
> > > > > At least VSC8530/8531/8540/8541 contain a clock output that can emit
> > > > > a predefined rate of 25, 50 or 125MHz.
> > > > > 
> > > > > This may then feed back into the network interface as source clock.
> > > > > So expose a clock-provider from the phy using the common clock framework
> > > > > to allow setting the rate.
> > > > > 
> > > > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > > > > ---
> > > > >  drivers/net/phy/mscc/mscc.h      |  13 +++
> > > > >  drivers/net/phy/mscc/mscc_main.c | 182 +++++++++++++++++++++++++++++--
> > > > >  2 files changed, 187 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
> > > > > index fbcee5fce7b2..94883dab5cc1 100644
> > > > > --- a/drivers/net/phy/mscc/mscc.h
> > > > > +++ b/drivers/net/phy/mscc/mscc.h
> > > > > @@ -218,6 +218,13 @@ enum rgmii_clock_delay {
> > > > >  #define INT_MEM_DATA_M			  0x00ff
> > > > >  #define INT_MEM_DATA(x)			  (INT_MEM_DATA_M & (x))
> > > > >  
> > > > > +#define MSCC_CLKOUT_CNTL		  13
> > > > > +#define CLKOUT_ENABLE			  BIT(15)
> > > > > +#define CLKOUT_FREQ_MASK		  GENMASK(14, 13)
> > > > > +#define CLKOUT_FREQ_25M			  (0x0 << 13)
> > > > > +#define CLKOUT_FREQ_50M			  (0x1 << 13)
> > > > > +#define CLKOUT_FREQ_125M		  (0x2 << 13)
> > > > > +
> > > > >  #define MSCC_PHY_PROC_CMD		  18
> > > > >  #define PROC_CMD_NCOMPLETED		  0x8000
> > > > >  #define PROC_CMD_FAILED			  0x4000
> > > > > @@ -360,6 +367,12 @@ struct vsc8531_private {
> > > > >  	 */
> > > > >  	unsigned int base_addr;
> > > > >  
> > > > > +#ifdef CONFIG_COMMON_CLK
> > > > > +	struct clk_hw clkout_hw;
> > > > > +#endif
> > > > > +	u32 clkout_rate;
> > > > > +	int clkout_enabled;
> > > > > +
> > > > >  #if IS_ENABLED(CONFIG_MACSEC)
> > > > >  	/* MACsec fields:
> > > > >  	 * - One SecY per device (enforced at the s/w implementation level)
> > > > > diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
> > > > > index 5d2777522fb4..727a9dd58403 100644
> > > > > --- a/drivers/net/phy/mscc/mscc_main.c
> > > > > +++ b/drivers/net/phy/mscc/mscc_main.c
> > > > > @@ -7,6 +7,7 @@
> > > > >   * Copyright (c) 2016 Microsemi Corporation
> > > > >   */
> > > > >  
> > > > > +#include <linux/clk-provider.h>
> > > > >  #include <linux/firmware.h>
> > > > >  #include <linux/jiffies.h>
> > > > >  #include <linux/kernel.h>
> > > > > @@ -431,7 +432,6 @@ static int vsc85xx_dt_led_mode_get(struct phy_device *phydev,
> > > > >  
> > > > >  	return led_mode;
> > > > >  }
> > > > > -
> > > > >  #else
> > > > >  static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
> > > > >  {
> > > > > @@ -1508,6 +1508,43 @@ static int vsc85xx_config_init(struct phy_device *phydev)
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > +static int vsc8531_config_init(struct phy_device *phydev)
> > > > > +{
> > > > > +	struct vsc8531_private *vsc8531 = phydev->priv;
> > > > > +	u16 val;
> > > > > +	int rc;
> > > > > +
> > > > > +	rc = vsc85xx_config_init(phydev);
> > > > > +	if (rc)
> > > > > +		return rc;
> > > > > +
> > > > > +#ifdef CONFIG_COMMON_CLK
> > > > > +	switch (vsc8531->clkout_rate) {
> > > > > +	case 25000000:
> > > > > +		val = CLKOUT_FREQ_25M;
> > > > > +		break;
> > > > > +	case 50000000:
> > > > > +		val = CLKOUT_FREQ_50M;
> > > > > +		break;
> > > > > +	case 125000000:
> > > > > +		val = CLKOUT_FREQ_125M;
> > > > > +		break;
> > > > > +	default:
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	if (vsc8531->clkout_enabled)
> > > > > +		val |= CLKOUT_ENABLE;
> > > > > +
> > > > > +	rc = phy_write_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO,
> > > > > +			     MSCC_CLKOUT_CNTL, val);
> > > > > +	if (rc)
> > > > > +		return rc;
> > > > > +#endif
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > 
> > > > > +static int vsc8531_clkout_prepare(struct clk_hw *hw)
> > > > > +{
> > > > > +	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
> > > > > +
> > > > > +	vsc8531->clkout_enabled = true;
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static void vsc8531_clkout_unprepare(struct clk_hw *hw)
> > > > > +{
> > > > > +	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
> > > > > +
> > > > > +	vsc8531->clkout_enabled = false;
> > > > > +}
> > > > > +
> > > > 
> > > > > +static const struct clk_ops vsc8531_clkout_ops = {
> > > > > +	.prepare = vsc8531_clkout_prepare,
> > > > > +	.unprepare = vsc8531_clkout_unprepare,
> > > > > +	.is_prepared = vsc8531_clkout_is_prepared,
> > > > > +	.recalc_rate = vsc8531_clkout_recalc_rate,
> > > > > +	.round_rate = vsc8531_clkout_round_rate,
> > > > > +	.set_rate = vsc8531_clkout_set_rate,
> > > > 
> > > > I'm not sure this is the expected behaviour. The clk itself should
> > > > only start ticking when the enable callback is called. But this code
> > > > will enable the clock when config_init() is called. I think you should
> > > > implement the enable and disable methods.
> > > 
> > > That is actually incorrect.  The whole "prepare" vs "enable" difference
> > > is that prepare can schedule, enable isn't permitted.  So, if you need
> > > to sleep to enable the clock, then enabling the clock in the prepare
> > > callback is the right thing to do.
> > > 
> > > However, the above driver just sets a flag, which only gets used when
> > > the PHY's config_init method is called; that really doesn't seem to be
> > > sane - the clock is available from the point that the PHY has been
> > > probed, and it'll be expected that once the clock is published, it can
> > > be made functional.
> > 
> > Though I'm not sure how this fits in the whole bringup of ethernet phys.
> > Like the phy is dependent on the underlying ethernet controller to
> > actually turn it on.
> > 
> > I guess we should check the phy-state and if it's not accessible, just
> > keep the values and if it's in a suitable state do the configuration.
> > 
> > Calling a vsc8531_config_clkout() from both the vsc8531_config_init()
> > as well as the clk_(un-)prepare  and clk_set_rate functions and being
> > protected by a check against phy_is_started() ?
> 
> It sounds like it doesn't actually fit the clk API paradym then.  I
> see that Rob suggested it, and from the DT point of view, it makes
> complete sense, but then if the hardware can't actually be used in
> the way the clk API expects it to be used, then there's a semantic
> problem.
> 
> What is this clock used for?

It provides a source for the mac-clk for the actual transfers, here to
provide the 125MHz clock needed for the RGMII interface .

So right now the old rk3368-lion devicetree just declares a stub
fixed-clock and instructs the soc's clock controller to use it [0] .
And in the cover-letter here, I show the update variant with using
the clock defined here.


I've added the idea from my previous mail like shown below [1].
which would take into account the phy-state.

But I guess I'll wait for more input before spamming people with v6.

Thanks
Heiko


[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3368-lion.dtsi#n150
[1]
@@ -1508,6 +1508,157 @@ static int vsc85xx_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+#ifdef CONFIG_COMMON_CLK
+#define clkout_hw_to_vsc8531(_hw) container_of(_hw, struct vsc8531_private, clkout_hw)
+
+static int clkout_rates[] = {
+	125000000,
+	50000000,
+	25000000,
+};
+
+static int vsc8531_config_clkout(struct phy_device *phydev)
+{
+	struct vsc8531_private *vsc8531 = phydev->priv;
+	u16 val;
+
+	/* when called from clk functions, make sure phy is running */
+	if (phy_is_started(phydev))
+		return 0;
+
+	switch (vsc8531->clkout_rate) {
+	case 25000000:
+		val = CLKOUT_FREQ_25M;
+		break;
+	case 50000000:
+		val = CLKOUT_FREQ_50M;
+		break;
+	case 125000000:
+		val = CLKOUT_FREQ_125M;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (vsc8531->clkout_enabled)
+		val |= CLKOUT_ENABLE;
+
+	return phy_write_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO,
+			       MSCC_CLKOUT_CNTL, val);
+}
+
+static unsigned long vsc8531_clkout_recalc_rate(struct clk_hw *hw,
+						unsigned long parent_rate)
+{
+	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
+
+	return vsc8531->clkout_rate;
+}
+
+static long vsc8531_clkout_round_rate(struct clk_hw *hw, unsigned long rate,
+				      unsigned long *prate)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(clkout_rates); i++)
+		if (clkout_rates[i] <= rate)
+			return clkout_rates[i];
+	return 0;
+}
+
+static int vsc8531_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
+				   unsigned long parent_rate)
+{
+	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
+	struct phy_device *phydev = vsc8531->phydev;
+
+	vsc8531->clkout_rate = rate;
+	return vsc8531_config_clkout(phydev);
+}
+
+static int vsc8531_clkout_prepare(struct clk_hw *hw)
+{
+	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
+	struct phy_device *phydev = vsc8531->phydev;
+
+	vsc8531->clkout_enabled = true;
+	return vsc8531_config_clkout(phydev);
+}
+
+static void vsc8531_clkout_unprepare(struct clk_hw *hw)
+{
+	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
+	struct phy_device *phydev = vsc8531->phydev;
+
+	vsc8531->clkout_enabled = false;
+	vsc8531_config_clkout(phydev);
+}
+
+static int vsc8531_clkout_is_prepared(struct clk_hw *hw)
+{
+	struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
+
+	return vsc8531->clkout_enabled;
+}
+
+static const struct clk_ops vsc8531_clkout_ops = {
+	.prepare = vsc8531_clkout_prepare,
+	.unprepare = vsc8531_clkout_unprepare,
+	.is_prepared = vsc8531_clkout_is_prepared,
+	.recalc_rate = vsc8531_clkout_recalc_rate,
+	.round_rate = vsc8531_clkout_round_rate,
+	.set_rate = vsc8531_clkout_set_rate,
+};
+
+static int vsc8531_register_clkout(struct phy_device *phydev)
+{
+	struct vsc8531_private *vsc8531 = phydev->priv;
+	struct device *dev = &phydev->mdio.dev;
+	struct device_node *of_node = dev->of_node;
+	struct clk_init_data init;
+	int ret;
+
+	init.name = "vsc8531-clkout";
+	init.ops = &vsc8531_clkout_ops;
+	init.flags = 0;
+	init.parent_names = NULL;
+	init.num_parents = 0;
+	vsc8531->clkout_hw.init = &init;
+
+	/* optional override of the clockname */
+	of_property_read_string(of_node, "clock-output-names", &init.name);
+
+	/* register the clock */
+	ret = devm_clk_hw_register(dev, &vsc8531->clkout_hw);
+	if (!ret)
+		ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
+						  &vsc8531->clkout_hw);
+
+	return ret;
+}
+#else
+static int vsc8531_register_clkout(struct phy_device *phydev)
+{
+	return 0;
+}
+
+static int vsc8531_config_clkout(struct phy_device *phydev)
+{
+	return 0;
+}
+#endif
+
+static int vsc8531_config_init(struct phy_device *phydev)
+{
+	int rc;
+
+	rc = vsc85xx_config_init(phydev);
+	if (rc)
+		return rc;
+
+	return vsc8531_config_clkout(phydev);
+}
+

Re: [PATCH v5 3/3] net: phy: mscc: handle the clkout control on some phy variants

[Russell King - ARM Linux admin]

On Thu, Jun 18, 2020 at 06:01:29PM +0200, Heiko Stübner wrote:
> Am Donnerstag, 18. Juni 2020, 17:47:48 CEST schrieb Russell King - ARM Linux admin:
> > On Thu, Jun 18, 2020 at 05:41:54PM +0200, Heiko Stübner wrote:
> > > Though I'm not sure how this fits in the whole bringup of ethernet phys.
> > > Like the phy is dependent on the underlying ethernet controller to
> > > actually turn it on.
> > > 
> > > I guess we should check the phy-state and if it's not accessible, just
> > > keep the values and if it's in a suitable state do the configuration.
> > > 
> > > Calling a vsc8531_config_clkout() from both the vsc8531_config_init()
> > > as well as the clk_(un-)prepare  and clk_set_rate functions and being
> > > protected by a check against phy_is_started() ?
> > 
> > It sounds like it doesn't actually fit the clk API paradym then.  I
> > see that Rob suggested it, and from the DT point of view, it makes
> > complete sense, but then if the hardware can't actually be used in
> > the way the clk API expects it to be used, then there's a semantic
> > problem.
> > 
> > What is this clock used for?
> 
> It provides a source for the mac-clk for the actual transfers, here to
> provide the 125MHz clock needed for the RGMII interface .
> 
> So right now the old rk3368-lion devicetree just declares a stub
> fixed-clock and instructs the soc's clock controller to use it [0] .
> And in the cover-letter here, I show the update variant with using
> the clock defined here.
> 
> 
> I've added the idea from my previous mail like shown below [1].
> which would take into account the phy-state.
> 
> But I guess I'll wait for more input before spamming people with v6.

Let's get a handle on exactly what this is.

The RGMII bus has two clocks: RXC and TXC, which are clocked at one
of 125MHz, 25MHz or 2.5MHz depending on the RGMII data rate.  Some
PHYs replace TXC with GTX clock, which always runs at 125MHz.  These
clocks are not what you're referring to.

You are referring to another commonly provided clock between the MAC
and the PHY, something which is not unique to your PHY.

We seem to be heading down a path where different PHYs end up doing
different things in DT for what is basically the same hardware setup,
which really isn't good. :(

We have at803x using:

qca,clk-out-frequency
qca,clk-out-strength
qca,keep-pll-enabled

which are used to control the CLK_25M output pin on the device, which
may be used to provide a reference clock for the MAC side, selecting
between 25M, 50M, 62.5M and 125MHz.  This was introduced in November
2019, so not that long ago.

Broadcom PHYs configure their 125MHz clock through the PHY device
flags passed from the MAC at attach/connect time.

There's the dp83867 and dp83869 configuration (I'm not sure I can
make sense of it from reading the code) using ti,clk-output-sel -
but it looks like it's the same kind of thing.  Introduced February
2018 into one driver, and November 2019 in the other.

It seems the Micrel PHYs produce a 125MHz clock irrespective of any
configuration (maybe configured by firmware, or hardware strapping?)

So it seems we have four ways of doing the same thing today, and now
the suggestion is to implement a fifth different way.  I think there
needs to be some consolidation here, maybe choosing one approach and
sticking with it.

Hence, I disagree with Rob - we don't need a fifth approach, we need
to choose one approach and decide that's our policy for this and
apply it evenly across the board, rather than making up something
different each time a new PHY comes along.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v5 3/3] net: phy: mscc: handle the clkout control on some phy variants

[Heiko Stübner]

Am Donnerstag, 18. Juni 2020, 18:40:15 CEST schrieb Russell King - ARM Linux admin:
> On Thu, Jun 18, 2020 at 06:01:29PM +0200, Heiko Stübner wrote:
> > Am Donnerstag, 18. Juni 2020, 17:47:48 CEST schrieb Russell King - ARM Linux admin:
> > > On Thu, Jun 18, 2020 at 05:41:54PM +0200, Heiko Stübner wrote:
> > > > Though I'm not sure how this fits in the whole bringup of ethernet phys.
> > > > Like the phy is dependent on the underlying ethernet controller to
> > > > actually turn it on.
> > > > 
> > > > I guess we should check the phy-state and if it's not accessible, just
> > > > keep the values and if it's in a suitable state do the configuration.
> > > > 
> > > > Calling a vsc8531_config_clkout() from both the vsc8531_config_init()
> > > > as well as the clk_(un-)prepare  and clk_set_rate functions and being
> > > > protected by a check against phy_is_started() ?
> > > 
> > > It sounds like it doesn't actually fit the clk API paradym then.  I
> > > see that Rob suggested it, and from the DT point of view, it makes
> > > complete sense, but then if the hardware can't actually be used in
> > > the way the clk API expects it to be used, then there's a semantic
> > > problem.
> > > 
> > > What is this clock used for?
> > 
> > It provides a source for the mac-clk for the actual transfers, here to
> > provide the 125MHz clock needed for the RGMII interface .
> > 
> > So right now the old rk3368-lion devicetree just declares a stub
> > fixed-clock and instructs the soc's clock controller to use it [0] .
> > And in the cover-letter here, I show the update variant with using
> > the clock defined here.
> > 
> > 
> > I've added the idea from my previous mail like shown below [1].
> > which would take into account the phy-state.
> > 
> > But I guess I'll wait for more input before spamming people with v6.
> 
> Let's get a handle on exactly what this is.
> 
> The RGMII bus has two clocks: RXC and TXC, which are clocked at one
> of 125MHz, 25MHz or 2.5MHz depending on the RGMII data rate.  Some
> PHYs replace TXC with GTX clock, which always runs at 125MHz.  These
> clocks are not what you're referring to.
> 
> You are referring to another commonly provided clock between the MAC
> and the PHY, something which is not unique to your PHY.
> 
> We seem to be heading down a path where different PHYs end up doing
> different things in DT for what is basically the same hardware setup,
> which really isn't good. :(
> 
> We have at803x using:
> 
> qca,clk-out-frequency
> qca,clk-out-strength
> qca,keep-pll-enabled
> 
> which are used to control the CLK_25M output pin on the device, which
> may be used to provide a reference clock for the MAC side, selecting
> between 25M, 50M, 62.5M and 125MHz.  This was introduced in November
> 2019, so not that long ago.

Because it was not that old, was the reason I followed that example in
my v1 :-) 

And Andrew then suggested to at least try to use a common property
for the target frequency that other phys could migrate to.


> Broadcom PHYs configure their 125MHz clock through the PHY device
> flags passed from the MAC at attach/connect time.
> 
> There's the dp83867 and dp83869 configuration (I'm not sure I can
> make sense of it from reading the code) using ti,clk-output-sel -
> but it looks like it's the same kind of thing.  Introduced February
> 2018 into one driver, and November 2019 in the other.
> 
> It seems the Micrel PHYs produce a 125MHz clock irrespective of any
> configuration (maybe configured by firmware, or hardware strapping?)
> 
> So it seems we have four ways of doing the same thing today, and now
> the suggestion is to implement a fifth different way.  I think there
> needs to be some consolidation here, maybe choosing one approach and
> sticking with it.
> 
> Hence, I disagree with Rob - we don't need a fifth approach, we need
> to choose one approach and decide that's our policy for this and
> apply it evenly across the board, rather than making up something
> different each time a new PHY comes along.

@Dave, @Andrew: what's you opinion here?

As Russell above (and Florian in the binding patch) pointed out,
integrating this into the clock-framework may prove difficult not only
for consistency but also for dependency reasons.

Personally I'm fine with either solution, I just want to achieve a working
ethernet on my board :-D .


Thanks
Heiko

Re: [PATCH v5 3/3] net: phy: mscc: handle the clkout control on some phy variants

[Jakub Kicinski]

On Thu, 18 Jun 2020 14:11:39 +0200 Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> At least VSC8530/8531/8540/8541 contain a clock output that can emit
> a predefined rate of 25, 50 or 125MHz.
> 
> This may then feed back into the network interface as source clock.
> So expose a clock-provider from the phy using the common clock framework
> to allow setting the rate.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

Doesn't build with allmodconfig:

../drivers/net/phy/mscc/mscc_macsec.c:391:42: warning: cast from restricted sci_t
../drivers/net/phy/mscc/mscc_macsec.c:393:42: warning: restricted sci_t degrades to integer
../drivers/net/phy/mscc/mscc_macsec.c:400:42: warning: restricted __be16 degrades to integer
../drivers/net/phy/mscc/mscc_macsec.c:606:34: warning: cast from restricted sci_t
../drivers/net/phy/mscc/mscc_macsec.c:608:34: warning: restricted sci_t degrades to integer
../drivers/net/phy/mscc/mscc_macsec.c:391:42: warning: cast from restricted sci_t
../drivers/net/phy/mscc/mscc_macsec.c:393:42: warning: restricted sci_t degrades to integer
../drivers/net/phy/mscc/mscc_macsec.c:400:42: warning: restricted __be16 degrades to integer
../drivers/net/phy/mscc/mscc_macsec.c:606:34: warning: cast from restricted sci_t
../drivers/net/phy/mscc/mscc_macsec.c:608:34: warning: restricted sci_t degrades to integer
In file included from ../drivers/net/phy/mscc/mscc_macsec.c:17:
../drivers/net/phy/mscc/mscc.h:371:16: error: field ‘clkout_hw’ has incomplete type
  371 |  struct clk_hw clkout_hw;
      |                ^~~~~~~~~
make[5]: *** [drivers/net/phy/mscc/mscc_macsec.o] Error 1
make[5]: *** Waiting for unfinished jobs....
make[4]: *** [drivers/net/phy/mscc] Error 2
make[3]: *** [drivers/net/phy] Error 2
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [drivers/net] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [drivers] Error 2
make: *** [__sub-make] Error 2

Re: [PATCH v5 3/3] net: phy: mscc: handle the clkout control on some phy variants

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2421 bytes --]

Hi Heiko,

I love your patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on sparc-next/master net-next/master net/master linus/master v5.8-rc1 next-20200618]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Heiko-Stuebner/add-clkout-support-to-mscc-phys/20200618-201732
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 487ca07fcc75d52755c9fe2ee05bcb3b6eeeec44)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/net/phy/mscc/mscc_macsec.c:17:
>> drivers/net/phy/mscc/mscc.h:371:16: error: field has incomplete type 'struct clk_hw'
           struct clk_hw clkout_hw;
                         ^
   drivers/net/phy/mscc/mscc.h:371:9: note: forward declaration of 'struct clk_hw'
           struct clk_hw clkout_hw;
                  ^
   1 error generated.

vim +371 drivers/net/phy/mscc/mscc.h

   354	
   355	struct vsc8531_private {
   356		int rate_magic;
   357		u16 supp_led_modes;
   358		u32 leds_mode[MAX_LEDS];
   359		u8 nleds;
   360		const struct vsc85xx_hw_stat *hw_stats;
   361		u64 *stats;
   362		int nstats;
   363		/* PHY address within the package. */
   364		u8 addr;
   365		/* For multiple port PHYs; the MDIO address of the base PHY in the
   366		 * package.
   367		 */
   368		unsigned int base_addr;
   369	
   370	#ifdef CONFIG_COMMON_CLK
 > 371		struct clk_hw clkout_hw;
   372	#endif
   373		u32 clkout_rate;
   374		int clkout_enabled;
   375	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 75292 bytes --]