Re: [PATCH v3 05/13] of: Document timings subnode of nvidia,tegra-mc

[Tomeu Vizoso]

On 6 November 2014 09:12, Alexandre Courbot <acourbot@xxxxxxxxxx> wrote:
> On 10/30/2014 01:22 AM, Tomeu Vizoso wrote:
>>
>> The MC driver needs some timing-specific information to program the EMEM
>> during
>> a rate change of the EMC clock.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
>> ---
>>   .../memory-controllers/nvidia,tegra-mc.txt         | 46
>> +++++++++++++++++++++-
>>   1 file changed, 44 insertions(+), 2 deletions(-)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra-mc.txt
>> b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra-mc.txt
>> index f3db93c..8467b8c 100644
>> ---
>> a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra-mc.txt
>> +++
>> b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra-mc.txt
>> @@ -15,9 +15,26 @@ Required properties:
>>   This device implements an IOMMU that complies with the generic IOMMU
>> binding.
>>   See ../iommu/iommu.txt for details.
>>
>> -Example:
>> ---------
>> +The node should contain a "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 "timings" nodes :
>> +- nvidia,ram-code : Should contain the value of RAM_CODE this timing set
>> is used
>> +for.
>> +
>> +Each "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.
>
>
> In patch 04, a similar sub-node is named "emc-timings". Shouldn't the same
> name be used here for consistency?

Yeah, agreed.

> Also, it seems like there is a need for timing nodes in the MC to have match
> timing nodes in the CAR. It would be nice to add that information in all
> related bindings files.

Well, rather than the timing subnodes in a node having to match the
ones in another nodes, I think that all of them should match the
frequencies at which the EMC can run, which I think is clear enough
like this (already in each of the three bindings):

+Each "timings" node should contain a "timing" subnode for every
supported EMC clock rate.

>> +
>> +Required properties for "timing" nodes :
>> +- clock-frequency : Should contain the memory clock rate in Hz.
>> +- nvidia,emem-configuration : Values to be written to the EMEM register
>> block,
>> +as specified by the board documentation.
>
>
> Could you add some more information about which range of registers we are
> talking about, and whether they must all be specified or only part of them?
>
>
>> +
>> +Example SoC include file:
>> +
>> +/ {
>>         mc: memory-controller@0,70019000 {
>>                 compatible = "nvidia,tegra124-mc";
>>                 reg = <0x0 0x70019000 0x0 0x1000>;
>> @@ -34,3 +51,28 @@ Example:
>>                 ...
>>                 iommus = <&mc TEGRA_SWGROUP_SDMMC1A>;
>>         };
>> +};
>> +
>> +Example board file:
>> +
>> +/ {
>> +       memory-controller@0,70019000 {
>> +               timings@3 {
>> +                       nvidia,ram-code = <3>;
>> +
>> +                       timing@12750000 {
>> +                               clock-frequency = <12750000>;
>> +
>> +                               nvidia,emem-configuration = <
>> +                                       0x40040001 /* MC_EMEM_ARB_CFG */
>> +                                       0x8000000a /*
>> MC_EMEM_ARB_OUTSTANDING_REQ */
>> +                                       0x00000001 /*
>> MC_EMEM_ARB_TIMING_RCD */
>> +                                       0x00000001 /*
>> MC_EMEM_ARB_TIMING_RP */
>> +                                       0x00000002 /*
>> MC_EMEM_ARB_TIMING_RC */
>> +                                       0x00000000 /*
>> MC_EMEM_ARB_TIMING_RAS */
>> +                                       0x00000002 /*
>> MC_EMEM_ARB_TIMING_FAW */
>> +                               >;
>
>
> Looking at the actual board files I suppose this example here is incomplete.
> It would be nice to make this explicit, maybe by adding "..." on a new line
> to indicate more registers are expected.

Sounds good.

Thanks!

Tomeu


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