Re: [PATCH v2 4/4] net: phy: at803x: add RX and TX clock management for IPQ5018 PHY

[George Moussalem]

On 6/3/26 17:53, Dmitry Baryshkov wrote:
> On Tue, Jun 02, 2026 at 10:50:40AM +0400, George Moussalem via B4 Relay wrote:
>> From: George Moussalem <george.moussalem@xxxxxxxxxxx>
>>
>> Acquire and enable the RX and TX clocks for the IPQ5018 PHY. These
>> clocks are required for the PHY's datapath to function correctly.
>> In addition, gate the clocks upon link state changes for improved power
>> management.
>>
>> Fixes: d46502279a11 ("net: phy: qcom: at803x: Add Qualcomm IPQ5018 Internal PHY support")
>> Signed-off-by: George Moussalem <george.moussalem@xxxxxxxxxxx>
>> ---
>>  drivers/net/phy/qcom/at803x.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/net/phy/qcom/at803x.c b/drivers/net/phy/qcom/at803x.c
>> index 63726cf98cd4..b7361a14220d 100644
>> --- a/drivers/net/phy/qcom/at803x.c
>> +++ b/drivers/net/phy/qcom/at803x.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/regulator/consumer.h>
>>  #include <linux/of.h>
>>  #include <linux/phylink.h>
>> +#include <linux/clk.h>
>>  #include <linux/reset.h>
>>  #include <linux/phy_port.h>
>>  #include <dt-bindings/net/qca-ar803x.h>
>> @@ -176,6 +177,8 @@ struct at803x_context {
>>  };
>>  
>>  struct ipq5018_priv {
>> +	struct clk *rx_clk;
>> +	struct clk *tx_clk;
>>  	struct reset_control *rst;
>>  	bool set_short_cable_dac;
>>  };
>> @@ -1062,6 +1065,16 @@ static int ipq5018_config_init(struct phy_device *phydev)
>>  
>>  static void ipq5018_link_change_notify(struct phy_device *phydev)
>>  {
>> +	struct ipq5018_priv *priv = phydev->priv;
>> +
>> +	if (phydev->link) {
>> +		clk_enable(priv->rx_clk);
>> +		clk_enable(priv->tx_clk);
>> +	} else {
>> +		clk_disable(priv->rx_clk);
>> +		clk_disable(priv->tx_clk);
>> +	}
>> +
>>  	/*
>>  	 * Reset the FIFO buffer upon link disconnects to clear any residual data
>>  	 * which may cause issues with the FIFO which it cannot recover from.
>> @@ -1084,6 +1097,16 @@ static int ipq5018_probe(struct phy_device *phydev)
>>  	priv->set_short_cable_dac = of_property_read_bool(dev->of_node,
>>  							  "qcom,dac-preset-short-cable");
>>  
>> +	priv->rx_clk = devm_clk_get_enabled(dev, "rx");
> 
> Why are you enabling it here? Won't you get the notification to enable
> it if required?

it was mainly to cater for the case of a bootloader enabling the clocks
beforehand (especially during testing) and increase the enable count so
the CCF doesn't complain and spits out a stack dump when trying to
disable. Perhaps better to add if statements in link_change_notify to
check whether the clocks are enabled and then act accordingly?

> 
>> +	if (IS_ERR(priv->rx_clk))
>> +		return dev_err_probe(dev, PTR_ERR(priv->rx_clk),
>> +				     "failed to get and enable RX clock\n");
>> +
>> +	priv->tx_clk = devm_clk_get_enabled(dev, "tx");
>> +	if (IS_ERR(priv->tx_clk))
>> +		return dev_err_probe(dev, PTR_ERR(priv->tx_clk),
>> +				     "failed to get and enable TX clock\n");
>> +
>>  	priv->rst = devm_reset_control_array_get_exclusive(dev);
>>  	if (IS_ERR(priv->rst))
>>  		return dev_err_probe(dev, PTR_ERR(priv->rst),
>>
>> -- 
>> 2.53.0
>>
>>
> 

Best regards,
George