[PATCH] phy: cadence: Sierra: Add support for skipping configuration

[PATCH] phy: cadence: Sierra: Add support for skipping configuration

[Aswath Govindraju]

Skip the phy configuration if the required configurations were done in an
earlier boot stage.

Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
---
 drivers/phy/cadence/phy-cadence-sierra.c | 73 +++++++++++++++++-------
 1 file changed, 51 insertions(+), 22 deletions(-)

diff --git a/drivers/phy/cadence/phy-cadence-sierra.c b/drivers/phy/cadence/phy-cadence-sierra.c
index e265647e29a2..d4197524ee8d 100644
--- a/drivers/phy/cadence/phy-cadence-sierra.c
+++ b/drivers/phy/cadence/phy-cadence-sierra.c
@@ -370,6 +370,7 @@ struct cdns_sierra_phy {
 	int nsubnodes;
 	u32 num_lanes;
 	bool autoconf;
+	int already_configured;
 	struct clk_onecell_data clk_data;
 	struct clk *output_clks[CDNS_SIERRA_OUTPUT_CLOCKS];
 };
@@ -517,7 +518,7 @@ static int cdns_sierra_phy_init(struct phy *gphy)
 	int i, j;
 
 	/* Initialise the PHY registers, unless auto configured */
-	if (phy->autoconf || phy->nsubnodes > 1)
+	if (phy->autoconf || phy->already_configured || phy->nsubnodes > 1)
 		return 0;
 
 	clk_set_rate(phy->input_clks[CMN_REFCLK_DIG_DIV], 25000000);
@@ -646,6 +647,18 @@ static const struct phy_ops ops = {
 	.owner		= THIS_MODULE,
 };
 
+static int cdns_sierra_noop_phy_on(struct phy *gphy)
+{
+	usleep_range(5000, 10000);
+
+	return 0;
+}
+
+static const struct phy_ops noop_ops = {
+	.power_on	= cdns_sierra_noop_phy_on,
+	.owner		= THIS_MODULE,
+};
+
 static u8 cdns_sierra_pll_mux_get_parent(struct clk_hw *hw)
 {
 	struct cdns_sierra_pll_mux *mux = to_cdns_sierra_pll_mux(hw);
@@ -1118,13 +1131,6 @@ static int cdns_sierra_phy_get_clocks(struct cdns_sierra_phy *sp,
 	struct clk *clk;
 	int ret;
 
-	clk = devm_clk_get_optional(dev, "phy_clk");
-	if (IS_ERR(clk)) {
-		dev_err(dev, "failed to get clock phy_clk\n");
-		return PTR_ERR(clk);
-	}
-	sp->input_clks[PHY_CLK] = clk;
-
 	clk = devm_clk_get_optional(dev, "cmn_refclk_dig_div");
 	if (IS_ERR(clk)) {
 		dev_err(dev, "cmn_refclk_dig_div clock not found\n");
@@ -1160,17 +1166,33 @@ static int cdns_sierra_phy_get_clocks(struct cdns_sierra_phy *sp,
 	return 0;
 }
 
-static int cdns_sierra_phy_enable_clocks(struct cdns_sierra_phy *sp)
+static int cdns_sierra_phy_clk(struct cdns_sierra_phy *sp)
 {
+	struct device *dev = sp->dev;
+	struct clk *clk;
 	int ret;
 
+	clk = devm_clk_get_optional(dev, "phy_clk");
+	if (IS_ERR(clk)) {
+		dev_err(dev, "failed to get clock phy_clk\n");
+		return PTR_ERR(clk);
+	}
+	sp->input_clks[PHY_CLK] = clk;
+
 	ret = clk_prepare_enable(sp->input_clks[PHY_CLK]);
 	if (ret)
 		return ret;
 
+	return 0;
+}
+
+static int cdns_sierra_phy_enable_clocks(struct cdns_sierra_phy *sp)
+{
+	int ret;
+
 	ret = clk_prepare_enable(sp->output_clks[CDNS_SIERRA_PLL_CMNLC]);
 	if (ret)
-		goto err_pll_cmnlc;
+		return ret;
 
 	ret = clk_prepare_enable(sp->output_clks[CDNS_SIERRA_PLL_CMNLC1]);
 	if (ret)
@@ -1181,9 +1203,6 @@ static int cdns_sierra_phy_enable_clocks(struct cdns_sierra_phy *sp)
 err_pll_cmnlc1:
 	clk_disable_unprepare(sp->output_clks[CDNS_SIERRA_PLL_CMNLC]);
 
-err_pll_cmnlc:
-	clk_disable_unprepare(sp->input_clks[PHY_CLK]);
-
 	return ret;
 }
 
@@ -1382,16 +1401,24 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = cdns_sierra_phy_get_resets(sp, dev);
-	if (ret)
-		goto unregister_clk;
-
 	ret = cdns_sierra_phy_enable_clocks(sp);
 	if (ret)
 		goto unregister_clk;
 
-	/* Enable APB */
-	reset_control_deassert(sp->apb_rst);
+	regmap_field_read(sp->pma_cmn_ready, &sp->already_configured);
+
+	if (!(sp->already_configured)) {
+		ret = cdns_sierra_phy_clk(sp);
+		if (ret)
+			goto unregister_clk;
+
+		ret = cdns_sierra_phy_get_resets(sp, dev);
+		if (ret)
+			goto unregister_clk;
+
+		/* Enable APB */
+		reset_control_deassert(sp->apb_rst);
+	}
 
 	/* Check that PHY is present */
 	regmap_field_read(sp->macro_id_type, &id_value);
@@ -1433,8 +1460,10 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
 
 		sp->num_lanes += sp->phys[node].num_lanes;
 
-		gphy = devm_phy_create(dev, child, &ops);
-
+		if (!(sp->already_configured))
+			gphy = devm_phy_create(dev, child, &ops);
+		else
+			gphy = devm_phy_create(dev, child, &noop_ops);
 		if (IS_ERR(gphy)) {
 			ret = PTR_ERR(gphy);
 			of_node_put(child);
@@ -1455,7 +1484,7 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
 	}
 
 	/* If more than one subnode, configure the PHY as multilink */
-	if (!sp->autoconf && sp->nsubnodes > 1) {
+	if (!(sp->already_configured && sp->autoconf) && sp->nsubnodes > 1) {
 		ret = cdns_sierra_phy_configure_multilink(sp);
 		if (ret)
 			goto put_control;
-- 
2.17.1

Re: [PATCH] phy: cadence: Sierra: Add support for skipping configuration

[Dan Carpenter]

On Thu, Jan 27, 2022 at 02:26:58PM +0530, Aswath Govindraju wrote:
> Skip the phy configuration if the required configurations were done in an
> earlier boot stage.
> 

Why are you doing this?  Could you please put in the commit message if
the user will see an improvement from this change.

> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---

[ snip ]

> @@ -1382,16 +1401,24 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	ret = cdns_sierra_phy_get_resets(sp, dev);
> -	if (ret)
> -		goto unregister_clk;
> -
>  	ret = cdns_sierra_phy_enable_clocks(sp);
>  	if (ret)
>  		goto unregister_clk;
>  
> -	/* Enable APB */
> -	reset_control_deassert(sp->apb_rst);
> +	regmap_field_read(sp->pma_cmn_ready, &sp->already_configured);
> +
> +	if (!(sp->already_configured)) {

Delete extra parens.

> +		ret = cdns_sierra_phy_clk(sp);
> +		if (ret)
> +			goto unregister_clk;

The goto should release the most recent successful allocation which is
cdns_sierra_phy_enable_clocks().  So this should be goto clk_disable.
Except that will also call reset_control_assert() which is wrong...  The
rules are generally that error handling should be in the reverse order
from how we allocated it.  If allocation is optional the cleanup should
be optional.  The allocation and unwind code should mirror each other.

> +
> +		ret = cdns_sierra_phy_get_resets(sp, dev);
> +		if (ret)
> +			goto unregister_clk;

goto clk_disable;

> +
> +		/* Enable APB */
> +		reset_control_deassert(sp->apb_rst);

Since this is now optional it should be optional in the cleanup.

> +	}
>  

Since the order of allocations has changed, the other gotos need to be
updated to free the most recent allocation as well.  Then the error
handling looks like this:

	return 0;

put_control:
	while (--node >= 0)
		reset_control_put(sp->phys[node].lnk_rst);
ctrl_assert:
	if (!sp->already_configured)
		reset_control_assert(sp->apb_rst);
clk_disable:
	cdns_sierra_phy_disable_clocks(sp);
unregister_clk:
	cdns_sierra_clk_unregister(sp);
	return ret;


>  	/* Check that PHY is present */
>  	regmap_field_read(sp->macro_id_type, &id_value);
> @@ -1433,8 +1460,10 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
>  
>  		sp->num_lanes += sp->phys[node].num_lanes;
>  
> -		gphy = devm_phy_create(dev, child, &ops);
> -
> +		if (!(sp->already_configured))

Delete parens.

> +			gphy = devm_phy_create(dev, child, &ops);
> +		else
> +			gphy = devm_phy_create(dev, child, &noop_ops);
>  		if (IS_ERR(gphy)) {
>  			ret = PTR_ERR(gphy);
>  			of_node_put(child);
> @@ -1455,7 +1484,7 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
>  	}
>  
>  	/* If more than one subnode, configure the PHY as multilink */
> -	if (!sp->autoconf && sp->nsubnodes > 1) {
> +	if (!(sp->already_configured && sp->autoconf) && sp->nsubnodes > 1) {

It's normally easier to understand conditions when you push the ! as
far in as possible:

	if ((!sp->already_configured || !sp->autoconf) &&
	    sp->nsubnodes > 1) {

Is this condition right?  Shouldn't it be:

	if (!sp->already_configured && !sp->autoconf && sp->nsubnodes > 1) {

The ->already_configured is set/stored in firmware so I don't know when
that happens.  Please, add that information to the commit message when
you resend.

regards,
dan carpenter

Re: [PATCH] phy: cadence: Sierra: Add support for skipping configuration

[Aswath Govindraju]

Hi Dan,

On 27/01/22 3:49 pm, Dan Carpenter wrote:
> On Thu, Jan 27, 2022 at 02:26:58PM +0530, Aswath Govindraju wrote:
>> Skip the phy configuration if the required configurations were done in an
>> earlier boot stage.
>>
> 
> Why are you doing this?  Could you please put in the commit message if
> the user will see an improvement from this change.
> 

In some cases, the SerDes configuration can be done in the bootloaders
itself and in these the reconfiguration can be skipped in kernel. For
example 2 different cores can be using the SerDes, in this case the
bootloaders configure the SerDes, so that the cores can later on use the
lanes directly without configuring the lanes again.

I will include this in the commit message of the respin.

>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>> ---
> 
> [ snip ]
> 
>> @@ -1382,16 +1401,24 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		return ret;
>>  
>> -	ret = cdns_sierra_phy_get_resets(sp, dev);
>> -	if (ret)
>> -		goto unregister_clk;
>> -
>>  	ret = cdns_sierra_phy_enable_clocks(sp);
>>  	if (ret)
>>  		goto unregister_clk;
>>  
>> -	/* Enable APB */
>> -	reset_control_deassert(sp->apb_rst);
>> +	regmap_field_read(sp->pma_cmn_ready, &sp->already_configured);
>> +
>> +	if (!(sp->already_configured)) {
> 
> Delete extra parens.
> 
>> +		ret = cdns_sierra_phy_clk(sp);
>> +		if (ret)
>> +			goto unregister_clk;
> 
> The goto should release the most recent successful allocation which is
> cdns_sierra_phy_enable_clocks().  So this should be goto clk_disable.
> Except that will also call reset_control_assert() which is wrong...  The
> rules are generally that error handling should be in the reverse order
> from how we allocated it.  If allocation is optional the cleanup should
> be optional.  The allocation and unwind code should mirror each other.
> 
>> +
>> +		ret = cdns_sierra_phy_get_resets(sp, dev);
>> +		if (ret)
>> +			goto unregister_clk;
> 
> goto clk_disable;
> 
>> +
>> +		/* Enable APB */
>> +		reset_control_deassert(sp->apb_rst);
> 
> Since this is now optional it should be optional in the cleanup.
> 
>> +	}
>>  
> 
> Since the order of allocations has changed, the other gotos need to be
> updated to free the most recent allocation as well.  Then the error
> handling looks like this:
> 
> 	return 0;
> 
> put_control:
> 	while (--node >= 0)
> 		reset_control_put(sp->phys[node].lnk_rst);
> ctrl_assert:
> 	if (!sp->already_configured)
> 		reset_control_assert(sp->apb_rst);
> clk_disable:
> 	cdns_sierra_phy_disable_clocks(sp);
> unregister_clk:
> 	cdns_sierra_clk_unregister(sp);
> 	return ret;
> 
> 
>>  	/* Check that PHY is present */
>>  	regmap_field_read(sp->macro_id_type, &id_value);
>> @@ -1433,8 +1460,10 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
>>  
>>  		sp->num_lanes += sp->phys[node].num_lanes;
>>  
>> -		gphy = devm_phy_create(dev, child, &ops);
>> -
>> +		if (!(sp->already_configured))
> 
> Delete parens.
> 
>> +			gphy = devm_phy_create(dev, child, &ops);
>> +		else
>> +			gphy = devm_phy_create(dev, child, &noop_ops);
>>  		if (IS_ERR(gphy)) {
>>  			ret = PTR_ERR(gphy);
>>  			of_node_put(child);
>> @@ -1455,7 +1484,7 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
>>  	}
>>  
>>  	/* If more than one subnode, configure the PHY as multilink */
>> -	if (!sp->autoconf && sp->nsubnodes > 1) {
>> +	if (!(sp->already_configured && sp->autoconf) && sp->nsubnodes > 1) {
> 
> It's normally easier to understand conditions when you push the ! as
> far in as possible:
> 
> 	if ((!sp->already_configured || !sp->autoconf) &&
> 	    sp->nsubnodes > 1) {
> 
> Is this condition right?  Shouldn't it be:
> 
> 	if (!sp->already_configured && !sp->autoconf && sp->nsubnodes > 1) {
> 
> The ->already_configured is set/stored in firmware so I don't know when
> that happens.  Please, add that information to the commit message when
> you resend.
> 

Sorry, my logic was wrong. I will correct this in the respin.


Thank you for the review comments

Regards,
Aswath

> regards,
> dan carpenter
>