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

From: Mikko Perttunen
Date: Fri Nov 07 2014 - 06:35:57 EST

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@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
---
.../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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/