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

Joseph Lo
2019-05-16T09:01:19Z

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

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.