Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1471618 > unrolled thread

[PATCH 2/4] mmc: sdhci-of-arasan: Control clock for accessing syscon

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/4] mmc: sdhci-of-arasan: Control clock for accessing syscon Shawn Lin <shawn.lin@rock-chips.com> - 2016-08-29 10:10 +0200
    Re: [PATCH 2/4] mmc: sdhci-of-arasan: Control clock for accessing syscon Heiko Stübner <heiko@sntech.de> - 2016-08-29 10:30 +0200
      Re: [PATCH 2/4] mmc: sdhci-of-arasan: Control clock for accessing  syscon Shawn Lin <shawn.lin@rock-chips.com> - 2016-08-29 11:00 +0200
        Re: [PATCH 2/4] mmc: sdhci-of-arasan: Control clock for accessing syscon Heiko Stübner <heiko@sntech.de> - 2016-08-29 11:20 +0200
          Re: [PATCH 2/4] mmc: sdhci-of-arasan: Control clock for accessing  syscon Shawn Lin <shawn.lin@rock-chips.com> - 2016-08-29 11:30 +0200
          Re: [PATCH 2/4] mmc: sdhci-of-arasan: Control clock for accessing  syscon Shawn Lin <shawn.lin@rock-chips.com> - 2016-08-29 11:40 +0200
            Re: [PATCH 2/4] mmc: sdhci-of-arasan: Control clock for accessing syscon Heiko Stübner <heiko@sntech.de> - 2016-08-29 12:50 +0200

#1471618 — [PATCH 2/4] mmc: sdhci-of-arasan: Control clock for accessing syscon

In the eariler commit 65820199272d ("Documentation: mmc:
sdhci-of-arasan: Add soc-ctl-syscon for corecfg regs"), we
introduced syscon to control corecfg_* stuff provided by
arasan. But given that we may need to ungate the clock for
accessing corecfg_*, it not so perfect as it depends on
whether specific clock driver disables it if not referenced.
Meanwhile, if we don't need arasan contoller to work anymore,
there is no reason to still enable it. So let's control this
clock when needed.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 drivers/mmc/host/sdhci-of-arasan.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index 0b3a9cf..7ae3ae4 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -78,6 +78,7 @@ struct sdhci_arasan_soc_ctl_map {
  * struct sdhci_arasan_data
  * @host:		Pointer to the main SDHCI host structure.
  * @clk_ahb:		Pointer to the AHB clock
+ * @clk_syscon:		Pointer to the optional clock for accessing syscon
  * @phy:		Pointer to the generic phy
  * @is_phy_on:		True if the PHY is on; false if not.
  * @sdcardclk_hw:	Struct for the clock we might provide to a PHY.
@@ -88,6 +89,7 @@ struct sdhci_arasan_soc_ctl_map {
 struct sdhci_arasan_data {
 	struct sdhci_host *host;
 	struct clk	*clk_ahb;
+	struct clk	*clk_syscon;
 	struct phy	*phy;
 	bool		is_phy_on;
 
@@ -290,6 +292,7 @@ static int sdhci_arasan_suspend(struct device *dev)
 
 	clk_disable(pltfm_host->clk);
 	clk_disable(sdhci_arasan->clk_ahb);
+	clk_disable(sdhci_arasan->clk_syscon);
 
 	return 0;
 }
@@ -309,6 +312,12 @@ static int sdhci_arasan_resume(struct device *dev)
 	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
 	int ret;
 
+	ret = clk_enable(sdhci_arasan->clk_syscon);
+	if (ret) {
+		dev_err(dev, "Cannot enable syscon clock.\n");
+		return ret;
+	}
+
 	ret = clk_enable(sdhci_arasan->clk_ahb);
 	if (ret) {
 		dev_err(dev, "Cannot enable AHB clock.\n");
@@ -528,26 +537,33 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 					ret);
 			goto err_pltfm_free;
 		}
+
+		sdhci_arasan->clk_syscon = devm_clk_get(&pdev->dev,
+							"clk_syscon");
+		if (clk_prepare_enable(sdhci_arasan->clk_syscon)) {
+			dev_err(&pdev->dev, "Unable to enable syscon clock.\n");
+			goto err_pltfm_free;
+		}
 	}
 
 	sdhci_arasan->clk_ahb = devm_clk_get(&pdev->dev, "clk_ahb");
 	if (IS_ERR(sdhci_arasan->clk_ahb)) {
 		dev_err(&pdev->dev, "clk_ahb clock not found.\n");
 		ret = PTR_ERR(sdhci_arasan->clk_ahb);
-		goto err_pltfm_free;
+		goto clk_dis_syscon;
 	}
 
 	clk_xin = devm_clk_get(&pdev->dev, "clk_xin");
 	if (IS_ERR(clk_xin)) {
 		dev_err(&pdev->dev, "clk_xin clock not found.\n");
 		ret = PTR_ERR(clk_xin);
-		goto err_pltfm_free;
+		goto clk_dis_syscon;
 	}
 
 	ret = clk_prepare_enable(sdhci_arasan->clk_ahb);
 	if (ret) {
 		dev_err(&pdev->dev, "Unable to enable AHB clock.\n");
-		goto err_pltfm_free;
+		goto clk_dis_syscon;
 	}
 
 	ret = clk_prepare_enable(clk_xin);
@@ -607,6 +623,8 @@ clk_disable_all:
 	clk_disable_unprepare(clk_xin);
 clk_dis_ahb:
 	clk_disable_unprepare(sdhci_arasan->clk_ahb);
+clk_dis_syscon:
+	clk_disable_unprepare(sdhci_arasan->clk_syscon);
 err_pltfm_free:
 	sdhci_pltfm_free(pdev);
 	return ret;
@@ -631,6 +649,7 @@ static int sdhci_arasan_remove(struct platform_device *pdev)
 	ret = sdhci_pltfm_unregister(pdev);
 
 	clk_disable_unprepare(clk_ahb);
+	clk_disable_unprepare(sdhci_arasan->clk_syscon);
 
 	return ret;
 }
-- 
2.3.7

[toc] | [next] | [standalone]

#1471642

Hi Shawn,

Am Montag, 29. August 2016, 16:02:57 schrieb Shawn Lin:
> In the eariler commit 65820199272d ("Documentation: mmc:
> sdhci-of-arasan: Add soc-ctl-syscon for corecfg regs"), we
> introduced syscon to control corecfg_* stuff provided by
> arasan. But given that we may need to ungate the clock for
> accessing corecfg_*, it not so perfect as it depends on
> whether specific clock driver disables it if not referenced.
> Meanwhile, if we don't need arasan contoller to work anymore,
> there is no reason to still enable it. So let's control this
> clock when needed.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
>  drivers/mmc/host/sdhci-of-arasan.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
> b/drivers/mmc/host/sdhci-of-arasan.c index 0b3a9cf..7ae3ae4 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -78,6 +78,7 @@ struct sdhci_arasan_soc_ctl_map {
>   * struct sdhci_arasan_data
>   * @host:		Pointer to the main SDHCI host structure.
>   * @clk_ahb:		Pointer to the AHB clock
> + * @clk_syscon:		Pointer to the optional clock for accessing syscon
>   * @phy:		Pointer to the generic phy
>   * @is_phy_on:		True if the PHY is on; false if not.
>   * @sdcardclk_hw:	Struct for the clock we might provide to a PHY.
> @@ -88,6 +89,7 @@ struct sdhci_arasan_soc_ctl_map {
>  struct sdhci_arasan_data {
>  	struct sdhci_host *host;
>  	struct clk	*clk_ahb;
> +	struct clk	*clk_syscon;
>  	struct phy	*phy;
>  	bool		is_phy_on;
> 
> @@ -290,6 +292,7 @@ static int sdhci_arasan_suspend(struct device *dev)
> 
>  	clk_disable(pltfm_host->clk);
>  	clk_disable(sdhci_arasan->clk_ahb);
> +	clk_disable(sdhci_arasan->clk_syscon);
> 
>  	return 0;
>  }
> @@ -309,6 +312,12 @@ static int sdhci_arasan_resume(struct device *dev)
>  	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>  	int ret;
> 
> +	ret = clk_enable(sdhci_arasan->clk_syscon);
> +	if (ret) {
> +		dev_err(dev, "Cannot enable syscon clock.\n");
> +		return ret;
> +	}
> +
>  	ret = clk_enable(sdhci_arasan->clk_ahb);
>  	if (ret) {
>  		dev_err(dev, "Cannot enable AHB clock.\n");
> @@ -528,26 +537,33 @@ static int sdhci_arasan_probe(struct platform_device
> *pdev) ret);
>  			goto err_pltfm_free;
>  		}
> +
> +		sdhci_arasan->clk_syscon = devm_clk_get(&pdev->dev,
> +							"clk_syscon");
> +		if (clk_prepare_enable(sdhci_arasan->clk_syscon)) {
> +			dev_err(&pdev->dev, "Unable to enable syscon clock.\n");
> +			goto err_pltfm_free;
> +		}

doesn't look very "optional" to me.
clk_get specifies:
"Returns a struct clk corresponding to the clock producer, or
valid IS_ERR() condition containing errno."

So later clk_* on that err_ptr will produce failures and the clock-framework 
could also request deferal.


Heiko

[toc] | [prev] | [next] | [standalone]

#1471652 — Re: [PATCH 2/4] mmc: sdhci-of-arasan: Control clock for accessing syscon

Hi Heiko,

On 2016/8/29 16:25, Heiko Stübner wrote:
> Hi Shawn,
>
> Am Montag, 29. August 2016, 16:02:57 schrieb Shawn Lin:
>> In the eariler commit 65820199272d ("Documentation: mmc:
>> sdhci-of-arasan: Add soc-ctl-syscon for corecfg regs"), we
>> introduced syscon to control corecfg_* stuff provided by
>> arasan. But given that we may need to ungate the clock for
>> accessing corecfg_*, it not so perfect as it depends on
>> whether specific clock driver disables it if not referenced.
>> Meanwhile, if we don't need arasan contoller to work anymore,
>> there is no reason to still enable it. So let's control this
>> clock when needed.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>
>>  drivers/mmc/host/sdhci-of-arasan.c | 25 ++++++++++++++++++++++---
>>  1 file changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>> b/drivers/mmc/host/sdhci-of-arasan.c index 0b3a9cf..7ae3ae4 100644
>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>> @@ -78,6 +78,7 @@ struct sdhci_arasan_soc_ctl_map {
>>   * struct sdhci_arasan_data
>>   * @host:		Pointer to the main SDHCI host structure.
>>   * @clk_ahb:		Pointer to the AHB clock
>> + * @clk_syscon:		Pointer to the optional clock for accessing syscon
>>   * @phy:		Pointer to the generic phy
>>   * @is_phy_on:		True if the PHY is on; false if not.
>>   * @sdcardclk_hw:	Struct for the clock we might provide to a PHY.
>> @@ -88,6 +89,7 @@ struct sdhci_arasan_soc_ctl_map {
>>  struct sdhci_arasan_data {
>>  	struct sdhci_host *host;
>>  	struct clk	*clk_ahb;
>> +	struct clk	*clk_syscon;
>>  	struct phy	*phy;
>>  	bool		is_phy_on;
>>
>> @@ -290,6 +292,7 @@ static int sdhci_arasan_suspend(struct device *dev)
>>
>>  	clk_disable(pltfm_host->clk);
>>  	clk_disable(sdhci_arasan->clk_ahb);
>> +	clk_disable(sdhci_arasan->clk_syscon);
>>
>>  	return 0;
>>  }
>> @@ -309,6 +312,12 @@ static int sdhci_arasan_resume(struct device *dev)
>>  	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>>  	int ret;
>>
>> +	ret = clk_enable(sdhci_arasan->clk_syscon);
>> +	if (ret) {
>> +		dev_err(dev, "Cannot enable syscon clock.\n");
>> +		return ret;
>> +	}
>> +
>>  	ret = clk_enable(sdhci_arasan->clk_ahb);
>>  	if (ret) {
>>  		dev_err(dev, "Cannot enable AHB clock.\n");
>> @@ -528,26 +537,33 @@ static int sdhci_arasan_probe(struct platform_device
>> *pdev) ret);
>>  			goto err_pltfm_free;
>>  		}
>> +
>> +		sdhci_arasan->clk_syscon = devm_clk_get(&pdev->dev,
>> +							"clk_syscon");
>> +		if (clk_prepare_enable(sdhci_arasan->clk_syscon)) {
>> +			dev_err(&pdev->dev, "Unable to enable syscon clock.\n");
>> +			goto err_pltfm_free;
>> +		}
>
> doesn't look very "optional" to me.
> clk_get specifies:
> "Returns a struct clk corresponding to the clock producer, or
> valid IS_ERR() condition containing errno."
>
> So later clk_* on that err_ptr will produce failures and the clock-framework
> could also request deferal.

Thanks for quick feedback.:)

It makes sense. I think it's just because clk_get request deferral, so
we could simply assign NULL to cly_syscon and let clk_* return 0
directly. So the deferral should be handle when getting other clks like
clk_ahb?

>
>
> Heiko
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Best Regards
Shawn Lin

[toc] | [prev] | [next] | [standalone]

#1471661

Hi Shawn,

Am Montag, 29. August 2016, 16:54:10 schrieb Shawn Lin:
> On 2016/8/29 16:25, Heiko Stübner wrote:
> > Am Montag, 29. August 2016, 16:02:57 schrieb Shawn Lin:
> >> In the eariler commit 65820199272d ("Documentation: mmc:
> >> sdhci-of-arasan: Add soc-ctl-syscon for corecfg regs"), we
> >> introduced syscon to control corecfg_* stuff provided by
> >> arasan. But given that we may need to ungate the clock for
> >> accessing corecfg_*, it not so perfect as it depends on
> >> whether specific clock driver disables it if not referenced.
> >> Meanwhile, if we don't need arasan contoller to work anymore,
> >> there is no reason to still enable it. So let's control this
> >> clock when needed.
> >> 
> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> >> ---
> >> 
> >>  drivers/mmc/host/sdhci-of-arasan.c | 25 ++++++++++++++++++++++---
> >>  1 file changed, 22 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
> >> b/drivers/mmc/host/sdhci-of-arasan.c index 0b3a9cf..7ae3ae4 100644
> >> --- a/drivers/mmc/host/sdhci-of-arasan.c
> >> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> >> @@ -78,6 +78,7 @@ struct sdhci_arasan_soc_ctl_map {
> >> 
> >>   * struct sdhci_arasan_data
> >>   * @host:		Pointer to the main SDHCI host structure.
> >>   * @clk_ahb:		Pointer to the AHB clock
> >> 
> >> + * @clk_syscon:		Pointer to the optional clock for accessing syscon
> >> 
> >>   * @phy:		Pointer to the generic phy
> >>   * @is_phy_on:		True if the PHY is on; false if not.
> >>   * @sdcardclk_hw:	Struct for the clock we might provide to a PHY.
> >> 
> >> @@ -88,6 +89,7 @@ struct sdhci_arasan_soc_ctl_map {
> >> 
> >>  struct sdhci_arasan_data {
> >>  
> >>  	struct sdhci_host *host;
> >>  	struct clk	*clk_ahb;
> >> 
> >> +	struct clk	*clk_syscon;
> >> 
> >>  	struct phy	*phy;
> >>  	bool		is_phy_on;
> >> 
> >> @@ -290,6 +292,7 @@ static int sdhci_arasan_suspend(struct device *dev)
> >> 
> >>  	clk_disable(pltfm_host->clk);
> >>  	clk_disable(sdhci_arasan->clk_ahb);
> >> 
> >> +	clk_disable(sdhci_arasan->clk_syscon);
> >> 
> >>  	return 0;
> >>  
> >>  }
> >> 
> >> @@ -309,6 +312,12 @@ static int sdhci_arasan_resume(struct device *dev)
> >> 
> >>  	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
> >>  	int ret;
> >> 
> >> +	ret = clk_enable(sdhci_arasan->clk_syscon);
> >> +	if (ret) {
> >> +		dev_err(dev, "Cannot enable syscon clock.\n");
> >> +		return ret;
> >> +	}
> >> +
> >> 
> >>  	ret = clk_enable(sdhci_arasan->clk_ahb);
> >>  	if (ret) {
> >>  	
> >>  		dev_err(dev, "Cannot enable AHB clock.\n");
> >> 
> >> @@ -528,26 +537,33 @@ static int sdhci_arasan_probe(struct
> >> platform_device
> >> *pdev) ret);
> >> 
> >>  			goto err_pltfm_free;
> >>  		
> >>  		}
> >> 
> >> +
> >> +		sdhci_arasan->clk_syscon = devm_clk_get(&pdev->dev,
> >> +							"clk_syscon");
> >> +		if (clk_prepare_enable(sdhci_arasan->clk_syscon)) {
> >> +			dev_err(&pdev->dev, "Unable to enable syscon clock.\n");
> >> +			goto err_pltfm_free;
> >> +		}
> > 
> > doesn't look very "optional" to me.
> > clk_get specifies:
> > "Returns a struct clk corresponding to the clock producer, or
> > valid IS_ERR() condition containing errno."
> > 
> > So later clk_* on that err_ptr will produce failures and the
> > clock-framework could also request deferal.
> 
> Thanks for quick feedback.:)
> 
> It makes sense. I think it's just because clk_get request deferral, so
> we could simply assign NULL to cly_syscon and let clk_* return 0
> directly. So the deferral should be handle when getting other clks like
> clk_ahb?

nope, I think the position itself is fine, so just do something like

if (IS_ERR(sdhci_arasan->clk_syscon)) {
{
	if (PTR_ERR(sdhci_arasan->clk_syscon) == -EPROBE_DEFER)
		return -EPROBE_DEFER;
	else
		sdhci_arasan->clk_syscon = NULL;
}

as the syscon clk would only ever be necessary if the soc-ctl-syscon is 
actually defined. But there is no need to move that section I think.

[toc] | [prev] | [next] | [standalone]

#1471666 — Re: [PATCH 2/4] mmc: sdhci-of-arasan: Control clock for accessing syscon

On 2016/8/29 17:10, Heiko Stübner wrote:
> Hi Shawn,
>
> Am Montag, 29. August 2016, 16:54:10 schrieb Shawn Lin:
>> On 2016/8/29 16:25, Heiko Stübner wrote:
>>> Am Montag, 29. August 2016, 16:02:57 schrieb Shawn Lin:
>>>> In the eariler commit 65820199272d ("Documentation: mmc:
>>>> sdhci-of-arasan: Add soc-ctl-syscon for corecfg regs"), we
>>>> introduced syscon to control corecfg_* stuff provided by
>>>> arasan. But given that we may need to ungate the clock for
>>>> accessing corecfg_*, it not so perfect as it depends on
>>>> whether specific clock driver disables it if not referenced.
>>>> Meanwhile, if we don't need arasan contoller to work anymore,
>>>> there is no reason to still enable it. So let's control this
>>>> clock when needed.
>>>>
>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>> ---
>>>>
>>>>  drivers/mmc/host/sdhci-of-arasan.c | 25 ++++++++++++++++++++++---
>>>>  1 file changed, 22 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>>>> b/drivers/mmc/host/sdhci-of-arasan.c index 0b3a9cf..7ae3ae4 100644
>>>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>>>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>>>> @@ -78,6 +78,7 @@ struct sdhci_arasan_soc_ctl_map {
>>>>
>>>>   * struct sdhci_arasan_data
>>>>   * @host:		Pointer to the main SDHCI host structure.
>>>>   * @clk_ahb:		Pointer to the AHB clock
>>>>
>>>> + * @clk_syscon:		Pointer to the optional clock for accessing syscon
>>>>
>>>>   * @phy:		Pointer to the generic phy
>>>>   * @is_phy_on:		True if the PHY is on; false if not.
>>>>   * @sdcardclk_hw:	Struct for the clock we might provide to a PHY.
>>>>
>>>> @@ -88,6 +89,7 @@ struct sdhci_arasan_soc_ctl_map {
>>>>
>>>>  struct sdhci_arasan_data {
>>>>
>>>>  	struct sdhci_host *host;
>>>>  	struct clk	*clk_ahb;
>>>>
>>>> +	struct clk	*clk_syscon;
>>>>
>>>>  	struct phy	*phy;
>>>>  	bool		is_phy_on;
>>>>
>>>> @@ -290,6 +292,7 @@ static int sdhci_arasan_suspend(struct device *dev)
>>>>
>>>>  	clk_disable(pltfm_host->clk);
>>>>  	clk_disable(sdhci_arasan->clk_ahb);
>>>>
>>>> +	clk_disable(sdhci_arasan->clk_syscon);
>>>>
>>>>  	return 0;
>>>>
>>>>  }
>>>>
>>>> @@ -309,6 +312,12 @@ static int sdhci_arasan_resume(struct device *dev)
>>>>
>>>>  	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>>>>  	int ret;
>>>>
>>>> +	ret = clk_enable(sdhci_arasan->clk_syscon);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "Cannot enable syscon clock.\n");
>>>> +		return ret;
>>>> +	}
>>>> +
>>>>
>>>>  	ret = clk_enable(sdhci_arasan->clk_ahb);
>>>>  	if (ret) {
>>>>  	
>>>>  		dev_err(dev, "Cannot enable AHB clock.\n");
>>>>
>>>> @@ -528,26 +537,33 @@ static int sdhci_arasan_probe(struct
>>>> platform_device
>>>> *pdev) ret);
>>>>
>>>>  			goto err_pltfm_free;
>>>>  		
>>>>  		}
>>>>
>>>> +
>>>> +		sdhci_arasan->clk_syscon = devm_clk_get(&pdev->dev,
>>>> +							"clk_syscon");
>>>> +		if (clk_prepare_enable(sdhci_arasan->clk_syscon)) {
>>>> +			dev_err(&pdev->dev, "Unable to enable syscon clock.\n");
>>>> +			goto err_pltfm_free;
>>>> +		}
>>>
>>> doesn't look very "optional" to me.
>>> clk_get specifies:
>>> "Returns a struct clk corresponding to the clock producer, or
>>> valid IS_ERR() condition containing errno."
>>>
>>> So later clk_* on that err_ptr will produce failures and the
>>> clock-framework could also request deferal.
>>
>> Thanks for quick feedback.:)
>>
>> It makes sense. I think it's just because clk_get request deferral, so
>> we could simply assign NULL to cly_syscon and let clk_* return 0
>> directly. So the deferral should be handle when getting other clks like
>> clk_ahb?
>
> nope, I think the position itself is fine, so just do something like
>
> if (IS_ERR(sdhci_arasan->clk_syscon)) {
> {
> 	if (PTR_ERR(sdhci_arasan->clk_syscon) == -EPROBE_DEFER)
> 		return -EPROBE_DEFER;
> 	else
> 		sdhci_arasan->clk_syscon = NULL;
> }
>
> as the syscon clk would only ever be necessary if the soc-ctl-syscon is
> actually defined. But there is no need to move that section I think.

except for other arasan's instances of some other venders who do have
soc-ctl-syscon but without any clk gate when accessing syscon,
possible? :)

>
>
>


-- 
Best Regards
Shawn Lin

[toc] | [prev] | [next] | [standalone]

#1471676 — Re: [PATCH 2/4] mmc: sdhci-of-arasan: Control clock for accessing syscon

在 2016/8/29 17:10, Heiko Stübner 写道:
> Hi Shawn,
>
> Am Montag, 29. August 2016, 16:54:10 schrieb Shawn Lin:
>> On 2016/8/29 16:25, Heiko Stübner wrote:
>>> Am Montag, 29. August 2016, 16:02:57 schrieb Shawn Lin:
>>>> In the eariler commit 65820199272d ("Documentation: mmc:
>>>> sdhci-of-arasan: Add soc-ctl-syscon for corecfg regs"), we
>>>> introduced syscon to control corecfg_* stuff provided by
>>>> arasan. But given that we may need to ungate the clock for
>>>> accessing corecfg_*, it not so perfect as it depends on
>>>> whether specific clock driver disables it if not referenced.
>>>> Meanwhile, if we don't need arasan contoller to work anymore,
>>>> there is no reason to still enable it. So let's control this
>>>> clock when needed.
>>>>
>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>> ---
>>>>
>>>>  drivers/mmc/host/sdhci-of-arasan.c | 25 ++++++++++++++++++++++---
>>>>  1 file changed, 22 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>>>> b/drivers/mmc/host/sdhci-of-arasan.c index 0b3a9cf..7ae3ae4 100644
>>>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>>>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>>>> @@ -78,6 +78,7 @@ struct sdhci_arasan_soc_ctl_map {
>>>>
>>>>   * struct sdhci_arasan_data
>>>>   * @host:		Pointer to the main SDHCI host structure.
>>>>   * @clk_ahb:		Pointer to the AHB clock
>>>>
>>>> + * @clk_syscon:		Pointer to the optional clock for accessing syscon
>>>>
>>>>   * @phy:		Pointer to the generic phy
>>>>   * @is_phy_on:		True if the PHY is on; false if not.
>>>>   * @sdcardclk_hw:	Struct for the clock we might provide to a PHY.
>>>>
>>>> @@ -88,6 +89,7 @@ struct sdhci_arasan_soc_ctl_map {
>>>>
>>>>  struct sdhci_arasan_data {
>>>>
>>>>  	struct sdhci_host *host;
>>>>  	struct clk	*clk_ahb;
>>>>
>>>> +	struct clk	*clk_syscon;
>>>>
>>>>  	struct phy	*phy;
>>>>  	bool		is_phy_on;
>>>>
>>>> @@ -290,6 +292,7 @@ static int sdhci_arasan_suspend(struct device *dev)
>>>>
>>>>  	clk_disable(pltfm_host->clk);
>>>>  	clk_disable(sdhci_arasan->clk_ahb);
>>>>
>>>> +	clk_disable(sdhci_arasan->clk_syscon);
>>>>
>>>>  	return 0;
>>>>
>>>>  }
>>>>
>>>> @@ -309,6 +312,12 @@ static int sdhci_arasan_resume(struct device *dev)
>>>>
>>>>  	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>>>>  	int ret;
>>>>
>>>> +	ret = clk_enable(sdhci_arasan->clk_syscon);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "Cannot enable syscon clock.\n");
>>>> +		return ret;
>>>> +	}
>>>> +
>>>>
>>>>  	ret = clk_enable(sdhci_arasan->clk_ahb);
>>>>  	if (ret) {
>>>>  	
>>>>  		dev_err(dev, "Cannot enable AHB clock.\n");
>>>>
>>>> @@ -528,26 +537,33 @@ static int sdhci_arasan_probe(struct
>>>> platform_device
>>>> *pdev) ret);
>>>>
>>>>  			goto err_pltfm_free;
>>>>  		
>>>>  		}
>>>>
>>>> +
>>>> +		sdhci_arasan->clk_syscon = devm_clk_get(&pdev->dev,
>>>> +							"clk_syscon");
>>>> +		if (clk_prepare_enable(sdhci_arasan->clk_syscon)) {
>>>> +			dev_err(&pdev->dev, "Unable to enable syscon clock.\n");
>>>> +			goto err_pltfm_free;
>>>> +		}
>>>
>>> doesn't look very "optional" to me.
>>> clk_get specifies:
>>> "Returns a struct clk corresponding to the clock producer, or
>>> valid IS_ERR() condition containing errno."
>>>
>>> So later clk_* on that err_ptr will produce failures and the
>>> clock-framework could also request deferal.
>>
>> Thanks for quick feedback.:)
>>
>> It makes sense. I think it's just because clk_get request deferral, so
>> we could simply assign NULL to cly_syscon and let clk_* return 0
>> directly. So the deferral should be handle when getting other clks like
>> clk_ahb?
>
> nope, I think the position itself is fine, so just do something like
>
> if (IS_ERR(sdhci_arasan->clk_syscon)) {
> {
> 	if (PTR_ERR(sdhci_arasan->clk_syscon) == -EPROBE_DEFER)
> 		return -EPROBE_DEFER;
> 	else
> 		sdhci_arasan->clk_syscon = NULL;
> }
>
> as the syscon clk would only ever be necessary if the soc-ctl-syscon is
> actually defined. But there is no need to move that section I think.
>

except for some arasan's instances for some other vendors who do have
soc-ctl-syscon to control corecfg_* but without any clk gate for the
accessing path, is it possible? :)

>
>


-- 
Best Regards
Shawn Lin

[toc] | [prev] | [next] | [standalone]

#1471710

Am Montag, 29. August 2016, 17:22:27 schrieb Shawn Lin:
> 在 2016/8/29 17:10, Heiko Stübner 写道:
> > Hi Shawn,
> > 
> > Am Montag, 29. August 2016, 16:54:10 schrieb Shawn Lin:
> >> On 2016/8/29 16:25, Heiko Stübner wrote:
> >>> Am Montag, 29. August 2016, 16:02:57 schrieb Shawn Lin:
> >>>> In the eariler commit 65820199272d ("Documentation: mmc:
> >>>> sdhci-of-arasan: Add soc-ctl-syscon for corecfg regs"), we
> >>>> introduced syscon to control corecfg_* stuff provided by
> >>>> arasan. But given that we may need to ungate the clock for
> >>>> accessing corecfg_*, it not so perfect as it depends on
> >>>> whether specific clock driver disables it if not referenced.
> >>>> Meanwhile, if we don't need arasan contoller to work anymore,
> >>>> there is no reason to still enable it. So let's control this
> >>>> clock when needed.
> >>>> 
> >>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> >>>> ---
> >>>> 
> >>>>  drivers/mmc/host/sdhci-of-arasan.c | 25 ++++++++++++++++++++++---
> >>>>  1 file changed, 22 insertions(+), 3 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
> >>>> b/drivers/mmc/host/sdhci-of-arasan.c index 0b3a9cf..7ae3ae4 100644
> >>>> --- a/drivers/mmc/host/sdhci-of-arasan.c
> >>>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> >>>> @@ -78,6 +78,7 @@ struct sdhci_arasan_soc_ctl_map {
> >>>> 
> >>>>   * struct sdhci_arasan_data
> >>>>   * @host:		Pointer to the main SDHCI host structure.
> >>>>   * @clk_ahb:		Pointer to the AHB clock
> >>>> 
> >>>> + * @clk_syscon:		Pointer to the optional clock for accessing syscon
> >>>> 
> >>>>   * @phy:		Pointer to the generic phy
> >>>>   * @is_phy_on:		True if the PHY is on; false if not.
> >>>>   * @sdcardclk_hw:	Struct for the clock we might provide to a PHY.
> >>>> 
> >>>> @@ -88,6 +89,7 @@ struct sdhci_arasan_soc_ctl_map {
> >>>> 
> >>>>  struct sdhci_arasan_data {
> >>>>  
> >>>>  	struct sdhci_host *host;
> >>>>  	struct clk	*clk_ahb;
> >>>> 
> >>>> +	struct clk	*clk_syscon;
> >>>> 
> >>>>  	struct phy	*phy;
> >>>>  	bool		is_phy_on;
> >>>> 
> >>>> @@ -290,6 +292,7 @@ static int sdhci_arasan_suspend(struct device *dev)
> >>>> 
> >>>>  	clk_disable(pltfm_host->clk);
> >>>>  	clk_disable(sdhci_arasan->clk_ahb);
> >>>> 
> >>>> +	clk_disable(sdhci_arasan->clk_syscon);
> >>>> 
> >>>>  	return 0;
> >>>>  
> >>>>  }
> >>>> 
> >>>> @@ -309,6 +312,12 @@ static int sdhci_arasan_resume(struct device *dev)
> >>>> 
> >>>>  	struct sdhci_arasan_data *sdhci_arasan =
> >>>>  	sdhci_pltfm_priv(pltfm_host);
> >>>>  	int ret;
> >>>> 
> >>>> +	ret = clk_enable(sdhci_arasan->clk_syscon);
> >>>> +	if (ret) {
> >>>> +		dev_err(dev, "Cannot enable syscon clock.\n");
> >>>> +		return ret;
> >>>> +	}
> >>>> +
> >>>> 
> >>>>  	ret = clk_enable(sdhci_arasan->clk_ahb);
> >>>>  	if (ret) {
> >>>>  	
> >>>>  		dev_err(dev, "Cannot enable AHB clock.\n");
> >>>> 
> >>>> @@ -528,26 +537,33 @@ static int sdhci_arasan_probe(struct
> >>>> platform_device
> >>>> *pdev) ret);
> >>>> 
> >>>>  			goto err_pltfm_free;
> >>>>  		
> >>>>  		}
> >>>> 
> >>>> +
> >>>> +		sdhci_arasan->clk_syscon = devm_clk_get(&pdev->dev,
> >>>> +							"clk_syscon");
> >>>> +		if (clk_prepare_enable(sdhci_arasan->clk_syscon)) {
> >>>> +			dev_err(&pdev->dev, "Unable to enable syscon clock.\n");
> >>>> +			goto err_pltfm_free;
> >>>> +		}
> >>> 
> >>> doesn't look very "optional" to me.
> >>> clk_get specifies:
> >>> "Returns a struct clk corresponding to the clock producer, or
> >>> valid IS_ERR() condition containing errno."
> >>> 
> >>> So later clk_* on that err_ptr will produce failures and the
> >>> clock-framework could also request deferal.
> >> 
> >> Thanks for quick feedback.:)
> >> 
> >> It makes sense. I think it's just because clk_get request deferral, so
> >> we could simply assign NULL to cly_syscon and let clk_* return 0
> >> directly. So the deferral should be handle when getting other clks like
> >> clk_ahb?
> > 
> > nope, I think the position itself is fine, so just do something like
> > 
> > if (IS_ERR(sdhci_arasan->clk_syscon)) {
> > {
> > 
> > 	if (PTR_ERR(sdhci_arasan->clk_syscon) == -EPROBE_DEFER)
> > 	
> > 		return -EPROBE_DEFER;
> > 	
> > 	else
> > 	
> > 		sdhci_arasan->clk_syscon = NULL;
> > 
> > }
> > 
> > as the syscon clk would only ever be necessary if the soc-ctl-syscon is
> > actually defined. But there is no need to move that section I think.
> 
> except for some arasan's instances for some other vendors who do have
> soc-ctl-syscon to control corecfg_* but without any clk gate for the
> accessing path, is it possible? :)

sure, if your struct clk reference is NULL, all the clk_* functions will just 
return sucessful ... see [0] and friends. So essentially you just handle the 
EPROBE_DEFER, because that means the clock is specified in the dts but not 
available yet and just ignore other errors by setting the clk to NULL.


[0] http://lxr.free-electrons.com/source/drivers/clk/clk.c#L774

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web