diff mbox series

[v6,04/52] dt-bindings: memory: tegra20: emc: Document nvidia,memory-controller property

Message ID 20201025221735.3062-5-digetx@gmail.com
State Accepted
Commit e51a59f079c546b8b5c31604ad98e86b06ec93e9
Headers show
Series [v6,01/52] clk: tegra: Export Tegra20 EMC kernel symbols | expand

Commit Message

Dmitry Osipenko Oct. 25, 2020, 10:16 p.m. UTC
Tegra20 External Memory Controller talks to DRAM chips and it needs to be
reprogrammed when memory frequency changes. Tegra Memory Controller sits
behind EMC and these controllers are tightly coupled. This patch adds the
new phandle property which allows to properly express connection of EMC
and MC hardware in a device-tree, it also put the Tegra20 EMC binding on
par with Tegra30+ EMC bindings, which is handy to have.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 .../bindings/memory-controllers/nvidia,tegra20-emc.txt          | 2 ++
 1 file changed, 2 insertions(+)

Comments

Krzysztof Kozlowski Oct. 27, 2020, 8:54 a.m. UTC | #1
On Mon, Oct 26, 2020 at 01:16:47AM +0300, Dmitry Osipenko wrote:
> Tegra20 External Memory Controller talks to DRAM chips and it needs to be
> reprogrammed when memory frequency changes. Tegra Memory Controller sits
> behind EMC and these controllers are tightly coupled. This patch adds the
> new phandle property which allows to properly express connection of EMC
> and MC hardware in a device-tree, it also put the Tegra20 EMC binding on
> par with Tegra30+ EMC bindings, which is handy to have.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  .../bindings/memory-controllers/nvidia,tegra20-emc.txt          | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> index 567cffd37f3f..1b0d4417aad8 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> @@ -12,6 +12,7 @@ Properties:
>    irrespective of ram-code configuration.
>  - interrupts : Should contain EMC General interrupt.
>  - clocks : Should contain EMC clock.
> +- nvidia,memory-controller : Phandle of the Memory Controller node.

It looks like you adding a required property which is an ABI break.

Best regards,
Krzysztof
Dmitry Osipenko Oct. 27, 2020, 7:17 p.m. UTC | #2
27.10.2020 11:54, Krzysztof Kozlowski пишет:
> On Mon, Oct 26, 2020 at 01:16:47AM +0300, Dmitry Osipenko wrote:
>> Tegra20 External Memory Controller talks to DRAM chips and it needs to be
>> reprogrammed when memory frequency changes. Tegra Memory Controller sits
>> behind EMC and these controllers are tightly coupled. This patch adds the
>> new phandle property which allows to properly express connection of EMC
>> and MC hardware in a device-tree, it also put the Tegra20 EMC binding on
>> par with Tegra30+ EMC bindings, which is handy to have.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  .../bindings/memory-controllers/nvidia,tegra20-emc.txt          | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
>> index 567cffd37f3f..1b0d4417aad8 100644
>> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
>> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
>> @@ -12,6 +12,7 @@ Properties:
>>    irrespective of ram-code configuration.
>>  - interrupts : Should contain EMC General interrupt.
>>  - clocks : Should contain EMC clock.
>> +- nvidia,memory-controller : Phandle of the Memory Controller node.
> 
> It looks like you adding a required property which is an ABI break.
The T20 EMC driver is unused so far in upstream and it will become used
only once this series is applied. Hence it's fine to change the ABI.
Krzysztof Kozlowski Oct. 27, 2020, 7:30 p.m. UTC | #3
On Tue, Oct 27, 2020 at 10:17:19PM +0300, Dmitry Osipenko wrote:
> 27.10.2020 11:54, Krzysztof Kozlowski пишет:
> > On Mon, Oct 26, 2020 at 01:16:47AM +0300, Dmitry Osipenko wrote:
> >> Tegra20 External Memory Controller talks to DRAM chips and it needs to be
> >> reprogrammed when memory frequency changes. Tegra Memory Controller sits
> >> behind EMC and these controllers are tightly coupled. This patch adds the
> >> new phandle property which allows to properly express connection of EMC
> >> and MC hardware in a device-tree, it also put the Tegra20 EMC binding on
> >> par with Tegra30+ EMC bindings, which is handy to have.
> >>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  .../bindings/memory-controllers/nvidia,tegra20-emc.txt          | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> >> index 567cffd37f3f..1b0d4417aad8 100644
> >> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> >> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> >> @@ -12,6 +12,7 @@ Properties:
> >>    irrespective of ram-code configuration.
> >>  - interrupts : Should contain EMC General interrupt.
> >>  - clocks : Should contain EMC clock.
> >> +- nvidia,memory-controller : Phandle of the Memory Controller node.
> > 
> > It looks like you adding a required property which is an ABI break.
> The T20 EMC driver is unused so far in upstream and it will become used
> only once this series is applied. Hence it's fine to change the ABI.

The ABI is not about upstream, but downstream. There are no other
upstreams using this ABI. Unless you have in mind that existing T20 EMC
driver was a noop, doing absolutely nothing, therefore there is no
breakage of any other users?

Best regards,
Krzysztof
Dmitry Osipenko Oct. 27, 2020, 8:37 p.m. UTC | #4
27.10.2020 22:30, Krzysztof Kozlowski пишет:
> On Tue, Oct 27, 2020 at 10:17:19PM +0300, Dmitry Osipenko wrote:
>> 27.10.2020 11:54, Krzysztof Kozlowski пишет:
>>> On Mon, Oct 26, 2020 at 01:16:47AM +0300, Dmitry Osipenko wrote:
>>>> Tegra20 External Memory Controller talks to DRAM chips and it needs to be
>>>> reprogrammed when memory frequency changes. Tegra Memory Controller sits
>>>> behind EMC and these controllers are tightly coupled. This patch adds the
>>>> new phandle property which allows to properly express connection of EMC
>>>> and MC hardware in a device-tree, it also put the Tegra20 EMC binding on
>>>> par with Tegra30+ EMC bindings, which is handy to have.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>>  .../bindings/memory-controllers/nvidia,tegra20-emc.txt          | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
>>>> index 567cffd37f3f..1b0d4417aad8 100644
>>>> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
>>>> @@ -12,6 +12,7 @@ Properties:
>>>>    irrespective of ram-code configuration.
>>>>  - interrupts : Should contain EMC General interrupt.
>>>>  - clocks : Should contain EMC clock.
>>>> +- nvidia,memory-controller : Phandle of the Memory Controller node.
>>>
>>> It looks like you adding a required property which is an ABI break.
>> The T20 EMC driver is unused so far in upstream and it will become used
>> only once this series is applied. Hence it's fine to change the ABI.
> 
> The ABI is not about upstream, but downstream. There are no other
> upstreams using this ABI. Unless you have in mind that existing T20 EMC
> driver was a noop, doing absolutely nothing, therefore there is no
> breakage of any other users?

The T20 EMC driver was a 100% noop for now. It's safe to change the ABI.

The T30/124 EMC drivers have users, but these are unsafe drivers (with
the known issues) until this series is applied.
Rob Herring Oct. 28, 2020, 3:23 p.m. UTC | #5
On Tue, Oct 27, 2020 at 08:30:39PM +0100, Krzysztof Kozlowski wrote:
> On Tue, Oct 27, 2020 at 10:17:19PM +0300, Dmitry Osipenko wrote:
> > 27.10.2020 11:54, Krzysztof Kozlowski пишет:
> > > On Mon, Oct 26, 2020 at 01:16:47AM +0300, Dmitry Osipenko wrote:
> > >> Tegra20 External Memory Controller talks to DRAM chips and it needs to be
> > >> reprogrammed when memory frequency changes. Tegra Memory Controller sits
> > >> behind EMC and these controllers are tightly coupled. This patch adds the
> > >> new phandle property which allows to properly express connection of EMC
> > >> and MC hardware in a device-tree, it also put the Tegra20 EMC binding on
> > >> par with Tegra30+ EMC bindings, which is handy to have.
> > >>
> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > >> ---
> > >>  .../bindings/memory-controllers/nvidia,tegra20-emc.txt          | 2 ++
> > >>  1 file changed, 2 insertions(+)
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> > >> index 567cffd37f3f..1b0d4417aad8 100644
> > >> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> > >> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> > >> @@ -12,6 +12,7 @@ Properties:
> > >>    irrespective of ram-code configuration.
> > >>  - interrupts : Should contain EMC General interrupt.
> > >>  - clocks : Should contain EMC clock.
> > >> +- nvidia,memory-controller : Phandle of the Memory Controller node.
> > > 
> > > It looks like you adding a required property which is an ABI break.
> > The T20 EMC driver is unused so far in upstream and it will become used
> > only once this series is applied. Hence it's fine to change the ABI.
> 
> The ABI is not about upstream, but downstream. 

"If it's not upstream, it doesn't exist."

Though we do have to account for out of tree users where the DT is not 
in tree, but upstream drivers are used. Downstream as in vendor kernels 
typically has loads of other crap.

> There are no other
> upstreams using this ABI. Unless you have in mind that existing T20 EMC
> driver was a noop, doing absolutely nothing, therefore there is no
> breakage of any other users?

ABI breaks are ultimately up to the platform maintainers to decide.

Rob
Krzysztof Kozlowski Oct. 28, 2020, 3:35 p.m. UTC | #6
On Wed, Oct 28, 2020 at 10:23:03AM -0500, Rob Herring wrote:
> On Tue, Oct 27, 2020 at 08:30:39PM +0100, Krzysztof Kozlowski wrote:
> > On Tue, Oct 27, 2020 at 10:17:19PM +0300, Dmitry Osipenko wrote:
> > > 27.10.2020 11:54, Krzysztof Kozlowski пишет:
> > > > On Mon, Oct 26, 2020 at 01:16:47AM +0300, Dmitry Osipenko wrote:
> > > >> Tegra20 External Memory Controller talks to DRAM chips and it needs to be
> > > >> reprogrammed when memory frequency changes. Tegra Memory Controller sits
> > > >> behind EMC and these controllers are tightly coupled. This patch adds the
> > > >> new phandle property which allows to properly express connection of EMC
> > > >> and MC hardware in a device-tree, it also put the Tegra20 EMC binding on
> > > >> par with Tegra30+ EMC bindings, which is handy to have.
> > > >>
> > > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > > >> ---
> > > >>  .../bindings/memory-controllers/nvidia,tegra20-emc.txt          | 2 ++
> > > >>  1 file changed, 2 insertions(+)
> > > >>
> > > >> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> > > >> index 567cffd37f3f..1b0d4417aad8 100644
> > > >> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> > > >> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> > > >> @@ -12,6 +12,7 @@ Properties:
> > > >>    irrespective of ram-code configuration.
> > > >>  - interrupts : Should contain EMC General interrupt.
> > > >>  - clocks : Should contain EMC clock.
> > > >> +- nvidia,memory-controller : Phandle of the Memory Controller node.
> > > > 
> > > > It looks like you adding a required property which is an ABI break.
> > > The T20 EMC driver is unused so far in upstream and it will become used
> > > only once this series is applied. Hence it's fine to change the ABI.
> > 
> > The ABI is not about upstream, but downstream. 
> 
> "If it's not upstream, it doesn't exist."
> 
> Though we do have to account for out of tree users where the DT is not 
> in tree, but upstream drivers are used. Downstream as in vendor kernels 
> typically has loads of other crap.

That's the case I am referring to. Maybe not in case of Tegra, but
multiple other designs are quite popular in industrial uses and their
DTSes were not upstreamed.

This is anyway different case, as Dmitry explained - nothing got broken
because not much was working before around this.

> 
> > There are no other
> > upstreams using this ABI. Unless you have in mind that existing T20 EMC
> > driver was a noop, doing absolutely nothing, therefore there is no
> > breakage of any other users?
> 
> ABI breaks are ultimately up to the platform maintainers to decide.

Cool! That reshapes significantly my existing point of view, especially
about discussions on Exynos bindings (long time ago). Thanks for
clarification.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
index 567cffd37f3f..1b0d4417aad8 100644
--- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
+++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
@@ -12,6 +12,7 @@  Properties:
   irrespective of ram-code configuration.
 - interrupts : Should contain EMC General interrupt.
 - clocks : Should contain EMC clock.
+- nvidia,memory-controller : Phandle of the Memory Controller node.
 
 Child device nodes describe the memory settings for different configurations and clock rates.
 
@@ -24,6 +25,7 @@  Example:
 		reg = <0x7000f400 0x400>;
 		interrupts = <0 78 0x04>;
 		clocks = <&tegra_car TEGRA20_CLK_EMC>;
+		nvidia,memory-controller = <&mc>;
 	}