diff mbox

[v3,04/13] of: document new emc-timings subnode in nvidia, tegra124-car

Message ID 1414599796-30597-5-git-send-email-tomeu.vizoso@collabora.com
State Superseded, archived
Headers show

Commit Message

Tomeu Vizoso Oct. 29, 2014, 4:22 p.m. UTC 
The EMC clock needs some extra information for changing its rate.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 .../bindings/clock/nvidia,tegra124-car.txt         | 46 +++++++++++++++++++++-
 1 file changed, 44 insertions(+), 2 deletions(-)

Comments

Alexandre Courbot Nov. 6, 2014, 6:37 a.m. UTC | #1 
On 10/30/2014 01:22 AM, Tomeu Vizoso wrote:
> The EMC clock needs some extra information for changing its rate.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>   .../bindings/clock/nvidia,tegra124-car.txt         | 46 +++++++++++++++++++++-
>   1 file changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
> index ded5d62..42e0588 100644
> --- a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
> @@ -19,12 +19,35 @@ Required properties :
>     In clock consumers, this cell represents the bit number in the CAR's
>     array of CLK_RST_CONTROLLER_RST_DEVICES_* registers.
>
> +The node should contain a "emc-timings" subnode for each supported RAM type (see
> +field RAM_CODE in register PMC_STRAPPING_OPT_A), with its unit address being its
> +RAM_CODE.
> +
> +Required properties for "emc-timings" nodes :
> +- nvidia,ram-code : Should contain the value of RAM_CODE this timing set
> +  is used for.
> +
> +Each "emc-timings" node should contain a "timing" subnode for every supported
> +EMC clock rate. The "timing" subnodes should have the clock rate in Hz as their
> +unit address.

This seems to be a quite liberal use of unit addresses (same in the next 
patch) - is this allowed by DT?

> +
> +Required properties for "timing" nodes :
> +- clock-frequency : Should contain the memory clock rate to which this timing
> +relates.
> +- nvidia,parent-clock-frequency : Should contain the rate at which the current
> +parent of the EMC clock should be running at this timing.
> +- clocks : Must contain an entry for each entry in clock-names.
> +  See ../clocks/clock-bindings.txt for details.
> +- clock-names : Must include the following entries:
> +  - emc-parent : the clock that should be the parent of the EMC clock at this
> +timing.
> +
>   Example SoC include file:
>
>   / {
> -	tegra_car: clock {
> +	tegra_car: clock@0,60006000 {
>   		compatible = "nvidia,tegra124-car";
> -		reg = <0x60006000 0x1000>;
> +		reg = <0x0 0x60006000 0x0 0x1000>;
>   		#clock-cells = <1>;
>   		#reset-cells = <1>;
>   	};
> @@ -60,4 +83,23 @@ Example board file:
>   	&tegra_car {
>   		clocks = <&clk_32k> <&osc>;
>   	};
> +
> +	clock@0,60006000 {
> +		emc-timings@3 {
> +			nvidia,ram-code = <3>;
> +
> +			timing@12750000 {
> +				clock-frequency = <12750000>;
> +				nvidia,parent-clock-frequency = <408000000>;
> +				clocks = <&tegra_car TEGRA124_CLK_PLL_P>;
> +				clock-names = "emc-parent";
> +			};
> +			timing@20400000 {
> +				clock-frequency = <20400000>;
> +				nvidia,parent-clock-frequency = <408000000>;
> +				clocks = <&tegra_car TEGRA124_CLK_PLL_P>;
> +				clock-names = "emc-parent";
> +			};
> +		};
> +	};

At first it seems confusing to see a top-level node without a compatible 
property, until you realize it has already been defined before.

In patch 05, you put "Example board file:" above a similar node, which 
is enough to lift that ambiguity - could you do the same here?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomeu Vizoso Nov. 6, 2014, 9:11 a.m. UTC | #2 
On 6 November 2014 07:37, Alexandre Courbot <acourbot@nvidia.com> wrote:
> On 10/30/2014 01:22 AM, Tomeu Vizoso wrote:
>>
>> The EMC clock needs some extra information for changing its rate.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>>   .../bindings/clock/nvidia,tegra124-car.txt         | 46
>> +++++++++++++++++++++-
>>   1 file changed, 44 insertions(+), 2 deletions(-)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>> b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>> index ded5d62..42e0588 100644
>> --- a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>> @@ -19,12 +19,35 @@ Required properties :
>>     In clock consumers, this cell represents the bit number in the CAR's
>>     array of CLK_RST_CONTROLLER_RST_DEVICES_* registers.
>>
>> +The node should contain a "emc-timings" subnode for each supported RAM
>> type (see
>> +field RAM_CODE in register PMC_STRAPPING_OPT_A), with its unit address
>> being its
>> +RAM_CODE.
>> +
>> +Required properties for "emc-timings" nodes :
>> +- nvidia,ram-code : Should contain the value of RAM_CODE this timing set
>> +  is used for.
>> +
>> +Each "emc-timings" node should contain a "timing" subnode for every
>> supported
>> +EMC clock rate. The "timing" subnodes should have the clock rate in Hz as
>> their
>> +unit address.
>
>
> This seems to be a quite liberal use of unit addresses (same in the next
> patch) - is this allowed by DT?

Well, it's not that different from using the register address. I think
it helps with readability more than an arbitrary index.

>> +
>> +Required properties for "timing" nodes :
>> +- clock-frequency : Should contain the memory clock rate to which this
>> timing
>> +relates.
>> +- nvidia,parent-clock-frequency : Should contain the rate at which the
>> current
>> +parent of the EMC clock should be running at this timing.
>> +- clocks : Must contain an entry for each entry in clock-names.
>> +  See ../clocks/clock-bindings.txt for details.
>> +- clock-names : Must include the following entries:
>> +  - emc-parent : the clock that should be the parent of the EMC clock at
>> this
>> +timing.
>> +
>>   Example SoC include file:
>>
>>   / {
>> -       tegra_car: clock {
>> +       tegra_car: clock@0,60006000 {
>>                 compatible = "nvidia,tegra124-car";
>> -               reg = <0x60006000 0x1000>;
>> +               reg = <0x0 0x60006000 0x0 0x1000>;
>>                 #clock-cells = <1>;
>>                 #reset-cells = <1>;
>>         };
>> @@ -60,4 +83,23 @@ Example board file:
>>         &tegra_car {
>>                 clocks = <&clk_32k> <&osc>;
>>         };
>> +
>> +       clock@0,60006000 {
>> +               emc-timings@3 {
>> +                       nvidia,ram-code = <3>;
>> +
>> +                       timing@12750000 {
>> +                               clock-frequency = <12750000>;
>> +                               nvidia,parent-clock-frequency =
>> <408000000>;
>> +                               clocks = <&tegra_car TEGRA124_CLK_PLL_P>;
>> +                               clock-names = "emc-parent";
>> +                       };
>> +                       timing@20400000 {
>> +                               clock-frequency = <20400000>;
>> +                               nvidia,parent-clock-frequency =
>> <408000000>;
>> +                               clocks = <&tegra_car TEGRA124_CLK_PLL_P>;
>> +                               clock-names = "emc-parent";
>> +                       };
>> +               };
>> +       };
>
>
> At first it seems confusing to see a top-level node without a compatible
> property, until you realize it has already been defined before.
>
> In patch 05, you put "Example board file:" above a similar node, which is
> enough to lift that ambiguity - could you do the same here?

Oh, actually it was already in the file. I took this idea from here
for patch 05.

Thanks,

Tomeu

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandre Courbot Nov. 6, 2014, 9:15 a.m. UTC | #3 
On 11/06/2014 06:11 PM, Tomeu Vizoso wrote:
> On 6 November 2014 07:37, Alexandre Courbot <acourbot@nvidia.com> wrote:
>> On 10/30/2014 01:22 AM, Tomeu Vizoso wrote:
>>>
>>> The EMC clock needs some extra information for changing its rate.
>>>
>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>> ---
>>>    .../bindings/clock/nvidia,tegra124-car.txt         | 46
>>> +++++++++++++++++++++-
>>>    1 file changed, 44 insertions(+), 2 deletions(-)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>>> b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>>> index ded5d62..42e0588 100644
>>> --- a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>>> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>>> @@ -19,12 +19,35 @@ Required properties :
>>>      In clock consumers, this cell represents the bit number in the CAR's
>>>      array of CLK_RST_CONTROLLER_RST_DEVICES_* registers.
>>>
>>> +The node should contain a "emc-timings" subnode for each supported RAM
>>> type (see
>>> +field RAM_CODE in register PMC_STRAPPING_OPT_A), with its unit address
>>> being its
>>> +RAM_CODE.
>>> +
>>> +Required properties for "emc-timings" nodes :
>>> +- nvidia,ram-code : Should contain the value of RAM_CODE this timing set
>>> +  is used for.
>>> +
>>> +Each "emc-timings" node should contain a "timing" subnode for every
>>> supported
>>> +EMC clock rate. The "timing" subnodes should have the clock rate in Hz as
>>> their
>>> +unit address.
>>
>>
>> This seems to be a quite liberal use of unit addresses (same in the next
>> patch) - is this allowed by DT?
>
> Well, it's not that different from using the register address. I think
> it helps with readability more than an arbitrary index.
>
>>> +
>>> +Required properties for "timing" nodes :
>>> +- clock-frequency : Should contain the memory clock rate to which this
>>> timing
>>> +relates.
>>> +- nvidia,parent-clock-frequency : Should contain the rate at which the
>>> current
>>> +parent of the EMC clock should be running at this timing.
>>> +- clocks : Must contain an entry for each entry in clock-names.
>>> +  See ../clocks/clock-bindings.txt for details.
>>> +- clock-names : Must include the following entries:
>>> +  - emc-parent : the clock that should be the parent of the EMC clock at
>>> this
>>> +timing.
>>> +
>>>    Example SoC include file:
>>>
>>>    / {
>>> -       tegra_car: clock {
>>> +       tegra_car: clock@0,60006000 {
>>>                  compatible = "nvidia,tegra124-car";
>>> -               reg = <0x60006000 0x1000>;
>>> +               reg = <0x0 0x60006000 0x0 0x1000>;
>>>                  #clock-cells = <1>;
>>>                  #reset-cells = <1>;
>>>          };
>>> @@ -60,4 +83,23 @@ Example board file:
>>>          &tegra_car {
>>>                  clocks = <&clk_32k> <&osc>;
>>>          };
>>> +
>>> +       clock@0,60006000 {
>>> +               emc-timings@3 {
>>> +                       nvidia,ram-code = <3>;
>>> +
>>> +                       timing@12750000 {
>>> +                               clock-frequency = <12750000>;
>>> +                               nvidia,parent-clock-frequency =
>>> <408000000>;
>>> +                               clocks = <&tegra_car TEGRA124_CLK_PLL_P>;
>>> +                               clock-names = "emc-parent";
>>> +                       };
>>> +                       timing@20400000 {
>>> +                               clock-frequency = <20400000>;
>>> +                               nvidia,parent-clock-frequency =
>>> <408000000>;
>>> +                               clocks = <&tegra_car TEGRA124_CLK_PLL_P>;
>>> +                               clock-names = "emc-parent";
>>> +                       };
>>> +               };
>>> +       };
>>
>>
>> At first it seems confusing to see a top-level node without a compatible
>> property, until you realize it has already been defined before.
>>
>> In patch 05, you put "Example board file:" above a similar node, which is
>> enough to lift that ambiguity - could you do the same here?
>
> Oh, actually it was already in the file. I took this idea from here
> for patch 05.

My bad, missed it!

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Nov. 6, 2014, 3:12 p.m. UTC | #4 
On Thu, Nov 6, 2014 at 12:37 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> On 10/30/2014 01:22 AM, Tomeu Vizoso wrote:
>>
>> The EMC clock needs some extra information for changing its rate.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>>   .../bindings/clock/nvidia,tegra124-car.txt         | 46
>> +++++++++++++++++++++-
>>   1 file changed, 44 insertions(+), 2 deletions(-)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>> b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>> index ded5d62..42e0588 100644
>> --- a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>> @@ -19,12 +19,35 @@ Required properties :
>>     In clock consumers, this cell represents the bit number in the CAR's
>>     array of CLK_RST_CONTROLLER_RST_DEVICES_* registers.
>>
>> +The node should contain a "emc-timings" subnode for each supported RAM
>> type (see
>> +field RAM_CODE in register PMC_STRAPPING_OPT_A), with its unit address
>> being its
>> +RAM_CODE.
>> +
>> +Required properties for "emc-timings" nodes :
>> +- nvidia,ram-code : Should contain the value of RAM_CODE this timing set
>> +  is used for.
>> +
>> +Each "emc-timings" node should contain a "timing" subnode for every
>> supported
>> +EMC clock rate. The "timing" subnodes should have the clock rate in Hz as
>> their
>> +unit address.
>
>
> This seems to be a quite liberal use of unit addresses (same in the next
> patch) - is this allowed by DT?

No, unit address should match a reg property.

>> +
>> +Required properties for "timing" nodes :
>> +- clock-frequency : Should contain the memory clock rate to which this
>> timing
>> +relates.
>> +- nvidia,parent-clock-frequency : Should contain the rate at which the
>> current
>> +parent of the EMC clock should be running at this timing.
>> +- clocks : Must contain an entry for each entry in clock-names.
>> +  See ../clocks/clock-bindings.txt for details.
>> +- clock-names : Must include the following entries:
>> +  - emc-parent : the clock that should be the parent of the EMC clock at
>> this
>> +timing.
>> +
>>   Example SoC include file:
>>
>>   / {
>> -       tegra_car: clock {
>> +       tegra_car: clock@0,60006000 {

The comma here is wrong. Commas should be used when you have something
like PCI bus:dev:func for addressing.

>>                 compatible = "nvidia,tegra124-car";
>> -               reg = <0x60006000 0x1000>;
>> +               reg = <0x0 0x60006000 0x0 0x1000>;

The number of cell's is really irrelevant to the example.

>>                 #clock-cells = <1>;
>>                 #reset-cells = <1>;
>>         };
>> @@ -60,4 +83,23 @@ Example board file:
>>         &tegra_car {
>>                 clocks = <&clk_32k> <&osc>;
>>         };
>> +
>> +       clock@0,60006000 {
>> +               emc-timings@3 {
>> +                       nvidia,ram-code = <3>;
>> +
>> +                       timing@12750000 {
>> +                               clock-frequency = <12750000>;
>> +                               nvidia,parent-clock-frequency =
>> <408000000>;
>> +                               clocks = <&tegra_car TEGRA124_CLK_PLL_P>;
>> +                               clock-names = "emc-parent";

Why do you need both clocks and hardcoded values? clock-frequency is
the desired freq you want to set TEGRA124_CLK_PLL_P to?

The clocks property really belongs as part of the memory controller
node or a memory device node.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandre Courbot Nov. 7, 2014, 5:31 a.m. UTC | #5 
On 11/07/2014 12:12 AM, Rob Herring wrote:
> On Thu, Nov 6, 2014 at 12:37 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>> On 10/30/2014 01:22 AM, Tomeu Vizoso wrote:
>>>
>>> The EMC clock needs some extra information for changing its rate.
>>>
>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>> ---
>>>    .../bindings/clock/nvidia,tegra124-car.txt         | 46
>>> +++++++++++++++++++++-
>>>    1 file changed, 44 insertions(+), 2 deletions(-)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>>> b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>>> index ded5d62..42e0588 100644
>>> --- a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>>> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>>> @@ -19,12 +19,35 @@ Required properties :
>>>      In clock consumers, this cell represents the bit number in the CAR's
>>>      array of CLK_RST_CONTROLLER_RST_DEVICES_* registers.
>>>
>>> +The node should contain a "emc-timings" subnode for each supported RAM
>>> type (see
>>> +field RAM_CODE in register PMC_STRAPPING_OPT_A), with its unit address
>>> being its
>>> +RAM_CODE.
>>> +
>>> +Required properties for "emc-timings" nodes :
>>> +- nvidia,ram-code : Should contain the value of RAM_CODE this timing set
>>> +  is used for.
>>> +
>>> +Each "emc-timings" node should contain a "timing" subnode for every
>>> supported
>>> +EMC clock rate. The "timing" subnodes should have the clock rate in Hz as
>>> their
>>> +unit address.
>>
>>
>> This seems to be a quite liberal use of unit addresses (same in the next
>> patch) - is this allowed by DT?
>
> No, unit address should match a reg property.

Mmm, would you have any suggestion as to how this can be fixed? Right 
now what I can think of is either to replace the "clock-frequency" 
property by "reg" (which would be confusing), or to use a different 
naming scheme, e.g. timing-12750000. IIUC the naming is not essential 
for properly parsing these nodes, so maybe the second solution is the 
way to go?

>
>>> +
>>> +Required properties for "timing" nodes :
>>> +- clock-frequency : Should contain the memory clock rate to which this
>>> timing
>>> +relates.
>>> +- nvidia,parent-clock-frequency : Should contain the rate at which the
>>> current
>>> +parent of the EMC clock should be running at this timing.
>>> +- clocks : Must contain an entry for each entry in clock-names.
>>> +  See ../clocks/clock-bindings.txt for details.
>>> +- clock-names : Must include the following entries:
>>> +  - emc-parent : the clock that should be the parent of the EMC clock at
>>> this
>>> +timing.
>>> +
>>>    Example SoC include file:
>>>
>>>    / {
>>> -       tegra_car: clock {
>>> +       tegra_car: clock@0,60006000 {
>
> The comma here is wrong. Commas should be used when you have something
> like PCI bus:dev:func for addressing.
>
>>>                  compatible = "nvidia,tegra124-car";
>>> -               reg = <0x60006000 0x1000>;
>>> +               reg = <0x0 0x60006000 0x0 0x1000>;
>
> The number of cell's is really irrelevant to the example.
>
>>>                  #clock-cells = <1>;
>>>                  #reset-cells = <1>;
>>>          };
>>> @@ -60,4 +83,23 @@ Example board file:
>>>          &tegra_car {
>>>                  clocks = <&clk_32k> <&osc>;
>>>          };
>>> +
>>> +       clock@0,60006000 {
>>> +               emc-timings@3 {
>>> +                       nvidia,ram-code = <3>;
>>> +
>>> +                       timing@12750000 {
>>> +                               clock-frequency = <12750000>;
>>> +                               nvidia,parent-clock-frequency =
>>> <408000000>;
>>> +                               clocks = <&tegra_car TEGRA124_CLK_PLL_P>;
>>> +                               clock-names = "emc-parent";
>
> Why do you need both clocks and hardcoded values? clock-frequency is
> the desired freq you want to set TEGRA124_CLK_PLL_P to?

That would be nvidia,parent-clock-frequency IIUC, while clock-frequency 
is the resulting EMC clock.

>
> The clocks property really belongs as part of the memory controller
> node or a memory device node.

I would tend to agree here. Tomeu, does it make sense to move these 
properties to the EMC driver instead?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikko Perttunen Nov. 7, 2014, 11:35 a.m. UTC | #6 
To better facilitate discussion, here's an outline of how the driver works:

emc_set_rate (CAR) gets called
   The current timing (= rate,parent pair) is checked along with the
     target timing.
   If the target timing has the same clock source (~parent, see
     emc_parent_clk_sources in the clk driver), find a timing with a
     different clock source. This is to prevent a situation where we
     have switched to a new parent rate but the EMC has not yet been
     reprogrammed. This new timing is called a backup timing.
   If a backup timing was required, switch to it (using this same
     procedure).
   Set the rate of the new parent to the required rate and enable it. It
     is important to use the correct parent for each timing, since in
     addition to the rate requirements, there are jitter requirements.
     The specified timings have been verified by a hardware team.
   Call tegra_emc_prepare_timing_change. This does assorted things to
     prepare the EMC block for a timing change and also tells the MC
     driver to prepare for it.
   Prepare the value that will be written to CLK_SOURCE_EMC in the CAR
     block. This value contains the divisor to get from the parent rate
     to the EMC rate and specifies the parent clock to use.
   Write the value. This must be done in a single write, as it starts a
     state machine that uses the written value.
   Call tegra_emc_complete_timing_change. This will lead the state
     machine to completion.
   Tell CCF that our parent is now the specified one.
   Disable the old parent.

Note that information about the parent is only needed within the CAR 
driver and not in EMC or MC.
Those drivers only need to map the EMC rate to a set of configuration 
values.

If the parent clock information is moved under the EMC node, we have 
some options:
a) Add API to EMC that the CAR driver can use to query the parent 
information.
b) Add API to CAR that the EMC driver can use to write the rate and 
parent information.
c) Other?

Cheers,
Mikko.

On 11/07/2014 07:31 AM, Alexandre Courbot wrote:
> On 11/07/2014 12:12 AM, Rob Herring wrote:
>> On Thu, Nov 6, 2014 at 12:37 AM, Alexandre Courbot
>> <acourbot@nvidia.com> wrote:
>>> On 10/30/2014 01:22 AM, Tomeu Vizoso wrote:
>>>>
>>>> The EMC clock needs some extra information for changing its rate.
>>>>
>>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>>> ---
>>>>    .../bindings/clock/nvidia,tegra124-car.txt         | 46
>>>> +++++++++++++++++++++-
>>>>    1 file changed, 44 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>>>> b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>>>> index ded5d62..42e0588 100644
>>>> --- a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>>>> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>>>> @@ -19,12 +19,35 @@ Required properties :
>>>>      In clock consumers, this cell represents the bit number in the
>>>> CAR's
>>>>      array of CLK_RST_CONTROLLER_RST_DEVICES_* registers.
>>>>
>>>> +The node should contain a "emc-timings" subnode for each supported RAM
>>>> type (see
>>>> +field RAM_CODE in register PMC_STRAPPING_OPT_A), with its unit address
>>>> being its
>>>> +RAM_CODE.
>>>> +
>>>> +Required properties for "emc-timings" nodes :
>>>> +- nvidia,ram-code : Should contain the value of RAM_CODE this
>>>> timing set
>>>> +  is used for.
>>>> +
>>>> +Each "emc-timings" node should contain a "timing" subnode for every
>>>> supported
>>>> +EMC clock rate. The "timing" subnodes should have the clock rate in
>>>> Hz as
>>>> their
>>>> +unit address.
>>>
>>>
>>> This seems to be a quite liberal use of unit addresses (same in the next
>>> patch) - is this allowed by DT?
>>
>> No, unit address should match a reg property.
>
> Mmm, would you have any suggestion as to how this can be fixed? Right
> now what I can think of is either to replace the "clock-frequency"
> property by "reg" (which would be confusing), or to use a different
> naming scheme, e.g. timing-12750000. IIUC the naming is not essential
> for properly parsing these nodes, so maybe the second solution is the
> way to go?
>
>>
>>>> +
>>>> +Required properties for "timing" nodes :
>>>> +- clock-frequency : Should contain the memory clock rate to which this
>>>> timing
>>>> +relates.
>>>> +- nvidia,parent-clock-frequency : Should contain the rate at which the
>>>> current
>>>> +parent of the EMC clock should be running at this timing.
>>>> +- clocks : Must contain an entry for each entry in clock-names.
>>>> +  See ../clocks/clock-bindings.txt for details.
>>>> +- clock-names : Must include the following entries:
>>>> +  - emc-parent : the clock that should be the parent of the EMC
>>>> clock at
>>>> this
>>>> +timing.
>>>> +
>>>>    Example SoC include file:
>>>>
>>>>    / {
>>>> -       tegra_car: clock {
>>>> +       tegra_car: clock@0,60006000 {
>>
>> The comma here is wrong. Commas should be used when you have something
>> like PCI bus:dev:func for addressing.
>>
>>>>                  compatible = "nvidia,tegra124-car";
>>>> -               reg = <0x60006000 0x1000>;
>>>> +               reg = <0x0 0x60006000 0x0 0x1000>;
>>
>> The number of cell's is really irrelevant to the example.
>>
>>>>                  #clock-cells = <1>;
>>>>                  #reset-cells = <1>;
>>>>          };
>>>> @@ -60,4 +83,23 @@ Example board file:
>>>>          &tegra_car {
>>>>                  clocks = <&clk_32k> <&osc>;
>>>>          };
>>>> +
>>>> +       clock@0,60006000 {
>>>> +               emc-timings@3 {
>>>> +                       nvidia,ram-code = <3>;
>>>> +
>>>> +                       timing@12750000 {
>>>> +                               clock-frequency = <12750000>;
>>>> +                               nvidia,parent-clock-frequency =
>>>> <408000000>;
>>>> +                               clocks = <&tegra_car
>>>> TEGRA124_CLK_PLL_P>;
>>>> +                               clock-names = "emc-parent";
>>
>> Why do you need both clocks and hardcoded values? clock-frequency is
>> the desired freq you want to set TEGRA124_CLK_PLL_P to?
>
> That would be nvidia,parent-clock-frequency IIUC, while clock-frequency
> is the resulting EMC clock.
>
>>
>> The clocks property really belongs as part of the memory controller
>> node or a memory device node.
>
> I would tend to agree here. Tomeu, does it make sense to move these
> properties to the EMC driver instead?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomeu Vizoso Nov. 7, 2014, 1:03 p.m. UTC | #7 
On 11/07/2014 06:31 AM, Alexandre Courbot wrote:
> On 11/07/2014 12:12 AM, Rob Herring wrote:
>> On Thu, Nov 6, 2014 at 12:37 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>>> On 10/30/2014 01:22 AM, Tomeu Vizoso wrote:
>>>>
>>>> The EMC clock needs some extra information for changing its rate.
>>>>
>>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>>> ---
>>>>    .../bindings/clock/nvidia,tegra124-car.txt         | 46
>>>> +++++++++++++++++++++-
>>>>    1 file changed, 44 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>>>> b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>>>> index ded5d62..42e0588 100644
>>>> --- a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>>>> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
>>>> @@ -19,12 +19,35 @@ Required properties :
>>>>      In clock consumers, this cell represents the bit number in the CAR's
>>>>      array of CLK_RST_CONTROLLER_RST_DEVICES_* registers.
>>>>
>>>> +The node should contain a "emc-timings" subnode for each supported RAM
>>>> type (see
>>>> +field RAM_CODE in register PMC_STRAPPING_OPT_A), with its unit address
>>>> being its
>>>> +RAM_CODE.
>>>> +
>>>> +Required properties for "emc-timings" nodes :
>>>> +- nvidia,ram-code : Should contain the value of RAM_CODE this timing set
>>>> +  is used for.
>>>> +
>>>> +Each "emc-timings" node should contain a "timing" subnode for every
>>>> supported
>>>> +EMC clock rate. The "timing" subnodes should have the clock rate in Hz as
>>>> their
>>>> +unit address.
>>>
>>>
>>> This seems to be a quite liberal use of unit addresses (same in the next
>>> patch) - is this allowed by DT?
>>
>> No, unit address should match a reg property.
> 
> Mmm, would you have any suggestion as to how this can be fixed? Right 
> now what I can think of is either to replace the "clock-frequency" 
> property by "reg" (which would be confusing), or to use a different 
> naming scheme, e.g. timing-12750000. IIUC the naming is not essential 
> for properly parsing these nodes, so maybe the second solution is the 
> way to go?

Yeah, there seems to be a precedent for embedding the frequency in the
node name in the TI DTs, e.g.: virt_12000000_ck.

Thanks,

Tomeu
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomeu Vizoso Nov. 7, 2014, 1:10 p.m. UTC | #8 
On 11/06/2014 04:12 PM, Rob Herring wrote:
> On Thu, Nov 6, 2014 at 12:37 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>> On 10/30/2014 01:22 AM, Tomeu Vizoso wrote:
>>>                 #clock-cells = <1>;
>>>                 #reset-cells = <1>;
>>>         };
>>> @@ -60,4 +83,23 @@ Example board file:
>>>         &tegra_car {
>>>                 clocks = <&clk_32k> <&osc>;
>>>         };
>>> +
>>> +       clock@0,60006000 {
>>> +               emc-timings@3 {
>>> +                       nvidia,ram-code = <3>;
>>> +
>>> +                       timing@12750000 {
>>> +                               clock-frequency = <12750000>;
>>> +                               nvidia,parent-clock-frequency =
>>> <408000000>;
>>> +                               clocks = <&tegra_car TEGRA124_CLK_PLL_P>;
>>> +                               clock-names = "emc-parent";
> 
> Why do you need both clocks and hardcoded values? clock-frequency is
> the desired freq you want to set TEGRA124_CLK_PLL_P to?

For the EMC clock to run at 12.75 MHz, its parent should become
TEGRA124_CLK_PLL_P and it has to be running at 408 MHz.

> The clocks property really belongs as part of the memory controller
> node or a memory device node.

What would be the rationale for that? The clock provider needs to know
what clock should become the parent of the EMC clock when changing rates.

Thanks,

Tomeu
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
index ded5d62..42e0588 100644
--- a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
+++ b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
@@ -19,12 +19,35 @@  Required properties :
   In clock consumers, this cell represents the bit number in the CAR's
   array of CLK_RST_CONTROLLER_RST_DEVICES_* registers.
 
+The node should contain a "emc-timings" subnode for each supported RAM type (see
+field RAM_CODE in register PMC_STRAPPING_OPT_A), with its unit address being its
+RAM_CODE.
+
+Required properties for "emc-timings" nodes :
+- nvidia,ram-code : Should contain the value of RAM_CODE this timing set
+  is used for.
+
+Each "emc-timings" node should contain a "timing" subnode for every supported
+EMC clock rate. The "timing" subnodes should have the clock rate in Hz as their
+unit address.
+
+Required properties for "timing" nodes :
+- clock-frequency : Should contain the memory clock rate to which this timing
+relates.
+- nvidia,parent-clock-frequency : Should contain the rate at which the current
+parent of the EMC clock should be running at this timing.
+- clocks : Must contain an entry for each entry in clock-names.
+  See ../clocks/clock-bindings.txt for details.
+- clock-names : Must include the following entries:
+  - emc-parent : the clock that should be the parent of the EMC clock at this
+timing.
+
 Example SoC include file:
 
 / {
-	tegra_car: clock {
+	tegra_car: clock@0,60006000 {
 		compatible = "nvidia,tegra124-car";
-		reg = <0x60006000 0x1000>;
+		reg = <0x0 0x60006000 0x0 0x1000>;
 		#clock-cells = <1>;
 		#reset-cells = <1>;
 	};
@@ -60,4 +83,23 @@  Example board file:
 	&tegra_car {
 		clocks = <&clk_32k> <&osc>;
 	};
+
+	clock@0,60006000 {
+		emc-timings@3 {
+			nvidia,ram-code = <3>;
+
+			timing@12750000 {
+				clock-frequency = <12750000>;
+				nvidia,parent-clock-frequency = <408000000>;
+				clocks = <&tegra_car TEGRA124_CLK_PLL_P>;
+				clock-names = "emc-parent";
+			};
+			timing@20400000 {
+				clock-frequency = <20400000>;
+				nvidia,parent-clock-frequency = <408000000>;
+				clocks = <&tegra_car TEGRA124_CLK_PLL_P>;
+				clock-names = "emc-parent";
+			};
+		};
+	};
 };