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

From: Dmitry Osipenko <digetx@gmail.com>
To: Joseph Lo <josephl@nvidia.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	Peter De Schrijver <pdeschrijver@nvidia.com>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Rob Herring <robh+dt@kernel.org>, Stephen Boyd <sboyd@kernel.org>
Cc: linux-tegra@vger.kernel.org, devicetree@vger.kernel.org,
	linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH V3 1/8] dt-bindings: memory: tegra: Add external memory controller binding for Tegra210
Date: Thu, 16 May 2019 17:39:05 +0300	[thread overview]
Message-ID: <1a5b539a-03bb-c326-8ba2-56ea8de97237@gmail.com> (raw)
In-Reply-To: <f1e4ab1e-11b9-ba65-20b8-35c6459d2410@nvidia.com>

16.05.2019 12:01, Joseph Lo пишет:
> 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/

I'm not asking about what exactly it is, but saying that every supported
blob structure version should be documented in my opinion, otherwise the
documentation is not really useful.