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  wrote:
> Rob,
>
> On Fri, Sep 2, 2016 at 7:15 AM, Rob Herring  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 
>>>
>>> ---
>>>
>>>  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

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  wrote:
> Rob,
>
> On Fri, Sep 2, 2016 at 7:15 AM, Rob Herring  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 
>>>
>>> ---
>>>
>>>  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

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

[Doug Anderson]

Rob,

On Fri, Sep 2, 2016 at 7:15 AM, Rob Herring  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 
>>
>> ---
>>
>>  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 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 
> 
> ---
> 
>  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

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

[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 

---

 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 0xfe33 0x0 0x1>;
interrupts = ;
-   clocks = < SCLK_EMMC>, < ACLK_EMMC>;
-   clock-names = "clk_xin", "clk_ahb";
+   clocks = < SCLK_EMMC>, < ACLK_EMMC>, < 
ACLK_EMMC_GRF>;
+   clock-names = "clk_xin", "clk_ahb", "clk_syscon";
arasan,soc-ctl-syscon = <>;
assigned-clocks = < SCLK_EMMC>;
assigned-clock-rates = <2>;
-- 
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 

---

 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 0xfe33 0x0 0x1>;
interrupts = ;
-   clocks = < SCLK_EMMC>, < ACLK_EMMC>;
-   clock-names = "clk_xin", "clk_ahb";
+   clocks = < SCLK_EMMC>, < ACLK_EMMC>, < 
ACLK_EMMC_GRF>;
+   clock-names = "clk_xin", "clk_ahb", "clk_syscon";
arasan,soc-ctl-syscon = <>;
assigned-clocks = < SCLK_EMMC>;
assigned-clock-rates = <2>;
-- 
2.3.7