[PATCH 0/4] Control clock for accessing syscon provided arasan IP

[PATCH 0/4] Control clock for accessing syscon provided arasan IP

[Shawn Lin]

This patchset is gonna take over the ownership of aclk_emmc_grf
by sdhci-of-arasan which is used to access corecfg_* stuff provided
by arasan. This clock is optional for sdhci-of-arasan which means we
would not break the driver  without adding this. But we strongly
recommend the upcoming users whose platforms have the similar
cases to add this rather than to add any flags for their specific
clock drivers to make this clock always on.

This patchset is based on linux-next. And patch 4 should *not* be
landed before others, so it would be nice to pack these four patches
via Ulf's tree tree Heiko's ack for rockchip part, or vice versa. :)



Shawn Lin (4):
  Documentation: mmc: sdhci-of-arasan: Add clk_syscon as an optional one
  mmc: sdhci-of-arasan: Control clock for accessing syscon
  arm64: dts: rockchip: add clk_syscon for sdhci on rk3399
  clk: rockchip: remove CLK_IGNORE_UNUSED flag for aclk_emmc_grf on
    rk3399

 .../devicetree/bindings/mmc/arasan,sdhci.txt       |  7 ++++--
 arch/arm64/boot/dts/rockchip/rk3399.dtsi           |  5 +++--
 drivers/clk/rockchip/clk-rk3399.c                  |  2 +-
 drivers/mmc/host/sdhci-of-arasan.c                 | 25 +++++++++++++++++++---
 4 files changed, 31 insertions(+), 8 deletions(-)

-- 
2.3.7

[PATCH 1/4] Documentation: mmc: sdhci-of-arasan: Add clk_syscon as an optional one

[Shawn Lin]

We introduced soc-ctl-syscon to do several things, for instance, update
baseclk or update clkmul, etc. In odrder to access this physical block,
we need to explicitly enable its clock. Currently we don't control this
clock as we always add a CLK_IGNORE_UNUSED flag for it to indicate that
we will not gate it even if not referenced. This is not a correct way since
it is a clock parenting from clk_ahb which is used by sdhci-of-arasan now.
Without enabling clk_ahb, the flag don't guarantee we could access
soc-ctl-syscon. Moreover, we can't find a reason not to gate clk_syscon
once we remove/power-down emmc controller. So let's add clk_syscon and
enable/disable it explicitly when needed.

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

---

 Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
index 3404afa..b04eb02 100644
--- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
+++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
@@ -33,6 +33,9 @@ Optional Properties:
   - clock-output-names: If specified, this will be the name of the card clock
     which will be exposed by this device.  Required if #clock-cells is
     specified.
+  - clock-names: From clock bindings: Although we treat clock-names as required
+    property, there is still one, "clk_syscon", should be optional as it depends
+    on whether we need to control soc-ctl-syscon or not.
   - #clock-cells: If specified this should be the value <0>.  With this property
     in place we will export a clock representing the Card Clock.  This clock
     is expected to be consumed by our PHY.  You must also specify
@@ -62,8 +65,8 @@ Example:
 		compatible = "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1";
 		reg = <0x0 0xfe330000 0x0 0x10000>;
 		interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
-		clocks = <&cru SCLK_EMMC>, <&cru ACLK_EMMC>;
-		clock-names = "clk_xin", "clk_ahb";
+		clocks = <&cru SCLK_EMMC>, <&cru ACLK_EMMC>, <&cru ACLK_EMMC_GRF>;
+		clock-names = "clk_xin", "clk_ahb", "clk_syscon";
 		arasan,soc-ctl-syscon = <&grf>;
 		assigned-clocks = <&cru SCLK_EMMC>;
 		assigned-clock-rates = <200000000>;
-- 
2.3.7

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

[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;
+		}
 	}
 
 	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

[PATCH 3/4] arm64: dts: rockchip: add clk_syscon for sdhci on rk3399

[Shawn Lin]

We are intent on letting the sdhci variant driver handle this
optional clock on rk3399 platform now.

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

 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index bc86e8c..d26c6ad 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -233,8 +233,9 @@
 		arasan,soc-ctl-syscon = <&grf>;
 		assigned-clocks = <&cru SCLK_EMMC>;
 		assigned-clock-rates = <200000000>;
-		clocks = <&cru SCLK_EMMC>, <&cru ACLK_EMMC>;
-		clock-names = "clk_xin", "clk_ahb";
+		clocks = <&cru SCLK_EMMC>, <&cru ACLK_EMMC>,
+			 <&cru ACLK_EMMC_GRF>;
+		clock-names = "clk_xin", "clk_ahb", "clk_syscon";
 		clock-output-names = "emmc_cardclock";
 		#clock-cells = <0>;
 		phys = <&emmc_phy>;
-- 
2.3.7

[PATCH 4/4] clk: rockchip: remove CLK_IGNORE_UNUSED flag for aclk_emmc_grf on rk3399

[Shawn Lin]

aclk_emmc_grf is used for accessing corecfg_* of emmc stuff within
GRF block. We don't need to add CLK_IGNORE_UNUSED for it now as the
emmc driver will enable/disable it explicitly when needed.

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

 drivers/clk/rockchip/clk-rk3399.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
index ede6c47..908f684 100644
--- a/drivers/clk/rockchip/clk-rk3399.c
+++ b/drivers/clk/rockchip/clk-rk3399.c
@@ -934,7 +934,7 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
 			RK3399_CLKGATE_CON(32), 8, GFLAGS),
 	GATE(ACLK_EMMC_NOC, "aclk_emmc_noc", "aclk_emmc", CLK_IGNORE_UNUSED,
 			RK3399_CLKGATE_CON(32), 9, GFLAGS),
-	GATE(ACLK_EMMC_GRF, "aclk_emmcgrf", "aclk_emmc", CLK_IGNORE_UNUSED,
+	GATE(ACLK_EMMC_GRF, "aclk_emmcgrf", "aclk_emmc", 0,
 			RK3399_CLKGATE_CON(32), 10, GFLAGS),
 
 	/* perilp0 */
-- 
2.3.7

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

[Heiko Stübner]

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

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

[Shawn Lin]

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

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

[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.

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

[Shawn Lin]

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

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

[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? :)

>
>


-- 
Best Regards
Shawn Lin

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

[Heiko Stübner]

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

Re: [PATCH 1/4] Documentation: mmc: sdhci-of-arasan: Add clk_syscon as an optional one

[Rob Herring]

On Mon, Aug 29, 2016 at 04:02:56PM +0800, Shawn Lin wrote:
> We introduced soc-ctl-syscon to do several things, for instance, update
> baseclk or update clkmul, etc. In odrder to access this physical block,
> we need to explicitly enable its clock. Currently we don't control this
> clock as we always add a CLK_IGNORE_UNUSED flag for it to indicate that
> we will not gate it even if not referenced. This is not a correct way since
> it is a clock parenting from clk_ahb which is used by sdhci-of-arasan now.
> Without enabling clk_ahb, the flag don't guarantee we could access
> soc-ctl-syscon. Moreover, we can't find a reason not to gate clk_syscon
> once we remove/power-down emmc controller. So let's add clk_syscon and
> enable/disable it explicitly when needed.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 
> ---
> 
>  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> index 3404afa..b04eb02 100644
> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> @@ -33,6 +33,9 @@ Optional Properties:
>    - clock-output-names: If specified, this will be the name of the card clock
>      which will be exposed by this device.  Required if #clock-cells is
>      specified.
> +  - clock-names: From clock bindings: Although we treat clock-names as required
> +    property, there is still one, "clk_syscon", should be optional as it depends
> +    on whether we need to control soc-ctl-syscon or not.

No. This doesn't look right to me. The syscon is a separate block 
and the clock for it belongs in the syscon node itself. Probably there 
needs to be some sort of ref counting in the syscon so it can do 
runtime-pm.

Rob

Re: [PATCH 1/4] Documentation: mmc: sdhci-of-arasan: Add clk_syscon as an optional one

[Doug Anderson]

Rob,

On Fri, Sep 2, 2016 at 7:15 AM, Rob Herring <robh@kernel.org> wrote:
> On Mon, Aug 29, 2016 at 04:02:56PM +0800, Shawn Lin wrote:
>> We introduced soc-ctl-syscon to do several things, for instance, update
>> baseclk or update clkmul, etc. In odrder to access this physical block,
>> we need to explicitly enable its clock. Currently we don't control this
>> clock as we always add a CLK_IGNORE_UNUSED flag for it to indicate that
>> we will not gate it even if not referenced. This is not a correct way since
>> it is a clock parenting from clk_ahb which is used by sdhci-of-arasan now.
>> Without enabling clk_ahb, the flag don't guarantee we could access
>> soc-ctl-syscon. Moreover, we can't find a reason not to gate clk_syscon
>> once we remove/power-down emmc controller. So let's add clk_syscon and
>> enable/disable it explicitly when needed.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> ---
>>
>>  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> index 3404afa..b04eb02 100644
>> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> @@ -33,6 +33,9 @@ Optional Properties:
>>    - clock-output-names: If specified, this will be the name of the card clock
>>      which will be exposed by this device.  Required if #clock-cells is
>>      specified.
>> +  - clock-names: From clock bindings: Although we treat clock-names as required
>> +    property, there is still one, "clk_syscon", should be optional as it depends
>> +    on whether we need to control soc-ctl-syscon or not.
>
> No. This doesn't look right to me. The syscon is a separate block
> and the clock for it belongs in the syscon node itself. Probably there
> needs to be some sort of ref counting in the syscon so it can do
> runtime-pm.

I'm not an expert, but one thing to note is that this is actually a
separate clock just for this small part of the GRF (general register
files).  Yeah, it's bizarre.

Said another way, the GRF is a sorta hodgepodge location for all sorts
of stuff.  ...included in there are the "corecfg" registers that are
used by the SDHCI IP block.  Within the GRF register space these
registers are 0x0f000 - 0x0f050 (last register is at 0x0f04c).  I
believe that only registers in that special range need this clock.

Maybe the right answer is that we should actually have had a sub-node
of the GRF for these registers and then SDHCI should have referenced
that as its syscon?

-Doug

Re: [PATCH 1/4] Documentation: mmc: sdhci-of-arasan: Add clk_syscon as an optional one

[Rob Herring]

On Fri, Sep 2, 2016 at 11:12 AM, Doug Anderson <dianders@chromium.org> wrote:
> Rob,
>
> On Fri, Sep 2, 2016 at 7:15 AM, Rob Herring <robh@kernel.org> wrote:
>> On Mon, Aug 29, 2016 at 04:02:56PM +0800, Shawn Lin wrote:
>>> We introduced soc-ctl-syscon to do several things, for instance, update
>>> baseclk or update clkmul, etc. In odrder to access this physical block,
>>> we need to explicitly enable its clock. Currently we don't control this
>>> clock as we always add a CLK_IGNORE_UNUSED flag for it to indicate that
>>> we will not gate it even if not referenced. This is not a correct way since
>>> it is a clock parenting from clk_ahb which is used by sdhci-of-arasan now.
>>> Without enabling clk_ahb, the flag don't guarantee we could access
>>> soc-ctl-syscon. Moreover, we can't find a reason not to gate clk_syscon
>>> once we remove/power-down emmc controller. So let's add clk_syscon and
>>> enable/disable it explicitly when needed.
>>>
>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>
>>> ---
>>>
>>>  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>>> index 3404afa..b04eb02 100644
>>> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>>> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>>> @@ -33,6 +33,9 @@ Optional Properties:
>>>    - clock-output-names: If specified, this will be the name of the card clock
>>>      which will be exposed by this device.  Required if #clock-cells is
>>>      specified.
>>> +  - clock-names: From clock bindings: Although we treat clock-names as required
>>> +    property, there is still one, "clk_syscon", should be optional as it depends
>>> +    on whether we need to control soc-ctl-syscon or not.
>>
>> No. This doesn't look right to me. The syscon is a separate block
>> and the clock for it belongs in the syscon node itself. Probably there
>> needs to be some sort of ref counting in the syscon so it can do
>> runtime-pm.
>
> I'm not an expert, but one thing to note is that this is actually a
> separate clock just for this small part of the GRF (general register
> files).  Yeah, it's bizarre.

I wasn't doubting that. Only that the connection is not to the sdhci block.

> Said another way, the GRF is a sorta hodgepodge location for all sorts
> of stuff.  ...included in there are the "corecfg" registers that are
> used by the SDHCI IP block.  Within the GRF register space these
> registers are 0x0f000 - 0x0f050 (last register is at 0x0f04c).  I
> believe that only registers in that special range need this clock.
>
> Maybe the right answer is that we should actually have had a sub-node
> of the GRF for these registers and then SDHCI should have referenced
> that as its syscon?

Sub-node or not is an orthogonal question. The main part is now we
have a syscon link and we need to power manage the syscon. Or is there
really a need to manage this clock at runtime? Perhaps it can just be
turned on and left on, but that should be in the syscon driver. The
SDHCI driver could retrieve the clocks from syscon node and manage it
directly. I'd be fine with that from a binding perspective, but seems
a bit hacky from a kernel perspective.

Rob