| Message ID | 1318873976-25335-1-git-send-email-olof@lixom.net |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, Oct 17, 2011 at 10:52 AM, Olof Johansson <olof@lixom.net> wrote: > First cut at device tree bindings for the EMC tables on tegra. > > Note that I have a prerequisite patch that changes the tegra2_emc code > to be a platform driver; but I wanted to do a sanity-check of my device > tree usage here before posting the whole series. > > Signed-off-by: Olof Johansson <olof@lixom.net> > --- > .../devicetree/bindings/arm/tegra/emc.txt | 77 ++++++++++++++++++++ > arch/arm/boot/dts/tegra-seaboard.dts | 36 +++++++++ > arch/arm/boot/dts/tegra20.dtsi | 7 ++ > 3 files changed, 120 insertions(+), 0 deletions(-) > create mode 100644 Documentation/devicetree/bindings/arm/tegra/emc.txt > > diff --git a/Documentation/devicetree/bindings/arm/tegra/emc.txt b/Documentation/devicetree/bindings/arm/tegra/emc.txt > new file mode 100644 > index 0000000..dd7f845 > --- /dev/null > +++ b/Documentation/devicetree/bindings/arm/tegra/emc.txt > @@ -0,0 +1,77 @@ > +Embedded Memory Controller > + > +Properties: > +- name : Should be emc > +- #address-cells : Should be 1 > +- #size-cells : Should be 0 > +- compatible : should contain "nvidia,tegra20-emc". > +- reg : Offset and length of the register set for the device > +- nvidia,use-ram-code : If present, the sub-nodes will be addressed > + and chosen using the ramcode board selector. If omitted, only one > + set of tables can be present and said tables will be used > + irrespective of ram-code configuration. > + > +Child device nodes describe the memory settings for different configurations and clock rates. > + > +Example: > + > + emc@7000f400 { > + #address-cells = < 1 >; > + #size-cells = < 0 >; > + compatible = "nvidia,tegra20-emc"; > + reg = <0x7000f4000 0x200>; > + } > + > + > +Embedded Memory Controller configuration table > + > +This is a table containing the EMC register settings for the various > +operating speeds of the memory controller. They are always located as > +subnodes of the emc controller node. > + > +There are two ways of specifying which tables to use: > + > +* The simplest is if there is just one set of tables in the device tree, > + and they will always be used (based on which frequency is used). > + This is the preferred method, especially when firmware can fill in > + this information based on the specific system information and just > + pass it on to the kernel. > + > +* The slightly more complex one is when more than one memory configuration > + might exist on the system. The Tegra20 platform handles this during > + early boot by selecting one out of possible 4 memory settings based > + on a 2-pin "ram code" bootstrap setting on the board. The values of > + these strappings can be read through a register in the SoC, and thus > + used to select which tables to use. > + > +Properties: > +- name : Should start with emc-table > +- compatible : should contain "nvidia,tegra20-emc-table". > +- reg : only needed if nvidia,use-ram-code is present in the This looks good to me. And in the future if we don't use the boot ROM defined RAM code straps we can define a new nvidia,use-xxx-code capability to specify how to select the table. Thanks, Anton > + parent. If so, the numerical representation of the selected ram code > + as reported by the strap option APB misc register. > +- clock-frequency : the clock frequency for the EMC at which this > + table should be used (in kHz). > +- nvidia,emc-registers : a 46 word array of EMC registers to be programmed > + for operation at the 'clock-frequency' setting. > + The order and contents of the registers are defined in the Tegra TRM. > + > + emc-table-333mhz@0 { > + reg = <0>; > + compatible = "nvidia,tegra20-emc-table"; > + clock-frequency = < 333000 >; > + nvidia,emc-registers = < 0 0 0 0 0 0 0 0 0 0 0 0 0 0 > + 0 0 0 0 0 0 0 0 0 0 0 0 0 0 > + 0 0 0 0 0 0 0 0 0 0 0 0 0 0 > + 0 0 0 0 >; > + }; > + > + emc-table-666mhz@0 { > + reg = <0>; > + compatible = "nvidia,tegra20-emc-table"; > + clock-frequency = < 666000 >; > + nvidia,emc-registers = < 0 0 0 0 0 0 0 0 0 0 0 0 0 0 > + 0 0 0 0 0 0 0 0 0 0 0 0 0 0 > + 0 0 0 0 0 0 0 0 0 0 0 0 0 0 > + 0 0 0 0 >; > + }; > diff --git a/arch/arm/boot/dts/tegra-seaboard.dts b/arch/arm/boot/dts/tegra-seaboard.dts > index a72299b..2d60b5ff 100644 > --- a/arch/arm/boot/dts/tegra-seaboard.dts > +++ b/arch/arm/boot/dts/tegra-seaboard.dts > @@ -20,6 +20,42 @@ > clock-frequency = < 216000000 >; > }; > > + emc { > + emc-table-166 { > + compatible = "nvidia,tegra20-emc-table"; > + clock-frequency = < 166500 >; > + nvidia,emc-registers = < 0x0000000a 0x00000021 > + 0x00000008 0x00000003 0x00000004 0x00000004 > + 0x00000002 0x0000000c 0x00000003 0x00000003 > + 0x00000002 0x00000001 0x00000004 0x00000005 > + 0x00000004 0x00000009 0x0000000d 0x000004df > + 0x00000000 0x00000003 0x00000003 0x00000003 > + 0x00000003 0x00000001 0x0000000a 0x000000c8 > + 0x00000003 0x00000006 0x00000004 0x0000000f > + 0x00000002 0x00000000 0x00000000 0x00000002 > + 0x00000000 0x00000000 0x00000083 0xa04004ae > + 0x007fd010 0x00000000 0x00000000 0x00000000 > + 0x00000000 0x00000000 0x00000000 0x00000000 >; > + }; > + > + emc-table-333 { > + compatible = "nvidia,tegra20-emc-table"; > + clock-frequency = < 333000 >; > + nvidia,emc-registers = < 0x00000014 0x00000041 > + 0x0000000f 0x00000005 0x00000004 0x00000005 > + 0x00000003 0x0000000c 0x00000005 0x00000005 > + 0x00000003 0x00000001 0x00000004 0x00000005 > + 0x00000004 0x00000009 0x0000000d 0x000009ff > + 0x00000000 0x00000003 0x00000003 0x00000005 > + 0x00000005 0x00000001 0x0000000f 0x000000c8 > + 0x00000003 0x0000000c 0x00000006 0x0000000f > + 0x00000002 0x00000000 0x00000000 0x00000002 > + 0x00000000 0x00000000 0x00000083 0xe034048b > + 0x007e8010 0x00000000 0x00000000 0x00000000 > + 0x00000000 0x00000000 0x00000000 0x00000000 >; > + }; > + }; > + > sdhci@c8000400 { > cd-gpios = <&gpio 69 0>; /* gpio PI5 */ > wp-gpios = <&gpio 57 0>; /* gpio PH1 */ > diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi > index 65d7e6a..f4dc8d3 100644 > --- a/arch/arm/boot/dts/tegra20.dtsi > +++ b/arch/arm/boot/dts/tegra20.dtsi > @@ -120,6 +120,13 @@ > interrupts = < 123 >; > }; > > + emc@7000f400 { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "nvidia,tegra20-emc"; > + reg = <0x7000f400 0x200>; > + }; > + > sdhci@c8000000 { > compatible = "nvidia,tegra20-sdhci"; > reg = <0xc8000000 0x200>; > -- > 1.7.4.1 > > _______________________________________________ > devicetree-discuss mailing list > devicetree-discuss@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Olof Johansson wrote at Monday, October 17, 2011 11:53 AM: > First cut at device tree bindings for the EMC tables on tegra. > > Note that I have a prerequisite patch that changes the tegra2_emc code > to be a platform driver; but I wanted to do a sanity-check of my device > tree usage here before posting the whole series. ... > +++ b/Documentation/devicetree/bindings/arm/tegra/emc.txt ... > +Embedded Memory Controller > + > +Properties: > +- name : Should be emc > +- #address-cells : Should be 1 > +- #size-cells : Should be 0 The address/size-cells properties define the require number of cells for child nodes, where the child nodes are addressable objects in a bus. For the EMC table child nodes, they're more configuration than nodes on a bus, so I don't think it's appropriate for the EMC controller to define the address/size-cells properties. Those properties would be appropriate if we ever have child nodes for the individual SDRAM chips, but I suspect we won't ever do that? ... > +Embedded Memory Controller configuration table ... > +Properties: > +- name : Should start with emc-table > +- compatible : should contain "nvidia,tegra20-emc-table". > +- reg : only needed if nvidia,use-ram-code is present in the > + parent. If so, the numerical representation of the selected ram code > + as reported by the strap option APB misc register. I don't think the compatible or reg properties are needed; the EMC tables aren't addressable objects on a bus, so no need for reg. I could see an argument that we'd want to version the emc-table format, and so the compatible flag might be useful, yet AIUI, compatible is more for defining HW compatibility (and hence driver instantiation), rather than a property of some configuration node. > +- clock-frequency : the clock frequency for the EMC at which this > + table should be used (in kHz). > +- nvidia,emc-registers : a 46 word array of EMC registers to be programmed > + for operation at the 'clock-frequency' setting. > + The order and contents of the registers are defined in the Tegra TRM. That's not a very semantic representation. Still, I guess there isn't any benefit representing this at a higher level. ... > +++ b/arch/arm/boot/dts/tegra-seaboard.dts > @@ -20,6 +20,42 @@ > clock-frequency = < 216000000 >; > }; > > + emc { I think that should be emc@7000f400, so it matches tegra20.dtsi?
On Tue, Oct 18, 2011 at 11:30 AM, Stephen Warren <swarren@nvidia.com> wrote: > Olof Johansson wrote at Monday, October 17, 2011 11:53 AM: >> First cut at device tree bindings for the EMC tables on tegra. >> >> Note that I have a prerequisite patch that changes the tegra2_emc code >> to be a platform driver; but I wanted to do a sanity-check of my device >> tree usage here before posting the whole series. > > ... >> +++ b/Documentation/devicetree/bindings/arm/tegra/emc.txt > ... >> +Embedded Memory Controller >> + >> +Properties: >> +- name : Should be emc >> +- #address-cells : Should be 1 >> +- #size-cells : Should be 0 > > The address/size-cells properties define the require number of cells for > child nodes, where the child nodes are addressable objects in a bus. For > the EMC table child nodes, they're more configuration than nodes on a > bus, so I don't think it's appropriate for the EMC controller to define > the address/size-cells properties. Those properties would be appropriate > if we ever have child nodes for the individual SDRAM chips, but I suspect > we won't ever do that? I'm not aware of anyone going down to the granularity of representing actual memory chips in the device tree. It's more common to use SPD on DIMMs for it. [Also, see below] > ... >> +Embedded Memory Controller configuration table > ... >> +Properties: >> +- name : Should start with emc-table > >> +- compatible : should contain "nvidia,tegra20-emc-table". >> +- reg : only needed if nvidia,use-ram-code is present in the >> + parent. If so, the numerical representation of the selected ram code >> + as reported by the strap option APB misc register. > > I don't think the compatible or reg properties are needed; the EMC tables > aren't addressable objects on a bus, so no need for reg. I could see an > argument that we'd want to version the emc-table format, and so the > compatible flag might be useful, yet AIUI, compatible is more for defining > HW compatibility (and hence driver instantiation), rather than a property > of some configuration node. I was struggling with a good way to specify the selection of the modules. I can definitely use a nvidia,ram-code property instead of reg (with a similar definition to how reg was used here). Compatible is still needed, in my opinion -- otherwise there will be no way to tell if the node is there to describe emc timings or if it's some new node used to describe something else (such as SDRAM chips as mentioned above). >> +- clock-frequency : the clock frequency for the EMC at which this >> + table should be used (in kHz). >> +- nvidia,emc-registers : a 46 word array of EMC registers to be programmed >> + for operation at the 'clock-frequency' setting. >> + The order and contents of the registers are defined in the Tegra TRM. > > That's not a very semantic representation. Still, I guess there isn't > any benefit representing this at a higher level. Yeah, it's not all that pretty but I don't see much value in describing the actual values for which the register values are derived from. It's all black magic from start to finish, as far as I am concerned (i.e. we've always received a register blob from nvidia for various boards with very little information on why and what certain values were tweaked like they were). >> +++ b/arch/arm/boot/dts/tegra-seaboard.dts >> @@ -20,6 +20,42 @@ >> clock-frequency = < 216000000 >; >> }; >> >> + emc { > > I think that should be emc@7000f400, so it matches tegra20.dtsi? If there is only one entry, unit address should be optional. I thought this would match it up with the one from tegra20.dtsi and overlay that already. I'll double-check on that dtc behavior and change if needed. -Olof -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Olof Johansson wrote at Tuesday, October 18, 2011 12:43 PM: > On Tue, Oct 18, 2011 at 11:30 AM, Stephen Warren <swarren@nvidia.com> wrote: > > Olof Johansson wrote at Monday, October 17, 2011 11:53 AM: > > ... > >> +Embedded Memory Controller configuration table > > ... > >> +Properties: > >> +- name : Should start with emc-table > > > >> +- compatible : should contain "nvidia,tegra20-emc-table". > >> +- reg : only needed if nvidia,use-ram-code is present in the > >> + parent. If so, the numerical representation of the selected ram code > >> + as reported by the strap option APB misc register. > > > > I don't think the compatible or reg properties are needed; the EMC tables > > aren't addressable objects on a bus, so no need for reg. I could see an > > argument that we'd want to version the emc-table format, and so the > > compatible flag might be useful, yet AIUI, compatible is more for defining > > HW compatibility (and hence driver instantiation), rather than a property > > of some configuration node. > > I was struggling with a good way to specify the selection of the > modules. I can definitely use a nvidia,ram-code property instead of > reg (with a similar definition to how reg was used here). Ah, so reg is the ram-code value. I missed that. Using an explicitly named property for this seems better to me, but I'll defer to DT experts on what's the standard practice for this. > Compatible is still needed, in my opinion -- otherwise there will be > no way to tell if the node is there to describe emc timings or if it's > some new node used to describe something else (such as SDRAM chips as > mentioned above). Can't you go by node name; enumerate all nodes with a particular name. Or define another intermediate node that will always contain tables and nothing else, then just enumerate all child nodes of that node: emc@xxxxx { emc-tables { table-333@0 {}; table-666@0 {}; }; }; The Tegra pinmux bindings I proposed certainly used this technique; a main node with a well-known name, followed by enumeration of all child nodes of that, and nobody /said/ anything about that being a bad idea.
On Tue, Oct 18, 2011 at 11:54 AM, Stephen Warren <swarren@nvidia.com> wrote: > Olof Johansson wrote at Tuesday, October 18, 2011 12:43 PM: >> On Tue, Oct 18, 2011 at 11:30 AM, Stephen Warren <swarren@nvidia.com> wrote: >> > Olof Johansson wrote at Monday, October 17, 2011 11:53 AM: >> > ... >> >> +Embedded Memory Controller configuration table >> > ... >> >> +Properties: >> >> +- name : Should start with emc-table >> > >> >> +- compatible : should contain "nvidia,tegra20-emc-table". >> >> +- reg : only needed if nvidia,use-ram-code is present in the >> >> + parent. If so, the numerical representation of the selected ram code >> >> + as reported by the strap option APB misc register. >> > >> > I don't think the compatible or reg properties are needed; the EMC tables >> > aren't addressable objects on a bus, so no need for reg. I could see an >> > argument that we'd want to version the emc-table format, and so the >> > compatible flag might be useful, yet AIUI, compatible is more for defining >> > HW compatibility (and hence driver instantiation), rather than a property >> > of some configuration node. >> >> I was struggling with a good way to specify the selection of the >> modules. I can definitely use a nvidia,ram-code property instead of >> reg (with a similar definition to how reg was used here). > > Ah, so reg is the ram-code value. I missed that. Using an explicitly > named property for this seems better to me, but I'll defer to DT experts > on what's the standard practice for this. Sounds like for these binding-specific things the rules are pretty loose in general, but see below on my new proposal. >> Compatible is still needed, in my opinion -- otherwise there will be >> no way to tell if the node is there to describe emc timings or if it's >> some new node used to describe something else (such as SDRAM chips as >> mentioned above). > > Can't you go by node name; enumerate all nodes with a particular name. > Or define another intermediate node that will always contain tables and > nothing else, then just enumerate all child nodes of that node: > > emc@xxxxx { > emc-tables { > table-333@0 {}; > table-666@0 {}; > }; > }; > > The Tegra pinmux bindings I proposed certainly used this technique; a > main node with a well-known name, followed by enumeration of all child > nodes of that, and nobody /said/ anything about that being a bad idea. I'm not really picky on this, but I think I would rather use a compatible field than rely on naming. That being said, doing a two-level approach will probably make it easier than the flat structure I initially had. So: emc@xxx { nvidia,use-ram-code; emc-table-ram-code-0 { nvidia,ram-code = < 0 >; table-166 { compatible = "tegra20-emc-table"; ... }; table-333 { ... }; }; emc-table-ram-code-1 { nvidia,ram-code = < 1 >; ... }; }; ... and for none-ram-code, just leave out the emc-table-ramcode-x level. So, for nvidia,use-ram-code case, it'll be one intermediate step of finding the right subnode, the rest of the table setup code will be common. None of it will be bound to actual node names though -- first step is iterating child nodes looking for nvidia,ram-code properties to match, and second step iterates by matching compatible fields. -Olof -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Olof Johansson wrote at Tuesday, October 18, 2011 2:54 PM: > On Tue, Oct 18, 2011 at 11:54 AM, Stephen Warren <swarren@nvidia.com> wrote: > > Olof Johansson wrote at Tuesday, October 18, 2011 12:43 PM: ... > >> Compatible is still needed, in my opinion -- otherwise there will be > >> no way to tell if the node is there to describe emc timings or if it's > >> some new node used to describe something else (such as SDRAM chips as > >> mentioned above). > > > > Can't you go by node name; enumerate all nodes with a particular name. > > Or define another intermediate node that will always contain tables and > > nothing else, then just enumerate all child nodes of that node: > > > > emc@xxxxx { > > emc-tables { > > table-333@0 {}; > > table-666@0 {}; > > }; > > }; > > > > The Tegra pinmux bindings I proposed certainly used this technique; a > > main node with a well-known name, followed by enumeration of all child > > nodes of that, and nobody /said/ anything about that being a bad idea. > > I'm not really picky on this, but I think I would rather use a > compatible field than rely on naming. > > That being said, doing a two-level approach will probably make it > easier than the flat structure I initially had. So: > > emc@xxx { > nvidia,use-ram-code; > emc-table-ram-code-0 { > nvidia,ram-code = < 0 >; > table-166 { compatible = "tegra20-emc-table"; ... }; > table-333 { ... }; > }; > > emc-table-ram-code-1 { > nvidia,ram-code = < 1 >; > ... > }; > }; > > ... and for none-ram-code, just leave out the emc-table-ramcode-x level. > > So, for nvidia,use-ram-code case, it'll be one intermediate step of > finding the right subnode, the rest of the table setup code will be > common. None of it will be bound to actual node names though -- first > step is iterating child nodes looking for nvidia,ram-code properties > to match, and second step iterates by matching compatible fields. I only suggested the well-known-named sub-nodes in order to eliminate the need for a compatible property. My inclination is that if we use compatible to distinguish the tables from anything else, there's little point having the extra level of nodes; we may as well lay it out as in your original patch, just with an explicit nvidia,ram-code property in each table (or omitted/ignored when not using it) instead of reg?
On 10/18/2011 04:01 PM, Stephen Warren wrote: > Olof Johansson wrote at Tuesday, October 18, 2011 2:54 PM: >> On Tue, Oct 18, 2011 at 11:54 AM, Stephen Warren <swarren@nvidia.com> wrote: >>> Olof Johansson wrote at Tuesday, October 18, 2011 12:43 PM: > ... >>>> Compatible is still needed, in my opinion -- otherwise there will be >>>> no way to tell if the node is there to describe emc timings or if it's >>>> some new node used to describe something else (such as SDRAM chips as >>>> mentioned above). >>> >>> Can't you go by node name; enumerate all nodes with a particular name. >>> Or define another intermediate node that will always contain tables and >>> nothing else, then just enumerate all child nodes of that node: >>> >>> emc@xxxxx { >>> emc-tables { >>> table-333@0 {}; >>> table-666@0 {}; >>> }; >>> }; >>> >>> The Tegra pinmux bindings I proposed certainly used this technique; a >>> main node with a well-known name, followed by enumeration of all child >>> nodes of that, and nobody /said/ anything about that being a bad idea. >> >> I'm not really picky on this, but I think I would rather use a >> compatible field than rely on naming. >> >> That being said, doing a two-level approach will probably make it >> easier than the flat structure I initially had. So: >> >> emc@xxx { >> nvidia,use-ram-code; >> emc-table-ram-code-0 { >> nvidia,ram-code = < 0 >; >> table-166 { compatible = "tegra20-emc-table"; ... }; >> table-333 { ... }; >> }; >> >> emc-table-ram-code-1 { >> nvidia,ram-code = < 1 >; >> ... >> }; >> }; >> >> ... and for none-ram-code, just leave out the emc-table-ramcode-x level. >> >> So, for nvidia,use-ram-code case, it'll be one intermediate step of >> finding the right subnode, the rest of the table setup code will be >> common. None of it will be bound to actual node names though -- first >> step is iterating child nodes looking for nvidia,ram-code properties >> to match, and second step iterates by matching compatible fields. > > I only suggested the well-known-named sub-nodes in order to eliminate > the need for a compatible property. > > My inclination is that if we use compatible to distinguish the tables > from anything else, there's little point having the extra level of nodes; > we may as well lay it out as in your original patch, just with an explicit > nvidia,ram-code property in each table (or omitted/ignored when not using > it) instead of reg? Node names should be generic like serial or ethernet. Compatible is used to specify the specific model. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 18, 2011 at 2:01 PM, Stephen Warren <swarren@nvidia.com> wrote: > Olof Johansson wrote at Tuesday, October 18, 2011 2:54 PM: >> On Tue, Oct 18, 2011 at 11:54 AM, Stephen Warren <swarren@nvidia.com> wrote: >> > Olof Johansson wrote at Tuesday, October 18, 2011 12:43 PM: > ... >> >> Compatible is still needed, in my opinion -- otherwise there will be >> >> no way to tell if the node is there to describe emc timings or if it's >> >> some new node used to describe something else (such as SDRAM chips as >> >> mentioned above). >> > >> > Can't you go by node name; enumerate all nodes with a particular name. >> > Or define another intermediate node that will always contain tables and >> > nothing else, then just enumerate all child nodes of that node: >> > >> > emc@xxxxx { >> > emc-tables { >> > table-333@0 {}; >> > table-666@0 {}; >> > }; >> > }; >> > >> > The Tegra pinmux bindings I proposed certainly used this technique; a >> > main node with a well-known name, followed by enumeration of all child >> > nodes of that, and nobody /said/ anything about that being a bad idea. >> >> I'm not really picky on this, but I think I would rather use a >> compatible field than rely on naming. >> >> That being said, doing a two-level approach will probably make it >> easier than the flat structure I initially had. So: >> >> emc@xxx { >> nvidia,use-ram-code; >> emc-table-ram-code-0 { >> nvidia,ram-code = < 0 >; >> table-166 { compatible = "tegra20-emc-table"; ... }; >> table-333 { ... }; >> }; >> >> emc-table-ram-code-1 { >> nvidia,ram-code = < 1 >; >> ... >> }; >> }; >> >> ... and for none-ram-code, just leave out the emc-table-ramcode-x level. >> >> So, for nvidia,use-ram-code case, it'll be one intermediate step of >> finding the right subnode, the rest of the table setup code will be >> common. None of it will be bound to actual node names though -- first >> step is iterating child nodes looking for nvidia,ram-code properties >> to match, and second step iterates by matching compatible fields. > > I only suggested the well-known-named sub-nodes in order to eliminate > the need for a compatible property. Using compatible has one more benefit: It allows the data format to change in the future if it has to, by bumping to a new compatible string. Also, since names have to be unique (due to lack of unit addressing), the well-known-name is really a well-known-name-prefix. Using compatible just seems cleaner. > My inclination is that if we use compatible to distinguish the tables > from anything else, there's little point having the extra level of nodes; > we may as well lay it out as in your original patch, just with an explicit > nvidia,ram-code property in each table (or omitted/ignored when not using > it) instead of reg? The main point of having one more level is to not have to iterate through all tables to find which ones apply to the current ram-code strapping. The cost of having another level is low, so it's not much overhead. I should revisit the pinmux patch, I suppose, and see if I agree with the conventions there. :) -Olof -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 18, 2011 at 7:37 PM, Rob Herring <robherring2@gmail.com> wrote: > On 10/18/2011 04:01 PM, Stephen Warren wrote: >> I only suggested the well-known-named sub-nodes in order to eliminate >> the need for a compatible property. >> >> My inclination is that if we use compatible to distinguish the tables >> from anything else, there's little point having the extra level of nodes; >> we may as well lay it out as in your original patch, just with an explicit >> nvidia,ram-code property in each table (or omitted/ignored when not using >> it) instead of reg? > > Node names should be generic like serial or ethernet. Compatible is used > to specify the specific model. In cases where unit addresses can be used to separate out identical entries, yes. For something like this, there's no logical addressing of the tables so something else must be used to distinguish them. -Olof -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> In cases where unit addresses can be used to separate out identical > entries, yes. For something like this, there's no logical addressing > of the tables so something else must be used to distinguish them. You can always add some dummy addressing, like is also used for cpu nodes etc. Segher -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/18/2011 10:28 PM, Olof Johansson wrote: > On Tue, Oct 18, 2011 at 7:37 PM, Rob Herring <robherring2@gmail.com> wrote: >> On 10/18/2011 04:01 PM, Stephen Warren wrote: >>> I only suggested the well-known-named sub-nodes in order to eliminate >>> the need for a compatible property. >>> >>> My inclination is that if we use compatible to distinguish the tables >>> from anything else, there's little point having the extra level of nodes; >>> we may as well lay it out as in your original patch, just with an explicit >>> nvidia,ram-code property in each table (or omitted/ignored when not using >>> it) instead of reg? >> >> Node names should be generic like serial or ethernet. Compatible is used >> to specify the specific model. > > In cases where unit addresses can be used to separate out identical > entries, yes. For something like this, there's no logical addressing > of the tables so something else must be used to distinguish them. Using the frequency as was previously proposed would work assuming that is unique. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 19, 2011 at 7:36 AM, Rob Herring <robherring2@gmail.com> wrote: > On 10/18/2011 10:28 PM, Olof Johansson wrote: >> On Tue, Oct 18, 2011 at 7:37 PM, Rob Herring <robherring2@gmail.com> wrote: >>> On 10/18/2011 04:01 PM, Stephen Warren wrote: >>>> I only suggested the well-known-named sub-nodes in order to eliminate >>>> the need for a compatible property. >>>> >>>> My inclination is that if we use compatible to distinguish the tables >>>> from anything else, there's little point having the extra level of nodes; >>>> we may as well lay it out as in your original patch, just with an explicit >>>> nvidia,ram-code property in each table (or omitted/ignored when not using >>>> it) instead of reg? >>> >>> Node names should be generic like serial or ethernet. Compatible is used >>> to specify the specific model. >> >> In cases where unit addresses can be used to separate out identical >> entries, yes. For something like this, there's no logical addressing >> of the tables so something else must be used to distinguish them. > > Using the frequency as was previously proposed would work assuming that > is unique. Someone suggested (off-list) to just use dummy addressing like cpu nodes do. Sounds reasonable to me. So: emc { compatible = "tegra20-emc"; nvidia,use-ram-code; emc-tables@<ram-code> { nvidia,ram-code = < <ram-code> >; emc-table@<dummy enumerator> { compatible = "tegra20-emc-table"; clock-frequency = < >; nvidia,emc-regs = < >; } } This also avoids having to handle 2-dimensional dummy numbering (i.e. <ramcode,table-number>) by breaking it in two levels: Where nvidia,use-ram-code is missing in the emc node, the immediate child nodes will be scanned for the compatible nodes Where nvidia,use-ram-code is present, first scan will be of all child nodes containing an nvidia,ram-code property, then from there treat it the same as the first case. In the above, none of the names have meaning, so they can be changed as needed (but these seem like a reasonably generic and descriptive name to me). -Olof -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Olof Johansson wrote at Wednesday, October 19, 2011 9:07 AM: > On Wed, Oct 19, 2011 at 7:36 AM, Rob Herring <robherring2@gmail.com> wrote: ... > > Using the frequency as was previously proposed would work assuming that > > is unique. > > Someone suggested (off-list) to just use dummy addressing like cpu > nodes do. Sounds reasonable to me. > > So: > > emc { > compatible = "tegra20-emc"; > nvidia,use-ram-code; > emc-tables@<ram-code> { > nvidia,ram-code = < <ram-code> >; > emc-table@<dummy enumerator> { > compatible = "tegra20-emc-table"; > clock-frequency = < >; > nvidia,emc-regs = < >; > } > } > > This also avoids having to handle 2-dimensional dummy numbering (i.e. > <ramcode,table-number>) by breaking it in two levels: > > Where nvidia,use-ram-code is missing in the emc node, the immediate > child nodes will be scanned for the compatible nodes > Where nvidia,use-ram-code is present, first scan will be of all child > nodes containing an nvidia,ram-code property, then from there treat it > the same as the first case. > > In the above, none of the names have meaning, so they can be changed > as needed (but these seem like a reasonably generic and descriptive > name to me). OK, I'm good with that general structure. But, as Rob suggests, you may as well use the frequency instead of the <dummy enumerator> for the unit address of the final tables, right? The search algorithm might not care, but it'll be easier for humans to read the resulting dts file.
On Wed, Oct 19, 2011 at 8:11 AM, Stephen Warren <swarren@nvidia.com> wrote: > Olof Johansson wrote at Wednesday, October 19, 2011 9:07 AM: >> On Wed, Oct 19, 2011 at 7:36 AM, Rob Herring <robherring2@gmail.com> wrote: > ... >> > Using the frequency as was previously proposed would work assuming that >> > is unique. >> >> Someone suggested (off-list) to just use dummy addressing like cpu >> nodes do. Sounds reasonable to me. >> >> So: >> >> emc { >> compatible = "tegra20-emc"; >> nvidia,use-ram-code; >> emc-tables@<ram-code> { >> nvidia,ram-code = < <ram-code> >; >> emc-table@<dummy enumerator> { >> compatible = "tegra20-emc-table"; >> clock-frequency = < >; >> nvidia,emc-regs = < >; >> } >> } >> >> This also avoids having to handle 2-dimensional dummy numbering (i.e. >> <ramcode,table-number>) by breaking it in two levels: >> >> Where nvidia,use-ram-code is missing in the emc node, the immediate >> child nodes will be scanned for the compatible nodes >> Where nvidia,use-ram-code is present, first scan will be of all child >> nodes containing an nvidia,ram-code property, then from there treat it >> the same as the first case. >> >> In the above, none of the names have meaning, so they can be changed >> as needed (but these seem like a reasonably generic and descriptive >> name to me). > > OK, I'm good with that general structure. > > But, as Rob suggests, you may as well use the frequency instead of the > <dummy enumerator> for the unit address of the final tables, right? > The search algorithm might not care, but it'll be easier for humans > to read the resulting dts file. Sure, anything can probably be used so that's fine with me -- it won't change the table walking code. -Olof -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> emc { > compatible = "tegra20-emc"; > nvidia,use-ram-code; > emc-tables@<ram-code> { > nvidia,ram-code = < <ram-code> >; > emc-table@<dummy enumerator> { > compatible = "tegra20-emc-table"; > clock-frequency = < >; > nvidia,emc-regs = < >; > } > } You need to have a reg in the emc-table node, and #address-cells #size-cells in its parent node. Segher -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Segher Boessenkool wrote at Wednesday, October 19, 2011 2:13 PM: > > emc { > > compatible = "tegra20-emc"; > > nvidia,use-ram-code; > > emc-tables@<ram-code> { > > nvidia,ram-code = < <ram-code> >; > > emc-table@<dummy enumerator> { > > compatible = "tegra20-emc-table"; > > clock-frequency = < >; > > nvidia,emc-regs = < >; > > } > > } > > You need to have a reg in the emc-table node, and #address-cells > #size-cells > in its parent node. Even if the nodes aren't addressable entities, but just groupings for configuration parameters?
On Wed, Oct 19, 2011 at 1:13 PM, Segher Boessenkool <segher@kernel.crashing.org> wrote: >> emc { >> compatible = "tegra20-emc"; >> nvidia,use-ram-code; >> emc-tables@<ram-code> { >> nvidia,ram-code = < <ram-code> >; >> emc-table@<dummy enumerator> { >> compatible = "tegra20-emc-table"; >> clock-frequency = < >; >> nvidia,emc-regs = < >; >> } >> } > > You need to have a reg in the emc-table node, and #address-cells #size-cells > in its parent node. Ah, yes, I left that out for brevity. It'll be there in the patch. (And apologies about the comment about the suggestion from you being off-list when it wasn't. -- I read it on my phone and for some reason it wasn't showing me the list of cc:s). -Olof -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/arm/tegra/emc.txt b/Documentation/devicetree/bindings/arm/tegra/emc.txt new file mode 100644 index 0000000..dd7f845 --- /dev/null +++ b/Documentation/devicetree/bindings/arm/tegra/emc.txt @@ -0,0 +1,77 @@ +Embedded Memory Controller + +Properties: +- name : Should be emc +- #address-cells : Should be 1 +- #size-cells : Should be 0 +- compatible : should contain "nvidia,tegra20-emc". +- reg : Offset and length of the register set for the device +- nvidia,use-ram-code : If present, the sub-nodes will be addressed + and chosen using the ramcode board selector. If omitted, only one + set of tables can be present and said tables will be used + irrespective of ram-code configuration. + +Child device nodes describe the memory settings for different configurations and clock rates. + +Example: + + emc@7000f400 { + #address-cells = < 1 >; + #size-cells = < 0 >; + compatible = "nvidia,tegra20-emc"; + reg = <0x7000f4000 0x200>; + } + + +Embedded Memory Controller configuration table + +This is a table containing the EMC register settings for the various +operating speeds of the memory controller. They are always located as +subnodes of the emc controller node. + +There are two ways of specifying which tables to use: + +* The simplest is if there is just one set of tables in the device tree, + and they will always be used (based on which frequency is used). + This is the preferred method, especially when firmware can fill in + this information based on the specific system information and just + pass it on to the kernel. + +* The slightly more complex one is when more than one memory configuration + might exist on the system. The Tegra20 platform handles this during + early boot by selecting one out of possible 4 memory settings based + on a 2-pin "ram code" bootstrap setting on the board. The values of + these strappings can be read through a register in the SoC, and thus + used to select which tables to use. + +Properties: +- name : Should start with emc-table +- compatible : should contain "nvidia,tegra20-emc-table". +- reg : only needed if nvidia,use-ram-code is present in the + parent. If so, the numerical representation of the selected ram code + as reported by the strap option APB misc register. +- clock-frequency : the clock frequency for the EMC at which this + table should be used (in kHz). +- nvidia,emc-registers : a 46 word array of EMC registers to be programmed + for operation at the 'clock-frequency' setting. + The order and contents of the registers are defined in the Tegra TRM. + + emc-table-333mhz@0 { + reg = <0>; + compatible = "nvidia,tegra20-emc-table"; + clock-frequency = < 333000 >; + nvidia,emc-registers = < 0 0 0 0 0 0 0 0 0 0 0 0 0 0 + 0 0 0 0 0 0 0 0 0 0 0 0 0 0 + 0 0 0 0 0 0 0 0 0 0 0 0 0 0 + 0 0 0 0 >; + }; + + emc-table-666mhz@0 { + reg = <0>; + compatible = "nvidia,tegra20-emc-table"; + clock-frequency = < 666000 >; + nvidia,emc-registers = < 0 0 0 0 0 0 0 0 0 0 0 0 0 0 + 0 0 0 0 0 0 0 0 0 0 0 0 0 0 + 0 0 0 0 0 0 0 0 0 0 0 0 0 0 + 0 0 0 0 >; + }; diff --git a/arch/arm/boot/dts/tegra-seaboard.dts b/arch/arm/boot/dts/tegra-seaboard.dts index a72299b..2d60b5ff 100644 --- a/arch/arm/boot/dts/tegra-seaboard.dts +++ b/arch/arm/boot/dts/tegra-seaboard.dts @@ -20,6 +20,42 @@ clock-frequency = < 216000000 >; }; + emc { + emc-table-166 { + compatible = "nvidia,tegra20-emc-table"; + clock-frequency = < 166500 >; + nvidia,emc-registers = < 0x0000000a 0x00000021 + 0x00000008 0x00000003 0x00000004 0x00000004 + 0x00000002 0x0000000c 0x00000003 0x00000003 + 0x00000002 0x00000001 0x00000004 0x00000005 + 0x00000004 0x00000009 0x0000000d 0x000004df + 0x00000000 0x00000003 0x00000003 0x00000003 + 0x00000003 0x00000001 0x0000000a 0x000000c8 + 0x00000003 0x00000006 0x00000004 0x0000000f + 0x00000002 0x00000000 0x00000000 0x00000002 + 0x00000000 0x00000000 0x00000083 0xa04004ae + 0x007fd010 0x00000000 0x00000000 0x00000000 + 0x00000000 0x00000000 0x00000000 0x00000000 >; + }; + + emc-table-333 { + compatible = "nvidia,tegra20-emc-table"; + clock-frequency = < 333000 >; + nvidia,emc-registers = < 0x00000014 0x00000041 + 0x0000000f 0x00000005 0x00000004 0x00000005 + 0x00000003 0x0000000c 0x00000005 0x00000005 + 0x00000003 0x00000001 0x00000004 0x00000005 + 0x00000004 0x00000009 0x0000000d 0x000009ff + 0x00000000 0x00000003 0x00000003 0x00000005 + 0x00000005 0x00000001 0x0000000f 0x000000c8 + 0x00000003 0x0000000c 0x00000006 0x0000000f + 0x00000002 0x00000000 0x00000000 0x00000002 + 0x00000000 0x00000000 0x00000083 0xe034048b + 0x007e8010 0x00000000 0x00000000 0x00000000 + 0x00000000 0x00000000 0x00000000 0x00000000 >; + }; + }; + sdhci@c8000400 { cd-gpios = <&gpio 69 0>; /* gpio PI5 */ wp-gpios = <&gpio 57 0>; /* gpio PH1 */ diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi index 65d7e6a..f4dc8d3 100644 --- a/arch/arm/boot/dts/tegra20.dtsi +++ b/arch/arm/boot/dts/tegra20.dtsi @@ -120,6 +120,13 @@ interrupts = < 123 >; }; + emc@7000f400 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "nvidia,tegra20-emc"; + reg = <0x7000f400 0x200>; + }; + sdhci@c8000000 { compatible = "nvidia,tegra20-sdhci"; reg = <0xc8000000 0x200>;
First cut at device tree bindings for the EMC tables on tegra. Note that I have a prerequisite patch that changes the tegra2_emc code to be a platform driver; but I wanted to do a sanity-check of my device tree usage here before posting the whole series. Signed-off-by: Olof Johansson <olof@lixom.net> --- .../devicetree/bindings/arm/tegra/emc.txt | 77 ++++++++++++++++++++ arch/arm/boot/dts/tegra-seaboard.dts | 36 +++++++++ arch/arm/boot/dts/tegra20.dtsi | 7 ++ 3 files changed, 120 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/arm/tegra/emc.txt