diff mbox series

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

Message ID 20190510084719.18902-2-josephl@nvidia.com (mailing list archive)
State Superseded, archived
Headers show
Series Add EMC scaling support for Tegra210 | expand

Commit Message

Joseph Lo May 10, 2019, 8:47 a.m. UTC
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

Comments

Dmitry Osipenko May 14, 2019, 4:28 p.m. UTC | #1
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.
Joseph Lo May 15, 2019, 7:17 a.m. UTC | #2
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.
- 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/

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/

Thanks,
Joseph
Dmitry Osipenko May 15, 2019, 1:50 p.m. UTC | #3
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.
Joseph Lo May 16, 2019, 9:01 a.m. UTC | #4
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.
>
Dmitry Osipenko May 16, 2019, 2:39 p.m. UTC | #5
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.
diff mbox series

Patch

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";
+		};
+	};
+
+	external-memory-controller@7001b000 {
+		compatible = "nvidia,tegra210-emc";
+		reg = <0x0 0x7001b000 0x0 0x1000>,
+		      <0x0 0x7001e000 0x0 0x1000>,
+		      <0x0 0x7001f000 0x0 0x1000>;
+		clocks = <&tegra_car TEGRA210_CLK_EMC>,
+			 <&tegra_car TEGRA210_CLK_PLL_M>,
+			 <&tegra_car TEGRA210_CLK_PLL_C>,
+			 <&tegra_car TEGRA210_CLK_PLL_P>,
+			 <&tegra_car TEGRA210_CLK_CLK_M>,
+			 <&tegra_car TEGRA210_CLK_PLL_M_UD>,
+			 <&tegra_car TEGRA210_CLK_PLL_MB_UD>,
+			 <&tegra_car TEGRA210_CLK_PLL_MB>,
+			 <&tegra_car TEGRA210_CLK_PLL_P_UD>;
+		clock-names = "emc", "pll_m", "pll_c", "pll_p", "clk_m",
+			      "pll_m_ud", "pll_mb_ud", "pll_mb", "pll_p_ud";
+		interrupts = <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>;
+		memory-region = <&emc_table>;
+		nvidia,memory-controller = <&mc>;
+	};