| Message ID | 20190510084719.18902-2-josephl@nvidia.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | Add EMC scaling support for Tegra210 | expand |
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.
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
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.
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. >
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 --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>; + };
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