Re: [PATCH V3 1/8] dt-bindings: memory: tegra: Add external memory controller binding for Tegra210

[Joseph Lo <josephl@nvidia.com>]

On 5/15/19 12:28 AM, Dmitry Osipenko wrote:
> 10.05.2019 11:47, Joseph Lo пишет:
>> Add the binding document for the external memory controller (EMC) which
>> communicates with external LPDDR4 devices. It includes the bindings of
>> the EMC node and a sub-node of EMC table which under the reserved memory
>> node. The EMC table contains the data of the rates that EMC supported.
>>
>> Signed-off-by: Joseph Lo <josephl@nvidia.com>
>> ---
>> v3:
>> - drop the bindings of EMC table
>> - add memory-region and reserved-memory node for EMC table
>> ---
>>   .../nvidia,tegra210-emc.txt                   | 55 +++++++++++++++++++
>>   1 file changed, 55 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/memory-controllers/nvidia,tegra210-emc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra210-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra210-emc.txt
>> new file mode 100644
>> index 000000000000..d65aeef2329c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra210-emc.txt
>> @@ -0,0 +1,55 @@
>> +NVIDIA Tegra210 SoC EMC (external memory controller)
>> +====================================================
>> +
>> +Device node
>> +===========
>> +Required properties :
>> +- compatible : should be "nvidia,tegra210-emc".
>> +- reg : physical base address and length of the controller's registers.
>> +- clocks : phandles of the possible source clocks.
>> +- clock-names : names of the possible source clocks.
>> +- interrupts : Should contain the EMC general interrupt.
>> +- memory-region : phandle to the reserved memory (see
>> +  Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt) which
>> +  contains a sub-node of EMC table.
>> +- nvidia,memory-controller : phandle of the memory controller.
>> +
>> +Reserved memory node
>> +====================
>> +Should contain a sub-node of EMC table with required properties:
>> +- compatible : should be "nvidia,tegra210-emc-table".
>> +- reg : physical address and length of the location of EMC table.
>> +
>> +Example:
>> +	reserved-memory {
>> +		#address-cells = <2>;
>> +		#size-cells = <2>;
>> +		ranges;
>> +
>> +		emc_table: emc-table@8be00000 {
>> +			compatible = "nvidia,tegra210-emc-table";
>> +			reg = <0x0 0x8be00000 0x0 0x10000>;
>> +			status = "okay";
>> +		};
> 
> You essentially moved the v1 binding into obscure and undocumented blob,
> ignoring previous review comments. This is a very odd move... please
> explain what is going on.
> 

Discussed with Thierry offline which way we prefer to pass the EMC table 
to the kernel. Some reasons below we decide to chose this one (via 
binary blob).

- The EMC table is much bigger than the previous Tegra generations 
(LPDDR4 v.s. LPDDR2/3). It's harder to settle in the review process. And 
if there is a new fix of the table in the future, we'll need to go 
through that again.
- Because it's LPDDR4 we want to support here, to support higher rates, 
the devices have must be gone through the training process, which is 
done in the firmware. Which means We already have the table somewhere in 
the memory and kernel can just re-use that. No need to convert them back 
to DT and pass to the kernel. This is much easier to maintain in the 
future if there is something needs to fix.
- With the mechanism above, we don't need to maintain the huge EMC table 
in the DT file like below.
http://patchwork.ozlabs.org/patch/1063886/
http://patchwork.ozlabs.org/patch/1063889/

And sorry, maybe it's not clear at that moment, but I did mention that 
we want to go with the new method (via binary blob) in the previous review.
Please see http://patchwork.ozlabs.org/patch/1084467/

Thanks,
Joseph