[PATCH v2 1/2 net-next] net: phy: mscc: add support for CLKOUT ctrl reg for VSC8531 and similar

[PATCH v2 1/2 net-next] net: phy: mscc: add support for CLKOUT ctrl reg for VSC8531 and similar

[Alexandru Ardelean]

The VSC8531 and similar PHYs (i.e. VSC8530, VSC8531, VSC8540 & VSC8541)
have a CLKOUT pin on the chip that can be controlled by register (13G in
the General Purpose Registers page) that can be configured to output a
frequency of 25, 50 or 125 Mhz.

This is useful when wanting to provide a clock source for the MAC in some
board designs.

Signed-off-by: Marius Muresan <marius.muresan@mxt.ro>
Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
---

Changelog v1 -> v2:
* https://lore.kernel.org/netdev/20230706081554.1616839-1-alex@shruggie.ro/
* changed property name 'vsc8531,clkout-freq-mhz' -> 'mscc,clkout-freq-mhz'
  as requested by Rob
* introduced 'goto set_reg' to reduce indentation (no idea why I did not
  think of that sooner)
* added 'net-next' tag as requested by Andrew

 drivers/net/phy/mscc/mscc.h      |  5 ++++
 drivers/net/phy/mscc/mscc_main.c | 41 ++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
index 7a962050a4d4..4ea21921a7ba 100644
--- a/drivers/net/phy/mscc/mscc.h
+++ b/drivers/net/phy/mscc/mscc.h
@@ -181,6 +181,11 @@ enum rgmii_clock_delay {
 #define VSC8502_RGMII_TX_DELAY_MASK	  0x0007
 #define VSC8502_RGMII_RX_CLK_DISABLE	  0x0800
 
+/* CKLOUT Control register, for VSC8531 and similar */
+#define VSC8531_CLKOUT_CNTL		  13
+#define VSC8531_CLKOUT_CNTL_ENABLE	  BIT(15)
+#define VSC8531_CLKOUT_CNTL_FREQ_MASK	  GENMASK(14, 13)
+
 #define MSCC_PHY_WOL_LOWER_MAC_ADDR	  21
 #define MSCC_PHY_WOL_MID_MAC_ADDR	  22
 #define MSCC_PHY_WOL_UPPER_MAC_ADDR	  23
diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 4171f01d34e5..ec029d26071d 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -618,6 +618,42 @@ static void vsc85xx_tr_write(struct phy_device *phydev, u16 addr, u32 val)
 	__phy_write(phydev, MSCC_PHY_TR_CNTL, TR_WRITE | TR_ADDR(addr));
 }
 
+static int vsc8531_clkout_config(struct phy_device *phydev)
+{
+	static const u32 freq_vals[] = { 25, 50, 125 };
+	struct device *dev = &phydev->mdio.dev;
+	u16 mask, set;
+	u32 freq, i;
+	int rc;
+
+	mask = VSC8531_CLKOUT_CNTL_ENABLE | VSC8531_CLKOUT_CNTL_FREQ_MASK;
+	set = 0;
+
+	if (device_property_read_u32(dev, "mscc,clkout-freq-mhz", &freq))
+		goto set_reg;
+
+	/* The indices from 'freq_vals' are used in the register */
+	for (i = 0; i < ARRAY_SIZE(freq_vals); i++) {
+		if (freq != freq_vals[i])
+			continue;
+
+		set |= VSC8531_CLKOUT_CNTL_ENABLE |
+		       FIELD_PREP(VSC8531_CLKOUT_CNTL_FREQ_MASK, i);
+		break;
+	}
+	if (set == 0)
+		dev_warn(dev, "Invalid 'mscc,clkout-freq-mhz' value %u\n",
+			 freq);
+
+set_reg:
+	mutex_lock(&phydev->lock);
+	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO,
+			      VSC8531_CLKOUT_CNTL, mask, set);
+	mutex_unlock(&phydev->lock);
+
+	return rc;
+}
+
 static int vsc8531_pre_init_seq_set(struct phy_device *phydev)
 {
 	int rc;
@@ -1852,6 +1888,11 @@ static int vsc85xx_config_init(struct phy_device *phydev)
 		rc = vsc8531_pre_init_seq_set(phydev);
 		if (rc)
 			return rc;
+
+		rc = vsc8531_clkout_config(phydev);
+		if (rc)
+			return rc;
+
 	}
 
 	rc = vsc85xx_eee_init_seq_set(phydev);
-- 
2.41.0

[PATCH v2 2/2] dt-bindings: net: phy: vsc8531: document 'vsc8531,clkout-freq-mhz' property

[Alexandru Ardelean]

For VSC8351 and similar PHYs, a new property was added to generate a clock
signal on the CLKOUT pin.
This change documents the change in the device-tree bindings doc.

Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
---

Changelog v1 -> v2:
* https://lore.kernel.org/netdev/20230706081554.1616839-2-alex@shruggie.ro/
* changed property name 'vsc8531,clkout-freq-mhz' -> 'mscc,clkout-freq-mhz'
  as requested by Rob
* added 'net-next' tag as requested by Andrew

 Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
index 0a3647fe331b..085d0e8a834e 100644
--- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
+++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
@@ -31,6 +31,10 @@ Optional properties:
 			  VSC8531_LINK_100_ACTIVITY (2),
 			  VSC8531_LINK_ACTIVITY (0) and
 			  VSC8531_DUPLEX_COLLISION (8).
+- mscc,clkout-freq-mhz	: For VSC8531 and similar PHYs, this will output
+			  a clock signal on the CLKOUT pin of the chip.
+			  The supported values are 25, 50 & 125 Mhz.
+			  Default value is no clock signal on the CLKOUT pin.
 - load-save-gpios	: GPIO used for the load/save operation of the PTP
 			  hardware clock (PHC).
 
@@ -69,5 +73,6 @@ Example:
                 vsc8531,edge-slowdown	= <7>;
                 vsc8531,led-0-mode	= <VSC8531_LINK_1000_ACTIVITY>;
                 vsc8531,led-1-mode	= <VSC8531_LINK_100_ACTIVITY>;
+                mscc,clkout-freq-mhz	= <50>;
 		load-save-gpios		= <&gpio 10 GPIO_ACTIVE_HIGH>;
         };
-- 
2.41.0

Re: [PATCH v2 2/2] dt-bindings: net: phy: vsc8531: document 'vsc8531,clkout-freq-mhz' property

[Rob Herring]

On Thu, Jul 13, 2023 at 11:21:23PM +0300, Alexandru Ardelean wrote:
> For VSC8351 and similar PHYs, a new property was added to generate a clock
> signal on the CLKOUT pin.

Sorry, didn't think about it on v1, but I would imagine other vendors' 
PHYs have similar functionality. We should have something common. We 
have the clock binding for clocks already, so we should consider if 
that should be used here. It may look like an overkill for what you 
need, but things always start out that way. What if you want to turn the 
clock on and off as well?

> This change documents the change in the device-tree bindings doc.

That's obvious.

> 
> Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
> ---
> 
> Changelog v1 -> v2:
> * https://lore.kernel.org/netdev/20230706081554.1616839-2-alex@shruggie.ro/
> * changed property name 'vsc8531,clkout-freq-mhz' -> 'mscc,clkout-freq-mhz'
>   as requested by Rob
> * added 'net-next' tag as requested by Andrew
> 
>  Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> index 0a3647fe331b..085d0e8a834e 100644
> --- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> +++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> @@ -31,6 +31,10 @@ Optional properties:
>  			  VSC8531_LINK_100_ACTIVITY (2),
>  			  VSC8531_LINK_ACTIVITY (0) and
>  			  VSC8531_DUPLEX_COLLISION (8).
> +- mscc,clkout-freq-mhz	: For VSC8531 and similar PHYs, this will output
> +			  a clock signal on the CLKOUT pin of the chip.
> +			  The supported values are 25, 50 & 125 Mhz.
> +			  Default value is no clock signal on the CLKOUT pin.
>  - load-save-gpios	: GPIO used for the load/save operation of the PTP
>  			  hardware clock (PHC).
>  
> @@ -69,5 +73,6 @@ Example:
>                  vsc8531,edge-slowdown	= <7>;
>                  vsc8531,led-0-mode	= <VSC8531_LINK_1000_ACTIVITY>;
>                  vsc8531,led-1-mode	= <VSC8531_LINK_100_ACTIVITY>;
> +                mscc,clkout-freq-mhz	= <50>;
>  		load-save-gpios		= <&gpio 10 GPIO_ACTIVE_HIGH>;
>          };
> -- 
> 2.41.0
> 

Re: [PATCH v2 2/2] dt-bindings: net: phy: vsc8531: document 'vsc8531,clkout-freq-mhz' property

[Alexandru Ardelean]

On Fri, Jul 14, 2023 at 8:24 PM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Jul 13, 2023 at 11:21:23PM +0300, Alexandru Ardelean wrote:
> > For VSC8351 and similar PHYs, a new property was added to generate a clock
> > signal on the CLKOUT pin.
>
> Sorry, didn't think about it on v1, but I would imagine other vendors'
> PHYs have similar functionality. We should have something common. We
> have the clock binding for clocks already, so we should consider if
> that should be used here. It may look like an overkill for what you
> need, but things always start out that way. What if you want to turn the
> clock on and off as well?

So, there's the adin.c PHY driver which has a similar functionality
with the adin_config_clk_out().
Something in the micrel.c PHY driver (with
micrel,rmii-reference-clock-select-25-mhz); hopefully I did not
misread the code about that one.
And the at803x.c PHY driver has a 'qca,clk-out-frequency' property too.

Now with the mscc.c driver, there is a common-ality that could use a framework.

@Rob are you suggesting something like registering a clock provider
(somewhere in the PHY framework) and let the PHY drivers use it?
Usually, these clock signals (once enabled on startup), don't get
turned off; but I've worked mostly on reference designs; somewhere
down the line some people get different requirements.
These clocks get connected back to the MAC (usually), and are usually
like a "fixed-clock" driver.
In our case, turning off the clock would be needed if the PHY
negotiates a non-gigabit link; i.e 100 or 10 Mbps; in that case, the
CLKOUT signal is not needed and it can be turned off.

Maybe start out with a hook in 'struct phy_driver'?
Like "int (*config_clk_out)(struct phy_device *dev);" or something?
And underneath, this delegates to the CLK framework?

I'd let Andrew (or someone in netdev) have a final feedback here.

I can (probably) try to allocate some time to do this change based on
the MSCC driver in the next weeks, if there's a consensus.

Thanks
Alex

>
> > This change documents the change in the device-tree bindings doc.
>
> That's obvious.
>
> >
> > Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
> > ---
> >
> > Changelog v1 -> v2:
> > * https://lore.kernel.org/netdev/20230706081554.1616839-2-alex@shruggie.ro/
> > * changed property name 'vsc8531,clkout-freq-mhz' -> 'mscc,clkout-freq-mhz'
> >   as requested by Rob
> > * added 'net-next' tag as requested by Andrew
> >
> >  Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> > index 0a3647fe331b..085d0e8a834e 100644
> > --- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> > +++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> > @@ -31,6 +31,10 @@ Optional properties:
> >                         VSC8531_LINK_100_ACTIVITY (2),
> >                         VSC8531_LINK_ACTIVITY (0) and
> >                         VSC8531_DUPLEX_COLLISION (8).
> > +- mscc,clkout-freq-mhz       : For VSC8531 and similar PHYs, this will output
> > +                       a clock signal on the CLKOUT pin of the chip.
> > +                       The supported values are 25, 50 & 125 Mhz.
> > +                       Default value is no clock signal on the CLKOUT pin.
> >  - load-save-gpios    : GPIO used for the load/save operation of the PTP
> >                         hardware clock (PHC).
> >
> > @@ -69,5 +73,6 @@ Example:
> >                  vsc8531,edge-slowdown        = <7>;
> >                  vsc8531,led-0-mode   = <VSC8531_LINK_1000_ACTIVITY>;
> >                  vsc8531,led-1-mode   = <VSC8531_LINK_100_ACTIVITY>;
> > +                mscc,clkout-freq-mhz = <50>;
> >               load-save-gpios         = <&gpio 10 GPIO_ACTIVE_HIGH>;
> >          };
> > --
> > 2.41.0
> >

Re: [PATCH v2 2/2] dt-bindings: net: phy: vsc8531: document 'vsc8531,clkout-freq-mhz' property

[Andrew Lunn]

> So, there's the adin.c PHY driver which has a similar functionality
> with the adin_config_clk_out().
> Something in the micrel.c PHY driver (with
> micrel,rmii-reference-clock-select-25-mhz); hopefully I did not
> misread the code about that one.
> And the at803x.c PHY driver has a 'qca,clk-out-frequency' property too.
> 
> Now with the mscc.c driver, there is a common-ality that could use a framework.
> 
> @Rob are you suggesting something like registering a clock provider
> (somewhere in the PHY framework) and let the PHY drivers use it?
> Usually, these clock signals (once enabled on startup), don't get
> turned off; but I've worked mostly on reference designs; somewhere
> down the line some people get different requirements.
> These clocks get connected back to the MAC (usually), and are usually
> like a "fixed-clock" driver.

They are not necessarily fixed clocks. The clock you are adding here
has three frequencies. Two frequencies is common for PHY devices. So
you need to use something more than clk-fixed-rate.c. Also, mostly
PHYs allows the clock to be gated.

> In our case, turning off the clock would be needed if the PHY
> negotiates a non-gigabit link; i.e 100 or 10 Mbps; in that case, the
> CLKOUT signal is not needed and it can be turned off.

Who does not need it? The PHY, or the MAC? If it is the MAC, it should
really be the MAC driver which uses the common clock API to turn it
off. Just watch out for deadlocks with phydev->lock.

> Maybe start out with a hook in 'struct phy_driver'?
> Like "int (*config_clk_out)(struct phy_device *dev);" or something?
> And underneath, this delegates to the CLK framework?

Yes, have phy_device.c implement that registration/unregister of the
clock, deal with locking, and call into the PHY driver to actually
manipulate the clock. You missed the requested frequency in the
function prototype. I would also call it refclk. Three is sometimes
confusion about the different clocks.

Traditionally, clk_enable() can be called in atomic context, but that
is not allowed with phylib, it always assume thread context. I don't
know if the clock framework has some helpers for that, but i also
don't see there being a real need for MAC to enable the clock in
atomic context.

	Andrew

Re: [PATCH v2 2/2] dt-bindings: net: phy: vsc8531: document 'vsc8531,clkout-freq-mhz' property

[Alexandru Ardelean]

On Sun, Jul 16, 2023 at 6:07 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > So, there's the adin.c PHY driver which has a similar functionality
> > with the adin_config_clk_out().
> > Something in the micrel.c PHY driver (with
> > micrel,rmii-reference-clock-select-25-mhz); hopefully I did not
> > misread the code about that one.
> > And the at803x.c PHY driver has a 'qca,clk-out-frequency' property too.
> >
> > Now with the mscc.c driver, there is a common-ality that could use a framework.
> >
> > @Rob are you suggesting something like registering a clock provider
> > (somewhere in the PHY framework) and let the PHY drivers use it?
> > Usually, these clock signals (once enabled on startup), don't get
> > turned off; but I've worked mostly on reference designs; somewhere
> > down the line some people get different requirements.
> > These clocks get connected back to the MAC (usually), and are usually
> > like a "fixed-clock" driver.
>
> They are not necessarily fixed clocks. The clock you are adding here
> has three frequencies. Two frequencies is common for PHY devices. So
> you need to use something more than clk-fixed-rate.c. Also, mostly
> PHYs allows the clock to be gated.
>
> > In our case, turning off the clock would be needed if the PHY
> > negotiates a non-gigabit link; i.e 100 or 10 Mbps; in that case, the
> > CLKOUT signal is not needed and it can be turned off.
>
> Who does not need it? The PHY, or the MAC? If it is the MAC, it should
> really be the MAC driver which uses the common clock API to turn it
> off. Just watch out for deadlocks with phydev->lock.

The MAC needs the clock in GMII mode, when going in gigabit mode.

>
> > Maybe start out with a hook in 'struct phy_driver'?
> > Like "int (*config_clk_out)(struct phy_device *dev);" or something?
> > And underneath, this delegates to the CLK framework?
>
> Yes, have phy_device.c implement that registration/unregister of the
> clock, deal with locking, and call into the PHY driver to actually
> manipulate the clock. You missed the requested frequency in the
> function prototype. I would also call it refclk. Three is sometimes
> confusion about the different clocks.

Ack.
Then something like:
int (*config_refclk)(struct phy_device *dev, uint32_t frequency);

>
> Traditionally, clk_enable() can be called in atomic context, but that
> is not allowed with phylib, it always assume thread context. I don't
> know if the clock framework has some helpers for that, but i also
> don't see there being a real need for MAC to enable the clock in
> atomic context.
>
>         Andrew

Re: [PATCH v2 1/2 net-next] net: phy: mscc: add support for CLKOUT ctrl reg for VSC8531 and similar

[Andrew Lunn]

> +set_reg:
> +	mutex_lock(&phydev->lock);
> +	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO,
> +			      VSC8531_CLKOUT_CNTL, mask, set);
> +	mutex_unlock(&phydev->lock);

What is this mutex protecting?

     Andrew

Re: [PATCH v2 1/2 net-next] net: phy: mscc: add support for CLKOUT ctrl reg for VSC8531 and similar

[Alexandru Ardelean]

On Thu, Jul 13, 2023 at 11:35 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > +set_reg:
> > +     mutex_lock(&phydev->lock);
> > +     rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO,
> > +                           VSC8531_CLKOUT_CNTL, mask, set);
> > +     mutex_unlock(&phydev->lock);
>
> What is this mutex protecting?

This was inspired by vsc85xx_edge_rate_cntl_set().
Which has the same format.

I'll re-test with this lock removed.
I may be misremembering (or maybe I did something silly at some
point), but there was a weird stack-trace warning before adding this
lock there.
This was with a 5.10.116 kernel version.

>
>      Andrew

Re: [PATCH v2 1/2 net-next] net: phy: mscc: add support for CLKOUT ctrl reg for VSC8531 and similar

[Andrew Lunn]

On Fri, Jul 14, 2023 at 09:09:14AM +0300, Alexandru Ardelean wrote:
> On Thu, Jul 13, 2023 at 11:35 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > +set_reg:
> > > +     mutex_lock(&phydev->lock);
> > > +     rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO,
> > > +                           VSC8531_CLKOUT_CNTL, mask, set);
> > > +     mutex_unlock(&phydev->lock);
> >
> > What is this mutex protecting?
> 
> This was inspired by vsc85xx_edge_rate_cntl_set().
> Which has the same format.

phy_modify_paged() locks the MDIO bus while it swaps the page, so
nothing else can use it. That also protects the read/modify/write.

Nothing is modifying phydev, so the lock is not needed for that
either.

> I'll re-test with this lock removed.
> I may be misremembering (or maybe I did something silly at some
> point), but there was a weird stack-trace warning before adding this
> lock there.
> This was with a 5.10.116 kernel version.

This patch is for net-next, please test there.

When testing for locking issues, and when doing development in
general, it is a good idea to turn on CONFIG_PROVE_LOCKING and
CONFIG_DEBUG_ATOMIC_SLEEP.

	Andrew

Re: [PATCH v2 1/2 net-next] net: phy: mscc: add support for CLKOUT ctrl reg for VSC8531 and similar

[Alexandru Ardelean]

On Sat, Jul 15, 2023 at 1:27 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Fri, Jul 14, 2023 at 09:09:14AM +0300, Alexandru Ardelean wrote:
> > On Thu, Jul 13, 2023 at 11:35 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > > +set_reg:
> > > > +     mutex_lock(&phydev->lock);
> > > > +     rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO,
> > > > +                           VSC8531_CLKOUT_CNTL, mask, set);
> > > > +     mutex_unlock(&phydev->lock);
> > >
> > > What is this mutex protecting?
> >
> > This was inspired by vsc85xx_edge_rate_cntl_set().
> > Which has the same format.

Good news.
Removing this mutex works on a 5.10 kernel, with no issues.

>
> phy_modify_paged() locks the MDIO bus while it swaps the page, so
> nothing else can use it. That also protects the read/modify/write.
>
> Nothing is modifying phydev, so the lock is not needed for that
> either.

I remembered what I was doing wrong in that version that had issues
with the lock.
I was doing some manual page changes, with
phy_base_read/()phy_base_write() functions, which are in this file.

These functions have a warning + dump_stack() for when the
"phydev->mdio.bus->mdio_lock" is not held).
That threw me off initially.

>
> > I'll re-test with this lock removed.
> > I may be misremembering (or maybe I did something silly at some
> > point), but there was a weird stack-trace warning before adding this
> > lock there.
> > This was with a 5.10.116 kernel version.
>
> This patch is for net-next, please test there.

I've been testing on a Renesas board CIP project.
Kernel version (on our board is actually 5.10.83 ; I get them confused
since 5.10.xxx seems to be used here-n-there).

The kernel is here:
https://github.com/renesas-rz/rz_linux-cip/tree/rz-5.10-cip3

I'm trying to backport some ARCH patches, so that the board boots up.
I "think" I'm half way there; now the kernel prints something to
console and then stops (that's progress from no prints).

Let's see if we get a different consensus on Rob't suggestion; this
patch may require a different V3 :)



>
> When testing for locking issues, and when doing development in
> general, it is a good idea to turn on CONFIG_PROVE_LOCKING and
> CONFIG_DEBUG_ATOMIC_SLEEP.
>
>         Andrew