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

[Dmitry Osipenko <digetx@gmail.com>]

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

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel