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