Linux-Clk Archive on lore.kernel.org
 help / Atom feed
From: Joseph Lo <josephl@nvidia.com>
To: Dmitry Osipenko <digetx@gmail.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:01:19 +0800
Message-ID: <f1e4ab1e-11b9-ba65-20b8-35c6459d2410@nvidia.com> (raw)
In-Reply-To: <c3909c66-5aa4-161b-98d4-02ad20e02c88@gmail.com>

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

  reply index

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-10  8:47 [PATCH V3 0/8] Add EMC scaling support " Joseph Lo
2019-05-10  8:47 ` [PATCH V3 1/8] dt-bindings: memory: tegra: Add external memory controller binding " Joseph Lo
2019-05-14 16:28   ` Dmitry Osipenko
2019-05-15  7:17     ` Joseph Lo
2019-05-15 13:50       ` Dmitry Osipenko
2019-05-16  9:01         ` Joseph Lo [this message]
2019-05-16 14:39           ` Dmitry Osipenko
2019-05-10  8:47 ` [PATCH V3 2/8] clk: tegra: Add PLLP_UD and PLLMB_UD " Joseph Lo
2019-05-10  8:47 ` [PATCH V3 3/8] clk: tegra: Export functions for EMC clock scaling Joseph Lo
2019-05-14 16:29   ` Dmitry Osipenko
2019-05-15  7:25     ` Joseph Lo
2019-05-10  8:47 ` [PATCH V3 4/8] memory: tegra: Add Tegra210 EMC clock driver Joseph Lo
2019-05-13 16:54   ` Dmitry Osipenko
2019-05-14  9:22     ` Joseph Lo
2019-05-14 17:04   ` Dmitry Osipenko
2019-05-15  8:42     ` Joseph Lo
2019-05-15 15:25       ` Dmitry Osipenko
2019-05-16  7:52         ` Joseph Lo
2019-05-16 14:29           ` Dmitry Osipenko
2019-05-10  8:47 ` [PATCH V3 5/8] memory: tegra: Add EMC scaling support code for Tegra210 Joseph Lo
2019-05-13 17:02   ` Dmitry Osipenko
2019-05-14  8:47     ` Joseph Lo
2019-05-14 16:30   ` Dmitry Osipenko
2019-05-15 14:09   ` Dmitry Osipenko
2019-05-15 15:26   ` Dmitry Osipenko
2019-05-10  8:47 ` [PATCH V3 6/8] memory: tegra: Add EMC scaling sequence " Joseph Lo
2019-05-10  8:47 ` [PATCH V3 7/8] clk: tegra: Remove the old emc_mux clock " Joseph Lo
2019-05-10  8:47 ` [PATCH V3 8/8] arm64: tegra: Add external memory controller node " Joseph Lo

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f1e4ab1e-11b9-ba65-20b8-35c6459d2410@nvidia.com \
    --to=josephl@nvidia.com \
    --cc=devicetree@vger.kernel.org \
    --cc=digetx@gmail.com \
    --cc=jonathanh@nvidia.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=pdeschrijver@nvidia.com \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=thierry.reding@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org linux-clk@archiver.kernel.org
	public-inbox-index linux-clk


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/ public-inbox