On 5/15/19 9:50 PM, Dmitry Osipenko wrote:
15.05.2019 10:17, Joseph Lo пишет:
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@xxxxxxxxxx>
---
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.
I don't think that this a very good excuse for not documenting the
blob's structure.
The blob's structure is in patch 4 now that we originally wanted to
describe below. Basically, the content is the same.
http://patchwork.ozlabs.org/patch/1084467/
http://patchwork.ozlabs.org/patch/1063879/
- 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/
The blob's EMC table contains stuff specific to downstream kernel, hence
it's a not very re-usable downstream software ABI mixed with HW
description that you're bringing into upstream. This is not very
welcomed, although I don't see it as a big problem if you'll state that
all clearly in the commit message with a solid explanation why it is the
best possible option.
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/
Okay. It will be better if the discussion happened publicly, at least I
hope that Rob is involved in it.
