Re: [PATCH] net: phy: dp83867: respect ti,clk-output-sel DT property

[Roland Hieber <rhi@pengutronix.de>]

On Thu, Jun 01, 2023 at 12:08:23PM +0200, Roland Hieber wrote:
> On some boards it is necessary to configure the PHY to provide the
> correct clock to the FEC.
> 
> This ports the rest of the following two kernel commits:
> 
>     | commit 9708fb630d19ee51ae3aeb3a533e3010da0e8570
>     | Author: Wadim Egorov <w.egorov@phytec.de>
>     | Date:   Mi 2018-02-14 17:07:11
>     |
>     |     net: phy: dp83867: Add binding for the CLK_OUT pin muxing option
>     |
>     |     The DP83867 has a muxing option for the CLK_OUT pin. It is possible
>     |     to set CLK_OUT for different channels.
>     |     Create a binding to select a specific clock for CLK_OUT pin.
>     |
>     |     Signed-off-by: Wadim Egorov <w.egorov@phytec.de>
>     |     Signed-off-by: Daniel Schultz <d.schultz@phytec.de>
>     |     Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>     |     Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>     |     Signed-off-by: David S. Miller <davem@davemloft.net>
>     |
>     | commit 13c83cf8af0dcc6103982b4dc0b70826f0b54f21
>     | Author: Trent Piepho <tpiepho@impinj.com>
>     | Date:   Mi 2019-05-22 18:43:22
>     |
>     |     net: phy: dp83867: Add ability to disable output clock
>     |
>     |     Generally, the output clock pin is only used for testing and only serves
>     |     as a source of RF noise after this.  It could be used to daisy-chain
>     |     PHYs, but this is uncommon.  Since the PHY can disable the output, make
>     |     doing so an option.  I do this by adding another enumeration to the
>     |     allowed values of ti,clk-output-sel.
>     |
>     |     The code was not using the value DP83867_CLK_O_SEL_REF_CLK as one might
>     |     expect: to select the REF_CLK as the output.  Rather it meant "keep
>     |     clock output setting as is", which, depending on PHY strapping, might
>     |     not be outputting REF_CLK.
>     |
>     |     Change this so DP83867_CLK_O_SEL_REF_CLK means enable REF_CLK output.
>     |     Omitting the property will leave the setting as is (which was the
>     |     previous behavior in this case).
>     |
>     |     Out of range values were silently converted into
>     |     DP83867_CLK_O_SEL_REF_CLK.  Change this so they generate an error.
>     |
>     |     Cc: Andrew Lunn <andrew@lunn.ch>
>     |     Cc: Florian Fainelli <f.fainelli@gmail.com>
>     |     Cc: Heiner Kallweit <hkallweit1@gmail.com>
>     |     Signed-off-by: Trent Piepho <tpiepho@impinj.com>
>     |     Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>     |     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> Link: https://git.kernel.org/torvalds/c/9708fb630d19ee51ae3a
> Link: https://git.kernel.org/torvalds/c/13c83cf8af0dcc610398
> Signed-off-by: Roland Hieber <rhi@pengutronix.de>
> ---
>  drivers/net/phy/dp83867.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index 375dcd075308..0bc19dd11270 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c
> @@ -96,6 +96,9 @@
>  
>  #define DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX	0x0
>  #define DP83867_IO_MUX_CFG_IO_IMPEDANCE_MIN	0x1f
> +#define DP83867_IO_MUX_CFG_CLK_O_DISABLE	BIT(6)
> +#define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK	(0x1f << 8)
> +#define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT	8
>  
>  /* CFG4 bits */
>  #define DP83867_CFG4_PORT_MIRROR_EN		BIT(0)
> @@ -113,6 +116,8 @@ struct dp83867_private {
>  	int io_impedance;
>  	int port_mirroring;
>  	bool rxctrl_strap_quirk;
> +	bool set_clk_output;
> +	u32 clk_output_sel;
>  };
>  
>  static int dp83867_read_status(struct phy_device *phydev)
> @@ -174,6 +179,22 @@ static int dp83867_of_init(struct phy_device *phydev)
>  	dp83867->io_impedance = -EINVAL;
>  
>  	/* Optional configuration */
> +	ret = of_property_read_u32(of_node, "ti,clk-output-sel",
> +				   &dp83867->clk_output_sel);
> +	/* If not set, keep default */
> +	if (!ret) {
> +		dp83867->set_clk_output = true;
> +		/* Valid values are 0 to DP83867_CLK_O_SEL_REF_CLK or
> +		 * DP83867_CLK_O_SEL_OFF.
> +		 */
> +		if (dp83867->clk_output_sel > DP83867_CLK_O_SEL_REF_CLK &&
> +		    dp83867->clk_output_sel != DP83867_CLK_O_SEL_OFF) {
> +			dev_err(&phydev->dev, "ti,clk-output-sel value %u out of range\n",
> +				   dp83867->clk_output_sel);
> +			return -EINVAL;
> +		}
> +	}
> +
>  	if (of_property_read_bool(of_node, "ti,max-output-impedance"))
>  		dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX;
>  	else if (of_property_read_bool(of_node, "ti,min-output-impedance"))
> @@ -308,6 +329,21 @@ static int dp83867_config_init(struct phy_device *phydev)
>  	if (dp83867->port_mirroring != DP83867_PORT_MIRROING_KEEP)
>  		dp83867_config_port_mirroring(phydev);
>  
> +	/* Clock output selection if muxing property is set */
> +	if (dp83867->set_clk_output) {
> +		u16 mask = DP83867_IO_MUX_CFG_CLK_O_DISABLE;
> +
> +		if (dp83867->clk_output_sel == DP83867_CLK_O_SEL_OFF) {
> +			val = DP83867_IO_MUX_CFG_CLK_O_DISABLE;
> +		} else {
> +			mask |= DP83867_IO_MUX_CFG_CLK_O_SEL_MASK;
> +			val = dp83867->clk_output_sel <<
> +			      DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT;
> +		}
> +
> +		phy_modify(phydev, DP83867_IO_MUX_CFG, mask, val);

Sorry I forgot to test this… this is wrong, this needs to be an indirect
write.

 - Roland

> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.39.2
> 
> 

-- 
Roland Hieber, Pengutronix e.K.          | r.hieber@pengutronix.de     |
Steuerwalder Str. 21                     | https://www.pengutronix.de/ |
31137 Hildesheim, Germany                | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686         | Fax:   +49-5121-206917-5555 |